1042602 - Symbol behavior changes in ES6 draft rev 26

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

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

Comment 1

[ziyunfei]

Attachment: symbol_toNumber_toPrimitive.diff

Comment 2

[ziyunfei]

(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

Comment 3

[ziyunfei]

Attachment: symbol_toNumber_toPrimitive_v2.diff

Comment 4

[Jeff Walden [:Waldo]]

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

Comment 5

[Jason Orendorff [:jorendorff]]

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

Comment 6

[ziyunfei]

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

Comment 7

[Jason Orendorff [:jorendorff]]

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.

Comment 8

[Jason Orendorff [:jorendorff]]

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 9

[Jason Orendorff [:jorendorff]]

Attachment: bug-1042602-symbols-rev-26-v1.patch

Comment 10

[Hannes Verschore [:h4writer]]

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

Comment 11

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4e04daf2a6

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e4e04daf2a6

Comment 13

[Florian Scholz (Open Web Docs)]

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!