Closed Bug 792439 (WeakSet) Opened 12 years ago Closed 10 years ago

Implement ES6 WeakSet

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
relnote-firefox --- 34+

People

(Reporter: dherman, Assigned: evilpie)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, feature, Whiteboard: [js:p2][DocArea=JS])

Attachments

(1 file, 1 obsolete file)

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
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.
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.
(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 :-)
(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.
Assignee: general → bruant.d
Status: NEW → ASSIGNED
(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.
Will you be able to tell that it is implemented using a weak map in JS? Does that matter?
(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.
(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?
> 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.
Whiteboard: [js:p2]
This is now officially in the ES6 spec. See ES6 spec (July 2013 revision) section 15.17.
Blocks: es6
OS: Mac OS X → All
Hardware: x86 → All
Summary: prototype WeakSet → Implement ES6 WeakSet
Version: unspecified → Trunk
Alias: WeakSet
Keywords: feature
Keywords: dev-doc-needed
Whiteboard: [js:p2] → [js:p2][DocArea=JS]
Assignee: bruant.d → nobody
I would like to take this up.
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?
Status: ASSIGNED → NEW
Flags: needinfo?(terrence)
I haven't made any concrete plans yet, so let's go ahead with a simple implementation on top of WeakMap for now.
Flags: needinfo?(terrence)
Ok, cool.

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

There's also bug 825199, which we have the infrastructure for now.
Attached patch v1 (obsolete) — Splinter Review
This implements everything in the current spec, just not the constructor.
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
Attached patch v1Splinter Review
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.
Attachment #8474157 - Attachment is obsolete: true
Attachment #8474230 - Flags: review?(till)
(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 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.
Attachment #8474230 - Flags: review?(till) → review+
> @@ +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.
>Can you also use Map and WeakMap as iterables?
Map always a returns a new object like [key, value]. WeakMap has no iterator.
(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!
Blocks: 1055472
Blocks: 1055473
https://hg.mozilla.org/mozilla-central/rev/68eb1c3336bd
https://hg.mozilla.org/mozilla-central/rev/be20d02a5068
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
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)
relnote-firefox: --- → ?
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
Flags: qe-verify-
No longer blocks: 1055473
You need to log in before you can comment on or make changes to this bug.