Fix generators losing state across manual .next() calls#242
Merged
Conversation
`const g = gen(); g.next()` (manual iteration on a real function* generator,
as opposed to for...of) never advanced generator state: `g` is a storage-less
const binding, so each `g.next()` property access fell into
MLIRPropertyAccessCodeLogic's bound-ref fallback, which allocated a brand-new
temp alloca seeded from the pristine, unmutated original on every call site.
for...of avoided this because its lowering materializes one alloca up front
and reuses it.
Fix: a ScopedHashTable<mlir::Value, mlir::Value> cache on MLIRGenImpl, keyed
by the accessed object's SSA identity, lets repeated bound-ref accesses reuse
the ref materialized on the first access. Scoped (not just cleared) at
function entry via BoundRefCacheScopeT, mirroring symbolTable's own RAII
scoping, since nested closures re-enter mlirGenFunctionBody recursively.
Two follow-up fixes were needed after the initial full-suite run surfaced
real regressions:
- Gate the cache to real codegen passes only (!dummyRun && !allowPartialResolve).
The discovery/type-inference pass generates then erases throwaway ops, and
caching pointers into those ops let a later real pass read back a dangling
or address-recycled mlir::Value, producing "null operand found" errors in
unrelated functions.
- Restrict cache hits to the same MLIR block as the access site. A ref
materialized inside a nested `{ }` block doesn't dominate a reuse site
outside it ("operand does not dominate this use"). A precise fix would use
MLIR's DominanceInfo (not used anywhere else in this codebase); same-block
is a conservative, cheap approximation that's never wrong, just sometimes
conservative.
A previous, broader fix attempt (forcing every bound-method-bearing const
into real storage at declaration time) was reverted for breaking
interface/symbol value-passing; this fix is narrower; it only affects call
sites that already needed an address.
Adds 00generator_manual_next.ts (moved from docs/bugs/, now a real passing
regression test). Full suite: 702/702 passing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
const g = gen(); g.next()(manual iteration on a realfunction*generator, as opposed tofor...of) never advanced generator state —gis a storage-lessconstbinding, so eachg.next()property access re-materialized a temp alloca seeded from the pristine, unmutated original on every call site.ScopedHashTable<mlir::Value, mlir::Value>cache onMLIRGenImpl, keyed by the accessed object's SSA identity, lets repeated bound-ref accesses reuse the ref materialized on the first access. Scoped at function entry (mirroringsymbolTable's own RAII scoping) since nested closures re-enter function-body codegen recursively.constinto real storage at declaration time) was reverted for breaking interface/symbol value-passing; this fix is narrower and only affects call sites that already needed an address.docs/bugs/00generator_manual_next.tsintotest/tester/tests/as a real, passing regression test.Test plan
ctest -C Debug).next()iteration correctly advances generator state across 5 calls🤖 Generated with Claude Code