google / google/closure-compiler

CrossChunkCodeMotion breaks `--chunk_output_type ES_MODULES`

Open
#4,264 2 comments 0 reactions 1 assignee Claimed by @ChadKillingsworth View on GitHub
Dominant language
JavaScript
Stars
7.7k
Forks
1.2k
Avg merge
2d 12h
Merged PRs (30d)
6

Description

Given the following program:
```javascript
// basepoint.js
const foo = {instance: null};

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

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

console.log(getFoo());
```
```javascript
// otherfile.js
import {setFoo} from './basepoint.js';

setFoo('hi from otherfile.js');
```
Running the compiler produces a `JSC_IMPORT_ASSIGN` diagnostic:
```bash
$ 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/
```
```log
/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)
```bash
$ 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
```
```log
// 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
```log

// 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";

// ************************************
```

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.