fix #648: skip some implicit return optimizations

CHANGELOG.md

@@ -2,6 +2,18 @@
 
 ## Unreleased
 
+* Fix minification issue from previous release ([#648](https://github.com/evanw/esbuild/issues/648))
+
+    The minification optimization to omit certain `continue` and `return` statements when it's implied by control flow in version 0.8.29 caused a regression when the branch condition uses a hoisted function:
+
+    ```js
+    if (fn()) return;
+    ...
+    function fn() {}
+    ```
+
+    In that case, transforming the code by inverting the condition and moving the following statements inside the branch is not valid because the function is no longer hoisted to above the branch condition. This release fixes the regression by avoiding this optimization in cases like this.
+
 * Tree-shake unused code with `--format=iife` ([#639](https://github.com/evanw/esbuild/issues/639))
 
     When the output format is IIFE (which wraps the code in an immediately-invoked function expression), esbuild now assumes that it's safe to remove unused code. This is an assumption that esbuild always makes when bundling but that esbuild previously didn't make when not bundling. Now esbuild will remove code even when not bundling as long as the output format is IIFE.

internal/js_parser/js_parser.go

@@ -6363,15 +6363,39 @@ func (p *parser) mangleStmts(stmts []js_ast.Stmt, kind stmtsKind) []js_ast.Stmt
 						body = append(body, *s.No)
 					}
 					body = append(body, stmts[i+1:]...)
-					body = p.mangleStmts(body, kind)
-					bodyLoc := s.Yes.Loc
-					if len(body) > 0 {
-						bodyLoc = body[0].Loc
+
+					// Don't do this transformation if the branch condition could
+					// potentially access symbols declared later on on this scope below.
+					// If so, inverting the branch condition and nesting statements after
+					// this in a block would break that access which is a behavior change.
+					//
+					//   // This transformation is incorrect
+					//   if (a()) return; function a() {}
+					//   if (!a()) { function a() {} }
+					//
+					//   // This transformation is incorrect
+					//   if (a(() => b)) return; let b;
+					//   if (a(() => b)) { let b; }
+					//
+					canMoveBranchConditionOutsideScope := true
+					for _, stmt := range body {
+						if statementCaresAboutScope(stmt) {
+							canMoveBranchConditionOutsideScope = false
+							break
+						}
+					}
+
+					if canMoveBranchConditionOutsideScope {
+						body = p.mangleStmts(body, kind)
+						bodyLoc := s.Yes.Loc
+						if len(body) > 0 {
+							bodyLoc = body[0].Loc
+						}
+						return p.mangleIf(result, stmt.Loc, &js_ast.SIf{
+							Test: js_ast.Not(s.Test),
+							Yes:  stmtsToSingleStmt(bodyLoc, body),
+						}, mangleIfOpts{})
 					}
-					return p.mangleIf(result, stmt.Loc, &js_ast.SIf{
-						Test: js_ast.Not(s.Test),
-						Yes:  stmtsToSingleStmt(bodyLoc, body),
-					}, mangleIfOpts{})
 				}
 
 				if s.No != nil {

internal/js_parser/js_parser_test.go

@@ -1745,6 +1745,11 @@ func TestMangleLoopJump(t *testing.T) {
 	expectPrintedMangle(t, "while (x) { t(); if (y) continue; else z(); w(); }", "for (; x; )\n  t(), !y && (z(), w());\n")
 	expectPrintedMangle(t, "while (x) { debugger; if (y) continue; z(); }", "for (; x; ) {\n  debugger;\n  y || z();\n}\n")
 	expectPrintedMangle(t, "while (x) { debugger; if (y) continue; else z(); w(); }", "for (; x; ) {\n  debugger;\n  y || (z(), w());\n}\n")
+
+	// Do not optimize implicit continue for statements that care about scope
+	expectPrintedMangle(t, "while (x) { if (y) continue; function y() {} }", "for (; x; ) {\n  if (y)\n    continue;\n  function y() {\n  }\n}\n")
+	expectPrintedMangle(t, "while (x) { if (y) continue; let y }", "for (; x; ) {\n  if (y)\n    continue;\n  let y;\n}\n")
+	expectPrintedMangle(t, "while (x) { if (y) continue; var y }", "for (; x; )\n  if (!y)\n    var y;\n")
 }
 
 func TestMangleUndefined(t *testing.T) {
@@ -2075,6 +2080,11 @@ func TestMangleReturn(t *testing.T) {
 		"function x() {\n  !(y && z);\n}\n")
 	expectPrintedMangle(t, "function x() { if (y) { if (z) return; w(); } }",
 		"function x() {\n  if (y) {\n    if (z)\n      return;\n    w();\n  }\n}\n")
+
+	// Do not optimize implicit return for statements that care about scope
+	expectPrintedMangle(t, "function x() { if (y) return; function y() {} }", "function x() {\n  if (y)\n    return;\n  function y() {\n  }\n}\n")
+	expectPrintedMangle(t, "function x() { if (y) return; let y }", "function x() {\n  if (y)\n    return;\n  let y;\n}\n")
+	expectPrintedMangle(t, "function x() { if (y) return; var y }", "function x() {\n  if (!y)\n    var y;\n}\n")
 }
 
 func TestMangleThrow(t *testing.T) {

scripts/end-to-end-tests.js

@@ -998,6 +998,18 @@
     }),
   )
 
+  // Test certain minification transformations
+  for (const minify of [[], ['--minify-syntax']]) {
+    tests.push(
+      test(['in.js', '--outfile=node.js'].concat(minify), {
+        'in.js': `let fn = (x) => { if (x && y) return; function y() {} throw 'fail' }; fn(fn)`,
+      }),
+      test(['in.js', '--outfile=node.js'].concat(minify), {
+        'in.js': `let fn = (a, b) => { if (a && (x = () => y) && b) return; var x; let y = 123; if (x() !== 123) throw 'fail' }; fn(fn)`,
+      }),
+    )
+  }
+
   // Test minification of top-level symbols
   tests.push(
     test(['in.js', '--outfile=node.js', '--minify'], {