792439 - (WeakSet) Implement ES6 WeakSet

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dave Herman [:dherman]]

ES6 will probably add a WeakSet constructor, analogous to the Set constructor (this came up in the "whitelist" argument to proxies, which I'll link to in the URL). This should be pretty easy to build and a good thing for users.

Dave

Comment 1

[David Bruant]

What about implementing it as self-hosted thing on top of WeakMaps? It would enable quick prototyping. If performance becomes an issue, it can be later improved.
Cc'ing Till for his opinion on the topic.

Comment 2

[Till Schneidereit [:till]]

I guess a pretty straight-forward implementation would be to wrap a WeakMap that's mapping entries to `true`. If that works feature-wise, then I think a self-hosted implementation could work pretty well and shouldn't even have much of a performance overhead.

We'd also need to add a way for the proxy code to verify that the given parameter really is a WeakSet, but that's also easy to do on the C++ side of things.

@David Bruant: would you be interested in implementing this? If so, I can give you pointers on where to start.

Comment 3

[David Bruant]

(In reply to Till Schneidereit [:till] from comment #2)
> @David Bruant: would you be interested in implementing this? If so, I can
> give you pointers on where to start.
First thing I did after my comment was opening a scratchpad to implement WeakSet on top of WeakMap, so I guess it's been 20 minutes that my answer is "yes".
I don't know anything about the C++ side of thing, but I'll be happy to take your pointers :-)

Comment 4

[Till Schneidereit [:till]]

(In reply to David Bruant from comment #3)
> (In reply to Till Schneidereit [:till] from comment #2)
> > @David Bruant: would you be interested in implementing this? If so, I can
> > give you pointers on where to start.
> First thing I did after my comment was opening a scratchpad to implement
> WeakSet on top of WeakMap, so I guess it's been 20 minutes that my answer is
> "yes".
> I don't know anything about the C++ side of thing, but I'll be happy to take
> your pointers :-)

Splendid! :)

Are you on IRC by any chance? If so, ping me privately or in #jsapi, whichever you prefer. If not, we can talk via Skype or Jabber.

Comment 5

[David Bruant]

(In reply to Till Schneidereit [:till] from comment #4)
> Are you on IRC by any chance? If so, ping me privately or in #jsapi,
> whichever you prefer. If not, we can talk via Skype or Jabber.
It won't be possible tonight, but I will.
I guess I also need to build spidermonkey from source, etc. I'll tell you when I'm done with that.

Comment 6

[Andrew McCreight [:mccr8]]

Will you be able to tell that it is implemented using a weak map in JS? Does that matter?

Comment 7

[Till Schneidereit [:till]]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> Will you be able to tell that it is implemented using a weak map in JS? Does
> that matter?

Depending on how it's done, you won't be able to tell, no. If the weak map is only used behind the scenes, not extended, then the only way to tell should be through stack traces - and we censor those for self-hosted code.

Comment 8

[David Bruant]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> Will you be able to tell that it is implemented using a weak map in JS? Does
> that matter?
Yes it matters (as far as I'm concerned) and no, it won't be possible. It might be that the current self-hosting infrastructure has limitations that prevent not leaking this information. If that's the case, I will work to add the new features to the self-hosting infrastructure to get to the point where it won't be possible to tell whether it's a self-hosted feature.

A question to which I don't have the answer to is whether I can do Object.prototype.toString.call(new WeakSet()) === '[object WeakSet]'.
@Till, what do you think?

Comment 9

[Till Schneidereit [:till]]

> A question to which I don't have the answer to is whether I can do
> Object.prototype.toString.call(new WeakSet()) === '[object WeakSet]'.
> @Till, what do you think?

Mmh, good question. Currently that's probably only possible on the C++ side, but we can add an intrinsic function similar to _MakeConstructible to make that possible in JS only.

Comment 10

[Brandon Benvie [:benvie]]

This is now officially in the ES6 spec. See ES6 spec (July 2013 revision) section 15.17.

Comment 11

[Sankha Narayan Guria [:sankha]]

I would like to take this up.

Comment 12

[Till Schneidereit [:till]]

I guess we can do the implementation proposed in comment 2 as a stop-gap solution.

However, I know that terrence wants to restructure all the WeakMap/Map/Set stuff pretty thoroughly, and I don't know if it's useful to work on this before.

However, given that the implementation shouldn't be too hard (note comment 8 and comment 9, though), maybe this is ok for now?

terrence, what do you think?

Comment 13

[Terrence Cole [:terrence]]

I haven't made any concrete plans yet, so let's go ahead with a simple implementation on top of WeakMap for now.

Comment 14

[Till Schneidereit [:till]]

Ok, cool.

Sankha, I think you have everything you need here, right?

There's also bug 825199, which we have the infrastructure for now.

Comment 15

[Tom S [:evilpie]]

Attachment: v1

This implements everything in the current spec, just not the constructor.

Comment 16

[Tom S [:evilpie]]

Attachment: v1

I think this is basically good to review. There is a slight problem with WeakSet.prototype. I think from my understanding of the spec it shouldn't actually be a WeakSet. WeakSet.prototype.has({}) throws because the reserved slot is undefined.

Comment 17

[David Bruant]

(In reply to Tom Schuster [:evilpie] from comment #16)
> I think from my understanding of the spec it shouldn't
> actually be a WeakSet.
It shouldn't indeed. That's inconsistent from previous versions of ECMAScript where Array.prototype is an array, Function.prototype is a function, etc. This inconsistency is on purpose.

> WeakSet.prototype.has({}) throws because the reserved
> slot is undefined.
That's the expected behavior as long as the error is of the correct type (TypeError I imagine)

Comment 18

[Till Schneidereit [:till]]

Comment on attachment 8474230 [details] [diff] [review]
v1

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

Nice \o/!

I have quite a few comments, but they're either minor or shouldn't hold up landing this, so r=me with at least those comments addressed that I didn't indicate as optional.

::: js/src/builtin/WeakSet.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#define WEAKSET_MAP_SLOT 0

Please move this into SelfHostingDefines.h (which is included in Utilities.js, so the defines are available here) and use that in WeakSetObject.cpp, too.

@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#define WEAKSET_MAP_SLOT 0
> +
> +/* 23.4.3.1 */

//-style comments for all the things, please.

@@ +5,5 @@
> +#define WEAKSET_MAP_SLOT 0
> +
> +/* 23.4.3.1 */
> +function WeakSet_add(value) {
> +    /* Step 1-2. */

s/Step/Steps/g for all comments about more than one step.

@@ +12,5 @@
> +        ThrowError(JSMSG_INCOMPATIBLE_PROTO, "WeakSet", "add", typeof S);
> +
> +    /* Step 3-4. */
> +    if (!IsWeakSet(S))
> +        ThrowError(JSMSG_INCOMPATIBLE_PROTO, "WeakSet", "add", typeof S);

Combine this check with the previous one here and in the other methods. See my comment in SelfHosting.cpp for more info.

::: js/src/builtin/WeakSetObject.cpp
@@ +9,5 @@
> +#include "jscntxt.h"
> +#include "jsiter.h"
> +#include "jsobj.h"
> +
> +#include "jsapi.h"

This probably needs to move up to pass style checks, right?

@@ +75,5 @@
> +
> +bool
> +WeakSetObject::construct(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    Rooted<WeakSetObject*> obj(cx, WeakSetObject::create(cx));

This should throw if !IsConstructing.

@@ +79,5 @@
> +    Rooted<WeakSetObject*> obj(cx, WeakSetObject::create(cx));
> +    if (!obj)
> +        return false;
> +
> +    // Based on our "Set" implementation instead of the more general ES6 steps.

Can you expand on what's different? Also, I'd like to have the spec steps documented here, with deviations explained where the relevant step is (not) implemented.

From a quick-ish comparison, I see that you don't treat null as undefined for the iterator argument. Please change that and add a test.

@@ +85,5 @@
> +    if (args.hasDefined(0)) {
> +        RootedObject map(cx, &obj->getReservedSlot(WEAKSET_MAP_SLOT).toObject());
> +
> +        ForOfIterator iter(cx);
> +        if (!iter.init(args[0]))

There's a spec bug here, but it's in the progress of being fixed: https://bugs.ecmascript.org/show_bug.cgi?id=3110

::: js/src/builtin/WeakSetObject.h
@@ +12,5 @@
> +namespace js {
> +
> +class WeakSetObject : public JSObject
> +{
> +    /* Stores the private WeakMap used for the implementation. */

nit: //-style comments, please.

@@ +13,5 @@
> +
> +class WeakSetObject : public JSObject
> +{
> +    /* Stores the private WeakMap used for the implementation. */
> +    static const unsigned WEAKSET_MAP_SLOT = 0;

Move this to SelfHostingDefines.h, see comment in WeakSet.js.

::: js/src/jit-test/tests/collections/WeakSet-constructor.js
@@ +5,5 @@
> +assertEq(ws.has(Function), true);
> +
> +ws = new WeakSet(Set(list));
> +assertEq(ws.has(Number), true);
> +assertEq(ws.has(Function), true);
\ No newline at end of file

Can you also use Map and WeakMap as iterables?

::: js/src/jit-test/tests/collections/WeakSet-error.js
@@ +15,5 @@
> +testMethod("add");
> +testMethod("delete");
> +testMethod("clear", false);
> +
> +assertThrowsInstanceOf(function() { new WeakSet({"@@iterator": 2}) }, TypeError);

Hmm, this test will probably fail silently once we change @@iterator to be a real symbol. Perhaps add Function- or eval-based canary that will throw once @@iterator is available, so this isn't overlooked?

::: js/src/jit-test/tests/collections/WeakSet-surface.js
@@ +10,5 @@
> +assertEq(WeakSet.length, 1);
> +assertEq(WeakSet.name, "WeakSet");
> +
> +assertEq(Object.getPrototypeOf(WeakSet.prototype), Object.prototype);
> +assertEq(Object.prototype.toString.call(WeakSet.prototype), "[object WeakSet]");

Ideally, this would print [object Object]. See [1] and [2] (and the entire containing thread, if you're curious) for discussions about this. I think it's not entirely straight-forward to implement this, though. I wouldn't hold up this patch if it's hard to change this aspect.

If you do decide to change this (see bug 861219 for an attempt to do this), please also assert `!(WeakSet.prototype instanceof WeakSet)`.

[1]: https://mail.mozilla.org/pipermail/es-discuss/2014-June/037669.html
[2]: https://mail.mozilla.org/pipermail/es-discuss/2014-June/037933.html

@@ +12,5 @@
> +
> +assertEq(Object.getPrototypeOf(WeakSet.prototype), Object.prototype);
> +assertEq(Object.prototype.toString.call(WeakSet.prototype), "[object WeakSet]");
> +assertEq(Object.prototype.toString.call(new WeakSet), "[object WeakSet]");
> +assertEq(Object.prototype.toString.call(WeakSet()), "[object WeakSet]");

This should throw: the WeakSet ctor shouldn't create a new instance when called as a function. See http://people.mozilla.org/~jorendorff/es6-draft.html#sec-weakset-iterable steps 1 to 4.

@@ +13,5 @@
> +assertEq(Object.getPrototypeOf(WeakSet.prototype), Object.prototype);
> +assertEq(Object.prototype.toString.call(WeakSet.prototype), "[object WeakSet]");
> +assertEq(Object.prototype.toString.call(new WeakSet), "[object WeakSet]");
> +assertEq(Object.prototype.toString.call(WeakSet()), "[object WeakSet]");
> +assertEq(Object.keys(WeakSet.prototype).join(), "");

Any reason not to use the .length === 0 check you used for WeakSet?

@@ +16,5 @@
> +assertEq(Object.prototype.toString.call(WeakSet()), "[object WeakSet]");
> +assertEq(Object.keys(WeakSet.prototype).join(), "");
> +assertEq(WeakSet.prototype.constructor, WeakSet);
> +
> +function checkMethod(name, arity) {

Nice. We should maybe have this in our testing harness.

::: js/src/vm/GlobalObject.cpp
@@ +24,5 @@
>  #include "builtin/RegExp.h"
>  #include "builtin/SIMD.h"
>  #include "builtin/SymbolObject.h"
>  #include "builtin/TypedObject.h"
> +#include "builtin/WeakSetObject.h"

Why're you including this here? Is it used by a macro or something?

::: js/src/vm/SelfHosting.cpp
@@ +651,5 @@
>      return true;
>  }
>  
> +static bool
> +intrinsic_IsWeakSet(JSContext *cx, unsigned argc, Value *vp)

Much nicer than the workarounds in Map and Set, thanks.

@@ +655,5 @@
> +intrinsic_IsWeakSet(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    JS_ASSERT(args.length() == 1);
> +    JS_ASSERT(args[0].isObject());

Can you change this to just result in a `false` rval instead of asserting. Then you can combine steps 2 - 4 of all methods in WeakSet.js into one.

Comment 19

[Tom S [:evilpie]]

> @@ +12,5 @@
> > +
> > +assertEq(Object.getPrototypeOf(WeakSet.prototype), Object.prototype);
> > +assertEq(Object.prototype.toString.call(WeakSet.prototype), "[object WeakSet]");
> > +assertEq(Object.prototype.toString.call(new WeakSet), "[object WeakSet]");
> > +assertEq(Object.prototype.toString.call(WeakSet()), "[object WeakSet]");
> 
> This should throw: the WeakSet ctor shouldn't create a new instance when
> called as a function. See
> http://people.mozilla.org/~jorendorff/es6-draft.html#sec-weakset-iterable
> steps 1 to 4.
> 
Seems like WeakSet.call(new WeakSet) would work? I am just throwing when not constructing for now, we need to fix the constructor anyway.

> @@ +655,5 @@
> > +intrinsic_IsWeakSet(JSContext *cx, unsigned argc, Value *vp)
> > +{
> > +    CallArgs args = CallArgsFromVp(argc, vp);
> > +    JS_ASSERT(args.length() == 1);
> > +    JS_ASSERT(args[0].isObject());
> 
> Can you change this to just result in a `false` rval instead of asserting.
> Then you can combine steps 2 - 4 of all methods in WeakSet.js into one.
No I don't like this approach. All of our IsXXX functions do this. I changed it to !IsObject() || !IsWeakSet() which is fairly short as well.

Comment 20

[Tom S [:evilpie]]

>Can you also use Map and WeakMap as iterables?
Map always a returns a new object like [key, value]. WeakMap has no iterator.

Comment 21

[Till Schneidereit [:till]]

(In reply to Tom Schuster [:evilpie] from comment #19)
> > @@ +12,5 @@
> > > +
> > > +assertEq(Object.getPrototypeOf(WeakSet.prototype), Object.prototype);
> > > +assertEq(Object.prototype.toString.call(WeakSet.prototype), "[object WeakSet]");
> > > +assertEq(Object.prototype.toString.call(new WeakSet), "[object WeakSet]");
> > > +assertEq(Object.prototype.toString.call(WeakSet()), "[object WeakSet]");
> > 
> > This should throw: the WeakSet ctor shouldn't create a new instance when
> > called as a function. See
> > http://people.mozilla.org/~jorendorff/es6-draft.html#sec-weakset-iterable
> > steps 1 to 4.
> > 
> Seems like WeakSet.call(new WeakSet) would work? I am just throwing when not
> constructing for now, we need to fix the constructor anyway.

No, see step 4. The error should probably have a different message, though.

> 
> > @@ +655,5 @@
> > > +intrinsic_IsWeakSet(JSContext *cx, unsigned argc, Value *vp)
> > > +{
> > > +    CallArgs args = CallArgsFromVp(argc, vp);
> > > +    JS_ASSERT(args.length() == 1);
> > > +    JS_ASSERT(args[0].isObject());
> > 
> > Can you change this to just result in a `false` rval instead of asserting.
> > Then you can combine steps 2 - 4 of all methods in WeakSet.js into one.
> No I don't like this approach. All of our IsXXX functions do this. I changed
> it to !IsObject() || !IsWeakSet() which is fairly short as well.

Ok, fair enough.

(In reply to Tom Schuster [:evilpie] from comment #20)
> >Can you also use Map and WeakMap as iterables?
> Map always a returns a new object like [key, value]. 

Right.

> WeakMap has no iterator.

D'oh!

Comment 22

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68eb1c3336bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/be20d02a5068 (Insert new lines at the end of some files)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/68eb1c3336bd
https://hg.mozilla.org/mozilla-central/rev/be20d02a5068

Comment 24

[Jean-Yves Perrier [:teoli]]

Florian documented it:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakSet
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakSet/prototype
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakSet/add
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakSet/clear
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakSet/delete
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakSet/has
and an note in:
https://developer.mozilla.org/en-US/Firefox/Releases/34

Comment 25

[Florian Bender]

Release Note Request (optional, but appreciated)
[Why is this notable]: more ES6
[Suggested wording]: Implemented WeakSet (ECMAScript 6).
[Links (documentation, blog post, etc)]: (comment 24)

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Added in the release notes with "ECMAScript 6 WeakSet Implemented (docs)" as wording.

Pointing to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakSet