Open hcnelson99 opened 6 years ago
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);
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](https://closure-compiler-debugger.appspot.com/#input0%3Dfunction%2520f()%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%26input1%26conformanceConfig%26externs%26refasterjs-template%26includeDefaultExterns%3Dtrue%26CHECK_TYPES%3Dtrue%26CHECK_SYMBOLS%3Dtrue%26TRANSPILE%3Dtrue%26INLINE_VARIABLES%3Dtrue%26CLOSURE_PASS%3Dtrue%26PRESERVE_TYPE_ANNOTATIONS%3Dtrue%26PRETTY_PRINT%3Dtrue) 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.
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?
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.
Hmmm, I would have expected the VariableReferenceChecks to be on by default? How are you running the compiler?
From the command line with no flags passed.
Something is not right with the webservice as well, even with // @warning_level VERBOSE the warning is not presented:
But it is in the debugger with "all diagnostics" enabled:
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.
Ah, that would be effectively the "third_party" flag which is always on for the webservice
(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.
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.
test.js:
Observed behavior:
Desired behavior: closure-compiler should not change program behavior.
Version: