fix(syntax): stop the formatter dropping reader prefixes - #100
Merged
Conversation
`edit format` silently dropped reader prefixes at exit 0, with output
that reparses -- the corrupted-Lisp-still-reparses failure mode. On the
Clojure standard library it turned
(let ~(subvec bindings 0 2) ...) -> (let (subvec bindings 0 2) ...)
and in system-fare-utils `(symbol-macrolet ,(loop ...) ,@Body)` lost its
unquote, after which the `loop` was re-laid-out as a binding list.
**The stated mechanism was wrong in every particular.** `BODY_FORMS`
lives in `reindent.rs`, whose only consumers are `SyntaxTree::reindent`
and the `inspect indentation` report -- `edit format` never calls it, so
the `("do", 2)` entry cannot affect this, and the theory did not explain
the Common Lisp `let` repro at all.
What actually happens: `Formatter::format_node` writes a node's reader
prefixes before dispatching on its head, so a shape renderer may open
its **own subject's** delimiter freely. Six renderers also open a
*child's* delimiter -- the binding list of `let`/`do`, one binding
entry, `flet`/`labels` bindings and each binding, and `cond`/`case`
clauses -- and nothing wrote that child's prefix. The corruption is
dialect-independent; fixing the dialect table would have left the CL
`let` case broken.
Fixed with a shared `carries_reader_prefix` predicate at those six
sites, handing a prefixed child back to `format_node`. This is also the
right *layout* answer: `` `(a b) `` in a `let`'s binding slot is a
quasiquoted datum, not a binding list. Opaque reader forms needed no
entry -- the parser gives them `NodeKind::Atom`, so the existing guard
already covers them, verified before assuming it.
Second bug, in the parser: a reader prefix immediately before a closing
delimiter was dropped in every dialect -- `(a ')` parsed as `(a)`.
`form()` accumulated prefixes, found a close delimiter, and called
`close_list()` without consuming them. It now returns
`MissingReaderForm`, exactly as a prefix at EOF already did, so `form`
and its scanning twin `skip_form` finally agree. PR #96 pinned this as
a *stated* expectation precisely so a fix would have to update it
deliberately; that test now asserts the refusal.
Differential over 5353 deduplicated files from nine dialects:
formatted output differs between binaries : 27
tree-inequality (BEFORE binary) : 27
tree-inequality (AFTER binary) : 0
newly refused by the AFTER binary : 0
The 27 changed files are **exactly** the 27 the old binary corrupted;
the other 5326 are byte-identical.
The oracle is tree equality, added as invariant 6 in `tests/corpus.rs`
with spans excluded. Its negative control is the important part:
reverting one guard makes it fail with "reader_prefixes: [Unquote]
became reader_prefixes: []" while **idempotence and the reparse check
stay silent** -- which is why this went unnoticed. That oracle also
cannot see the second bug, since input and output parse identically
wrong; only the refusal catches it.
Janet's column-sensitive long strings are deferred, and the reason is
not effort: `stringend` strips `indent_col` bytes after each newline
only if all of them are spaces, so moving the literal flips a mode
rather than shifting a value. Both candidate fixes are real design
decisions -- freezing every form containing one (29 of 210 corpus files,
and in docstring-heavy Janet that is most `defn`s), or teaching the
strictly top-down column model that an atom can pin its column. Scope
measured with Janet itself as oracle: 24 of 210 files changed meaning
before, 23 after, of which 22 are pure long-string cases and the 23rd is
the separate bare-`@` reader divergence.
treefmt formats 6 tracked files; all 6 are byte-identical under both
binaries, so no recipe regeneration is needed.
This was referenced Aug 3, 2026
takeokunn
added a commit
that referenced
this pull request
Aug 3, 2026
…ngs (#103) * fix(syntax): implement Hy's string prefixes, shebang and bracket strings Hy string prefixes were unimplemented across the board -- `f"..."`, `r"..."`, `b"..."`, `rb"..."`, `t"..."`. The atom scanner ran past the opening quote and stopped at a delimiter *inside* the literal, so `r"a)b"` closed its enclosing list early. f-strings are one instance of this, not the whole bug: it accounts for **417 of 469** baseline parse failures. Two more, both silent: `#!` shebangs were not stripped (393 files parsed at exit 0 with the shebang as two junk atoms), and bracket strings `#[[...]]` were parsed as *code* -- `#[[(defn evil [] 1)]]` yielded a real `defn` node inside a raw string, which is the shape where a lint rule fires on text that is not code. `#[delim[...]delim]` was silent too, not a loud failure as first reported. Measured against HEAD with the same code, over 2824 readable files: **493 failures -> 116**, with 378 newly parsing and 1 newly failing. Every failure class strictly decreased. Against **Hy 1.3.0's own reader** as external oracle, files we refuse that Hy accepts fell from **308 to 15**, with zero new over-refusals; all 15 already failed at HEAD with byte-identical errors. The one newly-failing file is a true positive -- real Hy refuses it too (`LexException: invalid string prefix`). It previously "passed" only because the f-string was never scanned. f-strings are modelled as **one opaque atom**, deliberately. `read_fcomponent` calls `parse_one_form`, so the interpolations are arbitrary code and finding the closing quote needs a sub-reader -- `f"{(str "}")}"` is legal. But `ExpressionKind` is only Root/List/Atom, so interleaved text-and-forms needs a new node kind rippling through the formatter, edit engine and every rule; and children are *editable*, so the formatter would reindent inside the literal segments between interpolations. Exposing them trades the `#[[...]]` silent-corruption class for a new one. Blind beats wrong, and today the whole file is invisible. A surprise worth recording: **format specs are nearly unreachable.** `:` is not in `NON_IDENT`, so `f"{x:>10}"` interpolates the *symbol* `x:>10`, and `f"{x:>{width}}"` is a LexException. You need `f"{x :>10}"`. `~` as a reader prefix is deliberately **not** included, though it was implemented and measured. Several formatter paths open a child list without writing that child's prefixes, so a prefixed list is emitted with its prefix deleted -- making `~` a prefix aimed that at the most common construct in Hy macro code, and `edit format` newly changed the meaning of 14 files. PR #100 has since fixed that family; `~` is the obvious next step, and `hy_unquote_is_not_yet_a_reader_prefix` pins the known-wrong reading so whoever does it finds the reasoning. Other dialects: **42142 file-parses, zero differences**, full-tree hash pre- vs post-change across all nine plus the permissive `Unknown` reader. A unit test pins `has_prefixed_strings`/`has_bracket_strings` false and `#[[a]]` as a *list* for all ten. Rebased across #96, #98 and #100, all of which edit the same files. The `classify_reader_macro` dispatch table was resolved arm by arm rather than by side: base had the group `Unknown | Lfe | Hy | Carp`, #98 pulled `Lfe` out and this change pulls `Hy` out, so each side's group line still named the other's dialect. Taking either side would have silently deleted a dialect's reader support. A 29-case probe confirms every recent PR's behaviour survives -- Janet's `'`, Clojure's spaced `#:ns {...}`, LFE's `#B(`/`#M(`/`#S(`/`#"`/`#\`/`|...|`, and #100's `MissingReaderForm` on `(a ')`. One pre-existing issue found and not touched: 13 of the 15 remaining over-refusals are the `,`-as-`Unquote` divergence colliding with #100. Hy's empty tuple is `(,)`, and a trailing `,` before `}` is common, so those are now hard refusals. `classify_hy`'s comment claims `,` "occurs at a token start essentially never" -- the corpus says 13 files, including `hylang/simalq`, `kanaka/mal` and Hy's own `contrib/walk.hy`. Live on main today, and the obvious next Hy item. * perf(syntax): keep the Hy string check off the other dialects' parse path CI's bench gate measured `parse-scaling/reader-conditional` +11.7% at 1MiB and +10.6% at 8MiB. Both sizes moving together by the same amount is the real-regression signature, not the `edit-all-loop` measurement artifact -- and unlike that one, this branch genuinely does touch the parser's inner loop. `at_hy_string` is called once per *form* from `parse_form`'s match and again from the discarded-form scanner. Its dialect test sat inside `hy_string_prefix_width`, one call deep, with a second `has_bracket_strings()` test after it -- so for the other nine dialects it was two guarded calls and two byte reads per form rather than one comparison against a loop-invariant field. Hoisted the dialect gate to the top and marked the function `#[inline]`. On an 8MiB Common Lisp document that is millions of calls that can only ever answer `false`. This is the ordering the Carp reader change adopted deliberately for the same reason: `self.dialect` is loop-invariant across the per-byte and per-form calls, so testing it first lets the rest fold away.
takeokunn
added a commit
that referenced
this pull request
Aug 3, 2026
`docs/LanguageGuide.md:170-173` defines `&` and `@` under a literal
"Reader Macros" heading -- `&x ;; same as (ref x)`, `@x ;; same as
(copy x)` -- but Carp routed through `classify_legacy`, which implements
neither. So `@(f x)` split into a bare `@` atom **plus a sibling**,
inflating the enclosing call's arity in **116 of 248 files (47%)**, 1493
atoms in all. Byte spans survived, so round-trips were lossless; what
broke was structure, which makes any argument-counting analysis unsound
for Carp.
Also fixed: `@"..."` silently split string literals, because `@` glued
to the next token and swallowed the opening quote; character literals
were unrecognized, so `\{ \} \[ \] \( \) \"` were read as real
delimiters; and `#"..."` Pattern literals were unrecognized.
Three more the original brief missed, all real: **`~` (deref) is also a
reader macro** (76 glued atoms in 15 files); **`$[...]` static arrays**
(55 bare `$` atoms, the same arity-inflating defect); and **`,` is
whitespace in Carp, not unquote** (39 sites, where the legacy reader
gave `max` and `val` phantom `Unquote` prefixes in `[min, max, val]`).
`src/Parsing.hs` settles the shape: `readerMacro` is `string macroStr`
then `expr <- sexpr`, recursing into `sexpr` rather than `atom`, so
prefixes bind any form, `@@x` stacks, and `@"..."` is a copy of a
genuine string literal. `validCharacters` excludes `& ~ @ % $ #`, so a
sigil can never be inside a symbol. `aChar` is `\` then one character
with no terminator, so `\{` and `\ ` are valid.
Corpus differential over all 248 files: parse failures **6 -> 0**, 171
trees changed, **1623 child slots removed** and reconciled
token-by-token, **0 files gained atoms**.
`%`/`%@` (Carp's real unquote) is deliberately deferred: it needs two
new `ReaderPrefix` variants threaded through 40+ sites and *adds*
findings inside macro templates, so it wants its own FP audit. Leaving
it keeps templates inert, which is the suppressing direction. `r"..."`
has zero corpus occurrences.
`Ref`/`Copy`/`Deref`/`StaticArray` are new `ReaderPrefix` variants
rather than reuses of `Function`, whose `as_source()` is `#'` and which
feeds three real re-emission paths -- reusing it would have been latent
corruption in place of the old kind.
No `carp` binary was obtainable (removed from nixpkgs 2026-02-05 as
broken; no ghc/stack/cabal), so the semantics are **grammar-derived from
`src/Parsing.hs`, not oracle-checked** -- 20 hand-verified cases. Said
plainly rather than implied.
Rebased across #96, #98, #100 and #102. The `classify_reader_macro`
dispatch table was resolved arm by arm: ours had `Unknown | Hy | Carp`
with `Lfe` pulled out by #98, theirs had `Unknown | Lfe | Hy` with
`Carp` pulled out -- each side's group line still named the other's
dialect, so taking either would have silently deleted a dialect's reader
support. A pristine `origin/main` control binary was built and diffed
over 36 repo files x 10 dialects plus a 28-case battery: **444
comparisons, 0 differences**. Nothing outside Carp changed.
Three things in `lint-carp-idiom` (#95) needed updating, because the
reader change was authored before that package existed and it documents
these defects as *current behaviour*. One was a real test failure:
`the_correct_corpus_exercises_the_readers_arity_inflation` pinned the
bug with `assert!(bare > 0)`. Inverted rather than deleted -- it now
asserts `bare == 0` *and* `prefixed_lists == 8`, keeping the
corpus-coverage pin it existed for, with the 8 derived from the corpus
text rather than read off the parser.
`edit format` on `@(unsafe-nth world i)` is confirmed fixed by #100 --
that was the original agent's top-ranked follow-up, and #100's
`carries_reader_prefix` fix is dialect-agnostic, so Carp's new variants
were covered automatically.
takeokunn
added a commit
that referenced
this pull request
Aug 3, 2026
`,` is not a reader macro in Hy. Its `@reader_for` table has no `,`
entry, and `NON_IDENT` -- the complete set of identifier terminators --
is `set("()[]{};\"'\`~")`, which lists `~` and not `,`. Confirmed by
running Hy 1.3.1: `(hy.read-many "(foo ,bar)")` gives
`Expression([Symbol('foo'), Symbol(',bar')])`, one symbol.
So classifying `,` as a prefix was wrong at the root, not merely at a
closing delimiter. It stayed invisible while a dangling prefix was
tolerated, because the mis-parse only changed the tree's shape. Once
#100 made a prefix with no following form a hard `MissingReaderForm`,
it started refusing ordinary Hy outright:
- `(,)` -- an expression whose single element is the symbol `,`, which
Hy's tuple constructor uses for the empty tuple.
- A trailing comma before `}` or `]`, as in `{"a" 1 ,}`.
Over 2825 real `.hy` files those two shapes account for **13 outright
parse failures**, including Hy's own `contrib/walk.hy`, `hylang/simalq`
and `kanaka/mal`. #103's comment claiming a leading `,` "occurs at a
token start essentially never" is corrected in place rather than left
standing, since the corpus refutes it.
Because `,` is not a reader macro, the fix is to stop classifying it as
one -- not to special-case "prefix before `)`", which would have undone
#100's fix. `'` and `` ` `` really are Hy reader macros taking exactly
one following form, so a closing delimiter after one is still refused,
pinned by a test.
Dropping the comma is enough on its own: `is_atom_boundary` never
treated `,` as a terminator, so `,bar` and `1,` already scanned as
single atoms and now agree with Hy at token start too.
With Hy as oracle: files paredit refuses that Hy accepts **308 -> 295**,
of which `MissingReaderForm` **17 -> 4**; structural agreement
217/2037 -> 230/2037, **+13 newly agreeing and 0 newly disagreeing**;
25 files newly parse and 0 newly fail. The 4 remaining
`MissingReaderForm` are f-string/raw-string cases, which #103 covers.
Other nine dialects: **24879 files, 0 changed, 0 newly parsing, 0 newly
failing**, by canonical full-tree dump. Pinned by test that `,` still
yields `Unquote` in CL/Elisp/Scheme/Racket/Fennel/LFE/Janet/Carp/Unknown
and is still whitespace in Clojure. The formatter round-trips all 2356
parseable Hy files with 0 non-idempotent and 0 shape-changed.
`~`/`~@` are the matching gap in the other direction and are left: they
cause no refusals, and #103 records that adding them changed the
*meaning* of 14 files through a formatter prefix-drop that #100 has
since fixed. That is now a clean follow-up rather than a blocker.
The dialect test stays where it was -- inside the existing
`match self.dialect` at the top of `classify_reader_macro`, so the
`b','` check is reachable only from the `Dialect::Hy` arm and the other
nine pay nothing. Inverting that ordering is what caused the
`parse-scaling/reader-conditional` regression on #103.
Merge note: git applied this patch as a *second* `classify_hy` rather
than merging its arm into #103's, which would not have compiled. The
two are combined here into one `match (byte, next)` carrying both the
`#[` and `,` arms.
takeokunn
added a commit
that referenced
this pull request
Aug 3, 2026
`docs/LanguageGuide.md:170-173` defines `&` and `@` under a literal
"Reader Macros" heading -- `&x ;; same as (ref x)`, `@x ;; same as
(copy x)` -- but Carp routed through `classify_legacy`, which implements
neither. So `@(f x)` split into a bare `@` atom **plus a sibling**,
inflating the enclosing call's arity in **116 of 248 files (47%)**, 1493
atoms in all. Byte spans survived, so round-trips were lossless; what
broke was structure, which makes any argument-counting analysis unsound
for Carp.
Also fixed: `@"..."` silently split string literals, because `@` glued
to the next token and swallowed the opening quote; character literals
were unrecognized, so `\{ \} \[ \] \( \) \"` were read as real
delimiters; and `#"..."` Pattern literals were unrecognized.
Three more the original brief missed, all real: **`~` (deref) is also a
reader macro** (76 glued atoms in 15 files); **`$[...]` static arrays**
(55 bare `$` atoms, the same arity-inflating defect); and **`,` is
whitespace in Carp, not unquote** (39 sites, where the legacy reader
gave `max` and `val` phantom `Unquote` prefixes in `[min, max, val]`).
`src/Parsing.hs` settles the shape: `readerMacro` is `string macroStr`
then `expr <- sexpr`, recursing into `sexpr` rather than `atom`, so
prefixes bind any form, `@@x` stacks, and `@"..."` is a copy of a
genuine string literal. `validCharacters` excludes `& ~ @ % $ #`, so a
sigil can never be inside a symbol. `aChar` is `\` then one character
with no terminator, so `\{` and `\ ` are valid.
Corpus differential over all 248 files: parse failures **6 -> 0**, 171
trees changed, **1623 child slots removed** and reconciled
token-by-token, **0 files gained atoms**.
`%`/`%@` (Carp's real unquote) is deliberately deferred: it needs two
new `ReaderPrefix` variants threaded through 40+ sites and *adds*
findings inside macro templates, so it wants its own FP audit. Leaving
it keeps templates inert, which is the suppressing direction. `r"..."`
has zero corpus occurrences.
`Ref`/`Copy`/`Deref`/`StaticArray` are new `ReaderPrefix` variants
rather than reuses of `Function`, whose `as_source()` is `#'` and which
feeds three real re-emission paths -- reusing it would have been latent
corruption in place of the old kind.
No `carp` binary was obtainable (removed from nixpkgs 2026-02-05 as
broken; no ghc/stack/cabal), so the semantics are **grammar-derived from
`src/Parsing.hs`, not oracle-checked** -- 20 hand-verified cases. Said
plainly rather than implied.
Rebased across #96, #98, #100 and #102. The `classify_reader_macro`
dispatch table was resolved arm by arm: ours had `Unknown | Hy | Carp`
with `Lfe` pulled out by #98, theirs had `Unknown | Lfe | Hy` with
`Carp` pulled out -- each side's group line still named the other's
dialect, so taking either would have silently deleted a dialect's reader
support. A pristine `origin/main` control binary was built and diffed
over 36 repo files x 10 dialects plus a 28-case battery: **444
comparisons, 0 differences**. Nothing outside Carp changed.
Three things in `lint-carp-idiom` (#95) needed updating, because the
reader change was authored before that package existed and it documents
these defects as *current behaviour*. One was a real test failure:
`the_correct_corpus_exercises_the_readers_arity_inflation` pinned the
bug with `assert!(bare > 0)`. Inverted rather than deleted -- it now
asserts `bare == 0` *and* `prefixed_lists == 8`, keeping the
corpus-coverage pin it existed for, with the 8 derived from the corpus
text rather than read off the parser.
`edit format` on `@(unsafe-nth world i)` is confirmed fixed by #100 --
that was the original agent's top-ranked follow-up, and #100's
`carries_reader_prefix` fix is dialect-agnostic, so Carp's new variants
were covered automatically.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
edit formatsilently dropped reader prefixes at exit 0, with output that reparses — the corrupted-Lisp-still-reparses failure mode. Found independently by three agents doing unrelated reader work.Real damage in the corrupting set:
The stated mechanism was wrong in every particular
BODY_FORMSlives inreindent.rs, whose only consumers areSyntaxTree::reindentand theinspect indentationreport.edit formatnever calls it. The("do", 2)entry can't affect this, and the theory didn't explain the Common Lispletrepro at all — nodois involved.What actually happens:
Formatter::format_node(formatter/core.rs:848-853) writes a node's reader prefixes before dispatching on its head, so every shape renderer may open its own subject's delimiter freely. Six renderers also open a child's delimiter, and nothing wrote that child's prefix:general.rs::format_sequence_list— the binding list oflet/dogeneral.rsalignment branch /alignable_binding_name— one binding entrybindings.rs::format_local_callable_bindings/format_local_callable_bindingclauses.rs::format_clause/format_body_clauseThe corruption is dialect-independent. Had I "fixed" the dialect table as briefed, the CL
letcase would still corrupt.Fixed with a shared
carries_reader_prefixpredicate at those six sites, handing a prefixed child back toformat_node. That's also the correct layout answer:`(a b)in alet's binding slot is a quasiquoted datum, not a binding list. It cannot touch prefix-free code. Opaque reader forms (#+sbcl (…)) needed no entry — the parser gives themNodeKind::Atom, so the existingkind != Listguard already covers them, verified empirically before assuming it.Second bug: a prefix before a closing delimiter
(a ')→(a), in every dialect.form()accumulated prefixes, found a close delimiter, and calledclose_list()without consuming them. It now returnsMissingReaderForm, exactly as a prefix at EOF already did — soformand its scanning twinskip_form, which was already refusing this shape, finally agree.PR #96 pinned this as a stated expectation precisely so a fix would have to update it deliberately rather than silently. That test now asserts the refusal, with an eight-dialect matrix and an
(a 'b)control.Differential: 5353 files, exactly the 27 that were corrupted
The other 5326 files are byte-identical. The 852 non-zero exits are pre-existing parse failures, unchanged — the parser fix refused nothing new.
The oracle, and why this went unnoticed
Tree equality, added as invariant 6 in
tests/corpus.rs(spans excluded — spans are the one thing formatting should change). The negative control is the important part: reverting one guard and re-running over three known-bad files gives…while invariant 3 (idempotence) and the reparse check stay silent. A consistently wrong format is a fixed point.
Worth recording: that oracle cannot see the second bug, because the corruption happens in the parser — input and output parse identically wrong. Only the refusal catches it.
Janet's column-sensitive long strings: deferred, with the reason
Not effort.
stringend(parse.c:355-383) setsindent_col = top.column - 1and strips that many bytes after each newline — but only if every one of them is a space; otherwise reindenting is disabled wholesale. So moving the literal right doesn't shift the value, it flips a mode.Both candidate fixes are real design decisions: freeze every form containing one (29 of 210 corpus files, and in docstring-heavy Janet that's most
defns — also only sound when the form starts at column 0), or teach the strictly top-down column model that an atom can pin its column. Rewriting the literal's interior to compensate is a third option worth rejecting outright: it makes the formatter edit string contents.Scope measured with Janet itself as oracle (
parse-all+deep=): 24 of 210 files changed meaning before, 23 after — one was a bug-1 prefix drop. Of the 23, 22 are pure long-string cases; the 23rd is the separate bare-@reader divergence already documented in #96.Verification
cargo build --workspace,cargo test --workspace,cargo test --test cli(3085 passed),cargo fmt --all --check,cargo clippy --all-targets --all-features -- -D warnings— all exit 0, re-run after rebasing onto merged #96.treefmt formats 6 tracked files (4
lint-scheme-idiomfixtures + 2migrate/recipes/*.lisp); all 6 are byte-identical under both binaries, so no recipe regeneration is needed andtreefmt-pr-checkpasses. No golden or pinned count moved.New fixtures include a case for the non-compact path — it preserves prefixes for a different reason than
compact_node, so a partial fix would have missed it — plus prefix-on-last-element,('), nested, and two CLI--writetests through the real binary.