Partially removed default export produces invalid syntax

[hisland]

version=0.52.0

see the rollupjs.org/repl repro
or https://github.com/hisland/rollup-issue

[lukastaegert]

Thanks for the issue. I hope we find some time soon to get to the bottom of this. To anyone who wants to take a look at this: This is probably related to something happening in the rendering function for default exports. I guess in the presence of unused default exports with side-effects, we would still need to introduce a variable if the expression is not a statement.

[lukastaegert]

Actually, we might also consider if we could interpret a default export with an object literal the same way as a namespace export. If we slightly adjust rollup-plugin-commonjs, this might make tree-shaking possible in many more CJS scenarios.

[kzc]

we might also consider if we could interpret a default export with an object literal the same way as a namespace export.

I had wondered why they generated different code in rollup-plugin-commonjs. I just assumed they had different behaviors in ES6 with respect to side effects and/or live bindings or something.

[lukastaegert]

To just fix the issue at hand, adding parentheses should suffice:

{x: 1} // BlockStatement that contains illegal syntax
({ x: 1 }) // ExpressionStatement that contains an ObjectExpression

Then we could do the rest as a separate enhancement if we want to.

[hisland]

@lukastaegert see this repl, not work

[lukastaegert]

Which part isn't working?

[lukastaegert]

BTW, rollup does not yet know that [].map does not have side-effects. That is quite a different issue and has nothing to do with the issue at hand.

[eventualbuddha]

If rollup were to assume no side effects for [].map, I think it’d have to be an option (like assumeBuiltinsImmutable: true or something). Rollup can’t know that the map method wasn’t altered.

[kzc]

Rollup can’t know that the map method wasn’t altered.

Rollup has always had an implicit assumption that it's generating code for an ES6 platform with unmodified built-in classes.

[hisland]

in my opinion, i think all these will be removed by tree-shaking
repl

through your comment, i know there may have some side-effect

so this issue may focus on default export object-literal that may have side-effect
even if i wrap with (), not work, see

[lukastaegert]

Again: Why do you think this is not working? When I paste this into the console of my browser, it is recognised as valid syntax (I have changed it a bit so that we have an actual side-effect):

({
  map: console.log('effect') || []
})

it just doesn't look very valid but your JS engine will probably not care about looks 😜 (our parser agrees with me here as well). Or is this incompatible with older interpreters? I am also asking as I think the fix on our side should be to add the parentheses for you in the future if necessary.

As for the side-effect detection, the relevant issue here in my eyes is #673 so discussions about this should probably continue there. Just note that [].map(fn) is not as pure as it could be. In fact, if fn has a side-effect when called, then the map expression will have a side-effect as well. That is to say we will need to provide a table of instance method signatures and what potential side-effects an instance method has, e.g. which arguments are called or mutated. This is not unlike what I think should be done for builtin global variables so I would like to implement that first to hopefully find some synergies there. And then there are some even more pressing issues I want to fix before that so just to give you a perspective on priorities. Nevertheless, this has already been on my list for quite some time and I expect this to be resolved eventually.

[hisland]

ok, thank you very much! it's my fault.
I haven't write obj-literal that not assign to a variable

// I often write this
let someVar = {
  obj: 'literal'
}
// not just this
{
  obj: 'literal'
}
// or even this
({
  obj: 'literal'
})

create then drop the obj-literal, it's truly valid! 😀

[lukastaegert]

No problem, I understand it's confusing. No-one would write a literal without a variable. It is just in this specific situation where the literal definition might have a side-effect that rollup cannot simply remove it and we need a way to keep the expression without producing invalid syntax. Maybe in the future we can remove even more code so that just the side-effect itself will remain in the code.

[lukastaegert]

I have updated the issue name to make it easier for us to reference it.

[eventualbuddha]

Rollup has always had an implicit assumption that it's generating code for an ES6 platform with unmodified built-in classes.

You may be right, @kzc. Even if that’s the case, it would have to do type flow analysis to determine that something is an array except in trivial cases like this one. To my knowledge, rollup doesn’t do that (yet).

[lukastaegert]

As long as variables are not reassigned, we already have type flow analysis now, and it's working quite well 😜. If a variable or object key is reassigned, it is marked as "unknown" in the type. This is the magic done by the VariableReassignmentTracker. We have come quite a long way.

[eventualbuddha]

we already have type flow analysis now

I guess I need to pay more attention 😉 Fortunately, I finally got a Mac again after not having a personal computer capable of software development for most of this year. I'll have to take another look at the source code!

[lukastaegert]

This was only added in Rollup@0.51.0. It is actually somewhat hidden. Basically, a variable or object property is considered an "ArrayExpression" if the only thing assigned to it is something that is itself considered to be an "ArrayExpression" (including of course ArrayExpressions themselves). Thus, Rollup now strongly favours immutability.