Normative: Update GetSubstitution to match reality #1732

gibson042 · 2019-10-09T01:54:47Z

THIS PR HAS BEEN REPLACED BY #3157

$nn patterns fall back to $n when there aren't at least nn captures

Preview: https://deploy-preview-1732--ecma262-snapshots.netlify.com/#sec-getsubstitution

spec.html

jmdyck · 2019-10-09T13:38:18Z

spec.html

-                        1. If _capture_ is *undefined*, replace the text through `>` with the empty string.
-                        1. Otherwise, replace the text through `>` with ? ToString(_capture_).
-                  </emu-alg>
+                  If _nn_ is `00` or the MV of _nn_ &gt; _m_, the replacement is the string-concatenation of the replacement for the first two matched code units (i.e., a replacement specified by one of the two preceding rows) and the remaining matched code units (i.e., a single |DecimalDigit|).


Suggested change

If _nn_ is `00` or the MV of _nn_ > _m_, the replacement is the string-concatenation of the replacement for the first two matched code units (i.e., a replacement specified by one of the two preceding rows) and the remaining matched code units (i.e., a single |DecimalDigit|).

If _nn_ is `00` or the MV of _nn_ > _m_, the replacement is the string-concatenation of the replacement for the first two matched code units (i.e., a replacement specified by one of the two preceding rows) and the remaining matched code unit (i.e., a single |DecimalDigit|).

("units" -> "unit")

I want to leave this, because the intent is to split the matched code units into two subsequences, replacing the first and preserving the second (which happens to be a single code unit, but is not required to be so for the logic to work).

spec.html

jmdyck · 2019-10-09T13:50:45Z

spec.html

+            1. Let _k_ be 0.
+            1. Repeat, while _k_ &lt; _replacementLength_
+              1. Let _replaceablePattern_ be the longest sequence of consecutive code units from _replacement_ beginning with the code unit at index _k_ such that _replaceablePattern_ matches the &ldquo;Code units&rdquo; column of a row in <emu-xref href="#table-45"></emu-xref>.
+              1. If _replaceablePattern_ is not empty,


Suggested change

1. If _replaceablePattern_ is not empty,

1. If _replaceablePattern_ is not empty, then

Also, this condition assumes that _replaceablePattern_ is empty if there's no match, but there's nothing that actually makes it so. If might be better to say If no such match is found, then (and swap the two arms).

The logic itself makes it so. "The longest sequence of consecutive code units from replacement beginning with the code unit at index k…" is either zero code units or more than zero code units. If it is more than zero, then replaceablePattern is not empty and we will append the corresponding replacement text to result. Otherwise, we will append the code unit at index k.

"The longest sequence of consecutive code units from replacement beginning with the code unit at index k…" is either zero code units or more than zero code units.

Based on that quote, it could be, but the part you elided is such that _replaceablePattern_ matches the "Code units" column of a row in [Table 53], and an empty sequence of code units does not match the "Code units" column of any row in the table.

jmdyck · 2019-10-09T14:53:39Z

spec.html

+                  If the MV of _n_ &gt; _m_, the replacement is the matched code units (equivalent to no replacement).
+                  <br>
+                  Otherwise, if the element in _captures_ at index equal to the MV of _n_ minus 1 is *undefined*, the replacement is the empty String.
+                  <br>
+                  Otherwise, the replacement is the element in _captures_ at index equal to the MV of _n_ minus 1.


If you were to use an <emu-alg>, that would allow (something like) Let _nmv_ be the MV of _n_, which you could then use at 3 points.

It would also let you replace the element in _captures_ at index equal to the MV of _n_ minus 1 with _captures_[_nmv_ - 1].

For actual names, I think I'd recommend _N_ for the code unit (which matches the variable in column 1), and then _n_ for its MV.

I considered <emu-alg>, but ultimately preferred the appearance without it: https://deploy-preview-1732--ecma262-snapshots.netlify.com/#table-45 (and I'd get rid of the <emu-alg> for $<…> if I could find a good way to do so). As always, though, I'm willing to change if there's consensus.

As for N vs. n, we must use the latter because the former is not defined as the expansion of a lexical grammar production and thus has no MV.

As for N vs. n, we must use the latter because the former is not defined as the expansion of a lexical grammar production and thus has no MV.

Well, of course, you'd need to change the name in column 2 as well. Sorry that wasn't clear. Anyhow, the point there was to free up the name _n_ for the MV of _N_, but that isn't needed if you don't convert to <emu-alg>.

jmdyck · 2019-10-09T23:20:49Z

spec.html

+                  <br>
+                  Otherwise, if the element in _captures_ at index equal to the MV of _nn_ minus 1 is *undefined*, the replacement is the empty String.
+                  <br>
+                  Otherwise, the replacement is the element in _captures_ at index equal to the MV of _nn_ minus 1.


Ditto above comment re using <emu-alg>.

gibson042

Thanks for the review, @jmdyck. I've updated the PR with some of your suggestions and added explanations for why I didn't accept the others.

gibson042 · 2019-10-10T01:13:33Z

spec.html

+            1. Let _k_ be 0.
+            1. Repeat, while _k_ &lt; _replacementLength_
+              1. Let _replaceablePattern_ be the longest sequence of consecutive code units from _replacement_ beginning with the code unit at index _k_ such that _replaceablePattern_ matches the &ldquo;Code units&rdquo; column of a row in <emu-xref href="#table-45"></emu-xref>.
+              1. If _replaceablePattern_ is not empty,


The logic itself makes it so. "The longest sequence of consecutive code units from replacement beginning with the code unit at index k…" is either zero code units or more than zero code units. If it is more than zero, then replaceablePattern is not empty and we will append the corresponding replacement text to result. Otherwise, we will append the code unit at index k.

gibson042 · 2019-10-10T01:17:29Z

spec.html

-                        1. If _capture_ is *undefined*, replace the text through `>` with the empty string.
-                        1. Otherwise, replace the text through `>` with ? ToString(_capture_).
-                  </emu-alg>
+                  If _nn_ is `00` or the MV of _nn_ &gt; _m_, the replacement is the string-concatenation of the replacement for the first two matched code units (i.e., a replacement specified by one of the two preceding rows) and the remaining matched code units (i.e., a single |DecimalDigit|).


I want to leave this, because the intent is to split the matched code units into two subsequences, replacing the first and preserving the second (which happens to be a single code unit, but is not required to be so for the logic to work).

gibson042 · 2019-10-10T01:22:28Z

spec.html

+                  If the MV of _n_ &gt; _m_, the replacement is the matched code units (equivalent to no replacement).
+                  <br>
+                  Otherwise, if the element in _captures_ at index equal to the MV of _n_ minus 1 is *undefined*, the replacement is the empty String.
+                  <br>
+                  Otherwise, the replacement is the element in _captures_ at index equal to the MV of _n_ minus 1.


I considered <emu-alg>, but ultimately preferred the appearance without it: https://deploy-preview-1732--ecma262-snapshots.netlify.com/#table-45 (and I'd get rid of the <emu-alg> for $<…> if I could find a good way to do so). As always, though, I'm willing to change if there's consensus.

As for N vs. n, we must use the latter because the former is not defined as the expansion of a lexical grammar production and thus has no MV.

spec.html

$nn patterns fall back to $n when there aren't at least nn captures Fixes tc39gh-1426

spec.html

Ref tc39gh-1780

…replace

…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 (tc39/ecma262#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. git-svn-id: http://svn.webkit.org/repository/webkit/trunk@252836 268f45cc-cd09-0410-ab3c-d52691b4dbfc

littledan

This change looks editorially correct to me (and I'm in favor of it on that basis), ~~but I don't understand why it's normative. I thought the no-substitution case was covered by clauses like, "If n > m, no replacement is done."~~ Edit: And seems like a critical fix, given how confusing my previous wording was. LGTM!

devsnek · 2020-08-16T23:53:07Z

is this still a thing?

ljharb · 2020-08-17T01:29:39Z

I believe so - it needs test262 tests, a rebase, and an updated review.

gibson042 · 2020-08-18T20:58:42Z

I'm planning to rebase and update this PR after #2021 lands.

michaelficarra · 2020-10-12T21:48:58Z

@gibson042 #2021 has landed, do you want to rebase this?

gibson042 · 2020-10-13T16:17:00Z

Yep, it should be done this week.

…replace

…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 (tc39/ecma262#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. Canonical link: https://commits.webkit.org/217811@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@252836 268f45cc-cd09-0410-ab3c-d52691b4dbfc

bakkot · 2021-08-17T01:34:38Z

I think this already has tests, actually, or at least one test, as pointed out in the original issue.

gibson042 · 2023-08-29T14:48:46Z

Replaced by #3157.

gibson042 added normative change Affects behavior required to correctly evaluate some ECMAScript source text needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 web reality labels Oct 9, 2019

ljharb reviewed Oct 9, 2019

View reviewed changes

spec.html Outdated Show resolved Hide resolved

ljharb requested review from mathiasbynens, waldemarhorwat and littledan October 9, 2019 02:29

jmdyck reviewed Oct 9, 2019

View reviewed changes

gibson042 commented Oct 10, 2019

View reviewed changes

shvaikalesh mentioned this pull request Nov 13, 2019

Editorial: Move ToObject out of GetSubstitution #1580

Merged

bakkot reviewed Nov 13, 2019

View reviewed changes

spec.html Outdated Show resolved Hide resolved

gibson042 and others added 4 commits November 13, 2019 16:19

Normative: Update GetSubstitution to match reality

accf7f5

$nn patterns fall back to $n when there aren't at least nn captures Fixes tc39gh-1426

Editorial: Remove extra space

fdd9272

Editorial: Apply suggestions from code review

19b2af1

Normative: Account for abrupt completion of named capture replacement

e5941f0

gibson042 force-pushed the gh-1426-GetSubstitution-dollar-nn branch from 87592e6 to e5941f0 Compare November 13, 2019 21:20

bakkot reviewed Nov 13, 2019

View reviewed changes

spec.html Show resolved Hide resolved

gibson042 added 2 commits November 13, 2019 19:07

Editorial: Be explicit when the "longest sequence" is empty

603f835

Editorial: Replace "the empty string" with "the empty String"

b66bbe3

Ref tc39gh-1780

Editorial: Improve table anchor

1ab6c46

littledan reviewed Mar 5, 2020

View reviewed changes

littledan approved these changes Mar 5, 2020

View reviewed changes

ljharb requested review from syg, michaelficarra, a team and bakkot March 5, 2020 21:15

ljharb force-pushed the master branch 3 times, most recently from 3d0c24c to 7a79833 Compare June 29, 2021 02:21

jmdyck mentioned this pull request Aug 17, 2021

Editorial: Incorporate GetSubstitution's table into the algorithm #2484

Merged

jmdyck mentioned this pull request Apr 14, 2022

Editorial: Simplify GetSubstitution #2739

Merged

gibson042 marked this pull request as draft April 14, 2022 18:55

gibson042 closed this Aug 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normative: Update GetSubstitution to match reality #1732

Normative: Update GetSubstitution to match reality #1732

gibson042 commented Oct 9, 2019 •

edited

Loading

jmdyck Oct 9, 2019

gibson042 Oct 10, 2019

jmdyck Oct 9, 2019

gibson042 Oct 10, 2019

jmdyck Oct 10, 2019

jmdyck Oct 9, 2019

gibson042 Oct 10, 2019

jmdyck Oct 10, 2019

jmdyck Oct 9, 2019

gibson042 left a comment

gibson042 Oct 10, 2019

gibson042 Oct 10, 2019

gibson042 Oct 10, 2019

littledan left a comment •

edited

Loading

devsnek commented Aug 16, 2020

ljharb commented Aug 17, 2020

gibson042 commented Aug 18, 2020

michaelficarra commented Oct 12, 2020

gibson042 commented Oct 13, 2020

bakkot commented Aug 17, 2021

gibson042 commented Aug 29, 2023

	If _nn_ is `00` or the MV of _nn_ > _m_, the replacement is the string-concatenation of the replacement for the first two matched code units (i.e., a replacement specified by one of the two preceding rows) and the remaining matched code units (i.e., a single \|DecimalDigit\|).
	If _nn_ is `00` or the MV of _nn_ > _m_, the replacement is the string-concatenation of the replacement for the first two matched code units (i.e., a replacement specified by one of the two preceding rows) and the remaining matched code unit (i.e., a single \|DecimalDigit\|).

	1. If _replaceablePattern_ is not empty,
	1. If _replaceablePattern_ is not empty, then

Normative: Update GetSubstitution to match reality #1732

Normative: Update GetSubstitution to match reality #1732

Conversation

gibson042 commented Oct 9, 2019 • edited Loading

THIS PR HAS BEEN REPLACED BY #3157

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littledan left a comment • edited Loading

Choose a reason for hiding this comment

devsnek commented Aug 16, 2020

ljharb commented Aug 17, 2020

gibson042 commented Aug 18, 2020

michaelficarra commented Oct 12, 2020

gibson042 commented Oct 13, 2020

bakkot commented Aug 17, 2021

gibson042 commented Aug 29, 2023

gibson042 commented Oct 9, 2019 •

edited

Loading

littledan left a comment •

edited

Loading