fix: handle empty matches in replace and replaceAll (#335)

Author: theRizwan

README.md

@@ -239,6 +239,8 @@ The differences from [`String.replace`](<(https://developer.mozilla.org/en-US/do
 
 - It will always match against the **original string**
 - It mutates the magic string state (use `.clone()` to be immutable)
+- A zero-length match spans no characters, so there is nothing to overwrite - the
+  substitution is inserted at the matched position, as if by `s.appendRight(index, substitution)`
 
 ### s.replaceAll( regexpOrString, substitution )
 

src/MagicString.ts

@@ -1222,35 +1222,40 @@ export default class MagicString {
         return replacement(match[0], ...match.slice(1), match.index, str, match.groups)
       }
     }
-    function matchAll(re: RegExp, str: string): RegExpExecArray[] {
-      const matches = []
-      while (true) {
-        const match = re.exec(str)
-        if (!match)
-          break
-
-        matches.push(match)
+    const replaceMatch = (match: RegExpMatchArray): void => {
+      if (match.index == null)
+        return
+
+      const replacement = getReplacement(match, this.original)
+      if (replacement === match[0])
+        return
+
+      if (match[0].length === 0) {
+        // a zero-length match spans no characters, so there is no range to
+        // overwrite - the replacement is an insertion at the matched position,
+        // which is what `String.prototype.replace` does for an empty match
+        this.appendRight(match.index, replacement)
+      }
+      else {
+        this.overwrite(match.index, match.index + match[0].length, replacement)
       }
-      return matches
     }
+
     if (searchValue.global) {
-      const matches = matchAll(searchValue, this.original)
-      matches.forEach((match) => {
-        if (match.index != null) {
-          const replacement = getReplacement(match, this.original)
-          if (replacement !== match[0]) {
-            this.overwrite(match.index, match.index + match[0].length, replacement)
-          }
-        }
-      })
+      // `String.prototype.replace` starts a global regexp from the beginning of
+      // the string, so reset `lastIndex` - a regexp that has already been used
+      // would otherwise resume from wherever it stopped and skip earlier matches.
+      // `matchAll` also steps over a zero-length match, where `exec` in a loop
+      // would keep rematching it at an unmoving `lastIndex` and never terminate.
+      searchValue.lastIndex = 0
+      for (const match of this.original.matchAll(searchValue)) {
+        replaceMatch(match)
+      }
     }
     else {
       const match = this.original.match(searchValue)
-      if (match && match.index != null) {
-        const replacement = getReplacement(match, this.original)
-        if (replacement !== match[0]) {
-          this.overwrite(match.index, match.index + match[0].length, replacement)
-        }
+      if (match) {
+        replaceMatch(match)
       }
     }
     return this
@@ -1266,7 +1271,15 @@ export default class MagicString {
         replacement = replacement(string, index, original)
       }
       if (string !== replacement) {
-        this.overwrite(index, index + string.length, replacement)
+        if (string.length === 0) {
+          // an empty search string matches the empty range at the start of the
+          // string, which has no characters to overwrite - the replacement is an
+          // insertion there, as it is for `String.prototype.replace`
+          this.appendRight(index, replacement)
+        }
+        else {
+          this.overwrite(index, index + string.length, replacement)
+        }
       }
     }
 
@@ -1288,6 +1301,21 @@ export default class MagicString {
   _replaceAllString(string: string, replacement: string | ReplacementFunction): this {
     const { original } = this
     const stringLength = string.length
+
+    // an empty search string matches the empty range before every character plus
+    // one at the end, and `indexOf` clamps its start index to the string length,
+    // so it can neither find those ranges nor ever report -1 - step through them
+    if (stringLength === 0) {
+      for (let index = 0; index <= original.length; index += 1) {
+        const _replacement
+          = typeof replacement === 'function' ? replacement('', index, original) : replacement
+        if (_replacement !== '')
+          this.appendRight(index, _replacement)
+      }
+
+      return this
+    }
+
     for (
       let index = original.indexOf(string);
       index !== -1;

test/MagicString.test.ts

@@ -2121,6 +2121,27 @@ describe('magicString', () => {
 
       assert.strictEqual(s.firstChunk, s.lastChunk)
     })
+
+    it('should insert at a zero-length match instead of overwriting nothing', () => {
+      // an empty match spans no characters, so there is no range to overwrite -
+      // `String.prototype.replace` inserts at the matched position
+      assert.strictEqual(new MagicString('abc').replace(/x?/, 'Y').toString(), 'Yabc')
+      assert.strictEqual(new MagicString('abc').replace('', 'X').toString(), 'Xabc')
+    })
+
+    it('should terminate on a global regexp that matches the empty string', () => {
+      assert.strictEqual(new MagicString('bab').replace(/a*/g, 'X').toString(), 'XbXXbX')
+      assert.strictEqual(new MagicString('a b').replace(/\s*/g, '_').toString(), '_a__b_')
+      assert.strictEqual(new MagicString('axb').replace(/x?/g, 'Y').toString(), 'YaYYbY')
+    })
+
+    it('should start a global regexp from the beginning of the string', () => {
+      const re = /o/g
+      re.exec('foo') // leaves lastIndex at 2
+
+      assert.strictEqual(new MagicString('foo').replace(re, 'X').toString(), 'fXX')
+      assert.strictEqual(re.lastIndex, 0)
+    })
   })
 
   describe('replaceAll', () => {
@@ -2202,5 +2223,52 @@ describe('magicString', () => {
       assert.strictEqual(s1.slice(), 'ello world')
       assert.equal(s1.move(0, 1, 2).slice(0), 'elo world')
     })
+
+    it('should insert at every zero-length match', () => {
+      assert.strictEqual(
+        new MagicString('a\nb\nc').replaceAll(/^/gm, '// ').toString(),
+        '// a\n// b\n// c',
+      )
+      assert.strictEqual(new MagicString('a\nb').replaceAll(/$/gm, ';').toString(), 'a;\nb;')
+      assert.strictEqual(new MagicString('ab cd').replaceAll(/\b/g, '|').toString(), '|ab| |cd|')
+      assert.strictEqual(new MagicString('abc').replaceAll(/x*/g, '-').toString(), '-a-b-c-')
+      assert.strictEqual(new MagicString('abc').replaceAll('', '-').toString(), '-a-b-c-')
+    })
+
+    it('should step over a whole code point for a unicode-aware regexp', () => {
+      // without the `u` flag the surrogate halves are matched between, as they are
+      // by `String.prototype.replaceAll`
+      const emoji = '\u{1F600}'
+
+      assert.strictEqual(
+        new MagicString(`a${emoji}b`).replaceAll(/x*/gu, '.').toString(),
+        `.a.${emoji}.b.`,
+      )
+      assert.strictEqual(
+        new MagicString(`a${emoji}b`).replaceAll(/x*/g, '.').toString(),
+        `.a.${emoji[0]}.${emoji[1]}.b.`,
+      )
+    })
+
+    it('should report the index of every empty-string match to a replacer', () => {
+      const indexes: number[] = []
+      const s = new MagicString('ab').replaceAll('', (_match, index) => {
+        indexes.push(index)
+        return `<${index}>`
+      })
+
+      assert.strictEqual(s.toString(), '<0>a<1>b<2>')
+      assert.deepEqual(indexes, [0, 1, 2])
+    })
+
+    it('should leave the original alone when an empty match is replaced by itself', () => {
+      const s = new MagicString('abc')
+
+      s.replaceAll(/x*/g, '')
+      s.replaceAll('', '')
+
+      assert.strictEqual(s.toString(), 'abc')
+      assert.strictEqual(s.hasChanged(), false)
+    })
   })
 })