IIFE paren style affects tree shaking

[kzc]

Differing paren styles of IIFEs will affect whether code is tree shaken away.

$ node_modules/.bin/rollup --version
rollup version 0.36.3

$ cat iife.js 
(function () {
    function CrockfordIIFE() {}
    return CrockfordIIFE;
}());

(function () {
    function ClassicIIFE() {}
    return ClassicIIFE;
})();

!function () {
    function UglifyIIFE() {}
    return UglifyIIFE;
}();

Both CrockfordIIFE and UglifyIIFE were tree shaken away, but ClassicIIFE was not:

$ node_modules/.bin/rollup iife.js
(function () {
    function ClassicIIFE() {}
    return ClassicIIFE;
})();

This is odd because the acorn ASTs of ClassicIIFE and CrockfordIIFE are basically identical except for the start and end of the CallExpression:

--- iife_classic.js.ast	
+++ iife_crockford.js.ast	
@@ -9,8 +9,8 @@
       "end": 62,
       "expression": {
         "type": "CallExpression",
-        "start": 0,
-        "end": 61,
+        "start": 1,
+        "end": 60,
         "callee": {
           "type": "FunctionExpression",
           "start": 1,

[kzc]

I just learned something about acorn:

  // When enabled, parenthesized expressions are represented by
  // (non-standard) ParenthesizedExpression nodes
  preserveParens: false,

If the following change is made to rollup then all IIFE types are successfully tree shaken away:

--- a/src/Module.js
+++ b/src/Module.js
@@ -18,7 +18,7 @@ function tryParse ( code, comments, acornOptions, id ) {
                        ecmaVersion: 7,
                        sourceType: 'module',
                        onComment: ( block, text, start, end ) => comments.push({ block, text, start, end }),
-                       preserveParens: true
+                       preserveParens: false
                }, acornOptions ));
        } catch ( err ) {
                err.code = 'PARSE_ERROR';

However 3 export tests fail with this change so it's not a solution:

  1) rollup function default-exports-in-parens:
  2) rollup function export-two-ways-default-b:
  3) rollup function parenthesised-default-export:

So some tweaking of ParenthesizedExpression node handling in rollup is needed.

[kzc]

This is the fix to allow classic IIFEs to be tree shaken away. No regressions.

--- a/src/ast/nodes/CallExpression.js
+++ b/src/ast/nodes/CallExpression.js
@@ -5,6 +5,8 @@ import callHasEffects from './shared/callHasEffects.js';

 export default class CallExpression extends Node {
        bind ( scope ) {
+               while ( this.callee.type === 'ParenthesizedExpression' ) this.callee = this.callee.expression;
+
                if ( this.callee.type === 'Identifier' ) {
                        const declaration = scope.findDeclaration( this.callee.name );

[kzc]

There are problems with other ParenthesizedExpressions in rollup.

This code:

export default class AssignmentExpression extends Node {
        bind ( scope ) {
                let subject = this.left;
                while ( this.left.type === 'ParenthesizedExpression' ) subject = subject.expression;

                this.subject = subject;

errors out with TypeError: Cannot read property 'expression' of undefined on this input:

(((x))) = 4;

Likewise UpdateExpression has a similar bug:

export default class UpdateExpression extends Node {
        bind ( scope ) {
                let subject = this.argument;
                while ( this.argument.type === 'ParenthesizedExpression' ) subject = subject.expression;

                this.subject = subject;

for the input:

((x))++;

The while loop conditions this.type.left and this.argument.type are invariant, and the use of this.subject appear to be typos.

[kzc]

the use of this.subject appear to be typos

I was mistaken on that point.

Here's what I think is the fix for AssignmentExpression and UpdateExpression:

--- a/src/ast/nodes/AssignmentExpression.js
+++ b/src/ast/nodes/AssignmentExpression.js
@@ -6,7 +6,7 @@ import { NUMBER, STRING } from '../values.js';
 export default class AssignmentExpression extends Node {
        bind ( scope ) {
                let subject = this.left;
-               while ( this.left.type === 'ParenthesizedExpression' ) subject = subject.expression;
+               while ( subject.type === 'ParenthesizedExpression' ) subject = subject.expression;

                this.subject = subject;
                disallowIllegalReassignment( scope, subject );

--- a/src/ast/nodes/UpdateExpression.js
+++ b/src/ast/nodes/UpdateExpression.js
@@ -6,7 +6,7 @@ import { NUMBER } from '../values.js';
 export default class UpdateExpression extends Node {
        bind ( scope ) {
                let subject = this.argument;
-               while ( this.argument.type === 'ParenthesizedExpression' ) subject = subject.expression;
+               while ( subject.type === 'ParenthesizedExpression' ) subject = subject.expression;

                this.subject = subject;
                disallowIllegalReassignment( scope, this.argument );

[kzc]

@Rich-Harris since ParenthesizedExpression AST nodes are generally problematic, might it be better for rollup either remove them all at a common lower level except in the case of export where it is apparently needed?

[kzc]

Removing ParenthesizedExpression AST nodes and replacing them with a parens property (or some other name) in the child expression node would be less intrusive to program logic.

[kzc]

Fix in #1138