Skip to content

Commit

Permalink
fix #1080: omit esm wrapper calls for dead files
Browse files Browse the repository at this point in the history
  • Loading branch information
@evanw
evanw committed Mar 29, 2021
1 parent e402940 commit 6cbfaca
Show file tree
Hide file tree
Showing 6 changed files with 126 additions and 18 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@

Internal `import()` of a CommonJS module inside the bundle turns into a call to `Promise.resolve().then(() => require())`. However, a space was not inserted before the `Promise` token when minifying, which could lead to a syntax error. This bug has been fixed.

* Fix code generation for unused imported files without side effects ([#1080](https://github.com/evanw/esbuild/issues/1080))

When esbuild adds a wrapping closure around a file to turn it from a statically-initialized file to a dynamically-initialized file, it also needs to turn import statements in other files that import the wrapped file into calls to the wrapper so that the wrapped file is initialized in the correct ordering. However, although tree-shaking is disabled for wrapped CommonJS files because CommonJS exports are dynamic, tree-shaking is still enabled for wrapped ESM files because ESM exports are static.

This caused a bug when files that have been marked with [`"sideEffects": false`](https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free) end up being completely unused in the resulting bundle. In that case the file is removed entirely, but esbuild was still turning `import` statements to that file into calls to the ESM wrapper. These wrapper calls should instead be omitted if the file was completely removed from the bundle as dead code. This bug has been fixed.

## 0.11.0

**This release contains backwards-incompatible changes.** Since esbuild is before version 1.0.0, these changes have been released as a new minor version to reflect this (as [recommended by npm](https://docs.npmjs.com/cli/v6/using-npm/semver/)). You should either be pinning the exact version of `esbuild` in your `package.json` file or be using a version range syntax that only accepts patch upgrades such as `~0.10.0`. See the documentation about [semver](https://docs.npmjs.com/cli/v6/using-npm/semver/) for more information.
Expand Down
5 changes: 1 addition & 4 deletions internal/bundler/bundler_default_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1703,7 +1703,7 @@ func TestRuntimeNameCollisionNoBundle(t *testing.T) {
})
}

func TestTopLevelReturnForbiddenImport(t *testing.T) {
func TestTopLevelReturnAllowedImport(t *testing.T) {
default_suite.expectBundled(t, bundled{
files: map[string]string{
"/entry.js": `
Expand All @@ -1716,9 +1716,6 @@ func TestTopLevelReturnForbiddenImport(t *testing.T) {
Mode: config.ModePassThrough,
AbsOutputFile: "/out.js",
},
expectedScanLog: `entry.js: error: Top-level return cannot be used inside an ECMAScript module
entry.js: note: This file is considered an ECMAScript module because of the "import" keyword here
`,
})
}

Expand Down
26 changes: 18 additions & 8 deletions internal/bundler/linker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,13 @@ func (bs bitSet) copyFrom(other bitSet) {
copy(bs.entries, other.entries)
}

func (bs *bitSet) bitwiseOrWith(other bitSet) {
for i := range bs.entries {
bs.entries[i] |= other.entries[i]
func (bs *bitSet) isAllZeros() bool {
for _, v := range bs.entries {
if v != 0 {
return false
}
}
return true
}

type linkerContext struct {
Expand Down Expand Up @@ -2791,7 +2794,6 @@ func sanitizeFilePathForVirtualModulePath(path string) string {
func (c *linkerContext) computeChunks() []chunkInfo {
jsChunks := make(map[string]chunkInfo)
cssChunks := make(map[string]chunkInfo)
neverReachedKey := string(newBitSet(uint(len(c.entryPoints))).entries)

// Create chunks for entry points
for i, entryPoint := range c.entryPoints {
Expand Down Expand Up @@ -2823,11 +2825,11 @@ func (c *linkerContext) computeChunks() []chunkInfo {
// Figure out which files are in which chunk
for _, sourceIndex := range c.reachableFiles {
file := &c.files[sourceIndex]
key := string(file.entryBits.entries)
if key == neverReachedKey {
// Ignore this file if it was never reached
if file.entryBits.isAllZeros() {
// Ignore this file if it's not included in the bundle
continue
}
key := string(file.entryBits.entries)
var chunk chunkInfo
var ok bool
switch file.repr.(type) {
Expand Down Expand Up @@ -3155,7 +3157,8 @@ func (c *linkerContext) shouldRemoveImportExportStmt(
return true
}

otherRepr := c.files[record.SourceIndex.GetIndex()].repr.(*reprJS)
otherFile := &c.files[record.SourceIndex.GetIndex()]
otherRepr := otherFile.repr.(*reprJS)
switch otherRepr.meta.wrap {
case wrapNone:
// Remove the statement entirely if this module is not wrapped
Expand All @@ -3171,6 +3174,13 @@ func (c *linkerContext) shouldRemoveImportExportStmt(
})

case wrapESM:
// Ignore this file if it's not included in the bundle. This can happen for
// wrapped ESM files but not for wrapped CommonJS files because we allow
// tree shaking inside wrapped ESM files.
if otherFile.entryBits.isAllZeros() {
break
}

// Replace the statement with a call to "init()"
value := js_ast.Expr{Loc: loc, Data: &js_ast.ECall{Target: js_ast.Expr{Loc: loc, Data: &js_ast.EIdentifier{Ref: otherRepr.ast.WrapperRef}}}}
if otherRepr.meta.isAsyncOrHasAsyncDependency {
Expand Down
6 changes: 6 additions & 0 deletions internal/bundler/snapshots/snapshots_default.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2749,6 +2749,12 @@ await foo;
for await (foo of bar)
;

================================================================================
TestTopLevelReturnAllowedImport
---------- /out.js ----------
return;
import "foo";

================================================================================
TestTypeofRequireBadPatterns
---------- /out.js ----------
Expand Down
6 changes: 2 additions & 4 deletions internal/js_parser/js_parser.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8187,12 +8187,10 @@ func (p *parser) visitAndAppendStmt(stmts []js_ast.Stmt, stmt js_ast.Stmt) []js_
s.Value = p.visitExpr(s.Value)

case *js_ast.SReturn:
// Forbid top-level return inside ECMAScript modules
// Forbid top-level return inside modules with ECMAScript-style exports
if p.fnOrArrowDataVisit.isOutsideFnOrArrow {
var where logger.Range
if p.es6ImportKeyword.Len > 0 {
where = p.es6ImportKeyword
} else if p.es6ExportKeyword.Len > 0 {
if p.es6ExportKeyword.Len > 0 {
where = p.es6ExportKeyword
} else if p.topLevelAwaitKeyword.Len > 0 {
where = p.topLevelAwaitKeyword
Expand Down
95 changes: 93 additions & 2 deletions scripts/end-to-end-tests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1382,7 +1382,97 @@
'in.js': `
return import('./in.js')
`,
})
}),
)

// This shouldn't crash
// https://github.com/evanw/esbuild/issues/1080
tests.push(
// Various CommonJS cases
test(['in.js', '--outfile=node.js', '--bundle', '--log-level=error'], {
'in.js': `import "pkg"; return`,
'node_modules/pkg/package.json': `{ "sideEffects": false }`,
'node_modules/pkg/index.js': `module.exports = null; throw 'fail'`,
}),
test(['in.js', '--outfile=node.js', '--define:foo={"x":0}', '--bundle'], {
'in.js': `if (foo.x !== 0) throw 'fail'; return`,
}),
test(['in.js', '--outfile=node.js', '--define:foo.bar={"x":0}', '--bundle'], {
'in.js': `if (foo.bar.x !== 0) throw 'fail'; return`,
}),
test(['in.js', '--outfile=node.js', '--define:module={"x":0}', '--bundle'], {
'in.js': `if (module.x !== void 0) throw 'fail'; return`,
}),
test(['in.js', '--outfile=node.js', '--define:module.foo={"x":0}', '--bundle'], {
'in.js': `if (module.foo !== void 0) throw 'fail'; return`,
}),
test(['in.js', '--outfile=node.js', '--define:exports={"x":0}', '--bundle'], {
'in.js': `if (exports.x !== void 0) throw 'fail'; return`,
}),
test(['in.js', '--outfile=node.js', '--define:exports.foo={"x":0}', '--bundle'], {
'in.js': `if (exports.foo !== void 0) throw 'fail'; return`,
}),
test(['in.js', '--outfile=node.js', '--define:foo=["x"]', '--bundle'], {
'in.js': `if (foo[0] !== 'x') throw 'fail'; return`,
}),
test(['in.js', '--outfile=node.js', '--define:foo.bar=["x"]', '--bundle'], {
'in.js': `if (foo.bar[0] !== 'x') throw 'fail'; return`,
}),
test(['in.js', '--outfile=node.js', '--define:module=["x"]', '--bundle'], {
'in.js': `if (module[0] !== void 0) throw 'fail'; return`,
}),
test(['in.js', '--outfile=node.js', '--define:module.foo=["x"]', '--bundle'], {
'in.js': `if (module.foo !== void 0) throw 'fail'; return`,
}),
test(['in.js', '--outfile=node.js', '--define:exports=["x"]', '--bundle'], {
'in.js': `if (exports[0] !== void 0) throw 'fail'; return`,
}),
test(['in.js', '--outfile=node.js', '--define:exports.foo=["x"]', '--bundle'], {
'in.js': `if (exports.foo !== void 0) throw 'fail'; return`,
}),

// Various ESM cases
test(['in.js', '--outfile=node.js', '--bundle', '--log-level=error'], {
'in.js': `import "pkg"`,
'node_modules/pkg/package.json': `{ "sideEffects": false }`,
'node_modules/pkg/index.js': `module.exports = null; throw 'fail'`,
}),
test(['in.js', '--outfile=node.js', '--define:foo={"x":0}', '--bundle'], {
'in.js': `if (foo.x !== 0) throw 'fail'; export {}`,
}),
test(['in.js', '--outfile=node.js', '--define:foo.bar={"x":0}', '--bundle'], {
'in.js': `if (foo.bar.x !== 0) throw 'fail'; export {}`,
}),
test(['in.js', '--outfile=node.js', '--define:module={"x":0}', '--bundle'], {
'in.js': `if (module.x !== 0) throw 'fail'; export {}`,
}),
test(['in.js', '--outfile=node.js', '--define:module.foo={"x":0}', '--bundle'], {
'in.js': `if (module.foo.x !== 0) throw 'fail'; export {}`,
}),
test(['in.js', '--outfile=node.js', '--define:exports={"x":0}', '--bundle'], {
'in.js': `if (exports.x !== 0) throw 'fail'; export {}`,
}),
test(['in.js', '--outfile=node.js', '--define:exports.foo={"x":0}', '--bundle'], {
'in.js': `if (exports.foo.x !== 0) throw 'fail'; export {}`,
}),
test(['in.js', '--outfile=node.js', '--define:foo=["x"]', '--bundle'], {
'in.js': `if (foo[0] !== 'x') throw 'fail'; export {}`,
}),
test(['in.js', '--outfile=node.js', '--define:foo.bar=["x"]', '--bundle'], {
'in.js': `if (foo.bar[0] !== 'x') throw 'fail'; export {}`,
}),
test(['in.js', '--outfile=node.js', '--define:module=["x"]', '--bundle'], {
'in.js': `if (module[0] !== 'x') throw 'fail'; export {}`,
}),
test(['in.js', '--outfile=node.js', '--define:module.foo=["x"]', '--bundle'], {
'in.js': `if (module.foo[0] !== 'x') throw 'fail'; export {}`,
}),
test(['in.js', '--outfile=node.js', '--define:exports=["x"]', '--bundle'], {
'in.js': `if (exports[0] !== 'x') throw 'fail'; export {}`,
}),
test(['in.js', '--outfile=node.js', '--define:exports.foo=["x"]', '--bundle'], {
'in.js': `if (exports.foo[0] !== 'x') throw 'fail'; export {}`,
}),
)

// Tests for "arguments" scope issues
Expand Down Expand Up @@ -3733,7 +3823,8 @@
// If the test doesn't specify a format, test both formats
for (const format of formats) {
const formatArg = `--format=${format}`
const modifiedArgs = (!hasBundle || args.includes(formatArg) ? args : args.concat(formatArg)).concat('--log-level=warning')
const logLevelArgs = args.some(arg => arg.startsWith('--log-level=')) ? [] : ['--log-level=warning']
const modifiedArgs = (!hasBundle || args.includes(formatArg) ? args : args.concat(formatArg)).concat(logLevelArgs)
const thisTestDir = path.join(testDir, '' + testCount++)

try {
Expand Down

0 comments on commit 6cbfaca

Please sign in to comment.