Destructuring assignment prevents tree shaking of function with prototypes

[BenoitZugmeyer]

• Rollup Version: 2.33.3
• Operating System (or Browser): Firefox
• Node Version (if applicable): N/A
• Link to reproduction (IMPORTANT, read below): https://rollupjs.org/repl/?version=2.33.3&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmZ1bmN0aW9uJTIwRm9vJTIwKCklMjAlN0IlN0QlNUNuJTVDbkZvby5wcm90b3R5cGUuYmFyJTIwJTNEJTIwZnVuY3Rpb24lMjAoKSUyMCU3QiU3RCU1Q24lNUNuY29uc3QlMjBvYmolMjAlM0QlMjAlN0IlMjBGb28lMjQlM0ElMjBGb28lMjAlN0QlNUNuJTVDbmNvbnN0JTIwJTdCJTIwRm9vJTI0JTIwJTdEJTIwJTNEJTIwb2JqJTVDbiU1Q24lMkYlMkYlMjBFdmVyeXRoaW5nJTIwaXMlMjB0cmVlc2hha2VkJTIwaWYlMjB0aGUlMjBkZXN0cnVjdHVyaW5nJTIwZGVjbGFyYXRpb24lMjBpcyUyMHJlcGxhY2VkJTIwYnklM0ElNUNuJTJGJTJGY29uc3QlMjBGb28lMjQlMjAlM0QlMjBvYmouRm9vJTI0JTVDbiUyMiUyQyUyMmlzRW50cnklMjIlM0F0cnVlJTdEJTVEJTJDJTIyb3B0aW9ucyUyMiUzQSU3QiUyMmZvcm1hdCUyMiUzQSUyMmVzJTIyJTJDJTIybmFtZSUyMiUzQSUyMm15QnVuZGxlJTIyJTJDJTIyYW1kJTIyJTNBJTdCJTIyaWQlMjIlM0ElMjIlMjIlN0QlMkMlMjJnbG9iYWxzJTIyJTNBJTdCJTdEJTdEJTJDJTIyZXhhbXBsZSUyMiUzQW51bGwlN0Q=

In this configuration:

function Foo () {}

Foo.prototype.bar = function () {}

const obj = { Foo$: Foo }

const { Foo$ } = obj

I can't figure out why "Foo" is not removed after tree-shaking. Using const Foo$ = obj.Foo$ or removing Foo.prototype.bar declaration fixes the issue.

Expected Behavior

In the example above, everything should be removed after tree-shaking

Actual Behavior

The function Foo and Foo.prototype.bar declarations are kept after thee-shaking

[kzc]

Looks like another pass through rollup would eliminate the unused code in question. A similar issue was discussed in #3200.

[lukastaegert]

The reason here is two-fold:

• The logic to track reassignments through destructuring is basically non-existing, which means that having a variable on the left side of a destructuring assignment marks it as "untracked" in the logic
• Rollup does not yet track reassignments through the "flow" of the program code, i.e. the logic "forgets" that the destructuring assignment happens after the prototype assignment

So in short: The prototype assignment is retained because Rollup does not know if Foo has not been tampered with in a way that would make the assignment have a side-effect before that assignment happens.

[lukastaegert]

I long wanted to improve the logic for destructuring but unfortunately never got to it between more important issues and features.

[lukastaegert]

Here is a slight variation: https://rollupjs.org/repl/?version=2.33.3&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmNvbnN0JTIwRm9vJTIwJTNEJTIwJTdCJTIwYmFyJTNBJTIwJTdCJTdEJTIwJTdEJTNCJTVDbkZvby5iYXIuYmF6JTIwJTNEJTIwMSUzQiU1Q24lNUNuY29uc3QlMjAlN0IlMjBCYXIlMjAlN0QlMjAlM0QlMjAlN0IlMjBCYXIlM0ElMjBGb28lMjAlN0QlNUNuJTJGJTJGJTIwY29uc3QlMjBCYXIlMjAlM0QlMjBGb28lM0IlNUNuJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXMlMjIlMkMlMjJuYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJhbWQlMjIlM0ElN0IlMjJpZCUyMiUzQSUyMiUyMiU3RCUyQyUyMmdsb2JhbHMlMjIlM0ElN0IlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

[kzc]

Is there any fundamental obstacle to have rollup perform a configurable number of passes on the AST and its accompanying internal data structure to reduce the original example to nothing? Wouldn't it be a matter of reinitializing the tree back to the initial state for the retained nodes and running the algo again?

i.e., the API equivalent of this:

$ cat test.js
let x = {y: {}};
x.y.z = 1;
let {a} = {a: x};

$ cat test.js | rollup --silent
let x = {y: {}};
x.y.z = 1;

$ cat test.js | rollup --silent | rollup --silent
// empty

[lukastaegert]

No. The algorithm is already doing multiple passes on the existing tree. If we want to run it again, we would need to create some preliminary output and reparse it to start from scratch. But I do not think this is needed, especially considering performance would likely be terrible.
But for all issues brought forth where multiple passes would help, there was usually also a simple solution to improve the analysis without the need for multiple passes. Priority for these things is just low at the moment.

[kzc]

Using that terminology the default could remain one "run" and for those who wish to further optimize their output could opt in to multiple "runs" without the need to enhance rollup for each new unanticipated scenario.

Is there a way to avoid the reparse of the output between runs? I appreciate that rollup treats the AST as immutable, but there ought to be a way to determine which nodes are retained in the accompanying internal data structure and reset them to an initial state. Or is there something in the internal data structure that makes it inherently single use?

[kzc]

@lukastaegert I've been working on implementing specifying an optional number of runs (passes if you will) in rollup without the need to reparse the output between runs. Although it's a work in progress it works for the most part with minimal performance impact. But during debugging of this new feature I've run into an odd issue with hasEffects. I was under the impression that this method in AST classes was side effect free internally within the rollup code. Am I mistaken in this regard? For example, if you apply the following patch to latest master and run npm test I'd expect it to have no effect and it ought to pass all tests, but that's not the case and 6 failures result:

--- a/src/ast/nodes/UpdateExpression.ts
+++ b/src/ast/nodes/UpdateExpression.ts
@@ -26,6 +26,8 @@ export default class UpdateExpression extends NodeBase {
        }
 
        hasEffects(context: HasEffectsContext): boolean {
+               this.argument.hasEffects(context) ||
+               this.argument.hasEffectsWhenAssignedAtPath(EMPTY_PATH, context);
                return (
                        this.argument.hasEffects(context) ||
                        this.argument.hasEffectsWhenAssignedAtPath(EMPTY_PATH, context)

I just used UpdateExpression as an example. The same issue would result for any other AST class' hasEffects method if changed in a similar manner.

Do the AST hasEffects methods intentionally have internal side effects of some kind and if so what rules govern their use?