@const creeping

[alexlamsl]

See #1448 (comment) onwards

$ echo '/*@const*/ var b = 1; b = 2; console.log(1+b);' | node
3

$ echo '/*@const*/ var b = 1; b = 2; console.log(1+b);' | uglifyjs -c
var b=1;b=2,console.log(2);

$ echo '/*@const*/ var b = 1; b = 2; console.log(1+b);' | uglifyjs -c | node
2

[alexlamsl]

@kzc I'm more than happy to deprecate /*@const*/ as it does not have the same safety guarantee as reduce_vars, though I wonder if there are cases where people would want such unsafe overrides in existing code.

[alexlamsl]

/*@const*/ was introduced a bit over a year ago in 8b71c65

PR #928

[kzc]

deprecate /*@const*/ as it does not have the same safety guarantee as reduce_vars

Should remove support for @const from uglify altogether. It's flawed by design.

It will be treated like any other comment - just ignored and produce suboptimal code. Documentation should be updated to advise the use of the reduce_vars option.

However, uglify ought to raise a fatal error in this case upon the illegal reassignment:

$ echo 'const b = 1; b = 2; console.log(1+b);' | uglifyjs -c
const b=1;b=2,console.log(2);

[alexlamsl]

Node.js (and I supposed web browsers) would do this for us:

$ echo 'const b=1;b=2,console.log(2);' | node
TypeError: Assignment to const
   at Global code ([stdin]:1:11)
   at Script.prototype.runInThisContext (vm.js:25:5)
   at exports.runInThisContext (vm.js:77:3)
   at Anonymous function ([stdin]-wrapper:6:1)
   at Module.prototype._compile (module.js:560:3)
   at Anonymous function (bootstrap_node.js:345:7)
   at _combinedTickCallback (internal/process/next_tick.js:67:7)
   at _tickCallback (internal/process/next_tick.js:98:9)

So would this not fall under the category of garbage in, garbage out?

[kzc]

So would this not fall under the category of garbage in, garbage out?

Works for me.

[kzc]

I think we discounted raising an error upon const reassignment in Uglify too quickly.

Using either v2.7.5 or 2.8.0 staging #1485:

$ echo 'const b = 1, b = 2; console.log(1+b);' | bin/uglifyjs -c
const b=1,b=2;console.log(3);

$ echo 'const b = 1; b = 2; console.log(1+b);' | bin/uglifyjs -c
const b=1;b=2,console.log(2);

Yes, both are invalid programs, but Uglify is inconsistent between variable redeclaration and reassignment.

Are we certain that no transform will incorrectly remove one of the reassignments thus emitting a "valid" program?

[alexlamsl]

Oh woah that is surprising, and I don't like surprises. Let me investigate the why and the how before getting back to you on that one.

[alexlamsl]

... ah, my eyes failed me. Okay those aren't as bad as I thought they are.

Back in #1448 (comment) b=3 was stripped down and become 3, which can be problematic:

master

$ echo '!function(){const b=2;b=3;return 1+b}()' | node
TypeError: Assignment to const
   at Anonymous function ([stdin]:1:23)
   at Global code ([stdin]:1:1)
...

$ echo '!function(){const b=2;b=3;return 1+b}()' | uglifyjs -c
!function(){const b=2;return b=3,3}();

$ echo '!function(){const b=2;b=3;return 1+b}()' | uglifyjs -c | node
TypeError: Assignment to const
   at Anonymous function ([stdin]:1:23)
   at Global code ([stdin]:1:1)
...

#1485

$ echo '!function(){const b=2;b=3;return 1+b}()' | node
TypeError: Assignment to const
   at Anonymous function ([stdin]:1:23)
   at Global code ([stdin]:1:1)
...

$ echo '!function(){const b=2;b=3;return 1+b}()' | uglifyjs -c
WARN: Dropping unused variable b [-:1,18]
!function(){return 3,3}();

$ echo '!function(){const b=2;b=3;return 1+b}()' | uglifyjs -c | node
WARN: Dropping unused variable b [-:1,18]

So #1450 optimised away the assignment which becomes problematic.

[alexlamsl]

The more I think about this, the more I'm tempted to eliminate this special case const treatment and just turn reduce_var on by default. That way, those substitutions won't take place and we will happily give the same error-prone code back out.

[kzc]

Enabling reduce_var by default won't solve this bug:

$ echo '!function(){const b=2;b=3;return 1+b}()' | uglifyjs -c | node
WARN: Dropping unused variable b [-:1,18]

But then again const is in uglify (master) is an anomaly - it is not part of ES5.1 - it's just grandfathered behavior. It is also scoped incorrectly for ES6 as it does not follow block scope.

How difficult is it to just raise an error upon reassigning or redeclaring a const variable? Where would it be easier to trap - in parse or figure_out_scope?

The other issue with enabling reduce_vars or collapse_vars by default is that they eliminate top level vars. Some users may not been keen on that. Uglify releases do not presently do this with default compress options.

[kzc]

The heuristic for trapping bad const reassignments is pretty straightforward. All variables in uglify have references to their variable definition. If a variable is used in LHS context and its variable definition is const, then raise a fatal error.

[alexlamsl]

@kzc yes it would - just comment out this line to disable const detection and see.

[alexlamsl]

reduce_vars already detects any modifications (including b++ etc.) to a variable, and at the moment it indiscriminately analyses both var and const symbols. So it would just leave those bad const instances alone and won't perform any substitutions.

[kzc]

But commenting out that line will emit garbage-in/garbage-out code where the error has to be trapped by the JS engine.

We should have uglify raise a fatal error at that point and exit. It's ill-formed JS.

[kzc]

By not trapping the problem early we're putting a burden on all the optimizations to check for this ridiculous case.

[alexlamsl]

I thought uglify-js only emits parser errors - this will be the first case the compressor throws an error (it only reports warnings AFAICT)

[kzc]

Yeah, you have a point. It's a runtime error anyway, not a parse error:

$ echo '!function(){const b=2; try{ b=3 } catch(x){console.error("Caught:", x)} }()' | node
Caught: TypeError: Assignment to constant variable.

[kzc]

At the very least uglify ought to warn that a const variable is being redeclared or reassigned.

[kzc]

Maybe uglify could offer a --strict mode where const reassignment could be considered to be a fatal error.

[alexlamsl]

I can do a check for instanceof AST_SymbolConst and emit the warning around here

[kzc]

I thought reset_opt_flags() is not run for default compress. Only when passes > 1.

[alexlamsl]

Not if reduce_vars is enabled since #1473

[kzc]

That does not help in the default compress case. Unless you wish to enable reduce_vars by default and make a shift in policy for top level declarations.

[kzc]

If you do decide to make such a change to enable reduce_vars by default, might as well enable collapse_vars by default as well. It's been in the code base for a year and tested in the wild.

[alexlamsl]

@avdg @mishoo @rvanvelzen any objections to turning collapse_vars and reduce_vars on by default?

[kzc]

I can do a check for instanceof AST_SymbolConst and emit the warning

The problem with using warnings for const variable reassignment is that most third party users of uglify (webpack and other wrappers) disable warnings altogether. I suggest adding a strict compress option that is enabled by default to raise a fatal error in such cases. Although it's technically a runtime error, no coder would want such incorrect code to go unnoticed by uglify.

[avdg]

No opinion on my part

[alexlamsl]

@kzc what other things can you think of strict to enforce besides const?

[kzc]

That's the only strict error I can think of.