Last case clauses should be tree-shaken when empty

[stephanedr]

• Rollup Version: 1.24.0
• Operating System (or Browser): Linux
• Node Version: 10.16.3

How Do We Reproduce?

switch (a) {
    case 0:
        console.log();
        break;
    case 1:
    default:
}

Expected Behavior

switch (a) {
    case 0:
        console.log();
        break;
}

Actual Behavior

switch (a) {
    case 0:
        console.log();
        break;
    case 1:
    default:
}

[kzc]

Any break at the end of a switch can be dropped as well.

switch (a) {
  case 0:
    console.log();
}

Be aware that the case clause must be side effect free to be dropped:

$ echo 'switch (1){ case console.log("effect"), 2: }' | node
effect

@lukastaegert Slightly off topic... Are the internal ASTs themselves generally updated in such transforms or are just MagicString code string manipulations performed? If the latter at some point repeated application of various transforms would cease to work I would think.

[lukastaegert]

Any break at the end of a switch can be dropped as well

In general any break inside a switch where there are no further side-effects in any of the cases after it.

I think this would be a nice-to-have but will not provide any further tree-shaking benefits that would remove additional code. Note that a switch statements that does not contain side-effects will be removed entirely even if it contains break statements: https://rollupjs.org/repl/?version=1.24.0&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMnN3aXRjaCUyMChnbG9iYWxUaGlzLnVua25vd24pJTIwJTdCJTVDbiU1Q3RjYXNlJTIwMSUzQSU1Q24lNUN0JTVDdGNvbnN0JTIwdW51c2VkJTIwJTNEJTIwMSUzQiU1Q24lNUN0JTVDdGJyZWFrJTNCJTVDbiU1Q3QlNUN0Y29uc29sZS5sb2coJ2RlYWQlMjBjb2RlJyklM0IlNUNuJTVDdGRlZmF1bHQlM0ElNUNuJTVDdCU1Q3RicmVhayUzQiU1Q24lN0QlMjIlMkMlMjJpc0VudHJ5JTIyJTNBdHJ1ZSU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlMjJmb3JtYXQlMjIlM0ElMjJlc20lMjIlMkMlMjJuYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJhbWQlMjIlM0ElN0IlMjJpZCUyMiUzQSUyMiUyMiU3RCUyQyUyMmdsb2JhbHMlMjIlM0ElN0IlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

If someone is interested, I will leave this issue for someone to pick it up put I will probably not tackle it.

Are the internal ASTs themselves generally updated in such transforms or are just MagicString code string manipulations performed?

The AST is never modified as otherwise we would need to include an entire mechanic to regenerate the code from the AST, which would very much bloat Rollup and is not a goal. If done right, until now, MagicString was capable of dealing with the necessary transformations and it would have no issues with the transformations described here if they are done on the right level.

[lukastaegert]

On second though, the case expressions themselves could reference variables that might be tree-shaken. Maybe I will give it another though. Removing trailing break-statements, just as trailing continue statements or returns, will not provide any additional benefit, though.

[kzc]

I don't think it's generally safe to remove trailing continue statements or returns in a switch.

$ dist/bin/rollup -v
rollup v1.24.0

$ cat bug.js
for (var i = 0; i < 4; i++) {
    switch (i) {
      case 0:
      case 1:
        continue;
    }
    console.log(i);
}

$ cat bug.js | node
2
3

$ dist/bin/rollup bug.js -f es --silent | node
0
1
2
3

$ dist/bin/rollup bug.js -f es --silent
for (var i = 0; i < 4; i++) {
    console.log(i);
}

[lukastaegert]

Sorry, I was skipping topic here. continue would refer to loops, not switches. return would refer to the situation where the switch statement is also the last statement in a function. The problem is that these extra conditions are usually not that easy to check.

[kzc]

True enough. Should a new issue be opened for the bug above or will it be addressed in #3166?

[lukastaegert]

Ah I see. I will try to tackle it in #3166

[lukastaegert]

I added a fix

[kzc]

@lukastaegert Regarding ASTs and MagicString, you may find this interesting:

https://twitter.com/mikesherov/status/1184673919279751172

I have great respect for the work done with MagicString, but I do think Rollup could benefit from direct AST transformation as it becomes an optimizing compiler in its own right.