Skip to content

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
@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