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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

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
* $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
@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.

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.

None yet

4 participants