CrossChunkCodeMotion breaks `--chunk_output_type ES_MODULES`

[DavidANeil]

Given the following program:

// basepoint.js
const foo = {instance: null};

/**
 * @nocollapse
 * @noinline
 */
export function setFoo(v) {
    foo.instance = v;
}

export function getFoo() {
    return foo.instance;
}

console.log(getFoo());

// otherfile.js
import {setFoo} from './basepoint.js';

setFoo('hi from otherfile.js');

Running the compiler produces a JSC_IMPORT_ASSIGN diagnostic:

$ bazel run //:compiler_uberjar -- --chunk_output_type ES_MODULES --chunk base:1 --chunk other:1:base --js /home/davidneil/github/closure-compiler/basepoint.js --js /home/davidneil/github/closure-compiler/otherfile.js  --compilation_level=ADVANCED_OPTIMIZATIONS --use_types_for_optimization=false --chunk_output_path_prefix /home/davidneil/github/closure-compiler/out/

/home/davidneil/github/closure-compiler/basepoint.js:10:8: ERROR - [JSC_IMPORT_ASSIGN] Imported symbol "a" in chunk "other.js" cannot be assigned (defined in "base.js")
  10|     foo.instance = v;
              ^^^^^^^^

If we inspect the output with debugging enabled we see when the bug is introduced: (output is trimmed to relevant parts)

 $ bazel run //:compiler_uberjar -- --chunk_output_type ES_MODULES --chunk base:1 --chunk other:1:base --js /home/davidneil/github/closure-compiler/basepoint.js --js /home/davidneil/github/closure-compiler/otherfile.js  --compilation_level=ADVANCED_OPTIMIZATIONS --use_types_for_optimization=false --chunk_output_path_prefix /home/davidneil/github/closure-compiler/out/ --formatting PRETTY_PRINT --debug --print_source_after_each_pass

// parseInputs yields:
// ************************************
// module 'base'
const foo = {instance:null};
export function setFoo(v) {
  foo.instance = v;
}
export function getFoo() {
  return foo.instance;
}
console.log(getFoo());
// module 'other'
import{setFoo}from "./basepoint.js";
setFoo("hi from otherfile.js");

// ************************************

// earlyInlineVariables yields:
// ************************************
// module 'base'
var foo$$module$home$davidneil$github$closure_compiler$basepoint$instance = null;
function setFoo$$module$home$davidneil$github$closure_compiler$basepoint(v$jscomp$1) {
  foo$$module$home$davidneil$github$closure_compiler$basepoint$instance = v$jscomp$1;
}
console.log(function() {
  return foo$$module$home$davidneil$github$closure_compiler$basepoint$instance;
}());
// module 'other'
setFoo$$module$home$davidneil$github$closure_compiler$basepoint("hi from otherfile.js");

// ************************************

// crossChunkCodeMotion yields:
// ************************************
// module 'base'
var foo$$module$home$davidneil$github$closure_compiler$basepoint$instance = null;
console.log(function() {
  return foo$$module$home$davidneil$github$closure_compiler$basepoint$instance;
}());
// module 'other'
function setFoo$$module$home$davidneil$github$closure_compiler$basepoint(v$jscomp$1) {
  foo$$module$home$davidneil$github$closure_compiler$basepoint$instance = v$jscomp$1;
}
setFoo$$module$home$davidneil$github$closure_compiler$basepoint("hi from otherfile.js");

// ************************************

// inlineFunctions yields:
// ************************************
// module 'base'
var foo$$module$home$davidneil$github$closure_compiler$basepoint$instance = null;
console.log(foo$$module$home$davidneil$github$closure_compiler$basepoint$instance);
// module 'other'
function setFoo$$module$home$davidneil$github$closure_compiler$basepoint(v$jscomp$1) {
  foo$$module$home$davidneil$github$closure_compiler$basepoint$instance = v$jscomp$1;
}
setFoo$$module$home$davidneil$github$closure_compiler$basepoint("hi from otherfile.js");

// ************************************

// convertChunksToESModules yields:
// ************************************
// module 'base'
var $foo$$module$home$davidneil$github$closure_compiler$basepoint$instance$$ = null;
console.log($foo$$module$home$davidneil$github$closure_compiler$basepoint$instance$$);
export{$foo$$module$home$davidneil$github$closure_compiler$basepoint$instance$$};
// module 'other'
import{$foo$$module$home$davidneil$github$closure_compiler$basepoint$instance$$}from "./base.js";
function $setFoo$$module$home$davidneil$github$closure_compiler$basepoint$$($v$jscomp$1$$) {
  $foo$$module$home$davidneil$github$closure_compiler$basepoint$instance$$ = $v$jscomp$1$$;
}
$setFoo$$module$home$davidneil$github$closure_compiler$basepoint$$("hi from otherfile.js");

// ************************************

Either CrossChunkCodeMotion needs to not move setFoo to another chunk if it has non-importable symbol dependencies, or it needs to add exports/imports for any symbols that setFoo depends on. I think the former is the better solution.

Related, if the @nocollapse and @noinline are removed, then it looks like the bug is actually introduced by the earlyInlineVariables pass

// parseInputs yields:
// ************************************
// module 'base'
const foo = {instance:null};
export function setFoo(v) {
  foo.instance = v;
}
export function getFoo() {
  return foo.instance;
}
console.log(getFoo());
// module 'other'
import{setFoo}from "./basepoint.js";
setFoo("hi from otherfile.js");

// ************************************

// es6RewriteModule yields:
// ************************************
// module 'base'
const foo$$module$home$davidneil$github$closure_compiler$basepoint = {instance:null};
function setFoo$$module$home$davidneil$github$closure_compiler$basepoint(v) {
  foo$$module$home$davidneil$github$closure_compiler$basepoint.instance = v;
}
function getFoo$$module$home$davidneil$github$closure_compiler$basepoint() {
  return foo$$module$home$davidneil$github$closure_compiler$basepoint.instance;
}
console.log(getFoo$$module$home$davidneil$github$closure_compiler$basepoint());
var module$home$davidneil$github$closure_compiler$basepoint = {};
module$home$davidneil$github$closure_compiler$basepoint.getFoo = getFoo$$module$home$davidneil$github$closure_compiler$basepoint;
module$home$davidneil$github$closure_compiler$basepoint.setFoo = setFoo$$module$home$davidneil$github$closure_compiler$basepoint;
// module 'other'
setFoo$$module$home$davidneil$github$closure_compiler$basepoint("hi from otherfile.js");
var module$home$davidneil$github$closure_compiler$otherfile = {};

// ************************************

// normalize yields:
// ************************************
// module 'base'
const foo$$module$home$davidneil$github$closure_compiler$basepoint = {instance:null};
function setFoo$$module$home$davidneil$github$closure_compiler$basepoint(v$jscomp$1) {
  foo$$module$home$davidneil$github$closure_compiler$basepoint.instance = v$jscomp$1;
}
function getFoo$$module$home$davidneil$github$closure_compiler$basepoint() {
  return foo$$module$home$davidneil$github$closure_compiler$basepoint.instance;
}
console.log(getFoo$$module$home$davidneil$github$closure_compiler$basepoint());
var module$home$davidneil$github$closure_compiler$basepoint = {};
module$home$davidneil$github$closure_compiler$basepoint.getFoo = getFoo$$module$home$davidneil$github$closure_compiler$basepoint;
module$home$davidneil$github$closure_compiler$basepoint.setFoo = setFoo$$module$home$davidneil$github$closure_compiler$basepoint;
// module 'other'
setFoo$$module$home$davidneil$github$closure_compiler$basepoint("hi from otherfile.js");
var module$home$davidneil$github$closure_compiler$otherfile = {};

// ************************************

// inlineAndCollapseProperties yields:
// ************************************
// module 'base'
var foo$$module$home$davidneil$github$closure_compiler$basepoint$instance = null;
function setFoo$$module$home$davidneil$github$closure_compiler$basepoint(v$jscomp$1) {
  foo$$module$home$davidneil$github$closure_compiler$basepoint$instance = v$jscomp$1;
}
function getFoo$$module$home$davidneil$github$closure_compiler$basepoint() {
  return foo$$module$home$davidneil$github$closure_compiler$basepoint$instance;
}
console.log(getFoo$$module$home$davidneil$github$closure_compiler$basepoint());
var module$home$davidneil$github$closure_compiler$basepoint$getFoo = null;
var module$home$davidneil$github$closure_compiler$basepoint$setFoo = null;
// module 'other'
setFoo$$module$home$davidneil$github$closure_compiler$basepoint("hi from otherfile.js");

// ************************************

// earlyInlineVariables yields:
// ************************************
// module 'base'
var foo$$module$home$davidneil$github$closure_compiler$basepoint$instance = null;
console.log(function() {
  return foo$$module$home$davidneil$github$closure_compiler$basepoint$instance;
}());
// module 'other'
(function(v$jscomp$1) {
  foo$$module$home$davidneil$github$closure_compiler$basepoint$instance = v$jscomp$1;
})("hi from otherfile.js");

// ************************************

// inlineFunctions yields:
// ************************************
// module 'base'
var foo$$module$home$davidneil$github$closure_compiler$basepoint$instance = null;
console.log(foo$$module$home$davidneil$github$closure_compiler$basepoint$instance);
// module 'other'
 {
  foo$$module$home$davidneil$github$closure_compiler$basepoint$instance = "hi from otherfile.js";
}
;
// ************************************

// peepholeOptimizations yields:
// ************************************
// module 'base'
var foo$$module$home$davidneil$github$closure_compiler$basepoint$instance = null;
console.log(foo$$module$home$davidneil$github$closure_compiler$basepoint$instance);
// module 'other'
foo$$module$home$davidneil$github$closure_compiler$basepoint$instance = "hi from otherfile.js";

// ************************************

// renameVars yields:
// ************************************
// module 'base'
var $foo$$module$home$davidneil$github$closure_compiler$basepoint$instance$$ = null;
console.log($foo$$module$home$davidneil$github$closure_compiler$basepoint$instance$$);
// module 'other'
$foo$$module$home$davidneil$github$closure_compiler$basepoint$instance$$ = "hi from otherfile.js";

// ************************************

// convertChunksToESModules yields:
// ************************************
// module 'base'
var $foo$$module$home$davidneil$github$closure_compiler$basepoint$instance$$ = null;
console.log($foo$$module$home$davidneil$github$closure_compiler$basepoint$instance$$);
export{$foo$$module$home$davidneil$github$closure_compiler$basepoint$instance$$};
// module 'other'
import{$foo$$module$home$davidneil$github$closure_compiler$basepoint$instance$$}from "./base.js";
$foo$$module$home$davidneil$github$closure_compiler$basepoint$instance$$ = "hi from otherfile.js";

// ************************************

[concavelenz]

Chad can you comment on your expectations here?

[ChadKillingsworth]

This was always a danger of ESModules since you cannot mutate an export directly. It's not clear to me the best way to avoid this issue as any number of compiler pass optimizations can create this situation. Blocking them not only runs the risk of playing "whack-a-mole" but also would likely reduce the effectiveness of ADVANCED mode substantially.

If I get time, I may play around with some ideas. It may mean that the convert to modules pass needs to inject a helper function back into the exporting module to perform the actual mutation.