Skip to content

fix: don't treat chunks emptied by an overwrite as removed in replace - #367

Merged
antfu merged 2 commits into
Rich-Harris:masterfrom
theRizwan:fix-replace-overwritten-range
Oct 26, 2026
Merged

antfu merged 2 commits into
Rich-Harris:masterfrom
theRizwan:fix-replace-overwritten-range

Conversation

@theRizwan

Copy link
Copy Markdown
Contributor

Since #352, replace() and replaceAll() skip a match that lands on removed content, and _hasRemovedContent() decides that by looking for an empty chunk in the matched range. update() and overwrite() leave empty chunks too: when the range spans several chunks, the replacement goes on the first one and the others are emptied. Their text was replaced, not removed, but a match over them is skipped all the same. So whether a match is replaced depends on whether some earlier edit happened to split the range, which the caller cannot see.

Reproductions

All checked against current master ([email protected]).

The only difference between these two is a split that inserts nothing:

const a = new MagicString('aa')
a.update(0, 2, 'Q')
a.replaceAll(/a+/g, 'P')
a.toString() // 'P'

const b = new MagicString('aa')
b.appendLeft(1, '')
b.update(0, 2, 'Q')
b.replaceAll(/a+/g, 'P')
b.toString() // 'Q', expected 'P'

The same happens with replace(), with a string pattern, and with overwrite() in place of update(). Any earlier edit that splits the range has this effect, not only an empty insert:

new MagicString('abcd').remove(0, 1).update(0, 2, 'Q').replace('ab', 'P').toString()
// 'Qcd', expected 'Pcd': 'ab' was overwritten after the removal, not removed

A range that an earlier replaceAll() overwrote cannot be matched again either:

const s = new MagicString('aa')
s.appendLeft(1, '')
s.replaceAll(/a+/g, 'P')
s.replaceAll(/a+/g, 'Q').toString() // 'P', and 'Q' without the split

The fix

update() now records on the chunks it empties where the overwritten range starts, and on the first chunk where the range ends (replacedFrom and replacesUntil on Chunk). remove() and reset() clear both, a split keeps replacedFrom on both halves, and clone() copies them. _hasRemovedContent() treats an emptied chunk as present when the match includes the first chunk of that overwrite and that chunk's replacement still reaches it. Removed chunks, trimmed ones and ranges overwritten with '' are unaffected.

Both conditions matter:

  • A match that leaves the first chunk out is still skipped. After update(0, 3, 'Q') on 'abc' split at 1, replace('bc', 'P') still gives 'Q'. Overwriting only the emptied chunks would leave 'Q' in place and add 'P' after it. Without the split, the same call throws cannot split a chunk that has already been edited.
  • After such a split, the first chunk can be edited again on its own, and its replacement then no longer stands for the rest of the range. In the same example, update(0, 1, 'R') or reset(0, 1) followed by replace('abc', 'P') still gives 'R' or 'a'. Recording only where the range starts would bring those chunks back. With an empty replacement it would even make the result depend on the split again, since update(0, 2, '') followed by update(0, 1, 'Y') can be reached with or without it.

One consequence to be aware of: a match that includes the first chunk but ends inside the range is now replaced, and the rest of the range loses its replacement, as it does when overwrite() is called on that part directly. Master already did this when the match ended exactly where the first chunk ends, and skipped the match when it ended further in. So on 'abc', update(0, 3, 'Q') then replace('ab', 'P') gives 'P' with a split at 2 and 'Q' with a split at 1 (and throws with none). This branch gives 'P' for both. Skipping both would also be consistent, but it changes far more existing results (2,588 against 144 in the comparison below), because the first case is the common one.

Evidence

The same metamorphic fuzz as in #361: random sequences of edits on random strings, each run with and without a few appendLeft(k, '') splits, comparing toString(), slice(), lastChar(), lastLine(), isEmpty(), hasChanged() and length(). Divergences from replace()/replaceAll():

run master this branch
200,000 trials, every edit method plus replace/replaceAll 4 0
200,000 trials, inserts, update, overwrite, remove, replace, replaceAll 27 0

With #361 merged, these are the only divergences left on master in both runs.

The fuzz never uses an empty replacement and never trims one away, so I also checked exhaustively on 'abcde': every sequence of one or two edits (remove, update with content, update with '', reset) over every range, under every placement of zero, one or two empty inserts, followed by replace/replaceAll of every substring, as a string and as a RegExp, 1,811,700 cases in all. Each result is compared with the one without the inserts, and each skip decision with a per-character model of which text is still represented in the output (the model's rendering matched toString() in every case):

master this branch
cases where nothing throws 1,650,048 1,649,452
result depends on the empty inserts 18,100 0
skip decision differs from the model 64,716 0

The 596 cases that now throw are matches that master skipped and that also end inside another overwritten chunk, so overwriting them throws cannot split a chunk that has already been edited. The same calls throw on master as well when the range is not split.

A second exhaustive check runs trim(), trimStart(), trimEnd() or trimLines() between the edits and the replace, with whitespace replacements that the trim can empty: 228,830 of 6,436,950 comparisons depend on the inserts on master, 0 on this branch.

On the same random trials, 145 of 355,590 runs give a different result than on master. 142 of them are matches that master skipped and this branch replaces, as described above. In one trial (2 runs) a later replaceAll('a', 'P') now throws cannot split a chunk that has already been edited, because it falls inside a range that a partial match before it now replaces. One run no longer throws.

3 tests added: two fail on master, and the third pins down the matches that must still be skipped. The 454 existing tests pass unchanged. Lint is clean, and pnpm run typecheck reports the same TS2341 errors as on master.

This touches _hasRemovedContent() like #366, which fixes empty matches at a chunk boundary inside removed content. The two only conflict in the test file, where both add a test at the end of the same block. I checked the combination: all 460 tests pass, the fuzz runs above show no divergences, and both exhaustive checks (and the one from #366) report 0.

theRizwan and others added 2 commits October 20, 2026 22:02
`update()` and `overwrite()` over a range that spans several chunks store
the replacement on the first chunk and empty the others.
`_hasRemovedContent()` took those for removed content, so `replace()` and
`replaceAll()` skipped a match over the range whenever an earlier edit
had split it, even an insert of nothing.

The first chunk now records where the overwritten range ends, and the
others where it starts. An emptied chunk counts as present while the
match includes the first chunk and that chunk's replacement still
reaches it. A match that leaves the first chunk out is still skipped, so
the replacement is never doubled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants