Variable inlining changes program behavior by ignoring var hoisting

[hcnelson99]

trafficstars

test.js:

var x = 1;
var z = function () {
    var y = x;
    var x = 0;
    return y;
}();
console.log(z);

Observed behavior:

$ node test.js
undefined
$ closure-compiler test.js
var x=1,z=function(){return 0}();console.log(z);
$ closure-compiler test.js | node
0

Desired behavior: closure-compiler should not change program behavior.

Version:

$ closure-compiler --version
Closure Compiler (http://github.com/google/closure-compiler)
Version: 1.0-SNAPSHOT
Built on: 2018-07-18 15:08

[tjgq]

Repro: https://closure-compiler-debugger.appspot.com/#input0%3Dvar%2520x%2520%253D%25201%253B%250Avar%2520z%2520%253D%2520function%2520()%2520%257B%250A%2520%2520%2520%2520var%2520y%2520%253D%2520x%253B%250A%2520%2520%2520%2520var%2520x%2520%253D%25200%253B%250A%2520%2520%2520%2520return%2520y%253B%250A%257D()%253B%250Aconsole.log(z)%253B%26input1%26conformanceConfig%26externs%26refasterjs-template%26includeDefaultExterns%3Dtrue%26CHECK_TYPES%3Dtrue%26TRANSPILE%3Dtrue%26FOLD_CONSTANTS%3Dtrue%26INLINE_FUNCTIONS%3Dtrue%26INLINE_VARIABLES%3Dtrue%26CLOSURE_PASS%3Dtrue%26PRESERVE_TYPE_ANNOTATIONS%3Dtrue%26PRETTY_PRINT%3Dtrue

@lauraharker, can you take a look?

[lauraharker]

Huh, it looks like this was intended behavior 10 years ago.

      // Reject anything that might modify relevant state. We assume that
      // nobody relies on variables being undeclared, which will break
      // constructions like:
      //   var a = b;
      //   var b = 3;
      //   alert(a);

https://github.com/google/closure-compiler/blob/6171836b0b942d57b94aebae6d4055658552f4df/src/com/google/javascript/jscomp/NodeIterators.java#L273

I don't see this documented anywhere, though. It's not in https://github.com/google/closure-compiler/wiki/Compiler-Assumptions or https://developers.google.com/closure/compiler/docs/limitations.

The compiler does issue a warning on this code if you enable CHECK_SYMBOLS.

This probably wouldn't be a trivial fix. If we naively assume that every assignment var b = 3; can modify state, we'll probably see a code size regression in a lot of projects. Maybe we should instead normalize this case before running the variable inlining pass.

[concavelenz]

We generally allow the compiler to perform optimization with the assumption that the code is correct (we allow the compiler to "fix" broken code). If this were a "let" instead of a "var" the code would throw a runtime exception.

Do you have an example of real code where this breaks valid program behavior?

[hcnelson99]

Do you have an example of real code where this breaks valid program behavior?

I do not.

I'd consider this fixed if closure compiler warned about it by default.

[concavelenz]

Hmmm, I would have expected the VariableReferenceChecks to be on by default? How are you running the compiler?

[hcnelson99]

From the command line with no flags passed.

[concavelenz]

Something is not right with the webservice as well, even with // @warning_level VERBOSE the warning is not presented:

https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540compilation_level%2520ADVANCED_OPTIMIZATIONS%250A%252F%252F%2520%2540output_file_name%2520default.js%250A%252F%252F%2520%2540formatting%2520pretty_print%250A%252F%252F%2520%2540warning_level%2520VERBOSE%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250Avar%2520x%2520%253D%25201%253B%250Avar%2520z%2520%253D%2520function%2520()%2520%257B%250A%2520%2520%2520%2520var%2520y%2520%253D%2520x%253B%250A%2520%2520%2520%2520var%2520x%2520%253D%25200%253B%250A%2520%2520%2520%2520return%2520y%253B%250A%257D()%253B%250Aconsole.log(z)%253B%250A%250A

But it is in the debugger with "all diagnostics" enabled:

https://closure-compiler-debugger.appspot.com/#input0%3Dvar%2520x%2520%253D%25201%253B%250A%252F**%2520%2540return%2520%257B%253F%257D%2520*%252F%250Avar%2520z%2520%253D%2520function%2520()%2520%257B%250A%2520%2520%2520%2520var%2520y%2520%253D%2520x%253B%250A%2520%2520%2520%2520var%2520x%2520%253D%25200%253B%250A%2520%2520%2520%2520return%2520y%253B%250A%257D%253B%250Az()%253B%250Ause(z)%253B%26input1%26conformanceConfig%26externs%3D%252F**%2520%2540param%2520%257B%253F%257D%2520a%2520*%252F%2520function%2520use(a)%2520%257B%257D%250A%26refasterjs-template%26includeDefaultExterns%3Dtrue%26ENABLE_ALL_DIAGNOSTIC_GROUPS%3Dtrue%26CHECK_TYPES%3Dtrue%26TRANSPILE%3Dtrue%26FOLD_CONSTANTS%3Dtrue%26INLINE_FUNCTIONS%3Dtrue%26INLINE_VARIABLES%3Dtrue%26CLOSURE_PASS%3Dtrue%26PRESERVE_TYPE_ANNOTATIONS%3Dtrue%26PRETTY_PRINT%3Dtrue

[lauraharker]

The command line warning only shows up with --warning_level VERBOSE or jscomp_warning=checkVars, but is off by default. https://github.com/google/closure-compiler/blob/8cb0d7e9dde943cd2f46a9cb916c511fc9616901/src/com/google/javascript/jscomp/WarningLevel.java#L82

And apparently the webservice backend (which isn't open sourced) explicitly disables this warning with options.checkSymbols = false; no matter what the warning level is.

[concavelenz]

Ah, that would be effectively the "third_party" flag which is always on for the webservice

[lauraharker]

(going through old issues assigned to me)

I think this hasn't changed since 2018: the command line runner still does not report "checkSymbols" warnings/errors by default, and the webservice always disables "checkSymbols".

I'll try to change the webservice to at least not unilaterally ignore "checkSymbols" but otherwise am not currently prioritizing this.

[lauraharker]

The webservice will now show undeclared symbol errors in ADVANCED mode by default: https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540compilation_level%2520ADVANCED_OPTIMIZATIONS%250A%252F%252F%2520%2540output_file_name%2520default.js%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250A%252F%252F%2520ADD%2520YOUR%2520CODE%2520HERE%250Afunction%2520hello(name)%2520%257B%250A%2520%2520alert('Hello%252C%2520'%2520%252B%2520name)%253B%250A%257D%250Ahello('New%2520user')%253B%250A%250Aalert(missingName)%253B%250A

Unassigning since I'm not planning to work on this further.