Symbol behavior changes in ES6 draft rev 26

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla34
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=JS])

Attachments

(3 attachments)

* 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.
Assignee: nobody → 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 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 ;)
Attachment #8465669 - Flags: review?(hv1989) → review+
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
https://hg.mozilla.org/mozilla-central/rev/8e4e04daf2a6
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1054512
Depends on: 1054753
You need to log in before you can comment on or make changes to this bug.