feat(ir): def+name* IR-compile seam — top-level (def x (fn …)) via the IR path - #647
Conversation
mparrett
left a comment
There was a problem hiding this comment.
Smallest of the four by diff, though not by behavior — it inserts a live call into defCompiler's path. Three things before it merges.
The seam is inert without #650. maybeIRCompileDefFnArg resolves ir.passes.pipeline/compile-def-fn-value and returns nil when that lookup fails. The var isn't on main and isn't in this PR — it's defined in #650's pipeline.lg diff. I built this branch alone and the var resolves to nil; built with #650 on top, the hook fires. So merged on its own this compiles, passes CI, and never runs, and with no test in the PR nothing would catch it. Either move the compile-def-fn-value defn here, or retarget this onto #650 and land them together.
Nothing in the repo produces the shape this hooks. name* has no definition on main — the only traces are a stub in pkg/ir/lisp_constfold_test.go:701 ("name*": '(defn name* [a b c] b)') and the unwrapping in passes/inline.lg. Even with #650 merged, no in-repo code constructs (def NAME (name* … (fn …) …)). That makes the coverage question sharper than "add a test": what exercises this seam, and can a test be written against main at all? I'd want that answered before it goes into defCompiler with zero coverage.
Strict mode. The seam consults clojure.core/*ir-compile* but never *ir-compile-strict*, and every early return is a bare return nil. The doc comment says the silence mirrors the defn macro's hybrid path, which held when #625 opened — #580 merged about a day later, adding *ir-compile-strict* (core.lg:991, throw at :1036) and the bytecode-path census specifically to make this class of fallback visible. As written the new seam is a hole the census can't see. Should it honor strict?
Separately: ir_bridge.lg adds ir/chunk-emit-pop-n, commented as "used by the branch pass-through fast path", but nothing calls it — not here, not in #648/#649/#650, and not in closed #625 either. Either its caller is in a piece that hasn't been posted, or it and the 24 generated lines behind it should come out.
4aa1528 to
408b874
Compare
|
Reworked to land self-contained. Addressing each point: Inert without #650 — fixed. Moved the Coverage / what exercises the seam — added Strict mode — now honored. The seam consults
Verified: compiler + ir Go suites green, |
|
What is in it:
The rest is now dead weight. It also made Rebase onto I have not force-pushed it away. Would like to not get in the habit of writing to your branches. On review state: my changes-requested is stale, not standing. You reworked this and force-pushed on 31 July, then replied point by point covering four items — inert without #650, coverage, strict mode, and dropping |
25261cd to
5a80eaa
Compare
Status Update: All Round-1 Items AddressedRework completed on 2026-07-31 covers all four original review points:
Current state:
|
nooga
left a comment
There was a problem hiding this comment.
LGTM. All three blockers from the 07-30 review are fixed: the seam is now self-contained (compile-def-fn-value lives in this PR, no dependency on other open PRs), the "can this even be exercised" concern is answered with a non-vacuous strict-mode throw test, and *ir-compile-strict* is honored on every early-return path.
Verified:
go build ./...clean, merges cleanly with currentmainlocally (see note below)go test ./pkg/ir/... ./pkg/compiler/... ./pkg/rt/...and the newtest/ir_def_namestar_seam_test.lg(5 deftests / 7 assertions) all pass- Confirmed no duplication vs. #598/#661 — reuses
ir.build/return-hinted-arity-vector?/unwrap-arity-vector
One nit worth a one-line fix: in compile-def-fn-value (pipeline.lg:773-783), when the arg vector carries a return-type hint (^long [n]), the code builds defn-form from the unwrapped vector rather than the original hinted one — unlike the main defn-compile path, which keeps the hint and lets build-fn strip it internally. Nothing consumes that tag on the bytecode path today so this is inert, but it'd silently diverge from the main path's behavior the moment something does.
Also: GitHub shows this as CONFLICTING against main, but a local merge resolved with zero real conflicts — just needs a rebase push to refresh GitHub's mergeability check before landing.
5a80eaa to
d47ac24
Compare
mparrett
left a comment
There was a problem hiding this comment.
Re-reviewed d47ac240. All four points from my 07-30 review are addressed, and my stale CHANGES_REQUESTED was the only thing still blocking this — clearing it now.
- Inert without #650 — fixed.
compile-def-fn-valueis defined here (pipeline.lg:764), so the hook fires when the branch is built alone. - Nothing exercises the shape — fixed.
test/ir_def_namestar_seam_test.lg, five deftests. - Strict mode — fixed, and better than what I asked for. The seam separates a shape mismatch, which stays silent because it is not a miss, from a matched-but-unlowerable form, which errors so the #580 census can see it. The comment at
compiler.go:47-51states that distinction outright. - Unused
chunk-emit-pop-n— removed, along with the generated lines behind it.
The coverage answer holds up under falsification
My hardest objection was whether this seam can be exercised against main at all. The test's argument is that a multi-arity name*-wrapped fn must throw under *ir-compile-strict*, and that a silent compile would prove the seam never ran. I checked that directly by stubbing maybeIRCompileDefFnArg to return immediately:
- seam enabled:
Tests: 5 Pass: 7 Fail: 0 Error: 0 - seam stubbed off:
Tests: 5 Pass: 4 Fail: 1 Error: 1
So the assertion is load-bearing rather than decorative. All five deftests register and run.
On the outstanding nit
The defn-form-from-unwrapped-vector point is already fixed: the current code passes args-vec, the hinted original, and uses unwrapped only for the variadic? and arity computation. That landed in d47ac240, after the approving review, so the review reads stale in the favorable direction.
No findings
Two things I checked and am not raising. The docstring in compile-def-fn-value sits after the arg vector, which makes it an inert expression rather than a docstring — but seed-inline-registry-from-cache (:58) and realize-specialized-fold! (:135) in the same file do the same, so this is the file's existing convention rather than a defect in this change. And make generate dirtied the digest in my worktree only because I had rebased; on the pushed head generated-artifacts is green.
Before landing
It is CONFLICTING only because five PRs landed on main today. A local rebase applied with no real conflicts, but make generate was needed afterward — committing that regenerated digest is the step to not skip, since omitting it is what is currently reding CI on #675.
d47ac24 to
9baf6fc
Compare
….)) via the IR path Extracted from nooga#625 [1/4]. Adds the compiler.go seam that routes a top-level single-fn def through *ir-compile* (top-level-only, single-fn, silent fallback, FormSource preserved) plus the ir_bridge.lg hook.
The seam must unwrap return-hinted arity vectors (with-meta forms) before checking for single-arity. This aligns with nooga#661's b170a08 fix for the same issue in the pipeline's defn compilation path. Fixes: multi-arity detection failure on (def x (name* ... (fn ^T [args] ...))) Tests: go test ./pkg/ir passes; check-generated passes
9baf6fc to
609c8a9
Compare
Summary
IR-compile seam for def+name* (grammar-style rule defs) — routes top-level
(def NAME (name* ... (fn ...) ...))through the IR pipeline via compile-def-fn-value.AC-2 Verification (Seam Exercise)
The seam is proven by
test/ir_def_namestar_seam_test.lgcase 5 (seam-strict-throws-on-unlowerable-shape):*ir-compile-strict*, a multi-arity name*-wrapped fn throws an error mentioning "ir-compile-strict" and "multi-arity"Shape is grammar-specific (not in main codebase by design); shape-specific test + non-vacuous proof satisfies workflow seam rule.
Return-Hinted Arity Vector Fix
Aligned compile-def-fn-value with #661's fix (b170a08): unwrap return-hinted arity vectors before testing for single-arity. Fixes forms like
(def x (name* ... (fn ^long [args] ...))).Base & Merge Order
Base:
main@upstreamMerge order: 3rd (independent from #648/#649, stacked after their landing)
Verification
go build ./...passesgo test ./pkg/irpasses (including seam test with return-hinted cases)make check-generatedpasses