ES6: Implement WeakSet

src/collection.js

Index: src/collection.js
diff --git a/src/collection.js b/src/collection.js
index c5604ab30f359b1287a664ed22d3656d8843b075..351d797cc96a136b73ffed36f5de35b3cbc32613 100644
--- a/src/collection.js
+++ b/src/collection.js
@@ -325,3 +325,92 @@ function SetUpWeakMap() {
 }
 
 SetUpWeakMap();
+
+
+// -------------------------------------------------------------------
+// Harmony WeakSet
+
+var weaksetDataSymbol = %CreateSymbol(void 0);

[Michael Starzinger, 2013/07/17 12:56:41]

This requires that Symbols can model private state, which currently holds for
our implementation but will probably not hold in ES6, so it would be good to not
introduce this requirement here. Since we already need modifications to the
bootstrapper (see comment below), adding a new instance type (i.e.
JS_WEAK_SET_TYPE) is trivial.

As for the runtime functions (i.e. Runtime_WeakMapHas and friends) we could
either duplicate them in runtime.cc for WeakSet or introduce a common super-type
in the type hirarchy in objects.h, both approaches sound equally fine to Andreas
and me.

[arv (Not doing code reviews), 2013/07/17 15:26:26]

We already depend on private symbols in a few other places. We can change to use
WeakMap in those few places if I fail to convince TC39 to have private symbols.

[Michael Starzinger, 2013/07/18 08:33:18]

Andreas, what is your opinion about using a Synbol here to model private state?
I could be convinced to land the CL with this current approach. But I'll leave
the final say to Andreas as he is the expert on our Symbol implementation.

+
+
+function WeakSet() {

[Michael Starzinger, 2013/07/17 12:56:41]

nit: s/WeakSet/WeakSetConstructor/ for consistency.

[arv (Not doing code reviews), 2013/07/17 15:26:26]

No, then the name property would be wrong.

Adding test.

[Michael Starzinger, 2013/07/18 08:33:18]

Good point, thanks for adding the tests. But I would still use
%FunctionSetName() to set the correct "name" property on that function instead
of forcing the property name on the builtins global object to have the same name
(or alternatively address the next comment).

Then we can s/WeakSet/WeakSetConstructor/ here.

+  if (%_IsConstructCall()) {
+    this[weaksetDataSymbol] = new $WeakMap();
+  } else {
+    return new $WeakSet();
+  }
+}
+
+
+var $WeakSet = global.WeakSet = WeakSet;

[Michael Starzinger, 2013/07/17 12:56:41]

This way of adding "global.WeakSet" unfortunately makes this property
enumerable. While I generally support the move to bootstrap as much as possible
in JavaScript, I am afraid this part of the initialization needs to go into
bootstrapper.cc similar to e.g. WeakMap. Once we can move InstallFunction() into
JavaScript, we should do so for all global properties at once. This can be
checked using either of the following code in global scope:

Object.getOwnPropertyDescriptor(this, "WeakSet");
Object.keys(this).indexOf("WeakSet");

[arv (Not doing code reviews), 2013/07/17 15:26:26]

Easy to fix without C++ bootstrapping.

[Michael Starzinger, 2013/07/18 08:33:18]

This issue might be easy to fix without C++, but it introduces disparity in the
into bootstrapping process, that's what I meant with "we should do so for all
global properties at once" in my original comment.

To give one example of where that will bite us: The experimental builtins (e.g.
WeakMap, Symbol, ...) all have their prototype property (e.g. WeakMap.prototype,
...) be writable which is wrong according to the spec draft. We intend to fix
this soon and I would have no clue how to fix this in plain JavaScript.

So I would kindly ask you to install "global.WeakSet" in C++ for the sake of
consistency and maintainability.

+
+
+function WeakSetAdd(value) {
+  var weaksetData = this[weaksetDataSymbol];

[Michael Starzinger, 2013/07/17 12:56:41]

It would be nice to get rid of this additional indirection. See comment at the
top if this file.

[arv (Not doing code reviews), 2013/07/17 15:26:26]

Instead of this indirection we would need to check that there is a
[[WeakSetData]] or the traditional way of checking [[Class]]. I assume checking
the [[Class]] is faster.

+  if (!weaksetData) {
+    throw %MakeTypeError('incompatible_method_receiver',
+                         ['WeakSet.prototype.add', this]);
+  }
+  if (!(IS_SPEC_OBJECT(value) || IS_SYMBOL(value))) {
+    throw %MakeTypeError('invalid_weakset_value', [this, value]);
+  }
+  return %WeakMapSet(weaksetData, value, true);
+}
+
+
+function WeakSetHas(value) {
+  var weaksetData = this[weaksetDataSymbol];
+  if (!weaksetData) {
+    throw %MakeTypeError('incompatible_method_receiver',
+                         ['WeakSet.prototype.has', this]);
+  }
+  if (!(IS_SPEC_OBJECT(value) || IS_SYMBOL(value))) {
+    throw %MakeTypeError('invalid_weakset_value', [this, value]);
+  }
+  return %WeakMapHas(weaksetData, value);
+}
+
+
+function WeakSetDelete(value) {
+  var weaksetData = this[weaksetDataSymbol];
+  if (!weaksetData) {
+    throw MakeTypeError('incompatible_method_receiver',
+                        ['WeakSet.prototype.delete', this]);
+  }
+  if (!(IS_SPEC_OBJECT(value) || IS_SYMBOL(value))) {
+    throw %MakeTypeError('invalid_weakset_value', [this, value]);
+  }
+  return %WeakMapDelete(weaksetData, value);
+}
+
+
+function WeakSetClear() {
+  if (!this[weaksetDataSymbol]) {
+    throw %MakeTypeError('incompatible_method_receiver',
+                         ['WeakSet.prototype.clear', this]);
+  }
+
+  this[weaksetDataSymbol] = new $WeakMap();
+}
+
+
+// -------------------------------------------------------------------
+
+function SetUpWeakSet() {
+  %CheckIsBootstrapping();
+
+  %FunctionSetInstanceClassName($WeakSet, 'WeakSet');
+
+  %FunctionSetPrototype($WeakSet, new $Object());
+  %SetProperty($WeakSet.prototype, "constructor", $WeakSet, DONT_ENUM);
+
+  // Set up the non-enumerable functions on the WeakSet prototype object.
+  InstallFunctions($WeakSet.prototype, DONT_ENUM, $Array(
+    "add", WeakSetAdd,
+    "has", WeakSetHas,
+    "delete", WeakSetDelete,
+    "clear", WeakSetClear
+  ));
+}
+
+SetUpWeakSet();