-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(hir): replace this inside for-of iterator wrapper exprs when lifting a Symbol.iterator generator #10650
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
Closed
fix(hir): replace this inside for-of iterator wrapper exprs when lifting a Symbol.iterator generator #10650
Changes from all commits
Commits
Show all changes
3 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,26 @@ | ||
| Fixed: a class generator method keyed by `[Symbol.iterator]`, when a `for…of` | ||
| loop inside it iterated a method call on `this` (`for (const x of | ||
| this.gen()) yield x;`), saw `this === undefined` and threw `Cannot read | ||
| properties of undefined (reading 'gen')`. Every consumer that dispatches | ||
| through the class's iterator protocol (`for…of`, spread, `Array.from`) hit it | ||
| identically; the identical body under an ordinary method name worked. | ||
|
|
||
| Root cause: lifting a `*[Symbol.iterator]()` method to its top-level | ||
| generator (`synthesize_symbol_iterator_wrapper`) rewrites `this` to an | ||
| explicit parameter via `replace_this_in_stmts`/`replace_this_in_expr` | ||
| (`crates/perry-hir/src/analysis.rs`). That rewrite had no arm for | ||
| `Expr::GetIterator`/`GetAsyncIterator`/`MapEntries`/`SetValues` — the wrapper | ||
| expressions a `for…of` iterable lowers to when it can't be proven a plain | ||
| Array/Map/Set — so a `this` buried inside one of those wrappers fell through | ||
| to the catch-all and was never rewritten. | ||
|
|
||
| Fix: added the four missing arms, recursing into the wrapped expression the | ||
| same way the existing `Await`/`TypeOf`/`Void` arms do. | ||
|
|
||
| Validation: new gap test (`test_gap_10445_symbol_iterator_generator_this.ts`) | ||
| covering the issue repro plus a two-level generator chain, a | ||
| `Symbol.iterator` generator on a class expression, and `yield*` delegation | ||
| alongside a `for…of` over `this.method()` — proven to fail on the pre-fix | ||
| tree and pass on this one, byte-identical to Node 26.5.1. `cargo test | ||
| --release -p perry-hir --tests`: 748 passed. Lint: 76/77 gates (the one red | ||
| is the pre-existing, repo-wide benchmark-freshness check). |
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
89 changes: 89 additions & 0 deletions
89
test-files/test_gap_10445_symbol_iterator_generator_this.ts
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,89 @@ | ||
| // #10445: a generator method keyed by `[Symbol.iterator]`, whose `for…of` | ||
| // iterable is a method call on `this` (`for (const x of this.gen()) …`), | ||
| // saw `this === undefined` inside the callee. Root cause: | ||
| // `synthesize_symbol_iterator_wrapper` (lower_decl/class_decl.rs) lifts the | ||
| // method's body to a top-level generator taking `this` as an explicit | ||
| // param, then `replace_this_in_stmts`/`replace_this_in_expr` (analysis.rs) | ||
| // rewrites every `Expr::This` in that body to the param. A `for…of` whose | ||
| // iterable can't be proven a plain Array/Map/Set lowers to one of | ||
| // `GetIterator`/`GetAsyncIterator`/`MapEntries`/`SetValues` wrapping the | ||
| // receiver expression (stmt_loops.rs's `lower_stmt_for_of_inner`) -- and | ||
| // `replace_this_in_expr` had no arm for any of those wrappers, so a `this` | ||
| // buried inside one fell through to the catch-all and was left unreplaced. | ||
| // Every consumer that dispatches through the lifted function (spread, | ||
| // `for…of`, `Array.from`) hit the same bug identically. | ||
|
|
||
| class Bag { | ||
| items = [1, 2]; | ||
| *gen() { | ||
| yield* this.items; | ||
| } | ||
| *[Symbol.iterator]() { | ||
| for (const x of this.gen()) yield x; // the repro shape | ||
| } | ||
| *viaLocal() { | ||
| for (const x of this.gen()) yield x; // same body, ordinary name: control | ||
| } | ||
| } | ||
|
|
||
| // Two-level: the iterator method's for-of iterable is ANOTHER method whose | ||
| // OWN for-of iterable is a THIRD method -- this must survive two hops of | ||
| // the lifted-generator's this-substitution, not just one. | ||
| class TwoLevel { | ||
| items = [10, 20, 30]; | ||
| *inner() { | ||
| for (const x of this.items) yield x * 2; | ||
| } | ||
| *middle() { | ||
| for (const x of this.inner()) yield x + 1; | ||
| } | ||
| *[Symbol.iterator]() { | ||
| for (const x of this.middle()) yield x; | ||
| } | ||
| } | ||
|
|
||
| // Class EXPRESSION (not a declaration) -- the lift/this-rewrite must not be | ||
| // keyed off a named-declaration-only path. | ||
| const ExprClass = class { | ||
| items = ["a", "b", "c"]; | ||
| *gen() { | ||
| yield* this.items; | ||
| } | ||
| *[Symbol.iterator]() { | ||
| for (const x of this.gen()) yield x; | ||
| } | ||
| }; | ||
|
|
||
| // `yield*` delegation alongside a for-of over `this.method()` in the SAME | ||
| // generator -- confirms the fix doesn't disturb the already-working | ||
| // yield*-over-this.gen() path while also fixing the for-of one. | ||
| class Mixed { | ||
| items = [1, 2, 3]; | ||
| *gen() { | ||
| yield* this.items; | ||
| } | ||
| *[Symbol.iterator]() { | ||
| yield* this.gen(); | ||
| for (const x of this.gen()) yield x * 10; | ||
| } | ||
| } | ||
|
|
||
| const show = (label: string, f: () => unknown) => { | ||
| try { | ||
| console.log(label, JSON.stringify(f())); | ||
| } catch (e: any) { | ||
| console.log(label, "threw:", e.message); | ||
| } | ||
| }; | ||
|
|
||
| show("spread over *[Symbol.iterator]:", () => [...new Bag()]); | ||
| show("for-of over *[Symbol.iterator]:", () => { | ||
| const out: number[] = []; | ||
| for (const x of new Bag()) out.push(x); | ||
| return out; | ||
| }); | ||
| show("Array.from(bag):", () => Array.from(new Bag())); | ||
| show("named generator, same body:", () => [...new Bag().viaLocal()]); | ||
| show("two-level generator:", () => [...new TwoLevel()]); | ||
| show("class expression generator:", () => [...new ExprClass()]); | ||
| show("yield* + for-of mixed:", () => [...new Mixed()]); | ||
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
🔎 Supported by static analysis
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 17849
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 26837
Add assertions for the three untested wrapper arms. This fixture exercises only
Expr::GetIteratorthrough synchronousfor...ofoverthis.gen(). Add focused*[Symbol.iterator]cases with asserted output for:for await (const x of this.gen()), which lowers toExpr::GetAsyncIteratoreven when the containing generator is synchronous.MapandSetfields onthis, using loop heads that bypass their fast paths and lower toExpr::MapEntriesandExpr::SetValues.These cases cover
replace_this_in_exprthrough every changed wrapper and catch regressions that leave nestedthisreferences unreplaced.🤖 Prompt for AI Agents