fix #2080 fix #2085 fix #2098 fix #2099: var bug

evanw · Mar 13, 2022 · ea3b3d3 · ea3b3d3
1 parent 9fe4a61
commit ea3b3d3
Show file tree

Hide file tree

Showing 4 changed files with 203 additions and 2 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,45 @@
 
 ## Unreleased
 
+* Fix a tree shaking regression regarding `var` declarations ([#2080](https://github.com/evanw/esbuild/issues/2080), [#2085](https://github.com/evanw/esbuild/pull/2085), [#2098](https://github.com/evanw/esbuild/issues/2098), [#2099](https://github.com/evanw/esbuild/issues/2099))
+
+    Version 0.14.8 of esbuild enabled removal of duplicate function declarations when minification is enabled (see [#610](https://github.com/evanw/esbuild/issues/610)):
+
+    ```js
+    // Original code
+    function x() { return 1 }
+    console.log(x())
+    function x() { return 2 }
+
+    // Output (with --minify-syntax)
+    console.log(x());
+    function x() {
+      return 2;
+    }
+    ```
+
+    This transformation is safe because function declarations are "hoisted" in JavaScript, which means they are all done first before any other code is evaluted. This means the last function declaration will overwrite all previous function declarations with the same name.
+
+    However, this introduced an unintentional regression for `var` declarations in which all but the last declaration was dropped if tree-shaking was enabled. This only happens for top-level `var` declarations that re-declare the same variable multiple times. This regression has now been fixed:
+
+    ```js
+    // Original code
+    var x = 1
+    console.log(x)
+    var x = 2
+
+    // Old output (with --tree-shaking=true)
+    console.log(x);
+    var x = 2;
+
+    // New output (with --tree-shaking=true)
+    var x = 1;
+    console.log(x);
+    var x = 2;
+    ```
+
+    This case now has test coverage.
+
 * Add support for parsing "instantiation expressions" from TypeScript 4.7 ([#2038](https://github.com/evanw/esbuild/pull/2038))
 
     The upcoming version of TypeScript now lets you specify `<...>` type parameters on a JavaScript identifier without using a call expression:

diff --git a/internal/bundler/bundler_dce_test.go b/internal/bundler/bundler_dce_test.go
@@ -3016,3 +3016,87 @@ func TestCrossModuleConstantFolding(t *testing.T) {
 		},
 	})
 }
+
+func TestMultipleDeclarationTreeShaking(t *testing.T) {
+	dce_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/var2.js": `
+				var x = 1
+				console.log(x)
+				var x = 2
+			`,
+			"/var3.js": `
+				var x = 1
+				console.log(x)
+				var x = 2
+				console.log(x)
+				var x = 3
+			`,
+			"/function2.js": `
+				function x() { return 1 }
+				console.log(x())
+				function x() { return 2 }
+			`,
+			"/function3.js": `
+				function x() { return 1 }
+				console.log(x())
+				function x() { return 2 }
+				console.log(x())
+				function x() { return 3 }
+			`,
+		},
+		entryPaths: []string{
+			"/var2.js",
+			"/var3.js",
+			"/function2.js",
+			"/function3.js",
+		},
+		options: config.Options{
+			Mode:         config.ModeBundle,
+			AbsOutputDir: "/out",
+			MinifySyntax: false,
+		},
+	})
+}
+
+func TestMultipleDeclarationTreeShakingMinifySyntax(t *testing.T) {
+	dce_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/var2.js": `
+				var x = 1
+				console.log(x)
+				var x = 2
+			`,
+			"/var3.js": `
+				var x = 1
+				console.log(x)
+				var x = 2
+				console.log(x)
+				var x = 3
+			`,
+			"/function2.js": `
+				function x() { return 1 }
+				console.log(x())
+				function x() { return 2 }
+			`,
+			"/function3.js": `
+				function x() { return 1 }
+				console.log(x())
+				function x() { return 2 }
+				console.log(x())
+				function x() { return 3 }
+			`,
+		},
+		entryPaths: []string{
+			"/var2.js",
+			"/var3.js",
+			"/function2.js",
+			"/function3.js",
+		},
+		options: config.Options{
+			Mode:         config.ModeBundle,
+			AbsOutputDir: "/out",
+			MinifySyntax: true,
+		},
+	})
+}
diff --git a/internal/bundler/snapshots/snapshots_dce.txt b/internal/bundler/snapshots/snapshots_dce.txt
@@ -894,6 +894,77 @@ TestJSONLoaderRemoveUnused
 // entry.js
 console.log("unused import");
 
+================================================================================
+TestMultipleDeclarationTreeShaking
+---------- /out/var2.js ----------
+// var2.js
+var x = 1;
+console.log(x);
+var x = 2;
+
+---------- /out/var3.js ----------
+// var3.js
+var x = 1;
+console.log(x);
+var x = 2;
+console.log(x);
+var x = 3;
+
+---------- /out/function2.js ----------
+// function2.js
+function x() {
+  return 1;
+}
+console.log(x());
+function x() {
+  return 2;
+}
+
+---------- /out/function3.js ----------
+// function3.js
+function x() {
+  return 1;
+}
+console.log(x());
+function x() {
+  return 2;
+}
+console.log(x());
+function x() {
+  return 3;
+}
+
+================================================================================
+TestMultipleDeclarationTreeShakingMinifySyntax
+---------- /out/var2.js ----------
+// var2.js
+var x = 1;
+console.log(x);
+var x = 2;
+
+---------- /out/var3.js ----------
+// var3.js
+var x = 1;
+console.log(x);
+var x = 2;
+console.log(x);
+var x = 3;
+
+---------- /out/function2.js ----------
+// function2.js
+console.log(x());
+function x() {
+  return 2;
+}
+
+---------- /out/function3.js ----------
+// function3.js
+console.log(x());
+console.log(x());
+function x() {
+  return 3;
+}
+
 ================================================================================
 TestPackageJsonSideEffectsArrayGlob
 ---------- /out.js ----------

diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go
@@ -15303,8 +15303,15 @@ func (p *parser) toAST(parts []js_ast.Part, hashbang string, directive string) j
 		for partIndex, part := range parts {
 			for _, declared := range part.DeclaredSymbols {
 				if declared.IsTopLevel {
-					p.topLevelSymbolToParts[declared.Ref] = append(
-						p.topLevelSymbolToParts[declared.Ref], uint32(partIndex))
+					// If this symbol was merged, use the symbol at the end of the
+					// linked list in the map. This is the case for multiple "var"
+					// declarations with the same name, for example.
+					ref := declared.Ref
+					for p.symbols[ref.InnerIndex].Link != js_ast.InvalidRef {
+						ref = p.symbols[ref.InnerIndex].Link
+					}
+					p.topLevelSymbolToParts[ref] = append(
+						p.topLevelSymbolToParts[ref], uint32(partIndex))
 				}
 			}
 		}