1274393 - RegExp(/\-/, 'u') incorrectly postpones the expected SyntaxError

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Claude Pache]

Recall that /\-/ is a valid regexp but /\-/u is invalid, and consider this program:

    var rxu = RegExp(/\-/, 'u')
    alert(rxu.source)
    rxu.exec('foo')

Actual:
Line 2 alerts « \- »
Line 3 throws a SyntaxError (invalid escape)

Expected:
Line 1 throws a SyntaxError (invalid escape)

NB: The following code does work as expected (throws a SyntaxError):

    RegExp(/\-/.source, 'u')

Comment 1

[Till Schneidereit [:till]]

Thanks for reporting this. CC'ing arai who worked on both the unicode support and the flags parsing.

Comment 2

[Tooru Fujisawa [:arai]]

Thanks :)

So, we skipped RegExpInitialize step 7 [1] when `pattern` argument is RegExp, but if we're adding 'u' flag, we should re-check syntax there, as syntax becomes more strict.

will fix this shortly.

[1] https://tc39.github.io/ecma262/#sec-regexpinitialize

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Check the pattern syntax again when adding unicode flag to RegExp object in RegExp constructor.

Separated irregexp::ParsePatternSyntax call into CheckPatternSyntax function, to reuse from js::regexp_construct.

|g->getFlags()| has no side effect, so get it in all cases and if |flags| argument is provided, check if |g->getFlags()| doesn't have UnicodeFlag and |flags| have UnicodeFlag, and in that case call CheckPatternSyntax.

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 8754537 [details] [diff] [review]
Check the pattern syntax again when adding unicode flag to RegExp object in RegExp constructor.

Review of attachment 8754537 [details] [diff] [review]:
-----------------------------------------------------------------

Wow, that was incredibly fast!

::: js/src/builtin/RegExp.cpp
@@ +181,5 @@
> +    {
> +        return false;
> +    }
> +
> +    return true;

This can be simplified to just `return irregexp::ParsePatternSyntax(..)`

@@ +419,5 @@
>          // don't reuse the RegExpShared below.
>          RootedObject patternObj(cx, &patternValue.toObject());
>  
>          RootedAtom sourceAtom(cx);
> +        RegExpFlag origFlags;

I think I'd keep this as "flags" ...

@@ +447,3 @@
>          if (args.hasDefined(1)) {
>              // Step 4.c / 21.2.3.2.2 RegExpInitialize step 4.
>              flags = RegExpFlag(0);

... and rename this to "flagsArg" or something like that

@@ +458,5 @@
> +
> +                // ES 2017 draft rev 9b49a888e9dfe2667008a01b2754c3662059ae56
> +                // 21.2.3.2.2 step 7.
> +                if (!CheckPatternSyntax(cx, sourceAtom, flags))
> +                    return false;

And then do `flags = flagsArg` after this, getting rid of the `else` block.

Comment 5

[Jason Orendorff [:jorendorff]]

> Wow, that was incredibly fast!

I was going to say that. But you beat me to it.

Comment 6

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/458743bb901f19cf830b7133a128d1ff561d7ef0
Bug 1274393 - Check the pattern syntax again when adding unicode flag to RegExp object in RegExp constructor. r=till

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/458743bb901f