Skip to content

Fix destructuring evaluation order for initializers #41094

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 30, 2020

Conversation

rbuckton
Copy link
Contributor

@rbuckton rbuckton commented Oct 14, 2020

This ensures we correctly follow the order of evaluation for destructuring (initializers are evaluated before computed property names in an object pattern), per the following algorithms (emphasis added):

12.15.7.5 - Runtime Semantics: IteratorDestructuringAssignmentEvaluation

With parameter iteratorRecord.

...

AssignmentElement : DestructuringAssignmentTarget Initializer?
  1. If DestructuringAssignmentTarget is neither an _ObjectLiteral nor an ArrayLiteral, then
    1. Let lref be the result of evaluating DestructuringAssignmentTarget.
    2. ReturnIfAbrupt(lref).
  2. If iteratorRecord.[[Done]] is false, then
    1. Let next be IteratorStep(iteratorRecord).
    2. If next is an abrupt completion, set iteratorRecord.[[Done]] to true.
    3. ReturnIfAbrupt(next).
    4. If next is false, set iteratorRecord.[[Done]] to true.
    5. Else,
      1. Let value be IteratorValue(next).
      2. If value is an abrupt completion, set iteratorRecord.[[Done]] to true.
      3. ReturnIfAbrupt(value).
  3. If iteratorRecord.[[Done]] is true, let value be undefined.
  4. If Initializer is present and value is undefined, then
    1. If IsAnonymousFunctionDefinition(Initializer) is true and IsIdentifierRef of DestructuringAssignmentTarget is true, then
      1. Let v be NamedEvaluation of Initializer with argument GetReferencedName(lref).
    2. Else,
      1. Let defaultValue be the result of evaluating Initializer.
      2. Let v be ? GetValue(defaultValue).
  5. Else, let v be value.
  6. If DestructuringAssignmentTarget is an ObjectLiteral or an ArrayLiteral, then
    1. Let nestedAssignmentPattern be the AssignmentPattern that is covered by DestructuringAssignmentTarget.
    2. Return the result of performing DestructuringAssignmentEvaluation of nestedAssignmentPattern with v as the argument.
  7. Return ? PutValue(lref, v).

12.15.7.6 Runtime Semantics: KeyedDestructuringAssignmentEvaluation

With parameters value and propertyName.

...

AssignmentElement : DestructuringAssignmentTarget Initializer?
  1. If DestructuringAssignmentTarget is neither an ObjectLiteral nor an ArrayLiteral, then
    1. Let lref be the result of evaluating DestructuringAssignmentTarget.
    2. ReturnIfAbrupt(lref).
  2. Let v be ? GetV(value, propertyName).
  3. If Initializer is present and v is undefined, then
    1. If IsAnonymousFunctionDefinition(Initializer) and IsIdentifierRef of DestructuringAssignmentTarget are both 1, then
      1. Let rhsValue be NamedEvaluation of Initializer with argument GetReferencedName(lref).
    2. Else,
      1. Let defaultValue be the result of evaluating Initializer.
      2. Let rhsValue be ? GetValue(defaultValue).
  4. Else, let rhsValue be v.
  5. If DestructuringAssignmentTarget is an ObjectLiteral or an ArrayLiteral, then
    1. Let assignmentPattern be the AssignmentPattern that is covered by DestructuringAssignmentTarget.
    2. Return the result of performing DestructuringAssignmentEvaluation of assignmentPattern with rhsValue as the argument.
  6. Return ? PutValue(lref, rhsValue).

13.3.3.8 - Runtime Semantics: IteratorBindingInitialization

With parameters iteratorRecord and environment.

...

BindingElement : BindingPattern Initializer?
  1. If iteratorRecord.[[Done]] is false, then
    1. Let next be IteratorStep(iteratorRecord).
    2. If next is an abrupt completion, set iteratorRecord.[[Done]] to true.
    3. ReturnIfAbrupt(next).
    4. If next is false, set iteratorRecord.[[Done]] to true.
    5. Else,
      1. Let v be IteratorValue(next).
      2. If v is an abrupt completion, set iteratorRecord.[[Done]] to true.
      3. ReturnIfAbrupt(v).
  2. If iteratorRecord.[[Done]] is true, let v be undefined.
  3. If Initializer is present and v is undefined, then
    1. Let defaultValue be the result of evaluating Initializer.
    2. Set v to ? GetValue(defaultValue).
  4. Return the result of performing BindingInitialization of BindingPattern with v and environment as the arguments.

13.3.3.9 Runtime Semantics: KeyedBindingInitialization

With parameters value, environment, and propertyName.

...

BindingElement : BindingPattern Initializer?
  1. Let v be ? GetV(value, propertyName).
  2. If Initializer is present and v is undefined, then
    1. Let defaultValue be the result of evaluating Initializer.
    2. Set v to ? GetValue(defaultValue).
  3. Return the result of performing BindingInitialization for BindingPattern passing v and environment as arguments.

Fixes #39205
Fixes #39181

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Oct 14, 2020
@evanw
Copy link
Contributor

evanw commented Oct 14, 2020

Thanks for working on this! I'm excited that this is going to be fixed.

FWIW it could be useful to double-check this PR with the fuzzer I wrote: https://github.com/evanw/js-destructuring-fuzzer. For example, I just ran this branch through the fuzzer and I think there are still cases that result in generating code with syntax errors. Here's one such case:

let a, b, c = {x: {a: 1, y: 2}};
({x: {a, ...b} = foo} = c);
console.log(a, b);

That should print out 1 { y: 2 } but instead I get SyntaxError: Invalid left-hand side in assignment with the code generated by the TypeScript compiler, which looks like this:

var _a;
var a, b, c = { x: { a: 1, y: 2 } };
((_a = foo.a, foo, b = __rest(foo, ["a"]), foo) = c.x);
console.log(a, b);

@rbuckton
Copy link
Contributor Author

@evanw: Thanks, I'll take a look at that

@rbuckton
Copy link
Contributor Author

@weswigham @sandersn, either of you have a moment to spare to take a look?

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This emit looks OK (it's admittedly dense), but should we also be testing that control for narrowing reflects this order of execution as well? (Eg, replace order(0) with (x, x = 0) and so on)

@rbuckton
Copy link
Contributor Author

rbuckton commented Oct 24, 2020

Looks like control-flow is wrong as well:

Case 1

image

y should be 0 because the default is evaluated before the computed property name.

Case 2

image

Here y should be 9 because the default evaluates first.

Cases 3 & 4

We also don't seem to narrow when we know the default won't be evaluated:

image
image

In both cases above, y should be 8 because the default will never be evaluated due to the fact the outer destructured value is a tuple with a fixed length.

Case 5

image

In this case y should be 0 | 8 because we don't know the length of the outer destructured array, and 0 should be the constituent instead of 1 because of Case 1 (above).

Case 6

image

This case should probably be considered correct. y should be 0 | 8 because we don't know the length of the outer destructured array.

@rbuckton rbuckton force-pushed the destructuringEvaluationOrder branch from 03bd565 to 8a15291 Compare October 28, 2020 23:50
@rbuckton
Copy link
Contributor Author

I've updated the control flow for destructuring, though I've decided to leave cases 3 & 4 as is (you will still end up with the union of the assigned value and the default value).

@rbuckton
Copy link
Contributor Author

@weswigham can you take a look at the binder changes?

@rbuckton rbuckton merged commit bcbe1d7 into master Oct 30, 2020
@rbuckton rbuckton deleted the destructuringEvaluationOrder branch October 30, 2020 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Incorrect side effect ordering in destructuring transform Syntax error when lowering object rest into Object.assign
4 participants