don't assume direct eval can access imports

CHANGELOG.md

@@ -41,6 +41,22 @@
 
     This release fixes an issue with the `--chunk-names=` feature where import paths in between two different automatically-generated code splitting chunks were relative to the output directory instead of relative to the importing chunk. This caused an import failure with the imported chunk if the chunk names setting was configured to put the chunks into a subdirectory. This bug has been fixed.
 
+* Remove the guarantee that direct `eval` can access imported symbols
+
+    Using direct `eval` when bundling is not a good idea because esbuild must assume that it can potentially reach anything in any of the containing scopes. Using direct `eval` has the following negative consequences:
+
+    * All names in all containing scopes are frozen and are not renamed during bundling, since the code in the direct `eval` could potentially access them. This prevents code in all scopes containing the call to direct `eval` from being minified or from being removed as dead code.
+
+    * The entire file is converted to CommonJS. This increases code size and decreases performance because exports are now resolved at run-time instead of at compile-time. Normally name collisions with other files are avoided by renaming conflicting symbols, but direct `eval` prevents symbol renaming so name collisions are prevented by wrapping the file in a CommonJS closure instead.
+
+    * Even with all of esbuild's special-casing of direct `eval`, referencing an ESM `import` from direct `eval` still doesn't necessarily work. ESM imports are live bindings to a symbol from another file and are represented by referencing that symbol directly in the flattened bundle. That symbol may use a different name which could break direct `eval`.
+
+    I recently realized that the last consequence of direct `eval` (the problem about not being able to reference `import` symbols) could cause subtle correctness bugs. Specifically esbuild tries to prevent the imported symbol from being renamed, but doing so could cause name collisions that make the resulting bundle crash when it's evaluated. Two files containing direct `eval` that both import the same symbol from a third file but that import it with different aliases create a system of unsatisfiable naming constraints.
+
+    So this release contains these changes to address this:
+
+    1. Direct `eval` is no longer guaranteed to be able to access imported symbols. This means imported symbols may be renamed or removed as dead code even though a call to direct `eval` could theoretically need to access them. If you need this to work, you'll have to store the relevant imports in a variable in a nested scope and move the call to direct `eval` into that nested scope.
+
 ## 0.8.53
 
 * Support chunk and asset file name templates ([#733](https://github.com/evanw/esbuild/issues/733), [#888](https://github.com/evanw/esbuild/issues/888))

internal/js_ast/js_ast.go

@@ -1846,6 +1846,7 @@ func MergeSymbols(symbols SymbolMap, old Ref, new Ref) Ref {
 	oldSymbol.Link = new
 	newSymbol.UseCountEstimate += oldSymbol.UseCountEstimate
 	if oldSymbol.MustNotBeRenamed {
+		newSymbol.OriginalName = oldSymbol.OriginalName
 		newSymbol.MustNotBeRenamed = true
 	}
 	return new

internal/js_parser/js_parser.go

@@ -1063,6 +1063,51 @@ func (p *parser) popScope() {
 	// We cannot rename anything inside a scope containing a direct eval() call
 	if p.currentScope.ContainsDirectEval {
 		for _, member := range p.currentScope.Members {
+			// Using direct eval when bundling is not a good idea in general because
+			// esbuild must assume that it can potentially reach anything in any of
+			// the containing scopes. We try to make it work but this isn't possible
+			// in some cases.
+			//
+			// For example, symbols imported using an ESM import are a live binding
+			// to the underlying symbol in another file. This is emulated during
+			// scope hoisting by erasing the ESM import and just referencing the
+			// underlying symbol in the flattened bundle directly. However, that
+			// symbol may have a different name which could break uses of direct
+			// eval:
+			//
+			//   // Before bundling
+			//   import { foo as bar } from './foo.js'
+			//   console.log(eval('bar'))
+			//
+			//   // After bundling
+			//   let foo = 123 // The contents of "foo.js"
+			//   console.log(eval('bar'))
+			//
+			// There really isn't any way to fix this. You can't just rename "foo" to
+			// "bar" in the example above because there may be a third bundled file
+			// that also contains direct eval and imports the same symbol with a
+			// different conflicting import alias. And there is no way to store a
+			// live binding to the underlying symbol in a variable with the import's
+			// name so that direct eval can access it:
+			//
+			//   // After bundling
+			//   let foo = 123 // The contents of "foo.js"
+			//   const bar = /* cannot express a live binding to "foo" here */
+			//   console.log(eval('bar'))
+			//
+			// Technically a "with" statement could potentially make this work (with
+			// a big hit to performance), but they are deprecated and are unavailable
+			// in strict mode. This is a non-starter since all ESM code is strict mode.
+			//
+			// So while we still try to obey the requirement that all symbol names are
+			// pinned when direct eval is present, we make an exception for imported
+			// symbols. There is no guarantee that "eval" will be able to reach these
+			// symbols and they are free to be renamed or removed by tree shaking.
+			if p.options.mode == config.ModeBundle && p.currentScope.Parent == nil &&
+				p.symbols[member.Ref.InnerIndex].Kind == js_ast.SymbolImport {
+				continue
+			}
+
 			p.symbols[member.Ref.InnerIndex].MustNotBeRenamed = true
 		}
 	}