-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(hir): let a declarator's own initializer forward-capture its binding #9720
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
proggeramlug
wants to merge
2
commits into
PerryTS:main
from
proggeramlug:fix/9718-initializer-self-binding
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| **A closure in a `let`/`const` declarator's own initializer can now see the | ||
| binding that declarator introduces** — `const off = ev.on(() => off())`, | ||
| `const { unmount } = await render({ onDone: () => unmount() })`. It previously | ||
| threw `ReferenceError: <name> is not defined` (#9718). | ||
|
|
||
| The function-body forward-capture pre-pass (`pre_register_forward_captured_lets`) | ||
| decides whether a `let`/`const` binding needs a boxed, TDZ-seeded forward | ||
| declaration by asking whether any closure *seen so far* references its name. It | ||
| recorded a declarator's own initializer into that set only **after** making that | ||
| decision — which is all the later-declarator case needs (`let z = (w) => { … O … }, | ||
| O = setTimeout(z, K)`, the minified `new Promise` executor shape), but left the | ||
| self-referential shape unregistered. The reference then fell through to | ||
| `js_global_get_or_throw_unresolved`. Recording the initializer's closure refs | ||
| before the decision is a strict superset: later declarators still see them. | ||
|
|
||
| The hole was reachable from every declarator form, because the pre-pass is what | ||
| the destructuring path relies on — `destructuring/var_decl.rs`'s own | ||
| `is_function_expr_init` pre-registration covers only simple `Pat::Ident` | ||
| bindings, and its `ast_expr_contains_function_expr` scan does not descend | ||
| through `await`. So `const O = await mk({ cb: () => O() })` (plain binding | ||
| behind an `await`), every object/array/nested pattern, and `{ key }` shorthand | ||
| all failed; only a closure created *after* the declaration worked. | ||
|
|
||
| Found in claude-code: the `install` subcommand is | ||
|
|
||
| ```js | ||
| let { unmount: O } = await eB(el(wMA, { onDone: (w, $) => { O(), q(w, $) }, … })) | ||
| ``` | ||
|
|
||
| so `claude install <bad-channel>` threw an uncaught `ReferenceError: O is not | ||
| defined` mid-teardown, losing the final newline and the exit code — perry exited | ||
| 0 where node exits 1 (the `B60_install_badtarget` divergence carried in #9575). | ||
|
|
||
| `test-files/test_gap_9718_initializer_self_binding.ts` pins all of it against | ||
| node: plain binding, object pattern, `{ key }` shorthand, array pattern, nested | ||
| pattern, `let` and `const`, with and without `await`, plus a multi-declarator | ||
| statement so the reordered scan's pre-existing earlier-refs-later behavior stays | ||
| covered. Unpatched, 7 of its 12 lines are `ReferenceError`; patched, the output | ||
| is byte-identical to node 26.5.1. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| // #9718: a closure created INSIDE a `let`/`const` declarator's initializer may | ||
| // reference the binding that declarator introduces. The closure body does not | ||
| // run until after initialization, so node resolves it normally — only the TDZ | ||
| // window between entering the declaration and completing it is off limits. | ||
| // | ||
| // Perry pre-registered such a binding only when the reference came from an | ||
| // EARLIER statement or an EARLIER declarator of the same declaration, never | ||
| // from the declarator's own initializer, so the reference fell through to the | ||
| // unresolved-global path and threw `ReferenceError: <name> is not defined`. | ||
| // claude-code's `install` subcommand is exactly this shape: | ||
| // let { unmount: O } = await render(el({ onDone: (a, b) => { O(), done(a, b) } })) | ||
| // | ||
| // Every declarator form is covered: plain binding, object pattern, `{ key }` | ||
| // shorthand, array pattern, nested pattern, `let` and `const`, with and | ||
| // without `await`, plus a multi-declarator statement (the earlier-declarator | ||
| // case that already worked, to pin that this did not regress). | ||
|
|
||
| type Handle = { u: () => string }; | ||
|
|
||
| // Hands the callback to a later turn, then resolves — so the callback always | ||
| // runs after the declaration has completed, exactly as in the real bundle. | ||
| function render(cb: () => string): Promise<Handle> { | ||
| return new Promise<Handle>((resolve) => { | ||
| pending.push(cb); | ||
| resolve({ u: () => "handle" }); | ||
| }); | ||
| } | ||
| function renderSync(cb: () => string): Handle { | ||
| pending.push(cb); | ||
| return { u: () => "handle" }; | ||
| } | ||
| const pending: (() => string)[] = []; | ||
|
|
||
| function report(label: string, run: () => string): void { | ||
| try { | ||
| console.log(label + ": " + run()); | ||
| } catch (e) { | ||
| console.log(label + ": THREW " + ((e as Error) && (e as Error).name) + " " + ((e as Error) && (e as Error).message)); | ||
| } | ||
| } | ||
|
|
||
| async function main(): Promise<void> { | ||
| // 1. plain binding, awaited initializer | ||
| const a = await render(() => a.u() + "/plain-await"); | ||
| // 2. object pattern, awaited initializer | ||
| const { u: b } = await render(() => b() + "/obj-await"); | ||
| // 3. object pattern, synchronous initializer | ||
| const { u: c } = renderSync(() => c() + "/obj-sync"); | ||
| // 4. array pattern, awaited initializer | ||
| const [d] = await render(() => d.u() + "/array-await").then((h) => [h] as [Handle]); | ||
| // 5. `let` rather than `const` | ||
| let { u: e } = await render(() => e() + "/let-obj-await"); | ||
| // 6. `{ key }` shorthand pattern | ||
| const { u } = await render(() => u() + "/shorthand-await"); | ||
| // 7. nested pattern | ||
| const { inner: { u: g } } = await Promise.resolve({ inner: { u: () => "handle" } as Handle }) | ||
| .then((v) => { pending.push(() => g() + "/nested-await"); return v; }); | ||
| // 8. multi-declarator: an EARLIER declarator's closure references a LATER one | ||
| // (already worked before #9718 — pinned so the reordered scan keeps it) | ||
| const h = () => i() + "/earlier-refs-later", | ||
| i = (): string => "handle"; | ||
|
|
||
| report("plain-await", () => pending[0]!()); | ||
| report("obj-await", () => pending[1]!()); | ||
| report("obj-sync", () => pending[2]!()); | ||
| report("array-await", () => pending[3]!()); | ||
| report("let-obj-await", () => pending[4]!()); | ||
| report("shorthand-await", () => pending[5]!()); | ||
| report("nested-await", () => pending[6]!()); | ||
| report("earlier-refs-later", h); | ||
|
|
||
| // The shapes the pre-pass's original ordering existed for. They passed | ||
| // before this fix and must keep passing: moving the initializer scan earlier | ||
| // is a superset, not a replacement. | ||
| const fact = (n: number): number => (n <= 1 ? 1 : n * fact(n - 1)); | ||
| const fib = function rec(n: number): number { return n < 2 ? n : rec(n - 1) + rec(n - 2); }; | ||
| const off = renderSync(() => off.u() + "/init-call-result"); | ||
| console.log("self-recursive-arrow=" + fact(5)); | ||
| console.log("named-fn-expr-recursion=" + fib(10)); | ||
| report("init-call-result", () => pending[7]!()); | ||
|
|
||
| // The declaration is complete by the time these run, so the direct calls | ||
| // must agree with what the closures saw. | ||
| report("direct-plain", () => a.u()); | ||
| report("direct-obj", () => b()); | ||
| report("direct-array", () => d.u()); | ||
| report("direct-nested", () => g()); | ||
| console.log("count=" + pending.length + " c=" + c() + " e=" + e() + " u=" + u() + " i=" + i()); | ||
| } | ||
|
|
||
| void main(); | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a regression case for the pre-initialization TDZ.
The current helpers enqueue callbacks and execute them only after the declaration completes. Therefore, this fixture does not verify that a callback executed during its initializer reads the binding as TDZ and throws
ReferenceError. Add a synchronous callback invocation inside an initializer, while keeping the existing deferred cases.🤖 Prompt for AI Agents