fix #3195: tree-shaking bug that didn't keep code

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+* Fix a tree-shaking bug that removed side effects ([#3195](https://github.com/evanw/esbuild/issues/3195))
+
+    This fixes a regression in version 0.18.4 where combining `--minify-syntax` with `--keep-names` could cause expressions with side effects after a function declaration to be considered side-effect free for tree shaking purposes. The reason was because `--keep-names` generates an expression statement containing a call to a helper function after the function declaration with a special flag that makes the function call able to be tree shaken, and then `--minify-syntax` could potentially merge that expression statement with following expressions without clearing the flag. This release fixes the bug by clearing the flag when merging expression statements together.
+
 ## 0.18.9
 
 * Fix `await using` declarations inside `async` generator functions

internal/bundler_tests/bundler_dce_test.go

@@ -4423,3 +4423,27 @@ func TestDCEOfUsingDeclarations(t *testing.T) {
 		},
 	})
 }
+
+func TestDCEOfExprAfterKeepNamesIssue3195(t *testing.T) {
+	dce_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry.js": `
+				(() => {
+					function f() {}
+					firstImportantSideEffect(f());
+				})();
+				(() => {
+					function g() {}
+					debugger;
+					secondImportantSideEffect(g());
+				})();
+			`,
+		},
+		entryPaths: []string{"/entry.js"},
+		options: config.Options{
+			MinifySyntax:  true,
+			KeepNames:     true,
+			AbsOutputFile: "/out.js",
+		},
+	})
+}

internal/bundler_tests/snapshots/snapshots_dce.txt

@@ -505,6 +505,21 @@ __decorateClass([
   __decorateParam(0, fn)
 ], StaticParameter, "foo", 1);
 
+================================================================================
+TestDCEOfExprAfterKeepNamesIssue3195
+---------- /out.js ----------
+(() => {
+  function f() {
+  }
+  __name(f, "f"), firstImportantSideEffect(void 0);
+})(), (() => {
+  function g() {
+  }
+  __name(g, "g");
+  debugger;
+  secondImportantSideEffect(void 0);
+})();
+
 ================================================================================
 TestDCEOfIIFE
 ---------- /out/remove-these.js ----------

internal/js_ast/js_ast_helpers.go

@@ -1859,21 +1859,20 @@ func StmtsCanBeRemovedIfUnused(stmts []Stmt, flags StmtsCanBeRemovedIfUnusedFlag
 			}
 
 		case *SExpr:
-			if s.IsFromClassOrFnThatCanBeRemovedIfUnused {
-				// This statement was automatically generated when lowering a class
-				// or function that we were able to analyze as having no side effects
-				// before lowering. So we consider it to be removable. The assumption
-				// here is that we are seeing at least all of the statements from the
-				// class lowering operation all at once (although we may possibly be
-				// seeing even more statements than that). Since we're making a binary
-				// all-or-nothing decision about the side effects of these statements,
-				// we can safely consider these to be side-effect free because we
-				// aren't in danger of partially dropping some of the class setup code.
-				return true
-			}
-
 			if !ExprCanBeRemovedIfUnused(s.Value, isUnbound) {
-				return false
+				if s.IsFromClassOrFnThatCanBeRemovedIfUnused {
+					// This statement was automatically generated when lowering a class
+					// or function that we were able to analyze as having no side effects
+					// before lowering. So we consider it to be removable. The assumption
+					// here is that we are seeing at least all of the statements from the
+					// class lowering operation all at once (although we may possibly be
+					// seeing even more statements than that). Since we're making a binary
+					// all-or-nothing decision about the side effects of these statements,
+					// we can safely consider these to be side-effect free because we
+					// aren't in danger of partially dropping some of the class setup code.
+				} else {
+					return false
+				}
 			}
 
 		case *SLocal:

internal/js_parser/js_parser.go

@@ -8690,6 +8690,9 @@ func (p *parser) mangleStmts(stmts []js_ast.Stmt, kind stmtsKind) []js_ast.Stmt
 			if len(result) > 0 {
 				prevStmt := result[len(result)-1]
 				if prevS, ok := prevStmt.Data.(*js_ast.SExpr); ok {
+					if !s.IsFromClassOrFnThatCanBeRemovedIfUnused {
+						prevS.IsFromClassOrFnThatCanBeRemovedIfUnused = false
+					}
 					prevS.Value = js_ast.JoinWithComma(prevS.Value, s.Value)
 					continue
 				}

scripts/end-to-end-tests.js

@@ -3145,6 +3145,24 @@ for (const minify of [[], ['--minify-syntax']]) {
         var x = class { static [-0xFFFF_FFFF_FFFF] = 1 }; if (x['-281474976710655'] !== 1) throw 'fail: -0xFFFF_FFFF_FFFF'
       `,
     }),
+
+    // See: https://github.com/evanw/esbuild/issues/3195
+    test(['in.js', '--outfile=node.js', '--keep-names'].concat(minify), {
+      'in.js': `
+        const log = [];
+        const sideEffect = x => log.push(x);
+        (() => {
+          function f() {}
+          sideEffect(1, f());
+        })();
+        (() => {
+          function g() {}
+          debugger;
+          sideEffect(2, g());
+        })();
+        if (log + '' !== '1,2') throw 'fail: ' + log;
+      `,
+    }),
   )
 
   // Check property access simplification