[JSC] Legacy RegExp fields should be accessors

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[JSC] Legacy RegExp fields should be accessors

[Yusuke Suzuki]

Created attachment 416922 [details]
Patch

[Yusuke Suzuki]

Created attachment 416923 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 416923 [details]
Patch

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

> JSTests/test262/expectations.yaml:13
> +test/annexB/built-ins/RegExp/prototype/compile/this-cross-realm-instance.js:
> +  default: 'Test262Error: `RegExp.prototype.compile.call(otherRealm_regexp)` throws TypeError Expected a TypeError to be thrown but no exception was thrown at all'
> +  strict mode: 'Test262Error: `RegExp.prototype.compile.call(otherRealm_regexp)` throws TypeError Expected a TypeError to be thrown but no exception was thrown at all'
> +test/annexB/built-ins/RegExp/prototype/compile/this-subclass-instance.js:
> +  default: 'Test262Error: `subclass_regexp.compile()` throws TypeError Expected a TypeError to be thrown but no exception was thrown at all'
> +  strict mode: 'Test262Error: `subclass_regexp.compile()` throws TypeError Expected a TypeError to be thrown but no exception was thrown at all'

This is legacy-regexp features that are not implemented yet.

> JSTests/test262/expectations.yaml:692
> +test/built-ins/Function/prototype/toString/built-in-function-object.js:
> +  default: 'Test262Error: Conforms to NativeFunction Syntax: "function $*() {\n    [native code]\n}" (%RegExp%.$*)'
> +  strict mode: 'Test262Error: Conforms to NativeFunction Syntax: "function $*() {\n    [native code]\n}" (%RegExp%.$*)'

Previously, RegExp.$* is not an accessor. So it passed. But now, it is failing because it becomes accessor, and it has a bit fun name.
We should fix it in a subsequent patch.

[Yusuke Suzuki]

Created attachment 416924 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 416924 [details]
Patch

Will look into failures.

[Yusuke Suzuki]

Comment on attachment 416924 [details]
Patch

Hmm, custom accessors are looking into caller's lexicalGlobalObject more in DOM. So, it looks like we should not use CustomAccessor in RegExp. Let's use usual accessors.

[Yusuke Suzuki]

Created attachment 416928 [details]
Patch

[Yusuke Suzuki]

Created attachment 416929 [details]
Patch

[Yusuke Suzuki]

Created attachment 416968 [details]
Patch

[Alexey Shvayka]

Comment on attachment 416968 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSObject.cpp:879
> +                    if (result != TriState::Indeterminate)

Since putEntry() is guarded with staticPropertiesReified(), can we assert that result is not TriState::Indeterminate?

> Source/JavaScriptCore/runtime/RegExpConstructor.cpp:62
> +    multiline       regExpConstructorMultiline      Accessor|DontEnum

The spec doesn't define `RegExp.multiline` / `RegExp.$*`.
Since they are not implemented in Chrome / Firefox, maybe we can drop them?

[Yusuke Suzuki]

Comment on attachment 416968 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSObject.cpp:879
>> +                    if (result != TriState::Indeterminate)
> 
> Since putEntry() is guarded with staticPropertiesReified(), can we assert that result is not TriState::Indeterminate?

Yes, that's correct. Changed.

>> Source/JavaScriptCore/runtime/RegExpConstructor.cpp:62
>> +    multiline       regExpConstructorMultiline      Accessor|DontEnum
> 
> The spec doesn't define `RegExp.multiline` / `RegExp.$*`.
> Since they are not implemented in Chrome / Firefox, maybe we can drop them?

I think this is OK for now since having these old extensions does not cause problems. Considering about the risks breaking apps (like, iOS / macOS apps using JavaScriptCore as scripting language internally), I think it is worth keeping.

[Yusuke Suzuki]

Comment on attachment 416968 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/JSObject.cpp:879
>>> +                    if (result != TriState::Indeterminate)
>> 
>> Since putEntry() is guarded with staticPropertiesReified(), can we assert that result is not TriState::Indeterminate?
> 
> Yes, that's correct. Changed.

Ah, lookupPut is not guarding putEntry with staticPropertiesReified.

[Yusuke Suzuki]

Comment on attachment 416968 [details]
Patch

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

>>>> Source/JavaScriptCore/runtime/JSObject.cpp:879
>>>> +                    if (result != TriState::Indeterminate)
>>> 
>>> Since putEntry() is guarded with staticPropertiesReified(), can we assert that result is not TriState::Indeterminate?
>> 
>> Yes, that's correct. Changed.
> 
> Ah, lookupPut is not guarding putEntry with staticPropertiesReified.

Yeah, right. I've inserted assert here.

[Yusuke Suzuki]

Created attachment 416976 [details]
Patch

[Alexey Shvayka]

Comment on attachment 416976 [details]
Patch

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

> Source/JavaScriptCore/runtime/Lookup.cpp:87
> +TriState setUpAndCallStaticSetterSlot(JSGlobalObject* globalObject, VM& vm, const ClassInfo* classInfo, const HashTableValue* entry, JSObject* baseObject, PropertyName propertyName, JSValue value, PutPropertySlot& slot)

I think this method is unnecessary, so is putEntry() return type change. The similar function above setUpStaticFunctionSlot() is used for [[Get]].
Let's copycat what putEntry() does for CustomAccessor: it acquires getter/setter from `entry` via propertyPutter().
Since we can't use it directly (function signatures don't match), let's rename it to customPropertyPutter(), and introduce propertyPutter() for regular GetterSetter.

> Source/JavaScriptCore/runtime/IntlLocalePrototype.cpp:61
> +  baseName         IntlLocalePrototypeGetterBaseName         DontEnum|Accessor|ReadOnly

It would nice to add property redefinition tests, covering ReadOnly + Accessors, since those are tricky:
  Object.defineProperty(%getter_no_setter%, {writable: true / false});
  Object.defineProperty(%getter_no_setter%, {value: 'foo'});
  Object.defineProperty(%getter_and_setter%, {writable: true / false});
  Object.defineProperty(% getter_and_setter%, {value: 'foo'});

> JSTests/test262/expectations.yaml:8
> +test/annexB/built-ins/RegExp/prototype/compile/this-cross-realm-instance.js:

These might pass now, since CustomAccessor is no longer used.

[Yusuke Suzuki]

Comment on attachment 416976 [details]
Patch

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

>> Source/JavaScriptCore/runtime/Lookup.cpp:87
>> +TriState setUpAndCallStaticSetterSlot(JSGlobalObject* globalObject, VM& vm, const ClassInfo* classInfo, const HashTableValue* entry, JSObject* baseObject, PropertyName propertyName, JSValue value, PutPropertySlot& slot)
> 
> I think this method is unnecessary, so is putEntry() return type change. The similar function above setUpStaticFunctionSlot() is used for [[Get]].
> Let's copycat what putEntry() does for CustomAccessor: it acquires getter/setter from `entry` via propertyPutter().
> Since we can't use it directly (function signatures don't match), let's rename it to customPropertyPutter(), and introduce propertyPutter() for regular GetterSetter.

This does not work well currently. We should materialize GetterSetter for setter invocation while we can invoke CustomAccessors without materialization.
Our GetterSetter IC relies on the fact that we can get GetterSetter cells currently. (`slot.setCacheableSetter(baseObject, offset);`)
So, invoking a setter will cause regression because of lack of IC.
And, invoking setter function from C++ world is more costly compared to invoking custom accessor functions from C++ since setter function needs to be invoked via vmEntryXXX.

So, I think the current strategy (materializing setter) is better for performance.

[Yusuke Suzuki]

Comment on attachment 416976 [details]
Patch

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

>> Source/JavaScriptCore/runtime/IntlLocalePrototype.cpp:61
>> +  baseName         IntlLocalePrototypeGetterBaseName         DontEnum|Accessor|ReadOnly
> 
> It would nice to add property redefinition tests, covering ReadOnly + Accessors, since those are tricky:
>   Object.defineProperty(%getter_no_setter%, {writable: true / false});
>   Object.defineProperty(%getter_no_setter%, {value: 'foo'});
>   Object.defineProperty(%getter_and_setter%, {writable: true / false});
>   Object.defineProperty(% getter_and_setter%, {value: 'foo'});

Sounds good! Added.

>> JSTests/test262/expectations.yaml:8
>> +test/annexB/built-ins/RegExp/prototype/compile/this-cross-realm-instance.js:
> 
> These might pass now, since CustomAccessor is no longer used.

We need to introduce realm check in compile function. And this patch does not change that because this patch focuses on accessor changes first.
So, in this patch, we simply leave it. We should implement remaining part of legacy-regexp thing (legacy flag, invalidation, compile's realm check etc.) and then it should pass in that patch.

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #16)
> This does not work well currently. We should materialize GetterSetter for
> setter invocation while we can invoke CustomAccessors without
> materialization.
> Our GetterSetter IC relies on the fact that we can get GetterSetter cells
> currently. (`slot.setCacheableSetter(baseObject, offset);`)
> So, invoking a setter will cause regression because of lack of IC.
> And, invoking setter function from C++ world is more costly compared to
> invoking custom accessor functions from C++ since setter function needs to
> be invoked via vmEntryXXX.

Thanks for the explanation.
putEntry() is already complicated, thanks to TriState::Indeterminate of callCustomSetter, and it's complexity will grow once we resolve FIXMEs + use it in putToPrimitive().

(In reply to Yusuke Suzuki from comment #12)
> Ah, lookupPut is not guarding putEntry with staticPropertiesReified.

Yes, but it it's only invoked for a single non-configurable custom accessor (`window.location`).
Is there a way we can revert putEntry() to return `bool`, and just add an ASSERT?
With `result == TriState::True`, we currently don't handle TriState::Indeterminate.

(In reply to Yusuke Suzuki from comment #17)
> Comment on attachment 416976 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416976&action=review
> 
> >> Source/JavaScriptCore/runtime/IntlLocalePrototype.cpp:61
> >> +  baseName         IntlLocalePrototypeGetterBaseName         DontEnum|Accessor|ReadOnly
> > 
> > It would nice to add property redefinition tests, covering ReadOnly + Accessors, since those are tricky:
> >   Object.defineProperty(%getter_no_setter%, {writable: true / false});
> >   Object.defineProperty(%getter_no_setter%, {value: 'foo'});
> >   Object.defineProperty(%getter_and_setter%, {writable: true / false});
> >   Object.defineProperty(% getter_and_setter%, {value: 'foo'});
> 
> Sounds good! Added.

Also please cover `Object.create(Object.defineProperty(%getter_no_setter%, {set() {}})).%key% = {}`.
I wish we wouldn't mix ReadOnly with accessors, and use something like `Getter|Setter` in property table (for custom accessors as well, but I won't block on it).

[Radar WebKit Bug Importer]

<rdar://problem/72986721>

[Alexey Shvayka]

*** Bug 151348 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

Created attachment 434156 [details]
Patch

[Alexey Shvayka]

Comment on attachment 434156 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:1
> [phone]  Yusuke Suzuki  <[e-mail]> and Alexey Shvayka  <[e-mail]>

LGTM, yet I'm not sure I can formally r+ given that the co-authorship :)

> Source/JavaScriptCore/ChangeLog:13
> +            1. We use Accessor attributes to make getter / setter for these fields. This patch adds setter support for Accessor LUT field.

This line no longer holds true: we use CustomAccessor instead.
It would nice for ChangeLog to state what parts of the proposal is (not) implemented, the (intentionally) required same-realmness, and that the issue described in #151348 is now fixed.
My variant would be:

        This patch implements a part of Legacy RegExp features proposal [1], replacing
        custom values with custom accessors that require |this| value to be RegExp
        constructor of the same realm.

        Apart from fixing property descriptors, this change brings legacy RegExpConstructor
        fields in compliance with invariants of internal methods [2] (described in #151348),
        aligning JSC with V8 and SpiderMonkey.

        It doesn't, however, implement [[LegacyFeaturesEnabled]] and RegExp.prototype.compile
        changes.

        [1]: https://github.com/tc39/proposal-regexp-legacy-features
        [2]: https://tc39.es/ecma262/#sec-invariants-of-the-essential-internal-methods

[Alexey Shvayka]

Comment on attachment 434156 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:1
> [phone]  Yusuke Suzuki  <[e-mail]> and Alexey Shvayka  <[e-mail]>

LGTM, yet I'm not sure I can formally r+ given that the co-authorship :)

> Source/JavaScriptCore/ChangeLog:13
> +            1. We use Accessor attributes to make getter / setter for these fields. This patch adds setter support for Accessor LUT field.

This line no longer holds true: we use CustomAccessor instead.
It would nice for ChangeLog to state what parts of the proposal is (not) implemented, the (intentionally) required same-realmness, and that the issue described in #151348 is now fixed.
My variant would be:

        This patch implements a part of Legacy RegExp features proposal [1], replacing
        custom values with custom accessors that require |this| value to be RegExp
        constructor of the same realm.

        Apart from fixing property descriptors, this change brings legacy RegExpConstructor
        fields in compliance with invariants of internal methods [2] (described in #151348),
        aligning JSC with V8 and SpiderMonkey.

        It doesn't, however, implement [[LegacyFeaturesEnabled]] and RegExp.prototype.compile
        changes.

        [1]: https://github.com/tc39/proposal-regexp-legacy-features
        [2]: https://tc39.es/ecma262/#sec-invariants-of-the-essential-internal-methods

[Sam Weinig]

Comment on attachment 434156 [details]
Patch

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

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1480
> +        Structure* currStructure = hasAlternateBase() ? alternateBase()->structure(vm) : structure();

This could be auto given all the names clearly indicate the type.

> Source/JavaScriptCore/runtime/RegExpConstructor.cpp:100
> +        return throwVMTypeError(globalObject, scope, "RegExp.$N getters require RegExp constructor as |this|"_s);

Do we use the "|this|" syntax in any other exceptions?

> Source/JavaScriptCore/runtime/RegExpConstructor.cpp:111
> +        return throwVMTypeError(globalObject, scope, "RegExp.input getters require RegExp constructor as |this|"_s);

The exception string reads a bit odd to me. I think it should probably be "RegExp.input getter requires RegExp constructor as |this|". (same for others).

[Yusuke Suzuki]

Comment on attachment 434156 [details]
Patch

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

Thanks!

>>> Source/JavaScriptCore/ChangeLog:13
>>> +            1. We use Accessor attributes to make getter / setter for these fields. This patch adds setter support for Accessor LUT field.
>> 
>> This line no longer holds true: we use CustomAccessor instead.
>> It would nice for ChangeLog to state what parts of the proposal is (not) implemented, the (intentionally) required same-realmness, and that the issue described in #151348 is now fixed.
>> My variant would be:
>> 
>>         This patch implements a part of Legacy RegExp features proposal [1], replacing
>>         custom values with custom accessors that require |this| value to be RegExp
>>         constructor of the same realm.
>> 
>>         Apart from fixing property descriptors, this change brings legacy RegExpConstructor
>>         fields in compliance with invariants of internal methods [2] (described in #151348),
>>         aligning JSC with V8 and SpiderMonkey.
>> 
>>         It doesn't, however, implement [[LegacyFeaturesEnabled]] and RegExp.prototype.compile
>>         changes.
>> 
>>         [1]: https://github.com/tc39/proposal-regexp-legacy-features
>>         [2]: https://tc39.es/ecma262/#sec-invariants-of-the-essential-internal-methods
> 
> This line no longer holds true: we use CustomAccessor instead.
> It would nice for ChangeLog to state what parts of the proposal is (not) implemented, the (intentionally) required same-realmness, and that the issue described in #151348 is now fixed.
> My variant would be:
> 
>         This patch implements a part of Legacy RegExp features proposal [1], replacing
>         custom values with custom accessors that require |this| value to be RegExp
>         constructor of the same realm.
> 
>         Apart from fixing property descriptors, this change brings legacy RegExpConstructor
>         fields in compliance with invariants of internal methods [2] (described in #151348),
>         aligning JSC with V8 and SpiderMonkey.
> 
>         It doesn't, however, implement [[LegacyFeaturesEnabled]] and RegExp.prototype.compile
>         changes.
> 
>         [1]: https://github.com/tc39/proposal-regexp-legacy-features
>         [2]: https://tc39.es/ecma262/#sec-invariants-of-the-essential-internal-methods

Sounds good!

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:1480
>> +        Structure* currStructure = hasAlternateBase() ? alternateBase()->structure(vm) : structure();
> 
> This could be auto given all the names clearly indicate the type.

We would like to use types especially in AccessCase IC algorithm since it is too complex and this type can offer good documentation meaning (e.g. it is not RegisteredStructure).
e.g. https://bugs.webkit.org/show_bug.cgi?id=226072#c33

>> Source/JavaScriptCore/runtime/RegExpConstructor.cpp:100
>> +        return throwVMTypeError(globalObject, scope, "RegExp.$N getters require RegExp constructor as |this|"_s);
> 
> Do we use the "|this|" syntax in any other exceptions?

Yes in JSC.

>> Source/JavaScriptCore/runtime/RegExpConstructor.cpp:111
>> +        return throwVMTypeError(globalObject, scope, "RegExp.input getters require RegExp constructor as |this|"_s);
> 
> The exception string reads a bit odd to me. I think it should probably be "RegExp.input getter requires RegExp constructor as |this|". (same for others).

Changed, thanks!

[Yusuke Suzuki]

Created attachment 434194 [details]
Patch

[Tadeu Zagallo]

Comment on attachment 434194 [details]
Patch

r=me

[Alexey Shvayka]

Created attachment 434595 [details]
Patch for landing

[EWS]

Committed r280460 (240095@main): <https://commits.webkit.org/240095@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434595 [details].

[Truitt Savell]

It looks like these changes broke 42 JSC tests on Debug arm64, tracking in https://bugs.webkit.org/show_bug.cgi?id=228652

[Alexey Shvayka]

*** Bug 177813 has been marked as a duplicate of this bug. ***