fix(compiler): solve recursive function closures - #78
Merged
Conversation
Make each script-level function call synchronously walk its recursive call closure until every signature is inferred and one final pass makes no changes. This rebuilds current-script binding and expression facts for warm FuncCache entries before typing a sibling or later statement. Add cold/warm cache-reuse coverage, require mutual-recursion closure completion, and keep future PIR body analysis out of FuncCache. Fixes #71
thiremani
force-pushed
the
codex/fix-full-function-solving
branch
from
July 31, 2026 19:47
1d15a03 to
d868bcd
Compare
Replace InProgress with walkedFuncs, marked before the walk so it cuts recursive backedges and repeated sibling calls alike. A pass now costs one walk per specialization instead of one per path through the call graph, which removes an O(2^depth) cliff: a 31-function fan-out went from ~80 minutes to 0.10s. Add settledFuncs, a per-script memo of specializations a stable pass already rebuilt, so later script statements skip a rewalk they would only repeat. This is a performance memo; rewalking before lowering is what fixes #71. Compiling 256 call sites over a 201-function closure drops from 0.92s to 0.18s. Derive the convergence budget from the output slots of every specialization walked so far rather than a fixed 100 passes. Only an unresolved output slot becoming resolved keeps the loop going, so slot count plus a stable pass bounds it; specialization count does not, and a 104-output recursive signature was rejected. The union matters because an argument that resolves late remangles its callee, so a provisional specialization can consume passes and then vanish from the walk. Exhausting the derived bound is now an ICE, since it can only mean a pass continued without resolving a slot. Report the convergence failure against the specialization that did not resolve instead of the script-level root, whose own body is usually fine. Drop the lowering-time ExprCache refresh in compileCallExpression: the stable pass now rebuilds those entries, and instrumentation showed the overwrite never changed a type across the whole end-to-end suite. Tests: closure walks stay flat across both call-graph depth and script call sites; a 130-output specialization converges; the convergence error names the failing callee; and a warm FuncCache rebuilds its binding types, which pins issue #71 at the solver level for the first time. Refs #71
FuncStatement.Token is the function's name identifier, and funcTemplates is keyed by that literal while lookup goes by call-site name, so the template already carries both facts the convergence diagnostic needs. Hold the template itself and read the name off its token, which also stops the name and token from drifting apart.
…uncMangled ScriptFunc was written once, cleared once, and read once, only ever tested for emptiness. TypeFunc runs solely under TypeScriptFunc and no expression is typed between ScriptFunc being set and FuncNameMangled being set, so the two agreed as booleans everywhere; instrumenting the single read site showed no divergence across the unit and end-to-end suites. Test FuncNameMangled directly instead. FuncNameMangled named two different things: a cursor on TypeSolver and Compiler, and a component of BindingKey and ExprKey. Rename only the cursor, so the moving value and the key field no longer read alike.
The firstUnresolved field held the head of the pending-callee sequence, but both facts anyone consumed — is anything unresolved, and which — are already derivable from walkedFuncs plus each specialization's cached OutTypes. Scan for them at the end of the pass instead of latching one during it, which removes the field and the state it had to be threaded through InferFuncTypes to maintain. Blame moves from walk-unwind order to sorted order, so a cycle now names a different member than before. TestCycles asserted "Function f" only because fbb5192 always named the root; every member of the cycle is equally stuck, so it now asserts that the report names one of them and points at that function's own declaration. The property that matters — a root is never blamed for a callee's failure — stays pinned by TestNonConvergingCalleeIsBlamed.
Two things force the repeat, and neither is visible from the loop: a resolved argument remangles its call site, so specializations appear mid-solve; and a body typed against an unresolved callee holds stale facts even once its own signature is final. Clearing walkedFuncs is what makes each pass re-enter every function rather than only the root. Pin the second property with a nested back edge. C's local p resolves through a back edge into B, so the first walk of C cannot type it and only a later sweep reaching two levels down records it. Dropping the clear leaves p untyped, which the test now catches.
It memoized specializations a stable pass had already rebuilt so later script statements skipped them. That is a compile-time optimization, not a correctness device: removing it leaves TestWarmFuncCacheRebuildsBindingTypes green, because what fixes issue #71 is rewalking the closure before lowering, which the pass loop still does. The price is that each script-level call re-walks its closure. Measured: 256 calls into a 201-function closure 0.19s -> 0.95s, and 64 distinct roots sharing a 151-function closure 0.16s -> 0.38s. Fan-out is unaffected at 0.11s for 2^30 call paths, since walkedFuncs still cuts repeated sibling calls within a pass. Worth restoring if compile time on wide scripts ever matters; today it buys less than the state costs. Also note at the single Converging assignment that TypeScriptFunc's termination and its output-slot pass bound both depend on that being the only one, since overrunning the bound is now an ICE.
Removing it cost 5x on scripts that call repeatedly into one closure, because every script-level call re-walked the whole closure from scratch: 256 calls into a 201-function closure ran ~103k body walks instead of ~400. Measured 0.93s -> 0.18s restoring it, and 0.38s -> 0.15s for 64 distinct roots sharing a closure. It stays a pure optimization. Rewalking before lowering is what fixes issue #71, and its per-script lifetime is what makes skipping safe, so it can never carry body facts between scripts. Restore the call-site assertion that pins it, beside the depth assertion that pins walkedFuncs. The unresolvedCallee scan it was measured against is unchanged: with settledFuncs present that scan runs a couple of times per closure rather than once per call, so latching the first unresolved callee instead measured identical (0.17s vs 0.18s) and is not worth a second mechanism.
TypeBlock fixes an output on the pass that first resolves it, but resolved is not final: Array(Empty) still refines to Array(I64), StrG to StrH. The old solver hid this — it stopped as soon as the root resolved, so these programs died at lowering with "unknown type in mapToLLVMType: ?". Driving the closure to convergence makes every type resolved, so the stale signature now reaches codegen: a recursive array builder compiled clean and printed [] instead of [1 1 1], and its string form leaked 64 bytes because the caller saw StrG where the body returned StrH. Refining the output instead is the real fix, but it lets one slot change more than once, which invalidates the output-slot pass bound. Until that is redesigned, report the disagreement rather than miscompile. Render the mangled form when the two user-facing names match, so the string case does not read "Str, then Str". Restore the unwind-order latch for blame. Scanning walkedFuncs in sorted order blamed an innocent wrapper: Root -> AA -> ZBrokenX reported AA, because AA sorts first. Setting the latch on unwind picks the innermost unresolved call instead. Tests: the wrapper case, both refinement shapes, and a remangling case pinning that the pass budget spans provisional specializations that later vanish.
Seed cached output storage types during closure rewalks and join each body observation into the cached signature. This lets empty arrays, string ownership, tables, and struct field ownership widen without stale lowering metadata or type downgrades.\n\nDrive closure solving to an unchanged semantic fixed point instead of imposing a numeric pass ceiling, and cover the recursive array/string cases through leak-checked end-to-end tests.\n\nRefs #71
Restore FuncNameMangled, ScriptFunc, and TypeFunc's boolean result so the issue #71 fix does not carry unrelated exported API changes. The closure solver still uses per-pass walked specializations and stable full-body sweeps; this commit only restores the previous public surface and flow marker.
Restore the existing call-site output refresh. Stable closure sweeps provide the semantic fix for #71, while keeping this defensive alignment avoids an unrelated lowering change and leaves compiler.go untouched versus master.
Assert that the stable body sweep remangles the exact call site with its widened output storage type, and cover table output refinement and empty-table reset behavior through TypeFunc. Refresh solver and PIR-plan comments so they describe monotonic joins and unchanged closure sweeps accurately.
Remove outputRevision and use the existing Converging flag as the sole sticky progress bit for each closure sweep. The settled-function and per-pass traversal memos are unchanged; regression counters and timings remain identical. Refs #71
Exercise the parser-valid function-body path that rejects an incompatible output assignment during solving, including its diagnostic token and location. Trim narrative solver comments while retaining the stable-sweep, output-seeding, and per-script memo invariants. Refs #71
Rename the traversal latch to firstUnresolvedCallee so its scope and selection semantics are explicit without a comment. Remove the one-use type-change formatter and keep the defensive diagnostic exact by formatting readable and mangled types at its call site. Refs #71
Use firstUnresolved for the private FuncStatement latch; its type and use sites already make the callee context clear. The shorter name also avoids unrelated gofmt alignment churn in TypeSolver. Refs #71
Restore the top-level solver invariant by clearing ScriptFunc when TypeScriptFunc returns instead of preserving an unreachable nested value.
Give scripts a namespaced root and retain final binding-slot types and settlement state on cached function specializations. Reuse settled closures across scripts while keeping walkedFuncs as the per-pass recursion guard. Refs #71
Pass function and expression metadata through one ScriptCache so settled specializations cannot be reused without the expression facts required for lowering. Update compiler entry points and regressions to share that cache atomically across scripts.
Remove the unused settled marker from script roots, preserve concise output-type diagnostics, and document the cross-script specialization boundary. Clarify that the final incompatible-output check is defensive and distinguish rebuilt variable snapshots from generated expression rewrites.
Rename the cross-script metadata bundle to CompileInfo and let each CodeCompiler own exactly one instance. Script compilers now reuse it automatically, removing the constructor, explicit cache arguments, and exported embedded pointer that allowed accidental map replacement.
Keep Func as a pure type-system value and move Vars and Settled into FuncInfo, the cached specialization record. This prevents mutable body metadata from being copied through the Type interface and gives PIR a coherent place for future specialization facts.
Stop TypeFunc from replacing cached specialization identity, treat impossible output conflicts as internal invariant failures, and keep shared expression metadata immutable during lowering. Add a two-script regression proving unresolved provisional calls are revisited and remangled to the later concrete type.
Share the owning code compiler's CompileInfo without transient allocations and validate specialization identity before every TypeFunc return path. Strengthen the cross-script regression to prove unsettled provisional bodies are revisited before concrete call metadata is reused.
Rename FuncInfo.Signature to Sig to match the compiler's existing signature terminology and keep frequent specialization access concise. This is a mechanical rename with no behavioral change.
Script.Mangle spliced the raw literal "script" into a mangled path. Every other symbol goes through length-prefixed segments, and "script" starts with s, which is the separator code for /, so demanglePath consumed it as a path separator and abandoned the rest: Pt_5mymod_p_script_6report demangled to "mymod./" with no error, silently dropping the script name. Give script roots an ENTRY suffix instead, mirroring how F marks a function arity. The marker trails the name because a leading one would be eaten the same way, and it cannot begin with d, s or h for that reason. A script root is now a first-class SymbolScript that round-trips, and the three symbols sharing a name stay distinct: Pt_5mymod_p_6report constant Pt_5mymod_p_6report_e script root Pt_5mymod_p_6report_f1_I64 function Introduced by this branch, so fixing it here rather than filing it: FuncCache would otherwise hold a mix of demanglable symbols and ones that print garbage the moment a diagnostic or symbol dump touches a cache key.
List the script-root forms in the DemangleParsed grammar and place the _e branch in the parsing flow, so the comment covers every shape the function parses. Pin Demangle's rendering in the round-trip test; the SymbolScript branch of Demangled.String had no coverage. Correct the ENTRY comment. It read as though any leading marker would be consumed as a separator, which is only true of d, s and h. A leading marker fails whatever letter it starts with, because the name position needs a length-prefixed segment: a safe letter yields an empty name, a separator code yields a corrupted one.
Keep FuncCache and ExprCache directly on Compiler and have NewCompiler inherit both map references from the owning CodeCompiler. This removes the CompileInfo wrapper while preserving coherent cross-script reuse for settled specializations.
Treat a missing current FuncInfo during lowering as an invariant violation instead of silently using an expression-local type. Update the direct lowering test to establish the body context provided by normal script compilation.
Name the regression for both shared caches and compile the cold and warm targets under the same script identity. This isolates cache warmth as the only source of any IR difference.
Render script roots as their path-qualified source names, matching constants and the common symbol formatting. DemangleParsed still preserves SymbolScript and the _e marker remains part of the mangled identity.
This was referenced Aug 4, 2026
Encode script basenames through the path grammar and retain the entry suffix. Demangle symbols with a /./ module boundary and append .spt to script roots. Validate script and relative-path names without rejecting uppercase or Unicode, report resolution errors once at the CLI boundary, and add a dotted-script regression. Refs #71 Refs #81
thiremani
marked this pull request as ready for review
August 4, 2026 07:47
Treat an unsettled specialization reaching lowering as an internal invariant violation. Inline the warm-cache IR regression setup so independent and shared compiler state are explicit.
Replace the in-process IR comparison with a CLI regression that compiles the cache-reuse target cold and after a warming script, using isolated disk caches. Assert the warming order and fail if the fixture is not exercised so the check cannot degrade silently. Refs #71
Render demangled symbols as conventional full paths without the synthetic /./ module boundary. Keep structured module and relative-path data available through DemangleParsed.
Render functions and constants with path.entity and script roots as extensionless path/name. Document that Demangle is presentation-only and structured callers should use DemangleParsed.
Cache the script-root mangled key once and use the active body cursor to distinguish script and function inference. Remove the redundant ScriptFunc state and the duplicate Script.Mangle representation.
Load each FuncInfo from the shared cache inside TypeFunc and remove redundant invariant checks. Closure settlement still validates every reachable specialization before publishing settled state.
Assert that the first closure walk leaves isOdd partially unresolved, the next walk completes it, and the stable sweep settles both specializations. Derive cache keys through the mangler and remove the obsolete second-script progression.
Move reusable solver test setup to package-level helpers and keep pass transitions inline. Record the same preference for production and test code in the repository guidance files.
Keep project setup concise and point readers to the ABI path-validation section as the single source of truth.
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
Root cause
FuncCachewas shared across scripts, but body-derived binding types were solver-local. A warm specialization could therefore skip body typing because its outputs were already inferred while the next script lacked the variable storage metadata required to lower that body. Output depended on script compilation order.The closure walk also exposed a related issue: a resolved output is not always final. Empty collections can gain a concrete element type, and borrowed strings, tables, or struct fields can later require owning storage. Lowering against the first resolved type can produce wrong output or leak memory.
Design
TypeScriptFuncis the synchronous solver boundary. It sweeps the complete reachable specialization closure until one pass is both complete and unchanged.walkedFuncsmarks a specialization before entering its body, cutting recursive backedges and repeated sibling calls once per pass.Convergingrecords only a strict persisted refinement.Leaf(X)becomingLeaf(I64). Concrete specializations are discovered and solved on later sweeps; provisional ones remain unsettled.FuncInfo.Varsis cleared and rebuilt whenever an unsettled body is walked. An unchanged complete pass validates every walked specialization before publishingFuncInfo.Settledfor the closure.Funcremains the language-level function type. Mutable specialization facts live separately inFuncInfo, which ownsSig,Vars, andSettled;FuncInfotherefore cannot accidentally be used as aType.CompilerretainsFuncCacheandExprCachedirectly.NewCompilercreates both for the owningCodeCompiler; every script compiler automatically inherits both map references from that sameCodeCompiler.NewScriptCompileraccepts no cache arguments, so normal construction keeps the paired metadata on one lifetime.Each script has a validated, path-encoded basename, a trailing
_ekind marker, and a rootFuncInfo. This namespaces top-level variables and expression entries without using the empty function key. Script roots are current-compilation state; only completed function specializations are reused as settled closures.Human-readable demangling follows conventional notation: functions and constants use
module/relative.name, while scripts usemodule/relative/name. Structured consumers useDemangleParsedwhen exact module, relative-path, or symbol-kind data is required. Filesystem-derived script and relative-path names preserve uppercase and Unicode, while ABI-ambiguous separator forms are rejected before mangling.Lowering treats the shared
ExprInforecords as immutable and checks that selected specializations are settled. The PIR plan records the sharedFuncCache/ExprCachelifetime andFuncInfosettlement boundary that future write effects and validation metadata must extend atomically.Performance
walkedFuncskeeps each pass linear in the number of reached specializations, whileFuncInfo.Settledavoids repeating a stable closure at later call sites or in later scripts.Manual stress results (warm build/tool cache, best of three):
2^21call paths)walkedFuncs2^30call paths)17179869184The 130-output regression also passes without the former fixed 100-pass ceiling.
Validation
go test -race -count=1 ./...go vet ./...gofmtandgit diff --checkpython3 test.py— 68/68 passedpython3 test.py --leak-check— 68/68 passed with no leaks[1 1 1]with no leaksReview notes
The commit history intentionally preserves the implementation's evolution. Some earlier commits contain temporary pass-budget and cache-shape approaches that later commits supersede; the final diff and the latest invariant/ownership commits describe the resulting design.
A separate stress case can generate an unbounded chain of new specializations inside one traversal. It reproduces unchanged on
masterand is not caused by this fixed-point loop, so it should be handled as a focused follow-up rather than folded into the #71 fix.Follow-ups: #80 tracks output-file collisions; #82 tracks case-preserving module paths with case-safe cache keys.
Closes #71
Closes #81