Skip to content

feat(reader): support custom data readers - #770

Open
nnunley wants to merge 1 commit into
nooga:mainfrom
nnunley:feat/custom-data-readers
Open

feat(reader): support custom data readers#770
nnunley wants to merge 1 commit into
nooga:mainfrom
nnunley:feat/custom-data-readers

Conversation

@nnunley

@nnunley nnunley commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • add Clojure-compatible dynamic *data-readers* registration for custom tagged literals
  • honor active readers in read-string, read-all-string, load-string, ordinary compilation, and child eval contexts
  • expose a concurrency-safe per-compiler TaggedReaderRegistry for embedding Go programs
  • preserve legacy unknown-tag behavior without an explicit registry while allowing custom entries to override built-in #uuid and #inst
  • document registration, precedence, current limitations, and the embedding API

PR #768 is stacked on this change and adds the raw #go{...} reader separately.

Validation

  • go test ./pkg/compiler -count=1 — 63 passed
  • focused go test -race — 8 passed
  • focused .lg reader suites — passed
  • go test -short -count=1 ./... — 1,799 passed
  • make lint — 0 issues
  • make check-generated
  • normal 8 ms boot smoke — passed
  • guarded jj push pre-push suite — passed

make bench-ratchet still reports the known current-main versus forward-baseline mismatch (BenchmarkInitFromLGB and existing IR allocation bars); the exact output is unchanged in kind from the previously documented PR #768 run.

Comment thread pkg/compiler/reader.go Outdated

func taggedLiteralError(r *LispReader, tag string, err error) error {
message := fmt.Sprintf("reading tagged literal #%s", tag)
if errors.IsCausedBy(err, io.EOF) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

taggedLiteralError's EOF branch never calls .Wrap(err), breaking IsIncomplete() for truncated tagged literals.

The non-EOF branch two lines down does NewReaderError(r, message).Wrap(err), but this branch returns a bare NewReaderError(r, message+": unexpected EOF") — the resulting error's cause is nil even though errors.IsCausedBy(err, io.EOF) just confirmed an EOF was detected.

ReaderError.IsEOF() only inspects its own cause field, so it returns false here. Verified by building this branch: api.Run("#uuid \"abc") (truncated tagged literal) reports IsIncomplete() == false — a hard error — while the structurally equivalent api.Run("(defn foo [x]") correctly reports true. Any REPL/interactive frontend relying on the documented api.IsIncomplete contract to decide whether to prompt for more input will misreport a truncated #uuid/#inst/custom-tag literal as a fatal syntax error.

Fix is presumably NewReaderError(r, message+": unexpected EOF").Wrap(err).

Comment thread pkg/compiler/reader.go
if found {
value, err := reader(val)
if err != nil {
return vm.NIL, taggedLiteralError(r, tagStr, err)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Same taggedLiteralError helper, reused here for a custom TaggedDataReader handler's own returned error: if a handler error happens to wrap io.EOF for reasons unrelated to end-of-input (e.g. some validation failure inside the handler that itself uses an EOF-derived sentinel), its real message gets discarded and replaced with a generic "unexpected EOF" — making the actual bug in the embedder's reader function undiagnosable from the error text.

Comment thread pkg/compiler/eval.go Outdated
return vm.NIL, fmt.Errorf("read-string: expected String, got %T", vs[0])
}
return ReadString(string(s))
reader := newLispReaderWithResolvers(strings.NewReader(string(s)), "<read-string>", nil, execContextDataReaderResolver(ec))

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

read-string, read-all-string, and load-string (here, and the same pattern at ~237 and ~279) always construct their reader with registry=nil — only the dynamic *data-readers* var (via execContextDataReaderResolver) is consulted, never an explicit TaggedReaderRegistry installed via SetTaggedReaders.

An embedder relying on this PR's headline capability — a concurrency-safe per-compiler registry for embedding Go programs — would expect (read-string "#app/id ...") inside compiled/loaded code to see tags registered that way. Instead it silently falls back to legacy passthrough unless the same tag is also mirrored into *data-readers*, an easy-to-miss gap between the two registration mechanisms this PR introduces.


func execContextDataReaderResolver(ec *vm.ExecContext) taggedDataReaderResolver {
return func(tag string) (TaggedDataReader, bool, error) {
dataReaders := rt.NS(rt.NameCoreNS).Lookup(vm.Symbol("*data-readers*"))

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

execContextDataReaderResolver re-derives *data-readers* via rt.NS(...).Lookup(...) on every #tag literal read — including plain #uuid/#inst, which paid nothing before this PR. The 3-case type-switch below (vm.Map / *vm.PersistentMap / *vm.SortedMap) to get a Lookup could also just be a single readers.(vm.Lookup) assertion — all three already implement vm.Lookup's ValueAt/ValueAtOr, and a future Lookup-implementing map type (Record, TransientMap, …) would otherwise silently fall to the "must be a map" error branch instead of working automatically like every other Lookup consumer in the codebase.

Reading a file full of #inst/#uuid literals now pays a namespace lookup + symbol lookup per literal it never used to pay for.

Comment thread pkg/compiler/reader.go Outdated
return vm.NIL, NewReaderError(r, "reading tagged literal value")
return vm.NIL, taggedLiteralError(r, tagStr, err)
}
if !found && tagStr != "uuid" && tagStr != "inst" && r.taggedReaders != nil {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The built-in tag set ("uuid", "inst") is hardcoded twice: here (the not-found gate deciding when to raise "unknown tagged literal") and again a few lines below in the switch that actually parses them. Adding a third built-in tag later requires updating both spots in lockstep — missing the gate update means the new built-in gets wrongly reported as "unknown tagged literal" whenever an explicit registry is installed, even though the switch below would otherwise handle it correctly.

Comment thread pkg/compiler/reader.go

func (r *LispReader) resolveCustomDataReader(tag string) (TaggedDataReader, bool, error) {
if reader, ok := r.taggedReaders.lookup(tag); ok {
return reader, true, nil

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Tag resolution now runs through three independently-coded mechanisms — explicit registry (here), the dynamic *data-readers* resolver, and the hardcoded uuid/inst switch — with precedence expressed only by call order in readTaggedLiteral/resolveCustomDataReader. No test (Go or .lg) exercises the case where the same tag is registered in more than one place simultaneously, so a future refactor of any one path could silently change which handler wins with nothing to catch the regression.

@nooga

nooga commented Aug 22, 2026

Copy link
Copy Markdown
Owner

Two more findings, on code this PR doesn't touch but that's newly inconsistent because of the fields it adds to compiler.Context — flagging as a general comment since neither line falls inside the diff for inline anchoring:

pkg/resolver/resolver.go:152loadSource builds every (require ...)'d namespace's compiler.Context via a fresh compiler.NewCompiler(r.ctx.Consts(), scratch) that never copies r.ctx.taggedReaders/r.ctx.dataReaderResolver from the parent context — unlike Context.ChildForEval, which explicitly propagates both. An embedder that does ctx := compiler.NewCompiler(...).SetTaggedReaders(registry) and compiles a script that (require 's lib.ns) will have lib.ns's file compiled through this fresh, registry-less context — any #tag literal registered only in the Go registry silently falls back to legacy passthrough in that file, inconsistent with the same tag working at the top level of the requiring file.

pkg/compiler/eval.go:339 — the exported ReadString (wired to rt.SetReadEDN for pod EDN parsing) wasn't updated to be ExecContext-aware like the sibling read-string builtin — it still uses plain NewLispReader, so it only ever resolves *data-readers* against vm.RootExecContext. A future caller expecting compiler.ReadString and (read-string ...) to have equivalent *data-readers* visibility semantics will get root-only resolution instead — an easy divergence to miss.

@mparrett

Copy link
Copy Markdown
Collaborator

@nnunley — this went CONFLICTING when #717 landed, and it is smaller than the badge suggests. Both conflicts are frontmatter date stamps:

docs/README.md

<<<<<<< origin/main
last-verified: 2026-08-11
=======
last-verified: 2026-08-21
>>>>>>>

docs/guide/clojure-compatibility.md is the same two keys, last-verified and human-verified. #768 carries the identical pair, so both PRs resolve the same way.

Nothing else collides. Your docs-index row and the *data-readers* prose sit in different parts of both files from #717's os guide row, and git merge-tree merges them without complaint. Take your dates and the rest replays clean.

generated.manifest and generated.sums also read as conflicts on GitHub, but they resolve under the sums driver that make install-hooks registers, so a local rebase settles them. Run make check-generated afterward: the digest that driver writes mid-rebase can be the one from an intermediate commit rather than the tip. Detail in #747.

@nooga nooga left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Converting my Aug 22 comments into a formal request since nothing has moved. The two blocking ones still reproduce on HEAD:

  1. reader.go:1536taggedLiteralError's EOF branch never calls .Wrap(err), so IsIncomplete() is false for a truncated #uuid "abc while it's true for an unterminated (defn foo [x].
  2. eval.go:216/237/279read-string/read-all-string/load-string build their reader with a nil registry, so an explicit TaggedReaderRegistry from SetTaggedReaders is invisible to them; only *data-readers* is honoured.

The other four (resolver overhead and the 3-case Lookup switch in tagged_reader.go:24, the duplicated uuid/inst set at reader.go:1550, the missing precedence test at reader.go:1531, and preserving a handler's own message when it wraps io.EOF at reader.go:1566) are smaller but open. Please address and re-request; #768 is waiting on this.

@nnunley
nnunley force-pushed the feat/custom-data-readers branch from 60fda52 to cc6ab3a Compare September 4, 2026 04:17
@nnunley

nnunley commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

Restacked onto current main (bdd8268c, #730) as a single commit: the review-fix commit that had been staged locally on a merge of the old head with main (6e876b45, "complete custom reader propagation") is squashed into this PR's commit. Head moves 60fda522cc6ab3af.

What changed during the restack, beyond the rebase itself:

  • docs/README.md and docs/guide/clojure-compatibility.md conflicted only on last-verified:; took the newer date and kept the existing human-verified: 2026-08-11.
  • core_compiled.lgb, generated.manifest, generated.sums regenerated (make generate twice, byte-identical); make check-generated clean.

Evidence on the new head: go vet, go test ./pkg/compiler ./pkg/vm ./pkg/rt ./pkg/ir ./test all green, with the clojure-test-suite submodule linked. Pushed without the local ratchet gate, which is red on main itself independent of this PR (#791; fix in #794).

#768 is restacked on this head in the same pass.

@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.

Reviewed head cc6ab3a. One blocking finding remains.

pkg/resolver/resolver.go:152 creates the compiler for every (require ...)d source with compiler.NewCompiler(r.ctx.Consts(), scratch), but it does not inherit the parent context's TaggedReaderRegistry (or its reader resolver / execution context). As a result, explicit per-compiler readers work in the requiring file and runtime string readers but silently disappear inside dependencies.

I reproduced this with a temporary source file containing (def value #review/probe 1), a parent compiler whose review/probe handler returns 42, and (require 'pr770-registry-dep). Reading pr770-registry-dep/value returns 1, not 42: the dependency falls back to legacy unknown-tag passthrough. The temporary regression test was removed after the run. Please propagate the parent reader configuration into freshCtx and retain a require-path regression test.

The earlier EOF and runtime-reader blockers are fixed on this head. The compiler/resolver/API suites, focused tagged-reader race run, make check-generated, and git diff --check pass.

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.

3 participants