fix direct "eval" variable renaming edge case

CHANGELOG.md

@@ -1,5 +1,51 @@
 # Changelog
 
+## Unreleased
+
+* Fix an edge case with direct `eval` and variable renaming
+
+    Use of the direct `eval` construct causes all variable names in the scope containing the direct `eval` and all of its parent scopes to become "pinned" and unable to be renamed. This is because the dynamically-evaluated code is allowed to reference any of those variables by name. When this happens esbuild avoids renaming any of these variables, which effectively disables minification for most of the file, and avoids renaming any non-pinned variables to the name of a pinned variable.
+
+    However, there was previously a bug where the pinned variable name avoidance only worked for pinned variables in the top-level scope but not in nested scopes. This could result in a non-pinned variable being incorrectly renamed to the name of a pinned variable in certain cases. For example:
+
+    ```js
+    // Input to esbuild
+    return function($) {
+      function foo(arg) {
+        return arg + $;
+      }
+      // Direct "eval" here prevents "$" from being renamed
+      // Repeated "$" puts "$" at the top of the character frequency histogram
+      return eval(foo($$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$))
+    }(2);
+    ```
+
+    When this code is minified with `--minify-identifiers`, the non-pinned variable `arg` is incorrectly transformed into `$` resulting in a name collision with the nested pinned variable `$`:
+
+    ```js
+    // Old output from esbuild (incorrect)
+    return function($) {
+      function foo($) {
+        return $ + $;
+      }
+      return eval(foo($$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$));
+    }(2);
+    ```
+
+    This is because the non-pinned variable `arg` is renamed to the top character in the character frequency histogram `$` (esbuild uses a character frequency histogram for smaller gzipped output sizes) and the pinned variable `$` was incorrectly not present in the list of variable names to avoid. With this release, the output is now correct:
+
+    ```js
+    // New output from esbuild (correct)
+    return function($) {
+      function foo(n) {
+        return n + $;
+      }
+      return eval(foo($$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$));
+    }(2);
+    ```
+
+    Note that even when esbuild handles direct `eval` correctly, using direct `eval` is not recommended because it disables minification for the file and likely won't work correctly in the presence of scope hoisting optimizations. See https://esbuild.github.io/link/direct-eval for more details.
+
 ## 0.12.23
 
 * Parsing of rest arguments in certain TypeScript types ([#1553](https://github.com/evanw/esbuild/issues/1553))

internal/renamer/renamer.go

@@ -25,21 +25,35 @@ func ComputeReservedNames(moduleScopes []*js_ast.Scope, symbols js_ast.SymbolMap
 
 	// All unbound symbols must be reserved names
 	for _, scope := range moduleScopes {
-		for _, member := range scope.Members {
-			symbol := symbols.Get(member.Ref)
-			if symbol.Kind == js_ast.SymbolUnbound || symbol.MustNotBeRenamed {
-				names[symbol.OriginalName] = 1
-			}
+		computeReservedNamesForScope(scope, symbols, names)
+	}
+
+	return names
+}
+
+func computeReservedNamesForScope(scope *js_ast.Scope, symbols js_ast.SymbolMap, names map[string]uint32) {
+	for _, member := range scope.Members {
+		symbol := symbols.Get(member.Ref)
+		if symbol.Kind == js_ast.SymbolUnbound || symbol.MustNotBeRenamed {
+			names[symbol.OriginalName] = 1
 		}
-		for _, ref := range scope.Generated {
-			symbol := symbols.Get(ref)
-			if symbol.Kind == js_ast.SymbolUnbound || symbol.MustNotBeRenamed {
-				names[symbol.OriginalName] = 1
-			}
+	}
+	for _, ref := range scope.Generated {
+		symbol := symbols.Get(ref)
+		if symbol.Kind == js_ast.SymbolUnbound || symbol.MustNotBeRenamed {
+			names[symbol.OriginalName] = 1
 		}
 	}
 
-	return names
+	// If there's a direct "eval" somewhere inside the current scope, continue
+	// traversing down the scope tree until we find it to get all reserved names
+	if scope.ContainsDirectEval {
+		for _, child := range scope.Children {
+			if child.ContainsDirectEval {
+				computeReservedNamesForScope(child, symbols, names)
+			}
+		}
+	}
 }
 
 type Renamer interface {

scripts/js-api-tests.js

@@ -3776,6 +3776,38 @@ let transformTests = {
     assert.strictEqual(code2, `foo;\n`)
   },
 
+  async nameCollisionEvalRename({ esbuild }) {
+    const { code } = await esbuild.transform(`
+      // "arg" must not be renamed to "arg2"
+      return function(arg2) {
+        function foo(arg) {
+          return arg + arg2;
+        }
+        // "eval" prevents "arg2" from being renamed
+        // "arg" below causes "arg" above to be renamed
+        return eval(foo(1)) + arg
+      }(2);
+    `)
+    const result = new Function('arg', code)(10)
+    assert.strictEqual(result, 13)
+  },
+
+  async nameCollisionEvalMinify({ esbuild }) {
+    const { code } = await esbuild.transform(`
+      // "arg" must not be renamed to "$"
+      return function($) {
+        function foo(arg) {
+          return arg + $;
+        }
+        // "eval" prevents "$" from being renamed
+        // Repeated "$" puts "$" at the top of the character frequency histogram
+        return eval(foo($$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$))
+      }(2);
+    `, { minifyIdentifiers: true })
+    const result = new Function('$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$', code)(1)
+    assert.strictEqual(result, 3)
+  },
+
   async dynamicImportString({ esbuild }) {
     const { code: code1 } = await esbuild.transform(`import('foo')`, { target: 'chrome63' })
     assert.strictEqual(code1, `import("foo");\n`)