Upgrade Symbol implementation to match current ES6 behavior.

test/mjsunit/harmony/symbols.js

 // Copyright 2013 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 12 matching lines @@
 // DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
 // THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 // Flags: --harmony-symbols --harmony-collections
 // Flags: --expose-gc --allow-natives-syntax
 
 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.
     gc()        // Promote existing symbols and then allocate some more.
   }
 }
 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])))
   }
 }
 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__)
   }
 }
 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]))
   }
 }
 TestToString()
 
 
 function TestToBoolean() {
   for (var i in symbols) {
     assertTrue(Boolean(symbols[i]).valueOf())
     assertFalse(!symbols[i])
@@ skipping 20 matching lines @@
 
 
 function TestEquality() {
   // Every symbol should equal itself, and non-strictly equal its wrapper.
   for (var i in symbols) {
     assertSame(symbols[i], symbols[i])
     assertEquals(symbols[i], symbols[i])
     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.
   for (var i = 0; i < symbols.length; ++i) {
     for (var j = i + 1; j < symbols.length; ++j) {
       assertFalse(Object.is(symbols[i], symbols[j]))
       assertFalse(symbols[i] === symbols[j])
       assertFalse(symbols[i] == symbols[j])
     }
   }
 
   // Symbols should not be equal to any other value (and the test terminates).
   var values = [347, 1.275, NaN, "string", null, undefined, {}, function() {}]
   for (var i in symbols) {
     for (var j in values) {
       assertFalse(symbols[i] === values[j])
       assertFalse(values[j] === symbols[i])
       assertFalse(symbols[i] == values[j])
       assertFalse(values[j] == symbols[i])
     }
   }
 }
 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"])
     assertEquals(undefined, symbols[i][62])
   }
 }
 TestGet()
 
 
 function TestSet() {
   for (var i in symbols) {
     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
     assertEquals(undefined, symbols[i]["a" + "b"])
     symbols[i][62] = 0
     assertEquals(undefined, symbols[i][62])
   }
 }
 TestSet()
 
@@ skipping 126 matching lines @@
   // but not between scavenges.  This must also apply for symbol keys.
   var key = Symbol("key");
   var a = {};
   a[key] = "abc";
 
   for (var i = 0; i < 100000; i++) {
     a[key] += "a";  // Allocations cause a scavenge.
   }
 }
 TestCachedKeyAfterScavenge();