Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(do-expr): SwitchStatement with IfStatement cases #11728

Merged
merged 12 commits into from Sep 18, 2020
Merged

fix(do-expr): SwitchStatement with IfStatement cases #11728

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7ce7758
test(do-expr): condition before break
Jun 17, 2020
ed71ebf
test(do-expr): labeled break
Jun 17, 2020
8439fc8
fix(do-expr): add check for break in BlockStatement
Jun 17, 2020
363e2a5
fix(do-expr): add LabeledStatement case for getCompletionRecords
Jun 17, 2020
063baf0
test(do-expr): rename condition before break to condition before expr…
Jun 18, 2020
38088a1
revert(do-expr): undo remove break for do switch
Jun 18, 2020
1cbb509
revert(do-expr): undo remove labeled break
Jun 18, 2020
8cadeb1
test(do-expr): add condition before break
Jun 18, 2020
3723a28
test(do-expr): update condition before break
Jun 18, 2020
9510c99
test(do-expr): remove labeled break
Jun 18, 2020
a3c491a
fix(do-expr): add tree search in findBreak
Jun 18, 2020
cd1fdbb
fix(do-expr): ignore isFunction case in findBreak
Jun 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions ...al-do-expressions/test/fixtures/do-expression-switch-case/condition-before-break/input.js
View file
Open in desktop
@@ -0,0 +1,10 @@
const x = (n) => do {
switch (n) {
case 0:
if (true) {
'out';
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test case where BreakStatement does not follow an ExpressionStatement?

case 1:
  if (true) {
    break;
  }


}
}
9 changes: 9 additions & 0 deletions ...l-do-expressions/test/fixtures/do-expression-switch-case/condition-before-break/output.js
View file
Open in desktop
@@ -0,0 +1,9 @@
const x = n => function () {
switch (n) {
case 0:
if (true) {
return 'out';
}

}
}();
6 changes: 6 additions & 0 deletions .../babel-plugin-proposal-do-expressions/test/fixtures/do-expressions/labeled-break/input.js
View file
Open in desktop
@@ -0,0 +1,6 @@
const x = do {
y: {
'out';
break y;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can not remove labelled break statement without more execution context, i.e.

// returns 42
do { y: { "out"; break y; } 42 }

I don't come up with a satisfiable fix regarding to the LabeledStatement without significant rewriting getCompetionsRecords. I suggest we separate this fix into another PR and this PR is focused on SwitchStatement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do, but does getCompletionRecords not get the last node of the BlockStatement, which would be 42 in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we replace "out"; break y; by return "out", the compiled output will be

(() => { y: { return "out"; } return 42 } })()
// returns "out"

I don't think we correctly handle the labeled break statement in getCompletionRecords, so I suggest we address LabelStatement in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see. I forgot about the loop. Thanks for the feedback, and sounds good!

}
}
3 changes: 3 additions & 0 deletions ...el-plugin-proposal-do-expressions/test/fixtures/do-expressions/labeled-break/options.json
View file
Open in desktop
@@ -0,0 +1,3 @@
{
"plugins": ["transform-block-scoping", "proposal-do-expressions"]
}
5 changes: 5 additions & 0 deletions ...babel-plugin-proposal-do-expressions/test/fixtures/do-expressions/labeled-break/output.js
View file
Open in desktop
@@ -0,0 +1,5 @@
var x = function () {
y: {
return 'out';
}
}();
11 changes: 9 additions & 2 deletions packages/babel-traverse/src/path/family.js
View file
Open in desktop
Expand Up @@ -81,14 +81,21 @@ export function getCompletionRecords(): NodePath[] {
paths = addCompletionRecords(this.get("alternate"), paths);
} else if (this.isDoExpression() || this.isFor() || this.isWhile()) {
paths = addCompletionRecords(this.get("body"), paths);
} else if (this.isProgram() || this.isBlockStatement()) {
} else if (this.isProgram()) {
paths = addCompletionRecords(this.get("body").pop(), paths);
} else if (this.isBlockStatement()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can not remove BreakStatement because it will lose the completion record when the prevSibling is neither an ExpressionStatement nor a BlockStatement. i.e.

do { switch(0) { case 0: {
  if (true) { break; } "unreachable"; 
  } }
}

If break is removed, "unreachable" will be incorrectly included in the completion record.

The completion record tracking of SwitchStatement was introduced at #10070. The BreakStatement is handled in

babel/packages/babel-traverse/src/path/family.js

Lines 52 to 60 in e498bee

if (
breakStatement.key > 0 &&
(prevSibling.isExpressionStatement() || prevSibling.isBlockStatement())
) {
paths = addCompletionRecords(prevSibling, paths);
breakStatement.remove();
} else {
breakStatement.replaceWith(breakStatement.scope.buildUndefinedNode());
paths = addCompletionRecords(breakStatement, paths);

The BreakStatement is searched in consequent and consequent[type=BlockStatement] > body

babel/packages/babel-traverse/src/path/family.js

Lines 29 to 41 in e498bee

findBreak: for (const statement of consequent) {
if (statement.isBlockStatement()) {
for (const statementInBlock of statement.get("body")) {
if (statementInBlock.isBreakStatement()) {
breakStatement = statementInBlock;
break findBreak;
}
}
} else if (statement.isBreakStatement()) {
breakStatement = statement;
break;
}
}

Apparently this routine can not find BreakStatement nested in IfStatement of the SwitchCase. We can do a tree search for BreakStatement, which resembles getCompletionRecords.

function findBreak(statements) {
  let breakStatement;
  if (!Array.isArray(statements)) {
    statements = [statements];
  }

  for (const statement of statements) {
    if (statement.isBlockStatement()) {
      breakStatement = findBreak(statement.get("body"));
    } else if (statement.isIfStatement()) {
      breakStatement =
        findBreak(statement.get("consequent")) ??
        findBreak(statement.get("alternate"));
    } else if (statement.isCatchClause() || statement.isLabeledStatement()) {
      breakStatement = findBreak(statement.get("body"));
    // todo: add other statements except Iteration and Switch, which is breakable statement.
    } else if (statement.isBreakStatement()) {
      breakStatement = statement;
    }
    if (breakStatement) {
      return breakStatement;
    }
  }
  return null;
}

By doing so we can reuse current completionRecordForSwitch routine which already handled BreakStatement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the reasoning behind returning getCompletionRecords as opposed to appending to paths in the isFunction case?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good question! Each function owns a new execution context. So when a Function is met, we should skip finding break inside the function body because the statements inside can not interfere with completion records (control flows) outside. i.e.

{
  a: 42;
  () => { break a }
}

is invalid because the label a is not defined in the execution context own by the arrow function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That completely makes sense. I have made the necessary changes. Please let me know if there needs to be any others!

const body = this.get("body");
const last = body[body.length - 1];
if (last.isBreakStatement()) {
last.remove();
}
paths = addCompletionRecords(this.get("body").pop(), paths);
} else if (this.isFunction()) {
return this.get("body").getCompletionRecords();
} else if (this.isTryStatement()) {
paths = addCompletionRecords(this.get("block"), paths);
paths = addCompletionRecords(this.get("handler"), paths);
} else if (this.isCatchClause()) {
} else if (this.isCatchClause() || this.isLabeledStatement()) {
paths = addCompletionRecords(this.get("body"), paths);
} else if (this.isSwitchStatement()) {
paths = completionRecordForSwitch(this.get("cases"), paths);
Expand Down