* symbol == Object(symbol) now returns true * ToNumber(symbol) now throws

(In reply to Jason Orendorff [:jorendorff] from comment #0) > * symbol == Object(symbol) now returns true > * ToNumber(symbol) now throws Hi, Jason. I'm a newbie to SpiderMonkey, Could you tell me where to fix this on my last patch: js> Symbol.iterator == Object(Symbol.iterator) typein:1:0 TypeError: can't convert symbol to number // Shoud be true js> Symbol.iterator == 1 typein:2:0 TypeError: can't convert symbol to number // Shoud be false

(In reply to 446240525 from comment #2) > Could you tell me where to fix this on my last patch: > > js> Symbol.iterator == Object(Symbol.iterator) > typein:1:0 TypeError: can't convert symbol to number // Shoud be true This isn't what I read should happen in rev26. Looks like in 7.2.10 you'd hit step 8 and return false, no? > js> Symbol.iterator == 1 > typein:2:0 TypeError: can't convert symbol to number // Shoud be false This looks fine, tho. As for where: it's a slightly tricky question. JS_LooselyEqual in jsapi.cpp will point you toward some of the code that'll need fixing for this. Also check out JSOP_EQ in vm/Interpreter.cpp, but I think that'll point you at the same code. Past the simple, as-a-method definition, you also need to change JIT behavior. Otherwise simple expressions typed into the shell will work correctly, but put those expressions in a loop that occurs 1e4 times and at some point the comparison will start evaluating to the wrong value. Searching for JSOP_EQ in jit/ is a good way to find places that'll need fixing, although you may have to dig a bit from there to find exact instances. Particularly, you're likely to find that these equality operations are defined as inline caches. That means you'll probably need to touch BaselineIC.cpp (for the fast-but-slower-code JIT) and IonIC.cpp (for the slow-but-faster-code JIT) for this stuff. That's probably enough to get you started, without necessarily going into so much detail that you're just following exact instructions that mean you don't learn as much. :-) If you need more info, happy to provide it, tho.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4) > (In reply to 446240525 from comment #2) > > Could you tell me where to fix this on my last patch: > > > > js> Symbol.iterator == Object(Symbol.iterator) > > typein:1:0 TypeError: can't convert symbol to number // Shoud be true > > This isn't what I read should happen in rev26. Looks like in 7.2.10 you'd > hit step 8 and return false, no? Yes, you're right. But the notes at <http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts#july_18_2014_draft_rev_26> say otherwise. Filed <https://bugs.ecmascript.org/show_bug.cgi?id=3066> to try to get to the bottom of this. Either I'm missing something obvious, or the spec is buggy...

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4) > That's probably enough to get you started, without necessarily going into so > much detail that you're just following exact instructions that mean you > don't learn as much. :-) If you need more info, happy to provide it, tho. Thanks for your review and tips, Jeff! but I'm afraid that's already far beyond my knowledge level, If someone else wants to carry on, that'd be great.

Allen confirms in <https://bugs.ecmascript.org/show_bug.cgi?id=3066#c1>: > Right the update to 72.10 to make s==Object(s) produce true was wrong. > > The new step 8 is unnecessary and steps 10&11 should include Symbol > along with String and Number as triggering a ToPrimitive call. I'll take this and carry 446240525's work forward.

Allen responded <https://bugs.ecmascript.org/show_bug.cgi?id=3066#c1>: > Right the update to 72.10 to make s==Object(s) produce true was wrong. > > The new step 8 is unnecessary and steps 10&11 should include Symbol along with String and > Number as triggering a ToPrimitive call. > > Fixed in rev27 editor's draft I'll implement it that way.

Comment on attachment 8465669 [details] [diff] [review] bug-1042602-symbols-rev-26-v1.patch Review of attachment 8465669 [details] [diff] [review]: ----------------------------------------------------------------- Nice, I think that should do it. ::: js/src/jit/Lowering.cpp @@ -1745,1 @@ > JS_ASSERT(conversion != MToDouble::NumbersOnly); Add comment in the default case of this switch: // Symbols will throw. @@ -1795,1 @@ > JS_ASSERT(conversion != MToFloat32::NumbersOnly); Add comment in the default case of this switch: // Symbols will throw. ::: js/src/jit/TypePolicy.cpp @@ +199,2 @@ > if (in->type() == MIRType_Object || in->type() == MIRType_String || > + in->type() == MIRType_Undefined || in->type() == MIRType_Symbol) I think this change is not required. Did you have fallout because of this? I assume not. And this will disable some optimizations of MCompare with MIRType_Symbol: http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.cpp?from=tryfold&case=true#2502 Related to this, can you revert the comment in http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonMacroAssembler.cpp#1777 Reading over that code, the comment is not correct. We will go to the fail branch for MIRType_Symbol in all cases. (i.e. bail). Plz keep the s/BinaryArithPolicy/ArithPolicy ;)

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol#Symbol_type_conversions https://developer.mozilla.org/en-US/Firefox/Releases/34#JavaScript Any review is very much appreciated!