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-statementlist behavior #11724

Merged
merged 8 commits into from Jun 25, 2020
Merged

fix: do-statementlist behavior #11724

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ed2759d
fix: statementlist behavior
wlawt Jun 16, 2020
3fea0ec
fixed prettier and babel-types/converter errors
wlawt Jun 16, 2020
6812a42
added check for first node
wlawt Jun 18, 2020
42ea46e
remove node length check, only check if node is first
wlawt Jun 19, 2020
78cdd64
add reset eLU if proceeding is non-empty
wlawt Jun 20, 2020
87a9784
fix failure for ci tests
wlawt Jun 22, 2020
8001aef
remove .expressions since sequence expression isn't produced
wlawt Jun 22, 2020
f469bdf
changed test title
wlawt Jun 23, 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
13 changes: 13 additions & 0 deletions packages/babel-plugin-proposal-do-expressions/test/fixtures/do-expressions/semicolons.js
View file
Open in desktop
@@ -0,0 +1,13 @@
expect(do {
JLHwung marked this conversation as resolved.
Show resolved Hide resolved
x = do { 1; };
}).toBe(1);

expect(do {
z = do { 1;;;; };
}).toBe(1)

expect(do {
w = (do { 1;;;; });
}).toBe(1);

expect(do { ;; }).toBe(undefined);
7 changes: 6 additions & 1 deletion packages/babel-types/src/converters/gatherSequenceExpressions.js
View file
Open in desktop
Expand Up @@ -66,7 +66,12 @@ export default function gatherSequenceExpressions(
exprs.push(body);
} else if (isEmptyStatement(node)) {
// empty statement so ensure the last item is undefined if we're last
ensureLastUndefined = true;
// checks if node is first
// checks if input is `;` or `;;` which will return `()` in where
// the length will always be 2
if (nodes.indexOf(node) === 0 || nodes.length === 2) {
Copy link
Contributor

@JLHwung JLHwung Jun 18, 2020 •
edited

Choose a reason for hiding this comment

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

I don't think we need to check specifically nodes.length === 2? i.e. do { ;;; } should also return undefined.

To mark non-leading EmptyStatement as no-op, we can reset ensureLastUndefined = false only if node is not an EmptyStatement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i added the node.length because () always threw an error regardless of the amount of ; given.

just to clarify what does no-op mean ?

and do we need to explicitly mark it false since we're in the condition that it is true?

Copy link
Contributor

Choose a reason for hiding this comment

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

() always threw an error regardless of the amount of ; given.

It is syntax error if there is only empty statement in the (), i.e. (), (;), (;;) should throw.

just to clarify what does no-op mean ?

no-op means we do not change ensureLastUndefined, as you can see it is reset as false in the beginning of loop. We can check empty statement before validating.

Let eLU be ensureLastUndefined, we can illustrate how eLU is changed between EmptyStatements ant other statements.

;                ;;;;;                              42                  ;;;;;
|--> eLU = true  |--> eLU = (previous eLU) = true   |--> eLU = false    |--> eLU= (previous eLU) = false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if () , (;) , (;;) should throw would it be a different issue because checking if node is first fails the cases just mentioned?

Copy link
Contributor

Choose a reason for hiding this comment

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

Babel currently throws syntax error for (), (;), which is expected. Generally when you are working on AST, you may assume that it is always valid combinations of AST nodes, in this case, it is EmptyStatement nested in BlockStatement. The combinations of punctuators, i.e. (, ) should be cared by the parser, and by the time when you have a valid AST, it can not be a representation of the source like ().

Generally parentheses do not affect the semantic meaning, it is a valid combination only if there is an expression wrapped inside parentheses: https://tc39.es/ecma262/#prod-ParenthesizedExpression

ParenthesizedExpression[Yield, Await]:
  ( Expression[+In, ?Yield, ?Await] )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, ok the changes i just did keeps the checking if node is first and letting it throw if () , (;)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I understand what you mean by () and (;)

In the example { ;; }, the parse AST is

[ EmptyStatement, EmptyStatement ]

In the first loop, ensureLastUndefined is set to true.
In the second loop, ensureLastUndefined is set to false in the beginning of the loop body:

babel/packages/babel-types/src/converters/gatherSequenceExpressions.js

Line 28 in 6812a42

ensureLastUndefined = false;

Since exprs is an empty array, ensureLastUndefined is false, sequenceExpression(exprs) will be printed as (), which will throw syntax error because sequence expression can not be empty.

We can reset ensureLastUndefined only if node is not an EmptyStatement.

for (const node of nodes) {
  if (!isEmptyStatement(node)) {
    ensureLastUndefined = false;
  }
// ...
}

So this ensures that when we are processing an EmptyStatement following any statements, it becomes an no-op, which does not change ensureLastUndefined.

If we only check node is first of nodes && nodes.length === 2, it can not handle do { ;;; }: When processing the third empty statement, ensureLastUndefined will still be reset to false and gatherSequenceExpressions will still generate an empty sequence expression.

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 makes so much sense, thank u!

ensureLastUndefined = true;
}
} else {
// bailed, we can't turn this statement into an expression
return;
Expand Down