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

Is /[\k](?<a>)/ legal? #2037

Closed
bakkot opened this issue Jun 9, 2020 · 10 comments
Closed

Is /[\k](?<a>)/ legal? #2037

bakkot opened this issue Jun 9, 2020 · 10 comments

Comments

@bakkot
Copy link
Contributor

bakkot commented Jun 9, 2020

In JSC, V8, and XS, /[\k](?<a>)/ is a legal regular expression. (Chakra and Spidermonkey do not implement named capturing groups and therefore reject it for that reason.)

However, my reading of the spec says it should not be legal: ParsePattern says that the presence of a group name after the initial parse should cause a reparse with +N, which means that Annex B's SourceCharacterIdentityEscape should not allow \k. (Assuming the obvious fix to #1081, which would thread the +N through ClassAtomNoDash -> ClassEscape -> CharacterEscape -> IdentityEscape -> SourceCharacterIdentityEscape.)

@shvaikalesh
Copy link
Member

shvaikalesh commented Jun 9, 2020

However, my reading of the spec says it should not be legal: ParsePattern says that the presence of a group name after the initial parse should cause a reparse with +N, which means that Annex B's SourceCharacterIdentityEscape should not allow \k.

This matches my reading of the spec; r259026 made /[\k](?<a>)/ an early SyntaxError in JSC (available in Safari TP).

@mathiasbynens would you consider aligning with JSC (and the current spec) if I add a test262 coverage?

@mathiasbynens
Copy link
Member

would you consider aligning with JSC (and the current spec) if I add a test262 coverage?

@schuay what do you think?

@schuay
Copy link
Contributor

schuay commented Jun 9, 2020

would you consider aligning with JSC (and the current spec) if I add a test262 coverage?

@schuay what do you think?

For throwing the SyntaxError, sure, thanks for the heads up. Opened https://crbug.com/v8/10602. (Early errors are another topic https://crbug.com/v8/896.)

@michaelficarra
Copy link
Member

I think \k in a character class should just be a k. Like how [\z] is the same as [z] (or z).

@waldemarhorwat
Copy link

\k in a character class should do the same thing as \k not in a character class. This is what the spec currently states and we shouldn't change that.

@michaelficarra
Copy link
Member

@waldemarhorwat Why? A lot of things are different in character classes.

@shvaikalesh
Copy link
Member

@michaelficarra [\z] is the same as [z] due to IdentityEscape of Annex B. In Unicode patterns, it is an early SyntaxError.

If a pattern contains named capture group, we can apply stricter parsing rules (and make [\k] illegal) without backwards-compatibility risk, bringing a slightly better DX.

Implementation-wise (YARR), it is a tiny bit easier to throw than to allow it.

@jmdyck
Copy link
Collaborator

jmdyck commented Jun 11, 2020

If a pattern contains named capture group, we can apply stricter parsing rules (and make [\k] illegal)

We already do:
SourceCharacterIdentityEscape[N] :: [+N] SourceCharacter but not one of c or k

@schuay
Copy link
Contributor

schuay commented Sep 28, 2021

Fixed in V8 (crbug.com/v8/10602).

@bakkot
Copy link
Contributor Author

bakkot commented Nov 27, 2024

I think everyone now agrees this is illegal; closing.

@bakkot bakkot closed this as completed Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants