Upgrade Symbol implementation to match current ES6 behavior.

test/mjsunit/harmony/symbols.js

Index: test/mjsunit/harmony/symbols.js
diff --git a/test/mjsunit/harmony/symbols.js b/test/mjsunit/harmony/symbols.js
index 3fcd06dfdca985fcd81abd8630cb8ededa8fe0d7..06bef4738f608a1d93f7fb91d0a7cd5251afbf41 100644
--- a/test/mjsunit/harmony/symbols.js
+++ b/test/mjsunit/harmony/symbols.js
@@ -30,22 +30,19 @@
 
 var symbols = []
 
+
 // Test different forms of constructor calls, all equivalent.
 function TestNew() {
-  function IndirectSymbol() { return new Symbol }
-  function indirect() { return new IndirectSymbol() }
+  function IndirectSymbol() { return Symbol() }
+  function indirect() { return IndirectSymbol() }
   for (var i = 0; i < 2; ++i) {
     for (var j = 0; j < 5; ++j) {
       symbols.push(Symbol())
       symbols.push(Symbol(undefined))
       symbols.push(Symbol("66"))
       symbols.push(Symbol(66))
-      symbols.push(Symbol(Symbol()))
-      symbols.push((new Symbol).valueOf())
-      symbols.push((new Symbol()).valueOf())
-      symbols.push((new Symbol(Symbol())).valueOf())
       symbols.push(Object(Symbol()).valueOf())
-      symbols.push((indirect()).valueOf())
+      symbols.push((indirect()))
     }
     %OptimizeFunctionOnNextCall(indirect)
     indirect()  // Call once before GC throws away type feedback.
@@ -55,13 +52,18 @@ function TestNew() {
 TestNew()
 
 
+function TestRepeatedApplication() {
+  assertThrows(function () { Symbol(Symbol()); }, TypeError)
+}
+TestRepeatedApplication()
+
+
 function TestType() {
   for (var i in symbols) {
     assertEquals("symbol", typeof symbols[i])
     assertTrue(typeof symbols[i] === "symbol")
     assertFalse(%SymbolIsPrivate(symbols[i]))
     assertEquals(null, %_ClassOf(symbols[i]))
-    assertEquals("Symbol", %_ClassOf(new Symbol(symbols[i])))
     assertEquals("Symbol", %_ClassOf(Object(symbols[i])))
   }
 }
@@ -69,13 +71,15 @@ TestType()
 
 
 function TestPrototype() {
+  var symbolPrototype = Object.getPrototypeOf(Object(Symbol()))
+  assertSame(Function.prototype, Symbol.prototype)

[rossberg, 2014/01/08 17:52:25]

That seems wrong. Symbol.prototype is an ordinary object.

[sof, 2014/01/08 18:13:43]

https://people.mozilla.org/~jorendorff/es6-draft.html#sec-properties-of-the-s...
says it's Function.prototype.

[arv (Not doing code reviews), 2014/01/08 18:54:44]

It says that "The value of the [[Prototype]] internal slot" is
Function.prototype. This is reflected as

assertSame(Object.getPrototypeOf(Symbol), Function.prototype)

[rossberg, 2014/01/09 10:35:53]

Don't confuse .prototype and .[[Prototype]] (a.k.a. __proto__). As Arv says,
that spec section says that Symbol.__proto__ === Function.prototype.

Symbol.prototype is specified elsewhere:

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-properties-of-the-sy...

[sof, 2014/01/09 13:17:13]

I made that only available on the "SymbolWrapper" object that Object()
instantiates.

I will need to sit down and change the encoding of Symbol to expose the expected
.prototype, updating the tests here to reflect this. Sorry about the mess.

   assertSame(Object.prototype, Symbol.prototype.__proto__)
   assertSame(Symbol.prototype, Symbol().__proto__)
-  assertSame(Symbol.prototype, Symbol(Symbol()).__proto__)
-  assertSame(Symbol.prototype, (new Symbol).__proto__)
-  assertSame(Symbol.prototype, (new Symbol()).__proto__)
-  assertSame(Symbol.prototype, (new Symbol(Symbol())).__proto__)
-  assertSame(Symbol.prototype, Object(Symbol()).__proto__)
+  assertSame(symbolPrototype, Object(Symbol()).__proto__)

[rossberg, 2014/01/08 17:52:25]

Is this different from Symbol.prototype? Why?

[sof, 2014/01/08 18:13:43]

Yes, Object(Symbol()).__proto__ is the prototype of the wrapper object, not
Symbol.prototype.

[arv (Not doing code reviews), 2014/01/08 18:54:44]

The [[Prototype]] of a symbol wrapper should be Symbol.prototype. This is the
same as for number, string and boolean wrappers.

+  assertSame(Symbol.prototype, Symbol.__proto__)

[rossberg, 2014/01/08 17:52:25]

Shouldn't Symbol.__proto__ be Function.prototype?

+  // FIXME: what should __proto__ be for a Symbol value?
+  assertSame(Function.prototype, (Symbol()).__proto__)

[rossberg, 2014/01/08 17:52:25]

And Symbol().__proto__ should be Symbol.prototype

[sof, 2014/01/08 18:13:43]

Definitely not.

[arv (Not doing code reviews), 2014/01/08 18:54:44]

When you do a [[Get]] on a non object a temporary wrapper is created so

var sym = Symbol();
sym.foo === Object(sym).foo;

for all foos

[rossberg, 2014/01/09 10:35:53]

See above.

+  assertTrue(typeof ((Symbol()).prototype) === "undefined")

[rossberg, 2014/01/08 17:52:25]

Not sure why this test is necessary.

[sof, 2014/01/08 18:13:43]

Just testing that typeof over this .prototype is behaving like it does for other
primitive values.

[rossberg, 2014/01/09 10:35:53]

Well, it just tests that the symbol wrapper object does not have a property
named 'prototype'. It's not wrong to assert this, but I don't see a particular
reason to do so -- that property name is only special for function objects. For
others, it's no more relevant than testing that 'xyz' is undefined.

   for (var i in symbols) {
     assertSame(Symbol.prototype, symbols[i].__proto__)
   }
@@ -86,36 +90,39 @@ TestPrototype()
 function TestConstructor() {
   assertFalse(Object === Symbol.prototype.constructor)

[rossberg, 2014/01/08 17:52:25]

This should be true now.

[sof, 2014/01/08 18:13:43]

No, I believe this is correct.

[arv (Not doing code reviews), 2014/01/08 18:54:44]

Looks correct to me too:

assertSame(Symbol.prototype.constructor, Symbol);

just like for all other wrappers.

However, using assertSame would be better.

[rossberg, 2014/01/09 10:35:53]

Oops, you are both right, sorry.

   assertFalse(Symbol === Object.prototype.constructor)
-  assertSame(Symbol, Symbol.prototype.constructor)
-  assertSame(Symbol, Symbol().__proto__.constructor)
-  assertSame(Symbol, Symbol(Symbol()).__proto__.constructor)
-  assertSame(Symbol, (new Symbol).__proto__.constructor)
-  assertSame(Symbol, (new Symbol()).__proto__.constructor)
-  assertSame(Symbol, (new Symbol(Symbol())).__proto__.constructor)
+  assertSame(Function, Symbol.prototype.constructor)

[rossberg, 2014/01/08 17:52:25]

This should be Object, not Function

[sof, 2014/01/08 18:13:43]

Same; are we reading the same draft version of the spec?

[arv (Not doing code reviews), 2014/01/08 18:54:44]

This should be

assertSame(Symbol.prototype.constructor, Symbol);

https://people.mozilla.org/~jorendorff/es6-draft.html#sec-symbol.prototype.co...

[rossberg, 2014/01/09 10:35:53]

Arv is correct.

+  assertSame(Function, Symbol().__proto__.constructor)

[rossberg, 2014/01/08 17:52:25]

Dito

[arv (Not doing code reviews), 2014/01/08 18:54:44]

Should be

assertSame(Symbol, Symbol().__proto__.constructor)

+  assertSame(Function, Object(Symbol()).valueOf().__proto__.constructor)

[rossberg, 2014/01/08 17:52:25]

Dito here. And why the redundant valueOf()?

+  assertSame(Function, (Symbol).__proto__.constructor)

[rossberg, 2014/01/08 17:52:25]

This is an unrelated test, since it tests the constructor of the constructor.

+  assertSame(Function, (Symbol()).__proto__.constructor)

[rossberg, 2014/01/08 17:52:25]

This is repeating line 94

+  assertSame(Symbol, (Object(Symbol())).__proto__.constructor)

[rossberg, 2014/01/08 17:52:25]

Nit: redundant parens (here and above)

   assertSame(Symbol, Object(Symbol()).__proto__.constructor)
   for (var i in symbols) {
-    assertSame(Symbol, symbols[i].__proto__.constructor)
+    assertSame(Function, symbols[i].__proto__.constructor)

[rossberg, 2014/01/08 17:52:25]

Should also be Symbol

   }
 }
 TestConstructor()
 
 
-function TestName() {
+function TestValueOf() {
   for (var i in symbols) {
-    var name = symbols[i].name
-    assertTrue(name === undefined || name === "66")
+    var value = Object(symbols[i]).valueOf()

[rossberg, 2014/01/08 17:52:25]

Why the redundant Object()?

+    assertTrue(value === symbols[i])
   }
 }
-TestName()
+TestValueOf()
 
 
 function TestToString() {
   for (var i in symbols) {
+    var stringified;
+    assertDoesNotThrow(function () { stringified = String(Object(symbols[i])); })

[rossberg, 2014/01/08 17:52:25]

Nit: line length (just remove the space after 'function')

+    assertTrue(stringified == "Symbol(66)" || stringified == "Symbol()")
     assertThrows(function() { String(symbols[i]) }, TypeError)
     assertThrows(function() { symbols[i] + "" }, TypeError)
     assertThrows(function() { symbols[i].toString() }, TypeError)
-    assertThrows(function() { (new Symbol(symbols[i])).toString() }, TypeError)
-    assertThrows(function() { Object(symbols[i]).toString() }, TypeError)
+    assertThrows(function() { (Symbol(symbols[i])).toString() }, TypeError)
+    assertDoesNotThrow(function() { Object(symbols[i]).toString() })

[rossberg, 2014/01/08 17:52:25]

Why not check the expected result?

     assertEquals("[object Symbol]", Object.prototype.toString.call(symbols[i]))
   }
 }
@@ -156,10 +163,17 @@ function TestEquality() {
     assertTrue(Object.is(symbols[i], symbols[i]))
     assertTrue(symbols[i] === symbols[i])
     assertTrue(symbols[i] == symbols[i])
-    assertFalse(symbols[i] === new Symbol(symbols[i]))
-    assertFalse(new Symbol(symbols[i]) === symbols[i])
-    assertTrue(symbols[i] == new Symbol(symbols[i]))
-    assertTrue(new Symbol(symbols[i]) == symbols[i])
+    assertTrue(symbols[i] === Object(symbols[i]).valueOf())

[rossberg, 2014/01/08 17:52:25]

The original test was to ensure that symbols are distinct from their wrappers,
so this should be

assertFalse(symbols[i] === Object(symbols[i]))

+    assertTrue(Object(symbols[i]).valueOf() === symbols[i])

[rossberg, 2014/01/08 17:52:25]

Likewise

+    assertTrue(symbols[i] == Object(symbols[i]).valueOf())
+    assertTrue(Object(symbols[i]).valueOf() == symbols[i])
+    assertEquals(Object(symbols[i]), Object(symbols[i]))

[rossberg, 2014/01/08 17:52:25]

This is surprising. Why are the two objects equal?

+    assertEquals(Object(symbols[i]).valueOf(), Object(symbols[i]).valueOf())
+    // Comparing Symbol wrappers via equality should be done without
+    // invoking ToPrimitive() (which throws for Symbol wrappers.)
+    var sym1 = Object(symbols[i])
+    assertFalse(sym1 == Object(symbols[i]))

[rossberg, 2014/01/08 17:52:25]

That seems to contradict line 170 -- I'm really confused now.

+    assertTrue(sym1 == sym1)
   }
 
   // All symbols should be distinct.
@@ -188,7 +202,7 @@ TestEquality()
 function TestGet() {
   for (var i in symbols) {
     assertThrows(function() { symbols[i].toString() }, TypeError)
-    assertEquals(symbols[i], symbols[i].valueOf())
+    assertEquals(symbols[i], Object(symbols[i]).valueOf())

[rossberg, 2014/01/08 17:52:25]

Why is Object() needed?

[sof, 2014/01/08 18:13:43]

To test that the wrapper object's valueOf() returns the same symbol value.

[rossberg, 2014/01/09 10:35:53]

But the wrapper is supposed to be created implicitly when you project from a
primitive, you don't need to invoke Object() explicitly.

[sof, 2014/01/10 10:53:52]

Where is that requirement given? Symbol.prototype.valueOf() states that if
there's no [[SymbolData]] on its incoming 'this' (== not a wrapper object), it
should throw.

     assertEquals(undefined, symbols[i].a)
     assertEquals(undefined, symbols[i]["a" + "b"])
     assertEquals(undefined, symbols[i]["" + "1"])
@@ -203,7 +217,7 @@ function TestSet() {
     symbols[i].toString = 0
     assertThrows(function() { symbols[i].toString() }, TypeError)
     symbols[i].valueOf = 0
-    assertEquals(symbols[i], symbols[i].valueOf())
+    assertEquals(symbols[i], Object(symbols[i]).valueOf())

[rossberg, 2014/01/08 17:52:25]

Dito

     symbols[i].a = 0
     assertEquals(undefined, symbols[i].a)
     symbols[i]["a" + "b"] = 0