Invalid dead code removal for return statement due to ASI

[evanw]

• Rollup Version: 2.26.0
• Operating System (or Browser): Chrome 84
• Node Version (if applicable):
• Link to reproduction (IMPORTANT, read below): repl link

Input:

function foo() {
  return false ||
    true || 
    false
}
console.log(foo());

Expected Behavior

function foo() {
  return true
}
console.log(foo());

Actual Behavior

function foo() {
  return 
    true 
}
console.log(foo());

A function which returns true is incorrectly transformed into a function which returns undefined. This is a reduced test case from real-world code here that looks like this:

function getComponentName(target) {
  return (process.env.NODE_ENV !== 'production' ? typeof target === 'string' && target : false) || // $FlowFixMe
  target.displayName || // $FlowFixMe
  target.name || 'Component';
}

[kzc]

Similar ASI issues are also present in throw and yield...

throw

$ cat throw.js
process.on("uncaughtException", x => console.log(x.message))
throw null ||
    new Error("PASS") ||
    false

expected:

$ cat throw.js | node
PASS

actual:

$ cat throw.js | dist/bin/rollup --silent
process.on("uncaughtException", x => console.log(x.message));
throw 
    new Error("PASS") ||
    false

$ cat throw.js | dist/bin/rollup --silent | node
[stdin]:2
throw 
^^^^^

SyntaxError: Illegal newline after throw

yield

$ cat yield.js 
function* f(x) {
    yield 0 ||
        x
}
let iter = f(5)
console.log(iter.next().value)

expected:

$ cat yield.js | node
5

actual:

$ cat yield.js | dist/bin/rollup --silent
function* f(x) {
    yield 
        x;
}
let iter = f(5);
console.log(iter.next().value);

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

I didn't investigate whether await has the same issue.

[lukastaegert]

Thanks for spotting, I will have a look. I know I already fixed a similar group of issues in #3035 but apparently I missed something 🙄

[lukastaegert]

Ah I see, I forgot to pass down the preventASI flag for nested expressions... Fix at #3732

[kzc]

@lukastaegert PR #3732 does not address the ASI bug in throw - see #3729 (comment).

[kzc]

There's also still an issue with yield ...

$ dist/bin/rollup -v
rollup v2.26.2

$ cat yield2.js
function* f(x) {
    yield 0 ||
        x ||
        false
}
let iter = f("PASS")
console.log(iter.next().value)

expected:

$ cat yield2.js | node
PASS

actual:

$ cat yield2.js | dist/bin/rollup --silent
function* f(x) {
    yield 
        x ||
        false;
}
let iter = f("PASS");
console.log(iter.next().value);

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

[lukastaegert]

Ah I see, I did not consider that due to execution order, the AST is nested so the first logical expression is nested into the second one, not the other way around. So if the second one is indeterminate, the first one could still be simplified to cause the issue, but the preventASI flag needs to be passed through the second one.

I didn't investigate whether await has the same issue.

I do not think so because I am quite sure that there is no ASI after await. I think I assumed the same for yield but apparently that assumption was wrong.