fix #4353: remove empty try/finally clauses

Author: evanw

CHANGELOG.md

@@ -27,6 +27,28 @@
     a:e:try{for(;;)break a}catch{break e}
     ```
 
+* The minifier now strips empty `finally` clauses ([#4353](https://github.com/evanw/esbuild/issues/4353))
+
+    This improvement means that `finally` clauses containing dead code can potentially cause the associated `try` statement to be removed from the output entirely in minified builds:
+
+    ```js
+    // Original code
+    function foo(callback) {
+      if (DEBUG) stack.push(callback.name);
+      try {
+        callback();
+      } finally {
+        if (DEBUG) stack.pop();
+      }
+    }
+
+    // Old output (with --minify --define:DEBUG=false)
+    function foo(a){try{a()}finally{}}
+
+    // New output (with --minify --define:DEBUG=false)
+    function foo(a){a()}
+    ```
+
 * Allow tree-shaking of the `Symbol` constructor
 
     With this release, calling `Symbol` is now considered to be side-effect free when the argument is known to be a primitive value. This means esbuild can now tree-shake module-level symbol variables:

internal/js_parser/js_parser.go

@@ -11084,41 +11084,62 @@ func (p *parser) visitAndAppendStmt(stmts []js_ast.Stmt, stmt js_ast.Stmt) []js_
 			p.popScope()
 		}
 
-		// Drop the whole thing if the try body is empty
-		if p.options.minifySyntax && len(s.Block.Stmts) == 0 {
-			keepCatch := false
+		if p.options.minifySyntax {
+			if len(s.Block.Stmts) == 0 {
+				// Try to drop the whole thing if the try body is empty
+				keepCatch := false
 
-			// Certain "catch" blocks need to be preserved:
-			//
-			//   try {} catch { let foo } // Can be removed
-			//   try {} catch { var foo } // Must be kept
-			//
-			if s.Catch != nil {
-				for _, stmt2 := range s.Catch.Block.Stmts {
-					if shouldKeepStmtInDeadControlFlow(stmt2) {
-						keepCatch = true
-						break
+				// Certain "catch" blocks need to be preserved:
+				//
+				//   try {} catch { let foo } // Can be removed
+				//   try {} catch { var foo } // Must be kept
+				//
+				if s.Catch != nil {
+					for _, stmt2 := range s.Catch.Block.Stmts {
+						if shouldKeepStmtInDeadControlFlow(stmt2) {
+							keepCatch = true
+							break
+						}
 					}
 				}
-			}
 
-			// Make sure to preserve the "finally" block if present
-			if !keepCatch {
-				if s.Finally == nil {
-					return stmts
-				}
-				finallyNeedsBlock := false
-				for _, stmt2 := range s.Finally.Block.Stmts {
-					if statementCaresAboutScope(stmt2) {
-						finallyNeedsBlock = true
-						break
+				// Make sure to preserve the "finally" block if present
+				if !keepCatch {
+					if s.Finally == nil {
+						return stmts
+					}
+					finallyNeedsBlock := false
+					for _, stmt2 := range s.Finally.Block.Stmts {
+						if statementCaresAboutScope(stmt2) {
+							finallyNeedsBlock = true
+							break
+						}
 					}
+					if !finallyNeedsBlock {
+						return append(stmts, s.Finally.Block.Stmts...)
+					}
+					block := s.Finally.Block
+					stmt = js_ast.Stmt{Loc: s.Finally.Loc, Data: &block}
 				}
-				if !finallyNeedsBlock {
-					return append(stmts, s.Finally.Block.Stmts...)
+			} else if s.Finally != nil && len(s.Finally.Block.Stmts) == 0 {
+				if s.Catch != nil {
+					// Just remove the "finally" block if there's a "catch"
+					s.Finally = nil
+				} else {
+					// Otherwise, try to unwrap the whole "try" statement
+					tryNeedsBlock := false
+					for _, stmt2 := range s.Block.Stmts {
+						if statementCaresAboutScope(stmt2) {
+							tryNeedsBlock = true
+							break
+						}
+					}
+					if !tryNeedsBlock {
+						return append(stmts, s.Block.Stmts...)
+					}
+					block := s.Block
+					stmt = js_ast.Stmt{Loc: s.Finally.Loc, Data: &block}
 				}
-				block := s.Finally.Block
-				stmt = js_ast.Stmt{Loc: s.Finally.Loc, Data: &block}
 			}
 		}
 

internal/js_parser/js_parser_test.go

@@ -6665,6 +6665,18 @@ func TestMangleTry(t *testing.T) {
 	expectPrintedMangle(t, "try {} finally { let x = foo() }", "{\n  let x = foo();\n}\n")
 	expectPrintedMangle(t, "try {} catch (e) { foo() } finally { let x = bar() }", "{\n  let x = bar();\n}\n")
 
+	expectPrintedMangle(t, "try { foo() } catch {}", "try {\n  foo();\n} catch {\n}\n")
+	expectPrintedMangle(t, "try { foo() } catch {} finally {}", "try {\n  foo();\n} catch {\n}\n")
+	expectPrintedMangle(t, "try { foo() } finally {}", "foo();\n")
+
+	expectPrintedMangle(t, "try { var x = foo() } catch {}", "try {\n  var x = foo();\n} catch {\n}\n")
+	expectPrintedMangle(t, "try { var x = foo() } catch {} finally {}", "try {\n  var x = foo();\n} catch {\n}\n")
+	expectPrintedMangle(t, "try { var x = foo() } finally {}", "var x = foo();\n")
+
+	expectPrintedMangle(t, "try { let x = foo() } catch {}", "try {\n  let x = foo();\n} catch {\n}\n")
+	expectPrintedMangle(t, "try { let x = foo() } catch {} finally {}", "try {\n  let x = foo();\n} catch {\n}\n")
+	expectPrintedMangle(t, "try { let x = foo() } finally {}", "{\n  let x = foo();\n}\n")
+
 	// The Kotlin compiler apparently generates code like this.
 	// See https://github.com/evanw/esbuild/issues/4064 for info.
 	expectPrintedMangle(t, "x: try { while (true) ; break x } catch {}", "x: try {\n  for (; ; ) ;\n  break x;\n} catch {\n}\n")