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.

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

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

Merged
merged 12 commits into from Sep 18, 2020

Conversation

barronwei
Copy link
Contributor

@barronwei barronwei commented Jun 17, 2020

Q                       A
Fixed Issues? Fixes #11608
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes? No
License MIT

This PR checks for whether the last statement in the BlockStatement case for getCompletionRecords is a BreakStatement.

As of right now, the code just pops off the last statement of a BlockStatement and pushes that onto completionRecords.

This last statement could be a BreakStatement in either a IfStatement or LabeledStatement, rendering the return value to be undefined.

This PR correctly handles:

const x = (n) => do {
  switch (n) {
    case 0:
      if (true) {
        'out';
        break;
      }
      
   }
}
const x = do {
  y: {
    'out';
    break y;
  }
}

@babel-bot
Copy link
Collaborator

babel-bot commented Jun 17, 2020

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/24195/

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 17, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit cd1fdbb:

Sandbox Source
happy-frog-you2w Configuration
zen-cartwright-yg88d Configuration

@barronwei barronwei changed the title Gh issue 11608 fix(do-expr): SwitchStatement with IfStatement and LabeledStatement cases Jun 17, 2020
@JLHwung JLHwung self-requested a review June 17, 2020 19:45
@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Do Expressions labels Jun 17, 2020
} 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

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

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!

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;
  }

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!

@JLHwung JLHwung changed the base branch from master to main June 19, 2020 12:40
@barronwei barronwei requested a review from JLHwung June 19, 2020 17:24
@JLHwung JLHwung changed the title fix(do-expr): SwitchStatement with IfStatement and LabeledStatement cases fix(do-expr): SwitchStatement with IfStatement cases Jun 19, 2020
Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

Nice work. Thanks!

@barronwei
Copy link
Contributor Author

Is there anything that needs to be changed?

@JLHwung
Copy link
Contributor

JLHwung commented Jul 10, 2020

@barronwei Generally we have two-approval process for PR. I will ping some core members to see if they can review.

cc @existentialism @kaicataldo @nicolo-ribaudo

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@JLHwung JLHwung merged commit 1a8e7ff into babel:main Sep 18, 2020
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Dec 19, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Do Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

do-expressions and nested breaks
5 participants