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

Fix es.regexp.constructor - handleNCG - consider captured groups inside non-capturing groups #1352

Merged
merged 4 commits into from
Jul 2, 2024

Conversation

Ulop
Copy link
Contributor

@Ulop Ulop commented Jul 1, 2024

handleNCG method from es.regexp.constructor does not work correctly in some cases.

If named captured group placed inside non-capturing group(or other group, that doesn't follow (?<group_name>...) pattern) then group id counter extra incremented.

 case chr === '(':
        if (exec(IS_NCG, stringSlice(string, index + 1))) {
          index += 2;
          ncg = true;
          // where increment is needed
        }
        result += chr;
        groupid++;  // where actually incremented
        continue;

Real example: H_REGEX from vscode repository.
const H_REGEX = /(?<tag>[\w\-]+)?(?:#(?<id>[\w\-]+))?(?<class>(?:\.(?:[\w\-]+))*)(?:@(?<name>(?:[\w\_])+))?/;

When it used through RegExp constructor new RegExp('(?<tag>[\\w\\-]+)?(?:#(?<id>[\\w\\-]+))?(?<class>(?:\\.(?:[\\w\\-]+))*)(?:@(?<name>(?:[\\w\\_])+))?'), exec method return wrong groups values. Tested on 'div.editor.original@original' input value.
With polyfill:

{
    "tag": "div",
    "id": ".editor.original",
    "name": undefined,
    "class": "original"
}

Native:

{
    "tag": "div",
    "id": undefined,
    "class": ".editor.original",
    "name": "original"
}

Link to test

}
result += chr;
groupid++;
Copy link
Owner

@zloirock zloirock Jul 1, 2024

Choose a reason for hiding this comment

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

It will break cases like

RegExp('foo:(?<foo>\\w+),bar:(\\w+),buz:(?<buz>\\w+)').exec('foo:abc,bar:def,buz:ghi').groups.buz

since here is a non-named group.

Could you fix it and add a couple of cases like that to the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. Changes like these really break some cases.

I added an additional check to a non-capturing group after '(' character and revert the previous changes.
Also extended regexp constructor test with your example.

@zloirock
Copy link
Owner

zloirock commented Jul 2, 2024

Could you fix linting issues? Note that regexp/no-useless-non-capturing-group and regexp/no-unused-capturing-group should be disabled with eslint-disable in the tests file.

@Ulop
Copy link
Contributor Author

Ulop commented Jul 2, 2024

Could you fix linting issues? Note that regexp/no-useless-non-capturing-group and regexp/no-unused-capturing-group should be disabled with eslint-disable in the tests file.

oops, didn't pay attention to the linter
of course it will be fixed

@zloirock zloirock merged commit 6b83ac1 into zloirock:master Jul 2, 2024
19 checks passed
@zloirock
Copy link
Owner

zloirock commented Jul 2, 2024

Thanks.

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.

2 participants