Changeset 252836 in webkit

Timestamp:

Nov 23, 2019 3:23:31 PM (4 years ago)

Author:

Ross Kirsling

Message:

[JSC] GetSubstitution is performed incorrectly via RegExp.prototype[@@replace]
​https://bugs.webkit.org/show_bug.cgi?id=204490

Reviewed by Mark Lam.

JSTests:

• stress/replace-indexed-backreferences.js: Added.
• test262/expectations.yaml: Mark four test cases as passing.

Source/JavaScriptCore:

String.prototype.replace and RegExp.prototype[Symbol.replace] are meant to perform the same substitution
of $-backreferences (called GetSubstitution in the spec: ​https://tc39.es/ecma262/#sec-getsubstitution).

The implementation of this in StringPrototype.cpp is correct but the one in RegExpPrototype.js is not.
In particular, the latter *removes* backreferences with out-of-range indices, instead of leaving them as-is.

One thing that is *not* broken in either implementation and thus maintained here is the fact $10 is interpreted
as $1 followed by a 0 when we have 1 <= n < 10 captures (and analogously for other invalid $nn backreferences).
This behavior is consistent across all engines but currently described incorrectly in the spec; this patch thus
aligns with the spec PR currently open to correct this (​https://github.com/tc39/ecma262/pull/1732).

• builtins/RegExpPrototype.js:

(getSubstitution): Ensure that invalid backreferences remain untouched in the output string.
(replace): Fix off-by-one error when populating captures list. We shouldn't be reserving a slot for the full match.

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/replace-indexed-backreferences.js (added)
• JSTests/test262/expectations.yaml (modified) (1 diff)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/builtins/RegExpPrototype.js (modified) (4 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2019-11-23  Ross Kirsling  <ross.kirsling@sony.com>
+
+        [JSC] GetSubstitution is performed incorrectly via RegExp.prototype[@@replace]
+        https://bugs.webkit.org/show_bug.cgi?id=204490
+
+        Reviewed by Mark Lam.
+
+        * stress/replace-indexed-backreferences.js: Added.
+        * test262/expectations.yaml: Mark four test cases as passing.
+
 2019-11-22  Tadeu Zagallo  <tzagallo@apple.com>
 

trunk/JSTests/test262/expectations.yaml

@@ -1507 +1507 @@
   default: 'Test262Error: Expected SameValue(«», «foo[toString value]bar») to be true'
   strict mode: 'Test262Error: Expected SameValue(«», «foo[toString value]bar») to be true'
-test/built-ins/RegExp/prototype/Symbol.replace/subst-capture-idx-1.js:
-  default: 'Test262Error: Expected SameValue(«a[cd]e», «a[cd$4$0]e») to be true'
-  strict mode: 'Test262Error: Expected SameValue(«a[cd]e», «a[cd$4$0]e») to be true'
-test/built-ins/RegExp/prototype/Symbol.replace/subst-capture-idx-2.js:
-  default: 'Test262Error: Expected SameValue(«a[cd]e», «a[cd$04$00]e») to be true'
-  strict mode: 'Test262Error: Expected SameValue(«a[cd]e», «a[cd$04$00]e») to be true'
 test/built-ins/RegExp/prototype/Symbol.search/u-lastindex-advance.js:
   default: 'Test262Error: Expected SameValue(«1», «-1») to be true'

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-11-23  Ross Kirsling  <ross.kirsling@sony.com>
+
+        [JSC] GetSubstitution is performed incorrectly via RegExp.prototype[@@replace]
+        https://bugs.webkit.org/show_bug.cgi?id=204490
+
+        Reviewed by Mark Lam.
+
+        String.prototype.replace and RegExp.prototype[Symbol.replace] are meant to perform the same substitution
+        of $-backreferences (called GetSubstitution in the spec: https://tc39.es/ecma262/#sec-getsubstitution).
+
+        The implementation of this in StringPrototype.cpp is correct but the one in RegExpPrototype.js is not.
+        In particular, the latter *removes* backreferences with out-of-range indices, instead of leaving them as-is.
+
+        One thing that is *not* broken in either implementation and thus maintained here is the fact $10 is interpreted
+        as $1 followed by a 0 when we have 1 <= n < 10 captures (and analogously for other invalid $nn backreferences).
+        This behavior is consistent across all engines but currently described incorrectly in the spec; this patch thus
+        aligns with the spec PR currently open to correct this (https://github.com/tc39/ecma262/pull/1732).
+
+        * builtins/RegExpPrototype.js:
+        (getSubstitution): Ensure that invalid backreferences remain untouched in the output string.
+        (replace): Fix off-by-one error when populating captures list. We shouldn't be reserving a slot for the full match.
+
 2019-11-22  Saam Barati  <sbarati@apple.com>
 

trunk/Source/JavaScriptCore/builtins/RegExpPrototype.js

@@ -223 +223 @@
                     let chCode = ch.charCodeAt(0);
                     if (chCode >= 0x30 && chCode <= 0x39) {
+                        let originalStart = start - 1;
                         start++;
+
                         let n = chCode - 0x30;
-                        if (n > m)
+                        if (n > m) {
+                            result = result + replacement.substring(originalStart, start);
                             break;
+                        }
+
                         if (start < replacementLength) {
                             let nextChCode = replacement.charCodeAt(start);
@@ -238 +243 @@
                         }
 
-                        if (n == 0)
+                        if (n == 0) {
+                            result = result + replacement.substring(originalStart, start);
                             break;
-
-                        if (captures[n] != @undefined)
-                            result = result + captures[n];
+                        }
+
+                        let capture = captures[n - 1];
+                        if (capture !== @undefined)
+                            result = result + capture;
                     } else
                         result = result + "$";
@@ -314 +322 @@
             if (capN !== @undefined)
                 capN = @toString(capN);
-            captures[n] = capN;
+            captures.@push(capN);
         }
 
@@ -320 +328 @@
 
         if (functionalReplace) {
-            let replacerArgs = [ matched ].concat(captures.slice(1));
+            let replacerArgs = [ matched ].concat(captures);
             replacerArgs.@push(position);
             replacerArgs.@push(str);