Closed
Bug 1274393
Opened 8 years ago
Closed 8 years ago
RegExp(/\-/, 'u') incorrectly postpones the expected SyntaxError
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: claude.pache, Assigned: arai)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
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•8 years ago
|
||
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•8 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•8 years ago
|
||
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 4•8 years ago
|
||
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+
Comment 5•8 years ago
|
||
> Wow, that was incredibly fast!
I was going to say that. But you beat me to it.
Assignee | ||
Comment 6•8 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/458743bb901f
Status: ASSIGNED → RESOLVED
Closed: 8 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.
Description
•