zloirock / core-js

Standard Library
MIT License
24.6k stars 1.66k forks source link

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

Closed Ulop closed 5 months ago

Ulop commented 5 months ago

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%3F(%3F%3Cclass%3E(%3F%3A%5C%5C.(%3F%3A%5B%5C%5Cw%5C%5C-%5D%2B))*)(%3F%3A%40(%3F%3Cname%3E(%3F%3A%5B%5C%5Cw%5C%5C_%5D)%2B))%3F')%3B%0Alet%20input%20%3D%20'div.editor.original%40original'%3B%0Aconsole.log(reg.exec(input))) to test

zloirock commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

Thanks.