fix(lower-go): assign each branch its own closure at a disagreeing join

[mparrett]

Fixes #766.

Why

A closure that captures a local cannot be lifted to a top-level fn, so it lowers to an inline rt.BoxNativeFn. When such a closure was a branch of a conditional, gogen emitted it as the enclosing function's unconditional return and collapsed the conditional to an empty if {} else {}, dropping the other branch:

(defn both-branches [flag x y]
  (if flag (fn [] x) (fn [] y)))

(both-branches false 1 2) returned the then closure. (if flag (fn [] x) nil) returned a closure instead of nil, and a closure over a let binding that only the taken branch established escaped with that binding unset.

The failure is silent. Go rejects the result only when the lowering happens to strand a dead temporary; otherwise the binary builds, exits 0, and returns the wrong branch's value. In the reported case one function failed with declared and not used; a sibling of the same shape compiled clean and had wrong semantics in the native binary, so "it builds" was not evidence the lowering was right.

Root cause

Closures are compile-time entities in this pass: they carry no runtime Go local, and closure-expr re-materializes them at each use site. closure-info* resolved the :block-arg case with some, taking the first incoming edge that resolved to a closure. At a join whose edges disagree — two different fn literals, or a closure against nil — the parameter was still classified as a closure value, so it got no local and both edges skipped assignment. The single use site then materialized whichever closure came first.

Non-capturing closures were unaffected: they lift to top-level fns, so the edges carry :load-var nodes and the join gets an ordinary local.

What changed

• Resolve every incoming edge, and treat the parameter as a closure value only when they unanimously agree. A disagreeing join falls back to a real local.

• emit-assignments-for-target no longer screens the arg side, so each edge materializes its own rt.BoxNativeFn into that local. A template threaded into a DCE-killed param is still skipped by the existing live? and const-param tests on the param.

• The visited guard yields a :cycle sentinel rather than nil, so unanimity can tell a loop's back edge apart from an edge that carries no closure. Without that distinction a loop-carried closure would lose the local-free treatment the lineage walk exists to provide (gogen_ir: residual timeouts + unresolved-dep symbols in lowering #266).

• A :cycle source is set aside rather than counted as disagreement, which makes the surviving answer true only for a walk with that loop header on the stack. Each result therefore carries the set of still-on-the-stack nids it leaned on, and only results that leaned on none are memoized. A parameter's own back edge is discharged where it is found instead of recorded: p = agree(inits…, p) has agree(inits…) as its fixed point, which is a property of the block graph rather than of the walk, so the ordinary loop-carried closure stays fully cacheable.

  A plain boolean "derived through a cycle" flag is not enough in either direction. Excluding only a literally-:cycle result caches a path-dependent concrete answer, and a disagreeing join nested in a loop then loses a branch the same way gogen: a capturing closure inside a conditional is emitted as the function's return value, discarding the branch #766 did — (choose-loop false 1 2) in the fixture below returned 2 instead of 1. Excluding every cycle-derived result instead locks the ordinary loop-carried closure out of the memo, and the per-param-per-edge re-walk of shared block-arg lineage is what made lowering time quadratic to begin with (gogen_ir: residual timeouts + unresolved-dep symbols in lowering #266).

pkg/rt/generated.manifest and pkg/rt/generated.sums are refreshed because lower_go.lg is a declared generator input (#765).

Verification

Two new native-entry fixtures. test/native-entry/conditional_closure.lg covers the acyclic disagreeing joins: closure against a different closure, closure against nil, and a closure over a let binding only one branch establishes. test/native-entry/loop_carried_closure.lg covers a loop-carried closure that a conditional either replaces or retains, which is the case the memo rule gets wrong. Each lowers through the production path, matches a committed AST shape pinning the per-branch assignment, builds, and prints byte-exact output that the pre-fix lowering got wrong. Both semantic mutants die, so the assertions are not vacuous.

make native-entry-gate, make test, and make gogen-diff pass; the engine-parity gate reports 0 output divergence.

make parity-full reports the lower-go leg as diverged, but that is pre-existing: main shows the same single :stress/timeout failure and the same bucket hash d88d9278. This branch passes two more assertions than main on that leg, and single-run wall time was slightly lower on both engines.

[mparrett]

Posted by Claude on @mparrett's behalf.

@nnunley — this came out of a local adversarial review of the PR head. The P1 it raised is real, and reproduced exactly as written: (choose-loop false 1 2) returned 2, from a binary that built and exited 0.

Fixed in dd050cc. Instead of a boolean "derived through a cycle" flag, each resolution now carries the set of still-on-the-stack nids it leaned on, and only results that leaned on none are memoized. A parameter's own back edge is discharged where it is found rather than recorded — p = agree(inits…, p) has agree(inits…) as its fixed point, which is a property of the block graph rather than of the walk — so the ordinary loop-carried closure stays fully cacheable.

That distinction turned out to matter. A plain boolean taint was the first attempt, and it cost a second :stress/timeout on the untagged ir-stress lower-go leg: excluding every cycle-derived answer also locks the ordinary loop-carried closure out of the memo, which is the #266 re-walk.

test/native-entry/loop_carried_closure.lg covers conditional replacement and retention of a loop-carried closure, as you asked.

Status

Green on the new head: make native-entry-gate (both new fixtures, semantic mutants dying), make test, make gogen-diff, and all GitHub checks.

Still open: re-running make parity-check to confirm the deps-set version leaves the ir-stress lower-go timing where it was — the boolean version did not, so this is the number I most want to see before calling it done. I will post those counts here within the hour, and follow up with a fix rather than leave it if they moved.

For reference, the baseline that leg diverges against is pre-existing: main shows one :stress/timeout and bucket d88d9278 on the untagged side.

[mparrett]

Posted by Claude on @mparrett's behalf.

Parity follow-up, as promised — with the honest version of the answer.

The gogen_ir leg is clean and stable on this head: pass=824 fail=0, bucket d41d8cd9, in every run I did. main measures 822/0.

I could not get a trustworthy number out of the untagged leg. It replays the lowering passes through the interpreted engine, its only failure bucket is :stress/timeout, and the count therefore tracks machine load rather than the diff. On this same commit I measured fail=0 at 67s wall and fail=5 at 268s wall, depending on what else was running; main measured fail=1 at 90s. An earlier read of mine looked like this change removed a pre-existing timeout — I don't believe that is established, and I'd rather retract it here than have it quoted back later.

So the timing question the boolean-taint attempt raised is still open on my side. What does not depend on timing, and is green: make native-entry-gate (both fixtures, semantic mutants dying), make test, make gogen-diff at 0 output divergence, and the GitHub checks on this head.

If the untagged lower-go leg is load-bearing for the merge decision, it wants a quiet machine — CI rather than my laptop, which is heavily contended right now.

[mparrett]

Posted by Claude on @mparrett's behalf.

Correcting myself on the last paragraph above: I suggested CI as the place to settle the untagged lower-go leg. That was wrong twice over.

It does not run in CI at all — .github/workflows/go.yml defers it explicitly ("Bootstrap-parity gate (deferred — runs as make parity-full locally)"), which matches the note in the contribution policy. And if it were wired up it would be a worse instrument, not a better one: :stress/timeout is a wall-clock budget (LG_STRESS_TIMEOUT_MS, default 5000ms per defn), so a free shared runner would be more contended than a laptop, not less. The perf-pr job is opt-in behind a perf label and benches the pkg/vm fleet, so it does not cover lowering either.

The useful consequence is that I had conflated two questions that want different instruments:

• Parity — do the two engines agree? Raise LG_STRESS_TIMEOUT_MS until nothing times out, then compare counts and bucket hashes. Contention cannot corrupt that, so it is answerable here and it is the half that matches the policy checkpoint.
• Lowering speed — a separate question, and a pass/fail count was never the right way to measure it. That one does want a quiet machine.

Running the raised-timeout parity now and will post the counts. The speed half I will flag as unmeasured rather than guess at it.

[mparrett]

Posted by Claude on @mparrett's behalf.

Parity numbers, measured the way that actually works: LG_STRESS_TIMEOUT_MS=60000, so the wall-clock bucket cannot fire and the counts stop tracking machine load. Both runs below were taken on a contended machine, which is the point — the method is what makes them comparable, not the conditions.

ir-stress lower-go

	untagged	gogen_ir	verdict
main @ a665761	pass=822 fail=0 d41d8cd9	pass=822 fail=0 d41d8cd9	PARITY
this branch @ dd050cc	pass=824 fail=0 d41d8cd9	pass=824 fail=0 d41d8cd9	PARITY

So the lower-go divergence we were both looking at — the one I reported as pre-existing on main — was a timeout artifact on both sides. Give the corpus enough time and the two engines agree, before and after. No parity divergence, and no bucket delta, which is the policy checkpoint.

The count moves 822 → 824: two ir-stress cases that did not lower cleanly before now do. That is the direction the fix predicts.

Two things I am explicitly not claiming:

• Lowering speed is still unmeasured. The wall times in those runs (377s for main untagged against 53s for the branch) were taken at load ~20 and ~8 respectively and are not comparable. I am not reading a speedup into that, and it is a separate question from parity.
• main's jank leg reported DIVERGED in its run, with compile=1 runtime=2 skips on the gogen_ir side and 239 of 242 files reached. That has the shape of load noise rather than a finding; this branch's jank leg was clean at pass=6311 fail=0 on both engines. Flagging it rather than burying it, but I would not treat it as real without a quiet re-run.

[mparrett]

Posted by Claude on @mparrett's behalf.

Better evidence for the leg I left unmeasured above, from a machine neither of us controls. I ran make parity-check on a free 4-core ubuntu-latest runner, on this branch and on main, at the repo-default per-defn timeout and at a raised one:

ref	LG_STRESS_TIMEOUT_MS	jank	lower-go untagged	lower-go gogen_ir	verdict
main @ a665761	5000 (default)	6311/0 both	822/0 d41d8cd9	822/0 d41d8cd9	PARITY
main @ a665761	60000	6311/0 both	822/0	822/0	PARITY
this branch @ dd050cc	5000 (default)	6311/0 both	824/0 d41d8cd9	824/0 d41d8cd9	PARITY
this branch @ dd050cc	60000	6311/0 both	824/0	824/0	PARITY

822 → 824, no failures on either side, identical bucket hash, OK: all suites parity-identical in all four. That is the same delta I measured locally, now reproduced somewhere contention cannot reach.

It also retires the caveat I raised earlier: the raised timeout turned out to be a workaround for my machine, not something the measurement needs. On a dedicated runner the default 5000ms budget never fires — 48.71s against 49.36s on the untagged leg — so the counts were never at risk there. The :stress/timeout noise I hit was purely local contention.

Whole suite runs in roughly 4.5 minutes. I have written that up separately as an infrastructure suggestion rather than piling it onto this PR.

[nooga]

Nice, tight fix — and the :cycle sentinel is the right call given closure-info* is memoized per-nid for perf (#266).

One observation for future reference rather than a request to change this PR: closure-info*'s shape (resolve a block-param's value by walking incoming edges, needing to survive loop back-edges) is the same problem const-param-map (line ~2941) already solves — "does this block-param provably hold one value on every path?" — but the two solve it with different algorithms:

• const-param-map is an optimistic SCCP-style fixed point: every param starts at :top, lowers to the meet of incoming args, iterated to convergence. Its own comment explains why: a pessimistic "only promote once every input is known" walk deadlocks on a loop-carried cycle, and starting optimistic sidesteps that by construction — no separate cycle marker needed.
• closure-info* is a pessimistic DFS + visited-set, which hits exactly that deadlock on loop-carried closures, and this PR fixes it by bolting on a :cycle sentinel to distinguish "still resolving a cycle above me" from "genuinely not a closure."

Both are correct now, but there are two bespoke "phi consistency across a possibly-cyclic CFG" algorithms living ~2000 lines apart in this file. Modeling closure identity as a lattice value (:top / {:template … :captures …} / :bottom, meet = "equal → same value, else :bottom") and computing it with the same worklist machinery const-param-map (or ir/lattice.lg's) already uses would drop the :cycle sentinel entirely and give the five closure-value? call sites (collect-local-ids x2, local-decls, both sides of emit-assignments-for-target) one precomputed map to consult instead of a lazily-triggered recursive resolver.

Not proposing it here — this fix is small and well-tested, and the unification wouldn't be an efficiency win (SCCP-style fixpoints do potentially several sweeps vs. one memoized pass here). But if another "compile-time value, no local" category shows up later and needs the same treatment, that's the signal to fold closure-info* into const-param-map's machinery rather than adding a third bespoke walker.

[nnunley]

Reviewed with a dual-engine comparison (bytecode vs scripts/lg-compile --entry-frame native binary) on the exact head dd050cc7 against base a6657612.

The fix works as claimed. The conditional-join case:

(loop [i 0 acc []]
  (if (< i 4)
    (recur (inc i) (conj acc (if (even? i) (fn [] i) (fn [] (- i)))))
    (mapv (fn [f] (f)) acc)))

On base, native lowering loses branch identity entirely: [4 4 4 4]. On this head each branch materializes its own closure: [4 -4 4 -4]. Bytecode reference: [0 -1 2 -3].

Related pre-existing defect, not introduced or worsened here, and not blocking this PR: the per-iteration capture value is still wrong in loops on both base and this head. Minimal case:

(defn capture-each [n]
  (loop [i 0 acc []]
    (if (< i n)
      (recur (inc i) (conj acc (fn [] i)))
      (mapv (fn [f] (f)) acc))))

Bytecode: [0 1 2]. Native (base and this head): [3 3 3]. Same shape with a loop-body let binding ([0 10 20] vs [20 20 20]).

Root cause is adjacent to the code this PR touches but distinct from it: closure-expr/box-as-value splice capture expressions (e.g. vm.Int(i)) inline into the inner Go func literal at :load-closed sites. A Go func literal captures the outer local by reference, and the lowered loop mutates that local in place, so all iterations observe the final value. The fix shape is to materialize each capture into a fresh local at the closure creation site (cap_n := vm.Int(i)) before rt.BoxNativeFn, which composes cleanly with the per-branch assignment this PR introduces. Happy to file it as its own issue with the repro if useful.

While fixing #766 independently before finding this PR, I also wrote structural unit tests in the lisp_lower_go_test.go render-assertion style: they pin that rt.BoxNativeFn count per branch matches, that no closure escapes past the function's top-level return, and that a dropped nil branch fails loudly. This PR's implementation is the better one (the path-aware memoization correctly handles the disagreeing-join-inside-loop case mine got wrong), but the tests are complementary to the end-to-end fixtures; patch available if you want them folded in.

[nnunley]

Reviewed at 981e7655106f5f29698c23aef5f6fa03c7ccbc8c. No blockers found.

The active-stack-before-memo ordering, dependency-carrying cycle sentinel, and dependency-free cache gate preserve the loop fixed-point invariant without leaking path-dependent answers. Disagreeing joins retain a real local, and the destination-only edge screen lets each incoming branch materialize its own closure.

Regression coverage exercises distinct closures, closure-versus-nil, branch-local capture, and both retain/replace paths for a loop-carried closure. The generated Go pins the per-branch assignments and the semantic mutants die.

Local validation:

• make lowered
• make native-entry-gate
• go build -tags gogen_ir ./...

All passed. GitHub checks are also green on the reviewed head.

Residual non-blocking risk: memo query order and mutually recursive multi-parameter SCCs are not directly unit-tested. Malformed anchorless CFG cycles and cycles through incomplete push-closed construction remain outside source-producible builder invariants; no valid counterexample was found.