Implement object shape and function return value tracking

[lukastaegert]

Latest update: This PR is ready to be merged

Resolves #1263
Resolves #1284
Resolves #1595

This is my third big rollup refactoring which will finally enable rollup to analyze both object shapes and function return values in its tree-shaking algorithm.

The most visible improvement to most will be that this resolves #1595 i.e. babel-helpers are now removed properly again. Moreover, ES5 classes can now be tree-shaken as well i.e. assignments to a function's prototype will no longer be counted as a side-effect unless the prototype is shared with another function that is included (see the test "side-effects-prototype-assignments"). Thus it also resolves #1284 and therefore https://gitlab.com/Rich-Harris/buble/issues/181. This also includes some fine-tuned logic to not overlook side-effects with computed members.

Moreover, it also includes support for side-effects in getters and setters as long as those are declared via object literals. Access to global getters and setters should also be retained properly now. Unfortunately, getters and setters with side-effects declared via Object.defineProperty are not yet recognized properly (but should still work in most cases). I hope to solve this properly in the future with an overhaul of the global variable handling.

Furthermore, function return values are now forwarded properly and even currying is now possible! I.e. unless y is used somewhere else, this will be removed now:

const x = a => b => ({
  foo: () => ({a, b})
})

const y = x()().foo()

This has dramatic effects when used in conjunction with the new ES6 version of ramda. If you create an ES6 bundle from

import 'ramda'

then it has

• 9321 lines with rollup 0.50.0 and tree-shaking switched off
• 9004 lines with rollup 0.50.0 + tree-shaking
• 3186 lines with this PR!

Of course, ramda is ideally suited for tree-shaking as nearly all functions are side-effect free i.e. they do not call or mutate their arguments. Most of the remaining lines are the result of called arguments.

Call arguments and function parameters are not associated by this PR as first attempts in this direction lead to terrible performance problems (basically, build time started to grow exponentially with the number of function calls). That is, rollup always assumes that function parameters have unknown values. To guard against the nasty case where a function mutates its parameter, we now assume that calling a or assigning to a member of an included variable is a potential side-effect. For instance in the following example, rollup is not able to associate the call to bar.addEffect with a call to Bar.prototype.addEffect. Nevertheless, everything important is retained by the algorithm:

• As bar.addEffect( foo ) cannot be associated with a function, the line is kept as it could potentially have side-effects
• Therefore, both foo and bar are kept
• Hence, Bar is included to be able to create bar
• Thus the prototype mutation of the included Bar is included as well, i.e. the function addEffect
• As the algorithm considers obj to be unknown, all of its mutations are retained
• foo.noEffect() is kept because it is a call to a member of an included variable

const foo = { noEffect: () => {} };
const exported = {};

function Bar () {}

Bar.prototype.addEffect = function ( obj ) {
	obj.noEffect = () => {
		exported.mutation = 'present';
	};
};

const bar = new Bar();
bar.addEffect( foo );
foo.noEffect();

export default exported;

How does it work?

Every variable on the scope now no longer only stores what expressions are assigned to it but also all assignments to nested objects together with the path of the assignment. I.e. if we write

const foo = {bar: {baz: 3}};
foo.bar = 'Hello';

rollup now knows that foo.bar can be either {baz: 3} or Hello and if we do further mutations with or calls to foo.bar, both of the possible values will be checked for side-effects. Hence this will be removed:

const foo = {bar: {baz: () => {}}, quux: {}};
foo.bar.baz();
foo.quux.blu = 'not a problem';

but this will all be kept:

const foo = {bar: {baz: () => {}}, quux: {}};
foo.bar.baz = () => console.log('effect')
foo.bar.baz();
foo.quux.blu.bli = 'this is too deep';

Note that delete is internally implemented as an assignment of undefined and is thus respected. Rollup can also handle more complex re-assignments and in the following, the first part will be removed while the second part will be kept:

const foo1 = {bar: {baz: () => {}}};
const bar1 = foo1.bar;
bar1.baz();

const foo2 = {bar: {baz: () => {}}};
const bar2 = foo2.bar;
bar2.baz = () => console.log('effect');
foo2.bar.baz();

This will even work across imports and exports, so the test mutations-in-imports

will remove all `z`s while keeping the rest

const x = { a: { b: () => {} } };
const y = { a: x.a };

const x$1 = { a: { b: () => {} } };
const y$1 = { a: x$1.a };

const x$2 = { a: { b: () => {} } };
const y$2 = { a: x$2.a };

y.a.b = () => console.log( 'effect' );
x.a.b();
y$1.a.b = () => console.log( 'effect' );
x$1.a.b();
y$2.a.b = () => console.log( 'effect' );
x$2.a.b();

In order to track function return values, return statements now register their argument with their surrounding function scope and additionally, an unknown return value is added if the last statement of a function is not a return statement. The new AST node method someReturnExpressionWhenCalledAtPath then enables checks that iterate over all possible return values of a function.

Rollup now also features a new type of AST node effect, hasEffectsWhenAccessedAtPath(). At the same time, the make-shift hasEffectsAsExpressionStatement() could finally be retired. hasEffectsWhenAccessedAtPath() is now called in the default implementation of Node.hasEffects() and is true if we access a getter with side-effects or a nested node of a missing node, e.g. in

const foo = {
	get noEffect () {},
	get effect () {
		console.log('effect');
	}
};
const removed1 = foo.bar;
const removed2 = foo.noEffect;
const retained1 = foo.bar.baz;
const retained2 = foo.effect;

the last two lines will be kept while the previous two lines will be removed. This should preserve functionality in case someone abuses try-catch around property access to determine if a member exist.

Some disclaimers

• This is not SSA. If you write

  let x;
  x = {};
  x.y = 1;

  then this will NOT be removed because rollup only knows that x is either {} or undefined–which is the value after the first line–and will check both values in the second line to determine if this assignment is valid. Of course, undefined.x = 1 would have the side-effect of throwing an error and everything will be kept. If this were SSA, rollup would know that the value undefined has become irrelevant after the second line. It is quite possible we might go there at some point but there are still many open questions and difficult problems especially related to asynchronous code so I think the current solution should be good enough for many use cases.
• Rollup has a hard limit on the length of object paths. As it turned out, the complex algorithm could be easily sent into a death spiral by writing something like

  let x = {value: {value: {}}};
  while (x.value) {
    x = x.value;
  }
  x.foo = 1;

  because rollup does not really understand the loop so to determine if the last line has an effect, rollup begins checking x, x.value, x.value.value, x.value.value.value and so on. After long consideration, my solution was to implement a hard limit on the path length (currently 8). If you access something that is nested more deeply, this will be considered a side-effect.

Releasing

This PR should be ready for release. Performance is a little slower than the previous version but this is something we might still be able to improve upon in the long run. I also checked that Rollup is able to bundle itself successfully.

[kzc]

@lukastaegert - Impressive work!

Any consideration for dropping the unused code seen in C2 below?

$ cat statics.js 
function C0() {}
C0.prototype.run = function() {
    console.log("C0.prototype.run");
};
C0.static = 1;

var C1 = function () {
    function C1() {}
    C1.prototype.run = function() {
        console.log("C1.prototype.run");
    };
    C1.static = 1;
    return C1;
}();

var C2 = function () {
    function C2() {}
    C2.prototype.run = function() {
        console.log("C2.prototype.run");
    };
    return C2;
}();
C2.static = 1;

$ bin/rollup statics.js -f es --silent
var C2 = function () {
    function C2() {}
    C2.prototype.run = function() {
        console.log("C2.prototype.run");
    };
    return C2;
}();
C2.static = 1;

If the C2 function were hoisted it would resemble C0 which you already handle.

[kzc]

@lukastaegert Have you tried bootstrapping Rollup with this PR?

I'm seeing some problems with this.nested.property reads and writes.

$ bin/rollup dist/rollup.js -f es --silent | diff -u dist/rollup.js -

--- dist/rollup.js	2017-10-06 12:46:35.000000000 -0400
+++ -	2017-10-06 13:11:33.000000000 -0400
@@ -8,17 +8,15 @@
 	Released under the MIT License.
 */
 
-'use strict';
-
 Object.defineProperty(exports, '__esModule', { value: true });
 
-function _interopDefault (ex) { return (ex && (typeof ex === 'object') && 'default' in ex) ? ex['default'] : ex; }
+function _interopDefault$1 (ex) { return (ex && (typeof ex === 'object') && 'default' in ex) ? ex['default'] : ex; }
 
 var path = require('path');
-var path__default = _interopDefault(path);
+var path__default = _interopDefault$1(path);
 var fs = require('fs');
-var EventEmitter = _interopDefault(require('events'));
-var module$1 = _interopDefault(require('module'));
+var EventEmitter = _interopDefault$1(require('events'));
+var module$1 = _interopDefault$1(require('module'));
 
 const DEBUG = false;
 const map = new Map;
@@ -398,11 +396,9 @@
 	}
 }
 
-var charToInteger$1 = {};
 var integerToChar$1 = {};
 
 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/='.split( '' ).forEach( function ( char, i ) {
-	charToInteger$1[ char ] = i;
 	integerToChar$1[ i ] = char;
 });
 
@@ -840,23 +836,6 @@
 	};
 }
 
-var Stats = function Stats () {
-	Object.defineProperties( this, {
-		startTimes: { value: {} }
-	});
-};
-
-Stats.prototype.time = function time ( label ) {
-	this.startTimes[ label ] = process.hrtime();
-};
-
-Stats.prototype.timeEnd = function timeEnd ( label ) {
-	var elapsed = process.hrtime( this.startTimes[ label ] );
-
-	if ( !this[ label ] ) { this[ label ] = 0; }
-	this[ label ] += elapsed[0] * 1e3 + elapsed[1] * 1e-6;
-};
-
 var warned = {
 	insertLeft: false,
 	insertRight: false,
@@ -890,7 +869,7 @@
 
 MagicString$1.prototype = {
 	addSourcemapLocation: function addSourcemapLocation ( char ) {
-		this.sourcemapLocations[ char ] = true;
+		
 	},
 
 	append: function append ( content ) {
@@ -1146,7 +1125,7 @@
 		if ( !first.previous ) { this.firstChunk = last.next; }
 		if ( !last.next ) {
 			this.lastChunk = first.previous;
-			this.lastChunk.next = null;
+			
 		}
 
 		first.previous = newLeft;
@@ -1185,7 +1164,7 @@
 
 		if ( storeName ) {
 			var original = this.original.slice( start, end );
-			this.storedNames[ original ] = true;
+			
 		}
 
 		var first = this.byStart[ start ];
@@ -1373,8 +1352,6 @@
 
 		var newChunk = chunk.split( index );
 
-		this.byEnd[ index ] = chunk;
-		this.byStart[ index ] = newChunk;
 		this.byEnd[ newChunk.end ] = newChunk;
 
 		if ( chunk === this.lastChunk ) { this.lastChunk = newChunk; }
@@ -4446,11 +4423,11 @@
 };
 
 pp$5.declareVarName = function(name) {
-  this.scopeStack[this.scopeStack.length - 1].var[name] = true;
+  
 };
 
 pp$5.declareLexicalName = function(name) {
-  this.scopeStack[this.scopeStack.length - 1].lexical[name] = true;
+  
 };
 
 var Node = function Node(parser, pos, loc) {
@@ -4624,13 +4601,6 @@
 };
 
 types.star.updateContext = function(prevType) {
-  if (prevType == types._function) {
-    var index = this.context.length - 1;
-    if (this.context[index] === types$1.f_expr)
-      { this.context[index] = types$1.f_expr_gen; }
-    else
-      { this.context[index] = types$1.f_gen; }
-  }
   this.exprAllowed = true;
 };
 
@@ -5670,7 +5640,7 @@
 
 (function (global, factory) {
   module.exports = factory();
-}(commonjsGlobal, function () { 'use strict';var SLICE$0 = Array.prototype.slice;
+}(commonjsGlobal, function () { var SLICE$0 = Array.prototype.slice;
 
   function createClass(ctor, superClass) {
     if (superClass) {
@@ -7366,7 +7336,6 @@
 
       if (keyMatch) {
         if (ownerID && ownerID === this.ownerID) {
-          this.entry[1] = value;
           return this;
         }
         return new ValueNode(ownerID, this.keyHash, [key, value]);
@@ -7530,7 +7499,7 @@
     var newNode;
     var nodes = idx1 === idx2 ?
       [mergeIntoNode(node, ownerID, shift + SHIFT, keyHash, entry)] :
-      ((newNode = new ValueNode(ownerID, keyHash, entry)), idx1 < idx2 ? [node, newNode] : [newNode, node]);
+      (newNode = new ValueNode(ownerID, keyHash, entry), idx1 < idx2 ? [node, newNode] : [newNode, node]);
 
     return new BitmapIndexedNode(ownerID, (1 << idx1) | (1 << idx2), nodes);
   }
@@ -8491,8 +8460,7 @@
       return this._iter.__iterate(
         this._useKeys ?
           function(v, k)  {return fn(v, k, this$0)} :
-          ((ii = reverse ? resolveSize(this) : 0),
-            function(v ) {return fn(v, reverse ? --ii : ii++, this$0)}),
+          (ii = reverse ? resolveSize(this) : 0, function(v ) {return fn(v, reverse ? --ii : ii++, this$0)}),
         reverse
       );
     };
@@ -13855,10 +13823,7 @@
 				}, node.start );
 			}
 
-			this.exports.default = {
-				localName: 'default',
-				identifier
-			};
+			
 		}
 
 		// export var { foo, bar } = ...
@@ -13871,13 +13836,13 @@
 			if ( declaration.type === 'VariableDeclaration' ) {
 				declaration.declarations.forEach( decl => {
 						extractNames( decl.id ).forEach( localName => {
-						this.exports[ localName ] = { localName };
+						
 					} );
 				} );
 			} else {
 				// export function foo () {}
 				const localName = declaration.id.name;
-				this.exports[ localName ] = { localName };
+				
 			}
 		}
 
@@ -13894,7 +13859,7 @@
 					}, specifier.start );
 				}
 
-				this.exports[ exportedName ] = { localName };
+				
 			} );
 		}
 	}
@@ -13918,7 +13883,7 @@
 			const isNamespace = specifier.type === 'ImportNamespaceSpecifier';
 
 			const name = isDefault ? 'default' : isNamespace ? '*' : specifier.imported.name;
-			this.imports[ localName ] = { source, specifier, name, module: null };
+			
 		} );
 	}
 
@@ -14199,7 +14164,6 @@
 			return false;
 		}
 		this.included = true;
-		this.module.used = true;
 		return true;
 	}
 
@@ -14226,9 +14190,6 @@
 	}
 
 	suggestName ( name ) {
-		if ( !this.nameSuggestions[ name ] ) { this.nameSuggestions[ name ] = 0; }
-		this.nameSuggestions[ name ] += 1;
-
 		if ( this.nameSuggestions[ name ] > this.mostCommonSuggestion ) {
 			this.mostCommonSuggestion = this.nameSuggestions[ name ];
 			this.name = name;
@@ -14627,7 +14588,7 @@
 	let acc = 'this';
 
 	return parts
-		.map( part => ( acc += property( part ), `${acc} = ${acc} || {};` ) )
+		.map( part => ( acc += property( part ), `${acc} = ${acc} || {};`) )
 		.join( '\n' ) + '\n';
 }
 
@@ -14719,7 +14680,7 @@
 
 	let acc = 'global';
 	return parts
-		.map( part => ( acc += property( part ), `${acc} = ${acc} || {}` ) )
+		.map( part => ( acc += property( part ), `${acc} = ${acc} || {}`) )
 		.concat( `${acc}${last}` )
 		.join( ', ' );
 }
@@ -14729,7 +14690,7 @@
 
 	let acc = 'global';
 	return parts
-		.map( part => ( acc += property( part ), acc ) )
+		.map( part => ( acc += property( part ), acc) )
 		.join( ` && ` );
 }
 
@@ -18305,17 +18266,6 @@
  * Licensed under the MIT License.
  */
 
-'use strict';
-
-
-
-
-
-
-/**
- * Expose `cache`
- */
-
 var cache = module.exports.cache = {};
 
 /**
@@ -18548,8 +18498,6 @@
 regexCache_1.basic = basic_1;
 
 var utils_1 = createCommonjsModule(function (module) {
-'use strict';
-
 var win32 = process && process.platform === 'win32';
 
 
@@ -18768,15 +18716,6 @@
 var chars_1 = chars;
 
 var glob = createCommonjsModule(function (module) {
-'use strict';
-
-
-
-
-/**
- * Expose `Glob`
- */
-
 var Glob = module.exports = function Glob(pattern, options) {
   if (!(this instanceof Glob)) {
     return new Glob(pattern, options);
@@ -18795,8 +18734,7 @@
 Glob.prototype.init = function(pattern) {
   this.orig = pattern;
   this.negated = this.isNegated();
-  this.options.track = this.options.track || false;
-  this.options.makeRe = true;
+  
 };
 
 /**
@@ -18840,7 +18778,7 @@
 
     // if imbalanced, don't optimize the pattern
     if (a && b && (a.length !== b.length)) {
-      this.options.makeRe = false;
+      
     }
 
     // expand brace patterns and join the resulting array
@@ -20083,4 +20021,3 @@
 exports.rollup = rollup;
 exports.watch = watch$1;
 exports.VERSION = version$1;
-//# sourceMappingURL=rollup.js.map

[lukastaegert]

Hi @kzc, thanks a lot for catching this at such an early stage! Looking at the issue you pointed out lead me to reconsider my whole approach to handling calls especially with regard to "this". Previously I tried to "bind" all calls to know where I had unsafe "this" values–the idea was that I could use this in the future to bind call parameters as well.
Now I became painfully aware that this is just a futile attempt–with JavaScript, you never gonna catch them all. Now the new logic is drastically easier (and feels much better):

• If a function is included, all "this" mutations are always retained
• When a function is called without "new", all "this" mutations are considered side-effects
• When a function is called with "new", all "this" mutations that would be safe for an empty object are considered safe

This should catch all "this" related problems. In fact, if you now let rollup bundle itself and compare with the previous version, you will find a little more unused code could be removed.

[lukastaegert]

As for your first comment, for that we will need function return value tracking. In the current iteration (as well as this pull request), function return values are considered to be "unknown" nodes and treated as such.
In fact, I plan for my next project to tackle exactly this. Hopefully, we will then be able to also tree-shake curried functions, #1263 is a fairly old issue related to this. Then, who knows what is possible.

[lukastaegert]

Thanks to Andarist's excellent work you can now directly install from the PR branch to try this out:

npm install lukastaegert/rollup#simple-object-shape-tracking

It requires npm >= 4 though.

[kzc]

@lukastaegert Any potential issues with handling super which is similar to this in some ways?

[lukastaegert]

I do not think so. In fact in the current iteration, super calls and accesses to members of super are always considered to be a side-effect. Not ideal but certainly safe.

[Andarist]

It requires npm >= 4 though.

I believe it requires npm >= 5. npm 4 recognizes prepare and prepublishOnly scripts, but starting from version 5 prepare is run in git dependencies.

I've checked out this PR on redux-saga - redux-saga/redux-saga#1202

• AsyncGenerator is still, probably because it is wrapped in IIFE
• some orphanated babelHelpers reference is leftover here

[lukastaegert]

IIFEs are no problem for the tree-shaking algorithm as long as their return value is not used. Maybe I can take a look tonight. Return values and possibly parameters will be the next big project.

[kzc]

@lukastaegert I think this PR is okay to merge into master assuming Rollup can bootstrap itself with the new code. It's hard to verify it in the best case due to the size of the change. Let the internet debug it - most have versions of Rollup locked down for their projects anyway - or should.

[Andarist]

@lukastaegert you like to drop commit 💣💣💣 , don't u? 😄

[lukastaegert]

Well, now that this PR has been sitting here for some time, I discovered that it actually contained an important oversight, basically

const foo = { mightBeExported: {} };
const exported = {};

function assignExported ( obj ) {
	obj.mightBeExported = exported;
};

assignExported( foo );
foo.mightBeExported.bar = 'present';

export default exported;

would not add bar: 'present' to exported because the previous algorithm did not know how to associate parameters. So I thought I extend the existing PR with my next one should now be able to handle parameters and return values.

I didn't get to update the PR description yet as I just discovered a remaining bug with chaining promises ('Maximum call stack size exceeded' is my biggest enemy).

But you are right, maybe I should start squashing commits :)
Btw. you should try out the new version, it should be much more capable now!

[Andarist]

I didn't mean it is a bad thing - I even might prefer smaller commits so the iterative work can be observed etc. I meant that solely as a good thing. Excellent work 👏

[jonataswalker]

Wow, what a great work!! Many thanks for your (precious) time @lukastaegert.

[kzc]

maybe I should start squashing commits

@lukastaegert Please don't squash your commits! It's a useful way to understand the code progression while you're reengineering the entire program.

It'd be fantastic if the internal workings documentation you provided in the top post made its way into the Rollup wiki in some form. For bonus points adding a glossary of terms for things like "paths" and "binding" in the context of Rollup would be helpful to other potential contributors. A 40,000 foot design overview is often more helpful than commenting methods. In particular what the various passes do.

I have mixed opinions on beta releases - they're rarely used and tested. Since every release is versioned anyway there's not much risk in frequently pushing out the latest and greatest. Bugs and blind alleys are caught much quicker. In the worst case scenario, users can lock down a previous working version until the dust settles.

Anyway, it's all good. Don't want to stifle your creativity - feel free to ignore.

[lukastaegert]

Ok, so I think everything is working properly now, enjoy! I also updated the description now to reflect the new features. @kzc I like your ideas and will take a look at the wiki once enough dust has settled.

[Andarist]

Unfortunately I cant confirm that babel helpers issue is resolved now - https://github.com/redux-saga/redux-saga/pull/1202/files

[lukastaegert]

@Andarist Thanks for checking this, damn. I think need to add async generator as an actual test, not just simplified versions of it. I had a quick check, the problem is this:

if (typeof Symbol === "function" && Symbol.asyncIterator) {
  AsyncGenerator.prototype[Symbol.asyncIterator] = function () {
    return this;
  };
}

more specifically, it is the access to Symbol.asyncIterator. Rollup does not know if Symbol.asyncIterator is a getter and accessing it might therefore have a side-effect (Yes, that is where we are now! Support for unknown getters!). I fear the only good solution is to add the last big piece to the rollup puzzle: A new handling for globals that knows much more about things like Symbol (currently we only know a few pure functions).

I think I will try the following:

1. Add a quick fix with special handling for Symbol to solve the common babel helpers issue so that we can finally release this
2. Prepare a new PR that improves the handling of known globals

I will not work on this today as otherwise may girlfriend might kill me 🙀(and rightfully so) but you can expect an update early next week. Until then I will change the PR description to make note of this.

[Andarist]

I will not work on this today as otherwise may girlfriend might kill me 🙀(and rightfully so) but you can expect an update early next week. Until then I will change the PR description to make note of this.

No worries, blacklisting asyncGenerator in my configs for the time being is fine. I know how OSS is time-consuming and I do not expect any work to be done really from anybody - ofc I'm happy there are some people working hard and pushing things forward when they want ;) Spend time with your girl, don't overwork and always get a decent sleep!

[kzc]

Unfortunately I cant confirm that babel helpers issue is resolved now

It's not a Rollup regression, per se, it's just that previous versions of Rollup happened to drop the code without sufficiently analyzing potential side effects. That's not a fault of this PR - I think it's good to go.

@Andarist Your work on babel@7 will mark such unused generators and async functions transformed into ES5 IIFEs as /*@__PURE__*/ to be dropped by uglify, will it not?

[Andarist]

It's not a Rollup regression, per se, it's just that previous versions of Rollup happened to drop the code without sufficiently analyzing potential side effects. That's not a fault of this PR - I think it's good to go.

Oh, yeah, I didnt mean this PR should be put on hold or anything, just pointed out that recent advances didnt "fix" babel helpers issue.

@Andarist Your work on babel@7 will mark such unused generators and async functions transformed into ES5 IIFEs as /@PURE/ to be dropped by uglify, will it not?

Not yet, I mean - adding those annotations is quite trivial. Im wondering lately if they should be on by default - in places where babel has started adding them (because of me) there is no validation included about the path being actually pure, so potentially it can lead to some side effects being dropped. Personally Im OK with that, if somebody writes a code that can be break because of it, then such code should be refactored. However I think babel can't be opinionated about this and some simple validation should be introduced before adding them.

Also while transpiled async generators might get their /*@__PURE__*/ annotations, the helper itself didnt receive it so far too.

[lukastaegert]

Babel helpers are correctly removed now! I also added the complete helpers as a test to guard against future breakage.

Previously, white-listed pure functions were the only globals that could be accessed without side-effects. I extended this by adding the (in my opinion correct) assumption that accessing members of pure functions (e.g. Symbol.iterator) as well as members of their prototypes (e.g. Array.prototype.join) can always be accessed safely.

Thus I again think this PR can be merged and I updated the description accordingly (+ some more notes how return values are handled).

[kentcdodds]

I just want to say this is clearly an amazing effort. Thank you so much for all the work you put into resolving these issues! 🎉

[michael-ciniawsky]

Yup this is really some astonishing work done here 💯 , thx ❤️

[hiendv]

@lukastaegert Thanks for the PR. Can you please explain more about the below differences when using destructuring assignment?
Repl 1
Repl 2

[lukastaegert]

Sure. There are actually two reasons here why tree-shaking does not happen for Y in the second case:

• Destructuring does not yet properly assign references but just marks the destructured values as "unknown". We hope to resolve this soon as this is a necessary prerequisite to be able to tree-shake object properties at some point.
• Function parameters are treated as "unknown" as well. There was a development build at some point that actually forwarded argument references but performance turned out to be terrible. This might come back in a limited form at some point but it will certainly take some time.

At the moment, the precise reason why Y is retained is the access to arg.value. As it is treated as "unknown", this could potentially throw an error if arg were to be undefined. You should be able to turn off this specific check by adding treeshake.propertyReadSideEffects: false to your rollup options.

Parameter destructuring as used in the first example, on the other hand, is less strictly checked which is why tree-shaking works here. We might change this at some point, though, in which case you would need treeshake.propertyReadSideEffects: false for both examples to work.

[hiendv]

Thanks @lukastaegert. So basically the current behavior for the destructuring assignment may change in the future, right? So using treeshake.propertyReadSideEffects: false is recommended to my problem.

[lukastaegert]

Definitely. I would actually recommend it to nearly everyone as it is most of the time a safe improvement. The reason it is not a default is that it breaks some niche programming patterns like

try {
 x.y
} catch(e) {
  doSomethingWhenThisAccessIsForbidden();
}

to check for existence of properties and of course getters with side-effects, which is also kind of an anti-pattern. Still, the goal is to make your example work without this flag at some point as well, but this will take time.

Update: Needed to change the example a little as it is really hard to make something that would actually fail.