Bug 41614 – RegExp constructor should throw a SyntaxError if invalid flags are specified

• Component: JavaScriptCore
• Status: RESOLVED WORKSFORME

[Kent Hansen]

ECMA-262 5th edition, section 15.10.4.1: "If [the flags string] contains any character other than "g", "i", or "m", or if it contains the same character more than once,
then throw a SyntaxError exception."

This is currently not the case in the JSC implementation; invalid characters are ignored, as are duplicate characters.

[Kent Hansen]

Covered by LayoutTests/fast/js/sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.4/S15.10.4.1_A5_T1.html et al.

[Kent Hansen]

Created attachment 62187 [details]
Patch

I could split the refactoring to move flags parsing out of the RegExp ctor into a separate commit, if that's preferred.

I made the error message generic to keep things simple; arguably "Invalid flag 'x'" is better, but it'd require dynamic (de)allocation of the error message, which RegExp::m_constructionError is not made for.

Actually, the whole process of flag validation/error reporting could be decoupled completely from the RegExp class, i.e. it wouldn't be possible to create a RegExp with invalid flags. But that would require modifying all the callsites of regExpCache->lookupOrCreate() to call some other create-regexp-but-maybe-not function and change the error handling according. Not sure if that would really be an improvement.

[Eric Seidel (no email)]

Olliej would either be able to review this or would know the right person to cc.

[Oliver Hunt]

Comment on attachment 62187 [details]
Patch

provisional r- -- if you can convince me that the "error" constructor is necessary i'll r+ this, but currently it seems really hacky.

JavaScriptCore/runtime/RegExpCache.cpp:40
 +          return RegExp::createWithInvalidFlags();
Why return a new "magic" regexp rather than null?

[Zoltan Herczeg]

EcmaScript-262: section 15.10.4.1 new RegExp(pattern, flags)

[...] and let F be the empty String if flags is undefined and ToString(flags) otherwise. [...] If F contains any character other than "g", "i", or "m", or if it contains the same character more than once, then throw a SyntaxError exception.

I am agree some cleanup could be useful on the code.

[Kent Hansen]

(In reply to comment #4)
> (From update of attachment 62187 [details])
> provisional r- -- if you can convince me that the "error" constructor is necessary i'll r+ this, but currently it seems really hacky.
> 
> JavaScriptCore/runtime/RegExpCache.cpp:40
>  +          return RegExp::createWithInvalidFlags();
> Why return a new "magic" regexp rather than null?

So that existing code that calls lookupOrCreate() can remain the same. No need to check for a null-RegExp (without information about what went wrong); instead isValid() and errorMessage() is used, just as with other types of RegExp construction errors. This is what I was trying to highlight in the last paragraph of comment #2.

[Kent Hansen]

(In reply to comment #5)
> EcmaScript-262: section 15.10.4.1 new RegExp(pattern, flags)
> 
> [...] and let F be the empty String if flags is undefined and ToString(flags) otherwise. [...] If F contains any character other than "g", "i", or "m", or if it contains the same character more than once, then throw a SyntaxError exception.

This is already stated in the bug description; I think Oliver was objecting to the createWithInvalidFlags() "helper" function.

> 
> I am agree some cleanup could be useful on the code.

Could you be more specific?

[Zoltan Herczeg]

> Could you be more specific?

As I am not a reviewer, it means not much, but:

1) I think a pure global regExpFlagsFromString is unnecessary, we could at least move to this function as a static member function of RegExp (or a static inline function in the .cpp file).

2) As Oliver mentioned, I don't see the reason why two constructor is needed. An "if" in the first constructor would probably be enough.

[Gavin Barraclough]

This is fixed by https://bugs.webkit.org/show_bug.cgi?id=56041