fix #3311: pop scope after --drop-labels= runs

evanw · evanw · commit 4d9b764f11b8 · 2023-08-13T21:45:53.000-04:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -18,6 +18,10 @@
 
     Note that this change means esbuild no longer considers any existing browser to support CSS nesting since none of the existing browsers support this new syntax. CSS nesting will now always be transformed when targeting a browser. This situation will change in the future as browsers add support for this new syntax.
 
+* Fix a scope-related bug with `--drop-labels=` ([#3311](https://github.com/evanw/esbuild/issues/3311))
+
+    The recently-released `--drop-labels=` feature previously had a bug where esbuild's internal scope stack wasn't being restored properly when a statement with a label was dropped. This could manifest as a tree-shaking issue, although it's possible that this could have also been causing other subtle problems too. The bug has been fixed in this release.
+
 * Make renamed CSS names unique across entry points ([#3295](https://github.com/evanw/esbuild/issues/3295))
 
     Previously esbuild's generated names for local names in CSS were only unique within a given entry point (or across all entry points when code splitting was enabled). That meant that building multiple entry points with esbuild could result in local names being renamed to the same identifier even when those entry points were built simultaneously within a single esbuild API call. This problem was especially likely to happen with minification enabled. With this release, esbuild will now avoid renaming local names from two separate entry points to the same name if those entry points were built with a single esbuild API call, even when code splitting is disabled.
diff --git a/internal/bundler_tests/bundler_dce_test.go b/internal/bundler_tests/bundler_dce_test.go
@@ -4510,3 +4510,24 @@ func TestRemoveCodeAfterLabelWithReturn(t *testing.T) {
 		},
 	})
 }
+
+func TestDropLabelTreeShakingBugIssue3311(t *testing.T) {
+	dce_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry.js": `
+				const myFunc = ()=> {
+					DROP: {console.log("drop")}
+					console.log("keep")
+				}
+				export default myFunc
+			`,
+		},
+		entryPaths: []string{"/entry.js"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			AbsOutputFile: "/out.js",
+			DropLabels:    []string{"DROP"},
+			OutputFormat:  config.FormatESModule,
+		},
+	})
+}
diff --git a/internal/bundler_tests/snapshots/snapshots_dce.txt b/internal/bundler_tests/snapshots/snapshots_dce.txt
@@ -803,6 +803,18 @@ var keepMe4 = keepMe3();
 var keepMe5 = pure();
 var keepMe6 = some.fn();
 
+================================================================================
+TestDropLabelTreeShakingBugIssue3311
+---------- /out.js ----------
+// entry.js
+var myFunc = () => {
+  console.log("keep");
+};
+var entry_default = myFunc;
+export {
+  entry_default as default
+};
+
 ================================================================================
 TestDropLabels
 ---------- /out.js ----------
diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go
@@ -10089,18 +10089,22 @@ func (p *parser) visitAndAppendStmt(stmts []js_ast.Stmt, stmt js_ast.Stmt) []js_
 			p.currentScope.LabelStmtIsLoop = true
 		}
 
-		// Drop this entire statement if requested
-		if _, ok := p.dropLabelsMap[name]; ok {
-			old := p.isControlFlowDead
+		// If we're dropping this statement, consider control flow to be dead
+		_, shouldDropLabel := p.dropLabelsMap[name]
+		old := p.isControlFlowDead
+		if shouldDropLabel {
 			p.isControlFlowDead = true
-			s.Stmt = p.visitSingleStmt(s.Stmt, stmtsNormal)
-			p.isControlFlowDead = old
-			return stmts
 		}
 
 		s.Stmt = p.visitSingleStmt(s.Stmt, stmtsNormal)
 		p.popScope()
 
+		// Drop this entire statement if requested
+		if shouldDropLabel {
+			p.isControlFlowDead = old
+			return stmts
+		}
+
 		if p.options.minifySyntax {
 			// Optimize "x: break x" which some people apparently write by hand
 			if child, ok := s.Stmt.Data.(*js_ast.SBreak); ok && child.Label != nil && child.Label.Ref == s.Name.Ref {
