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.

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

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

Any chances we could get this in any (non-standardized) form sometime soon ? Private names would be super helpful for our needs in Jetpack.

Private names have landed to webkit https://bugs.webkit.org/show_bug.cgi?id=86509

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.

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

Symbols have landed in V8 http://code.google.com/p/v8/issues/detail?id=2158

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.

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.

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

> 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.

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

(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 ;-)

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.

I'm posting some work in progress for terrence to look at...

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.

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.

(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.

Also, don't forget to update: http://dxr.mozilla.org/mozilla-central/source/xpcom/base/CycleCollectedJSRuntime.cpp#568

Snapshot of my stack.

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?

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.

(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.

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.

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

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.

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.

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

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.

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.

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.

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?

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.

Comment on attachment 8436279 [details] [diff] [review] Part 9 - Symbol.for r?terrence for the GC bits, r?efaust for everything else

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.

Bug 645416, part 13 - Update ToNumber for symbols. Not terribly interesting; converting a symbol to a number always produces NaN.

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.

Bug 645416, part 16 - Implement Symbol.prototype.valueOf.

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.)

Bug 645416, part 20 - Add JS::Symbol::dump() method for debugging.

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 :)

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.

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.

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.

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!

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.

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

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.

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.

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.

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.)

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.

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.

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.

Bug 645416, part 27 - Implement Object.getOwnPropertySymbols().

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).

Bug 645416, part 29 - Implement Symbol.keyFor().

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.

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.

(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 :)

(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.

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]?

(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.

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.

(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.

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.

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.

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.

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.

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.

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?

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.

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.

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.

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.

(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...?

> > + 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.

(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.

(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.

Reimplemented using StringBuffer.

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.

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.

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.

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?

(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...

Part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/c319984f3156 ... Part 29: https://hg.mozilla.org/integration/mozilla-inbound/rev/cd2894ed2c76

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

Covered by lots and lots of automated tests

Why does "tests/ecma_6/Symbol/as-base-value.js" test the broken strict mode boxing in SpiderMonkey? (Bug 603201, Bug 869098)