Fix unbound variable references produced by inlining (#37)#43
Merged
Conversation
…ountFreeRefs The IR's authoritative Let scoping convention (implemented by renameShadowedNames, DCE and Lua codegen) is sequential: in a standalone binding's RHS the earlier siblings of the same Let are in scope, while the binding's own name refers to an outer binder. shift, substitute and countFreeRefs in IR.Types implement the opposite convention, which makes inlining produce unbound references (issue #37). These tests are red until the convention is fixed.
…#37) The IR's Let scoping convention is sequential (let*): in a standalone binding's RHS the earlier siblings of the same Let are in scope, while the binding's own name refers to an outer binder. renameShadowedNames, DCE and the Lua codegen all follow this convention, but shift, substitute and countFreeRefs implemented the opposite one: they bumped the index for the binding's own name and ignored earlier siblings. As a result, inlining a non-exported binding whose body contained a let with a sibling-bound reference under a binder with the same name shifted that reference past its binder. DCE then deleted the "unused" binder and the Lua codegen rendered the dangling Ref (Local Bind1) 1 as an undefined variable Bind11. Also harden the codegen: a local reference with a non-zero index after renameShadowedNames is now a compile-time error (UnexpectedRefBound) instead of being silently rendered as an undefined Lua variable.
c63ba24 to
68e773e
Compare
…in Linker Consolidate the let*-scoping convention, previously copy-pasted at three Let cases in IR.Types and paraphrased across Lua codegen, DCE and two test specs, into a single GHC-style Note in IR.Types referenced from every site that implements or depends on it. Add a companion Note [Locals are uniquely named after renameShadowedNames] in IR.Optimizer documenting why renameShadowedNames must run last and why the Lua backend rejects non-zero local indices. While auditing for the convention, qualifyTopRefs in IR.Linker turned out to have the same defect issue #37 had: it qualified Let groupings independently instead of threading the per-name scope left to right, so a reference bound by an earlier sibling could be mis-qualified to a top-level binding. Fixed with mapAccumL threading mirroring the IR.Types functions, plus IR.Linker.Spec unit tests (red before the fix, green after).
68e773e to
69bd23c
Compare
This was referenced Jun 12, 2026
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
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.
The IR refers to locals by name plus a De Bruijn-style index, and the scoping rule for
Letwas inconsistent across the codebase.renameShadowedNames, DCE and the Lua codegen all treat let bindings sequentially: a standalone binding's RHS sees the earlier siblings of the same let, while the binding's own name points at an outer binder. The last part is the subtle bit. Standalone bindings are non-recursive, so a binding's own name is not in scope in its own RHS; if the same name is also bound further out, a self-reference there resolves to that outer binder:The index disambiguates which
xis meant.shift,substituteandcountFreeRefsimplemented the opposite rule: they bumped the index for the binding's own name and ignored siblings.So when the optimizer inlined a non-exported binding whose body contained a let with a sibling-bound reference (
Bind1in the issue), substituting it under a binder with the same name shifted that reference past its binder. DCE then dropped the binder as unused, and the codegen printed the danglingRef (Local Bind1) 1asBind11, a variable that exists nowhere. This is also why #38 could not reproduce the problem: without an export list nothing gets inlined, since every top-level binding becomes an export.What changed:
shift,substituteandcountFreeRefsnow thread the scope through let groupings left to right (mapAccumL), matching the convention used everywhere else.renameShadowedNamessuch a reference is never valid, so compilation now fails withUnexpectedRefBoundinstead of emitting broken Lua.No other golden files changed.
Closes #37. Supersedes #38 and #39. Thanks @Renegatto for the repro and for spotting that the export list is what triggers it.