Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible bug in GetSubstitution or test262 #1426

Closed
targos opened this issue Jan 20, 2019 · 7 comments · Fixed by #3157
Closed

Possible bug in GetSubstitution or test262 #1426

targos opened this issue Jan 20, 2019 · 7 comments · Fixed by #3157
Labels

Comments

@targos
Copy link
Contributor

targos commented Jan 20, 2019

I think I found a mismatch between the spec and some test262 tests.

In the table for Replacement Text Symbol Substitutions, the 5th row says "$n where n is one of 1 2 3 4 5 6 7 8 9 and $n is not followed by a decimal digit".

In the following test: https://github.com/tc39/test262/blob/master/test/built-ins/String/prototype/replace/S15.5.4.11_A3_T1.js
The substition string is "$1115". This string does not match that row of the table ($1 is followed by the decimal digit 1), but the test result expects that it does ($1 is replaced by the first capture).

Which is correct? Spect text or test?

@anba
Copy link
Contributor

anba commented Jan 21, 2019

Probably an oversight in #853, which changed it from If nn is 00 or nn>m, the result is implementation-defined. to If nn is 00 or nn > m, no replacement is done. while we actually want to fallback to n mod 10 first and then to no-replacement?

@claudepache
Copy link
Contributor

BTW, the wording ”no replacement is done” introduced in #853 is obscure (I first understood as: "foo".replace(/foo/, "x$42") will return "foo", and not "x$42"). Now, I understand that the expression meant in fact: ”use the literal string $nn”.

@targos
Copy link
Contributor Author

targos commented Jan 24, 2019

Probably an oversight in #853, which changed it from If nn is 00 or nn>m, the result is implementation-defined. to If nn is 00 or nn > m, no replacement is done. while we actually want to fallback to n mod 10 first and then to no-replacement?

That seems right. I tested Chrome, Firefox and Edge. All behave like this:

image

Should I try to do a PR to fix the spec?

@littledan
Copy link
Member

littledan commented Jan 24, 2019

A PR for the fix would be great. If the browsers agree, I think we should change the spec to match them. Thank you for your good work here in catching my error!

@ljharb
Copy link
Member

ljharb commented Jan 24, 2019

I ran that code in eshost and got this:

code
print('ABCDEFGHIJKL'.replace(/(A)(B)(C)(D)(E)(F)(G)(H)(I)(J)(K)(L)/, '$12'));
print('ABCDEFGHIJKL'.replace(/(A)(B)(C)(D)(E)(F)(G)(H)(I)(J)(K)/, '$12'));
results
#### Chakra
L
A2L

#### JavaScriptCore
L
A2L

#### SpiderMonkey
L
A2L

#### V8
L
A2L

#### V8 --harmony
L
A2L

so if the spec disagrees with that, then a PR to fix the spec would be much appreciated!

@ljharb
Copy link
Member

ljharb commented Apr 28, 2019

@targos any interest in making a PR?

gibson042 added a commit to gibson042/ecma262 that referenced this issue Oct 9, 2019
$nn patterns fall back to $n when there aren't at least nn captures

Fixes tc39gh-1426
gibson042 added a commit to gibson042/ecma262 that referenced this issue Nov 13, 2019
$nn patterns fall back to $n when there aren't at least nn captures

Fixes tc39gh-1426
gibson042 added a commit to gibson042/ecma262 that referenced this issue Apr 14, 2022
In anticipation of a fix for tc39#1426
(cf. tc39#2739 (comment) )
gibson042 added a commit to gibson042/ecma262 that referenced this issue Apr 26, 2022
In anticipation of a fix for tc39#1426
(cf. tc39#2739 (comment) )
gibson042 added a commit to gibson042/ecma262 that referenced this issue Aug 29, 2023
$nn patterns fall back to $n when there aren't at least nn captures

Fixes tc39gh-1426
@michaelficarra
Copy link
Member

For those like myself wondering how we managed to fail to fix this the last time it was addressed, here's the meeting notes: https://github.com/tc39/notes/blob/main/meetings/2017-03/mar-22.md#853-stringprototypereplace-edge-case-alignment.

It looks like we were only considering single-digit substitutions beyond the number of captures, not how double-digit substitutions would behave.

michaelficarra added a commit to gibson042/ecma262 that referenced this issue Sep 27, 2023
@ljharb ljharb closed this as completed in 5eaee2f Oct 12, 2023
zhangenming pushed a commit to zhangenming/ecma262 that referenced this issue Dec 22, 2023
$nn patterns fall back to $n when there aren't at least nn captures

Fixes tc39#1426
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
7 participants