Bug 188481 - Implement Object.fromEntries
Summary: Implement Object.fromEntries
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-10 22:33 PDT by Jordan Harband
Modified: 2018-09-12 04:37 PDT (History)
11 users (show)

See Also:


Attachments
Patch (6.45 KB, patch)
2018-08-11 12:17 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (6.81 KB, patch)
2018-08-11 12:18 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (10.93 KB, patch)
2018-08-11 12:55 PDT, Yusuke Suzuki
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jordan Harband 2018-08-10 22:33:34 PDT
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
Comment 1 Yusuke Suzuki 2018-08-11 12:17:45 PDT
Created attachment 346964 [details]
Patch
Comment 2 Yusuke Suzuki 2018-08-11 12:18:59 PDT
Created attachment 346965 [details]
Patch
Comment 3 Yusuke Suzuki 2018-08-11 12:55:17 PDT
Created attachment 346967 [details]
Patch
Comment 4 Jordan Harband 2018-08-12 22:26:09 PDT
Comment on attachment 346967 [details]
Patch

Thanks, this looks great!
Comment 5 Darin Adler 2018-08-13 10:32:50 PDT
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.
Comment 6 Jordan Harband 2018-08-13 11:07:27 PDT
There's existing test262 tests; can that cover it, or does webkit need its own separate tests?
Comment 7 Darin Adler 2018-08-13 17:05:36 PDT
(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.
Comment 8 Yusuke Suzuki 2018-08-14 05:11:16 PDT
(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.
Comment 9 Jordan Harband 2018-08-14 11:25:57 PDT
whoops, sorry :-) i assumed it worked like github, where it only counts +r's from people with the collaborator bit.
Comment 10 Yusuke Suzuki 2018-08-14 11:27:27 PDT
(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
Comment 11 Darin Adler 2018-08-15 12:40:18 PDT
Oh, I guess I didn't need to set review+ because Yusuke will upload a new version for review soon.
Comment 12 Yusuke Suzuki 2018-08-15 13:05:26 PDT
(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!
Comment 13 Darin Adler 2018-08-15 13:10:56 PDT
Did not mean to rush you at all. Just didn’t want to leave you hanging.
Comment 14 Saam Barati 2018-08-15 14:25:34 PDT
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
Comment 15 Yusuke Suzuki 2018-09-02 09:38:55 PDT
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!
Comment 16 Yusuke Suzuki 2018-09-02 09:41:50 PDT
Committed r235589: <https://trac.webkit.org/changeset/235589>
Comment 17 Radar WebKit Bug Importer 2018-09-02 09:42:18 PDT
<rdar://problem/44052565>