var use before defined incorrect, let/const TDZ errors masked by tree shaking

[kzc]

• Rollup Version: rollup v2.50.1
• Operating System (or Browser): n/a
• Node Version (if applicable): v16.1.0

Repro

$ cat misc.js
(function bug1() {
    var first = state();
    var on = true;
    var obj = { state: state };
    console.log(first, obj.state());
    function state() {
        return on ? "ON" : "OFF";
    }
})();
(function bug2() {
    var a = "PASS";
    for (var b = 2; --b >= 0; ) {
        (function() {
            var c = function() {
                return 1;
            }(c && (a = "FAIL"));
        })();
    }
    console.log(a);
})();

Expected Behavior

$ cat misc.js | node
OFF ON
PASS

Actual Behavior

$ cat misc.js | rollup --silent | node
ON ON
FAIL

$ cat misc.js | rollup --silent
(function bug1() {
    var first = state();
    var obj = { state: state };
    console.log(first, obj.state());
    function state() {
        return "ON" ;
    }
})();
(function bug2() {
    var a = "PASS";
    for (var b = 2; --b >= 0; ) {
        (function() {
            (function() {
                return 1;
            })((a = "FAIL"));
        })();
    }
    console.log(a);
})();

[kzc]

There might be a relatively simple fix for this. Consider the addition of these synthetically introduced redundant valueless var declarations at the top of the problematic functions in question:

$ cat misc2.js 
(function bug1() {
    var on;           // <----- introduce synthetically
    var first = state();
    var on = true;
    var obj = { state: state };
    console.log(first, obj.state());
    function state() {
        return on ? "ON" : "OFF";
    }
})();
(function bug2() {
    var a = "PASS";
    for (var b = 2; --b >= 0; ) {
        (function() {
            var c;    // <----- introduce synthetically
            var c = function() {
                return 1;
            }(c && (a = "FAIL"));
        })();
    }
    console.log(a);
})();

--- misc.js
+++ misc2.js
@@ -1,3 +1,4 @@
 (function bug1() {
+    var on;           // <----- introduce synthetically
     var first = state();
     var on = true;
@@ -12,4 +13,5 @@
     for (var b = 2; --b >= 0; ) {
         (function() {
+            var c;    // <----- introduce synthetically
             var c = function() {
                 return 1;

$ cat misc2.js | node
OFF ON
PASS

The test now passes:

$ rollup -v
rollup v2.50.3

$ cat misc2.js | rollup --silent | node
OFF ON
PASS

Since var variables have function scope, Rollup could initialize them internally as undefined at top of function rather than the position they appear within the function.

[lukastaegert]

True, but this basically means that all var variables are completely deoptimized. I.e. this would be more or less equivalent to just not track any values for var variables. Which would be a huge loss. But for now, I see no way to fix this without breaking analysis short of implementing basic flow analysis, which would be huge in and off itself. The only quick fix I see would be to introduce a flag to decide if we deoptimize all var variables.

[kzc]

At least in this case there might be a heuristic to remedy the situation:

            var c = function() {
                return 1;
            }(c && (a = "FAIL"));

as there are no assignments or updates to the var c in the decl's init subtree, so it ought to remain undefined. It only has a value after the init. One could mark all the c identifiers within the init tree to not assume it has a value. If there are any assignments or updates to the variable within the init tree then this particular variable has to be deoptimized for each of its uses.

[kzc]

@lukastaegert It appears that this bug is not limited to var variables. It is a general bug that also affects const and let variables - but in the opposite way. Incorrect code is transformed without warning to produce a program that has different runtime behavior:

Given an incorrectly written program with a runtime TDZ violation:

$ cat misc3.js 
console.log(function() {
    if (x) return "HELLO";
    const x = 1;
    return "WORLD";
}());

Here's the expected runtime error:

$ cat misc3.js | node
[stdin]:2
    if (x) return "HELLO";
    ^

ReferenceError: Cannot access 'x' before initialization

And here's the actual incorrect result produced with rollup:

$ rollup -v
rollup v2.50.4

$ cat misc3.js | rollup --silent | node
HELLO

Notice that rollup produced no error or warning:

$ cat misc3.js | rollup

- → stdout...
console.log(function() {
    return "HELLO";
}());

$ echo $?
0

If the rollup user does not test their code without rollup, or with rollup --no-treeshake, they wouldn't be aware of the error in their original code that was masked.

Minifiers such as uglify-js and terser strive to preserve the behavior of good or bad code with a garbage-in/garbage-out philosophy:

$ cat misc3.js | uglify-js --toplevel -mc
console.log(function(){if(n)return"HELLO";const n=1;return"WORLD"}());

$ cat misc3.js | node_modules/.bin/terser --toplevel -mc
console.log(function(){if(n)return"HELLO";const n=1;return"WORLD"}());

[kzc]

Here's a simpler example of a TDZ failure:

$ echo 'let x = x, y = console.log(123);' | node
[stdin]:1
let x = x, y = console.log(123);
        ^

ReferenceError: Cannot access 'x' before initialization

The runtime error is masked by rollup:

$ echo 'let x = x, y = console.log(123);' | rollup --silent
console.log(123);

uglify-js 3.13.8 correctly preserves the TDZ error:

$ echo 'let x = x, y = console.log(123);' | uglify-js --toplevel -mc
let l=l;console.log(123);

However, terser 5.7.0 has a bug:

$ echo 'let x = x, y = console.log(123);' | node_modules/.bin/terser --toplevel -mc
console.log(123);

[kzc]

Workaround for the var-use-before-defined bugs by deoptimizing all var variables: #4139

The only workaround for the masking of let/const TDZ errors is to disable tree shaking altogether - and if optimization is required a minifier like uglify-js that retains the TDZ violating code could be used. (Terser at time of this writing hides TDZ violations similar to rollup.)

These two issues above are result of the same underlying variable analysis problems. A proper fix for either would resolve both issues.