1635724 - Invalid regexp group error could be more useful

Status: RESOLVED DUPLICATE of bug 1559253

(Core :: JavaScript Engine, task, P2)

Description

[Nicolas Chevobbe [:nchevobbe]]

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"

Comment 1

[Steven DeTar [:sdetar]]

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

Comment 2

[Iain Ireland [:iain]]

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

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

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 /(?)/) ?

Comment 5

[Iain Ireland [:iain]]

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?

Comment 6

[Logan Smyth [: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.

Comment 7

[Iain Ireland [:iain]]

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.

Comment 8

[Logan Smyth [: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.