Spec bug with object rest destructuring assignment

[woess]

Consider this test case from test262 which expects a TypeError:

var rest;
assert.throws(TypeError, function() {
  0, {...rest} = null;
});

The problem: I cannot find the corresponding passage in the spec.

I believe that 12.15.5.2 Runtime Semantics: DestructuringAssignmentEvaluation is missing a Perform ? RequireObjectCoercible(value). as first step.

I'll walk you through the path I took, so you can correct me if I went wrong somewhere.

12.15.4 Runtime Semantics: Evaluation
AssignmentExpression : LeftHandSideExpression = AssignmentExpression

1. If LeftHandSideExpression is neither an ObjectLiteral nor an ArrayLiteral [...]
2. Let assignmentPattern be the AssignmentPattern that is covered by LeftHandSideExpression.
3. Let rref be the result of evaluating AssignmentExpression.
4. Let rval be ? GetValue(rref).
5. Perform ? DestructuringAssignmentEvaluation of assignmentPattern using rval as the argument.
6. Return rval.

12.15.5.2 Runtime Semantics: DestructuringAssignmentEvaluation (value)
ObjectAssignmentPattern : { AssignmentRestProperty }

1. Let excludedNames be a new empty List.
2. Return the result of performing RestDestructuringAssignmentEvaluation of AssignmentRestProperty with value and excludedNames as the arguments.

12.15.5.4 Runtime Semantics: RestDestructuringAssignmentEvaluation (value, excludedNames)
AssignmentRestProperty : ... DestructuringAssignmentTarget

1. Let lref be the result of evaluating DestructuringAssignmentTarget.
2. ReturnIfAbrupt(lref).
3. Let restObj be ObjectCreate(%ObjectPrototype%).
4. Perform ? CopyDataProperties(restObj, value, excludedNames).
5. Return PutValue(lref, restObj).

7.3.23 CopyDataProperties (target, source, excludedItems)

1. Assert: Type(target) is Object.
2. Assert: excludedItems is a List of property keys.
3. If source is undefined or null, return target.

[anba]

Agreed. ObjectAssignmentPattern : { AssignmentRestProperty } and ObjectAssignmentPattern : { AssignmentPropertyList , AssignmentRestProperty } should both invoke RequireObjectCoercible as the first step.

[anba]

Agreed. ObjectAssignmentPattern : { AssignmentRestProperty } and ObjectAssignmentPattern : { AssignmentPropertyList , AssignmentRestProperty } should both invoke RequireObjectCoercible as the first step.

[1020431880]

你好啊，老铁

[1020431880]

你好啊，老铁

[littledan]

Agree with what was said above. Cc @sebmarkbage @keithamus

[littledan]

Agree with what was said above. Cc @sebmarkbage @keithamus

[sebmarkbage]

Hm. I don't think RequireObjectCoercible is the right thing here because it doesn't handle the case when it is a string or number. The ToObject operations deeply nested handles that.

For normal destructuring, this ToObject happens within the GetValue operation for each property access. So it happens multiple times.

Which is observable:

var xObj;
var yObj;
Object.defineProperty(
  String.prototype,
  'x',
  {
    get() {
      xObj = this;
    }
  }
);
Object.defineProperty(
  String.prototype,
  'y',
  {
    get() {
      yObj = this;
    }
  }
);

var { x, y } = "foo";

console.log(xObj === yObj); // false

Keeping in line with that CopyDataProperties does the ToObject operation for the rest property. So there will be one more for that one.

However, there is a special case in CopyDataProperties for null/undefined so it doesn't get passed to ToObject. So in this case, this is not a spec bug - the test case is wrong. This should not throw a type error.

If we wanted to change this we should refactor CopyDataProperties to not special case null/undefined but it is important for the spread operation where null/undefined should be silently ignored.

(I kind of think that we should change the semantics to do an early ToObject and only one per destructuring instead of multiple but that's a bigger breaking change. But that would also fix this.)

[sebmarkbage]

Hm. I don't think RequireObjectCoercible is the right thing here because it doesn't handle the case when it is a string or number. The ToObject operations deeply nested handles that.

For normal destructuring, this ToObject happens within the GetValue operation for each property access. So it happens multiple times.

Which is observable:

var xObj;
var yObj;
Object.defineProperty(
  String.prototype,
  'x',
  {
    get() {
      xObj = this;
    }
  }
);
Object.defineProperty(
  String.prototype,
  'y',
  {
    get() {
      yObj = this;
    }
  }
);

var { x, y } = "foo";

console.log(xObj === yObj); // false

Keeping in line with that CopyDataProperties does the ToObject operation for the rest property. So there will be one more for that one.

However, there is a special case in CopyDataProperties for null/undefined so it doesn't get passed to ToObject. So in this case, this is not a spec bug - the test case is wrong. This should not throw a type error.

If we wanted to change this we should refactor CopyDataProperties to not special case null/undefined but it is important for the spread operation where null/undefined should be silently ignored.

(I kind of think that we should change the semantics to do an early ToObject and only one per destructuring instead of multiple but that's a bigger breaking change. But that would also fix this.)

[anba]

Hm. I don't think RequireObjectCoercible is the right thing here because it doesn't handle the case when it is a string or number. The ToObject operations deeply nested handles that.

var {...x} = null; already throws a TypeError. This issue is to make destructuring assignment consistent with destructuring binding, so that ({...x} = null); also throws a TypeError. Not throwing for ({...x} = null); is also inconsistent with var {} = null; or ({} = null);, which both also already do throw a TypeError.

The observable multiple calls to ToObject in GetValue only matter for non-strict code, in strict code implementers can optimize away the ToObject call if wished.

[anba]

Hm. I don't think RequireObjectCoercible is the right thing here because it doesn't handle the case when it is a string or number. The ToObject operations deeply nested handles that.

var {...x} = null; already throws a TypeError. This issue is to make destructuring assignment consistent with destructuring binding, so that ({...x} = null); also throws a TypeError. Not throwing for ({...x} = null); is also inconsistent with var {} = null; or ({} = null);, which both also already do throw a TypeError.

The observable multiple calls to ToObject in GetValue only matter for non-strict code, in strict code implementers can optimize away the ToObject call if wished.

[leobalter]

I agree this should have a spec fix for the consistency pointed by @anba. The test is slightly wrong with the current spec because it followed consistency with the var {...x} = null;. Also, a fix would reflect the current implementations:

 eshost -x 'var rest; ({...rest} = null); print(rest)'
#### d8
TypeError: Cannot destructure 'undefined' or 'null'.

#### ch (no unflagged support yet for Obj Rest in the latest Release)
SyntaxError: Expected identifier, string or number

#### jsshell
TypeError: null has no properties

#### jsc
TypeError: Right side of assignment cannot be destructured

[leobalter]

I agree this should have a spec fix for the consistency pointed by @anba. The test is slightly wrong with the current spec because it followed consistency with the var {...x} = null;. Also, a fix would reflect the current implementations:

 eshost -x 'var rest; ({...rest} = null); print(rest)'
#### d8
TypeError: Cannot destructure 'undefined' or 'null'.

#### ch (no unflagged support yet for Obj Rest in the latest Release)
SyntaxError: Expected identifier, string or number

#### jsshell
TypeError: null has no properties

#### jsc
TypeError: Right side of assignment cannot be destructured

[sebmarkbage]

I see. ObjectAssignmentPattern: {AssignmentPropertyList} and equivalent binding patterns already has RequireObjectCoercible. As along as it is consistently always before any ToObject from GetValue, that seems fine to me then.

[sebmarkbage]

I see. ObjectAssignmentPattern: {AssignmentPropertyList} and equivalent binding patterns already has RequireObjectCoercible. As along as it is consistently always before any ToObject from GetValue, that seems fine to me then.

[ljharb]

Fixed by #1163.

[ljharb]

Fixed by #1163.