Compile captured closures' own bodies, not just their creation - #53
Merged
Conversation
Op::MakeClosure (previous release) let a function that CREATES a closure compile even when that closure has captures, but always discarded the closure's own compiled body, forcing every invocation back through the interpreter regardless of list size -- reduce()'s own self-recursive accumulator, and any curried closure returned from functions like fnliterals.scad's f_1arg()/f_2arg()/f_3arg(), never benefited from compilation at all. Registers a captures-having closure's chunk for compiled invocation too, with Op::LoadUpvalue/Op::MakeClosure falling back to ctx.let_ (rooted at the closure's own capturedLet snapshot) whenever the live call-stack walk misses -- the same mechanism a genuinely escaped closure already needed. A new CompiledChunk::selfDecl field lets Op::MakeClosure tell a genuinely-local capture from one bubbled up by a nested literal (needed once a captures-having chunk's own nested MakeClosure sites can actually run, not just get discarded). Also fixes a pre-existing scope bug this exposed: isolatedCallCtxFor (the tail-call trampoline's hop-context builder) used the caller's own ctx.scope instead of the callee's declNode.scope(), unlike every other call-resolution function -- invisible until a captures-having closure could ever actually be trampolined into. Deliberately excludes a self-referential recursive closure (the reduce()/accumulate()/while() idiom, where the closure calls itself by name) from registration: its own name never resolves as a compile-time upvalue (LetOp's declareLocal for it runs only after compiling its own RHS), so it falls through to Op::LoadFree on every call -- registering it for compiled invocation turned an O(n) reduce() into O(n^2) (confirmed empirically against a real list), since each recursive call's fallback re-derivation nests its own capturedLet one level deeper than the last. This is a pre-existing gap (confirmed present in the already-published v0.7.0 too, unrelated to this change) that stays open for now -- closing it needs the self-reference to resolve as a real upvalue at compile time, not a follow-on to bundle in here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Jul 30, 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
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
map()/filter()already compiled fully (no closure of their own).reduce()/accumulate()/while()'s own self-recursive helper closure, and any curried closure (e.g. fnliterals.scad'sf_1arg()/f_2arg()/f_3arg()), always ran fully interpreted when invoked even after Op::MakeClosure let the creating function compile — the closure's own compiled chunk was discarded outright.Op::LoadUpvalue/Op::MakeClosurefall back toctx.let_(rooted at the closure's owncapturedLetsnapshot) whenever the live call-stack walk misses — the same mechanism an escaped closure already relied on. A newCompiledChunk::selfDeclfield letsOp::MakeClosuredistinguish a genuinely-local capture from one bubbled up by a nested literal, needed now that a captures-having chunk's own nestedMakeClosuresites can actually execute.isolatedCallCtxFor(the tail-call trampoline's hop-context builder) used the caller'sctx.scopeinstead of the callee's owndeclNode.scope(), unlike every other call-resolution function in the file.reduce()/accumulate()/while()idiom, where the closure calls itself by name) from registration: the self-reference never resolves as a compile-time upvalue (LetOp'sdeclareLocalfor it runs only after compiling its own RHS), so it falls through toOp::LoadFreeon every call. Registering it anyway turned an O(n)reduce()into O(n²) — confirmed empirically against a real list, and confirmed to be a pre-existing issue already present in the published v0.7.0, unrelated to this change. Closing that gap needs the self-reference to resolve as a real upvalue at compile time; left open as follow-up work.Test plan
MultiLevelCurriedClosureInvocationRunsCompiled(f_1arg-style two-level nested closure correctness),SelfReferentialRecursiveClosureStillRunsInterpretedAndCorrectly(guard keepsreduce()'s own idiom correct and interpreted).reduce()/f_1arg()correctness at small and large scale; confirmed the O(n²) case is unchanged from the published v0.7.0 baseline (not a new regression), and that the containsLoadFree guard doesn't accidentally affect map()/filter()/curry-adapter cases.🤖 Generated with Claude Code