fix(syntax): stop reading Hy's comma as an unquote - #106
Merged
Conversation
`,` 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
…alect pin (#108) `main` was broken by a semantic conflict between two PRs that were each green alone and merged cleanly. #106 added `hy_comma_arm_does_not_change_other_dialects`, asserting `,` still yields `ReaderPrefix::Unquote` in every dialect but Clojure -- including Carp, which at the time inherited the legacy reader where that was true. #104 then split Carp out and made `,` **whitespace**, which is what Carp actually does: `[min, max, val]` and `[x Int, y Int]` are separator syntax, and reading the commas as unquote gave `max` and `val` phantom prefixes at 39 corpus sites. Neither PR could have caught this. git saw no textual conflict, both CI runs were green, and the collision only exists once both are on the same tree. Carp moves from the unquote loop into the whitespace loop beside Clojure, and the test's doc comment now records why -- it previously said the split-out arm "still serves `Unknown` and `Carp`", which #104 made false.
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.
A live regression on
main. PR #100 correctly made a reader prefix before a closing delimiter a hardMissingReaderForm— but,was being classified as a prefix for Hy, and it shouldn't have been. So ordinary Hy stopped parsing.,is not a reader macro in HyHy's
@reader_fortable has no,entry, andNON_IDENT— the complete set of identifier terminators — isset("()[]{};\"'"), which listsand **not**,`. Confirmed by running Hy 1.3.1: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 landed it started refusing:(,)— an expression whose single element is the symbol,, which Hy's tuple constructor uses for the empty tuple.}or], as in{"a" 1 ,}— idiomatic in Hy's Python-flavoured literals.Over 2825 real
.hyfiles those account for 13 outright parse failures, including Hy's owncontrib/walk.hy,hylang/simalqandkanaka/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.The fix follows from the answer, and doesn't undo #100
Because
,isn't a reader macro, the fix is to stop classifying it as one — not to special-case "prefix before)", which would have reverted #100's correction of silent corruption in every dialect.'and`are real Hy reader macros taking exactly one following form, so a closing delimiter after one is still refused, pinned by a test.Dropping the comma suffices on its own:
is_atom_boundarynever treated,as a terminator, so,barand1,already scanned as single atoms and now agree with Hy at token start too.With Hy as oracle
MissingReaderFormThe 4 remaining
MissingReaderFormare f-string/raw-string cases — #103's scope, already merged.Other dialects untouched
24,879 files across the other nine: 0 changed, 0 newly parsing, 0 newly failing, by canonical full-tree dump. Pinned by test that
,still yieldsUnquotein CL/Elisp/Scheme/Racket/Fennel/LFE/Janet/Carp/Unknown, and is still whitespace in Clojure. The formatter round-trips all 2356 parseable Hy files: 0 non-idempotent, 0 shape-changed, and the comma shapes come back byte-identical.~/~@left, and it's now a clean follow-upThey're the matching gap in the other direction — real Hy reader macros read here as plain atom characters. They cause no refusals, and #103 recorded that adding them changed the meaning of 14 files via a formatter prefix-drop. #100 has since fixed that family, so this is now unblocked rather than risky.
Cost
The dialect test stays inside the existing
match self.dialectat the top ofclassify_reader_macro, so theb','check is reachable only from theDialect::Hyarm and the other nine pay nothing. Inverting that ordering is exactly what caused theparse-scaling/reader-conditionalregression on #103.Merge note
git apply --3wayapplied this as a secondclassify_hyrather than merging its arm into #103's — which would not have compiled. The two are combined here into onematch (byte, next)carrying both the#[and,arms, and verified end-to-end: all four comma shapes parse,(a ')still refuses, and #103's f-strings and bracket strings still parse.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. No golden or pinned count moved.