fix #613: imported symbols are side-effect free

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:

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",
+		},
+	})
+}

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 ----------

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)
 

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