Closed Bug 1899813 Opened 5 months ago Closed 3 months ago

Implement Regular Expression Pattern Modifiers proposal

Categories

(Core :: JavaScript Engine, enhancement, P3)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
130 Branch
Tracking Flags:
Tracking Status
firefox130 --- fixed

People

(Reporter: dminor, Assigned: kirill.kuts.dev, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, good-first-bug)

Attachments

(1 file, 1 obsolete file)

Bug 1899813 - Add pref for regexp pattern modifiers. r=#spidermonkey-reviewers,dminor!
48 bytes, text/x-phabricator-request
Details | Review

This is likely a good first bug for someone getting started on contributing to SpiderMonkey. You can find instructions on how to build and test SpiderMonkey here: https://firefox-source-docs.mozilla.org/js/build.html.

The implementation is contained in the upstream irregexp library, controlled by a flag here: https://searchfox.org/mozilla-central/rev/23efe2c8c5b3a3182d449211ff9036fb34fe0219/js/src/jit/JitOptions.cpp#380.

What's left to do is to add a preference to control enabling the feature, using https://phabricator.services.mozilla.com/D209744 as an example, and to enable the now passing tests, by moving the regexp-modifiers feature in https://searchfox.org/mozilla-central/rev/23efe2c8c5b3a3182d449211ff9036fb34fe0219/js/src/tests/test262-update.py#28 to FEATURE_CHECK_NEEDED, and re-importing the tests as described here: https://searchfox.org/mozilla-central/rev/23efe2c8c5b3a3182d449211ff9036fb34fe0219/js/src/tests/README.txt#56.

Severity: -- → S3
Priority: -- → P3
Assignee: nobody → kirill.kuts.dev
Status: NEW → ASSIGNED
Attachment #9406194 - Attachment description: Bug 1826573 - Add pref for duplicate named capture groups. r=#spidermonkey-reviewers,iain! → Bug 1899813 - Add tests for the duplicate named capture groups. r=#spidermonkey-reviewers,dminor!

Comment on attachment 9406194 [details]
Bug 1899813 - Add pref for regexp pattern modifiers. r=#spidermonkey-reviewers,dminor!

Revision D212971 was moved to bug 1826573. Setting attachment 9406194 [details] to obsolete.

Attachment #9406194 - Attachment is obsolete: true
Attachment #9406194 - Attachment description: Bug 1899813 - Add tests for the duplicate named capture groups. r=#spidermonkey-reviewers,dminor! → Bug 1899813 - Add pref for regexp pattern modifiers. r=#spidermonkey-reviewers,dminor!
Attachment #9406194 - Attachment is obsolete: false
Attachment #9406197 - Attachment description: Bug 1899813 - Add tests for the duplicate named capture groups. r=#spidermonkey-reviewers,dminor! → Bug 1899813 - Add tests for the regexp pattern modifiers. r=#spidermonkey-reviewers,dminor!

Hi! Thanks for your work on these patches :) Unfortunately, they'll need to be rebased because of the changes from https://bugzilla.mozilla.org/show_bug.cgi?id=1903288. Once you have a chance to do that, I'll be happy to land these for you.

Flags: needinfo?(kirill.kuts.dev)

(In reply to Dan Minor [:dminor] from comment #4)

Hi! Thanks for your work on these patches :) Unfortunately, they'll need to be rebased because of the changes from https://bugzilla.mozilla.org/show_bug.cgi?id=1903288. Once you have a chance to do that, I'll be happy to land these for you.

Thanks for sharing. Sorry the delayed reply, it took me a while to figure correct mercurial commands :)

Main pref branch was updated: https://phabricator.services.mozilla.com/D212971,

But I have not rebased the other request with the tests: https://phabricator.services.mozilla.com/D212973,

Which I guess should not have conflicts but I still wanted to get latest changes in.

I have some troubles with the jstests, and that seems to be happening even on the latest revisions.

Here is what I'm doing:

hg pull
hg heads .
hg update 826034:bd34f230f70f (last changeset from previous command)
python js/src/tests/test262-update.py
./mach build
./mach jstests

And that returns:

...
[46073|   99|    0| 7936] 100% ======================================>|  87.1s
REGRESSIONS

It gives the impression that the tests are broken in the latest revision, which seems unlikely. I would appreciate your help in figuring out what might be wrong.

Flags: needinfo?(kirill.kuts.dev)

Hi, I'm out this week, but I will have a look when I get back on July 8th. I pulled your changes last week, and rebased them and everything seemed to be working fine then, so maybe something has gone wrong with your rebase.

(In reply to Dan Minor [:dminor] from comment #6)

Hi, I'm out this week, but I will have a look when I get back on July 8th. I pulled your changes last week, and rebased them and everything seemed to be working fine then, so maybe something has gone wrong with your rebase.

Thanks for the follow-up. Hope you have a great time off :)

Hi, I just tested this out, and the first patch is applying cleanly. The second patch required a small rebase. When I ran it, I hit a few additional failures, that might be new tests that have been recently added:

    --enable-regexp-modifiers test262/built-ins/RegExp/regexp-modifiers/add-dotAll.js
    --enable-regexp-modifiers test262/built-ins/RegExp/regexp-modifiers/add-dotAll-does-not-affect-multiline-flag.js
    --enable-regexp-modifiers test262/built-ins/RegExp/regexp-modifiers/add-dotAll-does-not-affect-ignoreCase-flag.js
    --enable-regexp-modifiers test262/built-ins/RegExp/regexp-modifiers/add-dotAll-does-not-affect-dotAll-property.js
    --enable-regexp-modifiers test262/built-ins/RegExp/regexp-modifiers/syntax/valid/add-modifiers-when-nested.js

If you're able to rebase the second patch, and re-run the import script, I think this will be good to go.

Flags: needinfo?(kirill.kuts.dev)

(In reply to Dan Minor [:dminor] from comment #8)

Hi, I just tested this out, and the first patch is applying cleanly. The second patch required a small rebase. When I ran it, I hit a few additional failures, that might be new tests that have been recently added:

    --enable-regexp-modifiers test262/built-ins/RegExp/regexp-modifiers/add-dotAll.js
    --enable-regexp-modifiers test262/built-ins/RegExp/regexp-modifiers/add-dotAll-does-not-affect-multiline-flag.js
    --enable-regexp-modifiers test262/built-ins/RegExp/regexp-modifiers/add-dotAll-does-not-affect-ignoreCase-flag.js
    --enable-regexp-modifiers test262/built-ins/RegExp/regexp-modifiers/add-dotAll-does-not-affect-dotAll-property.js
    --enable-regexp-modifiers test262/built-ins/RegExp/regexp-modifiers/syntax/valid/add-modifiers-when-nested.js

If you're able to rebase the second patch, and re-run the import script, I think this will be good to go.

Thanks for the feedback. I rebased the second patch and added the listed tests to the skip list. Confirmed that the tests are passing now

Flags: needinfo?(kirill.kuts.dev)
Pushed by dminor@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c27e2c399c65 Add pref for regexp pattern modifiers. r=iain,spidermonkey-reviewers https://hg.mozilla.org/integration/autoland/rev/a9c4edb495bc Add tests for the regexp pattern modifiers. r=dminor,spidermonkey-reviewers

Backed out for causing Spidermonkey build bustages in add-ignoreCase-affects-slash-upper-b.js.

  • Backout link
  • Push with failures
  • Failure Log
  • Failure line: TEST-UNEXPECTED-FAIL | test262/built-ins/RegExp/regexp-modifiers/add-ignoreCase-affects-slash-upper-b.js | (args: "--dll /builds/worker/fetches/injector/libbreakpadinjector.so --ion-eager --ion-offthread-compile=off --more-compartments") [0.0 s]
Flags: needinfo?(kirill.kuts.dev)

(In reply to Dan Minor [:dminor] from comment #8)

Hi, I just tested this out, and the first patch is applying cleanly. The second patch required a small rebase. When I ran it, I hit a few additional failures, that might be new tests that have been recently added:

    --enable-regexp-modifiers test262/built-ins/RegExp/regexp-modifiers/add-dotAll.js
    --enable-regexp-modifiers test262/built-ins/RegExp/regexp-modifiers/add-dotAll-does-not-affect-multiline-flag.js
    --enable-regexp-modifiers test262/built-ins/RegExp/regexp-modifiers/add-dotAll-does-not-affect-ignoreCase-flag.js
    --enable-regexp-modifiers test262/built-ins/RegExp/regexp-modifiers/add-dotAll-does-not-affect-dotAll-property.js
    --enable-regexp-modifiers test262/built-ins/RegExp/regexp-modifiers/syntax/valid/add-modifiers-when-nested.js

If you're able to rebase the second patch, and re-run the import script, I think this will be good to go.

There seem to be a problem with multiple tests in the Spidermonkey build,.
I've tried to execute failing tests manually and all of them passed.

Is there any chance that's related to the feature check config?

https://hg.mozilla.org/integration/autoland/rev/1170480bd1e0#:~:text=%22regexp%2Dmodifiers%22%3A%20%22!(this.hasOwnProperty(%27getBuildConfiguration%27)%26%26!getBuildConfiguration(%27release_or_beta%27))%22%2C

Flags: needinfo?(kirill.kuts.dev)

I pulled and applied your patches before trying to land them, and they passed locally for me as well. I think you're right, it is something to do with the feature check. I'll have a look.

The error is a syntax error:

[task 2024-07-12T20:46:24.454Z] TEST-UNEXPECTED-FAIL | test262/built-ins/RegExp/regexp-modifiers/add-multiline-does-not-affect-dotAll-flag.js | (args: "--dll /builds/worker/fetches/injector/libbreakpadinjector.so --ion-eager --ion-offthread-compile=off --more-compartments") [0.0 s]
[task 2024-07-12T20:46:24.454Z] ## test262/built-ins/RegExp/regexp-modifiers/add-multiline-does-not-affect-dotAll-flag.js: rc = 3, run time = 0.018269
[task 2024-07-12T20:46:24.454Z] /builds/worker/checkouts/gecko/js/src/tests/test262/built-ins/RegExp/regexp-modifiers/add-multiline-does-not-affect-dotAll-flag.js:48:12 SyntaxError: repeated flag in regexp modifier:
[task 2024-07-12T20:46:24.454Z] /builds/worker/checkouts/gecko/js/src/tests/test262/built-ins/RegExp/regexp-modifiers/add-multiline-does-not-affect-dotAll-flag.js:48:12 (?m:es.$)
[task 2024-07-12T20:46:24.454Z] /builds/worker/checkouts/gecko/js/src/tests/test262/built-ins/RegExp/regexp-modifiers/add-multiline-does-not-affect-dotAll-flag.js:48:12 ..^

This is being produced here. This means the feature is being enabled correctly, but something else is going wrong. This seems to be the same class of error that was reported in Bug 1903454.

I'd suggest we add the failing tests from the CI run to the skip list in jstests.list for now, and we can investigate further in Bug 1903454. We also have a pending irregexp update in Bug 1907634, although I haven't checked to see if there are any fixes related to pattern modifiers.

I'm out of the office this week, but I can help when I get back next Monday.

(In reply to Dan Minor [:dminor] from comment #14)

The error is a syntax error:

[task 2024-07-12T20:46:24.454Z] TEST-UNEXPECTED-FAIL | test262/built-ins/RegExp/regexp-modifiers/add-multiline-does-not-affect-dotAll-flag.js | (args: "--dll /builds/worker/fetches/injector/libbreakpadinjector.so --ion-eager --ion-offthread-compile=off --more-compartments") [0.0 s]
[task 2024-07-12T20:46:24.454Z] ## test262/built-ins/RegExp/regexp-modifiers/add-multiline-does-not-affect-dotAll-flag.js: rc = 3, run time = 0.018269
[task 2024-07-12T20:46:24.454Z] /builds/worker/checkouts/gecko/js/src/tests/test262/built-ins/RegExp/regexp-modifiers/add-multiline-does-not-affect-dotAll-flag.js:48:12 SyntaxError: repeated flag in regexp modifier:
[task 2024-07-12T20:46:24.454Z] /builds/worker/checkouts/gecko/js/src/tests/test262/built-ins/RegExp/regexp-modifiers/add-multiline-does-not-affect-dotAll-flag.js:48:12 (?m:es.$)
[task 2024-07-12T20:46:24.454Z] /builds/worker/checkouts/gecko/js/src/tests/test262/built-ins/RegExp/regexp-modifiers/add-multiline-does-not-affect-dotAll-flag.js:48:12 ..^

This is being produced here. This means the feature is being enabled correctly, but something else is going wrong. This seems to be the same class of error that was reported in Bug 1903454.

I'd suggest we add the failing tests from the CI run to the skip list in jstests.list for now, and we can investigate further in Bug 1903454. We also have a pending irregexp update in Bug 1907634, although I haven't checked to see if there are any fixes related to pattern modifiers.

I'm out of the office this week, but I can help when I get back next Monday.

Thanks for the follow up. I'll add failing tests to the skip list.

There was a minor change to the related error message, however I don't think it's the failure trigger: https://phabricator.services.mozilla.com/D212971#:~:text=MSG_DEF(JSMSG_REPEATED_FLAG%2C%20%20%20%20%20%20%20%20%20%20%200%2C%20JSEXN_SYNTAXERR%2C%20%22repeated%20flag%20in%20regexp%20modifier%22)

The first patch applies fine, see: https://treeherder.mozilla.org/jobs?repo=try&revision=b2e4ab0544a81cc0c830def8efc2677be4e39064, but there's still errors with enabling the tests in CI, but not locally. I'm going to land the first patch to avoid having to rebase yet again, and investigate the CI failures further tomorrow.

Pushed by dminor@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ffec1daec4e8 Add pref for regexp pattern modifiers. r=iain,spidermonkey-reviewers

https://hg.mozilla.org/mozilla-central/rev/ffec1daec4e8

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
status-firefox130: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 130 Branch

(In reply to Dan Minor [:dminor] from comment #16)

The first patch applies fine, see: https://treeherder.mozilla.org/jobs?repo=try&revision=b2e4ab0544a81cc0c830def8efc2677be4e39064, but there's still errors with enabling the tests in CI, but not locally. I'm going to land the first patch to avoid having to rebase yet again, and investigate the CI failures further tomorrow.

Thanks for your help with the tests! Is there a way for me to help? I was thinking about tweaking the running configuration, or maybe trying to run one of the failing tests in isolation

Attachment #9406197 - Attachment is obsolete: true

I think we should follow up on the failing tests in Bug 1903454. The tests that pass locally but fail in CI follow that same pattern of SyntaxError: repeated flag in regexp modifier. I think at this point, we might as well fix the underlying cause of that problem, rather than continue to try to find a working subset of the tests here.

Please let me know if you'd like to investigate Bug 1903454, or if you're interested in doing something different. Thanks for your contribution :)

(In reply to Dan Minor [:dminor] from comment #20)

I think we should follow up on the failing tests in Bug 1903454. The tests that pass locally but fail in CI follow that same pattern of SyntaxError: repeated flag in regexp modifier. I think at this point, we might as well fix the underlying cause of that problem, rather than continue to try to find a working subset of the tests here.

Please let me know if you'd like to investigate Bug 1903454, or if you're interested in doing something different. Thanks for your contribution :)

Got it. Thanks for your help with landing this update. I’ll be happy to give Bug 1903454 a shot! I will keep you posted on my findings. :)

I have tried this in Firefox Nightly (131.0a1) & Firefox Beta (130.0b1) and i get the following error SyntaxError: invalid regexp group

I have tried this in Chrome 127 and it works fine.

This is the code I am testing: https://codepen.io/CodeRedDigital/pen/GRbEOLE?editors=0011

Flags: needinfo?(kirill.kuts.dev)

(In reply to Dave Letorey from comment #22)

I have tried this in Firefox Nightly (131.0a1) & Firefox Beta (130.0b1) and i get the following error SyntaxError: invalid regexp group

I have tried this in Chrome 127 and it works fine.

This is the code I am testing: https://codepen.io/CodeRedDigital/pen/GRbEOLE?editors=0011

Just to double-check, is the regexp_modifiers flag set to true in the build you've mentioned? FYI it's disabled by default in the modules/libpref/init/StaticPrefList.yaml (https://phabricator.services.mozilla.com/D212971#:~:text=javascript.options.experimental.regexp_modifiers)

Flags: needinfo?(kirill.kuts.dev)

Even with the pref javascript.options.experimental.regexp_modifiers enabled, I get an "Uncaught SyntaxError: repeated flag in regexp modifier" for the simple regexp /(?i:a)/. And the Codepen example from Dave also fails with the same error.
I assime that's covered by bug 1903454, right?

Sebastian

Flags: needinfo?(kirill.kuts.dev)
Blocks: 1903454

(In reply to Dave Letorey from comment #22)

I have tried this in Firefox Nightly (131.0a1) & Firefox Beta (130.0b1) and i get the following error SyntaxError: invalid regexp group

I have tried this in Chrome 127 and it works fine.

This is the code I am testing: https://codepen.io/CodeRedDigital/pen/GRbEOLE?editors=0011

Thanks for your reply. I can confirm that I observe the same error in the mentioned Nightly build. This is expected behavior (at least based on the bug description) with the feature check disabled (e.g., regexp_modifiers = false).

Could you please confirm that the error persists even with regexp_modifiers enabled? If so, could you please share the steps required to enable regexp_modifiers in the nightly build?

Flags: needinfo?(kirill.kuts.dev)

(In reply to Sebastian Zartner [:sebo] from comment #24)

Even with the pref javascript.options.experimental.regexp_modifiers enabled, I get an "Uncaught SyntaxError: repeated flag in regexp modifier" for the simple regexp /(?i:a)/. And the Codepen example from Dave also fails with the same error.
I assime that's covered by bug 1903454, right?

Sebastian

I don’t believe the mentioned bug addresses the problem described. With the regexp_modifiers flag enabled, it is expected that the mentioned regular expression (/(?i:a)/) and the regular expressions from Dave’s example should work correctly.

How can I enable regexp_modifiers in the Nightly built?
Tried searching inside the about:preferences and running ./firefox --enable-regexp-modifiers but no luck :(

Hi Just for confirm that with the javascript.options.experimental.regexp_modifiers flag set to true I now get a different error:

SyntaxError: repeated flag in regexp modifier

I get this error for :

  • const pattern = /(?-i:var|let|const) (foo|bar)\b/i;
  • const re1 = /^[a-z](?-i:[a-z])$/i;

But not for:

  • const re2 = /^(?i:[a-z])[a-z]$/;

This is because in the first two examples the i flag is used twice but this should be acceptable as the statement says check everything for any case except this part.

(In reply to Kiril K from comment #26)

How can I enable regexp_modifiers in the Nightly built?
This is done in about:config and then search for the flag and set to true

I can see that this has nnow been fixed and is working as expected in Firefox Nightly 131.0a1 (2024-08-28)

Although still not working in Firefox Beta 130.0

I assume that this is now being moved to the 131 release?

Flags: needinfo?(kirill.kuts.dev)

The feature is behind a nightly-only flag at the moment. Bug 1913752 tracks letting it ride to release.

Flags: needinfo?(kirill.kuts.dev)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: