Else branch follows the wrong if statement

[TrySound]

• Rollup Version: 2.23.1
• Operating System (or Browser):
• Node Version (if applicable):
• Link to reproduction: https://rollupjs.org/repl/?version=2.23.1&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMiUyMCUyMCUyMCUyMCUyMCUyMGlmJTIwKG91dGVyQ29uZGl0aW9uKSU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlMjAlMjBpZiUyMChpbm5lckNvbmRpdGlvbiklMjAlN0IlNUNuJTVDdCU1Q3QlNUN0JTVDdCU1Q3RhY3Rpb24xKCklNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTdEJTIwZWxzZSU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlMjAlMjAlMjAlMjBmYWxzZSU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlMjAlMjAlMjAlMjAlMjAlMjAlM0YlMjB3YXJuaW5nKCklNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTNBJTIwdm9pZCUyMDAlM0IlNUNuJTIwJTIwJTIwJTIwJTIwJTIwZWxzZSUyMGFjdGlvbjIoKSUzQiU1Q24lMjIlMkMlMjJpc0VudHJ5JTIyJTNBdHJ1ZSU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlMjJmb3JtYXQlMjIlM0ElMjJlcyUyMiUyQyUyMm5hbWUlMjIlM0ElMjJteUJ1bmRsZSUyMiUyQyUyMmFtZCUyMiUzQSU3QiUyMmlkJTIyJTNBJTIyJTIyJTdEJTJDJTIyZ2xvYmFscyUyMiUzQSU3QiU3RCU3RCUyQyUyMmV4YW1wbGUlMjIlM0FudWxsJTdE

I got this bug after upgrade to history v5 (cc @mjackson). Pop state cannot be called in production because rollup breaks else branch here https://github.com/ReactTraining/history/blob/master/packages/history/index.ts#L427-L459

Expected Behavior

warning() should be fired according to outer condition not the inner one

Actual Behavior

Else branch follows the wrong if statement

[lukastaegert]

This is a tricky one. The best I could think of is:
Detect if by removing an else branch, another else branch can be caught (i.e. the rendered parent is an if statement and we are in the consequent branch and there exists an alternate branch) and if that is the case, do not remove the branch but replace it with an empty else {} branch.

[kzc]

Or just add braces to the parent IfStatement's consequent in that case.

Back in the day for buble braces were added to all blockless if/else/loops to avoid similar problems. It's not worth the hassle to special case every blockless scenario.

[mjackson]

Would someone mind explaining briefly what the bug is? I've looked at it, but I can't say for sure I understand how and why Rollup is modifying the code here.

Is it because the warning() call is a no-op in production, so that else block is empty? What does Rollup do in that scenario?

[kzc]

Simplified runnable example:

$ cat else.js 
if (Math)
    if (!Math) foo()
    else "Side effect free code to drop"
else throw "Shouldn't be reached"
console.log("PASS")

Expected:

$ cat else.js | node
PASS

Actual incorrect result:

$ cat else.js | dist/bin/rollup --silent
if (Math)
    if (!Math) foo();
else throw "Shouldn't be reached"
console.log("PASS");

After the side-effect-free else is dropped you can see that the remaining else is associated with the wrong if:

$ cat else.js | dist/bin/rollup --silent | node

[stdin]:3
else throw "Shouldn't be reached"
     ^
Shouldn't be reached

But when braces are introduced in blockless if statements then the remaining else is associated with the correct if...

$ cat else2.js 
if (Math) {
    if (!Math) {
        foo()
    } else {
        "Side effect free code to drop"
    }
} else {
    throw "Shouldn't be reached"
}
console.log("PASS")

$ cat else2.js | node
PASS

$ cat else2.js | dist/bin/rollup --silent
if (Math) {
    if (!Math) {
        foo();
    }
} else {
    throw "Shouldn't be reached"
}
console.log("PASS");

$ cat else2.js | dist/bin/rollup --silent | node
PASS

[kzc]

Another bug variant involving an else in a loop...

Given:

$ cat loop-else.js 
if (Math)
    for (var x = 1; x--;)
        if (!Math) foo()
        else "Side effect free code to be dropped"
else throw "Shouldn't be reached"
console.log("PASS")

Expected:

$ cat loop-else.js | node
PASS

Actual incorrect result:

$ cat loop-else.js | dist/bin/rollup --silent
if (Math)
    for (var x = 1; x--;)
        if (!Math) foo();
else throw "Shouldn't be reached"
console.log("PASS");

$ cat loop-else.js | dist/bin/rollup --silent | node

[stdin]:4
else throw "Shouldn't be reached"
     ^
Shouldn't be reached

I think that the buble blockless body heuristic would be a low-risk fix for these types of bugs.

[lukastaegert]

Ok I see, and thanks for the additional examples. So solutions I see and understand so far that are easy enough to implement:

• always add braces to consequent branches, or
• do not remove else branches entirely but replace them with an empty block statement {} or empty statement ;, or
• the same, but only if somewhere in the parent hierarchy there is an if statement

I think the last would cause the least noise.

[kzc]

(1) is easiest to implement and has fewer potential points of failure.
(2) is less visually appealing and more verbose.
A combination of (1) and (3) would result in less churn and probably handle all known scenarios.

[lukastaegert]

(1) and (3) would also be my favourite option but it is technical very difficult as the parent would need to change its rendering because of a condition on a remote child, that is tricky to get right. I have now a version of (3) that works quite nicely, I'll put it up shortly.

[lukastaegert]

See #3725

[kzc]

Whatever works I guess. Minifiers will drop the empty else clause in any event.