Skip to content

fix(rt): resolve ns alias in LookupOrRegisterNSNoLoad - #627

Closed
nnunley wants to merge 1 commit into
nooga:mainfrom
nnunley:fix-nsnoload-canonical-alias
Closed

fix(rt): resolve ns alias in LookupOrRegisterNSNoLoad#627
nnunley wants to merge 1 commit into
nooga:mainfrom
nnunley:fix-nsnoload-canonical-alias

Conversation

@nnunley

@nnunley nnunley commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Problem

RegisterGeneratedPrimitives (the lginterop-generated //lg:native registrar) Defs primitives via LookupOrRegisterNSNoLoad("clojure.core"). Unlike its loading sibling LookupOrRegisterNS, that path never resolved the namespace alias, so it registered into a distinct clojure.core namespace instead of the canonical core.

Hand-registered primitives masked the bug: they also Def into the canonical ns via installLangNS, so their canonical binding always existed. But a primitive hoisted to a pure //lg:native decl has only the generated registration — it landed in the phantom namespace and was invisible to core.lg's own bootstrap compile, which fails with Can't resolve +.

Fix

Resolve the alias as the first step of LookupOrRegisterNSNoLoad, matching the loading variant (one line + rationale comment at pkg/rt/lang.go).

Why it matters

This unblocks hoisting the anonymous native closures in lang.go into named //lg:native primitives — giving them real stack-trace frames and letting them narrow their signatures (e.g. take ec *vm.ExecContext) instead of capturing free variables.

Verification

Proof-hoisted + - * / to //lg:native decls and confirmed:

  • make generate + lgbgen bootstrap: clean
  • make check-generated: OK on both artifacts (bundle + lowered tree)
  • clojure.core surface: 807 publics, byte-identical before/after
  • arithmetic correct ((+ 1 2 3) (- 10 3) (* 2 3 4) (/ 12 3)6 7 24 4)

This PR is the fix only — the arith hoist was the test vehicle and is not included. check-generated passes with the bundle untouched, since this is a Go-only registration fix.

RegisterGeneratedPrimitives (the lginterop-generated //lg:native
registrar) Defs primitives via LookupOrRegisterNSNoLoad("clojure.core"),
but that path — unlike the loading LookupOrRegisterNS — never resolved
the ns alias, so it registered into a DISTINCT "clojure.core" namespace
instead of the canonical "core". Hand-registered primitives masked this
by also Def'ing into the canonical ns via installLangNS; a primitive
hoisted to a pure //lg:native decl has ONLY the generated registration,
so it landed in the wrong namespace and was invisible to core.lg's own
bootstrap compile ("Can't resolve +").

Resolve the alias as the first step of LookupOrRegisterNSNoLoad, matching
the loading variant. This unblocks hoisting native closures in lang.go to
named //lg:native primitives for real stack-trace frames.

Verified: make check-generated OK (bundle + lowered tree unchanged, this
is a Go-only registration fix); clojure.core surface = 807 publics,
byte-identical before/after a proof hoist of + - * /.
@mparrett

Copy link
Copy Markdown
Collaborator

(stating the obvious): --- FAIL: TestLGBParity (0.03s)

@mparrett mparrett left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug this targets is real: defGeneratedPrimitive records bindings under the canonical key (native_prims_lifecycle.go:87, resolveNSAlias(nsName)) while the ns it Defs into comes from LookupOrRegisterNSNoLoad("clojure.string"), which registers a separate "clojure.string" object. The record and the Def disagree.

Canonicalizing only the NoLoad path moves that breakage rather than fixing it. I built both sides and ran the CI command (go test -tags bootstrap -short ./... -skip TestClojureTestSuite):

ref result
a01d512f (this PR's parent) exit 0, zero failures
59f08e32 (this PR) exit 1, 37 failures in pkg/bytecode, pkg/ir, pkg/wasmhost, test

That reproduces the red build job locally, test for test.

What's happening. RegisterGeneratedPrimitives runs at rt init and calls LookupOrRegisterNSNoLoad("clojure.string"). With this patch that creates the canonical "string" entry as a bare namespace: CoreNS refer plus baselines, no source. A later (require '[clojure.string]) reaches LookupOrRegisterNS and hits the short-circuit at lang.go:607:

if e != nil && !needsLoad { return e }

It finds the pre-created entry, returns it, and string.lg never loads. clojure.string has exactly one generated primitive, upper-case, so the observable result is that the native adapter survives while everything source-defined vanishes:

;; on 59f08e32, -tags bootstrap
(require '[clojure.string :as s])
(s/upper-case "hi")        ;=> "HI"
(s/join "," ["a" "b"])     ;=> Can't resolve s/join in this context

Both work on the parent commit.

That also explains why only the source-bootstrap lane goes red: in the bundled path, LoadCoreBundle marks decoded non-core/non-baseline chunks with MarkNSNeedsLoad after decoding, so the flag covers for the conjured entry. Nothing sets it under -tags bootstrap.

The fix is your call. Either the registrar stops conjuring namespaces it only partially populates, or a namespace conjured by the NoLoad path carries nsNeedsLoad so the real load still runs. I tried the blunt version of the second, setting nsNeedsLoad[name] = true wherever LookupOrRegisterNSNoLoad creates, and it over-fires: the failure becomes unable to load namespace set. So that's a mechanism proof, not a patch.

One correction to the comment, since it's the stated justification. "As the loading callers do" reads as though LookupOrRegisterNS resolves internally. It doesn't — the call sites at lang.go:487 and :495 pass an already-canonical name, and DefNSBare and LookupNS are the two that resolve inside. Worth rewording so the next reader doesn't go looking for resolution that isn't there.

The branch is also 15 commits behind origin/main, so it wants a rebase and a re-run regardless.

Stack implication: #639 hoists 222 primitives to //lg:native, which is what makes a canonical-namespace miss observable in the first place. So this looks load-bearing for #639/#640/#641 rather than separable from them — worth settling here before those get reviewed on top of it.

Happy to re-run the same differential once you've pushed a fix.

@nnunley

nnunley commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator Author

Folded into #639. The resolveNSAlias fix is now the base commit of the hoist PR's branch, because rebasing onto current main revealed the two are mutually dependent and must merge atomically:

  • The hoist needs this fix — hoisted //lg:native primitives register only via the generated registrar (the installLangNS ns.Def path is removed), so without alias canonicalization they'd register into the dead clojure.core namespace and vanish → core boot fails.
  • This fix needs the hoist — on current main, perf(rt): direct-call natives for the hot clojure.core surface #613 landed an arity-lossy direct-call surface (symbol/not=/assoc!/swap! registered at fixed arity, masked by the very dead-namespace bug this fix removes). Canonicalizing alone unmasks that loss and breaks those core fns; the hoist supplies the full-arity replacements.

Closing here; the change ships (and gets reviewed) as part of #639.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants