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

More tests for duplicate named capturing groups #3706

Merged
merged 3 commits into from
Nov 2, 2022

Conversation

ptomato
Copy link
Contributor

@ptomato ptomato commented Oct 28, 2022

See #3704. This covers the first and third tables in that issue: syntax tests, and tests covering the functionality of the .groups and .indices.groups objects.

@ptomato ptomato requested a review from a team as a code owner October 28, 2022 23:14
@ptomato
Copy link
Contributor Author

ptomato commented Oct 28, 2022

cc @bakkot

Copy link
Contributor

@bakkot bakkot left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for catching the issues in my original tests.

Parse-time syntax for RegExp literals is already tested. These two files
test runtime RegExp compilation, with respect to duplicate named capture
groups.

See: tc39#3704
Each named capturing group should count as its own parenthesized capturing
group, even if it has the same name as another group. So, some of these
expectations were missing `undefined` array elements for the variant of
the `x` capturing group that didn't match.

In the other expectations, we forgot to take into account that the
backreference is not inside a capturing group, so the group match should
not have doubled letters in it.
These tests should cover the full functionality of the .groups object (and
the .indices.groups object, in the case of the /d flag) for RegExp.p.exec
and String.p.match:

- Matched DNCG has a result
- Unmatched DNCG is present and undefined
- DNCG matched in previous iteration but not in current iteration is
  treated as unmatched
- Iteration order of properties corresponds with source order

See: tc39#3704
@Ms2ger Ms2ger force-pushed the duplicate-named-capturing-groups branch from 5a98bc5 to 991097c Compare November 2, 2022 14:19
@Ms2ger Ms2ger enabled auto-merge (rebase) November 2, 2022 14:19
@Ms2ger Ms2ger merged commit 27063ae into tc39:main Nov 2, 2022
@ptomato ptomato deleted the duplicate-named-capturing-groups branch November 2, 2022 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants