fix: do-statementlist behavior

[wlawt]

Q	A
Fixed Issues?	Fixes #9404
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

Added check for multi-use of semicolons where it was returning void 0 before.

Input

expect(do {
  x = do { 1; };
  z = do { 1;;;; };
  w = (do { 1;;;; });
})

Output

x=1
z=1
w=1

The parsing issue addressed in #9404 seems to be correct and ok (reference: 9404 comment)

[babel-bot]

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

[codesandbox-ci]

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 f469bdf:

Sandbox	Source
falling-darkness-mn2hx	Configuration
dreamy-chaum-5ktd1	Configuration

[JLHwung]

If all of nodes are empty statements, it should ensureLastUndefined. i.e. both eval("{ ; }") and eval("{ ;;; }") returns undefined. Note that you can mentally think of do { ; } as eval("{ ; }").

However, when empty statements follow any value producing statements, it should not ensureLastUndefined. i.e. eval("{ 3;; }") returns 3.

Therefore we should check

• If the empty statement is the first of nodes, we should ensureLastUndefined, otherwise it is a no-op because it can always be omitted. Conceptually it is more like ensureFirstUndefined.

I don't think we need to track multiColons because

• the specific value of multiColons is not used as a number later
• Transformers operates on AST level (EmptyStatement v.s. colon) while colon is a grammar structure and should be addressed by tokenizer instead

[wlawt]

yup that makes sense, just have a question: if accessing the path isn't available to check if it's inList or key is there a way to do it with the array of nodes being passed to the function?

I've also explored the scope that gets passed and trying to use it's path but inList and key are undefined for it.

[JLHwung]

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.

[wlawt]

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?

[JLHwung]

() 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

[wlawt]

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

[JLHwung]

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] )

[wlawt]

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

[JLHwung]

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.

[wlawt]

That makes so much sense, thank u!

[JLHwung]

The test is failing because the test assumption does not hold if the empty statement is following a value producing statement, in this test case, undefined ; will not produce a sequence expression because the empty statement is not the leading statement. Therefore sequence.expression is undefined.

However, asserting an error thrown because it is in fact not an sequence expression can be confusing to other contributors. Moreover, we know that it still gathers empty statement only if the EmptyStatement is the first element:

So we need to revise the test and clearly state its new behaviour

it("gathers leading empty statement", function() {
  const node = parseCode(";");
  const sequence = t.toSequenceExpression([node, t.cloneNode(node)], scope);
  expect(generateCode(sequence)).toBe("undefined");
});

[JLHwung]

Can you also change the test title here? Because we have changed the behaviour.

[JLHwung]

Thanks.

[JLHwung]

The e2e CI error has been fixed on main.