CommonJS modules incorrectly overwritten

[TJ09]

Version

$ google-closure-compiler --version
Closure Compiler (http:/google/closure-compiler)
Version: v20190415
Built on: 2019-04-17 23:52

(tested on v20190507-nightly as well)

Input

// a.js

module.exports = 'a';

// a-or-b.js

var aOrB = require('./a.js');

if (global.useB) {
  aOrB = 'b';
}

module.exports = aOrB;

// main.js

const aOrB = require('./a-or-b.js');

console.log(aOrB);

Command

$ google-closure-compiler \
> --compilation_level WHITESPACE_ONLY \
> --formatting PRETTY_PRINT \
> --process_common_js_modules \
> --entry_point main \
> --js_output_file out.js \
> --js {a,b,a-or-b,main}.js

Output

// out.js

var module$a = {};
module$a.default = "a";
var module$a_or_b = {};
var aOrB$$module$a_or_b = module$a.default;
if (global.useB) {
  module$a.default = "b";
}
module$a_or_b.default = module$a.default;
var aOrB = module$a_or_b.default;
console.log(module$a_or_b.default);

Issue

In the compiled version of a-or-b.js (that is, a CommonJS module that imports something and then overwrites the variable containing that imported module with something else), Closure incorrectly inlines the value of require('./a.js') anywhere the variable aOrB is referenced (even without optimizations being enabled). This has the side effect of overwriting the export of a.js everywhere.

The line module$a.default = "b"; is expected to be aOrB$$module$a_or_b = "b"; (and module$a_or_b.default = module$a.default; is expected to be module$a_or_b.default = aOrB$$module$a_or_b;

[brad4d]

Created internal issue http://b/132180272

[brad4d]

I find the presence of 'default' in the output suspicious.
Seems like the compiler is treating the modules as if they were being imported as ES6 modules, even though they aren't.

@ChadKillingsworth could you take a look? Thanks.

[ChadKillingsworth]

Thanks for the repo case. With type checking enabled, this warning is the clue to the problem:

test2.js:6: WARNING - constant property default assigned a value more than once
  aOrB = 'b';
  ^^^^

0 error(s), 1 warning(s), 92.8% typed

The CommonJS pass isn't recognizing the assignment to the export, so it marks the export as CONST and in ADVANCED mode the compiler then inlines the references.

[ChadKillingsworth]

I went back to look at this again. We don't support re-using the import variable in that way. Doing that would require full flow analysis.

The pass specifically tries to avoid creating aliases so references to the import variable are re-targeted to the host module directly.

To support this type of flow, you'd need:

// a-or-b.js

var a = require('./a.js');
var aOrB = a;
if (global.useB) {
  aOrB = 'b';
}

module.exports = aOrB;