-
Notifications
You must be signed in to change notification settings - Fork 902
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
Add support for ES2025 RegExp duplicate named capturing groups #1291
Conversation
test("/(?<x>a)|(?<x>b)|(?<x>c)/", {}, { ecmaVersion: 2025 }) | ||
test("/(?<x>a)|\\k<x>/", {}, { ecmaVersion: 2025 }) | ||
testFail("/(?<x>a)|(?<x>b)(?<x>c)/", "Invalid regular expression: /(?<x>a)|(?<x>b)(?<x>c)/: Duplicate capture group name (1:1)", { ecmaVersion: 2025 }) | ||
testFail("/(?:(?<x>a)|(?<x>b))(?<x>c)/", "Invalid regular expression: /(?:(?<x>a)|(?<x>b))(?<x>c)/: Duplicate capture group name (1:1)", { ecmaVersion: 2025 }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I try it in Safari, it's not a syntax error. I may be misunderstanding something 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I try the front and back swapped /(?<x>c)(?:(?<x>a)|(?<x>b))/
in Safari, I will get a syntax error.
Is it a bug in Safari that /(?:(?<x>a)|(?<x>b))(?<x>c)/
does not result in a syntax error? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I read the spec, yes, that should be a syntax error—there is no |
parse node that separates either of the first two definitions of <x>
from the last one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your comment!
I was relieved that my understanding was correct 😄
If the current specs are fine, I think this PR is ready for review.
Thanks for the implementation. However, I find it a bit hard to verify that it is correct in all circumstances. I've come up with a slightly more verbose, but I think more principled approach in patch 7b7c1cd . Does that look like a reasonable approach to you? |
So amazing! I think that's code is more readable as it seems to match the spec description. |
This PR adds support for ES2025 RegExp duplicate named capturing groups.
close #1290