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.
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.
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.
Olliej would either be able to review this or would know the right person to cc.
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?
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.
(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.
(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?
> 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.
This is fixed by https://bugs.webkit.org/show_bug.cgi?id=56041