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

Increase coverage of String.prototype.replace $xy replacement patterns #3931

Merged
merged 6 commits into from
Oct 10, 2024

Conversation

gibson042
Copy link
Contributor

  • $xy is a valid capture index
  • $xy is not a valid capture index but $x is
  • neither $xy nor $x is a valid capture index

Ref tc39/ecma262#1426
Ref tc39/ecma262#3157

@gibson042 gibson042 requested a review from a team as a code owner September 24, 2023 02:06
@gibson042 gibson042 force-pushed the ecma262-1426-getsubstitution-dollar-nn branch from a8a5d2d to 3e3e39c Compare September 25, 2023 20:16
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed these tests pass my ES2024 GetSubstitution implementation in es-abstract, and fail on <= ES2023 (as expected)

@ljharb
Copy link
Member

ljharb commented Sep 26, 2023

update; tc39/ecma262#3157 (comment) should get some coverage too ($01 - $09)

@gibson042
Copy link
Contributor Author

@ljharb coverage expanded

@ptomato
Copy link
Contributor

ptomato commented Oct 2, 2023

This normative change got consensus conditionally in the 2023-09 TC39 plenary meeting. "If there is a mismatch between engines, the presenter should bring the change back to TC39 to discuss next steps." So I guess before we land this, the proponents of the normative change should perform that investigation.

@gibson042
Copy link
Contributor Author

Done. The only implementation with failures is engine262, which does not implement the single-digit fallback:

  • $10 is not a capture index (but $1 is) in /(x)/
  • $10 is not a capture index (but $1 is) in /(x)($^)?/
  • $10 before 0 is not a capture index (but $1 is) in /(x)/
  • $10 before 0 is not a capture index (but $1 is) in /(x)($^)?/
  • $20 is not a capture index (but $2 is a failed capture index) in /(x)($^)?/
  • $20 is not a capture index (but $2 is) in /((((((((((x))))))))))/
  • $20 before 0 is not a capture index (but $2 is a failed capture index) in /(x)($^)?/
  • $20 before 0 is not a capture index (but $2 is) in /((((((((((x))))))))))/
$ eshost -se '"foo-x-bar".replace(/(x)/, "|$10|")'
#### ChakraCore, GraalJS, Hermes, JavaScriptCore, Moddable XS, QuickJS, SpiderMonkey, V8
foo-|x0|-bar

#### engine262
foo-|$10|-bar

@ptomato
Copy link
Contributor

ptomato commented Oct 3, 2023

Thanks. So does that mean there are no longer any blockers for this?

@gibson042
Copy link
Contributor Author

Yes, that is my understanding.

@gibson042 gibson042 force-pushed the ecma262-1426-getsubstitution-dollar-nn branch from 94d5c55 to 1bf3354 Compare October 9, 2024 04:05
@ljharb ljharb requested a review from a team October 9, 2024 04:08
@ptomato
Copy link
Contributor

ptomato commented Oct 10, 2024

What's the status of this? It looks like it has a positive review from Jordan, but still needs to be autosquashed.

@ljharb
Copy link
Member

ljharb commented Oct 10, 2024

It's easy to squash it at landing time, but I wanted more reviews before landing it.

Copy link
Member

@linusg linusg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, also passes with my literal implementation of GetSubstitution.

@Ms2ger Ms2ger force-pushed the ecma262-1426-getsubstitution-dollar-nn branch from 1bf3354 to e1a7d43 Compare October 10, 2024 11:40
@Ms2ger Ms2ger merged commit 5ae7de9 into tc39:main Oct 10, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants