869996 - Set.prototype.{keys, values, entries}

Status: RESOLVED FIXED

Description

[Brandon Benvie [:benvie]]

The ES6 spec specifies [1] that `Set.prototype` should have the following methods:

* entries - returns a SetIterator where each Set entry is returned in an array twice, `[value, value]`
* keys - returns a SetIterator that iterates through the values in the Set
* values - same as `keys`

See bug 817368 for Map.prototype.{keys,values,entries}.

[1] http://people.mozilla.org/~jorendorff/es6-draft.html#sec-15.16.4.5

Comment 1

[Sankha Narayan Guria [:sankha]]

Created attachment 753119 [details] [diff] [review]
patch v1

I have written a WIP patch looking at the implementation of Map.prototype.{keys, values, entries}. There are no test cases yet. Can you tell me if I am going in the right direction?

Comment 2

[Sankha Narayan Guria [:sankha]]

Created attachment 753127 [details] [diff] [review]
patch v1

Oops! That was a wrong patch. Can you provide feedback for this one?

Comment 3

[Brandon Benvie [:benvie]]

Comment on attachment 753127 [details] [diff] [review]
patch v1

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

Looking good overall, with a few tweaks.

::: js/src/builtin/MapObject.cpp
@@ +1465,5 @@
>      return static_cast<ValueSet::Range *>(getSlot(RangeSlot).toPrivate());
>  }
>  
> +inline SetObject::IteratorKind
> +MapIteratorObject::kind() const

Should be `SetIteratorObject`

@@ +1537,5 @@
>          thisobj.setReservedSlot(RangeSlot, PrivateValue(NULL));
>          return js_ThrowStopIteration(cx);
>      }
>  
> +    SetObject::IteratorKind kind = thisobj.kind();

`MapIteratorObject::next_impl` uses a switch here. It might be good to do the same here since you can just fall through for Keys/Values, due to them being the same.

@@ +1608,1 @@
>      JS_FN("iterator", iterator, 0, 0),

The ES6 spec says that `Set.prototype.@@iterator` is the same as `Set.prototype.values`. You can remove `SetObject::iterator` entirely and just set both "values" and "iterator" to `SetObject::values`. (Map does this with `entries`, for comparison)

Comment 4

[Sankha Narayan Guria [:sankha]]

Created attachment 753764 [details] [diff] [review]
patch v2

Comment 5

[Brandon Benvie [:benvie]]

Comment on attachment 753764 [details] [diff] [review]
patch v2

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

lgtm

Comment 6

[Sankha Narayan Guria [:sankha]]

I have just been able to run the jit-tests on my machine, and there is a problem with the Set-values-1.js file. I fear its failing the last case with the FAIL error. Can you please tell me what have I done wrong?

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 753764 [details] [diff] [review]
patch v2

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

Clearing the review flag. This is very much on the right track! Please refresh it and ask for review again.

::: js/src/builtin/MapObject.cpp
@@ +1540,5 @@
>  
> +    switch (thisobj.kind()) {
> +        case SetObject::Keys:
> +        case SetObject::Values:
> +            args.rval().set(range->front().get());

Tragically, the SpiderMonkey style guide requires 'case' labels to be indented just 2 spaces -- like in MapIteratorObject::next_impl.

@@ +1607,5 @@
>      JS_FN("delete", delete_, 1, 0),
> +    JS_FN("keys", keys, 0, 0),
> +    JS_FN("values", values, 0, 0),
> +    JS_FN("entries", entries, 0, 0),
> +    JS_FN("iterator", values, 0, 0),

The specification says that "keys", "values", and @@iterator are all the same function object.  Please add a test:
  assertEq(Set.prototype.keys, Set.prototype.values);
and another one for Map:
  assertEq(Map.prototype.iterator, Map.prototype.entries);

To make these tests pass, you will have to remove "keys", "values", and "iterator" from SetObject::methods. Then change SetObject::initClass to define those functions by hand. Unfortunately this is kind of a lot of code.

It'll be something like this:

> JSObject *
> SetObject::initClass(JSContext *cx, JSObject *obj)
> {
>     Rooted<GlobalObject*> global(cx, &obj->asGlobal());
>-    return InitClass(cx, global, &class_, JSProto_Set, construct, properties, methods);
>+    RootedObject proto(cx,
>+        InitClass(cx, global, &class_, JSProto_Set, construct, properties, methods));
>+    if (proto) {
>+        // Define the "values" method.
>+        JSFunction *fun = JS_DefineFunction(cx, proto, "values", values, 0, 0);
>+        if (!fun)
>+            return NULL;
>+
>+        // Define its aliases.
>+        RootedValue funval(cx, ObjectValue(*fun));
>+        if (!JS_DefineProperty(cx, proto, "keys", funval, NULL, NULL, 0))
>+            return NULL;
>+        if (!JS_DefineProperty(... "iterator" ...))
>+            return NULL;
>+    }
>+    return proto;
> }

Please do the same for "entries"/"iterator" in MapObject!

@@ +1823,1 @@
>  }

Please delete SetObject::keys.

::: js/src/builtin/MapObject.h
@@ +126,5 @@
>  };
>  
>  class SetObject : public JSObject {
>    public:
> +    enum IteratorKind { Keys, Values, Entries };

Only Values and Entries are necessary.

@@ +155,5 @@
> +    static JSBool keys(JSContext *cx, unsigned argc, Value *vp);
> +    static bool values_impl(JSContext *cx, CallArgs args);
> +    static JSBool values(JSContext *cx, unsigned argc, Value *vp);
> +    static bool entries_impl(JSContext *cx, CallArgs args);
> +    static JSBool entries(JSContext *cx, unsigned argc, Value *vp);

Please remove the declarations for keys and keys_impl.

::: js/src/jit-test/tests/collections/Set-values-2.js
@@ +13,5 @@
> +assertEq(ki.next(), 3);
> +assertEq(ki.next(), 4);
> +assertThrowsValue(function () { ki.next(); }, StopIteration);
> +
> +assertEq([k for (k of s.keys())].toSource(), [1, 2, 3, 4].toSource());

[k or (k of s.keys())] can be expressed like this:   [...s.keys()]

Comment 8

[Sankha Narayan Guria [:sankha]]

Created attachment 754046 [details] [diff] [review]
final patch

Comment 9

[Jason Orendorff [:jorendorff]]

(In reply to Sankha Narayan Guria [:sankha93] from comment #6)
> I have just been able to run the jit-tests on my machine, and there is a
> problem with the Set-values-1.js file. I fear its failing the last case with
> the FAIL error. Can you please tell me what have I done wrong?

You can get the command lines and output from the tests by passing the options -so to the test runner. Like this:

    python jit-test/jit_test.py -so $YOUR_BUILD_DIR/js Set-values-1

Looks like it's a typo in the test, not a bug in your C++.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 754046 [details] [diff] [review]
final patch

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

Looks great. I'm going to rename some test files but other than that we're all set. I will land this on Monday! Thanks!

Comment 11

[Jason Orendorff [:jorendorff]]

Monday was a holiday. I'll land this today.

Comment 12

[Jason Orendorff [:jorendorff]]

Finally got around to this, pushed to try server:
  https://tbpl.mozilla.org/?tree=Try&rev=7c2c8bfd4b33

Comment 13

[Jason Orendorff [:jorendorff]]

Trying again:
  https://tbpl.mozilla.org/?tree=Try&rev=c38f68d528f1

Comment 14

[Sankha Narayan Guria [:sankha]]

Created attachment 756136 [details] [diff] [review]
final patch

Just realized that the previous patch did not have my username and email set, so uploading the same patch with all the details.

Comment 15

[Sankha Narayan Guria [:sankha]]

I pushed to Try this time: https://tbpl.mozilla.org/?tree=Try&rev=6c13ecf09710

Comment 16

[Rick Waldron [:rwaldron]]

Comment on attachment 756136 [details] [diff] [review]
final patch

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

::: js/src/jit-test/tests/collections/Set-values-2.js
@@ +10,5 @@
> +var ki = s.keys();
> +assertEq(ki.next(), 1);
> +assertEq(ki.next(), 2);
> +assertEq(ki.next(), 3);
> +assertEq(ki.next(), 4);

Please see 15.16.6.2.2 SetIterator.prototype.next() of the latest specification draft.

.next() should produce an ItrResult object that has both a "value" and "done" property. The "value" property's own value will be as those shown above, the "done" property will be a boolean value that is |true| until there are no more values to yield, at which time it will have the boolean value |false|

@@ +11,5 @@
> +assertEq(ki.next(), 1);
> +assertEq(ki.next(), 2);
> +assertEq(ki.next(), 3);
> +assertEq(ki.next(), 4);
> +assertThrowsValue(function () { ki.next(); }, StopIteration);

See line 14 comments, StopIteration has been removed from the design of the feature

Comment 17

[Jason Orendorff [:jorendorff]]

I spoke with Rick on IRC. This is of course an incremental step. We will implement the standard.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/118b1725ac1e

Comment 19

[Sankha Narayan Guria [:sankha]]

Filed bug 881226 to change the methods to the latest spec.

Comment 20

[Jeremie Patonnier :Jeremie]

Follow up for documentation: https://bugzilla.mozilla.org/show_bug.cgi?id=885260