From d5f7fdd8f71ad398dae591d5270aac71bac17590 Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Sun, 20 Dec 2020 21:56:24 -0800 Subject: [PATCH] fix #613: imported symbols are side-effect free --- CHANGELOG.md | 15 ++++++++++++ internal/bundler/bundler_dce_test.go | 24 ++++++++++++++++++++ internal/bundler/snapshots/snapshots_dce.txt | 14 ++++++++++++ internal/js_parser/js_parser.go | 19 ++++++++++++++++ scripts/end-to-end-tests.js | 11 +++++++++ 5 files changed, 83 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 83cfa44f107..f90485f44e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,21 @@ The change in version 0.8.24 to share service instances caused problems for code that calls `process.chdir()` before calling `startService()` to be able to get a service with a different working directory. With this release, calls to `startService()` no longer share the service instance if the working directory was different at the time of creation. +* Consider import references to be side-effect free ([#613](https://github.com/evanw/esbuild/issues/613)) + + This change improves tree shaking for code containing top-level references to imported symbols such as the following code: + + ```js + import {Base} from './base' + export class Derived extends Base {} + ``` + + Identifier references are considered side-effect free if they are locally-defined, but esbuild special-cases identifier references to imported symbols in its AST (the identifier `Base` in this example). This meant they did not trigger this check and so were not considered locally-defined and therefore side-effect free. That meant that `Derived` in this example would never be tree-shaken. + + The reason for this is that the side-effect determination is made during parsing and during parsing it's not yet known if `./base` is a CommonJS module or not. If it is, then `Base` would be a dynamic run-time property access on `exports.Base` which could hypothetically be a property with a getter that has side effects. Therefore it could be considered incorrect to remove this code due to tree-shaking because there is technically a side effect. + + However, this is a very unlikely edge case and not tree-shaking this code violates developer expectations. So with this release, esbuild will always consider references to imported symbols as being side-effect free. This also aligns with ECMAScript module semantics because with ECMAScript modules, it's impossible to have a user-defined getter for an imported symbol. This means esbuild will now tree-shake unused code in cases like this. + * Warn about calling an import namespace object The following code is an invalid use of an import statement: diff --git a/internal/bundler/bundler_dce_test.go b/internal/bundler/bundler_dce_test.go index 45cadc4c7b7..a97f281c2b9 100644 --- a/internal/bundler/bundler_dce_test.go +++ b/internal/bundler/bundler_dce_test.go @@ -1187,3 +1187,27 @@ func TestImportReExportOfNamespaceImport(t *testing.T) { }, }) } + +func TestTreeShakingImportIdentifier(t *testing.T) { + dce_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.js": ` + import * as a from './a' + new a.Keep() + `, + "/a.js": ` + import * as b from './b' + export class Keep extends b.Base {} + export class REMOVE extends b.Base {} + `, + "/b.js": ` + export class Base {} + `, + }, + entryPaths: []string{"/entry.js"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputFile: "/out.js", + }, + }) +} diff --git a/internal/bundler/snapshots/snapshots_dce.txt b/internal/bundler/snapshots/snapshots_dce.txt index 80435c5985a..ad732bdb7e1 100644 --- a/internal/bundler/snapshots/snapshots_dce.txt +++ b/internal/bundler/snapshots/snapshots_dce.txt @@ -444,6 +444,20 @@ TestTextLoaderRemoveUnused // entry.js console.log("unused import"); +================================================================================ +TestTreeShakingImportIdentifier +---------- /out.js ---------- +// b.js +var Base = class { +}; + +// a.js +var Keep = class extends Base { +}; + +// entry.js +new Keep(); + ================================================================================ TestTreeShakingReactElements ---------- /out.js ---------- diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index 1659e14c6cf..f40a1004b5c 100644 --- a/internal/js_parser/js_parser.go +++ b/internal/js_parser/js_parser.go @@ -10654,6 +10654,25 @@ func (p *parser) exprCanBeRemovedIfUnused(expr js_ast.Expr) bool { return true } + case *js_ast.EImportIdentifier: + // References to an ES6 import item are always side-effect free in an + // ECMAScript environment. + // + // They could technically have side effects if the imported module is a + // CommonJS module and the import item was translated to a property access + // (which esbuild's bundler does) and the property has a getter with side + // effects. + // + // But this is very unlikely and respecting this edge case would mean + // disabling tree shaking of all code that references an export from a + // CommonJS module. It would also likely violate the expectations of some + // developers because the code *looks* like it should be able to be tree + // shaken. + // + // So we deliberately ignore this edge case and always treat import item + // references as being side-effect free. + return true + case *js_ast.EIf: return p.exprCanBeRemovedIfUnused(e.Test) && p.exprCanBeRemovedIfUnused(e.Yes) && p.exprCanBeRemovedIfUnused(e.No) diff --git a/scripts/end-to-end-tests.js b/scripts/end-to-end-tests.js index b0ba5505fca..b0561359730 100644 --- a/scripts/end-to-end-tests.js +++ b/scripts/end-to-end-tests.js @@ -1256,6 +1256,17 @@ 'foo/index.js': `global.dce5 = 123; exports.abc = 'abc'`, 'foo/package.json': `{ "sideEffects": true }`, }), + + // Note: Tree shaking this could technically be considered incorrect because + // the import is for a property whose getter in this case has a side effect. + // However, this is very unlikely and the vast majority of the time people + // would likely rather have the code be tree-shaken. This test case enforces + // the technically incorrect behavior as documentation that this edge case + // is being ignored. + test(['--bundle', 'entry.js', '--outfile=node.js'], { + 'entry.js': `import {foo, bar} from './foo'; let unused = foo; if (bar) throw 'expected "foo" to be tree-shaken'`, + 'foo.js': `module.exports = {get foo() { module.exports.bar = 1 }, bar: 0}`, + }), ) // Test obscure CommonJS symbol edge cases