1199546 - Wrong order of operations with destructuring and computed property name

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[u443057]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20150827030213

Steps to reproduce:

Eval this code.

```js
var s = 'foo';
var p = {
  toString: function() {
    return s;
  }
};
var o = {
  [p]: (function() {
    s = 'bar';
    return 'abc';
  })(),
  [p]: 'efg'
};
```


Actual results:

`o` is `{ bar: "efg" }`


Expected results:

`o` is `{ foo: "abc", bar: "efg" }`

http://ecma-international.org/ecma-262/6.0/#sec-object-initializer-runtime-semantics-evaluation
     
> ComputedPropertyName : [ AssignmentExpression ]
>    1. Let exprValue be the result of evaluating AssignmentExpression.
>    2. Let propName be GetValue(exprValue).
>    3. ReturnIfAbrupt(propName).
>    4. Return ToPropertyKey(propName).

I think that Computed property name should executed `ToPropertyKey` soon after evaluate.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Properly perform ToPropertyKey operations involved in computed property names in both object literals and destructuring patterns

Looking at the places that handle computed property names in the emitter turned up an additional case that we get wrong.  Specifically, the developer who fixed bug 1155900 was a doof.  It's not okay to do a RequireObjectCoercible before a non-empty object destructuring pattern in the case where the first property-target pair has a computed property name as the property -- with the potential for side effects in its evaluation.  (And further side effects if we were to too-soon ToPropertyKey it.)  This patch fixes both that fresh issue, and the one originally reported.

Paging André for review because clearly I am not so great at reading the spec on this.  Oh, and also because if I'm reading this right, es6draft is also buggy in the same fashion.  :-)

Comment 2

[Jeff Walden [:Waldo]]

Oh, on the issue of bugs: within the AssignmentProperty: PropertyName : AssignmentElement rule in DestructuringAssignmentEvaluation, isn't there a missing GetValue(name) somewhere, to actually get a value out of the computed property name expression?  Otherwise it could be a Reference or something, and not a fully-computed value.

Comment 3

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Oh, on the issue of bugs: within the AssignmentProperty: PropertyName :
> AssignmentElement rule in DestructuringAssignmentEvaluation, isn't there a
> missing GetValue(name) somewhere, to actually get a value out of the
> computed property name expression?  Otherwise it could be a Reference or
> something, and not a fully-computed value.

Computed property name evaluation (12.2.6.8) never returns a Reference type, so it's not necessary to call GetValue in 12.14.5.2.

Comment 4

[André Bargull [:anba]]

Comment on attachment 8654001 [details] [diff] [review]
Properly perform ToPropertyKey operations involved in computed property names in both object literals and destructuring patterns

Review of attachment 8654001 [details] [diff] [review]:
-----------------------------------------------------------------

r+ = me when reverting the RequireObjectCoercible change in emitDestructuringOpsObjectHelper, and adding JSOP_TOID for destructuring keys. Fixing the reference resolution for keyed destructuring assignment should go into a new bug.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4025,5 @@
> +        //
> +        // So only do this for empty patterns.  Yay special cases!
> +        if (!emitRequireObjectCoercible())                        // ... RHS
> +            return false;
> +    }

https://bugs.ecmascript.org/show_bug.cgi?id=4351 was fixed before publication, so this special case is not correct.

@@ +4079,5 @@
> +                //     throw new Error("not reached");
> +                //   }
> +                //   catch (e) { assertEq(e instanceof TypeError, true); }
> +                //   assertEq(res, "PASS");
> +                //

Well.... actually it's required to emit JSOP_TOID to support 'interesting' cases like:
```
var scope = new Proxy({x: 0}, {
  has(t, pk) {
    print("has:" + pk);
    return Reflect.has(t, pk);
  }
});
var obj = {get v() { print("get"); }};
var key = {toString() { print("toStr"); return "v"; }};
with (scope) {
  ({[key]: x} = obj)
}
```

Per ES2015 that program should output: "has:obj has:key toStr has:x get".
Current SpiderMonkey output is: "has:obj has:key toStr get has:x".

12.14.5.4 KeyedDestructuringAssignmentEvaluation step 1 requires to resolve the reference before retrieving the property value. But fixing that problem should happen in a different bug, probably.

::: js/src/jit-test/tests/basic/destructuring-null-or-undefined-into-computed-property-name.js
@@ +18,5 @@
> +assertEq(outer, "modified");
> +
> +outer = "unmodified";
> +assertThrowsInstanceOf(() => f(null), TypeError);
> +assertEq(outer, "modified");

`assertEq(outer, "unmodified");` per comment in BE.

@@ +22,5 @@
> +assertEq(outer, "modified");
> +
> +outer = "unmodified";
> +assertThrowsInstanceOf(() => f(undefined), TypeError);
> +assertEq(outer, "modified");

Same here.

Comment 5

[Jeff Walden [:Waldo]]

(In reply to André Bargull from comment #4)
> https://bugs.ecmascript.org/show_bug.cgi?id=4351 was fixed before
> publication, so this special case is not correct.

Bah.  I thought the rev38 I had was final.  :-(

> @@ +4079,5 @@
> > +                //     throw new Error("not reached");
> > +                //   }
> > +                //   catch (e) { assertEq(e instanceof TypeError, true); }
> > +                //   assertEq(res, "PASS");
> > +                //
> 
> Well.... actually it's required to emit JSOP_TOID to support 'interesting'
> cases like:

Er -- oh, evaluating PropertyName returns a ToPropertyKey'd thing, not jsut the value of the expression nested in it.  I'll merge the emitTree/toid into a single emitComputedPropertyName so it's harder to forget this.

> Per ES2015 that program should output: "has:obj has:key toStr has:x get".
> Current SpiderMonkey output is: "has:obj has:key toStr get has:x".
> 
> 12.14.5.4 KeyedDestructuringAssignmentEvaluation step 1 requires to resolve
> the reference before retrieving the property value. But fixing that problem
> should happen in a different bug, probably.

Eugh.  Different bug for sure, this requires a decent bit of internal rejiggering.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0f76dc4f5b0

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b0f76dc4f5b0