Closed Bug 645416 (harmony:symbols) Opened 13 years ago Closed 10 years ago

Implement symbols

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

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

People

(Reporter: gal, Assigned: jorendorff)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, feature)

Attachments

(31 files, 14 obsolete files)

16.32 KB, patch
luke
: review+
Details | Diff | Splinter Review
53.72 KB, patch
terrence
: review+
Details | Diff | Splinter Review
14.16 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review
31.36 KB, patch
efaust
: review+
Details | Diff | Splinter Review
82.03 KB, patch
jandem
: review+
Details | Diff | Splinter Review
2.94 KB, patch
terrence
: review+
Details | Diff | Splinter Review
7.27 KB, patch
terrence
: review+
Details | Diff | Splinter Review
23.90 KB, patch
efaust
: review+
terrence
: review+
Details | Diff | Splinter Review
29.14 KB, patch
efaust
: review+
terrence
: review+
Details | Diff | Splinter Review
9.10 KB, patch
jimb
: review+
Details | Diff | Splinter Review
1.10 KB, patch
sfink
: review+
Details | Diff | Splinter Review
1.86 KB, patch
sfink
: review+
Details | Diff | Splinter Review
4.26 KB, patch
sfink
: review+
Details | Diff | Splinter Review
5.80 KB, patch
sfink
: review+
Details | Diff | Splinter Review
7.00 KB, patch
sfink
: review+
Details | Diff | Splinter Review
5.21 KB, patch
till
: review+
Details | Diff | Splinter Review
4.58 KB, patch
efaust
: review+
Details | Diff | Splinter Review
25.49 KB, patch
efaust
: review+
terrence
: review+
jimb
: review+
Details | Diff | Splinter Review
24.25 KB, patch
terrence
: review+
Details | Diff | Splinter Review
15.30 KB, patch
efaust
: review+
Details | Diff | Splinter Review
14.82 KB, patch
efaust
: review+
Details | Diff | Splinter Review
23.92 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
2.36 KB, patch
luke
: review+
Details | Diff | Splinter Review
22.44 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
3.65 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
3.34 KB, patch
efaust
: review+
Details | Diff | Splinter Review
5.17 KB, patch
nbp
: review+
Details | Diff | Splinter Review
1018 bytes, patch
jorendorff
: feedback+
Details | Diff | Splinter Review
6.15 KB, patch
Details | Diff | Splinter Review
2.92 KB, patch
sfink
: review+
Details | Diff | Splinter Review
14.81 KB, patch
sfink
: review+
Details | Diff | Splinter Review
This is work on progress and experimental. There is no syntactic sugar yet. Method jit and tracer are not integrated yet. Usage:

var x = {}
var n1 = Name();
var n2 = Name();

x[n1] = 1
x[n2] = 2

Private names are objects of class Name. When used with array-like [] they do not convert to strings and instead become object jsids.

No enumeration shows private names.

Dave and I are still working on the semantics for proxy traps. More to come.
Attached patch patch (obsolete) — Splinter Review
Assignee: general → gal
var x = {}
var n = new Name();
var n2 = new Name();

x[n] = 1;
x[n2] = 2;

print(x[n]); // 1
print(x[n2]); // 2

var names = Object.getOwnPropertyNames(x);

print(names[0]); // [object Name]
print(x[names[0]]); // undefined

print(names[0].public == n.public); // true
Attached patch patch (obsolete) — Splinter Review
Attachment #522159 - Attachment is obsolete: true
Can you give Name an optional boolean argument that defaults to false, but if true, produces a unique-but-public name?

Dave
BTW, this is totally awesome. gal++

Brendan: Andreas is just doing this with typeof name === "object" for now, since it's more compatible and less invasive to implement. We can discuss the exact API and typeof type offline.

Dave
We can also do PublicName and PrivateName or something like that. Or the boolean flag. Whatever you prefer.

Ultimately I would suggest something like:

public x,y; === var x = PublicName(), y = PublicName();

private x,y; === var x = PrivateName(), y = PrivateName();

There is probably still some surprising things you can do with this, but maybe thats acceptable. This would work fine for example, but make little sense:

public x;

x = 12;

y[x] = ...;

Andreas
Let's leave syntax for another bug. It will be a while yet before the committee can come to agreement on that anyway, so there's no point discussing it here.

Dave
Blocks: es6
Any chances we could get this in any (non-standardized) form sometime soon ? Private names would be super helpful for our needs in Jetpack.
Blocks: 462300
No longer blocks: 462300
Blocks: 795885
A couple of things changed for this Harmony proposal. First, what used to be called "names" is now officially renamed "symbols".
There are 2 types of symbols:
* private symbols : not reflected at all (not even by Object.getOwnPropertyDescriptor)
* unique symbols : reflected by Object.getOwnPropertyDescriptor
There is no .public part any longer on any symbol.
Another difference is how symbols interact with proxies. I'm filing a separate bug for that.
There is ongoing discussion on es-discuss about what typeof should return.
Alias: harmony_symbols
Summary: implement private names → Implement symbols
Alias: harmony_symbols → harmony:symbols
Assignee: gal → jorendorff
Whiteboard: [js:ni]
Blocks: 769872
No longer blocks: 769872
Depends on: 837773
The last TC39 meeting had consensus on some things for Symbols:

* typeof symbol === "symbol"
* ToString(symbol) throws
* Object.prototype.toString.call(symbol) === "[object Symbol]"

https://github.com/rwldrn/tc39-notes/blob/master/es6/2013-03/mar-14.md#conclusionresolution
Blocks: 854030
Assignee: jorendorff → ejpbruel
Attached patch prototype (obsolete) — Splinter Review
So far I tried two approaches:
1. Add a new tag to jsval
2. Implement symbols as a JSObject

The first approach is really painful because we don't have any tag space left for 64-bit jsvals. We currently have 3 bits available for the tag and 48 bits for the payload. We currently distinguish between 8 different types, which fully uses up the tag space. We also need at least 48 bits for the payload because the virtual memory space of existing 64-bit systems is currently 48 bits wide.

I see two ways around this:
1. Use the sign bit for additional tag space
2. Shift pointer values 3 bits to the right

Both approaches come with a performance hit. The first approach requires us to add an AND operation to our double test. The second approach requires us to add a shift operation to our object-to-value and value-to-object conversions. The second approach has the advantage that it allows us tags to stay in continuous memory, so I'm currently leaning in that direction.

Another reason why changing the way we store jsvals is painful is that such a change will have to be propagated to both the interpreter and all the JITs. After discussing it with Waldo and dvander for a while, I decided to try another approach, which is to implement symbols as a JSObject that pretends to be a symbol.

This turns out to be even more painful. First of all, we don't currently have a nice way to overload the typeof, instanceof and in operators for such objects, so I had to add special case code for all of those. Even worse, when such objects are passed from one compartment to another, they are wrapped, making them unrecognizable as symbols, so all this special case code (which is based on the object class) breaks down. I have not yet found a way around this.

I'm putting what I have so far up for feedback, but the patch is incomplete, and currently fundamentally broken for the reason I just mentioned. At this stage, I really think we should just bite the bullet, and open up more tag space in jsval. We'll take a performance hit by doing this, and it will probably be an implementation nightmare, but given that TC39 are planning on adding even more primitive types in the future, its probably the right thing to do (can we somehow get a consensus on this from the rest of the team?)

As a final note, if we decide to go down that road, I don't think symbols will land any time soon. I was gambling I could come up with a working implementation in under a week, but it looks like I misjudged the difficulty involved. I'd like to get back to working on modules. so I'm not going to be able to work full time on this.
Attachment #736867 - Flags: feedback?(jwalden+bmo)
Attached patch prototype (obsolete) — Splinter Review
Attachment #736867 - Attachment is obsolete: true
Attachment #736867 - Flags: feedback?(jwalden+bmo)
Attachment #736868 - Flags: feedback?(jwalden+bmo)
In regards to typeof, the discussion at the last TC39 meeting on that can be found here [1]. The desire for future new typeofs, e.g. int64/struct/etc., is pretty high.


[1] https://github.com/rwldrn/tc39-notes/blob/master/es6/2013-03/mar-14.md#46-symbols
A potential idea for the JSObject path would be to have a type of object that can be passed around unwrapped. This wouldn't work for something like structs, but for immutables such as symbols and values types (int64, etc.) it could work. Since they're immutable from birth they don't need to be wrapped, right?
Expanding on my previous post, `symbol.valueOf` and `symbol.toString` aren't supposed to exist, per spec. A symbol should be prototypeless and immutable. This is why it would be safe to pass around unwrapped.
(In reply to Brandon Benvie [:bbenvie] from comment #18)
> Expanding on my previous post, `symbol.valueOf` and `symbol.toString` aren't
> supposed to exist, per spec. A symbol should be prototypeless and immutable.
> This is why it would be safe to pass around unwrapped.

And so they don't:
"toString" in Symbol() => false

However:
Symbol().toString()

Is legal in the same way:
"123".toString()

is legal
let sym = Symbol();

Object.prototype.toString.call(sym) => "[object Symbol]"

sym + ""; // throws a TypeError exception

The same should hold for:
sym.toString() 
Symbol().toString()
(In reply to Rick Waldron from comment #20)
> let sym = Symbol();
> 
> Object.prototype.toString.call(sym) => "[object Symbol]"
> 
> sym + ""; // throws a TypeError exception
> 
> The same should hold for:
> sym.toString() 
> Symbol().toString()

I was under the impression that:
- Symbol().toString() should return "[symbol]
- The Symbol constructor optionally takes a string to be displayed when converting
  a symbol to a string.

There were a couple of back and forths between jorendorff and dherman about this, but I don't remember the specifics. dherman, could you please clarify?

Also, how can Object.prototype.toString.call(sym) => "[object Symbol]" if symbols are not objects? On the other hand, Object.prototype.toString.call(new Symbol()) => "[object Symbol]" makes sense, and is what my current implementation does.
Flags: needinfo?(dherman)
> Also, how can Object.prototype.toString.call(sym) => "[object Symbol]" if symbols are not objects? On the other hand, Object.prototype.toString.call(new Symbol()) => "[object Symbol]" makes sense, and is what my current implementation does.

:) those are the same operation, and your current implementation is correct.


As for the optional Symbol name, that's at the .name property:

let s = Symbol("foo");
s.name === "foo";
Rick, correct me if any of this wrong, but I believe these tests demonstrate the desired behavior of Symbols: https://gist.github.com/Benvie/5375078. This differs significantly from the tests in Eddy's patch and, if correct, I think will help illuminate what I was saying about immutable prototypeless options.
(In reply to Eddy Bruel [:ejpbruel] from comment #14)
> The first approach is really painful because we don't have any tag space
> left for 64-bit jsvals. We currently have 3 bits available for the tag and
> 48 bits for the payload. We currently distinguish between 8 different types,
> which fully uses up the tag space.

I'm probably missing something, but AFAICS the layout of a double is like this:

-  1 sign bit
- 11 exponent bits
- 52 mantissa bits

We use 47 bits for the payload, so that leaves 17 bits for the tag. Of these 17 bits, 1 bit is the sign bit and 11 bits are the exponent, shouldn't that leave like 5 bits for the type tag instead of 3?
I have been saying that on IRC, but nobody believed me :(
I actually changed the object tag to 9 and everything worked just fine in the interpreter.
(In reply to Tom Schuster [:evilpie] from comment #27)
> I actually changed the object tag to 9 and everything worked just fine in
> the interpreter.

Tom, I owe you an apology. You were right all along.

Let me try to explain where I went wrong: the value of JSVAL_TAG_MAX_DOUBLE is 0x1FFF0. This is shifted JSVAL_TAG_SHIFT == 47 bits to the right to create JSVAL_SHIFTED_TAG_MAX_DOUBLE, which looks like this in memory:

1111 1111 1111 1xxx 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000

My understanding of Nan-boxing is that we need at least 48 bits for the payload, because current 64-bit systems have virtual memory size that is 48 bits in size. This, coupled with the fact that the maximum value of JSVAL_TYPE_* is 0x07 led me to believe that those 3 bits marked x are what's currently available for tag space.

However, on closer investigation, that's *not* what we currently do, and if I had checked JSVAL_PAYLOAD_MASK more closely, I would have spotted it. What's actually going on is that JSVAL_SHIFTED_TAG_MAX_DOUBLE looks like this:

1111 1111 1111 1xxx x000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000

The 4 bits marked x are 0 for TAG_MAX_DOUBLE, but the other tags a created by OR'ing in the value of the corresponding JSVAL_TYPE_*. So that, for instance, the value of JSVAL_SHIFTED_TAG_OBJECT looks like this in memory:

1111 1111 1111 1011 1000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000

And not like this (as I expected):

1111 1111 1111 1111 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000

I don't know why having a 47 bit payload is safe (all the articles I read about Nan boxing suggested that its not) but since this is already what we use, I see no problem with that.
(In reply to Eddy Bruel [:ejpbruel] from comment #14)
> Created attachment 736867 [details] [diff] [review]
> prototype
> 
> So far I tried two approaches:
> 1. Add a new tag to jsval
> 2. Implement symbols as a JSObject
> 
> The first approach is really painful because we don't have any tag space
> left for 64-bit jsvals. We currently have 3 bits available for the tag and
> 48 bits for the payload. We currently distinguish between 8 different types,
> which fully uses up the tag space. We also need at least 48 bits for the
> payload because the virtual memory space of existing 64-bit systems is
> currently 48 bits wide.
> 
> I see two ways around this:
> 1. Use the sign bit for additional tag space
> 2. Shift pointer values 3 bits to the right
> 
> Both approaches come with a performance hit. The first approach requires us
> to add an AND operation to our double test. The second approach requires us
> to add a shift operation to our object-to-value and value-to-object
> conversions. The second approach has the advantage that it allows us tags to
> stay in continuous memory, so I'm currently leaning in that direction.
> 
> Another reason why changing the way we store jsvals is painful is that such
> a change will have to be propagated to both the interpreter and all the
> JITs. After discussing it with Waldo and dvander for a while, I decided to
> try another approach, which is to implement symbols as a JSObject that
> pretends to be a symbol.
> 
> This turns out to be even more painful. First of all, we don't currently
> have a nice way to overload the typeof, instanceof and in operators for such
> objects, so I had to add special case code for all of those. Even worse,
> when such objects are passed from one compartment to another, they are
> wrapped, making them unrecognizable as symbols, so all this special case
> code (which is based on the object class) breaks down. I have not yet found
> a way around this.
> 
> I'm putting what I have so far up for feedback, but the patch is incomplete,
> and currently fundamentally broken for the reason I just mentioned. At this
> stage, I really think we should just bite the bullet, and open up more tag
> space in jsval. We'll take a performance hit by doing this, and it will
> probably be an implementation nightmare, but given that TC39 are planning on
> adding even more primitive types in the future, its probably the right thing
> to do (can we somehow get a consensus on this from the rest of the team?)
> 
> As a final note, if we decide to go down that road, I don't think symbols
> will land any time soon. I was gambling I could come up with a working
> implementation in under a week, but it looks like I misjudged the difficulty
> involved. I'd like to get back to working on modules. so I'm not going to be
> able to work full time on this.

luke just pointed out to me that we've since added a way to query a JSObject for its class that is proxy aware in the form of ObjectClassIs. That solves the problem of primitive symbols implemented as JSObjects being wrapped across compartments (as explained in comment 14)
Summarizing, it looks like we have a way to move forward regardless of whether symbols will end up being specced as objects or primitive values. The consensus still seems to be that adding a new value tag is not the way to go, so we'll end up implementing symbols as JSObject's in either case.

I'm going to hold off on this bug for a while, because apparently some people came away from the TC39 meeting with a different interpretation of what it meant. As soon as that's resolved, we can move forward again.
Comment on attachment 736868 [details] [diff] [review]
prototype

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

Sorry for taking so long to get to this.

As far as this goes, it seems fine enough.  But the whole hard part is dealing with all the places that test isObject(), or isPrimitive() -- since you're making isPrimitive() sometimes be true when isObject() is.  It seems really bad, for everyone's sanity, for isPrimitive() to ever be equal (either way) to isObject().  You aren't dealing with that at all, and that seems like the only truly interesting part of the whole problem.

It still feels to me like having new primitive types, that are not subtypes of object (as was the case in a certain other language extension that added a new typeof), is just a bad idea.  It's fighting against way too much specification legacy, and JS code legacy.  Sigh.  :-\  Honestly I don't understand how anyone thinking it through could think there's enough value to a new non-subobject-typeof to warrant the complexity it poses both to implementations and to JS code.

::: js/src/builtin/Symbol.cpp
@@ +80,5 @@
> +        /* TODO: If the argument passed is convertible to a symbol, use that
> +         * instead. This will make Symbol(symbol) and new Symbol(symbol) work
> +         * correctly. We need to add an new hint type to the defaultValue trap
> +         * to distinguish between property id's and strings before we can
> +         * implement this.

I'm not sure why hacking the defaultValue trap is really right here.  I'd think we could just class-check/ObjectClassIs here like we do everywhere.  The ToPrimitive and hint mechanism is pretty baroque, and not really something we want to extend.

@@ +90,5 @@
> +        if (!symbol)
> +            return false;
> +    } else {
> +        /* If no string was passed as an argument, the default string
> +         * associated with the symbol is "[symbol]".

If this is the case, we should add it to CommonPropertyNames.h.

::: js/src/jit-test/tests/symbol/symbol-valueOf.js
@@ +4,5 @@
> +assertEq(Symbol("foo").valueOf(), "foo");
> +assertEq(Symbol("foo") + 42, "foo42");
> +
> +assertEq(typeof (new Symbol()).valueOf, "function");
> +assertEq((new Symbol()).valueOf(), "[symbol]");

Prior valueOf semantics would have valueOf() return the symbol primitive, I would think, not a string.  Primitivizing to a string primitive seems pretty bizarre to me.
Attachment #736868 - Flags: feedback?(jwalden+bmo)
Jeff, This is on the agenda for the upcoming meeting May 21-13, it might be worth holding off on any further development until after that.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #31)
> It still feels to me like having new primitive types, that are not subtypes
> of object (as was the case in a certain other language extension that added
> a new typeof), is just a bad idea.  It's fighting against way too much
> specification legacy, and JS code legacy.  Sigh.  :-\  Honestly I don't
> understand how anyone thinking it through could think there's enough value
> to a new non-subobject-typeof to warrant the complexity it poses both to
> implementations and to JS code.
I empathize for the implementation, but I think it's good for JS code.
Here are a couple of things that make symbols not an object:
* Currently in "a[o]", if o is an object, it's first coerced to a string which goes against the purpose of symbols.
* If symbols were objects, it would make sense to wrap them with proxies, which actually doesn't make sense at all since the only interesting property of a symbol is its identity and a proxy would create a new identity.
* symbols are not expected to have own properties


> ::: js/src/jit-test/tests/symbol/symbol-valueOf.js
> @@ +4,5 @@
> > +assertEq(Symbol("foo").valueOf(), "foo");
> > +assertEq(Symbol("foo") + 42, "foo42");
> > +
> > +assertEq(typeof (new Symbol()).valueOf, "function");
> > +assertEq((new Symbol()).valueOf(), "[symbol]");
> 
> Prior valueOf semantics would have valueOf() return the symbol primitive, I
> would think, not a string.  Primitivizing to a string primitive seems pretty
> bizarre to me.
Agreed.
(In reply to David Bruant from comment #33)
> * Currently in "a[o]", if o is an object, it's first coerced to a string
> which goes against the purpose of symbols.

The previous incarnation of Symbols in the ES6 spec draft had ToPropertyKey automatically handling this issue by causing the symbol to not be coerced to a string when used in operations as keys on objects.

> * If symbols were objects, it would make sense to wrap them with proxies,
> which actually doesn't make sense at all since the only interesting property
> of a symbol is its identity and a proxy would create a new identity.

This is a legit problem, though I think it might be trivially resolved by disallowing Symbols (and any other future value objects) from being proxied.

> * symbols are not expected to have own properties

This was also resolved in the previous ES6 spec draft by making Symbol objects essentially immutable frozen-from-birth objects.
In the future we probably want other primitives, so I think we should take the time and design something that allows this to happen.
> In the future we probably want other primitives, so I think we should take
> the time and design something that allows this to happen.

Not too much time, please -- symbols are depended on by multiple other features, including for-of, generators, classes, and super.

Dave
Flags: needinfo?(dherman)
(In reply to Eddy Bruel [:ejpbruel] from comment #21)
> I was under the impression that:
> - Symbol().toString() should return "[symbol]

That was the original design, but TC39 decided in March that Symbol.prototype.toString throws:

https://github.com/rwldrn/tc39-notes/blob/master/es6/2013-03/mar-14.md#46-symbols

IIRC, Jason and I had a conversation afterwards where we decided that this was probably not the best design, but it is the current consensus. Jason and I should go through our reasoning and see if we can come up with an alternative to bring back to TC39.

> - The Symbol constructor optionally takes a string to be displayed when
> converting
>   a symbol to a string.

Yes, I believe this should still be the case for debugging, but if the toString conversion throws then that's not the place where this value would be exposed. V8's current implementation exposes this via a .name property.

> There were a couple of back and forths between jorendorff and dherman about
> this, but I don't remember the specifics. dherman, could you please clarify?

See above, but I do want to discuss with jorendorff about the toString situation.

> Also, how can Object.prototype.toString.call(sym) => "[object Symbol]" if
> symbols are not objects?

The current draft says in step 3 of 15.2.4.2 that this produces "[object Symbol]", so that's correct. The rationale I would give is that Object.prototype.toString returns the string representation of the object version of the receiver. If you call it on a primitive boolean, you get the Boolean object version of that boolean. If you call it on a primitive number, you get the Number object version of that number. So if you call it on a primitive symbol, you get the Symbol object version of that symbol.

> On the other hand,
> Object.prototype.toString.call(new Symbol()) => "[object Symbol]" makes
> sense, and is what my current implementation does.

Right, and Object.prototype.toString.call(sym) should produce the same result, for the reason I gave above.

Dave
It looks like the spec is changing back to what it was before with Symbols as objects.

http://esdiscuss.org/topic/how-primitive-are-symbols-bignums-etc#content-7
(In reply to Brandon Benvie [:bbenvie] from comment #38)
> It looks like the spec is changing back to what it was before with Symbols
> as objects.
> 
> http://esdiscuss.org/topic/how-primitive-are-symbols-bignums-etc#content-7

Note, however, that there's still some controversy about this. Especially Andreas Rossberg is questioning the theory behind the change in
https://mail.mozilla.org/pipermail/es-discuss/2013-July/031962.html
(In reply to Brandon Benvie [:bbenvie] from comment #38)
> It looks like the spec is changing back to what it was before with Symbols
> as objects.
> 
> http://esdiscuss.org/topic/how-primitive-are-symbols-bignums-etc#content-7

(In reply to Till Schneidereit [:till] from comment #39)
> Note, however, that there's still some controversy about this. Especially
> Andreas Rossberg is questioning the theory behind the change in
> https://mail.mozilla.org/pipermail/es-discuss/2013-July/031962.html

We might as well only post TC39 meeting notes, rather than es-discuss debats ;-)
Blocks: 918828
Keywords: feature
OS: Mac OS X → All
Hardware: x86 → All
Is anyone currently working on this feature? I'd like to rendezvous to discuss implications around the SIMD value types and other typed object support.
Assignee: nobody → jorendorff
I'm posting some work in progress for terrence to look at...
Attached patch bug-645416-part-2-api-v1.patch (obsolete) — Splinter Review
Comment on attachment 8425815 [details] [diff] [review]
bug-645416-part-3-allocation-v1.patch Option B: Symbols share an AllocKind and layout with dependent strings

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

As discussed on IRC.
Attachment #8425815 - Flags: feedback?(terrence) → feedback-
Comment on attachment 8425814 [details] [diff] [review]
bug-645416-part-3-allocation-v1.patch Option A: Symbols as a separate AllocKind

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

::: js/src/vm/Symbol.h
@@ +38,5 @@
> +
> +    JSString *description() { return description_; }
> +
> +    static inline js::ThingRootKind rootKind() { return js::THING_ROOT_SYMBOL; }
> +    inline void markDescription(JSTracer *trc);

Please call this Symbol::trace, per convention. E.g. it is tracing the Symbol's outbound edges: the fact that this calls Mark on some field is an implementation detail.
Attachment #8425814 - Flags: feedback?(terrence) → feedback+
(In reply to Terrence Cole [:terrence] from comment #48)
> Please call this Symbol::trace, per convention.

I was following the convention shown in the code below; all of this except for the Symbol implementation is pre-existing code.

> void
> gc::MarkChildren(JSTracer *trc, JSObject *obj)
> {
>     obj->markChildren(trc);
> }
> 
> static void
> gc::MarkChildren(JSTracer *trc, JSString *str)
> {
>     if (str->hasBase())
>         str->markBase(trc);
>     else if (str->isRope())
>         str->asRope().markChildren(trc);
> }
> 
> static void
> gc::MarkChildren(JSTracer *trc, JS::Symbol *sym)
> {
>     sym->markDescription(trc);
> }
> 
> static void
> gc::MarkChildren(JSTracer *trc, JSScript *script)
> {
>     script->markChildren(trc);
> }
> 
> static void
> gc::MarkChildren(JSTracer *trc, LazyScript *lazy)
> {
>     lazy->markChildren(trc);
> }
> 
> static void
> gc::MarkChildren(JSTracer *trc, Shape *shape)
> {
>     shape->markChildren(trc);
> }

Should I really change it?
Sure, that's the pattern in the code here. There are other more correct patterns. I guess for now you should call it markChildren to match the others, although still definitely not markDescriptor.

The problem here is that these methods all take a generic JSTracer, so "mark" is a total lie. Really there is only one conceptual primitive that the methods here deal with: tracing. It is an implementation detail of one particular JSTracer sub-class, GCMarker, that marking happens. This sort of accidental conflation of terms is why the GC is so confusing currently.
Attached file symbol-patches.tar.gz (obsolete) —
Snapshot of my stack.
Attachment #522166 - Attachment is obsolete: true
Attachment #736868 - Attachment is obsolete: true
Attachment #8425811 - Attachment is obsolete: true
Attachment #8425812 - Attachment is obsolete: true
Attachment #8425814 - Attachment is obsolete: true
Attachment #8425815 - Attachment is obsolete: true
Terrence, with these changes, Comparment::wrapId() becomes a no-op, and I'm removing it. However, oddly, that leaves this in JS_WrapId:

JS_PUBLIC_API(bool)
JS_WrapId(JSContext *cx, JS::MutableHandleId idp)
{
  AssertHeapIsIdle(cx);
  CHECK_REQUEST(cx);

  // <---- this is where we used to wrap idp

  jsid id = idp.get();
  if (JSID_IS_STRING(id))
      JS::ExposeGCThingToActiveJS(JSID_TO_STRING(id), JSTRACE_STRING);
  else if (JSID_IS_SYMBOL(id))
      JS::ExposeGCThingToActiveJS(JSID_TO_STRING(id), JSTRACE_STRING);
  return true;
}

Since idp is not being modified anymore, I can just delete this.  ...Right?
Flags: needinfo?(terrence)
That appears to be a read barrier. I'd guess it was added for a reason and that you'd still need the read barrier. Please do some archeology to figure out why it was added.
Flags: needinfo?(terrence)
Depends on: 1017067
(In reply to Terrence Cole [:terrence] from comment #54)
> That appears to be a read barrier. I'd guess it was added for a reason and
> that you'd still need the read barrier. Please do some archeology to figure
> out why it was added.

This was a whole saga. To make a long story short, that is there for the cycle collector (not incremental GC), and billm says it'll be fine to delete it.
Attached file symbol-patches-snapshot-2.tar.gz (obsolete) —
Feature-complete, down to updates for the gdb scripts and a test for __noSuchMethod__ when the method name is a symbol.

Likely full of bugs. I wanted to try-server it tonight, but hotel wifi is not cooperating.
Attachment #8428127 - Attachment is obsolete: true
P.S. Today I learned that self-test/assertDeepEq.js is a nightmare scenario for --ion-eager. With no flags, it runs in 2.6 seconds; with --ion-eager --ion-parallel-compile=off, 1 minute 34.8 seconds. Updating it for symbols found a bug in my jit patch---but only one, so I'm calling that a win...
Update: I've been hacking on this in spare moments at the bootcamp. Lots of try servering. Two bugs remain.

1.  Something's wrong with the MemoryMetrics accounting:

        Assertion failure: gcTotal == rtStats.gcHeapChunkTotal, at c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\js\xpconnect\src\XPCJSRuntime.cpp:2468 

    Should be easy to debug, right?

2.  jsapi-tests testNewRuntime crashes reliably on every machine but mine
Attached patch Part 1 - JSValueType (obsolete) — Splinter Review
Bug 645416, part 1 - Add an enum for symbols to JSValueType.

JSVAL_TYPE_SYMBOL is inserted between STRING and NULL, rather than added at the
end, in order to preserve all the inequality relations on JSValueTypes used
throughout Value.h. See JSVAL_LOWER_INCL_TYPE_OF_OBJ_OR_NULL_SET and others; or
just grep the header for the operators < > <= >=.

Otherwise, this pretty much just works. However, Ion snapshots no longer pack
quite so nicely. Changes to the format are needed, and that's most of the
patch. This costs us a byte per snapshot.
Attachment #8436233 - Flags: review?(nicolas.b.pierron)
Attached patch Part 2 - ValueSplinter Review
Bug 645416, part 2 - Add support for symbols to JS::Value.

The API for symbol Values is much like the API for strings.

The implementation behind all this is in a later patch. Here, a class
JS::Symbol is declared, but not defined anywhere yet.
Attachment #8436234 - Flags: review?(luke)
Comment on attachment 8436233 [details] [diff] [review]
Part 1 - JSValueType

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

::: js/src/jit/Snapshots.cpp
@@ -289,5 @@
>          p->fpu = FloatRegister::FromCode(reader.readByte());
>          break;
> -      case PAYLOAD_PACKED_TAG:
> -        p->type = JSValueType(*mode & 0x07);
> -        *mode = *mode & ~0x07;

We want to keep the tag as a packed tag instead of having it in a separated field.  This is a size concern that we have for encoding the snapshots.

Currently the RValueAllocations are padded on multiple of ALLOCATION_TABLE_ALLIGNMENT (= 2) and having a tag as a separate fields implies that the most common cases would be encoded on 3 bytes instead of being encoded on 2 bytes.  Encoding on 3 bytes with a padding of 2 will make us generate spare indexes for encoding the snapshots which will take much more memory.

I think, the easiest is to modify RValueAllocation::Mode[1] such as the *_MIN and *_MAX ranges are able to contain the various JSValueTag that we want to encode, and change the previous code such as we mask with larger bit mask than 0x07.  (maybe we should have a constant for that) which is defined next to the RValueAllocation::Mode.

Also, if you add new kind of values, make sure to add the corresponding test cases in jsapi-tests[2].

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/Snapshots.h#79
[2] http://dxr.mozilla.org/mozilla-central/source/js/src/jsapi-tests/testJitRValueAlloc.cpp
Attachment #8436233 - Flags: review?(nicolas.b.pierron)
Bug 645416, part 3 - Symbol layout and GC support for allocating them.

Layout: js/src/vm/Symbol.h defines the new class JS::Symbol. JS::Symbol is the
same size as JSString on all platforms, because the allocator does not support
smaller allocations.

Allocation: Since the purpose of symbols is to serve as property keys, they are
always allocated in the atoms compartment.

We take a lock when allocating. This could probably be replaced with a
main-thread-only assertion. However, if atom allocation is not already a
bottleneck, symbol allocation probably never will be.

Symbols are given their own finalize-class in the GC. This means we allocate a
page per zone for symbols, even though they are only ever allocated in the
atoms zone. Terrence thought this could be easily fixed later. It should be; we
never touch the page, but a 32-bit virtual address space does not just have
infinite pages to spare.

A jsapi-test exercises the new symbol allocation code. A few oddities in
jsapi-tests are fixed in passing.
Attachment #8436237 - Flags: review?(terrence)
Whiteboard: [js:ni]
Bug 645416, part 4 - Rename DefinePropertiesAndBrand -> DefinePropertiesAndFunctions.

Not strictly symbols-related. I can move this out of the symbols stack if anyone cares, but I'm not going to renumber the 25 (or whatever) patches on top of it. There will just have to be a hole.
Attachment #8436254 - Flags: review?(bhackett1024)
Bug 645416, part 5 - Add the Symbol constructor and Symbol wrapper objects.

This exposes a new primitive type to scripts for the first time since
JavaScript was first released, over 13 years ago.

The tests focus on identity, equality, and being able to pass a symbol around
as a value. Of course the point of symbols is that they can be property keys,
but that will have to wait for a later patch in this series.
Attachment #8436267 - Flags: review?(luke)
Bug 645416, part 6 - JIT support for symbol values.

Symbols are not yet supported as property keys at this point in the stack. The work here is to pass symbol pointers around in Ion JIT code unboxed.

The baseline compiler doesn't need much new code. A few kinds of ICs need to know all the primitive types.

This could have more tests; I only added tests for bugs that turned up after the initial patch was written. What kind of thing should I specially test for?
Attachment #8436268 - Flags: review?(jdemooij)
Attachment #8436272 - Flags: review?(terrence)
Bug 645416, part 8 - Support passing symbols across compartment boundaries.

Trivial. Since symbols are always allocated in the atoms compartment and all compartments are allowed to have direct references to them, Compartment::wrap on a symbol is a no-op.

...The question for the reviewer is if there's anyplace else I need to touch. I don't think so.
Attachment #8436273 - Flags: review?(terrence)
Attachment #8436279 - Flags: review?(efaustbmo)
Attachment #8436267 - Flags: review?(luke) → review?(efaustbmo)
Comment on attachment 8436279 [details] [diff] [review]
Part 9 - Symbol.for

r?terrence for the GC bits, r?efaust for everything else
Attachment #8436279 - Flags: review?(terrence)
Bug 645416, part 10 - Well-known symbols.

At present there is only one, Symbol.iterator, and it is not hooked up to
anything (this happens in bug 918828). ES6 defines 8 well-known symbols. Each
one is attached to a feature, so we'll add the symbols as we add features.
Symbol.create will appear when @@create semantics are implemented.
Attachment #8436283 - Flags: review?(efaustbmo)
Attachment #8436284 - Flags: review?(jimb)
Attachment #8436283 - Flags: review?(terrence)
Attachment #8436285 - Flags: review?(sphink)
Bug 645416, part 13 - Update ToNumber for symbols.

Not terribly interesting; converting a symbol to a number always produces NaN.
Attachment #8436286 - Flags: review?(sphink)
Attached patch Part 14 - ToString (obsolete) — Splinter Review
Bug 645416, part 14 - Update ToString for symbols.

The change in jit-test/tests/symbol/toString.js is that we now check that an
exception is actually thrown. Until this patch, stringifying a symbol did not
throw. (The test was mainly checking that we did not assert in Ion.)

No changes in Ion. If a symbol is being stringified, it's ok to be in a slow
path because that is going to throw anyway.
Attachment #8436287 - Flags: review?(sphink)
Attachment #8436289 - Flags: review?(sphink)
Bug 645416, part 16 - Implement Symbol.prototype.valueOf.
Attachment #8436292 - Flags: review?(sphink)
Bug 645416, part 17 - Implement ToPrimitive on Symbol wrapper objects.

The spec defines this by way of a @@toPrimitive method. We fake it using a
JSClass::convert hook. (Once @@toPrimitive is implemented, convert hooks
can be removed entirely, but we need symbols first.)
Attachment #8436294 - Flags: review?(sphink)
Attached patch Part 18 - ValueToSource (obsolete) — Splinter Review
Attachment #8436295 - Flags: review?(sphink)
Attachment #8436297 - Flags: review?(till)
Bug 645416, part 20 - Add JS::Symbol::dump() method for debugging.
Attachment #8436298 - Flags: review?(efaustbmo)
OK, right after this is where it gets interesting: allowing symbols as property keys. There are 9 more patches, but I am going to have to be done for the night. In the meantime, if you're interested:

https://tbpl.mozilla.org/?tree=Try&rev=85874a91743b
Comment on attachment 8436297 [details] [diff] [review]
Part 19 - assertDeepEq

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

How very exciting!

::: js/src/jit-test/tests/self-test/assertDeepEq.js
@@ +23,5 @@
> +assertDeepEq([Symbol(), Symbol(), Symbol()],
> +             [Symbol(), Symbol(), Symbol()]);
> +var sym = Symbol();
> +assertNotDeepEq([sym, sym], [Symbol(), Symbol()]);
> +assertNotDeepEq([sym, sym], [Symbol(), sym]);

For completeness' sake maybe add
assertDeepEq([sym, sym], [sym, sym]);
?

Also, I did not know that that's how assertDeepEq works. Good to know :)
Attachment #8436297 - Flags: review?(till) → review+
Comment on attachment 8436284 [details] [diff] [review]
Part 11 - GDB pretty-printers

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

::: js/src/gdb/mozilla/JSSymbol.py
@@ +1,5 @@
> +# Pretty-printers for SpiderMonkey strings.
> +
> +import gdb
> +import mozilla.prettyprinters
> +from mozilla.prettyprinters import ptr_pretty_printer

It's dumb, but you should include the following lines here:

# Forget any printers from previous loads of this module.
mozilla.prettyprinters.clear_module_printers(__name__)

@@ +22,5 @@
> +        elif code == UniqueSymbol:
> +            return "Symbol({})".format(desc)
> +        else:
> +            # Well-known symbol. Strip off the quotes added by the JSString *
> +            # pretty-printer.

Since description_ is a JSAtom *, I would expect you could say JSAtomPtr(self.value['description_']).to_string() to get the string contents directly.

Are you sure stripping off the quotes is a good idea, though? It makes it look like an enum value.
Attachment #8436284 - Flags: review?(jimb) → review+
Comment on attachment 8436234 [details] [diff] [review]
Part 2 - Value

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

::: js/public/Value.h
@@ +190,5 @@
>  #define JSVAL_SHIFTED_TAG_NULL       (((uint64_t)JSVAL_TAG_NULL)       << JSVAL_TAG_SHIFT)
>  #define JSVAL_SHIFTED_TAG_OBJECT     (((uint64_t)JSVAL_TAG_OBJECT)     << JSVAL_TAG_SHIFT)
>  
>  #endif  /* JS_PUNBOX64 */
>  #endif  /* !defined(__SUNPRO_CC) && !defined(__xlC__) */

Pre-existing but can you remove JSVAL_LOWER_INCL_TYPE_OF_OBJ_OR_NULL_SET, JSVAL_UPPER_EXCL_TYPE_OF_PRIMITIVE_SET, JSVAL_UPPER_INCL_TYPE_OF_NUMBER_SET, JSVAL_LOWER_INCL_TYPE_OF_PTR_PAYLOAD_SET?  I'm guessing these were used for TraceMonkey.

@@ +604,5 @@
>  JSVAL_IS_TRACEABLE_IMPL(jsval_layout l)
>  {
> +    return l.s.tag == JSVAL_TAG_STRING ||
> +           l.s.tag == JSVAL_TAG_SYMBOL ||
> +           l.s.tag == JSVAL_TAG_OBJECT;

At this point, it's probably more efficient to use the definition of JSVAL_IS_TRACEABLE_IMPL in the JS_PUNBOX64 path, at which point in time you can pull JSVAL_IS_TRACEABLE_IMPL out of the #ifdef.
Attachment #8436234 - Flags: review?(luke) → review+
Comment on attachment 8436237 [details] [diff] [review]
Part 3 - Layout and allocation

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

Seems okay to me.

::: js/src/jsapi-tests/testSymbol.cpp
@@ +8,5 @@
> +{
> +    using namespace JS;
> +
> +    RootedString desc(cx, nullptr);
> +    Rooted<Symbol*> sym1(cx);

I think you're going to want JS:: prefixes on these. We seem to need them or not, depending on what gets pulled into any particular unified build.

Also please use either the templates or typedefs, not both.

::: js/src/jsapi.h
@@ +4406,5 @@
> + * This function is infallible. If it returns null, that means the symbol's
> + * [[Description]] is undefined.
> + */
> +JS_PUBLIC_API(JSString *)
> +GetSymbolDescription(Handle<Symbol*> symbol);

Use the HandleSymbol typedef, per our style guide. I know jsapi.h is a mess in this respect right now, but no need to make it worse.

::: js/src/vm/Symbol.h
@@ +32,5 @@
> +    void operator=(const Symbol &) MOZ_DELETE;
> +
> +  public:
> +    static Symbol *
> +    new_(js::ExclusiveContext *cx, JSString *description);

This is only 74 columns, so 1 line should be fine.

@@ +40,5 @@
> +    static inline js::ThingRootKind rootKind() { return js::THING_ROOT_SYMBOL; }
> +    inline void markChildren(JSTracer *trc);
> +    inline void finalize(js::FreeOp *) {}
> +};
> +

Please add a static_assert(sizeof(Symbol) == 8 + (8 * JS_BITS_PER_WORD / 32)) or thereabouts.
Attachment #8436237 - Flags: review?(terrence) → review+
Comment on attachment 8436272 [details] [diff] [review]
Part 7 - Map and Set

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

r=me

::: js/src/tests/ecma_6/WeakMap/symbols.js
@@ +3,5 @@
> +
> +// Symbols can't be WeakMap keys.
> +var m = new WeakMap;
> +var sym = Symbol();
> +assertThrowsInstanceOf(() => m.set(sym, 0), TypeError);

Nice!
Attachment #8436272 - Flags: review?(terrence) → review+
Comment on attachment 8436273 [details] [diff] [review]
Part 8 - Compartments

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

Compartment assertions should have caught you out if there were other places to update. (I don't know of any.)

::: js/src/jscompartmentinlines.h
@@ +56,5 @@
> +     * Only GC things have to be wrapped or copied. Symbols are an exception:
> +     * they are GC things, but never need to be wrapped or copied because they
> +     * are always allocated in the atoms compartment.
> +     */
> +    if (!vp.isMarkable() || vp.isSymbol())

Please make the new test (and comment) a separate if-statement to make it clear that we are filtering two distinct conditions here.
Attachment #8436273 - Flags: review?(terrence) → review+
Bug 645416, part 21 - Add symbol jsids (SYMBOL_TO_JSID), removing the legacy support for object jsids (OBJECT_TO_JSID).

With just this patch, there are not actually any symbol jsids flowing through
the system, just as there are not actually any object jsids. But a subsequent
patch (part 23) changes this.

This patch deletes some code in CTypes.cpp that is simply confused about how
element accesses work: Int64 and UInt64 objects were never actually converted
to object jsids, so the code being removed here was already dead code.

r?terrence for the MarkSymbolUnbarriered bits

r?jimb for the js/src/gdb bits

r?efaust for everything else
Attachment #8437077 - Flags: review?(terrence)
Attachment #8437077 - Flags: review?(jimb)
Attachment #8437077 - Flags: review?(efaustbmo)
Comment on attachment 8436279 [details] [diff] [review]
Part 9 - Symbol.for

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

r=me

::: js/src/jsapi-tests/testSymbol.cpp
@@ +32,5 @@
> +    using namespace JS;
> +
> +    RootedString desc(cx, JS_NewStringCopyZ(cx, "ponies"));
> +    CHECK(desc);
> +    Rooted<Symbol*> sym1(cx);

Same as for patch 3.

::: js/src/tests/ecma_6/Symbol/equality.js
@@ +1,5 @@
>  /* Any copyright is dedicated to the Public Domain.
>   * http://creativecommons.org/licenses/publicdomain/ */
>  
> +// Symbol.for returns the same symbol whenever the same argument is passed.
> +assertEq(Symbol.for("q") === Symbol.for("q"), true);

Why is assertEq(Symbol.for("q"), Symbol.for("q")) not appropriate here?

::: js/src/vm/Symbol.cpp
@@ +20,5 @@
>  Symbol *
> +Symbol::newInternal(ExclusiveContext *cx, bool inRegistry, JSAtom *description)
> +{
> +    MOZ_ASSERT(cx->compartment() == cx->atomsCompartment());
> +    MOZ_ASSERT(cx->atomsCompartment()->runtimeFromAnyThread()->currentThreadHasExclusiveAccess());

\o/

::: js/src/vm/Symbol.h
@@ +44,5 @@
>      static Symbol *
> +    new_(js::ExclusiveContext *cx, bool inRegistry, JSString *description);
> +
> +    static Symbol *
> +    for_(js::ExclusiveContext *cx, js::HandleString description);

One line.
Attachment #8436279 - Flags: review?(terrence) → review+
Bug 645416, part 22 - Remove JSCompartment::wrapId.

This is unnecessary now that object jsids no longer exist. Both string and
symbol jsids point only to GC things in the atoms compartment, which are safe
to pass to any compartment without wrapping.
Attachment #8437090 - Flags: review?(terrence)
Comment on attachment 8436283 [details] [diff] [review]
Part 10 - Well-known symbols.

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

::: js/src/jsapi.h
@@ +4437,5 @@
> +const size_t WellKnownSymbolLimit = 0
> +#define JS_COUNT_SYMBOL(name) + 1
> +    JS_FOR_EACH_WELL_KNOWN_SYMBOL(JS_COUNT_SYMBOL)
> +#undef JS_COUNT_SYMBOL
> +    ;

Um, are you really using 16 lines of hideous macro gunk to generate an 8 line enum? No. Just no.

Write it out long form. If ES13 ends up adding 50 more of these, we'll presumably be able to use C++210x features to implement something much nicer.
Attachment #8436283 - Flags: review?(terrence)
Bug 645416, part 23 - Implement ValueToId for symbols. This makes symbols work as property keys.

Surprisingly small patch. Mostly tests. r?efaust because he probably knows what I'm screwing up here (will any ICs explode because of this, etc.)
Attachment #8437095 - Flags: review?(efaustbmo)
Bug 645416, part 24 - Modify proxy tests to add testing for symbol-keyed properties.

This patch also updates legacy direct proxies to cope with symbols. Not that
we care what legacy proxies do, but uniform behavior seems like the easiest
thing to carry forward.
Attachment #8437102 - Flags: review?(efaustbmo)
Bug 645416, part 25 - Add support for enumerating symbol-keyed properties.

Object.keys, Object.getOwnPropertyNames, and for-in loops skip symbol-keyed
properties per spec, but Object.defineProperties sees them, and a future
Reflect.ownKeys API will need to be able to see them.

This patch changes the comments on JSITER_FOREACH and JSITER_KEYVALUE, but not
the behavior. The comments were just wrong.
Attachment #8437110 - Flags: review?(jdemooij)
Bug 645416, part 26 - Update jsid sorting for JS_MORE_DETERMINISTIC.

Unfortunately, with symbols, the original goal of this code is apparently
impossible. For two unique symbols with the same description, short of doing
extra bookkeeping at run time, there's no ordering that's sure to be
consistent across runs. The new code orders all other symbols.
Attachment #8437115 - Flags: review?(luke)
Bug 645416, part 27 - Implement Object.getOwnPropertySymbols().
Attachment #8437119 - Flags: review?(jwalden+bmo)
Attachment #8437110 - Flags: review?(jdemooij) → review?(jwalden+bmo)
Bug 645416, part 28 - Update Object.prototype.toSource for symbol-keyed properties.

The new output uses syntax with square brackets: {[Symbol.for("key")]: "value"}
This syntax is not yet supported in SpiderMonkey, but it is part of ES6 (see
bug 924688 or search the ES6 drafts for ComputedPropertyName).
Attachment #8437136 - Flags: review?(jwalden+bmo)
Attached patch Part 29 - keyForSplinter Review
Bug 645416, part 29 - Implement Symbol.keyFor().
Attachment #8437137 - Flags: review?(efaustbmo)
Attachment #8432280 - Attachment is obsolete: true
Comment on attachment 8436268 [details] [diff] [review]
Part 6 - JIT support

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

Looks good, it's a large patch but most of the changes are very straight-forward. r=me with comments addressed.

::: js/src/jit/BaselineIC.cpp
@@ +6294,5 @@
>          primitiveType = JSVAL_TYPE_STRING;
>          proto = GlobalObject::getOrCreateStringPrototype(cx, global);
> +    } else if (val.isSymbol()) {
> +        primitiveType = JSVAL_TYPE_SYMBOL;
> +        proto = GlobalObject::getOrCreateSymbolPrototype(cx, global);

To test this code you'd need something like Symbol().foo(); -- I assume we have tests that do that?

::: js/src/jit/CodeGenerator.cpp
@@ +622,5 @@
>  
> +    if (mightBeSymbol) {
> +        // All symbols are truthy.
> +        MOZ_ASSERT(tagCount != 0);
> +        Label notSymbol;

Remove this unused label.

@@ +625,5 @@
> +        MOZ_ASSERT(tagCount != 0);
> +        Label notSymbol;
> +        if (tagCount != 1)
> +            masm.branchTestSymbol(Assembler::Equal, tag, ifTruthy);
> +        // else fall through to ifTruthy.

Nit: s/else/Else

@@ +957,5 @@
>  
> +    // Symbol
> +    if (lir->mir()->input()->mightBeType(MIRType_Symbol)) {
> +        masm.branchTestSymbol(Assembler::Equal, tag, ool->entry());
> +    }

Nit: no {}

@@ +7775,5 @@
>      masm.movePtr(ImmGCPtr(names.string), output);
> +    masm.jump(&done);
> +    masm.bind(&notString);
> +
> +    masm.movePtr(ImmGCPtr(names.symbol), output);

To make this fail immediately in debug builds when we add a new primitive type (instead of giving the wrong answer), please add the following before this line:

#ifdef DEBUG
Label isSymbol;
masm.branchTestSymbol(Assembler::Equal, tag, &isSymbol);
masm.assumeUnreachable("Unexpected type for TypeOfV");
masm.bind(&isSymbol);
#endif

::: js/src/jit/IonBuilder.cpp
@@ +7278,5 @@
>                                                         nullptr, types);
>  
>      // Always add a barrier if the index might be a string, so that the cache
>      // can attach stubs for particular properties.
> +    if (index->mightBeType(MIRType_String) || index->mightBeType(MIRType_Symbol))

Nit: update the comment.

::: js/src/jit/LIR-Common.h
@@ +8,5 @@
>  #define jit_LIR_Common_h
>  
>  #include "jit/shared/Assembler-shared.h"
>  
> +#include "vm/Symbol.h"

Remove this #include.

::: js/src/jit/Lowering.cpp
@@ +697,5 @@
>          return add(new(alloc()) LGoto(ifFalse));
>  
> +    // All symbols are truthy.
> +    if (opd->type() == MIRType_Symbol)
> +        return add(new(alloc()) LGoto(ifTrue));

Make sure we have tests for this. A loop with something like assertEq(Symbol() ? 1 : 0, 1); or an if-statement.

We should also test the boxed (MIRType_Value) case, your typeof.js test with a ternary instead of typeof should do.

@@ +1838,3 @@
>        case MIRType_Object:
>        case MIRType_Undefined:
> +        // Objects might be effectful. Undefined and symbols coerce to NaN, not int32.

We should have some tests for the unboxed-input-types cases, inside a loop (or function that's called multiple times):

assertEq(Symbol() + 2, ...);
assertEq(Symbol() * Symbol(), ...);
assertEq(Symbol() | Symbol(), ...);
...
assertEq(~Symbol(), ...);
assertEq(+Symbol(), ...);
...

And also test the Value case, with a function like this:

function f(a, b) { return a + b; }

Then call f(int32, int32), f(string, string), f(symbol, symbol), f(symbol, int32) etc.

@@ +2454,5 @@
>        case MIRType_Undefined:
>        case MIRType_Null:
>          return define(new(alloc()) LInteger(1), ins);
> +      case MIRType_Symbol:
> +        return define(new(alloc()) LInteger(0), ins);

Same here (JSOP_NOT).

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +3086,2 @@
>  {
>      ma_mov(operand.payloadReg(), dest);

Nit: to match the x86 version:

if (operand.payloadReg() != dest)
    ma_mov(...);

::: js/src/jit/x86/MacroAssembler-x86.h
@@ +829,3 @@
>      void unboxObject(const ValueOperand &src, Register dest) {
>          if (src.payloadReg() != dest)
>              movl(src.payloadReg(), dest);

Add this "if (src.payloadReg() != dest)" check to unboxNonDouble and call unboxNonDouble(src, dest); here as well. Nice refactoring btw.
Attachment #8436268 - Flags: review?(jdemooij) → review+
Comment on attachment 8437077 [details] [diff] [review]
Part 21 - SYMBOL_TO_JSID

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

::: js/src/jsfriendapi.h
@@ +2060,2 @@
>          return JS::Int32Value(JSID_TO_INT(id));
> +    if (MOZ_LIKELY(JSID_IS_SYMBOL(id)))

Let's just ditch the MOZ_LIKELY, unless we have evidence that it's doing anything useful. Guessing no, based on the existing code.

@@ -2060,2 @@
>          return JS::Int32Value(JSID_TO_INT(id));
> -    if (MOZ_LIKELY(JSID_IS_OBJECT(id)))

MOZ_LIKELY(neverTrue) -- classic SpiderMonkey.
Attachment #8437077 - Flags: review?(terrence) → review+
Attachment #8436254 - Flags: review?(bhackett1024) → review+
Attachment #8437090 - Flags: review?(terrence) → review+
Attachment #8436233 - Attachment is obsolete: true
Attachment #8437590 - Flags: review?(nicolas.b.pierron)
(In reply to Jim Blandy :jimb from comment #83)
> Are you sure stripping off the quotes is a good idea, though? It makes it
> look like an enum value.

That's pretty much what it is. There are three kinds of symbols, and they look like this:

    Symbol("unique user-generated symbol")
    Symbol.for("registry symbol, forgeable, shared by all realms")
    Symbol.iterator

The last is a well-known symbol, of which there are only 8. All their descriptions start with "Symbol.".

Part 18 makes uenval() render them just like this; it makes sense for the pretty-printers to do the same.
Comment on attachment 8437110 [details] [diff] [review]
Part 25 - Enumeration

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

::: js/src/tests/ecma_6/Symbol/enumeration-order.js
@@ +5,5 @@
> +// 
> +var log;
> +function LoggingProxy() {
> +    return new Proxy({}, {
> +        defineProperty: (t, key, desc) => { log.push(key); }

Add `return true;` for ES6 compatibility, so the test doesn't fail when proxies get updated?
(In reply to Terrence Cole [:terrence] from comment #85)
> I think you're going to want JS:: prefixes on these. We seem to need them or
> not, depending on what gets pulled into any particular unified build.

Ugh. This deal is getting worse all the time.

> Also please use either the templates or typedefs, not both.

Oh. I just didn't make any typedefs for Symbol. But based on these comments I'll add the works. You're right, it just looks ridiculous.

> Please add a static_assert(sizeof(Symbol) == 8 + (8 * JS_BITS_PER_WORD /
> 32)) or thereabouts.

Always happy to add a static assert, but they need to be actionable when they break. What does this assertion mean exactly?
(In reply to Terrence Cole [:terrence] from comment #89)
> ::: js/src/tests/ecma_6/Symbol/equality.js
> > +assertEq(Symbol.for("q") === Symbol.for("q"), true);
> 
> Why is assertEq(Symbol.for("q"), Symbol.for("q")) not appropriate here?

Yeah, good question. It's meant to be testing the === operator, not the implementation of assertEq (which uses JS_SameValue rather than === in the shell).
(In reply to Terrence Cole [:terrence] from comment #91)
> ::: js/src/jsapi.h
> @@ +4437,5 @@
> > +const size_t WellKnownSymbolLimit = 0
> > +#define JS_COUNT_SYMBOL(name) + 1
> > +    JS_FOR_EACH_WELL_KNOWN_SYMBOL(JS_COUNT_SYMBOL)
> > +#undef JS_COUNT_SYMBOL
> > +    ;
> 
> Um, are you really using 16 lines of hideous macro gunk to generate an 8
> line enum? No. Just no.
>
> Write it out long form. If ES13 ends up adding 50 more of these, we'll
> presumably be able to use C++210x features to implement something much nicer.

It seems like this amounts to saying the assertions in jsatom.cpp and SymbolObject.cpp are just not worth having, if a higher-order macro is the cost. Is that what you're saying? (It's OK to believe that. We might disagree though!)

If we do want to keep those assertions, the FOR_EACH_WELL_KNOWN_SYMBOL macro saves 6 lines of boilerplate code per well-known symbol (not counting the use quoted above). It pays for itself when there are 4 or more well-known symbols. We'll reach break-even this year.
Comment on attachment 8437115 [details] [diff] [review]
Part 26 - JS_MORE_DETERMINISTIC

Who needs integral ordering anyhow :)
Attachment #8437115 - Flags: review?(luke) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #104)
> (In reply to Terrence Cole [:terrence] from comment #85)
> 
> > Please add a static_assert(sizeof(Symbol) == 8 + (8 * JS_BITS_PER_WORD /
> > 32)) or thereabouts.
> 
> Always happy to add a static assert, but they need to be actionable when
> they break. What does this assertion mean exactly?

I'm sorry, I put that way too far down. I should have put that next to:

// The minimum allocation size is sizeof(JSString): 16 bytes on 32-bit
// architectures and 24 bytes on 64-bit. 8 bytes of padding makes Symbol
// the minimum size on both.
uint64_t unused2_;

If someone comes along and adds a member here without realizing this (or messing up the math when fixing the padding), then |new (p) Symbol(atom)| is going to silently corrupt the heap. The static_assert is to ensure that this does not happen. The size we're asserting is a function of JS_BITS_PER_WORD (16bytes for 32bit and 24bytes for 64bit) to avoid having to have separate assertions for 32 and 64 bit.

For some reason I was thinking that static_assert could not be in class scope, but I think it is actually fine to put it right under unused2_. Maybe even move the comment down to the static_assert message string?
(In reply to André Bargull from comment #103)
> Add `return true;` for ES6 compatibility, so the test doesn't fail when
> proxies get updated?

Yes. Thank you for your time, as always.
Comment on attachment 8437590 [details] [diff] [review]
Part 1 - JSValueType, v2

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

Sounds good. :)
Thanks for adding the constant for the Mask.
Attachment #8437590 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8437077 [details] [diff] [review]
Part 21 - SYMBOL_TO_JSID

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

The OldDebugAPI.cpp and GDB tests seem good; question on the Debugger.cpp patch.

::: js/src/vm/Debugger.cpp
@@ +5308,2 @@
>           } else {
> +             MOZ_ASSERT(JSID_IS_SYMBOL(id));

I'm confused; don't we need to store this in vals[i]?
Attachment #8437077 - Flags: review?(jimb) → review+
(In reply to Jim Blandy :jimb from comment #111)
> > +             MOZ_ASSERT(JSID_IS_SYMBOL(id));
> 
> I'm confused; don't we need to store this in vals[i]?

Oops -- this case can't happen at all, so I'm changing it to a MOZ_ASSERT_UNREACHABLE().

GetPropertyNames doesn't return symbols unless you specially ask for them by passing JSITER_SYMBOLS.
(In reply to Jason Orendorff [:jorendorff] from comment #106)
> (In reply to Terrence Cole [:terrence] from comment #91)
> > ::: js/src/jsapi.h
> > @@ +4437,5 @@
> > > +const size_t WellKnownSymbolLimit = 0
> > > +#define JS_COUNT_SYMBOL(name) + 1
> > > +    JS_FOR_EACH_WELL_KNOWN_SYMBOL(JS_COUNT_SYMBOL)
> > > +#undef JS_COUNT_SYMBOL
> > > +    ;
> > 
> > Um, are you really using 16 lines of hideous macro gunk to generate an 8
> > line enum? No. Just no.
> >
> > Write it out long form. If ES13 ends up adding 50 more of these, we'll
> > presumably be able to use C++210x features to implement something much nicer.
> 
> It seems like this amounts to saying the assertions in jsatom.cpp and
> SymbolObject.cpp are just not worth having, if a higher-order macro is the
> cost. Is that what you're saying? (It's OK to believe that. We might
> disagree though!)

Yeah, I guess that's pretty much what I'm saying :-). Those assertions appear to be comparing the literal Symbol.foo against the atom we just created from the literal Symbol.foo. Unless I'm misunderstanding, I think our machinery would have to be so totally broken for those asserts to fire that debugging the failure would be trivial.
Comment on attachment 8437119 [details] [diff] [review]
Part 27 - getOwnPropertySymbols

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

::: js/src/builtin/Object.cpp
@@ +884,2 @@
>  static bool
> +GetOwnPropertyKeys(JSContext *cx, CallArgs &args, unsigned flags, const char *fnname)

const CallArgs&

::: js/src/builtin/Object.h
@@ +25,5 @@
> + * Like IdToValue, but convert int jsids to strings. This is used when
> + * exposing a jsid to script for Object.getOwnProperty{Names,Symbols}
> + * or scriptable proxy traps.
> + */
> +bool

Might be the default, but I prefer seeing an extern here.

::: js/src/jsproxy.cpp
@@ +654,3 @@
>  Trap1(JSContext *cx, HandleObject handler, HandleValue fval, HandleId id, MutableHandleValue rval)
>  {
> +    if (!IdToStringOrSymbol(cx, id, rval)) // Re-use out-param to avoid Rooted overhead.

The comment seems not useful.

@@ +662,5 @@
>  Trap2(JSContext *cx, HandleObject handler, HandleValue fval, HandleId id, Value v_,
>        MutableHandleValue rval)
>  {
>      RootedValue v(cx, v_);
> +    if (!IdToStringOrSymbol(cx, id, rval)) // Re-use out-param to avoid Rooted overhead.

Slightly moreso, but not enough for me to keep it.
Attachment #8437119 - Flags: review?(jwalden+bmo) → review+
Attachment #8436285 - Flags: review?(sphink) → review+
Comment on attachment 8436285 [details] [diff] [review]
Part 12 - ToBoolean

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

::: js/src/tests/ecma_6/Symbol/conversions.js
@@ +14,5 @@
> +    assertEq(sym || 13, sym);
> +    assertEq(sym && 13, 13);
> +}
> +
> +reportCompare(0, 0);

I think this needs the 

if (typeof reportCompare == "function")

that you have in the others? For running under the browser, I guess.
Attachment #8436286 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] from comment #115)
> I think this needs the 
> 
> if (typeof reportCompare == "function")
> 
> that you have in the others? For running under the browser, I guess.

Then again, never mind. (Clarified on IRC: this is for people who want to run a test directly with including the test harness.)
Comment on attachment 8437136 [details] [diff] [review]
Part 28 - Object.prototype.toSource

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

::: js/src/builtin/Object.cpp
@@ +169,5 @@
>          }
>  
> +        /* Convert id to a string. */
> +        RootedString idstr(cx);
> +        if (JSID_IS_STRING(id) || JSID_IS_INT(id)) {

I'd flip to have the simpler-reading is-symbol check here, but whatever.
Attachment #8437136 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8437110 [details] [diff] [review]
Part 25 - Enumeration

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

::: js/src/jit-test/tests/debug/Object-getOwnPropertyNames-01.js
@@ +29,5 @@
> +test("(function () {\n" +
> +     "    var x = {};\n" +
> +     "    x[Symbol()] = 1; x[Symbol.for('moon')] = 2; x[Symbol.iterator] = 3;\n" +
> +     "    return x;\n" + 
> +     "})()");

Hm, this is kind of more a test of JSON stringification than anything else, seems to me.

::: js/src/jit-test/tests/debug/Object-getOwnPropertyNames-02.js
@@ -3,5 @@
>  var g = newGlobal();
>  var dbg = Debugger();
>  var gobj = dbg.addDebuggee(g);
>  g.p = {xyzzy: 8};  // makes a cross-compartment wrapper
> -assertEq(gobj.getOwnPropertyDescriptor("p").value.getOwnPropertyDescriptor("xyzzy").value, 8);

Interesting test for getOwnPropertyNames there!

::: js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames1.js
@@ +18,5 @@
>          configurable: true
>      }
> +});
> +obj[Symbol("moon")] = "something";
> +var names = Object.getOwnPropertyNames(Proxy(obj, {}));

This change looks right.  But this test is completely unreadable as written (Splinter happens to show the entire file *except* that most critical of lines, line 11, which also doesn't help) due to not having any named variables for the various objects in this proxy/prototype mashup.  Please land the patch I'm about to upload here, then make this patch's changes atop that.

::: js/src/jsiter.cpp
@@ +180,5 @@
> +            Shape &shape = r.front();
> +            jsid id = shape.propid();
> +            if (JSID_IS_SYMBOL(id)) {
> +                if (!Enumerate(cx, pobj, id, shape.enumerable(), flags, ht, props))
> +                    return false;

Could combine ifs into one.

I sort of wonder if having two vectors, one for names, one for symbols, might be nicer.  Symbols vector would stay empty most of the time, so no real extra work to reverse/unify them.  Meh.

::: js/src/jsproxy.cpp
@@ +1761,5 @@
>      }
>  
>      // steps g to n are shared
> +    return ArrayToIdVector(cx, proxy, target, trapResult, props,
> +                           JSITER_OWNONLY | JSITER_HIDDEN | JSITER_SYMBOLS,

I thought getOwnPropertyNames didn't include symbols...or is that fixed in some other patch in this series?

::: js/src/tests/ecma_6/Symbol/enumeration.js
@@ +21,5 @@
> +var p2 = new Proxy(obj2, {});
> +for (var x in p2)
> +    throw "FAIL: " + uneval(x);
> +
> +// Object.keys() and .getOwnPropertyNames() also skip symbols.

Don't you want these happening on obj2 and p2 as well?

::: js/src/tests/ecma_6/Symbol/for-in-order.js
@@ +11,5 @@
> +obj[Symbol.for("y")] = 2
> +obj.y = 3;
> +obj[Symbol.iterator] = function* () { yield 4; };
> +obj.z = 5;
> +

Might be worth tacking a symbolic property onto Object.prototype to test symbol absence along the whole prototype chain.
Attachment #8437110 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8436267 [details] [diff] [review]
Part 5 - Symbol constructor

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

Looks good to me. See comments.

::: js/src/builtin/SymbolObject.cpp
@@ +54,5 @@
> +
> +    // This uses &JSObject::class_ because: "The Symbol prototype object is an
> +    // ordinary object. It is not a Symbol instance and does not have a
> +    // [[SymbolData]] internal slot." (ES6 rev 24, 19.4.3)
> +    Rooted<JSObject*> proto(cx, global->createBlankPrototype(cx, &JSObject::class_));

Is there a reason we don't use RootedObject and RootedFunction here? Is that the builtin/ style?

::: js/src/jit-test/tests/debug/Environment-find-03.js
@@ +1,1 @@
> +// env.find() finds nonenumerable properties in with statements.

heh, nice catch

::: js/src/js.msg
@@ +235,5 @@
>  MSG_DEF(JSMSG_BAD_GENERATOR_SEND,     182, 1, JSEXN_TYPEERR, "attempt to send {0} to newborn generator")
>  MSG_DEF(JSMSG_SC_NOT_TRANSFERABLE,    183, 0, JSEXN_TYPEERR, "invalid transferable array for structured clone")
>  MSG_DEF(JSMSG_SC_DUP_TRANSFERABLE,    184, 0, JSEXN_TYPEERR, "duplicate transferable for structured clone")
>  MSG_DEF(JSMSG_CANT_REPORT_AS_NON_EXTENSIBLE, 185, 0, JSEXN_TYPEERR, "proxy can't report an extensible object as non-extensible")
> +MSG_DEF(JSMSG_NEW_SYMBOL,             186, 0, JSEXN_TYPEERR, "'new Symbol' is not a thing you can do")

Nit: This message seems a trifle informal, but my taste in this matter seems to disagree with other people's.

::: js/src/tests/ecma_6/Symbol/errors.js
@@ +10,5 @@
> +assertThrowsInstanceOf(() => Function.prototype.call.call(sym), TypeError);
> +
> +// 7.2.5 IsConstructor
> +assertThrowsInstanceOf(() => new sym(), TypeError);
> +

should we do |assertThrowsInstanceOf(() => new Symbol(), TypeError)| here as well?

::: js/src/vm/Interpreter.cpp
@@ +671,5 @@
> +    if (lval.isDouble()) {
> +        *equal = (lval.toDouble() == rval.toDouble());
> +        return true;
> +    }
> +    if (lval.isGCThing()) {  // objects or symbols

nit: there's also functions here, no? We are somewhat ambiguous about where we consider them "just special objects" and where they get their own love, and I struggle to know which is right when.
Attachment #8436267 - Flags: review?(efaustbmo) → review+
Comment on attachment 8436279 [details] [diff] [review]
Part 9 - Symbol.for

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

Works for me.

::: js/src/jsapi-tests/testSymbol.cpp
@@ +30,5 @@
> +BEGIN_TEST(testSymbol_GetSymbolFor)
> +{
> +    using namespace JS;
> +
> +    RootedString desc(cx, JS_NewStringCopyZ(cx, "ponies"));

that's a solid name. Much better than "test"

::: js/src/vm/Runtime.h
@@ +1187,5 @@
>      JSCompartment *atomsCompartment_;
>  
> +    // Set of all live symbols produced by Symbol.for(). All such symbols are
> +    // allocated in the atomsCompartment. This table is subject to the same
> +    // locking scheme as atoms_ and atomsCompartment_.

Is this really dependent on what they do, even if they change?

::: js/src/vm/Symbol.cpp
@@ +66,5 @@
> +    if (!sym)
> +        return nullptr;
> +
> +    // p is still valid here because we have held the lock since the
> +    // lookupForAdd call, and newInternal can't GC.

Can we add an RAII annotation or assertion to verify this notion? It's clearly true now, certainly.
Attachment #8436279 - Flags: review?(efaustbmo) → review+
Comment on attachment 8436292 [details] [diff] [review]
Part 16 - valueOf

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

Rest looks good to me.

::: js/src/tests/ecma_6/Symbol/valueOf.js
@@ +15,5 @@
> +
> +// Any other value throws.
> +var nonsymbols = [undefined, null, NaN, {}];
> +for (var nonsym of nonsymbols)
> +    assertThrowsInstanceOf(() => Symbol.prototype.toString.call(nonsym), TypeError);

prototype.valueof?
Comment on attachment 8436295 [details] [diff] [review]
Part 18 - ValueToSource

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

::: js/src/jsstr.cpp
@@ +4351,5 @@
> +        if (!desc)
> +            return nullptr;
> +    }
> +
> +    const char *prefix = (code == SymbolCode::InSymbolRegistry

I would use the StringBuffer class here.
Comment on attachment 8436283 [details] [diff] [review]
Part 10 - Well-known symbols.

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

I leave the specifics of the marking code to Terrence, but everything else looks good to me.

I think I would be OK with the enum as written. The lack of error-factor in adding new ones makes the slight clunk OK to me.
If you two hash it out and decide to change it to something more conventional, that's also fine with me.

::: js/src/builtin/SymbolObject.cpp
@@ +82,5 @@
> +
> +    // Define the well-known symbol properties, such as Symbol.iterator.
> +    RootedValue value(cx);
> +    unsigned attrs = JSPROP_READONLY | JSPROP_PERMANENT;
> +    WellKnownSymbols &wks = *cx->runtime()->wellKnownSymbols;

Is there a reason this doesn't just use the pointer directly?

::: js/src/gc/Barrier.h
@@ +591,2 @@
>      operator Handle<T>() const {
>          return Handle<T>::fromMarkedLocation(&value);

hyper-nit: should this use the recently added address()?

::: js/src/jsapi.h
@@ +4437,5 @@
> +const size_t WellKnownSymbolLimit = 0
> +#define JS_COUNT_SYMBOL(name) + 1
> +    JS_FOR_EACH_WELL_KNOWN_SYMBOL(JS_COUNT_SYMBOL)
> +#undef JS_COUNT_SYMBOL
> +    ;

I think I might disagree? It's a little clunky, but it's far less error prone.

::: js/src/tests/ecma_6/Symbol/well-known.js
@@ +8,5 @@
> +assertEq(Symbol.iterator !== Symbol.for("Symbol.iterator"), true);
> +
> +// They are shared across realms.
> +if (typeof Realm === 'function')
> +    throw new Error("please update this test to use Realms");

Thank you for adding this.

::: js/src/vm/Runtime.h
@@ +1259,5 @@
>      bool transformToPermanentAtoms();
>  
> +    // Cached well-known symbols (ES6 rev 24 6.1.5.1). Like permanent atoms,
> +    // these are shared with the parentRuntime, if any.
> +    js::WellKnownSymbols *wellKnownSymbols;

We hand back ImmutableSymbolPtrs from here, but this should be const as well.
Attachment #8436283 - Flags: review?(efaustbmo) → review+
Comment on attachment 8436298 [details] [diff] [review]
Part 20 - Symbol::dump()

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

wfm

::: js/src/vm/String.cpp
@@ +62,5 @@
>  
>  #ifdef DEBUG
>  
>  void
> +JSString::dumpChars(const jschar *s, size_t n, FILE *fp)

this is a change we wanted anyway, probably. Thanks for doing this.
Attachment #8436298 - Flags: review?(efaustbmo) → review+
Comment on attachment 8437077 [details] [diff] [review]
Part 21 - SYMBOL_TO_JSID

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

Looks pretty good. I'm hoping I missed something with the missing infallibleAppend? r=me with comments addressed.

::: js/public/Id.h
@@ +116,5 @@
>  {
>      jsid id;
> +    MOZ_ASSERT(sym != nullptr);
> +    MOZ_ASSERT((size_t(sym) & JSID_TYPE_MASK) == 0);
> +    JS_ASSERT(!js::gc::IsInsideNursery(JS::AsCell(sym)));

Also assert here that the symbol is not posioned. We want to make sure we aren't constructing JSIDs with UAFs.

::: js/src/builtin/Object.cpp
@@ +812,5 @@
>  obj_keys(JSContext *cx, unsigned argc, Value *vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    // step 1

should be 1-2, no? The "return false" is the ReturnIfAbrupt, right?

@@ +817,5 @@
>      RootedObject obj(cx);
>      if (!GetFirstArgumentAsObject(cx, args, "Object.keys", &obj))
>          return false;
>  
> +    // steps 3-10

perhaps we should be more specific with this? As written it's of little use in terms of being able to resconstruct the steps from the code or in terms of specific annotation.

@@ +822,2 @@
>      AutoIdVector props(cx);
>      if (!GetPropertyNames(cx, obj, JSITER_OWNONLY, &props))

Should we note here that without JSITER_HIDDEN, we do the enumerability check (19.1.2.14 step 10.c) in GetPropertyNames()?

@@ +822,5 @@
>      AutoIdVector props(cx);
>      if (!GetPropertyNames(cx, obj, JSITER_OWNONLY, &props))
>          return false;
>  
>      AutoValueVector vals(cx);

nit: do we want to make our names here match those in spec? I have no problem with |vals|, but it actually corresponds to |nameList|, right?

@@ +835,5 @@
>              if (!str)
>                  return false;
>              vals.infallibleAppend(StringValue(str));
>          } else {
> +            MOZ_ASSERT(JSID_IS_SYMBOL(id));

shouldn't we append the symbol here? am I missing something? Is this maybe coming in a later patch in the series?
Attachment #8437077 - Flags: review?(efaustbmo) → review+
Comment on attachment 8436283 [details] [diff] [review]
Part 10 - Well-known symbols.

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

I'm still not convinced that the macros are a good idea yet, but it's not worth holding up symbols over.

::: js/src/jsapi.h
@@ +4437,5 @@
> +const size_t WellKnownSymbolLimit = 0
> +#define JS_COUNT_SYMBOL(name) + 1
> +    JS_FOR_EACH_WELL_KNOWN_SYMBOL(JS_COUNT_SYMBOL)
> +#undef JS_COUNT_SYMBOL
> +    ;

Um, are you really using 16 lines of hideous macro gunk to generate an 8 line enum? No. Just no.

Write it out long form. If ES13 ends up adding 50 more of these, we'll presumably be able to use C++210x features to implement something much nicer.
Attachment #8436283 - Flags: review+
Comment on attachment 8437095 [details] [diff] [review]
Part 23 - ToPropertyKey

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

Looks good. Do we need any more spec location references or references to ToPropertyKey? I don't think so.

::: js/src/tests/ecma_6/Symbol/property-accessor.js
@@ +26,5 @@
> +// increment operator
> +gets = 0;
> +sets = [];
> +assertEq(obj[sym]++, 1);
> +assertDeepEq(sets, [2]);

This is clever. I like this a lot.
Attachment #8437095 - Flags: review?(efaustbmo) → review+
Comment on attachment 8437102 [details] [diff] [review]
Part 24 - Proxies

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

God I wish we could kill the old proxy API. Oh well. r=me with the one blatant test incorrectness corrected.

::: js/src/jit-test/tests/proxy/testDirectProxyDefineProperty2.js
@@ +29,5 @@
>  Object.defineProperty(p, 'foo', desc);
> +Object.defineProperty(p, Symbol.for('quux'), desc);
> +assertEq(log.length, 2);
> +assertEq(log[0], 'foo');
> +assertEq(log[1], Symbol.for('quux'));

Is this just a workaround for the fact that assertDeepEq isn't readily at hand here in jit-test land?

::: js/src/jit-test/tests/proxy/testDirectProxyHasOwn6.js
@@ +14,5 @@
>  });
>  assertEq(({}).hasOwnProperty.call(proxy, 'foo'), true);
>  assertEq(({}).hasOwnProperty.call(proxy, 'bar'), false);
> +assertEq(({}).hasOwnProperty.call(proxy, 'quux'), false);
> +assertEq(({}).hasOwnProperty.call(proxy, 'Symbol(quux)'), false);

quotes are wrong here. Should read |Symbol('quux')|, right?

::: js/src/jit-test/tests/proxy/testDirectProxySet2.js
@@ +3,5 @@
>   * argument, the name of the property as the second argument, the value as the
>   * third argument, and the receiver as the fourth argument
>   */
>  var target = {};
> +for (var key of ["foo", Symbol.for("quux")]) {

nit: is there a reason we went to double quotes here?

::: js/src/jit-test/tests/proxy/testDirectProxySet3.js
@@ +1,4 @@
>  load(libdir + "asserts.js");
>  
>  // Throw a TypeError if the trap sets a non-writable, non-configurable property
> +for (var key of ['foo', Symbol.for("quux")]) {

nit: these should at least match in quote, I would think.

::: js/src/jsproxy.cpp
@@ +664,3 @@
>  Trap1(JSContext *cx, HandleObject handler, HandleValue fval, HandleId id, MutableHandleValue rval)
>  {
> +    if (!IdToExposableValue(cx, id, rval)) // Re-use out-param to avoid Rooted overhead.

Nice! Glad we can clean these up a little bit. Thanks for doing this while you were here.
Attachment #8437102 - Flags: review?(efaustbmo) → review+
Comment on attachment 8437137 [details] [diff] [review]
Part 29 - keyFor

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

Good. r=me

::: js/src/builtin/SymbolObject.cpp
@@ +160,5 @@
>      args.rval().setSymbol(symbol);
>      return true;
>  }
>  
> +// ES6 rev 24 (2014 Apr 27) 19.4.2.2

This is 19.4.2.7 in the most recent draft (22 May, 2014). This step number certainly must be different than Symbol.for

@@ +177,5 @@
> +    if (arg.toSymbol()->code() == JS::SymbolCode::InSymbolRegistry) {
> +        args.rval().setString(arg.toSymbol()->description());
> +        return true;
> +    }
> +    

nit: whitespace on previous line.

@@ +178,5 @@
> +        args.rval().setString(arg.toSymbol()->description());
> +        return true;
> +    }
> +    
> +    // step 3: omitted

Do we have any way to assert that at least InSymbolRegistry and actually being in the registry are in sync? That's not the same thing as the assertion in step 3, but it's at least on the mind here.
Attachment #8437137 - Flags: review?(efaustbmo) → review+
(In reply to Eric Faust [:efaust] from comment #120)
> Is there a reason we don't use RootedObject and RootedFunction here? Is that
> the builtin/ style?

Hah, no, I have no idea what compelled me to write those out. Fixed.

> > +MSG_DEF(JSMSG_NEW_SYMBOL,             186, 0, JSEXN_TYPEERR, "'new Symbol' is not a thing you can do")
> 
> Nit: This message seems a trifle informal, but my taste in this matter seems
> to disagree with other people's.

Er, yes. I changed it to "Symbol is not a constructor", reusing JSMSG_NOT_CONSTRUCTOR.

The spec says Symbol *is* a constructor, technically, though if called as a constructor, it always throws a TypeError. Why? Who knows?

> > +assertThrowsInstanceOf(() => new sym(), TypeError);
> 
> should we do |assertThrowsInstanceOf(() => new Symbol(), TypeError)| here as
> well?

Added.

> > +    if (lval.isGCThing()) {  // objects or symbols
> 
> nit: there's also functions here, no? We are somewhat ambiguous about where
> we consider them "just special objects" and where they get their own love,
> and I struggle to know which is right when.

Hmm. From the perspective of code like this that's just looking at a Value, all functions are objects, in all circumstances. JSFunction is a subclass of JSObject. The spec is also clear on this point.

This particular line says "objects or symbols" because SameType only checks the JS::Value's type-tag, and that doesn't distinguish between functions and other objects; the type tag of a JS::Value that refers to a function is JSVAL_TAG_OBJECT.

There are tons of places where functions get their own special love and care, but that doesn't necessarily conflict with their being a special kind of object. What kind of place did you have in mind?
(In reply to Eric Faust [:efaust] from comment #121)
> ::: js/src/vm/Symbol.cpp
> > +    // p is still valid here because we have held the lock since the
> > +    // lookupForAdd call, and newInternal can't GC.
> 
> Can we add an RAII annotation or assertion to verify this notion? It's
> clearly true now, certainly.

Hmm. I dunno. Terrence, would it be OK for GCRuntime::collect to assert this:
    MOZ_ASSERT(!rt->currentThreadHasExclusiveAccess());
?

Better yet, does the static analysis catch holding the lock across a call that could GC?

ArenaLists::refillFreeList seems to assert that, or something like it. Maybe that's enough...?
Flags: needinfo?(terrence)
> > +    WellKnownSymbols &wks = *cx->runtime()->wellKnownSymbols;
> 
> Is there a reason this doesn't just use the pointer directly?

Nope. Changed.

> ::: js/src/gc/Barrier.h
> >      operator Handle<T>() const {
> >          return Handle<T>::fromMarkedLocation(&value);
> 
> hyper-nit: should this use the recently added address()?

Hmm. I don't think it matters; terrence can weigh in.

> > +    // Cached well-known symbols (ES6 rev 24 6.1.5.1). Like permanent atoms,
> > +    // these are shared with the parentRuntime, if any.
> > +    js::WellKnownSymbols *wellKnownSymbols;
> 
> We hand back ImmutableSymbolPtrs from here, but this should be const as well.

These are only immutable once initialized, and they're lazily initialized. Also JSRuntime::finishAtoms() js_deletes this pointer. So neither the pointer nor the WellKnownSymbols object can be const.
(In reply to Terrence Cole [:terrence] from comment #127)
> I'm still not convinced that the macros are a good idea yet, but it's not
> worth holding up symbols over.

I deleted the FOR_EACH_WELL_KNOWN_SYMBOL macro and the assertions that it was being used for.
(In reply to Tom Schuster [:evilpie] from comment #122)
> ::: js/src/tests/ecma_6/Symbol/valueOf.js
> > +    assertThrowsInstanceOf(() => Symbol.prototype.toString.call(nonsym), TypeError);
> 
> prototype.valueof?

Er, yes. Thank you.
(In reply to Tom Schuster [:evilpie] from comment #123)
> I would use the StringBuffer class here.

Yep. Much nicer. Thanks.
Attachment #8441861 - Flags: review?(sphink)
Attachment #8436295 - Attachment is obsolete: true
Attachment #8436295 - Flags: review?(sphink)
(In reply to Till Schneidereit [:till] from comment #82)
> Part 19 - assertDeepEq
> > +var sym = Symbol();
> > +assertNotDeepEq([sym, sym], [Symbol(), Symbol()]);
> > +assertNotDeepEq([sym, sym], [Symbol(), sym]);
> 
> For completeness' sake maybe add
> assertDeepEq([sym, sym], [sym, sym]);

Done.
(In reply to Eric Faust [:efaust] from comment #126)
> Also assert here that the symbol is not posioned. We want to make sure we
> aren't constructing JSIDs with UAFs.

Righteous suggestion. Done.

> > +    // step 1
> should be 1-2, no? The "return false" is the ReturnIfAbrupt, right?

Right.

> > +    // steps 3-10
> 
> perhaps we should be more specific with this? As written it's of little use
> in terms of being able to resconstruct the steps from the code or in terms
> of specific annotation.

I'll have to think about how best to comment this. The steps in the algorithm are kind of stupid, IMHO.

> Should we note here that without JSITER_HIDDEN, we do the enumerability
> check (19.1.2.14 step 10.c) in GetPropertyNames()?

Added.

> nit: do we want to make our names here match those in spec? I have no
> problem with |vals|, but it actually corresponds to |nameList|, right?

Good point. Changed.

> 
> @@ +835,5 @@
> >              if (!str)
> >                  return false;
> >              vals.infallibleAppend(StringValue(str));
> >          } else {
> > +            MOZ_ASSERT(JSID_IS_SYMBOL(id));
> 
> shouldn't we append the symbol here? am I missing something? Is this maybe
> coming in a later patch in the series?

Object.keys intentionally only returns string keys, not symbol keys. Note the type check in step 10.c. of Object.keys.

There is a method in ES6, Reflect.ownKeys, that returns both. We don't implement it yet.
(In reply to Eric Faust [:efaust] from comment #129)
> ::: js/src/jit-test/tests/proxy/testDirectProxyDefineProperty2.js
> > +assertEq(log.length, 2);
> > +assertEq(log[0], 'foo');
> > +assertEq(log[1], Symbol.for('quux'));
> 
> Is this just a workaround for the fact that assertDeepEq isn't readily at
> hand here in jit-test land?

Yes.

> ::: js/src/jit-test/tests/proxy/testDirectProxyHasOwn6.js
> > +assertEq(({}).hasOwnProperty.call(proxy, 'quux'), false);
> > +assertEq(({}).hasOwnProperty.call(proxy, 'Symbol(quux)'), false);
> 
> quotes are wrong here. Should read |Symbol('quux')|, right?

Oh. I think what I was thinking here was, "test that we're not calling .toString() on the key, like idiots" but there's no reason not to test both...

> > +for (var key of ["foo", Symbol.for("quux")]) {
> 
> nit: is there a reason we went to double quotes here?

No. Changed to single quotes.

> > +for (var key of ['foo', Symbol.for("quux")]) {
> 
> nit: these should at least match in quote, I would think.

Done.
Attachment #8439503 - Flags: feedback?(jorendorff) → feedback+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #118)
> ::: js/src/jit-test/tests/debug/Object-getOwnPropertyNames-01.js
> > +     "    x[Symbol()] = 1; x[Symbol.for('moon')] = 2; x[Symbol.iterator] = 3;\n" +
> > +     "    return x;\n" + 
> > +     "})()");
> 
> Hm, this is kind of more a test of JSON stringification than anything else,
> seems to me.

What's really being tested here is that Debugger.Object.prototype.getOwnPropertyNames also does not see symbol property keys (assuming Object.getOwnPropertyNames has test coverage elsewhere).

> ::: js/src/jit-test/tests/debug/Object-getOwnPropertyNames-02.js
> > -assertEq(gobj.getOwnPropertyDescriptor("p").value.getOwnPropertyDescriptor("xyzzy").value, 8);
> 
> Interesting test for getOwnPropertyNames there!

Yeah. :-\

> 
> ::: js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames1.js
> @@ +18,5 @@
> >          configurable: true
> >      }
> > +});
> > +obj[Symbol("moon")] = "something";
> > +var names = Object.getOwnPropertyNames(Proxy(obj, {}));
> 
> This change looks right.  But this test is completely unreadable as written

Yes. Thanks for the patch.

> ::: js/src/jsiter.cpp
> @@ +180,5 @@
> > +            Shape &shape = r.front();
> > +            jsid id = shape.propid();
> > +            if (JSID_IS_SYMBOL(id)) {
> > +                if (!Enumerate(cx, pobj, id, shape.enumerable(), flags, ht, props))
> > +                    return false;
> 
> Could combine ifs into one.

There are two different kinds of condition here -- (JSID_IS_SYMBOL(id)) is an honest question; could be true, could be false. The test is logically side-effect-free.  (!Enumerate(...)) is an error check; the function is mainly called for its side effects on ht and props.

I generally don't combine the two different kinds of condition in the same if-statement. I think the code is much easier to read if they're kept separate.

> I sort of wonder if having two vectors, one for names, one for symbols,
> might be nicer.  Symbols vector would stay empty most of the time, so no
> real extra work to reverse/unify them.  Meh.

It would be less embarrassing. I think this is a good idea. Might look into it tomorrow; or as a follow-up.

> ::: js/src/jsproxy.cpp
> @@ +1761,5 @@
> >      }
> >  
> >      // steps g to n are shared
> > +    return ArrayToIdVector(cx, proxy, target, trapResult, props,
> > +                           JSITER_OWNONLY | JSITER_HIDDEN | JSITER_SYMBOLS,
> 
> I thought getOwnPropertyNames didn't include symbols...or is that fixed in
> some other patch in this series?

*wince*  Yes, good question. I kept the name `getOwnPropertyNames` for the ProxyHandler method, even though it really corresponds to ES6 [[OwnPropertyKeys]]/"ownKeys". This is obviously not a good state to be in. Filed bug 1026918 to rename the ProxyHandler virtual method in the direction of ES6.

> > +// Object.keys() and .getOwnPropertyNames() also skip symbols.
> 
> Don't you want these happening on obj2 and p2 as well?

Added.

> ::: js/src/tests/ecma_6/Symbol/for-in-order.js
> @@ +11,5 @@
> > +obj[Symbol.for("y")] = 2
> > +obj.y = 3;
> > +obj[Symbol.iterator] = function* () { yield 4; };
> > +obj.z = 5;
> > +
> 
> Might be worth tacking a symbolic property onto Object.prototype to test
> symbol absence along the whole prototype chain.

Done.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #117)
> ::: js/src/builtin/Object.cpp
> > +        if (JSID_IS_STRING(id) || JSID_IS_INT(id)) {
> 
> I'd flip to have the simpler-reading is-symbol check here, but whatever.

Done.
(In reply to Eric Faust [:efaust] from comment #130)
> > +// ES6 rev 24 (2014 Apr 27) 19.4.2.2
> 
> This is 19.4.2.7 in the most recent draft (22 May, 2014). This step number
> certainly must be different than Symbol.for

Fixed.

> nit: whitespace on previous line.

Fixed.

> Do we have any way to assert that at least InSymbolRegistry and actually
> being in the registry are in sync? That's not the same thing as the
> assertion in step 3, but it's at least on the mind here.

It's easy to assert that if symbol->code() == InSymbolRegistry, then this symbol is in the registry (using Symbol::for_ to query it).  I've added that.

The inverse, that if symbol->code() != InSymbolRegistry, then this symbol is not in the registry, is not quite as easy to assert. Fortunately this is an unlikely mistake. The only place where symbols are added to the registry is in Symbol::for_, and it happens literally half a dozen lines after the code that created that symbol, specifying SymbolCode::InSymbolRegistry.
Pretty good try run for parts 1-4: https://tbpl.mozilla.org/?tree=Try&rev=20c917871d89
The orange is due to an issue with bug 1000182, unrelated.

Two issues apparent from this full try run: https://tbpl.mozilla.org/?tree=Try&rev=20c917871d89

*   assertDeepEq.js continues to achieve in the field of triggering assertions

*   The antique compiler we use to build for ICS is confused by part 9
(In reply to Jason Orendorff [:jorendorff] from comment #132)
> (In reply to Eric Faust [:efaust] from comment #121)
> > ::: js/src/vm/Symbol.cpp
> > > +    // p is still valid here because we have held the lock since the
> > > +    // lookupForAdd call, and newInternal can't GC.
> > 
> > Can we add an RAII annotation or assertion to verify this notion? It's
> > clearly true now, certainly.
> 
> Hmm. I dunno. Terrence, would it be OK for GCRuntime::collect to assert this:
>     MOZ_ASSERT(!rt->currentThreadHasExclusiveAccess());
> ?
> 
> Better yet, does the static analysis catch holding the lock across a call
> that could GC?
> 
> ArenaLists::refillFreeList seems to assert that, or something like it. Maybe
> that's enough...?

If we can assert it in refillFreeList, then we should be able to assert it in ::collect as well. Let's add it.
Flags: needinfo?(terrence)
Reimplemented using StringBuffer.
Attachment #8436287 - Attachment is obsolete: true
Attachment #8436287 - Flags: review?(sphink)
Attachment #8442464 - Flags: review?(sphink)
Assertion added in GCRuntime::collect, all green: https://tbpl.mozilla.org/?tree=Try&rev=26458eff53b6

With tweaks for GCC 4.old, looks good on ICS too: https://tbpl.mozilla.org/?tree=Try&rev=85a6108a115a
Comment on attachment 8436289 [details] [diff] [review]
Part 15 - ToObject

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

::: js/src/tests/ecma_6/Symbol/conversions.js
@@ +32,5 @@
> +    assertEq(Object.getPrototypeOf(obj), Symbol.prototype);
> +    assertEq(Object.getOwnPropertyNames(obj).length, 0);
> +    assertEq(Object(sym) === Object(sym), false);  // new object each time
> +    var f = function () { return this; };
> +    assertEq(f.call(sym) === f.call(sym), false);  // new object each time

Ok, I'm just sort of reading through the test and matching things up to the spec as best I can, but it seems like this last one is for non-strict mode only. In strict mode, shouldn't f.call(sym) === f.call(sym) because no ToObject is done? http://people.mozilla.org/~jorendorff/es6-draft.html#sec-function.prototype.call says "The thisArg value is passed without modification as the this value." If I'm not just completely off base, then it seems like you'd want another test for that behavior.

Oh. I bet you have it somewhere. It doesn't belong here in a ToObject test, does it? I'll leave this comment here because it'll be hard for me to track down the test in all these patches.
Attachment #8436289 - Flags: review?(sphink) → review+
Comment on attachment 8436292 [details] [diff] [review]
Part 16 - valueOf

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

::: js/src/tests/ecma_6/Symbol/valueOf.js
@@ +13,5 @@
> +    assertEq(Object(sym).valueOf(), sym);
> +}
> +
> +// Any other value throws.
> +var nonsymbols = [undefined, null, NaN, {}];

Symbol.prototype, for kicks?

@@ +15,5 @@
> +
> +// Any other value throws.
> +var nonsymbols = [undefined, null, NaN, {}];
> +for (var nonsym of nonsymbols)
> +    assertThrowsInstanceOf(() => Symbol.prototype.toString.call(nonsym), TypeError);

What Tom said.
Attachment #8436292 - Flags: review?(sphink) → review+
Comment on attachment 8436294 [details] [diff] [review]
Part 17 - ToPrimitive

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

::: CLOBBER
@@ +26,1 @@
>  to bug 1019955.

Now fixed.
Attachment #8436294 - Flags: review?(sphink) → review+
Comment on attachment 8442464 [details] [diff] [review]
Part 14 - ToString, v2

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

::: js/src/gc/Barrier.h
@@ +602,5 @@
>  
>      operator Handle<T>() const {
>          return Handle<T>::fromMarkedLocation(&value);
>      }
> +    Handle<T> getHandle() const { return *this; }

I don't see where this is used in this patch?
Attachment #8442464 - Flags: review?(sphink) → review+
Attachment #8441861 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] from comment #149)
> Part 15 - ToObject
> ::: js/src/tests/ecma_6/Symbol/conversions.js
> > +    var f = function () { return this; };
> > +    assertEq(f.call(sym) === f.call(sym), false);  // new object each time
> 
> Ok, I'm just sort of reading through the test and matching things up to the
> spec as best I can, but it seems like this last one is for non-strict mode
> only. In strict mode, shouldn't f.call(sym) === f.call(sym) because no
> ToObject is done?

Right.

> Oh. I bet you have it somewhere. It doesn't belong here in a ToObject test,
> does it?

That's what I figured. Same patch, different file: as-base-value.js.
(In reply to Steve Fink [:sfink] from comment #150)
> Symbol.prototype, for kicks?

Added.

> > +    assertThrowsInstanceOf(() => Symbol.prototype.toString.call(nonsym), TypeError);
> 
> What Tom said.

Fixed.
(In reply to Steve Fink [:sfink] from comment #151)
> ::: CLOBBER

All these have been removed from my stack locally.
(In reply to Steve Fink [:sfink] from comment #152)
> ::: js/src/gc/Barrier.h
> > +    Handle<T> getHandle() const { return *this; }
> 
> I don't see where this is used in this patch?

Ultimately unused, I guess. Deleted.
(In reply to Jan de Mooij [:jandem] from comment #99)
> ::: js/src/jit/Lowering.cpp
> Make sure we have tests for this. [...]

Thank you for the testing advice. This was a very helpful review.

I added jit-test/tests/symbol/{toNumber,truthiness,not}.js.

The int32 tests flushed out a bug: I had missed a switch statement in LIRGenerator::visitTruncateToInt32().
Would have pushed this last night, but there was a lightning strike near my house and our internet access is out. :-P

Full Try Server run, with sprinkles: https://tbpl.mozilla.org/?tree=Try&rev=a634204ae840
Looks pretty good, but I put some #includes in a dumb order and check_spidermonkey_style.py called me on it.

Fresh full Try Server run, with sprinkles: https://tbpl.mozilla.org/?tree=Try&rev=523ba92a033c

Looking forward to the fuzzbugs from this...
https://hg.mozilla.org/mozilla-central/rev/c319984f3156
https://hg.mozilla.org/mozilla-central/rev/192a1527e6f1
https://hg.mozilla.org/mozilla-central/rev/537d97cbf684
https://hg.mozilla.org/mozilla-central/rev/df1552da0b8f
https://hg.mozilla.org/mozilla-central/rev/5d71e73ce8d4
https://hg.mozilla.org/mozilla-central/rev/e08a6942e21c
https://hg.mozilla.org/mozilla-central/rev/cdf258b25a12
https://hg.mozilla.org/mozilla-central/rev/0513197d722e
https://hg.mozilla.org/mozilla-central/rev/82afa573b285
https://hg.mozilla.org/mozilla-central/rev/edcb8617d09f
https://hg.mozilla.org/mozilla-central/rev/7171929b0090
https://hg.mozilla.org/mozilla-central/rev/3140892419ab
https://hg.mozilla.org/mozilla-central/rev/fa33d2ea3793
https://hg.mozilla.org/mozilla-central/rev/db9d35e7c4fd
https://hg.mozilla.org/mozilla-central/rev/1870f6bc071b
https://hg.mozilla.org/mozilla-central/rev/7b12b5a75bb4
https://hg.mozilla.org/mozilla-central/rev/7513cafd84cd
https://hg.mozilla.org/mozilla-central/rev/81bd2529fa18
https://hg.mozilla.org/mozilla-central/rev/fdcaf5436d38
https://hg.mozilla.org/mozilla-central/rev/828bbf429995
https://hg.mozilla.org/mozilla-central/rev/3e98ff68c3dd
https://hg.mozilla.org/mozilla-central/rev/22f03ec25fcc
https://hg.mozilla.org/mozilla-central/rev/c763de6a2fde
https://hg.mozilla.org/mozilla-central/rev/dfefe211d083
https://hg.mozilla.org/mozilla-central/rev/adc814d90d4d
https://hg.mozilla.org/mozilla-central/rev/cd2894ed2c76
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Flags: in-testsuite?
Covered by lots and lots of automated tests
Flags: in-testsuite? → in-testsuite+
Why does "tests/ecma_6/Symbol/as-base-value.js" test the broken strict mode boxing in SpiderMonkey? (Bug 603201, Bug 869098)
>Why does "tests/ecma_6/Symbol/as-base-value.js" test the broken strict mode boxing in SpiderMonkey? >(Bug 603201, Bug 869098)
Maybe by accident, maybe so that we remember to test the new behavior when we change strict mode boxing.

We probably want a relnote for that.
Keywords: relnote
Oh, maybe you just have to set firefox-relnote? I am not sure how that works.
relnote-firefox: --- → ?
Keywords: relnote
Comment on attachment 8436234 [details] [diff] [review]
Part 2 - Value

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

::: js/public/Value.h
@@ +260,5 @@
>              int32_t        i32;
>              uint32_t       u32;
>              uint32_t       boo;     // Don't use |bool| -- it must be four bytes.
>              JSString       *str;
> +            JS::Symbol     *sym;

Don't you also need to add a corresponding 'sym' field to the alternative (conditionally-compiled) versions of jsval_layout below?
(In reply to André Bargull from comment #163)
> Why does "tests/ecma_6/Symbol/as-base-value.js" test the broken strict mode
> boxing in SpiderMonkey? (Bug 603201, Bug 869098)

By accident. Yuck. I had forgotten about this bug.
(In reply to Jonathan Kew (:jfkthame) from comment #166)
> Don't you also need to add a corresponding 'sym' field to the alternative
> (conditionally-compiled) versions of jsval_layout below?

Only one of them. Filed bug 1030400 for this.
Blocks: 838540
Documentation:    

Symbol as a new data type in JavaScript
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures
* https://developer.mozilla.org/en-US/docs/Glossary/Primitive
* https://developer.mozilla.org/en-US/docs/Glossary/Symbol

Symbol reference docs
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/for
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/keyFor
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/prototype
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/toString
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/toSource
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/valueOf

Well-known symbols
* Only mentioned on the main Symbol reference page. Need more detailed docs/pages when they get implemented.

Symbols as keys in Maps and Sets but not in WeakMap
* Map, Set, WeakMap pages mention allowed types accordingly.

Symbols and typeof
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/typeof

Symbols in JSON.stringify are omitted
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#Description

Object.getOwnPropertySymbols
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/getOwnPropertySymbols

Firefox 33 for developers release notes:
* https://developer.mozilla.org/en-US/Firefox/Releases/33#JavaScript


Any review to the documentation would be appreciated. More examples are also always welcome in the MDN wiki!
Also, I wasn't able to view Symbols in the devtools which felt a bit buggy, but I guess that's bug 1024054 and I hope it will be fixed in Fx 33, too :-)
Added to the 33 release notes with the wording:
"Support for the ECMAScript 6 Symbol data type added (learn more)"

Learn more pointing to: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol
Is anyone aware of an existing bug for this: 

(typed into Firefox 33 console)
typeof Symbol()
"symbol"
var s= Symbol()
undefined
s
typeof s
typeof s
typeof s

After the `s`, the console stops responding to Symbols

Here's a screen shot: 

http://i.gyazo.com/f2e06e9668234f612876f4963d09c630.png
(In reply to Rick Waldron [:rwaldron] from comment #171)
> Is anyone aware of an existing bug for this: 
> 
> (typed into Firefox 33 console)
> typeof Symbol()
> "symbol"
> var s= Symbol()
> undefined
> s
> typeof s
> typeof s
> typeof s
> 
> After the `s`, the console stops responding to Symbols
> 
> Here's a screen shot: 
> 
> http://i.gyazo.com/f2e06e9668234f612876f4963d09c630.png

Bug 1039686 for the debugger server / console to not completely break when it discovers an unknown type.

Bug 1024054 and bug 881480 for supporting Symbols propertly in the devtools.
Depends on: 1039686
(In reply to Jason Orendorff [:jorendorff] from comment #77)
> Created attachment 8436294 [details] [diff] [review]
> Part 17 - ToPrimitive
> 
> Bug 645416, part 17 - Implement ToPrimitive on Symbol wrapper objects.
> 
> The spec defines this by way of a @@toPrimitive method. We fake it using a
> JSClass::convert hook. (Once @@toPrimitive is implemented, convert hooks
> can be removed entirely, but we need symbols first.)

In rev 26, Symbol.prototype[@@toPrimitive] has changed to return the wrappered symbol value, not throw a TypeError exception any more.

See http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts#july_18_2014_draft_rev_26

"Updated Symbol conversions: aSym == “not a symbol” produces false. var s=Symbol(); s==Object(s) produces true. foo”+aSymbol or aSymbol+”foo” throws TypeError. Symbol @@toPrimitive returns the wrappered symbol value. ToNumber(aSymbol) throws."
Depends on: 1037890
Depends on: 1037718
Blocks: 1041631
Depends on: 1050847
Depends on: 1055033
Depends on: 1055034
No longer depends on: 1055034
Depends on: 1055015
Moving the item into the 34 aurora release notes
Release note for Symbols still exists in Mobile section of 33 Aurora.
(In reply to merihakar from comment #175)
> Release note for Symbols still exists in Mobile section of 33 Aurora.
Yes, that is normal. Symbols have been/are available during the most of the 33 aurora cycle.
This item won't be part of the 33 beta release notes (but will be part of 34 aurora).
(In reply to Sylvestre Ledru [:sylvestre] from comment #176)
> (In reply to merihakar from comment #175)
> > Release note for Symbols still exists in Mobile section of 33 Aurora.
> Yes, that is normal. Symbols have been/are available during the most of the
> 33 aurora cycle.
> This item won't be part of the 33 beta release notes (but will be part of 34
> aurora).

Oh, I thought it had been forgotten, since it is not listed in 33 Aurora desktop relnote. Thanks for explanation.
Depends on: 1061665
Comment on attachment 8494746 [details] [diff] [review]
No bug yet - Remove orphaned declaration of JS_WrapAutoIdVector, whose implementation was removed in rev 828bbf429995 ()

hg bzexport went haywire, please disregard this patch
Attachment #8494746 - Attachment is obsolete: true
Attachment #8494746 - Flags: review?(terrence)
You need to log in before you can comment on or make changes to this bug.