ADVANCED compilation incorrectly dead-code eliminates entire classes in modules (v20240317)

[AshleyScirra]

Using v20240317 with the following files (attached here: repro.zip)

// namespace.js
globalThis.MyNamespace = {};

// class.js
const MyNamespace = globalThis.MyNamespace;

MyNamespace.MyClass = class MyClass {}

// main.js
import "./namespace.js";
import "./class.js";

console.log(globalThis.MyNamespace.MyClass);

and compiling with the following command line:

./compiler.exe --js namespace.js --js class.js --js main.js --entry_point main.js --formatting PRETTY_PRINT --compilation_level ADVANCED --js_output_file out.js

produces the following output (note the undefined reference to globalThis.g.h, logging undefined):

globalThis.g = {};
console.log(globalThis.g.h);

The expected output is this (logging a class):

globalThis.g = {};
globalThis.g.h = class {
};
console.log(globalThis.g.h);

This can be worked around by changing the contents of class.js to:

globalThis.MyNamespace.MyClass = class MyClass {}

In other words the problem appears to be that Closure Compiler does not realise that const MyNamespace = globalThis.MyNamespace is aliasing a globally visible object, and so assigning to MyNamespace.MyClass is incorrectly deemed to be dead code. By assigning to globalThis directly it works correctly. This also only happens in ADVANCED mode - other modes like SIMPLE are not affected (presumably because they do not attempt dead code elimination).

This affects our commercial game development tool Construct (www.construct.net). Without the workaround, Closure Compiler deletes the entire class - all methods, properties, etc. - obviously breaking the output as one of the source code classes has been deleted.

[AshleyScirra]

Funnily enough if you change class.js to assign an object like this, it works correctly:

// class.js
const MyNamespace = globalThis.MyNamespace;

MyNamespace.MyClass = {
    "foo": "bar"
};

So it seems there is something specific to the way classes work here.

[brad4d]

This coding pattern is outside of the range of what we actively support.

1. Modules need to be defined with goog.module() and imported with goog.require() in order to be well supported.
2. Indirect "exporting" by assigning onto globalThis is not well supported. MyNamespace should be explicitly exported and imported.

If the problem occurs for code meeting those restrictions, then we are much more likely to fix it.

[AshleyScirra]

Does Closure Compiler not support using ES Modules?! We have been using it for that for a long time already!

It works correctly with an object so I had hoped this was some kind of relatively straightforward DCE issue affecting classes only and not objects... i.e. whatever Closure Compiler is currently doing with this code when using an object, it should also do when using a class.

[brad4d]

The support for ES modules was certainly added. However, within google all the JS that closure-compiler sees uses goog.require() and goog.module() to define modules. So, if it breaks we won't notice or be terribly motivated to fix it. We will accept PRs to fix it, though.

[brad4d]

I should also point out the compiler isn't designed to handle aliasing of global namespaces that are pseudo-exported via globalThis. This is the kind of non-standard coding pattern that we strongly discourage in any case.

[AshleyScirra]

It already took a fair amount of time to isolate this issue from a large production codebase, and unfortunately I just don't have any more time to spare to start learning a new programming language and contributing to a large and complex existing codebase. I recognize the limitations of open source projects but it is still quite a dispiriting response - in my view taking the time to isolate a minimal reproduction in order to report an issue in the most helpful way possible can still be a significant contribution to an open source project. It means I've already done as much work as possible to help solve the issue short of writing the fix myself, and a developer already familiar with the codebase could fix it far quicker than I could.

If this is not supported can we at least get a flag to turn off dead code elimination (or whatever pass is doing this) so we can work around it? Or does such a flag already exist? Otherwise we have to change how we write all our code for Closure Compiler to avoid this, including through third-party code, and keep doing that indefinitely.

FWIW we have to integrate third-party code that uses ES modules - we can't use the goog approach (and we'd prefer to stick to the standard approach anyway). Perhaps the code we use is a non-standard coding pattern, but it is perfectly valid code in the JavaScript language. It could well have other uses such as code for polyfills. Perhaps Google will run in to it themselves eventually...

[niloc132]

As a fellow user, I encourage you to keep in mind that ADVANCED_OPTIMIZATIONS optimizations are not suitable for general-purpose JS, but requires fairly static code that adheres to its expectations (and often expects annotations). For this reason you typically cannot just feed it arbitrary JS, even modern JS, from package managers, without customizing it with custom flags, or even your own PassConfig types.

The closure compiler is capable of correctly processing valid JS - but not with ADVANCED_OPTIMIZATIONS.

[brad4d]

I'd like to state clearly that I am not trying to blame the victim here. On the contrary, I'm trying to be as open and honest as possible about the shortcomings of closure-compiler, the use-cases we're able to support, and the use-cases we aren't able to support.

I fairly recently updated our README.md file with intention of warning potential and current users about these limitations.

[AshleyScirra]

Does anyone on the closure compiler team accept consulting work? This is affecting our commercial product and we're willing to pay for a fix.

For what it's worth, I believe the repro code complies with all documented restrictions for ADVANCED mode, and it works correctly with an object. So presumably Closure Compiler knows handle this properly because it works for objects, and that handling needs to be applied to class expressions too. This makes me think there's a chance it could be a straightforward fix (perhaps there is some oversight where something that is done for objects is not done for class expressions, and applying whatever is done for objects to class expressions may be sufficient to fix the issue).

Also FWIW, this may affect polyfill code - if you import a module that adds a class to a global object this bug may well end up just deleting the entire polyfill.

[bamtang-dev]

@AshleyScirra we also found that using the Closure Compiler outside of the path that is used inside Google is a BAD IDEA, I found some real issues with our code that just were shut down with "working as intended", fortunately we could use workarounds and it was not critical at all, but it made me realize that using the Closure Compiler may be a bad idea in the long run. Following this just to see if you find a solution to your case.

[AshleyScirra]

@bamtang-dev - I'm considering switching to something like esbuild for bundling and UglifyJS for minifying. But we used UglifyJS in the past and switched to Closure Compiler because of various issues, and switching is really painful (mainly due to making sure externs match up and running in to various minify bugs with the new tool). So if we have to switch back that will be all that pain all over again. And it would be a shame as Closure Compiler seems to be one of the most advanced and best tools for its advanced mode minification, when it works at least. But Closure Compiler also seems to be lagging behind - there's still no support for private fields for example. JavaScript tooling can be a real headache...

[bamtang-dev]

the thing that I'm missing the most is dead code removal, even TypeScript that has all the information to do this prefers not to do it, but well, maybe they will do it someday...

[AshleyScirra]

esbuild apparently does tree shaking, which is a type of dead code removal. I'm not sure how far it goes though (there's all sorts of cases, like deleting if (false) statements). Still, better to leave dead code in than delete live code...