fix: closures in for-loop init capture wrong binding - #18195
joelle-a-dev wants to merge 14 commits into
Conversation
for (let i = 0, f = () => i; ...) creates a closure over i inside the loop head. That closure should keep seeing the value i had before the loop started, but the transform let it share the same variable the loop body mutates, so f() returned the wrong value. Now that closure gets its own copy of the value, frozen right after the loop head runs, so it stays correct no matter what the loop body does to i afterward. Fixes babel#18191
|
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/62167 |
|
commit: |
|
@liuxingbaoyu Appreciate a review on this. |
nicolo-ribaudo
left a comment
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
Thanks for the PR. This test case shows multiple problems:
let res;
for (let i = 0, x = () => i, y = x(); i < 1; i++) {
res = y;
}
expect(y).toBe(1)// output
var res;
for (var i = 0, x = function x() {
return _i2;
}, _y2 = x(), _i3 = i, _i2 = i; i < 1; i++) {
res = _y2;
}
expect(y).toBe(1);- The
x()call returnsundefinedinstead of 0, because_i2is not defined yet at that point - There is a useless
_i3 = iassignment ydoesn't need to be renamed to_y2(but ignore this, it's pre-existing and not caused by this PR)
nicolo-ribaudo
added
PR: Bug Fix 🐛
|
@joelle-a-dev I see that you are keeping the branch up to date, but #18195 (review) still needs to be addressed. |
|
@nicolo-ribaudo I pushed the fix, please check now |
|
The failing CI looks like a timeout issue, already happening on main. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The snapshot timing and capture-path classification still alter semantics for valid initializer patterns.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Fixes lexical binding capture for closures created in for initializers.
Changes:
- Detects initializer closures capturing loop bindings.
- Introduces snapshot variables for captured values.
- Adds transformation and runtime regression fixtures.
| File | Description |
|---|---|
src/loop.ts |
Detects captures in loop initializers. |
src/index.ts |
Creates and rewrites snapshot bindings. |
regression/issue-18191/input.js |
Adds regression input. |
regression/issue-18191/output.js |
Records expected transformation. |
regression/issue-18191/options.json |
Configures the regression fixture. |
general/for-head-closure/exec.js |
Tests capture after body mutation. |
general/for-head-closure-called-in-head/exec.js |
Tests invocation during initialization. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| binding.path.insertAfter( | ||
| t.variableDeclarator( | ||
| t.identifier(frozenName), | ||
| t.identifier(name), | ||
| ), | ||
| ); |
| if (inHead && inClosure) { | ||
| if (id) headClosureCaptures.push(id as NodePath<t.Identifier>); | ||
| return null; |
|
@joelle-a-dev The two issues reported above by copilot seem correct; do you think you could fix it? |
In a loop like
for (let i = 0, f = () => i; ...), the closurefis created while the loop head runs, before the loop body starts changingieach time around.fshould always see the valueihad at that point, but the transform was makingfshare the same variable the loop body mutates, so callingf()later gave the wrong value.This gives that closure its own copy of the value, taken once right after the loop head finishes, so later changes to
iin the loop body don't affect it.