1286997 - Compound assignment calls ToPropertyKey before RequireObjectCoercible

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect, P3)

Description

[bakkot]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.106 Safari/537.36

Steps to reproduce:

null[{ toString:()=>{throw 'not reached';} }] += 0


Actual results:

uncaught exception: not reached


Expected results:

TypeError.

12.3.2.1 of the spec specifies that the receiver is ensured to be object-coercible before the name is converted to a string.

I suspect this was introduced in https://bugzilla.mozilla.org/show_bug.cgi?id=1260577 . I'd cc :efaust if I were more familiar with Bugzilla.

Comment 1

[Tooru Fujisawa [:arai]]

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=928fe146f1c10b2d439f2b088e242229b9b05350&tochange=cd2868983231ad1dcf5c4c9e12d0f03b12cbf950

Comment 2

[Yulia Startsev [:yulia]]

Attachment: Bug 1286997 - call RequireObjectCoercible before ToPropertyKey for inc/dec; r=jandem

Comment 3

[André Bargull [:anba]]

There's an open spec issue about this type of bugs: https://github.com/tc39/ecma262/issues/467

Basically we never invested time to implement the exact spec algorithms, because it leads to regressions (bloats byte code; worse error messages). And the spec was never changed to follow the implementations, because it requires reworking the Reference type (and some folks still prefer strict left-to-right evaluation).

Comment 4

[Yulia Startsev [:yulia]]

Those two reasons: bloats byte code and worse error messages, sounds like a good reason to change the spec. I understand that this might take some spec work, but since this is part of the stream this might be interesting for people to see. Is it worth the time investment to change the spec?

Comment 5

[André Bargull [:anba]]

It'd be easier to convince TC39 to change the spec if implementations all had the exact same semantics, but they're also all slightly different. :-/

---

Just to expand on "bloats byte code and worse error messages", for dis(function(o, p, v){ o[p] = v }), we're currently emitting

GetArg 0                        # o
GetArg 1                        # o p
GetArg 2                        # o p v
SetElem                         # o[p]

which has the minimum number of operations.

Per spec and with our current set of byte code operations, we'd need to emit:

GetArg 0                        # o
GetArg 1                        # o p
Swap                            # p o
CheckObjCoercible               # p o
Swap                            # o p
ToPropertyKey                   # o p
GetArg 2                        # o p v
SetElem                         # o[p]

which doubles the amount of byte code operations, so that's not really acceptable. Furthermore CheckObjCoercible will report TypeError: null has no properties when o == null, whereas we're currently reporting TypeError: can't access property 0, o is null. The less informative error message is already today visible:

js> null[0]++
typein:1:1 TypeError: null has no properties
Stack:
  @typein:1:1
js> null[0]+=1
typein:2:1 TypeError: can't access property 0 of null
Stack:
  @typein:2:1

To reduce the byte code bloat and also still report a proper error message, we'd need to add a new operation which combines CheckObjCoercible and ToPropertyKey:

GetArg 0                        # o
GetArg 1                        # o p
PrepareSetElem                  # o p
GetArg 2                        # o p v
SetElem                         # o[p]

But that's still a 25% increase in the number of byte code operations.

Comment 6

[Yulia Startsev [:yulia]]

Attachment: Bug 1286997 - Implement PrepareSetElem Opcode in interpreter, disable blinterp, ion, warp

Depends on D78905

Comment 7

[Yulia Startsev [:yulia]]

Attachment: Bug 1286997 - Enable baseline for PrepareSetElem, with VM implementation

Depends on D86231

Comment 8

[Yulia Startsev [:yulia]]

Attachment: Bug 1286997 - Enable PrepareSetElem in Ion with VM implementation

I have the worst ideas for optimization

Depends on D86232

Comment 9

[Yulia Startsev [:yulia]]

Attachment: Bug 1286997 - Implement CacheIR PrepareSetElem instruction and IC for Baseline; r=jandem

Depends on D86233

Comment 10

[Yulia Startsev [:yulia]]

Attachment: Bug 1286997 - Implement PrepareSetElemIC for Ion; r=jandem

Depends on D86234

Comment 11

[Yulia Startsev [:yulia]]

Attachment: Bug 1286997 - Implement PrepareSetElem in Warp; r=jandem

Depends on D86235

Comment 12

[Yulia Startsev [:yulia]]

Attachment: Bug 1286997 - inline PrepareSetElemOperation and remove ThrowObjectCoerciblePropertyKey

Depends on D86236

Comment 13

[Yulia Startsev [:yulia]]

Attachment: Bug 1286997 - Introduce CacheIR guard for IsNotNullOrUnefined; r=jandem

Depends on D86234

Comment 14

[Yulia Startsev [:yulia]]

Attachment: Bug 1286997 - Introduce ion/warp guards for IsNotNullOrUnefined; r=jandem

Depends on D90695

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:ystartsev, maybe it's time to close this bug?

Comment 16

[Yulia Startsev [:yulia]]

This has been addressed within the spec iirc (https://github.com/tc39/ecma262/pull/2267), so we can close this.