`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
There are also test262 tests for Object.hasOwn() here: https://github.com/tc39/test262/pull/2995
<rdar://problem/78782545>
FYI the test262 tests have been merged https://github.com/tc39/test262/pull/2995
Implementation note: When implementing it, wire HasOwnPropertyCache to this implementation too :)
And please add Option flag to disable this by default.
Created attachment 434828 [details] Patch
Created attachment 434829 [details] Patch
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, { });
Created attachment 434830 [details] Patch
(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!
I don't understand why the build is failing, it works fine when I compile with Tools/Scripts/build-jsc --jsc-only --debug.
(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.
Okay, sure! Thanks :)
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 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.
Created attachment 436076 [details] Patch
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 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.
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
Created attachment 436283 [details] Patch
Created attachment 436284 [details] Patch
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.
Tools/Scripts/svn-apply failed to apply attachment 436284 [details] to trunk. Please resolve the conflicts and upload a new patch.
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 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 :)
Created attachment 436795 [details] Patch
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 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))));`.
Created attachment 436865 [details] Patch
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 :)
Created attachment 436871 [details] Patch
Is it okay now? :)
Comment on attachment 436871 [details] Patch r=me
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].