Bug 226291 – Implement Object.hasOwn

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

[Jamie Kyle]

`Object.hasOwn` is a Stage 3 TC39 Proposal:

Proposal: https://github.com/tc39/proposal-accessible-object-hasownproperty
Spec: https://tc39.es/proposal-accessible-object-hasownproperty/

```
// Before:
({ foo: true }).hasOwnProperty("foo")
// After:
Object.hasOwn({ foo: true }, "foo")
```

`Object.hasOwn` can likely be implemented with much of the same code as `Object.prototype.hasOwnProperty`

However, note that the steps of these functions are flipped:

> Object.prototype.hasOwnProperty ( _V_ )
> 
>     1. Let _P_ be ? ToPropertyKey(_V_).
>     2. Let _O_ be ? ToObject(*this* value).
>     ...
> 
> Object.hasOwn ( _O_, _P_ )
> 
>     1. Let _obj_ be ? ToObject(_O_).
>     2. Let _key_ be ? ToPropertyKey(_P_).
>     ...

I also looked for some relevant JavaScriptCore source code:

- `JSObject#hasOwnProperty`: https://github.com/WebKit/WebKit/blob/e40702b9a8a6ff87d4e9c4edd80b8e07090b3540/Source/JavaScriptCore/runtime/JSObject.h#L662-L664
- `JSObjectConstructor`: https://github.com/WebKit/WebKit/blob/800d33d28d0b267466a3f6daec5fa5056e72348d/Source/JavaScriptCore/runtime/ObjectConstructor.cpp

[Jamie Kyle]

There are also test262 tests for Object.hasOwn() here: https://github.com/tc39/test262/pull/2995

[Radar WebKit Bug Importer]

<rdar://problem/78782545>

[Jamie Kyle]

FYI the test262 tests have been merged https://github.com/tc39/test262/pull/2995

[Yusuke Suzuki]

Implementation note: When implementing it, wire HasOwnPropertyCache to this implementation too :)

[Yusuke Suzuki]

And please add Option flag to disable this by default.

[Aditi Singh]

Created attachment 434828 [details]
Patch

[Aditi Singh]

Created attachment 434829 [details]
Patch

[Alexey Shvayka]

Comment on attachment 434828 [details]
Patch

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

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:1015
> +bool objectConstructorHasOwn(JSGlobalObject* globalObject, JSValue base, const Identifier& propertyName)

I might be missing something, but this method is identical to objectPrototypeHasOwnProperty().
If so, can we just use objectPrototypeHasOwnProperty() directly?

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:1046
> +    JSValue base = callFrame->argument(0).toObject(globalObject);

JSObject* base = callFrame->argument(0).toObject(globalObject);
RETURN_IF_EXCEPTION(scope, { });

If first argument is `undefined` or `null`, toObject() will throw an exception that we should handled before toPropertyKey(), which is observable.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:1048
> +    RETURN_IF_EXCEPTION(scope, encodedJSValue());

nit: RETURN_IF_EXCEPTION(scope, { });

[Aditi Singh]

Created attachment 434830 [details]
Patch

[Aditi Singh]

(In reply to Alexey Shvayka from comment #8)
> Comment on attachment 434828 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434828&action=review
> 
> > Source/JavaScriptCore/runtime/ObjectConstructor.cpp:1015
> > +bool objectConstructorHasOwn(JSGlobalObject* globalObject, JSValue base, const Identifier& propertyName)
> 
> I might be missing something, but this method is identical to
> objectPrototypeHasOwnProperty().
> If so, can we just use objectPrototypeHasOwnProperty() directly?

Yeah, it's exactly similar to objectPrototypHasOwnProperty()

> > Source/JavaScriptCore/runtime/ObjectConstructor.cpp:1046
> > +    JSValue base = callFrame->argument(0).toObject(globalObject);
> 
> JSObject* base = callFrame->argument(0).toObject(globalObject);
> RETURN_IF_EXCEPTION(scope, { });
> 
> If first argument is `undefined` or `null`, toObject() will throw an
> exception that we should handled before toPropertyKey(), which is observable.
> 
> > Source/JavaScriptCore/runtime/ObjectConstructor.cpp:1048
> > +    RETURN_IF_EXCEPTION(scope, encodedJSValue());
> 
> nit: RETURN_IF_EXCEPTION(scope, { });

Okay, I will include it!

[Aditi Singh]

I don't understand why the build is failing, it works fine when I compile with Tools/Scripts/build-jsc --jsc-only --debug.

[Alexey Shvayka]

(In reply to Aditi Singh from comment #11)
> I don't understand why the build is failing, it works fine when I compile
> with Tools/Scripts/build-jsc --jsc-only --debug.

I believe it's due to definition of `bool objectConstructorHasOwn(JSGlobalObject* globalObject, JSValue base, const Identifier& propertyName)`. It conflicts with the other objectConstructorHasOwn. Removing it and calling objectPrototypeHasOwnProperty() would surely fix this.

Also, it would be nice to refine objectPrototypeHasOwnProperty() to accept JSObject* base rather than JSValue.

[Aditi Singh]

Okay, sure! Thanks :)

[Yusuke Suzuki]

Comment on attachment 434830 [details]
Patch

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

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:1017
> +bool objectConstructorHasOwn(JSGlobalObject* globalObject, JSValue base, const Identifier& propertyName)

I think we do not need to define this function. Let's just implement it as one host function.

[Yusuke Suzuki]

Comment on attachment 434830 [details]
Patch

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

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:84
> +  hasOwn                    objectConstructorHasOwn                     DontEnum|Function 2 

And please hide this function with runtime option, and disabling it for now.

[Aditi Singh]

Created attachment 436076 [details]
Patch

[Alexey Shvayka]

Comment on attachment 436076 [details]
Patch

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

Great job, Aditi! r=me with few nits, rebase, and ChangeLog with proposal link. Please reupload with cq? flag then.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1024
>      JSString* string = asString(property);

for/in was recently reengineered, this op is now called "slow_path_enumerator_has_own_property".

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1027
> +    JSValue thisValue = base.toThis(globalObject, ECMAMode::strict());

👍 for preserving the previous behavior; on such scale, even a smallest change may break apps / sites.

nit: I am pretty sure toThis() call here is unnecessary (same goes for DFGOperations.cpp): we don't emit HasOwnPropertyFunctionCallDotNode for bases that need it. Not sure if we should add it, but please note toThis(..., ECMAMode::strict()) will be soon removed in https://bugs.webkit.org/show_bug.cgi?id=225397.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:1024
> +    RETURN_IF_EXCEPTION(scope, encodedJSValue());

nit: RETURN_IF_EXCEPTION(scope, { });

> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:96
> +    Structure* structure = object->structure(vm);

nit: WebKit project has a nice rule of "not making unnecessary changes" to keep diffs / history cleaner. I appreciate it during every `git blame`. If I was making this change, I would have just keep "thisObject" name.

[Alexey Shvayka]

Comment on attachment 436076 [details]
Patch

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

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:1021
> +    JSObject* base = callFrame->argument(0).toObject(globalObject);

Another non-blocking comment on test coverage.
Unlike Object.hasOwn, Object.prototype.hasOwnProperty coerces its property key first, before |this| value.
Can you please make sure this subtle difference is covered by test262 suite? The easiest way would be swapping L1021 and L1023 and checking if there are test failures. If there aren't, I would appreciate you filing an issue on GitHub. This will ensure other implementations are correct as well.

[Jamie Kyle]

test262 does include tests for the order of `ToObject` and `ToPropertyKey`:

- https://github.com/tc39/test262/blob/2c4f2665ec86f01bff7e42d3a5b54c9a09f9a362/test/built-ins/Object/prototype/hasOwnProperty/topropertykey_before_toobject.js
- https://github.com/tc39/test262/blob/2c4f2665ec86f01bff7e42d3a5b54c9a09f9a362/test/built-ins/Object/hasOwn/toobject_before_topropertykey.js

[Aditi Singh]

Created attachment 436283 [details]
Patch

[Aditi Singh]

Created attachment 436284 [details]
Patch

[Alexey Shvayka]

Comment on attachment 436284 [details]
Patch

(In reply to Jamie Kyle from comment #19)
> test262 does include tests for the order of `ToObject` and `ToPropertyKey`:

Jamie, thanks for all the work on this proposal, and especially for thorough test coverage!


Aditi, 🎉 on the first patch, well done!
A bugzilla tip for future: once a patch is reviewed, please edit ChangeLogs with reviewer's name and upload follow-ups w/o r? flag.

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 436284 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Alexey Shvayka]

Comment on attachment 436284 [details]
Patch

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

Ah, since a rebase is needed before landing, there are a few small tweaks I may suggest:

> Source/JavaScriptCore/ChangeLog:4
> +        Proposal Link: https://github.com/tc39/proposal-accessible-object-hasownproperty

Please move this line below "Reviewed by" for the logs to have a consistent header.

> Source/JavaScriptCore/builtins/BuiltinNames.h:186
> +    macro(hasOwn)

Please add trailing "\" (like `macro(hasOwn) \`) to make future additions a bit cleaner.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:2556
> +    JSObject* thisObject = base.toObject(globalObject);

Let's call this `baseObject`.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1095
> +    JSObject* thisObject = baseValue.toObject(globalObject);

Let's call this `baseObject`.

> Source/JavaScriptCore/runtime/OptionsList.h:544
> +    v(Bool, useHasOwn, false, Normal, "Expose the Object.hasOwn method")

Trailing `\` would be nice.

[Yusuke Suzuki]

Comment on attachment 436284 [details]
Patch

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

>> Source/JavaScriptCore/builtins/BuiltinNames.h:186
>> +    macro(hasOwn)
> 
> Please add trailing "\" (like `macro(hasOwn) \`) to make future additions a bit cleaner.

Can you define it in CommonIdentifiers instead? Since hasOwn does not need to have PrivateName. (BuiltinNames v.s. CommonIdentifiers is, BuiltinNames defines PrivateName too).

> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:123
> +    EXCEPTION_ASSERT(!!scope.exception() == !thisObject);
> +    if (UNLIKELY(!thisObject))
> +        return false;

RETURN_IF_EXCEPTION(scope, { }); is better here :)

[Aditi Singh]

Created attachment 436795 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 436795 [details]
Patch

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

> Source/JavaScriptCore/runtime/CommonIdentifiers.h:235
> +    macro(hasOwn) \

This list is sorted in alphabetical ordering. So please place it in the correct place :)

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:1026
> +    scope.release();
> +    return JSValue::encode(jsBoolean(objectPrototypeHasOwnProperty(globalObject, base, propertyName)));

Use `RELEASE_AND_RETURN(scope, JSValue::encode(jsBoolean(objectPrototypeHasOwnProperty(globalObject, base, propertyName))));`.

[Yusuke Suzuki]

Comment on attachment 436795 [details]
Patch

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

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:1025
> +    scope.release();

Remove this and use RELEASE_AND_RETURN.

> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:123
>      scope.release();
> -    return JSValue::encode(jsBoolean(objectPrototypeHasOwnProperty(globalObject, base, propertyName)));
> +    return JSValue::encode(jsBoolean(objectPrototypeHasOwnProperty(globalObject, thisObject, propertyName)));

Remove scope.release(), and use `RELEASE_AND_RETURN(scope, JSValue::encode(jsBoolean(objectPrototypeHasOwnProperty(globalObject, thisObject, propertyName))));`.

[Aditi Singh]

Created attachment 436865 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 436865 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        Reviewed by Alexey Shvayka.
> +
> +        Proposal Link: https://github.com/tc39/proposal-accessible-object-hasownproperty
> +

Please add descriptive information in ChangeLog.
In WebKit, we require enough information in ChangeLog.
The other part looks good to me :)

[Aditi Singh]

Created attachment 436871 [details]
Patch

[Aditi Singh]

Is it okay now? :)

[Yusuke Suzuki]

Comment on attachment 436871 [details]
Patch

r=me

[EWS]

Committed r281799 (241136@main): <https://commits.webkit.org/241136@main>

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