await without function call is dropped

[Conduitry]

Regression in 0.49.

async function foo() {
	await bar;
	baz();
}

foo();

produces

async function foo() {
	baz();
}

foo();

[Conduitry]

await Promise.all([ bar1, bar2 ]); is also dropped.

[kzc]

The old rollup tree shaking algo assumed that unregistered AST node types such as AwaitExpression had side effects and as such should be included. The new tree shaking algo assumes unregistered node types have no side effects and as such are not included.

The following patch addresses this issue with no test regressions:

diff --git a/src/ast/Node.js b/src/ast/Node.js
index 9f1e2b8..479b0a0 100644
--- a/src/ast/Node.js
+++ b/src/ast/Node.js
@@ -30,7 +30,7 @@ export default class Node {
        }
 
        hasEffects () {
-               return this.included || this.someChild( child => child.hasEffects() );
+               return this.included || this.someChild( child => child.hasEffects() || child.shouldBeIncluded() );
        }
 
        hasEffectsWhenAssigned () {
diff --git a/src/ast/nodes/AwaitExpression.js b/src/ast/nodes/AwaitExpression.js
new file mode 100644
index 0000000..ace39f0
--- /dev/null
+++ b/src/ast/nodes/AwaitExpression.js
@@ -0,0 +1,7 @@
+import Node from '../Node.js';
+
+export default class AwaitExpression extends Node {
+       hasEffects () {
+               return true;
+       }
+}
diff --git a/src/ast/nodes/index.js b/src/ast/nodes/index.js
index 7d28891..f924c12 100644
--- a/src/ast/nodes/index.js
+++ b/src/ast/nodes/index.js
@@ -1,6 +1,7 @@
 import ArrayPattern from './ArrayPattern.js';
 import ArrowFunctionExpression from './ArrowFunctionExpression.js';
 import AssignmentExpression from './AssignmentExpression.js';
+import AwaitExpression from './AwaitExpression.js';
 import BinaryExpression from './BinaryExpression.js';
 import BlockStatement from './BlockStatement.js';
 import CallExpression from './CallExpression.js';
@@ -44,6 +45,7 @@ export default {
        ArrayPattern,
        ArrowFunctionExpression,
        AssignmentExpression,
+       AwaitExpression,
        BinaryExpression,
        BlockStatement,
        CallExpression,

However I don't like it as it is not very robust in the face of future ecmascript additions - or even types like BreakStatement which is not presently registered in Rollup. An unregistered AST node type should be considered a side effect and always be included.

@lukastaegert Thoughts?

[lukastaegert]

For this particular case, I reused the logic I just added for return statements so that we should have proper side-effect detection in async functions.
@kzc I agree that unknown nodes should always have effects, I will look into this now.

[lukastaegert]

@kzc You were very right, I found quite a few more problems when making sure all unknown nodes have effects. Should be mostly fixed with #1586 now.

[kzc]

commit fc70479
...
* Assume that unknown nodes always have effects

@lukastaegert Is that comment correct? If that were the case one could safely remove the implementation of AwaitExpression and it should still behave correctly. However five tests fail when the following patch is applied to rollup version 0.49.1 (which includes #1586) :

diff --git a/src/ast/nodes/AwaitExpression.js b/src/ast/nodes/AwaitExpression.js
deleted file mode 100644
index 673d434..0000000
--- a/src/ast/nodes/AwaitExpression.js
+++ /dev/null
@@ -1,8 +0,0 @@
-import Node from '../Node.js';
-
-export default class AwaitExpression extends Node {
-       hasEffects ( options ) {
-               return super.hasEffects( options )
-                       || !options.inNestedFunctionCall;
-       }
-}
diff --git a/src/ast/nodes/index.js b/src/ast/nodes/index.js
index 08fbe74..eb6afee 100644
--- a/src/ast/nodes/index.js
+++ b/src/ast/nodes/index.js
@@ -1,7 +1,6 @@
 import ArrayPattern from './ArrayPattern.js';
 import ArrowFunctionExpression from './ArrowFunctionExpression.js';
 import AssignmentExpression from './AssignmentExpression.js';
-import AwaitExpression from './AwaitExpression.js';
 import BinaryExpression from './BinaryExpression.js';
 import BlockStatement from './BlockStatement.js';
 import BreakStatement from './BreakStatement';
@@ -53,7 +52,6 @@ export default {
        ArrowFunctionExpression,
        AssignmentExpression,
        AssignmentPattern: Node,
-       AwaitExpression,
        BinaryExpression,
        BlockStatement,
        BreakStatement,

[lukastaegert]

This is intended. This test in question is indeed "side-effects-await" because instead of assuming that await always has an effect, I implemented a hopefully correct tree-shaking logic which is tested and goes like this:

• If the argument of await has a side-effect, include the statement and the surrounding function
• If a function is included for any reason, all await statements inside that function are considered to have effects and are therefore included (the same as for return statements) to ensure proper control flow

[kzc]

I see. I had misread the new test. The test case at the top of this ticket does indeed work in latest rollup master if the AwaitExpression implementation is removed.

Is there a test for an ESTree node type not presently implemented in Rollup that demonstrates this implicit side effect logic?

[lukastaegert]

Unfortunately not. I was not sure what node to use for such a test, the ultimate goal being to properly handle ALL ESTree nodes. Maybe it is possible to write a different kind of test for this that artificially adds a new node type but I am not sure how to do this properly right now. Of course, any input is welcome here as I agree that this should indeed be tested.

[kzc]

If Rollup were to implement the ability to specify acorn plugins in rollup.config.js to extend the parser then it would be straightforward to test "unknown" node types.

Testing aside, such a rollup parser extension with the ability to register new Rollup AST classes at runtime would be useful in its own right - see #1512.

[lukastaegert]

I would love that. The ability to specify new nodes would also enable us to write plugins that e.g. make certain tree-shaking aspects less or more aggressive. And more. There is no limit 😎

Right now the next big topic I want to address is refactoring call handling, and then maybe check if we can add some kind of assignment tracking to member expressions, but maybe after that...