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

RESOLVED FIXED in Firefox 49

Status

()

Core
JavaScript: Standard Library
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Claude Pache, Assigned: arai)

Tracking

(Blocks: 1 bug)

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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')
(Reporter)

Updated

2 years ago
Blocks: 694100
Thanks for reporting this. CC'ing arai who worked on both the unicode support and the flags parsing.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 2

2 years ago
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
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
(Assignee)

Comment 3

2 years ago
Created attachment 8754537 [details] [diff] [review]
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.
Attachment #8754537 - Flags: review?(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.
Attachment #8754537 - Flags: review?(till) → review+
> Wow, that was incredibly fast!

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

Comment 6

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/458743bb901f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.