Replace unused pure call expressions with their side-effectful arguments

[Andarist]

I've noticed this when testing treeshakeability of a some library that this input code~:

const create = () => {
  throw new Error('test')
}

/* #__PURE__ */ create({
  foo: create({}),
});

pretty much stays in the output, while when using Terser the outer call (the one annotated with PURE) gets eliminated. The inner call makes the difference here, because if we annotate it the whole thing just vanishes in both cases (as expected).

Expected Behavior

Matching behavior to Terser

Actual Behavior

Different behavior from Terser

Repro

REPL repro

---

Note from maintainer (@lukastaegert) :
The consensus on this issue is that even though the current behaviour is acceptable, it would be nice to replace call expressions with just their side-effectful arguments in certain cases. If someone wants to work towards a PR to improve this, see #3144 (comment) for implementation suggestions.

[lukastaegert]

For reference, the terser output is

const create=()=>{throw new Error(test)};create();

I do not see this as a Terser incompatibility but Rollup being conservative due to its limitations as the code is still equivalent in its side-effects. Doing this requires completely replacing the call expression with its argument side-effects. This will unfortunately be quite a bit of work as it would mean

• extending the inclusion logic to signal we are only including for side-effects and do not need the return value of an expression
• changing the inclusion logic of CallExpressions to handle this case differently by not including the callee and side-effect-less arguments
• changing the rendering logic of CallExpressions to replace the call expression with a sequence of side-effectul arguments

If you also want the simplification of the ObjectExpression, it gets even more complicated, but maybe just fixing the first part would be a big thing.

[lukastaegert]

There are many other transformations that Terser does that Rollup does not match.

[Andarist]

There are many other transformations that Terser does that Rollup does not match.

Yeah, sure - that's totally fine. Different use cases. I would only expect that this particular thing would match Terser behavior as otherwise using PURE annotations is less predictable for package authors. Arguably - it shouldn't matter in terms of runtime logic, the only downside is that some code doesn't get removed.

as the code is still equivalent in its side-effects.

Not quite true, outer call and inner call can be different functions, this is just a dummy example. And then produced side effects can be different for Rollup and Terser output - also in practice this shouldn't matter because even if there are some side effects they should be relevant if the resulting value is being used, but as it's not the fact that side effect has happened or not shouldn't matter for the program.

I'm not sure if I understand at glance all 3 works that would have to be done to make this happen (especially first two - could you elaborate about those?) - but the last:

changing the rendering logic of CallExpressions to replace the call expression with a sequence of side-effectul arguments
seems like everything that I would imagine for this to work. I treat PURE as a special thing that allows us to skip further, often more complex, analysis and just drop the CallExpression in question (but leave its arguments).

[lukastaegert]

And then produced side effects can be different for Rollup and Terser output

To my understanding only if the pure-annotated function had side-effects. If that is the case, it is definitely the fault of whoever used the annotation wrongly.

As for the rest, little time at the moment, maybe tomorrow.

[Andarist]

To my understanding only if the pure-annotated function had side-effects. If that is the case, it is definitely the fault of whoever used the annotation wrongly.

Yes, you are right. I just wanted to point out that they can differ in such case - not that it's a serious problem per se (for this having any impact on the runtime it would have to be wrongly annotated/coded/designed)

[lukastaegert]

Sure, I just wanted to make clear that the contract Rollup is fulfilling is not "tree-shake everything Terser would" but "never remove something if terser would not remove it; if you keep it, try to keep the annotation for someone else to pick it up".

[Andarist]

The latter contract makes sense as well if that is what your goal is here 👍wasn't actually sure what are exact semantics for PURE in Rollup and had only Terser to verify against "expected" results. If you don't intend to match its behavior 1 to 1 then this can be closed I guess.

[lukastaegert]

Actually it would be an interesting enhancement. The necessary changes are a little more involved but if done right, it could open the doors for many more "just include the side-effects" transformations. As I said, I hope to formulate a few less rough ideas how we could get started tomorrow. There are several other situations where such a handling would have advantages.

[lukastaegert]

Ok, here is a little more elaborate sketch how this could be tackled. First some general intro:

At the moment, tree-shaking works by attaching a single flag to AST nodes: included: true/false. Initially, nothing is included, then we include all public exports and then in several passes we included everything that may have side-effects or declarations that are needed for other included statements.

Then in the generate phase, all included statements are rendered while the rest is omitted. For statements, this "omission" is handled by the BlockStatement and Program AST nodes, which actively remove everything from the rendered result that does not have the included flag.

This is however not limited to statements but also extends to other places that allow for conditional code, e.g. conditional expressions test ? consequent : alternate. When test can be resolved to a unique boolean value, there is no need to include the unneeded branch. So if test == true and test itself has no side-effects, then Rollup will only mark the ConditionalExpression itself and consequent as included, but not test or alternate. Then the ConditionalExpression has a special logic in its render method that detects that case and replaces the entire expression with its consequent branch. This is also how it is handled for other cases of conditional code execution such as if-statements, logical expressions etc.

So how could we extend this to call expressions? From a rendering perspective, it would not be too difficult to emulate conditional expressions. The logic could detect if the callee property is included as well and if that is not the case, switch to a special rendering logic that just renders the side-effectful arguments.

The difficult question, though, is how to get there. To understand why, note that inclusion does not happen by just switching the included flag but by calling include on an AST node. This is to make sure the node can take care of including its necessary children. For call expressions, however, the call expression cannot just do the same as the logical expression to decide whether or not its callee needs to be included because if the return value is used, then the callee always needs to be included.

One way to signal this is to extend the arguments of the include() method. At the moment it has a property includeChildrenRecursively which is meant to override the side-effect detection and

• include all children if the value is true
• include all children and trigger a special inclusion logic on called parameters when they are called from within a try statement

This argument could be replaced by an inclusionMode parameter that supports yet another inclusion mode: "include ignoring value". This could be used everywhere where the return value of an expression is unused, e.g. in expression statements. Nodes that check for this value could then do a partial inclusion and in their rendering replace themselves with a lot of freedom as they know their value does not matter.

In case of call expressions, they could just check for

this.callee.shouldBeIncluded() ||
this.callee.hasEffectsWhenCalledAtPath(EMPTY_PATH, this.callOptions)

and if this is false, just check for each argument if it shouldBeIncluded and just include those. Then this could render their single side-effectful argument or a sequence expression of several such arguments (note that in many situations, this will require additional parentheses). Note: shouldBeIncluded is similar to hasEffects but usually short-circuits when something is already included to include it again, and also has some special logic to always include or ignore certain things.

[Andarist]

Thanks for the writeup - definitely useful insight into how Rollup currently works. I would love diving into playing with this - but I'm afraid I have different OSS tasks with higher priority for myself right now. If no one picks it up I might come back here to start working on this, but that might take some time.

[lukastaegert]

Sure, the idea was not necessarily for you to pick it up but also as a general move to add more information and starting points to issues for potential new contributors. I know you are doing great work elsewhere!