Skip to content

Add RegExp hasIndices (d flag) polyfill, close #1507#1519

Open
Totoro-jam wants to merge 18 commits intozloirock:masterfrom
Totoro-jam:feature/regexp-hasIndices-polyfill
Open

Add RegExp hasIndices (d flag) polyfill, close #1507#1519
Totoro-jam wants to merge 18 commits intozloirock:masterfrom
Totoro-jam:feature/regexp-hasIndices-polyfill

Conversation

@Totoro-jam
Copy link

@Totoro-jam Totoro-jam commented Mar 6, 2026

This PR adds polyfill support for the hasIndices property (d flag) of RegExp, which is part of ES2022,to fix #1507 . The implementation:

  • Detects environments that don't support the d flag
  • Prevents SyntaxError when using new RegExp('.', 'd') in older environments
  • Adds the hasIndices property getter on RegExp.prototype
  • Calculates and returns the indices array in exec() results

Changes:

  • Add regexp-unsupported-has-indices.js for environment detection
  • Add es.regexp.has-indices.js module for hasIndices property
  • Modify es.regexp.constructor.js to handle d flag
  • Modify regexp-exec.js to calculate indices array
  • Add entry point in es/regexp/index.js
  • Add compat data in data.mjs and modules-by-versions.mjs
  • Add unit tests for hasIndices functionality.

Solution

The implementation follows the same pattern as dotAll and NCG polyfills in core-js:

  • Environment Detection: Created regexp-unsupported-has-indices.js to detect if the environment supports d flag
  • Constructor Modification: Modified es.regexp.constructor.js to accept d flag without throwing, storing hasIndices state internally
  • exec() Enhancement: Modified regexp-exec.js to calculate and add indices array to match results
  • Property Definition: Created es.regexp.has-indices.js module to define hasIndices getter on RegExp.prototype

Changes

  • Add packages/core-js/internals/regexp-unsupported-has-indices.js - Environment detection
  • Add packages/core-js/modules/es.regexp.has-indices.js - hasIndices property definition
  • Modify packages/core-js/modules/es.regexp.constructor.js - Handle d flag in constructor
  • Modify packages/core-js/internals/regexp-exec.js - Calculate indices array
  • Modify packages/core-js/es/regexp/index.js - Add entry point
  • Modify packages/core-js-compat/src/data.mjs - Add compat data
  • Modify packages/core-js-compat/src/modules-by-versions.mjs - Add version mapping
  • Modify tests/unit-global/es.regexp.constructor.js - Add unit tests
  • Modify tests/entries/unit.mjs - Add entry point test

Tests

All tests pass:

  • Unit tests for hasIndices property
  • Unit tests for indices array in exec() results
  • Tests for combinations with other flags (g, i, etc.)
  • Tests for named capture groups with indices.groups

Specification

@Totoro-jam Totoro-jam changed the title Add RegExp hasIndices (d flag) polyfill, close #1507 Add RegExp hasIndices (d flag) polyfill, close https://github.com/zloirock/core-js/issues/1507 Mar 6, 2026
@Totoro-jam Totoro-jam changed the title Add RegExp hasIndices (d flag) polyfill, close https://github.com/zloirock/core-js/issues/1507 Add RegExp hasIndices (d flag) polyfill, close issue #1504 Mar 6, 2026
@Totoro-jam Totoro-jam changed the title Add RegExp hasIndices (d flag) polyfill, close issue #1504 Add RegExp hasIndices (d flag) polyfill, close https://github.com/zloirock/core-js/issues/1507 Mar 6, 2026
@Totoro-jam Totoro-jam changed the title Add RegExp hasIndices (d flag) polyfill, close https://github.com/zloirock/core-js/issues/1507 Add RegExp hasIndices (d flag) polyfill, close #1507 Mar 6, 2026
@zloirock
Copy link
Owner

zloirock commented Mar 6, 2026

Please, make CI green, and after that, I'll take a look.

@Totoro-jam
Copy link
Author

Please, make CI green, and after that, I'll take a look.

Okay, I'm doing it.Please wait.

@Totoro-jam
Copy link
Author

Please, make CI green, and after that, I'll take a look.

maybe, The latest submission should be okay.

@johnzhou721
Copy link
Contributor

FYI if you need some help to get tests passing check out #1513 -- a pr I made adding a new polyfill. It was enough to get tests pass but may not be comprehensive.

@Totoro-jam
Copy link
Author

FYI if you need some help to get tests passing check out #1513 -- a pr I made adding a new polyfill. It was enough to get tests pass but may not be comprehensive.

Thank you for your suggestion. I tried to resolve some CI issues locally, and currently npm t works perfectly locally. It might work this time too. If it still doesn't work, then I probably need more help.

@Totoro-jam
Copy link
Author

Totoro-jam commented Mar 9, 2026

Please, make CI green, and after that, I'll take a look.

@zloirock halo, The CI has been approved; you can check it now. Thanks

@zloirock
Copy link
Owner

zloirock commented Mar 9, 2026

Half of those issues were found by Opus, so if you are making a PR with AI, you could initially ask for a review from it.

@Totoro-jam
Copy link
Author

Half of those issues were found by Opus, so if you are making a PR with AI, you could initially ask for a review from it.

Okay, actually I used a different AI model to pass the initial review twice, but I didn't use Opus. I will carefully review the issues you mentioned in your review, thank you for your guidance. Also, I'd like to discuss whether this repository can be integrated with some AI tools for code review.

@Totoro-jam Totoro-jam requested a review from zloirock March 10, 2026 10:23
@Totoro-jam
Copy link
Author

@zloirock Everything is fine. I've addressed the issues you mentioned in your review. The new test suite now includes tests for handling the missing groups property and capturing groups. The new implementation has passed both these additions and the existing test suite. I hope to receive more guidance and feedback to help further improve the functionality of this PR. Thank you.

@Totoro-jam
Copy link
Author

@zloirock hi, Hello author, the new PR didn't make any changes that might be causing the test-unit-karma test to fail. It seems to be an issue elsewhere; the preceding CIs are working fine. If you know more, I hope you can help me, or I can suggest retrying the failed jobs.

@Totoro-jam
Copy link
Author

@zloirock, now, ci/karma looks good. Thank you for seeing my message and retrying the task in time. Thank you very much.

@johnzhou721
Copy link
Contributor

New release just cut, so modules-by-version and changelog need updates.

@Totoro-jam
Copy link
Author

@zloirock , thank you for your suggestions and guidance. They are all very reasonable and necessary. I will handle it. Thank you so much for your guidance amidst your busy schedule.

@Totoro-jam
Copy link
Author

New release just cut, so modules-by-version and changelog need updates.

thanks, my friend, I get it.

@Totoro-jam Totoro-jam force-pushed the feature/regexp-hasIndices-polyfill branch 2 times, most recently from ad29b73 to 86f499e Compare March 17, 2026 04:03
@Totoro-jam
Copy link
Author

@zloirock The issue you mentioned should now be resolved. The latest test suite includes what you requested, and local npm t seems to be working fine. You can now review it again. I hope to receive more guidance and assistance. Thanks.

Copy link
Contributor

@johnzhou721 johnzhou721 left a comment

Choose a reason for hiding this comment

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

Not very familiar with regex, there's some minor things here

@Totoro-jam Totoro-jam requested a review from johnzhou721 March 18, 2026 08:25
Copy link
Contributor

@johnzhou721 johnzhou721 left a comment

Choose a reason for hiding this comment

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

Not too familiar with regex but mostly looks good. One comment

@Totoro-jam
Copy link
Author

@johnzhou721 I have adopted your suggestion, and it seems there are no similar issues now. Thank you very much for your suggestion. Now, please wait for the review by ci/cd and @zloirock. Halo zloirock. I really hope you can take a look at this PR when you are done with your work. Thank you very much.

@Totoro-jam Totoro-jam force-pushed the feature/regexp-hasIndices-polyfill branch from 31b632e to b6df4ef Compare March 20, 2026 01:59
- Add eslint-disable comment for unused str parameter in getCapturePositions
- Fix indentation in test file
- Add eslint-disable comments for eval, slow-regex, unused-capturing-group, letter-case, and unused-vars in tests
…pendencies

- Remove 'hasIndices' in re1 check that failed in non-native environments
- Fix actual/regexp/has-indices.js to reference stable/ instead of es/
- Fix PhantomJS test failure by wrapping `u` flag tests in try/catch
  (PhantomJS doesn't support ES6 unicode flag)
- Fix lookahead capture group position calculation in getCapturePositions
- Add compat test entry for es.regexp.has-indices
- Remove duplicate stringIndexOf binding (use indexOf instead)
- Fix rawFlags assignment order to prevent d flag being passed to state.raw

The root causes of karma test failures were:
1. PhantomJS 2.1.1 doesn't support the `u` (unicode) flag, causing
   tests using `new RegExp('(.)', 'ud')` to throw "Invalid flags"
2. getCapturePositions couldn't correctly handle lookahead capture groups
   because it only searched within match[0], but lookahead captures
   content outside the main match
Fix getCapturePositions algorithm to correctly handle edge cases:
- Duplicate captured strings: (b)(a)(b) matching 'bab' now correctly
  reports indices[3] as [2,3] instead of incorrectly finding [0,1]
- Empty capturing groups after non-empty captures now get correct position
- Track lastFoundPos to avoid finding the same position for duplicate strings

Add test cases for edge cases:
- Duplicate captured strings in same match
- Empty capturing group after non-empty capture
- Overlapping capturing groups
- Nested groups with repeated substrings

The original implementation used indexOf(matchString, captured, 0) which
always started searching from position 0, causing incorrect indices when
the same substring appears multiple times in the match result.
- Rewrite getCapturePositions with AST-level group parent analysis
- Add getGroupParents to parse regex source and build parent-child relationships
- Implement buildInstrumentedSource to inject empty marker groups for precise position tracking
- Fix issues with duplicate captured strings (e.g., (b)(a)(b))
- Fix nested group position calculation (e.g., (a)(aa)(a), ((a)(a)))
- Fix lookahead capture group positioning (e.g., (foo)(?=(bar)))
- Add proper empty group positioning based on parent group context
- Ensure groups property is undefined when no named groups (TC39 spec step 7a)
- Add comprehensive test cases for groups property behavior
- Fix all lint errors including trailing spaces and lonely if statements
- Improve fallback algorithm for engines without full regex support

This implementation now correctly handles all edge cases from the test suite
and fully complies with TC39 MakeMatchIndicesIndexPairArray specification.
The modules-by-versions.mjs file tracks which polyfills were added in
each core-js version, not which ES version a feature belongs to.

Since es.regexp.has-indices is a newly added polyfill, it should be
placed in a new 3.49 version block rather than 3.20.
… group indices

- Add NativeRegExp reference to avoid polyfill recursion issues
- Add parseBackreference helper to parse multi-digit backreferences
- Renumber backreferences in buildInstrumentedSource: \N -> \2N
- Use NativeRegExp instead of RegExp for instrumented regex creation
- Add test cases for nested groups with numeric and named backreferences

This fixes the issue where backreference indices were misaligned after
injecting empty capture groups before each capturing group.
…e tests

- Use uncurryThis(array-fill) for cleaner array initialization
- Cache String constructor to avoid potential issues
- Add comprehensive test cases for multi-digit backreferences (\10, \99)
- Verify all 99 capturing groups in \99 test case
- Ensure correct indices for backreference matching
…h indexOf

- Add uncurried push and pop methods for consistency with core-js coding style
- Remove redundant findCapturedPosition function, use native indexOf instead
- Fix eslint import/newline-after-import error
- Remove unused eslint-disable directive in test file
@Totoro-jam Totoro-jam force-pushed the feature/regexp-hasIndices-polyfill branch from b6df4ef to 3675d10 Compare March 20, 2026 02:06
@Totoro-jam
Copy link
Author

@zloirock Hello author, I'm very sorry to remind you, but I need your help. If you have time, please remember to review this PR and ensure CI/CD runs completely. If you have any further suggestions, please let me know. I look forward to your reply. Thank you.

@Totoro-jam
Copy link
Author

@zloirock
image
ci/karma maybe need rerun,thank you for your help, and I wish you all the best.

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.

[RegExp] Match Indices

3 participants