Skip to content

feat(aot): report multi-arity entries and group the frame's imports - #692

Open
mparrett wants to merge 3 commits into
mainfrom
wt/425-entry-followup
Open

feat(aot): report multi-arity entries and group the frame's imports#692
mparrett wants to merge 3 commits into
mainfrom
wt/425-entry-followup

Conversation

@mparrett

@mparrett mparrett commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

The two non-blocking notes from the #628 review, kept off that PR so its approval stays on the diff you read. Stacked on wt/425-entry — merge #628 first and this retargets to main.

Multi-arity entries are reported instead of vanishing. A multi-arity (defn -main ([] …) ([x] …)) has no single binding vector to classify, so parse-entry-form returned nil. That is the same answer as "there is no entry in this namespace," so the frame quietly fell back to the VM path and said nothing. It now parses as an entry with an :unsupported shape, which routes into the existing entry-arity-error path: lg-compile prints a diagnostic naming the entry and exits non-zero, the same treatment [first & rest] already gets:

native-entry: -main is multi-arity; the frame calls one Go signature, so give
it a single arity — [], [argv], or [& args] (#425)

Erroring rather than documenting-and-skipping seemed right for consistency: every other unsupported shape already stops the build, and a silent fallback is how this stayed invisible in the first place. The README's arity section now states the single-arity requirement alongside the supported shapes.

The frame's imports are goimports-grouped. One blank line between the stdlib specs and everything else:

import (
	_ "embed"
	"fmt"
	"os"

	prog "aotmod/fib"
	"github.com/nooga/let-go/pkg/rt"
	"github.com/nooga/let-go/pkg/vm"
)

The grouping is applied to the rendered text rather than the AST, because the AST cannot carry it. gogen/render builds a FileSet whose positions are densed gap-free so the rendered text is a pure function of the AST regardless of build concurrency (renderFset). A blank line is a position gap, so the determinism pass removes any group break carried on the tree. Rather than loosen that invariant for a cosmetic win, or pull in x/tools/imports for one import block, the grouping is restored on the rendered text next to the existing header prepend — local to this frame, nothing else in the lowering pipeline sees it.

Verified: the example rebuilds and runs unchanged (fib.native → 55, fib.native x → 5702887), the generated main.go is gofmt-clean, and make test passes. Four new assertions cover both notes — two on the multi-arity diagnostic (public and private), two on the group break.

@nnunley

nnunley commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Reporting multi-arity instead of silently dropping to the VM fallback is the right fix for
nooga's note.

One shape regresses, though. parse-entry-form reads (first body); for
(defn -main ([& args] …)) that is a list rather than a binding vector, so bvec is nil and
multi-arity-body? then sees a seq headed by a vector and classifies it multi-arity. That is
a single-arity entry written in the list form — valid Clojure — and on this branch it now
hard-fails --entry-frame with "-main is multi-arity; give it a single arity", which is both
a stop and the wrong diagnosis. Before this PR the same shape worked, via the fallback.
Unwrapping when the body has exactly one arity list is the whole fix.

Nit: group-import-block matches quoted "embed"/"fmt"/"os" in rendered text. The
quoting makes false positives unlikely and the rationale is documented, but it silently
no-ops if the renderer ever emits a single-spec import or reorders the block; indexing off
frame-imports would not have that failure mode.

Base automatically changed from wt/425-entry to main August 11, 2026 14:50
@mparrett
mparrett force-pushed the wt/425-entry-followup branch from bd93446 to 53928a8 Compare August 11, 2026 14:50
@mparrett
mparrett force-pushed the wt/425-entry-followup branch from 53928a8 to 667cc51 Compare August 11, 2026 19:19
@mparrett

Copy link
Copy Markdown
Collaborator Author

(agent) Good catch on the list-form single arity — that was a real regression from the multi-arity reporting change. Unwrapped when the body is exactly one arity list in 9cd2686; covered [] / [argv] / [& args], docstring, and return-hinted spellings.

Leaving the group-import-block string match for now: the failure modes are renderer changes we don't make, and the grouping test already guards the shape.

mparrett and others added 3 commits August 13, 2026 15:44
Two non-blocking notes from the #628 review.

A multi-arity `(defn -main ([] …) ([x] …))` had no single binding vector to
classify, so parse-entry-form returned nil — which reads as "no entry in this
namespace" and quietly emitted the VM fallback frame. It now parses as an
entry with an :unsupported shape, so the existing entry-arity-error path
reports it and lg-compile exits non-zero, same as [first & rest].

The frame's import block was one undifferentiated group. gogen renders through
a FileSet whose positions are densed so output stays a pure function of the AST
(renderFset), and that pass also drops any blank line the printer would emit —
so the goimports group break is restored on the rendered text instead, local to
this frame rather than by loosening the renderer's determinism invariant.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The mainNotContain field added by the previous commit is longer than the
surrounding names, so gofmt wants the whole block realigned. Pre-existing
on the pushed branch, not introduced by the rebase.
(defn -main ([& args] …)) is valid Clojure, but parse-entry-form treated
the lone arity list as multi-arity and hard-failed --entry-frame. Unwrap
when the body is exactly one arity list; keep true multi-arity rejected.

Co-authored-by: Cursor <cursoragent@cursor.com>
@mparrett
mparrett force-pushed the wt/425-entry-followup branch from 5cfd4a0 to 470284a Compare August 13, 2026 22:52
@mparrett

Copy link
Copy Markdown
Collaborator Author

@nnunley I forgot to tag you after the last changes were pushed. Could you please take another look and see if the concerns were addressed?

@mparrett mparrett added the review-priority/low Minimal review effort left / already approved label Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-priority/low Minimal review effort left / already approved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants