fix(ecmascript): support $ substitution patterns in String.prototype.replace/replaceAll - #991
Open
chiliec wants to merge 1 commit into
Open
Conversation
…replace/replaceAll String.prototype.replace and replaceAll with a non-callable replaceValue did a plain substring replacement, ignoring the $$, $&, $`, $' and $n replacement patterns required by GetSubstitution (22.1.3.19.1). Route the string-replacement paths through get_substitution, and fix two latent bugs in get_substitution itself: the fall-through branch consumed the whole remaining template at once (so patterns after a literal prefix were never substituted), and the $' branch mixed a UTF-16 position with UTF-8 byte lengths, panicking on non-ASCII input.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Closes #938.
String.prototype.replaceandString.prototype.replaceAllwith a non-callablereplaceValueignored the$replacement patterns. They did a plain substring replacement, so"abc".replace("b", "[$&]")returneda[$&]cinstead ofa[b]c.Fix
Route the string-replacement paths (functionalReplace = false) through the existing
get_substitution(GetSubstitution, spec 22.1.3.19.1), which handles$$,$&,$`,$'and$n.Doing that surfaced two latent bugs in
get_substitutionitself, both fixed here:$pattern) setrefto the entire remaining template and consumed it in one step, so any$pattern occurring after a literal prefix (e.g.[$&],a($1$1)) was never substituted. It now advances one code point at a time.$'branch computedtailPos = position + matchLengthusing the UTF-16positionbut then indexed the UTF-8str, panicking on non-ASCII input ('Ninguém'.replaceAll('é', "($'")). It now uses the UTF-8 offset consistently, matching the sibling$`branch.get_substitutionis pure (no regex), so its#[cfg(feature = "regexp")]gate is removed andget/Scopedmoved to the always-available imports.Tests
test262. This flips 17 tests from FAIL→PASS with no regressions across the entire
built-ins/Stringandbuilt-ins/RegExptrees:String/prototype/replaceAll/getSubstitution-0x0024*(5)String/prototype/replace/regexp-capture-by-indexRegExp/prototype/Symbol.replace/{subst-after,subst-before,subst-matched,subst-capture-idx-1,subst-capture-idx-2,result-coerce-*}(9)RegExp/S15.10.2.8_A3_T18,RegExp/named-groups/string-replace-nocapturesThe RegExp Symbol.replace tests flip because they share the same
get_substitution.Validation (real commands, real results)
expectations.jsonandmetrics.jsonare updated to reflect the newly-passing tests. I ran thebuilt-ins/Stringandbuilt-ins/RegExpsubtrees rather than the whole suite locally (host RAM limits);metrics.jsontotals were adjusted by the exact +17 pass / −17 fail delta — the full-suite metrics are regenerated by CI anyway.Happy to adjust anything.