String.prototype.replace() regressed due to r252683: <https://trac.webkit.org/r252683> for webkit.org/b/202471. The patch failed to handle InternalFunctions. This patch also fixed a spec compliance bug for String.prototype.replace() i.e. the replaceValue needs to be evaluated before we check if there's a match in the source string. See 21.1.3.14-9 at https://www.ecma-international.org/ecma-262/6.0/#sec-string.prototype.replace. For String.prototype.replaceAll(), make sure it "behaves just like String.prototype.replace if searchValue is a global regular expression". See https://github.com/tc39/proposal-string-replaceall. <rdar://problem/57354854>
Created attachment 384104 [details] proposed patch.
Comment on attachment 384104 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=384104&action=review > JSTests/stress/string-replaceAll-2.js:2 > +const supportsReplaceAll = (String.prototype.replaceAll != undefined); FYI, I added this so that I can run this test on Chrome and Firefox as well to validate the test results.
Comment on attachment 384104 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=384104&action=review It's a bit interesting that these tests fail silently in release, but I guess that's the sort of issue it is. > Source/JavaScriptCore/ChangeLog:10 > + String.prototype.replace() regressed due to r252683: <https://trac.webkit.org/r252683> > + for webkit.org/b/202471. The patch failed to handle InternalFunctions. Ugh, it seems that I underestimated the reason for the two huge branches in replaceUsingRegExpSearch. Sorry about that. :( > Source/JavaScriptCore/ChangeLog:15 > + This patch also fixed a spec compliance bug for String.prototype.replace() i.e. > + the replaceValue needs to be evaluated before we check if there's a match in the > + source string. > + Ref: 21.1.3.14-9 at https://www.ecma-international.org/ecma-262/6.0/#sec-string.prototype.replace Oh, good catch! I guess there haven't been any tests for this. BTW, this is now 21.1.3.17-6 in the living spec: https://tc39.es/ecma262/#sec-string.prototype.replace
Comment on attachment 384104 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=384104&action=review r=me too > Source/JavaScriptCore/ChangeLog:31 > + Ref: https://bugs.webkit.org/show_bug.cgi?id=204481 Maybe, due to the reason why we have two functions, we should split it. String#replace is one of the most frequently used function.
Comment on attachment 384104 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=384104&action=review >> Source/JavaScriptCore/ChangeLog:10 >> + for webkit.org/b/202471. The patch failed to handle InternalFunctions. > > Ugh, it seems that I underestimated the reason for the two huge branches in replaceUsingRegExpSearch. Sorry about that. :( The test that I just added will catch this from now on. >> Source/JavaScriptCore/ChangeLog:15 >> + Ref: 21.1.3.14-9 at https://www.ecma-international.org/ecma-262/6.0/#sec-string.prototype.replace > > Oh, good catch! I guess there haven't been any tests for this. > > BTW, this is now 21.1.3.17-6 in the living spec: https://tc39.es/ecma262/#sec-string.prototype.replace Is the living spec constantly changing? I'll stay with ecma-262/6.0 so that this ref doesn't go stale when the living spec changes again. I think it's reasonable for the reader to look up a newer version if he/she wants it. >> Source/JavaScriptCore/ChangeLog:31 >> + Ref: https://bugs.webkit.org/show_bug.cgi?id=204481 > > Maybe, due to the reason why we have two functions, we should split it. String#replace is one of the most frequently used function. I'll see what the microbenchmark says.
Created attachment 384116 [details] patch for landing (includes rebased ChakraCore test results).
Landed in r252758: <http://trac.webkit.org/r252758>.