Property mangling should throw an error on non-property global references

[AshleyScirra]

Uglify version (uglifyjs -V) v3.19.3

JavaScript input

globalThis.MyGlobal = 1;

console.log(MyGlobal);

The uglifyjs CLI command executed or minify() options used.

uglifyjs input.js --mangle-props --beautify --output output.js

JavaScript output or error produced.

globalThis.l = 1;

console.log(MyGlobal);

Explanation

In the case shown above, the code is written incorrectly for property mangling. MyGlobal is referred to both as a property on the global object (globalThis.MyGlobal), and as a non-property variable access (MyGlobal).

Quite understandably, property mangling does not attempt to rename the latter reference - doing so could cause name collisions with local variables. However in this case UglifyJS merely silently produces broken code. In such cases it is better to actually throw an error.

This is what Closure Compiler does: if you pass the input script with arguments ./compiler.exe --js input.js --formatting PRETTY_PRINT --compilation_level ADVANCED --js_output_file output.js, it throws an error on the second reference and does not produce output:

input.js:5:12: ERROR - [JSC_UNDEFINED_VARIABLE] variable MyGlobal is undeclared
  5| console.log(MyGlobal);
                 ^^^^^^^^

1 error(s), 0 warning(s)

The fix with both Closure Compiler and UglifyJS is to consistently write globalThis.MyGlobal, i.e. explicitly as a property of the global object, so property mangling works as expected with globals. However by throwing an error and pointing at the incorrect code, it makes it much easier to get this right, rather than dealing with a potentially large amount of minified output that does not work.

Note this may be a breaking change, as the way UglifyJS currently works implicitly treats all non-property global references as externals that do not get renamed, and it's easy to imagine some use cases depending on this, so perhaps it should be an option (IMO, an opt-out rather than opt-in).

[alexlamsl]

PTAL at #5934

I have implemented mangle.properties.globals to detect undeclared top-level variables and avoid mangling with their property names. You have to opt-in to this new behaviour so should not be a breaking change.

[AshleyScirra]

I don't think that's quite the same thing - this issue isn't about whether or not globals get mangled, it's about ensuring a failure occurs when encountering a global variable not written as a property.

[alexlamsl]

uglify-js does not perform any validations on input code beyond SyntaxErrors (and even then it won't necessarily throw, if it can perform garbage-in-garbage-out).

In addition, one of our assumptions on the input code is that you can prepend or append to the minified output, so (seemingly) undeclared variables are a natural occurance.

One solution to preserve runtime behaviour is by not mangling property access on the global object, as shown #5934. An alternative approach to this is to have a feature whereby undeclared variables are linked to properties of the global object, i.e. your code with this feature enabled will become:

globalThis.l = 1;

console.log(l);

That would require mangle.properties to recognise some common global object shorthands like self, window, globalThis etc., although we can probably allow the user to specify an array of these names (with a default of the commonly supported ones). Would that better address your concern?

[AshleyScirra]

If you do this:

globalThis.l = 1;

console.log(l);

then you need to make sure UglifyJS never chooses the name l for any local variable for the rest of the minification. For example if you have:

globalThis.MyGlobal = 1;

// 10,000 lines of code ...

function ShowMessage(message)
{
	console.log(message, MyGlobal);
}

then minifying to the following would be broken:

globalThis.l = 1;

// 10,000 lines of code ...

function ShowMessage(l)
{
	console.log(l, l);
}

There might be other edge cases. I assumed the reason Closure Compiler didn't support this was because it was too difficult to implement and avoid such cases, as renaming non-property global variables uses the same namespace as local variables.

Perhaps a solution would be to use a prefix for all global variables, e.g. _g, so you'd instead get the following which still works:

globalThis._gl = 1;

// 10,000 lines of code ...

function ShowMessage(l)
{
	console.log(l, _gl);
}

I guess that would be a useful solution - although I can still imagine there are difficult edge cases - you have to choose a prefix that doesn't collide with any existing global variables, doesn't collide with any existing or chosen local variables, and is still sufficiently short enough to minify nicely.

Closure Compiler's solution is reasonable in my view - it requires writing all globals as property accesses, and fails on undeclared variable names. It's better than leaving broken code as it means you find out the problem at minify time, rather than having to sift through potentially thousands of lines of broken minified code (i.e. fail fast principle). Perhaps Uglify could add the "fail on undeclared" mode as an option.

[alexlamsl]

Avoiding name collision is exactly what mangle is designed to do, so I don't see it as being a difficult nut to crack. Will change the PR to implement that at some point.

Closure Compiler has a habit of complaining about valid JavaScript code because it "knows better". As much as I appreciate they are put in place for good intentions, all these minor nuisances (false positives) will just render the tool unusable, at least for me.