Bug 226291 - Implement Object.hasOwn
Summary: Implement Object.hasOwn
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aditi Singh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-26 14:15 PDT by Jamie Kyle
Modified: 2021-08-31 05:48 PDT (History)
13 users (show)

See Also:


Attachments
Patch (3.03 KB, patch)
2021-08-03 06:59 PDT, Aditi Singh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (3.95 KB, patch)
2021-08-03 07:09 PDT, Aditi Singh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (4.07 KB, patch)
2021-08-03 07:18 PDT, Aditi Singh
no flags Details | Formatted Diff | Diff
Patch (10.31 KB, patch)
2021-08-21 10:00 PDT, Aditi Singh
no flags Details | Formatted Diff | Diff
Patch (9.44 KB, patch)
2021-08-24 07:22 PDT, Aditi Singh
no flags Details | Formatted Diff | Diff
Patch (9.45 KB, patch)
2021-08-24 07:27 PDT, Aditi Singh
no flags Details | Formatted Diff | Diff
Patch (9.72 KB, patch)
2021-08-30 10:56 PDT, Aditi Singh
no flags Details | Formatted Diff | Diff
Patch (9.72 KB, patch)
2021-08-31 03:04 PDT, Aditi Singh
no flags Details | Formatted Diff | Diff
Patch (10.02 KB, patch)
2021-08-31 03:47 PDT, Aditi Singh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jamie Kyle 2021-05-26 14:15:32 PDT
`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
Comment 1 Jamie Kyle 2021-05-26 14:17:27 PDT
There are also test262 tests for Object.hasOwn() here: https://github.com/tc39/test262/pull/2995
Comment 2 Radar WebKit Bug Importer 2021-06-02 14:16:19 PDT
<rdar://problem/78782545>
Comment 3 Jamie Kyle 2021-06-16 15:41:32 PDT
FYI the test262 tests have been merged https://github.com/tc39/test262/pull/2995
Comment 4 Yusuke Suzuki 2021-07-06 14:19:01 PDT
Implementation note: When implementing it, wire HasOwnPropertyCache to this implementation too :)
Comment 5 Yusuke Suzuki 2021-07-07 15:41:39 PDT
And please add Option flag to disable this by default.
Comment 6 Aditi Singh 2021-08-03 06:59:19 PDT
Created attachment 434828 [details]
Patch
Comment 7 Aditi Singh 2021-08-03 07:09:02 PDT
Created attachment 434829 [details]
Patch
Comment 8 Alexey Shvayka 2021-08-03 07:12:45 PDT
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, { });
Comment 9 Aditi Singh 2021-08-03 07:18:07 PDT
Created attachment 434830 [details]
Patch
Comment 10 Aditi Singh 2021-08-03 07:32:35 PDT
(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!
Comment 11 Aditi Singh 2021-08-03 07:58:10 PDT
I don't understand why the build is failing, it works fine when I compile with Tools/Scripts/build-jsc --jsc-only --debug.
Comment 12 Alexey Shvayka 2021-08-03 08:03:36 PDT
(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.
Comment 13 Aditi Singh 2021-08-03 08:15:54 PDT
Okay, sure! Thanks :)
Comment 14 Yusuke Suzuki 2021-08-03 12:40:49 PDT
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.
Comment 15 Yusuke Suzuki 2021-08-03 14:30:45 PDT
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.
Comment 16 Aditi Singh 2021-08-21 10:00:58 PDT
Created attachment 436076 [details]
Patch
Comment 17 Alexey Shvayka 2021-08-21 10:32:32 PDT
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.
Comment 18 Alexey Shvayka 2021-08-21 11:17:39 PDT
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.
Comment 20 Aditi Singh 2021-08-24 07:22:49 PDT
Created attachment 436283 [details]
Patch
Comment 21 Aditi Singh 2021-08-24 07:27:49 PDT
Created attachment 436284 [details]
Patch
Comment 22 Alexey Shvayka 2021-08-25 10:49:08 PDT
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.
Comment 23 EWS 2021-08-25 10:49:53 PDT
Tools/Scripts/svn-apply failed to apply attachment 436284 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 24 Alexey Shvayka 2021-08-25 11:11:06 PDT
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.
Comment 25 Yusuke Suzuki 2021-08-25 12:23:36 PDT
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 :)
Comment 26 Aditi Singh 2021-08-30 10:56:57 PDT
Created attachment 436795 [details]
Patch
Comment 27 Yusuke Suzuki 2021-08-30 11:02:51 PDT
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))));`.
Comment 28 Yusuke Suzuki 2021-08-30 11:03:42 PDT
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))));`.
Comment 29 Aditi Singh 2021-08-31 03:04:25 PDT
Created attachment 436865 [details]
Patch
Comment 30 Yusuke Suzuki 2021-08-31 03:16:04 PDT
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 :)
Comment 31 Aditi Singh 2021-08-31 03:47:28 PDT
Created attachment 436871 [details]
Patch
Comment 32 Aditi Singh 2021-08-31 03:47:56 PDT
Is it okay now? :)
Comment 33 Yusuke Suzuki 2021-08-31 05:19:45 PDT
Comment on attachment 436871 [details]
Patch

r=me
Comment 34 EWS 2021-08-31 05:48:24 PDT
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].