Bug 145599 – Implement RegExp.prototype.flags

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Jordan Harband]

Per https://people.mozilla.org/~jorendorff/es6-draft.html#sec-get-regexp.prototype.flags, the RegExp.prototype.flags getter should return a sorted string of the flag characters, suitable for being passed into the RegExp constructor.

[Jordan Harband]

Created attachment 254186 [details]
Patch

[Geoffrey Garen]

Comment on attachment 254186 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254186&action=review

> Source/JavaScriptCore/runtime/RegExpPrototype.cpp:76
> +void RegExpPrototype::finishCreation(VM& vm, JSGlobalObject* globalObject)
> +{
> +    JSFunction* flagsFunction = JSFunction::create(vm, globalObject, 0, vm.propertyNames->flags.string(), regExpProtoFuncGetFlags);
> +    GetterSetter* accessor = GetterSetter::create(vm, globalObject);
> +    accessor->setGetter(vm, globalObject, flagsFunction);
> +    putDirectNonIndexAccessor(vm, vm.propertyNames->flags, accessor, DontEnum | Accessor);
> +}

Can you use a standard host property instead of a Javascript getter? That's more efficient, and it should require less code.

> Source/JavaScriptCore/runtime/RegExpPrototype.cpp:139
> +static inline String getRegExpFlags(ExecState *exec, RegExpObject* regexp)

We usually use "get" to mean "returns a value through an out parameter". This function should be named "flags()" or "flagsString()".

[Jordan Harband]

(In reply to comment #2)
Can you use a standard host property instead of a Javascript getter? That's
> more efficient, and it should require less code.
No, the spec requires that it be a getter on the prototype.

> > Source/JavaScriptCore/runtime/RegExpPrototype.cpp:139
> > +static inline String getRegExpFlags(ExecState *exec, RegExpObject* regexp)
> 
> We usually use "get" to mean "returns a value through an out parameter".
> This function should be named "flags()" or "flagsString()".
Sure, I'll rename it this evening.

[Jordan Harband]

Actually, perhaps I misunderstood your comment - can you point me to an example of a "standard host property" instead of a getter?

Also, note that "flags" is meant to work with subclassing, so it'd always need to check the appropriate flag properties (global, etc), even if it's .call-ed with a RegExp subclass instance.

[Darin Adler]

(In reply to comment #4)
> Also, note that "flags" is meant to work with subclassing, so it'd always
> need to check the appropriate flag properties (global, etc), even if it's
> .call-ed with a RegExp subclass instance.

Should cover that with a test case.

[Jordan Harband]

(In reply to comment #5)
> (In reply to comment #4)
> > Also, note that "flags" is meant to work with subclassing, so it'd always
> > need to check the appropriate flag properties (global, etc), even if it's
> > .call-ed with a RegExp subclass instance.
> 
> Should cover that with a test case.
Good call - WK doesn't support subclassing yet afaik, but I can definitely call it with a generic object in tests and make sure the right strings are produced. Thanks!

[Jordan Harband]

Created attachment 254242 [details]
Patch

[Geoffrey Garen]

> Actually, perhaps I misunderstood your comment - can you point me to an
> example of a "standard host property" instead of a getter?

Consider the other properties of a RegExpObject: global; ignoreCase; multiline; and source. To produce a standard host property, they list themselves in a table:

/* Source for RegExpObject.lut.h
@begin regExpTable
    global        regExpObjectGlobal       DontDelete|ReadOnly|DontEnum
    ignoreCase    regExpObjectIgnoreCase   DontDelete|ReadOnly|DontEnum
    multiline     regExpObjectMultiline    DontDelete|ReadOnly|DontEnum
    source        regExpObjectSource       DontDelete|ReadOnly|DontEnum
@end

This table is processed by a script at compile time in order to produce a hash table of properties.

The second entry in the table indicates the function to call in order to access the property. 

Can you make flags work this way?

> Also, note that "flags" is meant to work with subclassing, so it'd always
> need to check the appropriate flag properties (global, etc), even if it's
> .call-ed with a RegExp subclass instance.

Yup.

[Jordan Harband]

(In reply to comment #8)
> > Actually, perhaps I misunderstood your comment - can you point me to an
> > example of a "standard host property" instead of a getter?
> 
> Consider the other properties of a RegExpObject: global; ignoreCase;
> multiline; and source. To produce a standard host property, they list
> themselves in a table:
> 
> /* Source for RegExpObject.lut.h
> @begin regExpTable
>     global        regExpObjectGlobal       DontDelete|ReadOnly|DontEnum
>     ignoreCase    regExpObjectIgnoreCase   DontDelete|ReadOnly|DontEnum
>     multiline     regExpObjectMultiline    DontDelete|ReadOnly|DontEnum
>     source        regExpObjectSource       DontDelete|ReadOnly|DontEnum
> @end
> 
> This table is processed by a script at compile time in order to produce a
> hash table of properties.
> 
> The second entry in the table indicates the function to call in order to
> access the property. 
> 
> Can you make flags work this way?
> 
> > Also, note that "flags" is meant to work with subclassing, so it'd always
> > need to check the appropriate flag properties (global, etc), even if it's
> > .call-ed with a RegExp subclass instance.
> 
> Yup.


However, currently `Object.getOwnPropertyDescriptor(RegExp.prototype, 'source')` returns a descriptor with "value" set, and not "get". The spec for "flags" requires that it be an actual getter function, so it'd be different from the four regExpTable examples you mentioned. I'm pretty new to C++ and Webkit both - do you think this would work in the same way if it was in the table?

[Geoffrey Garen]

> However, currently `Object.getOwnPropertyDescriptor(RegExp.prototype,
> 'source')` returns a descriptor with "value" set, and not "get". The spec
> for "flags" requires that it be an actual getter function, so it'd be
> different from the four regExpTable examples you mentioned.

Aha. I understand your motivation now.

In fact, all of these properties are supposed to be getters according to the ES6 spec. It's a bug in our implementation of these automatic tables that they do not produce proper getters.

Still, I think it is best to implement all of these properties through the same mechanism, rather than fixing the getter issue for one property through special-case code. Then, we should update the underlying mechanism to produce getters, as ES6 requires.

[Jordan Harband]

That's a fair point, but I'm super wary of introducing new functionality in a broken way. I'd prefer to add flags correctly, and migrate it to a "proper" mechanism once that exists - otherwise the es6-shim will have to overwrite Safari's broken implementation with its own, and the ES6 Compatibility Table will have to mark Webkit as failing to have that feature.

[Jordan Harband]

In other words, I feel strongly that spec correctness is far more important than "implementation code consistency", because the latter can be cleaned up later transparently - the former is a user-visible bug that the entire internet has to support for effectively forever.

[Darin Adler]

I agree with Geoff.

[Jordan Harband]

In that case, this shouldn't land. It's horrific to suggest landing a known broken feature and condemning the internet to support it forever, so this should just wait until the other properties can be changed to getters.

[Geoffrey Garen]

Comment on attachment 254242 [details]
Patch

I've filed this bug to follow up on getter-ness for all properties: https://bugs.webkit.org/show_bug.cgi?id=145705. Maybe you can help us resolve it.

[Yusuke Suzuki]

Comment on attachment 254242 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254242&action=review

Added comments :)

> Source/JavaScriptCore/runtime/RegExpPrototype.cpp:143
> +    if (regexp->get(exec, exec->propertyNames().global).toBoolean(exec))

We need to check an exceptions per `get` since it may cause an exception.

> Source/JavaScriptCore/runtime/RegExpPrototype.cpp:157
> +    JSValue flags = jsMakeNontrivialString(exec, flagsString(exec, asRegExpObject(thisValue)));

Because we just check `thisValue.inherits(JSObject::info())`, `asRegExpObject` is unsafe. It may not be `RegExpObject`.
For example,

var desc = Object.getOwnPropertyDescriptor(RegExp.prototype, 'flags');
desc.get.call([]);

According to the spec, RegExp.prototype.flags accepts all JSObject (indlucing JSArray). So I think using `isObject` guard and `asObject` is enough.

> Source/JavaScriptCore/runtime/RegExpPrototype.cpp:173
>      String source = thisObject->get(exec, exec->propertyNames().source).toString(exec)->value(exec);

We need to check an exception here before executing `flagsString`.

And since `get` may cause an exception, `thisObject->get(exec, exec->propertyNames().source).toString(exec)` is not safe.

... thisObject->get(exec, exec->propertyNames().source);
// check exception
....toString(exec)->value(exec);
// check exception

is needed.

[Jordan Harband]

Thanks, I'll update this patch with those comments after 145705 goes in.

[Jordan Harband]

Created attachment 254633 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 254633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254633&action=review

Patch itself looks nice. With nits.

> Source/JavaScriptCore/runtime/RegExpPrototype.cpp:224
> +    JSValue flags = jsMakeNontrivialString(exec, flagsString(exec, asObject(thisValue)));

flags = flagsString(exec, asObject(thisValue));
// check exception
return JSValue::encode(jsMakeNontrivialString(exec, flags));

is preferable.

[Jordan Harband]

Created attachment 254663 [details]
Patch

[Geoffrey Garen]

Comment on attachment 254663 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 254663 [details]
Patch

Clearing flags on attachment: 254663

Committed r185432: <http://trac.webkit.org/changeset/185432>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jordan Harband]

Yay! Thanks everyone for the reviews, and the patience as we worked this out, and special thanks to Yusuke for taking on 145705 to pave the way for this.

[Chris Dumez]

Looks like js/regexp-flags.html started crashing on several bots after this change:
https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r185432%20(4870)/results.html

Please address ASAP.

[Chris Dumez]

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x0000000110c6fd77 WTFCrash + 39
1   com.apple.JavaScriptCore      	0x0000000110686da8 JSC::jsNontrivialString(JSC::VM*, WTF::String const&) + 72
2   com.apple.JavaScriptCore      	0x0000000110686965 JSC::jsNontrivialString(JSC::ExecState*, WTF::String const&) + 37
3   com.apple.JavaScriptCore      	0x0000000110b674e5 JSC::JSValue JSC::jsMakeNontrivialString<WTF::String&>(JSC::ExecState*, WTF::String&&&) + 37
4   com.apple.JavaScriptCore      	0x0000000110b669ff JSC::regExpProtoGetterFlags(JSC::ExecState*) + 191
5   com.apple.JavaScriptCore      	0x0000000110a07399 vmEntryToNative + 367
6   com.apple.JavaScriptCore      	0x000000011084f30f JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 1535
7   com.apple.JavaScriptCore      	0x00000001102c670e JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 190
8   com.apple.JavaScriptCore      	0x000000011078dc5d JSC::callGetter(JSC::ExecState*, JSC::JSValue, JSC::JSValue) + 221
9   com.apple.JavaScriptCore      	0x0000000110b51861 JSC::PropertySlot::functionGetter(JSC::ExecState*) const + 145
10  com.apple.JavaScriptCore      	0x000000011025a3cd JSC::PropertySlot::getValue(JSC::ExecState*, JSC::PropertyName) const + 93
11  com.apple.JavaScriptCore      	0x000000011025d86b JSC::JSValue::get(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&) const + 91
12  com.apple.JavaScriptCore      	0x00000001109fbd21 llint_slow_path_get_by_id + 241
13  com.apple.JavaScriptCore      	0x0000000110a0a1b0 llint_entry + 11668
14  com.apple.JavaScriptCore      	0x0000000110a071d9 vmEntryToJavaScript + 361
15  com.apple.JavaScriptCore      	0x000000011086b01c JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 252
16  com.apple.JavaScriptCore      	0x000000011084bcff JSC::Interpreter::execute(JSC::EvalExecutable*, JSC::ExecState*, JSC::JSValue, JSC::JSScope*) + 2543
17  com.apple.JavaScriptCore      	0x000000011084b2d9 JSC::eval(JSC::ExecState*) + 1065
18  com.apple.JavaScriptCore      	0x0000000110a00c48 llint_slow_path_call_eval + 328
19  com.apple.JavaScriptCore      	0x0000000110a0dd36 llint_entry + 26906
20  ???                           	0x0000549da7601afa 0 + 93036094692090

[Chris Dumez]

Looks like an assertion hit in JSC::jsNontrivialString(JSC::VM*, WTF::String const&):
ASSERT(s.length() > 1);

[Yusuke Suzuki]

(In reply to comment #27)
> Looks like an assertion hit in JSC::jsNontrivialString(JSC::VM*, WTF::String
> const&):
> ASSERT(s.length() > 1);

Nice catch. Since jsNontrivialString requires the given string is *non-trivial*, the length of it should be larger than 1.
I'll fix it with a unreviewed very simple follow up patch.

[Yusuke Suzuki]

Committed r185440: <http://trac.webkit.org/changeset/185440>