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
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/24195/ |
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:
|
} else if (this.isProgram() || this.isBlockStatement()) { | ||
} else if (this.isProgram()) { | ||
paths = addCompletionRecords(this.get("body").pop(), paths); | ||
} else if (this.isBlockStatement()) { |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; | ||
} |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. Thanks!
Is there anything that needs to be changed? |
@barronwei Generally we have two-approval process for PR. I will ping some core members to see if they can review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
This PR checks for whether the last statement in the
BlockStatement
case forgetCompletionRecords
is aBreakStatement
.As of right now, the code just pops off the last statement of a
BlockStatement
and pushes that ontocompletionRecords
.This last statement could be a
BreakStatement
in either aIfStatement
orLabeledStatement
, rendering the return value to beundefined
.This PR correctly handles: