1208464 - Implement proposed ES7 functions Object.values and Object.entries

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

See http://ljharb.github.io/proposal-object-values-entries/ for the proposed spec, https://github.com/rwaldron/tc39-notes/blob/c61f48cea5f2339a1ec65ca89827c8cff170779b/es6/2014-04/apr-9.md#51-objectentries-objectvalues for tc39 discussions, and https://github.com/ljharb/proposal-object-values-entries for more details.

Comment 1

[Till Schneidereit [:till]]

Attachment: Implement proposed ES7 functions Object.values and Object.entries

I made this Nightly- and developer edition-only for now. It seems unlikely to me that it'll change in substantial ways, but it's only at stage 2 at this point.

Comment 2

[ljharb]

Awesome! My plan is to advance it to stage 3 at the November meeting, and as soon as it lands in two non-nightly browsers, stage 4 - which will hopefully be ready before the January meeting.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Do we do Intent to Implements for JS?

Comment 4

[ljharb]

Comment on attachment 8665974 [details] [diff] [review]
Implement proposed ES7 functions Object.values and Object.entries

Review of attachment 8665974 [details] [diff] [review]:
-----------------------------------------------------------------

This is awesome, thanks for doing it so quickly!!

My only comment is that the proposal explicitly changes the abstract `EnumerableOwnNames` operation which returns keys only (presumably called `OwnPropertyKeys` here) to an abstract `EnumerableOwnProperties` abstract operation that takes an enum to return keys, values, or entries as appropriate in a single iteration. Is it worth attempting the same efficiency here?

::: js/src/builtin/Object.js
@@ +139,5 @@
> +    // Steps 1-2.
> +    var object = ToObject(O);
> +    // Steps 3-4.
> +    // EnumerableOwnProperties is inlined here.
> +    var keys = OwnPropertyKeys(object, JSITER_OWNONLY);

Would JSITER_FOREACH or JSITER_KEYVALUE work here, to avoid the need for an extra iteration to collect the values?

@@ +155,5 @@
> +    // Steps 1-2.
> +    var object = ToObject(O);
> +    // Steps 3-4.
> +    // EnumerableOwnProperties is inlined here.
> +    var keys = OwnPropertyKeys(object, JSITER_OWNONLY);

Would JSITER_KEYVALUE work here, to avoid the need for an extra iteration to collect the values?

Comment 5

[Tom S [:evilpie]]

> Would JSITER_FOREACH or JSITER_KEYVALUE work here
No. They can only be used with the deprecated Iterator() function, or for each loops.

Comment 6

[Till Schneidereit [:till]]

(In reply to :Ms2ger from comment #3)
> Do we do Intent to Implements for JS?

Not really. Maybe we should, but we haven't ever done so until now.

(In reply to ljharb from comment #4)
> My only comment is that the proposal explicitly changes the abstract
> `EnumerableOwnNames` operation which returns keys only (presumably called
> `OwnPropertyKeys` here) to an abstract `EnumerableOwnProperties` abstract
> operation that takes an enum to return keys, values, or entries as
> appropriate in a single iteration. Is it worth attempting the same
> efficiency here?

I doubt it matters too much, performance-wise. However, I just realized that it's needed for correctness: if a not-yet-visited property is deleted during the second loop in `values` and `entries`, it shouldn't be contained in the result.

Comment 7

[ljharb]

> I doubt it matters too much, performance-wise. However, I just realized that
> it's needed for correctness: if a not-yet-visited property is deleted during
> the second loop in `values` and `entries`, it shouldn't be contained in the
> result.

Since JS is single-threaded, how could a call to `keys` and an immediate lookup of the values cause a deleted key's value to appear?

Comment 8

[Till Schneidereit [:till]]

(In reply to ljharb from comment #7)
> Since JS is single-threaded, how could a call to `keys` and an immediate
> lookup of the values cause a deleted key's value to appear?

It couldn't. But a getter for one property can delete another property:

var o = {get a() {delete this.b; return 1}, b: 2, c: 3};
var keys = Object.keys(o);
var entries = keys.map(key => o[key]);

This results in [1, undefined, 3], but should result in [1, 3]. The current implementation (just as the polyfill, IINM) has the same issue.

Comment 9

[ljharb]

Good point! I'll update the polyfill to handle this case as well.

Comment 10

[Till Schneidereit [:till]]

Attachment: Implement proposed ES7 functions Object.values and Object.entries. v2

Comment 11

[Till Schneidereit [:till]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00376862db4d

Comment 12

[ljharb]

All polyfills have been updated and published to include this fix. https://github.com/ljharb/proposal-object-values-entries/issues/4

Comment 13

[André Bargull [:anba]]

Comment on attachment 8666013 [details] [diff] [review]
Implement proposed ES7 functions Object.values and Object.entries. v2

Review of attachment 8666013 [details] [diff] [review]:
-----------------------------------------------------------------

The enumerable check should happen in the loop to handle things like:

  Object.values({get a(){ Object.defineProperty(this, "b", {enumerable: false}); return "A"; }, b: "B"});

That call should return ["A"], but currently returns ["A", "B"].

::: js/src/builtin/Object.js
@@ +139,5 @@
> +    // Steps 1-2.
> +    var object = ToObject(O);
> +    // Steps 3-4.
> +    // EnumerableOwnProperties is inlined here.
> +    var keys = OwnPropertyKeys(object, JSITER_OWNONLY);

var keys = OwnPropertyKeys(from, JSITER_OWNONLY | JSITER_HIDDEN);

@@ +144,5 @@
> +    var values = [];
> +    var valuesCount = 0;
> +    for (var i = 0; i < keys.length; i++) {
> +        var key = keys[i];
> +        if (!callFunction(std_Object_hasOwnProperty, object, key))

if (!callFunction(std_Object_propertyIsEnumerable, object, key))

@@ +159,5 @@
> +    // Steps 1-2.
> +    var object = ToObject(O);
> +    // Steps 3-4.
> +    // EnumerableOwnProperties is inlined here.
> +    var keys = OwnPropertyKeys(object, JSITER_OWNONLY);

var keys = OwnPropertyKeys(from, JSITER_OWNONLY | JSITER_HIDDEN);

@@ +164,5 @@
> +    var entries = [];
> +    var entriesCount = 0;
> +    for (var i = 0; i < keys.length; i++) {
> +        var key = keys[i];
> +        if (!callFunction(std_Object_hasOwnProperty, object, key))

if (!callFunction(std_Object_propertyIsEnumerable, object, key))

Comment 14

[ljharb]

I'm not familiar with the source, but presuming that `std_Object_propertyIsEnumerable` returns `false` for a nonexistent property, that seems like a perfect change :-D Thanks for reviewing this, André!

Comment 15

[Tom S [:evilpie]]

Comment on attachment 8666013 [details] [diff] [review]
Implement proposed ES7 functions Object.values and Object.entries. v2

I understand Till wanted to update this patch after Andre's feedback.

Comment 16

[ljharb]

WebKit Nightly patch: https://bugs.webkit.org/show_bug.cgi?id=150131

Comment 17

[ljharb]

As of today, this proposal is now at stage 3! How soon do you think we can land it in Firefox Stable? :-)

It needs to be shipped in at least two browsers by the January meeting, and I'd really like to see that happen.

Comment 18

[Till Schneidereit [:till]]

Attachment: Implement proposed ES7 functions Object.values and Object.entries. v2

My apologies for the long delay. Between a broken collarbone and other work, this fell off my radar for a while. :(

André, as usually, thanks a ton for the feedback!

Comment 19

[Tom S [:evilpie]]

Comment on attachment 8689746 [details] [diff] [review]
Implement proposed ES7 functions Object.values and Object.entries. v2

Review of attachment 8689746 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Object.js
@@ +137,5 @@
> +// Draft proposal http://ljharb.github.io/proposal-object-values-entries/#Object.values
> +function ObjectValues(O) {
> +    // Steps 1-2.
> +    var object = ToObject(O);
> +    // Steps 3-4.

Please give the code some breathing room. At least add a blank line before comments.

@@ +147,5 @@
> +        var key = keys[i];
> +        if (!callFunction(std_Object_propertyIsEnumerable, object, key))
> +            continue;
> +        var value = object[key];
> +        values[valuesCount++] = value;

This should be _DefineDataProperty, please add a test with Object.defineProperty(Array.prototype, 0, {set() .. }}.

@@ +153,5 @@
> +    // Step 5.
> +    return values;
> +}
> +
> +// Draft proposal http://ljharb.github.io/proposal-object-values-entries/#Object.entries

https://github.com/tc39/proposal-object-values-entries

::: js/src/builtin/SelfHostingDefines.h
@@ +52,5 @@
>  
> +// NB: keep these in sync with the copy in jsfriendapi.h.
> +#define JSITER_ENUMERATE  0x1   /* for-in compatible hidden default iterator */
> +#define JSITER_FOREACH    0x2   /* get obj[key] for each property */
> +#define JSITER_KEYVALUE   0x4   /* obsolete destructuring for-in wants [key, value] */

I don't think we should even expose the first three. They are wrong/dangerous.

::: js/src/tests/ecma_7/Object/entries.js
@@ +68,5 @@
> +            return "A";
> +        },
> +        b: "B"
> +    });
> +    assertDeepEq(values, [["a", "A"]]);

At least one test with Proxy checking ownKeys and getOwnPropertyDescriptor are used. (Same for values.js)

Comment 20

[Till Schneidereit [:till]]

Attachment: Implement proposed ES7 functions Object.values and Object.entries

(In reply to Tom Schuster [:evilpie] from comment #19)
> Please give the code some breathing room. At least add a blank line before
> comments.

I've come to like the slightly more compressed style recently, but it's not a strong preference, so there's more space, now.

> > +// Draft proposal http://ljharb.github.io/proposal-object-values-entries/#Object.entries
> 
> https://github.com/tc39/proposal-object-values-entries

I used the rendered version and deep-linked to the actual spec algorithms here and below.

> > +// NB: keep these in sync with the copy in jsfriendapi.h.
> > +#define JSITER_ENUMERATE  0x1   /* for-in compatible hidden default iterator */
> > +#define JSITER_FOREACH    0x2   /* get obj[key] for each property */
> > +#define JSITER_KEYVALUE   0x4   /* obsolete destructuring for-in wants [key, value] */
> 
> I don't think we should even expose the first three. They are
> wrong/dangerous.

I guess that makes sense, yes.

Comment 21

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4121f4a9058de38c509a7176e9fe446786e19b77
Bug 1208464 - Implement proposed ES7 functions Object.values and Object.entries. r=evilpie

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4121f4a9058d

Comment 23

[ljharb]

Awesome, thanks till! I have a first attempt at test262 tests up here https://github.com/tc39/test262/compare/master...ljharb:object_values_entries for anyone who's interested in trying to run these against the implementation.

Comment 24

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/45#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/entries
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/values

Comment 25

[Tooru Fujisawa [:arai]]

Object.values and Object.entries are non-release-only.
I've changed the compat table in the documents to nightly-only for now.

Comment 26

[ljharb]

Why? It's stage 3, and as soon as it's implemented in two non-nightly browsers, it will hit stage 4 - hopefully in January. Stage 3 proposal items are intended for browsers to ship in a stable release, according to the post-ES6 process.

Comment 27

[Till Schneidereit [:till]]

One reason for keeping it in non-release builds for now would be performance. The current implementation is going to be pretty slow compared to what a (non-compliant) polyfill could do, because it does a non-JIT-inlined property flag check in each iteration. While that can be fixed, I'm not sure it will happen in this release cycle. The reason I'd like to either have these functions be fast or not exist (in release builds) at all is that a slow implementation might cause developers to file them away as useless.

I filed bug 1228981 for making them fast.

Comment 28

[Kohei Yoshino]

Then why the poor performance for...of statement has been available in release builds for years? (Bug 874580)

Comment 29

[Till Schneidereit [:till]]

It's a trade-off between providing value to developers quickly or providing the best value. Optimizing for..of is far from trivial, so delaying it until every bit of performance has been squeezed out of it wouldn't make much sense. Additionally, for..of was much further along in the specification process and not very new, like Object.values and .entries are.

Finally, note that for..of has become faster over time - to the point where it's the fastest available option in some cases. See bug 1129313 comment 12.

Comment 30

[Kohei Yoshino]

How about classes? It's not optimized yet but are you shipping it now? (Bug 1197932) What an inconsistent policy!

Comment 31

[Jeff Walden [:Waldo]]

These things are certainly debatable.

In the case of classes, their organizational benefits may outweigh the demerits of their being less speedy than they could be, in the interim before they're optimized.  In many cases classes won't be on fast paths, so lessened optimizations may not bite some people.

And as a matter of market realities, other engines *are* shipping classes.  That agitates for quickly shipping classes once they're correct, even if they won't be 100% speedy at that time.

In general, tho, these arguments are not cut-and-dried.  The choice in each case is debatable.  Trying to hold *any* particular engine to some ideal of consistency is never going to be very successful.  There's some merit to the particular comparison you make, but the case isn't as indisputably strong as you make it out to be.

Comment 32

[Jeff Walden [:Waldo]]

Also, as regards these two functions, if I understand them correctly, they're self-implementable now if you want them.  Maybe less perf, but you can use them if you'd like.  Classes *must* be provided by the engine as a practical matter if you want to use them in shipping code.  In general, I think it makes a lot of sense to be more aggressive about shipping language features that user code *can't* emulate, than it does to be aggressive about shipping things that can be worked around without engine support.

Comment 33

[Jeff Walden [:Waldo]]

Oh, and while I'm throwing on reasons.  Classes *are* standardized, fully, set in stone in ES6.  These two functions are still only proposals, not necessarily finalized, not in a published standard.  Different rules of thumb seem only natural.

Comment 34

[Jason Orendorff [:jorendorff]]

For the most part, we ship features when they're correct and not wait on optimization. That's what we did with arrow functions, for-of, classes, and generators. There's no fallout I'm aware of from this policy.

Literally the only criticism I have ever heard of for-of's amazing slowness is from SM and V8 JS-engine developers. Not from people using the language to build stuff.

About the other features I've heard nothing negative at all.

Object.values() and Object.entries() are a special case because the implementations we've landed are no better than the polyfill that users are going to have to use anyway. I'll regret saying this if the optimization work doesn't get done so we can ship this soon --- but holding this back is the right call for now.

Comment 35

[ljharb]

I'd be very happy if one of the first implementations of this proposal was launched such that it was performant :-) but, if the optimization work isn't done soon, Firefox won't be the first implementation ¯\_(ツ)_/¯

jorendorff: unless the implementation is worse than the polyfill, imo it's still beneficial to include them, because it starts the clock for when the polyfill will no longer be needed, and encourages other implementations to move more quickly.

Thanks to till for implementing it and to whoever hopefully does the optimization work soon!