fix #1812: "catch" scope destructuring edge case

CHANGELOG.md

@@ -24,6 +24,27 @@
 
     Version 0.13.12 introduced simplification of `calc()` expressions in CSS when minifying. For example, `calc(100% / 4)` turns into `25%`. However, this is problematic for numbers with many fractional digits because either the number is printed with reduced precision, which is inaccurate, or the number is printed with full precision, which could be longer than the original expression. For example, turning `calc(100% / 3)` into `33.33333%` is inaccurate and turning it into `33.333333333333336%` likely isn't desired. In this release, minification of `calc()` is now disabled when any number in the result cannot be represented to full precision with at most five fractional digits.
 
+* Fix an edge case with `catch` scope handling ([#1812](https://github.com/evanw/esbuild/issues/1812))
+
+    This release fixes a subtle edge case with `catch` scope and destructuring assignment. Identifiers in computed properties and/or default values inside the destructuring binding pattern should reference the outer scope, not the inner scope. The fix was to split the destructuring pattern into its own scope, separate from the `catch` body. Here's an example of code that was affected by this edge case:
+
+    ```js
+    // Original code
+    let foo = 1
+    try {
+      throw ['a', 'b']
+    } catch ({ [foo]: y }) {
+      let foo = 2
+      assert(y === 'b')
+    }
+
+    // Old output (with --minify)
+    let foo=1;try{throw["a","b"]}catch({[o]:t}){let o=2;assert(t==="b")}
+
+    // New output (with --minify)
+    let foo=1;try{throw["a","b"]}catch({[foo]:t}){let o=2;assert(t==="b")}
+    ```
+
 ## 0.14.1
 
 * Fix `imports` in `package.json` ([#1807](https://github.com/evanw/esbuild/issues/1807))

internal/js_ast/js_ast.go

@@ -1160,9 +1160,10 @@ type SWith struct {
 }
 
 type Catch struct {
-	Loc          logger.Loc
 	BindingOrNil Binding
 	Body         []Stmt
+	Loc          logger.Loc
+	BodyLoc      logger.Loc
 }
 
 type Finally struct {
@@ -1626,6 +1627,7 @@ const (
 	ScopeLabel
 	ScopeClassName
 	ScopeClassBody
+	ScopeCatchBinding
 
 	// The scopes below stop hoisted variables from extending into parent scopes
 	ScopeEntry // This is a module, TypeScript enum, or TypeScript namespace

internal/js_parser/js_parser.go

@@ -1461,6 +1461,18 @@ func (p *parser) canMergeSymbols(scope *js_ast.Scope, existing js_ast.SymbolKind
 	return mergeForbidden
 }
 
+func (p *parser) addSymbolAlreadyDeclaredError(name string, newLoc logger.Loc, oldLoc logger.Loc) {
+	p.log.AddWithNotes(logger.Error, &p.tracker,
+		js_lexer.RangeOfIdentifier(p.source, newLoc),
+		fmt.Sprintf("The symbol %q has already been declared", name),
+
+		[]logger.MsgData{p.tracker.MsgData(
+			js_lexer.RangeOfIdentifier(p.source, oldLoc),
+			fmt.Sprintf("The symbol %q was originally declared here:", name),
+		)},
+	)
+}
+
 func (p *parser) declareSymbol(kind js_ast.SymbolKind, loc logger.Loc, name string) js_ast.Ref {
 	p.checkForUnrepresentableIdentifier(loc, name)
 
@@ -1473,10 +1485,7 @@ func (p *parser) declareSymbol(kind js_ast.SymbolKind, loc logger.Loc, name stri
 
 		switch p.canMergeSymbols(p.currentScope, symbol.Kind, kind) {
 		case mergeForbidden:
-			r := js_lexer.RangeOfIdentifier(p.source, loc)
-			p.log.AddWithNotes(logger.Error, &p.tracker, r, fmt.Sprintf("The symbol %q has already been declared", name),
-				[]logger.MsgData{p.tracker.MsgData(js_lexer.RangeOfIdentifier(p.source, existing.Loc),
-					fmt.Sprintf("The symbol %q was originally declared here:", name))})
+			p.addSymbolAlreadyDeclaredError(name, loc, existing.Loc)
 			return existing.Ref
 
 		case mergeKeepExisting:
@@ -1508,6 +1517,25 @@ func (p *parser) hoistSymbols(scope *js_ast.Scope) {
 		for _, member := range scope.Members {
 			symbol := &p.symbols[member.Ref.InnerIndex]
 
+			// Handle non-hoisted collisions between catch bindings and the catch body.
+			// This implements "B.3.4 VariableStatements in Catch Blocks" from Annex B
+			// of the ECMAScript standard version 6+ (except for the hoisted case, which
+			// is handled later on below):
+			//
+			// * It is a Syntax Error if any element of the BoundNames of CatchParameter
+			//   also occurs in the LexicallyDeclaredNames of Block.
+			//
+			// * It is a Syntax Error if any element of the BoundNames of CatchParameter
+			//   also occurs in the VarDeclaredNames of Block unless CatchParameter is
+			//   CatchParameter : BindingIdentifier .
+			//
+			if scope.Parent.Kind == js_ast.ScopeCatchBinding && symbol.Kind != js_ast.SymbolHoisted {
+				if existingMember, ok := scope.Parent.Members[symbol.OriginalName]; ok {
+					p.addSymbolAlreadyDeclaredError(symbol.OriginalName, member.Loc, existingMember.Loc)
+					continue
+				}
+			}
+
 			if !symbol.Kind.IsHoisted() {
 				continue
 			}
@@ -6300,7 +6328,7 @@ func (p *parser) parseStmt(opts parseStmtOpts) js_ast.Stmt {
 
 		if p.lexer.Token == js_lexer.TCatch {
 			catchLoc := p.lexer.Loc()
-			p.pushScopeForParsePass(js_ast.ScopeBlock, catchLoc)
+			p.pushScopeForParsePass(js_ast.ScopeCatchBinding, catchLoc)
 			p.lexer.Next()
 			var bindingOrNil js_ast.Binding
 
@@ -6332,10 +6360,15 @@ func (p *parser) parseStmt(opts parseStmtOpts) js_ast.Stmt {
 				p.declareBinding(kind, bindingOrNil, parseStmtOpts{})
 			}
 
+			bodyLoc := p.lexer.Loc()
 			p.lexer.Expect(js_lexer.TOpenBrace)
+
+			p.pushScopeForParsePass(js_ast.ScopeBlock, bodyLoc)
 			stmts := p.parseStmtsUpTo(js_lexer.TCloseBrace, parseStmtOpts{})
+			p.popScope()
+
 			p.lexer.Next()
-			catch = &js_ast.Catch{Loc: catchLoc, BindingOrNil: bindingOrNil, Body: stmts}
+			catch = &js_ast.Catch{Loc: catchLoc, BindingOrNil: bindingOrNil, BodyLoc: bodyLoc, Body: stmts}
 			p.popScope()
 		}
 
@@ -9381,11 +9414,15 @@ func (p *parser) visitAndAppendStmt(stmts []js_ast.Stmt, stmt js_ast.Stmt) []js_
 		p.popScope()
 
 		if s.Catch != nil {
-			p.pushScopeForVisitPass(js_ast.ScopeBlock, s.Catch.Loc)
+			p.pushScopeForVisitPass(js_ast.ScopeCatchBinding, s.Catch.Loc)
 			if s.Catch.BindingOrNil.Data != nil {
 				p.visitBinding(s.Catch.BindingOrNil, bindingOpts{})
 			}
+
+			p.pushScopeForVisitPass(js_ast.ScopeBlock, s.Catch.BodyLoc)
 			s.Catch.Body = p.visitStmts(s.Catch.Body, stmtsNormal)
+			p.popScope()
+
 			p.lowerObjectRestInCatchBinding(s.Catch)
 			p.popScope()
 		}

internal/js_parser/js_parser_test.go

@@ -2628,14 +2628,16 @@ func TestCatch(t *testing.T) {
 	expectPrinted(t, "try {} catch (e) { { function e() {} } }", "try {\n} catch (e) {\n  {\n    let e = function() {\n    };\n    var e = e;\n  }\n}\n")
 	expectPrinted(t, "try {} catch (e) { if (1) function e() {} }", "try {\n} catch (e) {\n  if (1) {\n    let e = function() {\n    };\n    var e = e;\n  }\n}\n")
 	expectPrinted(t, "try {} catch (e) { if (0) ; else function e() {} }", "try {\n} catch (e) {\n  if (0)\n    ;\n  else {\n    let e = function() {\n    };\n    var e = e;\n  }\n}\n")
+	expectPrinted(t, "try {} catch ({ e }) { { function e() {} } }", "try {\n} catch ({ e }) {\n  {\n    let e = function() {\n    };\n    var e = e;\n  }\n}\n")
 
 	errorText := `<stdin>: ERROR: The symbol "e" has already been declared
 <stdin>: NOTE: The symbol "e" was originally declared here:
 `
 
 	expectParseError(t, "try {} catch (e) { function e() {} }", errorText)
-	expectParseError(t, "try {} catch ({e}) { var e }", errorText)
-	expectParseError(t, "try {} catch ({e}) { function e() {} }", errorText)
+	expectParseError(t, "try {} catch ({ e }) { var e }", errorText)
+	expectParseError(t, "try {} catch ({ e }) { { var e } }", errorText)
+	expectParseError(t, "try {} catch ({ e }) { function e() {} }", errorText)
 	expectParseError(t, "try {} catch (e) { let e }", errorText)
 	expectParseError(t, "try {} catch (e) { const e = 0 }", errorText)
 }

scripts/end-to-end-tests.js

@@ -2252,6 +2252,41 @@
         if (y + '' !== '1') throw 'fail: ' + y
       `,
     }),
+
+    // https://github.com/evanw/esbuild/issues/1812
+    test(['in.js', '--outfile=node.js'], {
+      'in.js': `
+        let a = 1;
+        let def = "PASS2";
+        try {
+          throw [ "FAIL2", "PASS1" ];
+        } catch ({ [a]: b, 3: d = def }) {
+          let a = 0, def = "FAIL3";
+          if (b !== 'PASS1' || d !== 'PASS2') throw 'fail: ' + b + ' ' + d
+        }
+      `,
+    }),
+    test(['in.js', '--outfile=node.js', '--bundle'], {
+      'in.js': `
+        let a = 1;
+        let def = "PASS2";
+        try {
+          throw [ "FAIL2", "PASS1" ];
+        } catch ({ [a]: b, 3: d = def }) {
+          let a = 0, def = "FAIL3";
+          if (b !== 'PASS1' || d !== 'PASS2') throw 'fail: ' + b + ' ' + d
+        }
+      `,
+    }),
+    test(['in.js', '--outfile=node.js'], {
+      'in.js': `
+        try {
+          throw { x: 'z', z: 123 }
+        } catch ({ x, [x]: y }) {
+          if (y !== 123) throw 'fail'
+        }
+      `,
+    }),
   )
 
   // Test cyclic import issues (shouldn't crash on evaluation)