Wrong order of operations with destructuring and computed property name

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dc578av_adle, Assigned: Waldo)

Tracking

(Blocks 1 bug)

43 Branch
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

4 years ago
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.
Reporter

Updated

4 years ago
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Blocks: es6
Assignee

Comment 1

4 years ago
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.  :-)
Attachment #8654001 - Flags: review?(andrebargull)
Assignee

Updated

4 years ago
Assignee: nobody → jwalden+bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee

Comment 2

4 years ago
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.
(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 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.
Attachment #8654001 - Flags: review?(andrebargull) → review+
Assignee

Comment 5

4 years ago
(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.
Summary: Ignore order of evaluation for Computed property name → Wrong order of operations with destructuring and computed property name
https://hg.mozilla.org/mozilla-central/rev/b0f76dc4f5b0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.