feat(reader): support custom data readers - #770
Conversation
|
|
||
| func taggedLiteralError(r *LispReader, tag string, err error) error { | ||
| message := fmt.Sprintf("reading tagged literal #%s", tag) | ||
| if errors.IsCausedBy(err, io.EOF) { |
There was a problem hiding this comment.
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).
| if found { | ||
| value, err := reader(val) | ||
| if err != nil { | ||
| return vm.NIL, taggedLiteralError(r, tagStr, err) |
There was a problem hiding this comment.
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.
| 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)) |
There was a problem hiding this comment.
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*")) |
There was a problem hiding this comment.
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.
| 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 { |
There was a problem hiding this comment.
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.
|
|
||
| func (r *LispReader) resolveCustomDataReader(tag string) (TaggedDataReader, bool, error) { | ||
| if reader, ok := r.taggedReaders.lookup(tag); ok { | ||
| return reader, true, nil |
There was a problem hiding this comment.
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.
|
Two more findings, on code this PR doesn't touch but that's newly inconsistent because of the fields it adds to
|
|
@nnunley — this went
Nothing else collides. Your docs-index row and the
|
nooga
left a comment
There was a problem hiding this comment.
Converting my Aug 22 comments into a formal request since nothing has moved. The two blocking ones still reproduce on HEAD:
reader.go:1536—taggedLiteralError's EOF branch never calls.Wrap(err), soIsIncomplete()is false for a truncated#uuid "abcwhile it's true for an unterminated(defn foo [x].eval.go:216/237/279—read-string/read-all-string/load-stringbuild their reader with a nil registry, so an explicitTaggedReaderRegistryfromSetTaggedReadersis 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.
60fda52 to
cc6ab3a
Compare
|
Restacked onto current What changed during the restack, beyond the rebase itself:
Evidence on the new head: #768 is restacked on this head in the same pass. |
mparrett
left a comment
There was a problem hiding this comment.
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.
Summary
*data-readers*registration for custom tagged literalsread-string,read-all-string,load-string, ordinary compilation, and child eval contextsTaggedReaderRegistryfor embedding Go programs#uuidand#instPR #768 is stacked on this change and adds the raw
#go{...}reader separately.Validation
go test ./pkg/compiler -count=1— 63 passedgo test -race— 8 passed.lgreader suites — passedgo test -short -count=1 ./...— 1,799 passedmake lint— 0 issuesmake check-generatedjj pushpre-push suite — passedmake bench-ratchetstill reports the known current-main versus forward-baseline mismatch (BenchmarkInitFromLGBand existing IR allocation bars); the exact output is unchanged in kind from the previously documented PR #768 run.