Implement ES5's Object.keys(O)

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
12 years ago
4 years ago

People

(Reporter: Erik Arvidsson, Assigned: Waldo)

Tracking

({dev-doc-complete})

Trunk
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

12 years ago
Along the lines of improving JS towards a version 1.6 I think it might be useful
to be able get the keys and values as arrays (iterators would have been better).

Basic JS implementation (with the ugly side effects of modifying Object.prototype)

Object.prototype.keys = function () {
   var res = [];
   for (var k in this) [
      res.push(k);
   {
   return res;
};

Object.prototype.values = function () {
   var res = [];
   for each (var v in this) [
      res.push(v);
   {
   return res;
};
Unfortunately, one of those ugly side-effects is that the keys and values user-defined properties appear in the arrays their functions return, so these don't work.

Object.prototype.keys = function () { return [key for (key in this)] }; 
Object.prototype.values = function () { return [val for each (val in this)] };
var foo = { bar: 1, baz: 2 };

foo.keys() ->

  bar,baz,keys,values

foo.values() ->

  1,2,function () {
      return [key for (key in this)];
  },function () {
      return [val for each (val in this)];
  }
These methods should not be on Object.prototype, even if they were made non-enumerable.

Next Monday, the snapshot of the ES4 wiki at http://developer.mozilla.org/es4/ will be updated, and the http://developer.mozilla.org/es4/proposals/iterators_and_generators.html page there will specify iterator::keys and iterator::values functions (also iterator::items), along with other useful stuff.

After that, perhaps this bug should morph into "implement the final ES4/JS2 iterators and generators proposal (sans type system support) for Gecko 1.9 / JS1.8". Comments welcome.

/be
ES5 candidate draft 2009-025 specifies Object.keys(O) in 15.2.3.14; I don't see a values() method anywhere in sight.
Blocks: 445494
Keywords: js1.6
Summary: Add keys() and values() to Object → Implement ES5's Object.keys(O)
(In reply to comment #3)
> ES5 candidate draft 2009-025 specifies Object.keys(O) in 15.2.3.14; I don't see
> a values() method anywhere in sight.

Object.values = function (O) Object.keys(O).map(function(k) O[k]);

Waldo, are you willing to take this bug? Feel free to raise the value issue on es5-discuss. The one-liner above can be done in ES5 of course, if you rewrite the expression closures as full function expressions.

/be
Yeah, I assumed it was part of the package deal for the ES5 stuff I'll be hacking soon.
Assignee: general → jwalden+bmo
Created attachment 399296 [details] [diff] [review]
Short and sweet
Attachment #399296 - Flags: review?(jorendorff)

Comment 7

8 years ago
what should Object.keys(O) do if there's a scriptable iterator? things that make you go hmm.
Same thing as for-in does, which is what JS_Enumerate does, no?  (Note also the fun regarding object-valued indexes, although I'm not sure whether anything actually exposes those for enumeration rather than their being implicitly present with obj.@[p] or obj[p].)

Comment 9

8 years ago
(In reply to comment #8)
> Same thing as for-in does, which is what JS_Enumerate does, no?

iirc, you have to delve into jsiter.h.

There should be tests with this patch, and they should include objects with scriptable iterators.
(In reply to comment #9)
> (In reply to comment #8)
> > Same thing as for-in does, which is what JS_Enumerate does, no?
> 
> iirc, you have to delve into jsiter.h.

No, JS_Enumerate does what for-in does for "own" properties. The layering is via js_ValueToIterator.

Nothing except the for-in code does the proto-property enumeration with shadowing, deleted property suppression, etc.

Waldo, you can try namespaced properties easily:

js> const ns = new Namespace('foo')
js> o = {}
[object Object]
js> o.ns::prop = 42
4
js> for (let i in o) print(typeof i, i, o[i])
string foo::prop 42

/be
(In reply to comment #10)
> Nothing except the for-in code does the proto-property enumeration with
> shadowing, deleted property suppression, etc.

That was an aside: it's arguably a feature ;-). But if there's a legit API use-case, we could expose NativeEnumerators too.

/be
I do not recall whether Object.values was considered and rejected, but it was in

http://wiki.ecmascript.org/doku.php?id=es3.1:targeted_additions_to_array_string_object_date

back in the ES3.1 days.

Good question for es-discuss.

/be
(In reply to comment #10)
> js> o.ns::prop = 42
> 4

OMG LOL!

Actually, horror.

/be
Not just for the truncated printed number, either.  :-P
42 must not become 4 even when decimated to ASCII! Gregor filed bug 515273.

/be
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Same thing as for-in does, which is what JS_Enumerate does, no?
> > 
> > iirc, you have to delve into jsiter.h.
> 
> No, JS_Enumerate does what for-in does for "own" properties.

Sayrer gently reminded me that I was dreaming. JS_Enumerate does what it always did: returns a JSIdArray containing the ids of all enumerable, own properties.

We really need a first-class iteration API. C++ would help compared to C.

/be
Created attachment 401950 [details] [diff] [review]
With tests, actually rooting the idarray this time around

I thought about this awhile, and I think having Object.keys invoke a custom iterator isn't going to fly.  Users who invoke this method will expect to get back a list of properties, properties which can be accessed, set, getOwnPropertyDescriptor'd, etc.  __iterator__, however, returns values which may have no connection to the set of properties on the object.  This seems highly unintuitive, so the patch looks at own enumerable properties and nothing else.
Attachment #399296 - Attachment is obsolete: true
Attachment #401950 - Flags: review?(jorendorff)
Attachment #399296 - Flags: review?(jorendorff)

Comment 18

8 years ago
that seems reasonable, actually. if someone wants to control serialization of their object, they can use toJSON, and they can set their properties to be non-enumerable too. I guess I should change our JSON code to match this if it flies.
Yeah, I agree with comment 18.

js_NewUninitializedArray actually initializes the whole array to
a consistent state. Could you please rename that function to
something sensible? A separate changeset would be fine. r=me.

>+class JSAutoIdArray {

I think this convenience class would sit nicely alongside
JSAutoTempIdRooter in jscntxt.h. Your call.

>+    JSContext * const mCx;
>+    JSIdArray * const mIdArray;
>+    JSTempValueRooter mTvr;

I was told that consensus has been reached to avoid prefix
characters on member variables entirely. You can confirm this
with Luke and Brendan.

>+    jsid operator[](size_t i) const {
>+        return mIdArray->vector[i];
>+    }

JS_ASSERT(i < mIdArray->length) here?

>+    JSObject *obj = JSVAL_TO_OBJECT(vp[2]);

Not a bug, but you can just write `v` here instead of `vp[2]`.

>+    JS_ASSERT(size_t(slots[-1]) >= ida.length());

If we can avoid having this code know about dslots[-1] I think we
should. There's a macro or inline function for that.

Do dense arrays use dslots[-1] to mean "number of dslots"? It
would seem so from js_DenseArrayCapacity(). But if STOBJ_NSLOTS
is correct, dslots[-1] means "number of slots including both
fslots and dslots" for all objects. Which is right?

>+    for (size_t i = 0, sz = ida.length(); i < sz; i++) {

You could have declared
  size_t len = ida.length();
a couple lines above, before your first use of ida.length(). Then
this would be a totally plain vanilla for loop.

sz is used in jsregexp.cpp for byte counts (sizeof results -- not
element counts).

>+            /* NB: object-valued ids passed through as-is. */

How can that happen though? In the test:

>+  var o2 = {};
>+  var qn = new QName(new Namespace("foo"), "v");
>+  o2.f = 1;
>+  o2[qn] = 3;
>+  o2.baz = 4;
>+  var keys2 = Object.keys(o2);
>+  assertEq(arraysEqual(keys2, ["f", "foo::v", "baz"]), true,
>+           "" + keys2);

apparently "foo::v" comes out as a string. This makes sense for
non-XML objects. But I was also unable to get an object-valued
key into an XML object.

The only place OBJECT_TO_JSID appears in the code is in one
bizarre case in js_FindXMLProperty where apparently an XML object
is on the scope chain. I really have no idea whether the "NB:"
comment is talking about something that can happen or not.

>+/* ***** BEGIN LICENSE BLOCK *****

New tests should be public-domain, to avoid most of this
boilerplate.  http://www.mozilla.org/MPL/license-policy.html

r=me with the nits addressed and some kind of answer about object
jsids.
Attachment #401950 - Flags: review?(jorendorff) → review+
(In reply to comment #19)
> js_NewUninitializedArray actually initializes the whole array to
> a consistent state. Could you please rename that function to
> something sensible? A separate changeset would be fine. r=me.

I made the name-change before noticing the separate-commit addendum, so it's going to be in the landing for this bug.


> I think this convenience class would sit nicely alongside
> JSAutoTempIdRooter in jscntxt.h. Your call.

Good call.


> I was told that consensus has been reached to avoid prefix
> characters on member variables entirely. You can confirm this
> with Luke and Brendan.

Hm, I think you're right, hadn't thought about that possibly since before that change was made.  Style guide sez no prefix, my vague memories of the discussion agree, so ixnay on the prefix.


> JS_ASSERT(i < mIdArray->length) here?

Sure.


> >+    JS_ASSERT(size_t(slots[-1]) >= ida.length());
> 
> If we can avoid having this code know about dslots[-1] I think we
> should. There's a macro or inline function for that.
> 
> Do dense arrays use dslots[-1] to mean "number of dslots"? It
> would seem so from js_DenseArrayCapacity(). But if STOBJ_NSLOTS
> is correct, dslots[-1] means "number of slots including both
> fslots and dslots" for all objects. Which is right?

STOBJ_NSLOTS isn't really intended for dense arrays, only for native objects, I suspect.  js_DenseArrayCapacity is what you were thinking of; used that.


> >+    for (size_t i = 0, sz = ida.length(); i < sz; i++) {
> 
> You could have declared
>   size_t len = ida.length();
> a couple lines above, before your first use of ida.length(). Then
> this would be a totally plain vanilla for loop.

It's totally vanilla now, dunno why you think otherwise.  :-P  Moving the declaration to before the capacity-check does make sense, however, so done.


> >+            /* NB: object-valued ids passed through as-is. */
> 
> How can that happen though? In the test:

Hm, perhaps it can't at a closer look at your pointers and such.  I left a slightly less assertive comment indicating object-valued-jsids passed through directly as a hedge in case it's possible.
http://hg.mozilla.org/tracemonkey/rev/c144b72a1bbc
Flags: wanted1.9.2?
Whiteboard: fixed-in-tracemonkey
> It's totally vanilla now, dunno why you think otherwise.  :-P

When you get to a certain age, calcium deposits in the brain cause you only to be able to parse

  for (int i = 0; i < n; i++)

in visual-cortex hardware. Stray but a little bit from that idiom and the "experienced" hacker has to stop and think, which in some cases requires the turning of an actual hand crank. (That the semantics are trivially the same isn't the point, since the optimization you're defeating is the one that saves a call into the semantic-analysis part of the brain.)

Comment 23

8 years ago
Man, if Jason is an old fogie, then I'm...
Depends on: 518731

Comment 24

8 years ago
(In reply to comment #21)
> http://hg.mozilla.org/tracemonkey/rev/c144b72a1bbc

Waldo, please fix the test to call reportCompare() and update ecma_5/Object/jstests.list to remove the skip. Thanks.
http://hg.mozilla.org/tracemonkey/rev/87fcc6cb16af to update tests to use reportCompare once

Comment 26

8 years ago
http://hg.mozilla.org/mozilla-central/rev/c144b72a1bbc
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Flags: wanted1.9.2? → wanted1.9.2+
Resolution: --- → FIXED

Comment 27

8 years ago
js/tests/ecma_5/Object/15.2.3.14-01.js
Flags: in-testsuite+
I wrote up something minimal, still needs linking from better places than just deep in the JS reference:

https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global_Objects/Object/keys
Keywords: dev-doc-needed
completed the dev-doc under https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global_Objects/Object/keys and linked it at the "Global Object"
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.