hexer: closure values as callees/fields/elements (+ env-param primitive) - #2292
Open
tokyovigilante wants to merge 6 commits into
Open
hexer: closure values as callees/fields/elements (+ env-param primitive)#2292tokyovigilante wants to merge 6 commits into
tokyovigilante wants to merge 6 commits into
Conversation
Araq
reviewed
Aug 4, 2026
Araq
reviewed
Aug 4, 2026
| ## key: (env OBJECT type sym, captured local sym). Local syms are | ||
| ## only unique per proc (sem restarts numbering; iterinliner's | ||
| ## `ii temps reset per proc), so a bare-SymId key lets one proc's | ||
| ## capture leak into an unrelated proc that reuses the SymId. |
Member
There was a problem hiding this comment.
Yes, and that means you got the Context lifetimes wrong! Maybe you need a ProcContext inside Context, see how the other passes deal with this issue.
Araq
reviewed
Aug 4, 2026
| ## keys on module-LOCAL closure declarations, so a pure consumer never | ||
| ## ran it and emitted a direct call against the producer's tuple ABI. | ||
| ## Foreign decls are canonicalized to the lifted tuple at load | ||
| ## (canonForeignDecl); match that or a still-raw `.closure` shape. |
Member
There was a problem hiding this comment.
What is this shit? Looks like a gross hack on top of a phase ordering issue. Solve the phase ordering issue instead and remember that when implemented correctly, closures really are a local language feature, unlike methods which really are not.
Araq
reviewed
Aug 4, 2026
Araq
reviewed
Aug 4, 2026
| ) | ||
| # Module-wide pre-scan: ANY `(closure)` pragma anywhere — a proc decl, | ||
| # a proctype alias body, an object field, a parameter type — means | ||
| # pass 2 must run. The per-shape triggers in pass 1 cannot see closure |
Member
There was a problem hiding this comment.
Just run the pass always instead, this pre-scan is a terrible hack.
Araq
reviewed
Aug 4, 2026
| # Republish every routine pass 2 rewrote — same timing rationale as | ||
| # the iter flush above. | ||
| for i in 0 ..< c.shouldRepublish.len: | ||
| programs.publish(c.shouldRepublish[i][0], move c.shouldRepublish[i][1]) |
Member
There was a problem hiding this comment.
Indicates a design flaw, we must rethink the compilation pipeline instead.
Contributor
Author
|
Thanks, this is a bit sloppier than most. Will work through the feedback. |
Member
|
I know -- the last day they dumbed down Claude again... |
lambdalifting owns the closure env *param* — its name (`ep.0`) and the
`addEnvParam` emitter — as private symbols. But that param is a cross-pass
contract: any pass that emits a lowered closure signature must produce the
byte-identical env slot, or consumers type the same symbol two ways. Today
only lambdalifting pass-2 emits it; the follow-up cross-module foreign-decl
canonicalization (rewriting decls loaded from other modules' indexes to the
lifted shape) must emit the same slot.
Move the primitive down into coro_transform — the shared lower module
lambdalifting already imports — exported as `ClosureEnvParamName` /
`addClosureEnvParam`, next to `RootObjName`, `BareRootObjName` and the
wrapper-signature shape that already live there for exactly this reason
("both passes stay in lock-step automatically"). Renamed to avoid clashing
with coro_transform's own `EnvParamName` (`this.0`, the coroutine env). Pure
relocation: identical emitter behaviour, lambdalifting now calls the shared one.
Foundation for upstreaming the closure-value-callee lowering and the
cross-module closure ABI as a coordinated series that both build on this one
env-param definition. No behaviour change; build + self-host boot green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Calling a closure through anything other than a direct symbol — an object field (`d.handlers[i].handler(e)`), a captured local, a param, a seq element or a loop variable — crashed the backend (lambdalifting env.s==0 / eraiser ParamsTagId) or miscompiled into calling the raw tuple. Six coordinated fixes, one per gap the repro chain surfaced: - pass 1 trCall: a LOCAL callee must still run through `tr` — a cross-proc use in call position is a capture like any other; only routine-symbol callees keep the "a call does not escape" exemption. Without this the enclosing proc never created an environment for a captured-and-called closure. - pass 2 closure-value tupconstr: fall back to a nil env when no enclosing environment exists (toplevel closure values), mirroring the static-call lowering. - genCall: resolve an `(envp ...)` callee's type via the new envFieldType table (typenav cannot type envp nodes) — otherwise wantsEnv never fired for captured callees; and the expression-callee path now calls the temp's `.0` fn slot instead of the whole tuple (and types the temp via treType). - genObjectTypes: env fields go through treType, not tre — a captured lambda's type is decl-shaped `(proc ...)`, which tre's stmtKind dispatch would lift as a declaration. - pass 2 TypeS: type-declaration BODIES now get the closure lowering (object fields like `Handler.handler`, generic instance payloads like seq[proc()]'s data) — previously taken verbatim, so the field type stayed a raw fnptr while every value stored into it was the lowered tuple. - treProcLift: republish rewritten routine decls (deferred to after the walk, like the iter shouldPublish flush) so downstream passes type calls against the LOWERED signature — xelim's temp for an inlined `rawData` call otherwise got the pre-lowering closure type. Known remaining gap (separate work): the env ref is stored into the closure tuple through a plain cast, which the refcounting passes do not see through — a closure that outlives its creating scope reads a freed env. Tracked as the closure-env lifetime issue.
A `{.cursor.}` local captured by a `.closure` proc that assigns and reads
through it crashed hexer:
[Bug] could not find symbol: nodeRef.0
Lambda-lifting's capture analysis only hoisted locals of kind
{ParamY, LetY, VarY, ResultY} into the closure environment. A cursor local is
registered as CursorY (desugar lowers `let {.cursor.}` to a `(cursor ...)`
statement), so it fell through: the closure body kept a bare cross-proc
reference to the outer symbol, never rewritten to an env access. The following
duplifier pass opens a fresh scope per proc and does getType on that symbol,
which lives in no scope it can see — falling through to the typenav
"could not find symbol" ICE. Plain (non-cursor) captures escaped because their
kind was already in the set; hikaru has hundreds that work.
Add CursorY to the capture-eligible set so the cursor local is hoisted like any
other. But a naive hoist re-forms the exact ref cycle the cursor exists to
break: capturing by owning copy makes the heap env strongly own the object that
owns the closure (node -> releaseView -> env -> node), so node never frees.
Two coupled parts keep the non-owning semantics:
- genObjectTypes emits the env field with a `(cursor)` pragma when the captured
local was a cursor. The lifter already skips `.cursor` fields in the env's
auto-derived =destroy/=dup/=copy (unravelObjField), so the env holds a
non-owning alias and the cycle stays broken.
- duplifier's trAsgn treated only cursor *locals* (Symbol lhs) as raw-bitcopy
targets; the hoisted store is `env.field = value` (a DotX through a cursor
field), which still got `=dup`'d — reinflating the object's rc into the env.
writesThroughCursorField extends the cursor-lhs check to a write through a
`.cursor` object field, mirroring the constructor-side isCursorField, so the
store is a plain bitcopy.
Test: tests/nimony/closures/tclosure_capture_cursor.nim — the cursor breaks a
node->closure->env->node cycle; an embedded destroy canary proves the node is
destroyed exactly once (no leak, no double free) while the closure still reads
and writes through the cursor after assignment (valgrind: 0 errors, 0 leaked).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ping
copyWithMapping was a flat token walk that applied the local-rename
mapping to EVERY Symbol — including '(dot obj FIELD)' selectors and
'(kv FIELD val)' keys. sem's name.N numbering is per module, so a
foreign type's field sym can be the same interned string as a local
the mapping renames: render_graph's 'var writes = false' collided
with types.nim's 'RenderPass.writes' field and the nested
'for w in pass.writes' loop was buffered with the FIELD renamed to
the iterinliner temp ('no member named X60Qii_10').
Rewritten as a recursive walk with the same DotX/DdotX + KvU guards
inlineLoopBody and replaceSymbol already have.
Regression tests for the closure-value lowering: a closure value used as a
callee (param/captured/index/loopvar), env-projected field callees, captured
`{.cursor.}` locals hoisted into the env, and local/env name collisions in
object-constructor fields. All pass on the lowering in this series; the
cross-module (`xmod`) and iterator-ctor cases remain for the follow-up.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ut it Araq review of nim-lang#2292 (lambdalifting.nim:1027): "xelim got fixed in the meantime so this whole machinery might not be necessary anymore." The shouldRepublish machinery snapshotted every lifted proc's rewritten (env-param-appended, closure-tuple-lowered) signature in treProcLift and re-published it at end of pass 2 so downstream passes (xelim temp typing, lengcgen inlining) would type calls against the LOWERED signature rather than sem's original. With the xelim aggregate-read-through-call fix (5625b2a4) on integration, xelim types those calls correctly without the republish. Verified with it removed: closures 30/30 green; gateway (large cross-module closure + lengcgen-inlining consumer) builds clean. Also removes half of the :1693 deferred-publish "design flaw" surface (only pendingIterSigs remains). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
tokyovigilante
force-pushed
the
fork/pr/closure-value-lowering
branch
from
August 6, 2026 22:58
8eaefdb to
7bacf71
Compare
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
3 participants
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.
Closure values (not just proc symbols) used as callees, fields, and seq elements
currently trigger internal compiler errors in hexer (
could not find symbol: f.0,nifcore c.rem==0, …). This lowers them.Structured as the start of a series so it can land in pieces:
coro_transform— moves the lowered-closureenv param (
`ep.0+ emitter) out oflambdalifting's privates into the sharedmodule, next to
RootObjName/wrapper-shape. Pure relocation; it's the contractevery closure-signature-emitting pass must share.
lowering, built on that primitive.
Built against current
master, not ported from an older branch — sits on top of#2251 (
c2d25cc9) rather than fighting it (several pre-#2251 fixes turned outunnecessary and were dropped).
Verified:
hastur build all+hastur all682/682 + self-hosthastur boot, all green.Deferred (want to agree the shape first): the cross-module case needs a foreign-decl
canonicalizer reusing the same
`ep.0primitive, but it overlaps #2251'snamed-type-body lifting — the iterator ctor-collision case rides with it.