fix #1455: bundler hoisting bug with var+for loops

evanw · Jul 27, 2021 · bb3a4cd · bb3a4cd
1 parent 773f15f
commit bb3a4cd
Show file tree

Hide file tree

Showing 5 changed files with 156 additions and 16 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,65 @@
 # Changelog
 
+## Unreleased
+
+* Fix a hoisting bug in the bundler ([#1455](https://github.com/evanw/esbuild/issues/1455))
+
+    This release fixes a bug where variables declared using `var` inside of top-level `for` loop initializers were not hoisted inside lazily-initialized ES modules (such as those that are generated when bundling code that loads an ES module using `require`). This meant that hoisted function declarations incorrectly didn't have access to these loop variables:
+
+    ```js
+    // entry.js
+    console.log(require('./esm-file').test())
+
+    // esm-file.js
+    for (var i = 0; i < 10; i++) ;
+    export function test() { return i }
+    ```
+
+    Old output (incorrect):
+
+    ```js
+    // esm-file.js
+    var esm_file_exports = {};
+    __export(esm_file_exports, {
+      test: () => test
+    });
+    function test() {
+      return i;
+    }
+    var init_esm_file = __esm({
+      "esm-file.js"() {
+        for (var i = 0; i < 10; i++)
+          ;
+      }
+    });
+
+    // entry.js
+    console.log((init_esm_file(), esm_file_exports).test());
+    ```
+
+    New output (correct):
+
+    ```js
+    // esm-file.js
+    var esm_file_exports = {};
+    __export(esm_file_exports, {
+      test: () => test
+    });
+    function test() {
+      return i;
+    }
+    var i;
+    var init_esm_file = __esm({
+      "esm-file.js"() {
+        for (i = 0; i < 10; i++)
+          ;
+      }
+    });
+
+    // entry.js
+    console.log((init_esm_file(), esm_file_exports).test());
+    ```
+
 ## 0.12.16
 
 * Remove warning about bad CSS `@`-rules ([#1426](https://github.com/evanw/esbuild/issues/1426))

diff --git a/internal/bundler/snapshots/snapshots_lower.txt b/internal/bundler/snapshots/snapshots_lower.txt
@@ -997,12 +997,14 @@ TestLowerStrictModeSyntax
 // for-in.js
 if (test) {
   a = b;
-  for (var a in {})
+  for (a in {})
     ;
 }
+var a;
 x = y;
-for (var x in {})
+for (x in {})
   ;
+var x;
 
 ================================================================================
 TestLowerTemplateObject

diff --git a/internal/bundler/snapshots/snapshots_splitting.txt b/internal/bundler/snapshots/snapshots_splitting.txt
@@ -583,21 +583,31 @@ export {
 TestVarRelocatingBundle
 ---------- /out/top-level.js ----------
 // top-level.js
-for (var b; 0; )
+for (; 0; )
   ;
-for (var { c, x: [d] } = {}; 0; )
+var b;
+for ({ c, x: [d] } = {}; 0; )
   ;
-for (var e of [])
+var c;
+var d;
+for (e of [])
   ;
-for (var { f, x: [g] } of [])
+var e;
+for ({ f, x: [g] } of [])
   ;
-for (var h in {})
+var f;
+var g;
+for (h in {})
   ;
+var h;
 i = 1;
-for (var i in {})
+for (i in {})
   ;
-for (var { j, x: [k] } in {})
+var i;
+for ({ j, x: [k] } in {})
   ;
+var j;
+var k;
 
 ---------- /out/nested.js ----------
 // nested.js

diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go
@@ -8881,9 +8881,9 @@ func (p *parser) visitAndAppendStmt(stmts []js_ast.Stmt, stmt js_ast.Stmt) []js_
 			s.UpdateOrNil = p.visitExpr(s.UpdateOrNil)
 		}
 		s.Body = p.visitLoopBody(s.Body)
-		p.popScope()
 
-		// Potentially relocate "var" declarations to the top level
+		// Potentially relocate "var" declarations to the top level. Note that this
+		// must be done inside the scope of the for loop or they won't be relocated.
 		if s.InitOrNil.Data != nil {
 			if init, ok := s.InitOrNil.Data.(*js_ast.SLocal); ok && init.Kind == js_ast.LocalVar {
 				if assign, ok := p.maybeRelocateVarsToTopLevel(init.Decls, relocateVarsNormal); ok {
@@ -8896,6 +8896,8 @@ func (p *parser) visitAndAppendStmt(stmts []js_ast.Stmt, stmt js_ast.Stmt) []js_
 			}
 		}
 
+		p.popScope()
+
 		if p.options.mangleSyntax {
 			mangleFor(s)
 		}
@@ -8905,8 +8907,6 @@ func (p *parser) visitAndAppendStmt(stmts []js_ast.Stmt, stmt js_ast.Stmt) []js_
 		p.visitForLoopInit(s.Init, true)
 		s.Value = p.visitExpr(s.Value)
 		s.Body = p.visitLoopBody(s.Body)
-		p.popScope()
-		p.lowerObjectRestInForLoopInit(s.Init, &s.Body)
 
 		// Check for a variable initializer
 		if local, ok := s.Init.Data.(*js_ast.SLocal); ok && local.Kind == js_ast.LocalVar && len(local.Decls) == 1 {
@@ -8923,27 +8923,34 @@ func (p *parser) visitAndAppendStmt(stmts []js_ast.Stmt, stmt js_ast.Stmt) []js_
 			}
 		}
 
-		// Potentially relocate "var" declarations to the top level
+		// Potentially relocate "var" declarations to the top level. Note that this
+		// must be done inside the scope of the for loop or they won't be relocated.
 		if init, ok := s.Init.Data.(*js_ast.SLocal); ok && init.Kind == js_ast.LocalVar {
 			if replacement, ok := p.maybeRelocateVarsToTopLevel(init.Decls, relocateVarsForInOrForOf); ok {
 				s.Init = replacement
 			}
 		}
 
+		p.popScope()
+
+		p.lowerObjectRestInForLoopInit(s.Init, &s.Body)
+
 	case *js_ast.SForOf:
 		p.pushScopeForVisitPass(js_ast.ScopeBlock, stmt.Loc)
 		p.visitForLoopInit(s.Init, true)
 		s.Value = p.visitExpr(s.Value)
 		s.Body = p.visitLoopBody(s.Body)
-		p.popScope()
 
-		// Potentially relocate "var" declarations to the top level
+		// Potentially relocate "var" declarations to the top level. Note that this
+		// must be done inside the scope of the for loop or they won't be relocated.
 		if init, ok := s.Init.Data.(*js_ast.SLocal); ok && init.Kind == js_ast.LocalVar {
 			if replacement, ok := p.maybeRelocateVarsToTopLevel(init.Decls, relocateVarsForInOrForOf); ok {
 				s.Init = replacement
 			}
 		}
 
+		p.popScope()
+
 		p.lowerObjectRestInForLoopInit(s.Init, &s.Body)
 
 	case *js_ast.STry:

diff --git a/scripts/end-to-end-tests.js b/scripts/end-to-end-tests.js
@@ -2553,6 +2553,67 @@
     }),
   )
 
+  // Test hoisting variables inside for loop initializers outside of lazy ESM
+  // wrappers. Previously this didn't work due to a bug that considered for
+  // loop initializers to already be in the top-level scope. For more info
+  // see: https://github.com/evanw/esbuild/issues/1455.
+  tests.push(
+    test(['in.js', '--outfile=node.js', '--bundle'], {
+      'in.js': `
+        if (require('./nested').foo() !== 10) throw 'fail'
+      `,
+      'nested.js': `
+        for (var i = 0; i < 10; i++) ;
+        export function foo() { return i }
+      `,
+    }),
+    test(['in.js', '--outfile=node.js', '--bundle'], {
+      'in.js': `
+        if (require('./nested').foo() !== 'c') throw 'fail'
+      `,
+      'nested.js': `
+        for (var i in {a: 1, b: 2, c: 3}) ;
+        export function foo() { return i }
+      `,
+    }),
+    test(['in.js', '--outfile=node.js', '--bundle'], {
+      'in.js': `
+        if (require('./nested').foo() !== 3) throw 'fail'
+      `,
+      'nested.js': `
+        for (var i of [1, 2, 3]) ;
+        export function foo() { return i }
+      `,
+    }),
+    test(['in.js', '--outfile=node.js', '--bundle', '--target=es6'], {
+      'in.js': `
+        if (JSON.stringify(require('./nested').foo()) !== '{"b":2,"c":3}') throw 'fail'
+      `,
+      'nested.js': `
+        for (var {a, ...i} = {a: 1, b: 2, c: 3}; 0; ) ;
+        export function foo() { return i }
+      `,
+    }),
+    test(['in.js', '--outfile=node.js', '--bundle', '--target=es6'], {
+      'in.js': `
+        if (JSON.stringify(require('./nested').foo()) !== '{"0":"c"}') throw 'fail'
+      `,
+      'nested.js': `
+        for (var {a, ...i} in {a: 1, b: 2, c: 3}) ;
+        export function foo() { return i }
+      `,
+    }),
+    test(['in.js', '--outfile=node.js', '--bundle', '--target=es6'], {
+      'in.js': `
+        if (JSON.stringify(require('./nested').foo()) !== '{"b":2,"c":3}') throw 'fail'
+      `,
+      'nested.js': `
+        for (var {a, ...i} of [{a: 1, b: 2, c: 3}]) ;
+        export function foo() { return i }
+      `,
+    }),
+  )
+
   // Test tree shaking
   tests.push(
     // Keep because used (ES6)