Skip to content

feat(reader): support registered raw Go fragments - #768

Open
nnunley wants to merge 2 commits into
nooga:mainfrom
nnunley:feat/raw-go-fragment-reader
Open

feat(reader): support registered raw Go fragments#768
nnunley wants to merge 2 commits into
nooga:mainfrom
nnunley:feat/raw-go-fragment-reader

Conversation

@nnunley

@nnunley nnunley commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Stack

Depends on #770. Review the child-only delta at:
nnunley/let-go@feat/custom-data-readers...feat/raw-go-fragment-reader

Because both heads live on a fork, GitHub's upstream PR view includes #770 until the base PR merges; the branch itself is a two-commit stack.

Summary

  • extend the per-reader registry with externally implementable raw tagged readers through TaggedRawInput
  • add the default balanced #go{...} reader, preserving Go source while ignoring braces in strings, runes, raw strings, and comments
  • consume raw payloads safely in unselected reader-conditional branches and propagate malformed/truncated input
  • preserve explicit and dynamic data-reader precedence over the default raw #go handler
  • extend docs/guide/custom-data-readers.md with the raw Go reader contract and embedding API

Validation

  • go test ./pkg/compiler -count=1 — 77 passed
  • focused go test -race — 21 passed
  • focused .lg reader suites — passed
  • go test -short -count=1 ./... — 1,813 passed
  • make lint — 0 issues
  • make check-generated
  • normal 8 ms boot smoke — passed
  • guarded jj push pre-push suite — passed; exact head receipt finalized after GitHub propagation

make bench-ratchet was run under the exclusive benchmark baton. It reports the known forward-baseline mismatch on current main (BenchmarkInitFromLGB 26,331 vs 7,028 allocs/op plus existing IR deltas); the reader stack does not change those benchmarked initialization/lowering paths.

@mparrett

Copy link
Copy Markdown
Collaborator

Reviewed as the child-only delta against #770 (feat/custom-data-readers..feat/raw-go-fragment-reader), +591/−58 across 10 files. The upstream view shows +1124/−93 because both heads live on the fork.

The lexer balances correctly in every case I tested: nested braces, braces inside interpreted strings, raw strings, rune literals ('}'), escaped quotes ('\''), both comment forms, division-vs-comment disambiguation, and the empty fragment. Escape state does not leak between lexer states, and the single-rune pushback after the / lookahead is used legally. Precedence — registry, then *data-readers*, then default #go — agrees between the read and skip paths in every case but one, filed below.

One blocking item, in the skip path rather than the lexer.

Blocking: raw readers are not dispatched for a nested #tag

The dispatch added at pkg/compiler/reader.go:1385 is reached only when #tag is the entire form of an unselected reader-conditional branch. Nested inside any collection, the flat delimiter counter at pkg/compiler/reader.go:1273 consumes the payload instead. That counter understands Lisp " strings and \ char literals; it knows nothing about Go backtick raw strings, rune literals, or // and /* */ comments. A brace inside any of those desyncs the count, which then eats the enclosing ). It is the failure described in the comment above that default: branch.

top-level raw string        : 7
nested raw string           : error at 1:24
nested rune literal         : error at 1:31
nested line comment         : error at 2:3
nested in map, Go "string"  : 7      <- passes by accident
nested in map, rune literal : error at 1:37

The last two lines are the same shape with different payloads. The "-quoted one survives because the stray brace sits inside a Go string that the Lisp counter also happens to track. So this is not "nested does not work" — it is "nested works or breaks depending on the fragment's contents," which is the worse of the two to ship.

Not a regression: nested tagged literals were already brace-counted before this branch. But #go{...} is syntax this PR introduces, and docs/guide/custom-data-readers.md says an unselected branch "may invoke them solely to skip that payload safely." That holds only at branch top level. A reader registered through RegisterRaw behaves the same way, so this is not specific to #go.

Fix: have the collection skipper recurse through skipReaderForm when it hits #, rather than counting characters.

The dead-branch error is the same defect

#?(:clj #go [1] :default 7) raises #go requires an opening { from a branch that was never selected, pinned as intended at test/quick_wins_test.lg:498. Every other unknown tag still skips harmlessly: #?(:clj #foo [1] :default 7) reads as 7. Measured at both positions, the behavior inverts:

payload in a dead branch branch top level nested one level
#go [1] (non-brace) error 7
#go{s := `}`} (well-formed) 7 error

Both rows are the same missing dispatch decision in skipReaderForm: at top level the raw reader runs when it should not, and nested it does not run when it should. The same source text means different things depending on nesting depth, in both directions.

That matters for the fix. Making the skipper recurse on # handles the second row and leaves the first, so a decision about non-brace payloads is still needed. The cleanest pairing is to have the built-in #go claim the tag only when the payload opens with {. Falling back to skipReaderForm when a raw reader errors during skipping also works once recursion is in place, but on its own it makes top-level and nested agree while leaving well-formed nested fragments broken.

Running a raw reader while skipping is unavoidable in general: a raw payload is not a Lisp form, so a generic skipper cannot know where it ends. The design is forced; only the failure mode is a choice. Whichever way it goes, a semantic change to reader conditionals belongs in docs/guide/custom-data-readers.md and the PR body, not only in a test assertion.

Smaller items

The side-effect-free contract is on the wrong symbol. It sits at pkg/compiler/tagged_reader.go:76 on TaggedRawReader, but RegisterRaw at pkg/compiler/tagged_reader.go:129 is what someone registering a reader reads. Repeat it there: a registered raw reader runs for branches the program never selects.

The skip path swallows an error the read path treats as fatal. pkg/compiler/reader.go:1571 discards the resolver error with _, present, _ := r.dataReaderResolver(tag, false). A malformed *data-readers* errors when a form is read and falls through to the default raw #go when a form is skipped.

The summary's "propagate malformed/truncated input" is broader than what landed. skipReaderForm gained an error return, but every r.next() failure inside it still becomes return nil. The only new propagation comes from raw readers. Preserving the existing leniency is defensible; the description claims more than the diff does.

Errors never name the tag. The nested failures above surface as reading reader conditional key, at a line and column inside the Go fragment. Nothing points at #go or the raw reader.

Multi-line fragments carry their newlines. #go{⏎…⏎}, which is how anyone would format a multi-line fragment, yields a leading and trailing \n. Fine for display, a silent mismatch for byte-exact fixtures. State the verbatim-including-newlines rule in the guide.

On the #766 connection

Nothing consumes the payload yet: ReadRawGoFragment appears only at its own definition and the default registration, with no emission path on this branch. I also checked the nearest real consumer — the native-entry AST gate reads its expectations from .goexpect.json through encoding/json, so the let-go reader is never involved, and #go cannot reach it without a fixture-format change this PR does not make.

The capability is there for when a consumer arrives. All six Go fragments in the #767 fixtures round-trip byte-exact through #go{...}, and rebuilding conditional_closure as a .lg spec reproduced its functionFragment byte-identically at 284 bytes. The value is real; the PR body currently reads as though the connection is live. "Step one" would be more accurate.

Direction, for whenever a consumer lands

If the eventual answer to a lowering bug becomes "drop to #go{}", that routes around the miscompiler and around the machinery that catches it. #766 surfaced only because go build rejected a stranded temporary; the sibling function of the same shape compiled clean and was silently wrong. Raw Go fragments bypass the invariants the native-entry gate and the parity harness exist to check.

There is also a contract question. Policy §1 requires the standalone interpreter to work with no Go toolchain, and §3 forbids making the VM depend on the AOT path. Today #go{} is an inert string under both engines, so both hold. Once a consumer makes it live, the same source means different things per engine — the divergence parity-full exists to catch, and per #769 that gate is not in CI yet. Cheaper to shape now than after the consumer exists.

Verification

On the PR head: go build ./... clean, go test ./pkg/compiler -count=1 ok, go test -short -count=1 ./... green, make check-generated OK, make lint 0 issues. The reader-conditional behavior above is measured, not read off the diff.

@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. #770 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 commented Sep 1, 2026

Copy link
Copy Markdown
Owner

Holding this until #770 lands — the branch is #770 plus one commit, so the six open points there apply here unchanged. Separately, go vet flags pkg/compiler/tagged_reader.go:72 and :85: TaggedRawInput.ReadRune() doesn't match the io.RuneReader signature (rune, int, error). CI doesn't run vet, so it slipped through — please rename or fix the signature before this comes back for review.

@nnunley
nnunley force-pushed the feat/raw-go-fragment-reader branch from d0194ea to 1330c47 Compare September 4, 2026 04:17
@nnunley

nnunley commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

Restacked onto the new #770 head (cc6ab3af, itself on current main). Head moves d0194ea21330c470.

Three source files conflicted with the #770 review fix and were merged by intent: reader.go keeps the fix's builtin-literal classification alongside this PR's resolveCustomTaggedReader / lookupRawTaggedReader; tagged_reader.go keeps both the fix's registry binding and this PR's raw-reader types; the test file's imports are unioned. Generated artifacts regenerated; make check-generated clean.

Two test reconciliations worth a reviewer's eye:

  1. TestTaggedReaderErrorsCannotMasqueradeAsCleanEOF asserted that a truncated #need-value and a handler returning io.EOF must not read as EOF. The feat(reader): support custom data readers #770 review asked for the opposite and the fix implements it: a truncated tagged literal is incomplete input, exactly like an unterminated (defn, so api.IsIncomplete keeps a REPL prompting, and a handler's EOF keeps its causality (TestTaggedReaderRegistryPropagatesHandlerAndPayloadErrors). The test now guards what still must hold: a non-EOF handler error and a rejected raw payload on complete input are never reported as EOF.

  2. The quick_wins_test.lg assertion #go{if ready { return "}" }} came back from the restack as #go {if ready {return "}"}} and failed. Cause: a jj fix formatter configured for *.lg ran over the commit and, not knowing raw fragments, printed #go{...} as a tag applied to a map. Restored to the original text. This is a general hazard for raw fragments: any Clojure-aware formatter (cljfmt, zprint, editor format-on-save) will do the same to a #go{ payload, so users of this feature need to exclude such files or fragments from formatting. Worth a line in the docs for this PR.

Evidence on the new head: go vet (only #768's pre-existing ReadRune signature note), go test ./pkg/compiler ./pkg/vm ./pkg/rt ./pkg/ir ./test green. Pushed without the local ratchet gate (red on main, #791).

@mparrett
mparrett self-requested a review September 4, 2026 13:25

@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 the child-only delta cc6ab3a..1330c47. Four blocking findings remain:

  1. Nested raw tags are not dispatched while skipping an unselected reader-conditional collection. The collection branch of skipReaderForm uses a flat delimiter counter, so delimiters inside Go raw strings, runes, or comments alter Lisp depth. A disposable regression using a nested, well-formed #go fragment with two right-parens inside a Go raw string fails at review.lg:1:24 with reading reader conditional key instead of returning the :default value. The top-level raw-tag skip path works; the nested path needs to recurse/dispatch raw readers too.

  2. Truncated built-in #go input loses EOF causality. For #go{if ready {, the reader reports unterminated #go fragment but api.IsIncomplete(err) is false. That prevents REPL/API callers from prompting for the rest of a multiline form and is inconsistent with the incomplete tagged-literal contract fixed in #770. Preserve/wrap io.EOF for truncated raw input and add a public-API regression.

  3. The committed quick_wins assertion is red locally and in CI. The reader byte-preserves if ready {return "}"}, while the test expects if ready { return "}" }. Either restore matching fixture whitespace or update the expectation; the current build job fails here.

  4. go vet ./pkg/compiler fails because TaggedRawInput.ReadRune() has the well-known method name with a nonstandard signature. It reports both the interface and implementation and expects (rune, int, error). Rename the method (for example NextRune) or conform to io.RuneReader before exporting this API.

The raw lexer cases themselves and the focused compiler/race suites pass, and the worktree is clean. #770s separately reported require-propagation issue is inherited by the stack and is intentionally not counted again here.

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