From 6cbfaca883cc61f709f726cb29037459bb31cfc0 Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Mon, 29 Mar 2021 12:55:16 -0700 Subject: [PATCH] fix #1080: omit esm wrapper calls for dead files --- CHANGELOG.md | 6 ++ internal/bundler/bundler_default_test.go | 5 +- internal/bundler/linker.go | 26 +++-- .../bundler/snapshots/snapshots_default.txt | 6 ++ internal/js_parser/js_parser.go | 6 +- scripts/end-to-end-tests.js | 95 ++++++++++++++++++- 6 files changed, 126 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ba80566b98e..25862b9f0a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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. diff --git a/internal/bundler/bundler_default_test.go b/internal/bundler/bundler_default_test.go index f224f33ecac..2c326c4156a 100644 --- a/internal/bundler/bundler_default_test.go +++ b/internal/bundler/bundler_default_test.go @@ -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": ` @@ -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 -`, }) } diff --git a/internal/bundler/linker.go b/internal/bundler/linker.go index 364ad5b6ca6..726b1767add 100644 --- a/internal/bundler/linker.go +++ b/internal/bundler/linker.go @@ -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 { @@ -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 { @@ -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) { @@ -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 @@ -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 { diff --git a/internal/bundler/snapshots/snapshots_default.txt b/internal/bundler/snapshots/snapshots_default.txt index ac11f4fade0..7d5dbe6c5a7 100644 --- a/internal/bundler/snapshots/snapshots_default.txt +++ b/internal/bundler/snapshots/snapshots_default.txt @@ -2749,6 +2749,12 @@ await foo; for await (foo of bar) ; +================================================================================ +TestTopLevelReturnAllowedImport +---------- /out.js ---------- +return; +import "foo"; + ================================================================================ TestTypeofRequireBadPatterns ---------- /out.js ---------- diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index ac599b20e55..f08acd4ec8c 100644 --- a/internal/js_parser/js_parser.go +++ b/internal/js_parser/js_parser.go @@ -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 diff --git a/scripts/end-to-end-tests.js b/scripts/end-to-end-tests.js index 2379e9ee41c..837b3e2c359 100644 --- a/scripts/end-to-end-tests.js +++ b/scripts/end-to-end-tests.js @@ -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 @@ -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 {