Closed Bug 1635724 Opened 1 year ago Closed 1 year ago

Invalid regexp group error could be more useful

Categories

(Core :: JavaScript Engine, task, P2)

task

Tracking

()

RESOLVED DUPLICATE of bug 1559253

People

(Reporter: nchevobbe, Unassigned)

References

(Blocks 1 open bug)

Details

we recently got some feedback on Twitter from someone chasing a SyntaxError occuring in Firefox but not Chrome.

The error simply said invalid regexp group.

After some non-trivial investigation (because the issue was in a dependency), they managed to find that it was because the regex was using a negative lookbehind, which at the moment isn't supported in Firefox.

The error comes from /js/src/js.msg#557 :

MSG_DEF(JSMSG_INVALID_GROUP,           0, JSEXN_SYNTAXERR, "invalid regexp group")

It would be nice if the error would print the regex that is invalid, and even better if it can also print the specific group that is faulty.

Maybe something like Invalid regexp group "(?<!:)" in "/(?<!:)(\/*\.\/|\/{2,})/g"

Iain, could you take a look at this? Is this fixed by your current project?

Severity: -- → N/A
Flags: needinfo?(iireland)
Priority: -- → P2

The patch to fix this (plus a bunch of other missing regexp features) is landing in bug 1634135.

Flags: needinfo?(iireland)
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1225665

Will Bug 1634135 or Bug 1225665 will have a better error for invalid groups (not talking about only supporting new feature, but if the regex is like /(?)/) ?

Flags: needinfo?(iireland)

Oh, interesting. SpiderMonkey reports the error including a (potentially windowed) line of context along with a column number pointing to the precise location in that line of context where the error occurred. In the js shell, it looks like this:

typein:1:342 SyntaxError: invalid regexp group:
typein:1:342 , very, very long regexp with an error in the middle (?) followed by much, much, much, much, much, much, much, muc
typein:1:342 ......................................................^

It seems that this contextual information is dropped before the error is printed to the console, though. We shove all the information into an error object here. I'm not sure what happens to the error after that.

Logan, is this something that DevTools can improve?

Flags: needinfo?(iireland) → needinfo?(loganfsmyth)

Iain, it seems like the biggest issue here is that https://searchfox.org/mozilla-central/rev/446160560bf32ebf4cb7c4e25d7386ee22667255/js/src/irregexp/RegExpParser.cpp#293-295 creates line/column values that aren't useful in the context of an overall file. For instance if I run

;;;;;;/   (?<!:)/g;

in the console, the filename is right, but the line and column values are line 1 column 4 because the group is column 4 of the regexp itself.

Status: RESOLVED → REOPENED
Flags: needinfo?(loganfsmyth)
Resolution: DUPLICATE → ---
Flags: needinfo?(iireland)

The column we report is intended to be relative to the line of context that we include in the error object.

Is it better at your end if we just report a location in the source file? I think that would be an easy fix.

Flags: needinfo?(iireland) → needinfo?(loganfsmyth)

Ah interesting, we definitely assume that an error url/line/column triple will apply to the source file itself, since that's the case for runtime JS exceptions and I believe also for other JS parse errors. This seems like a change we should make.

Flags: needinfo?(loganfsmyth) → needinfo?(iireland)
Status: REOPENED → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1559253
Flags: needinfo?(iireland)
You need to log in before you can comment on or make changes to this bug.