Bug 188481 – Implement Object.fromEntries

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Jordan Harband]

Object.fromEntries is a stage 3 proposal: https://github.com/tc39/proposal-object-from-entries

It'd be great to get it implemented in Webkit/Safari soon!

Test262 tests are here: https://github.com/tc39/test262/pull/1676

Ref: https://github.com/tc39/proposal-object-from-entries/issues/21

[Yusuke Suzuki]

Created attachment 346964 [details]
Patch

[Yusuke Suzuki]

Created attachment 346965 [details]
Patch

[Yusuke Suzuki]

Created attachment 346967 [details]
Patch

[Jordan Harband]

Comment on attachment 346967 [details]
Patch

Thanks, this looks great!

[Darin Adler]

Here are the kinds of things I think about when adding tests for an operation like this one:

Imagine that someone is trying to optimize the implementation of this and write the function some different way, not in JavaScript. I want tests to cover what that person might get wrong, such as:

1) Verify no additional properties, including but not limited to "length", are "get" and no properties "set" on the iterable passed in and on the objects it returns.

2) Verify that prototypes are consulted properly when properties are missing on the objects returned by the iterable. Perhaps we'd want to test a prototype chain of more than one?

3) Verify that the operations are done in the order mentioned in the specification, for example, get "0" before "1"; this typically requires passing objects that produce side effects from the "get" operation. But even for tests that involve prototypes, test that operations are done in the correct order.

3a) Verify that exceptions raised are propagated correctly.

4) Verify that the behavior is correct when the object has no "0" or "1" property; related to the prototype tests, but the key is that lack of property turns into "undefined", if that’s the correct behavior.

5) Verify that there is no accidental transformation of either keys or values: null doesn't become undefined or vice versa, no conversion to string, etc.

6) Check what goes on in exotic cases like where the Object prototype has a non-standard value.

I’d like to see at least some of these tests be a standard practice when we add JavaScript functions. A major goal of these tests is to protect us from behavior changes if we later decide to do a different implementation. Also very useful for checking across other JavaScript implementations.

[Jordan Harband]

There's existing test262 tests; can that cover it, or does webkit need its own separate tests?

[Darin Adler]

(In reply to Jordan Harband from comment #6)
> There's existing test262 tests

Great.

> can that cover it, or does webkit need its own separate tests?

Sure, those are excellent, as long as we check them in to the WebKit tree and run them. This patch doesn’t show me new successes on any tests so I assume that means they are not in the tree yet.

If the test262 tests don’t cover all the things I suggested above, then I suggest we add coverage to them.

[Yusuke Suzuki]

(In reply to Darin Adler from comment #5)
> Here are the kinds of things I think about when adding tests for an
> operation like this one:
> 
> Imagine that someone is trying to optimize the implementation of this and
> write the function some different way, not in JavaScript. I want tests to
> cover what that person might get wrong, such as:
> 
> 1) Verify no additional properties, including but not limited to "length",
> are "get" and no properties "set" on the iterable passed in and on the
> objects it returns.
> 
> 2) Verify that prototypes are consulted properly when properties are missing
> on the objects returned by the iterable. Perhaps we'd want to test a
> prototype chain of more than one?
> 
> 3) Verify that the operations are done in the order mentioned in the
> specification, for example, get "0" before "1"; this typically requires
> passing objects that produce side effects from the "get" operation. But even
> for tests that involve prototypes, test that operations are done in the
> correct order.
> 
> 3a) Verify that exceptions raised are propagated correctly.
> 
> 4) Verify that the behavior is correct when the object has no "0" or "1"
> property; related to the prototype tests, but the key is that lack of
> property turns into "undefined", if that’s the correct behavior.
> 
> 5) Verify that there is no accidental transformation of either keys or
> values: null doesn't become undefined or vice versa, no conversion to
> string, etc.
> 
> 6) Check what goes on in exotic cases like where the Object prototype has a
> non-standard value.
> 
> I’d like to see at least some of these tests be a standard practice when we
> add JavaScript functions. A major goal of these tests is to protect us from
> behavior changes if we later decide to do a different implementation. Also
> very useful for checking across other JavaScript implementations.

Cool, I'll update the patch.

(In reply to Jordan Harband from comment #4)
> Comment on attachment 346967 [details]
> Patch
> 
> Thanks, this looks great!

Thanks for your informal review :)
And please do not set r+ for the patch in informal review since r+ from reviewers is only effective.

[Jordan Harband]

whoops, sorry :-) i assumed it worked like github, where it only counts +r's from people with the collaborator bit.

[Yusuke Suzuki]

(In reply to Jordan Harband from comment #9)
> whoops, sorry :-) i assumed it worked like github, where it only counts +r's
> from people with the collaborator bit.

No problem!! :D

[Darin Adler]

Oh, I guess I didn't need to set review+ because Yusuke will upload a new version for review soon.

[Yusuke Suzuki]

(In reply to Darin Adler from comment #11)
> Oh, I guess I didn't need to set review+ because Yusuke will upload a new
> version for review soon.

Oops, sorry. I was working on promise rejection in workers and some JSC issues found by UBSan :)
I'll land it with suggested tests!

[Darin Adler]

Did not mean to rush you at all. Just didn’t want to leave you hanging.

[Saam Barati]

Comment on attachment 346967 [details]
Patch

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

LGTM too, just a couple of suggestions.

> Source/JavaScriptCore/builtins/ObjectConstructor.js:48
> +    var object = {};

Style nit: Maybe use “let” in this function?

> Source/JavaScriptCore/builtins/ObjectConstructor.js:50
> +    for (var entry of iterable) {

Can we add tests for all the effects this function does?
- Iterator protocol
- Get on key 0 and 1

[Yusuke Suzuki]

Comment on attachment 346967 [details]
Patch

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

Thanks, I've added suggested tests.

>> Source/JavaScriptCore/builtins/ObjectConstructor.js:48
>> +    var object = {};
> 
> Style nit: Maybe use “let” in this function?

Changed.

>> Source/JavaScriptCore/builtins/ObjectConstructor.js:50
>> +    for (var entry of iterable) {
> 
> Can we add tests for all the effects this function does?
> - Iterator protocol
> - Get on key 0 and 1

Added both!

[Yusuke Suzuki]

Committed r235589: <https://trac.webkit.org/changeset/235589>

[Radar WebKit Bug Importer]

<rdar://problem/44052565>