fix(lint): stop the progn fixes deleting a reader prefix - #122
Merged
Conversation
`paredit fix --write` was corrupting source. Over 5556 unique Common Lisp
files, of the 2606 files a fix run changed, **206 that SBCL could read
before no longer read at all** -- and 116 of those were
`redundant-body-progn`, 98 `redundant-progn`.
The cause is a span, not a missing guard. `RuleFix::single(view.span, …)`
replaces a region that *starts at the form's own reader prefix*, while
the replacement text is built from the inner form, so the prefix is
deleted:
- `(progn
- (defun ,fn-name (,var ,remaining) ,@Body)
+ (defun ,fn-name (,var ,remaining) ,@Body)
which SBCL then rejects with `Comma not inside a backquote`. The silent
variant is worse: `` `(list* ,@required (sb-c::%rest-list .rest.)) ``
became `(cons ,@required (sb-c::%rest-list .rest.))`, which reads fine
and means something else.
`redundant-body-progn` also had a false premise. `` `(progn …) `` is the
standard shape of a multi-form macro expansion, not a redundant `progn`
in an implicit-progn body: 197 of its 204 classifiable firings (96.6%)
were quasiquoted templates, and its combined data-plus-template
false-positive rate was 98.0%.
Four changes, all in `lint-control-flow`:
- the fix region is now `view.content_span`, so a prefix survives --
`` `(progn ,x) `` rewrites to `` `,x `` rather than `,x`. The two spans
coincide on unprefixed code.
- a hard-quote guard, reading only the `hard` half of the existing
two-counter `QuoteState`. Quasiquote is deliberately not suppressed,
because a template really does become code. It reuses
`with_lexical_chain` -- a binary search and one top-level descent,
never `root_view()` -- and runs only once the walk already has a
finding.
- `(progn ,@Forms)` as the sole body form is exempt. It wraps an
expansion-dependent number of forms, and `` `,@Forms `` is not a
well-formed backquote expression at all. This mirrors the existing
reader-conditional exemption.
- `redundant-body-progn`'s `is_progn` now requires no reader prefix: a
prefixed child is a datum in that slot rather than a body form, and a
multi-form splice under a prefix has no valid rewrite.
Files SBCL could read before but not after go 206 to 16 on the same
corpus, and prefix-dropping fixes 2086 to 808. Fixes landing on
genuinely evaluated code stay at 2826 -- not one fix on real, unquoted
code was lost. `redundant-progn` fires 259 times instead of 259+... 41,
with its evaluated-code bucket unchanged at 28; `redundant-body-progn`
goes 204 to 5 with its bucket unchanged at 4.
Quote context was adjudicated by an oracle built from SBCL's own reader,
a custom readtable recording the byte ranges of `'`, `` ` `` and `,`,
validated against this parser over 1194 regions with no prefix-character
mismatches.
Reported and not fixed, to keep this change reviewable: 38 further rules
drop reader prefixes across 808 fixes -- worst by rate `redundant-prog1`
89.5%, `redundant-the` 80%, `if-not` 60%, `empty-let` 46.7%,
`constant-if-test` 38.8% -- and the remedy is the same `content_span`
change applied here. 91 `Fixable` rules still have no hard-quote guard.
The 16 residual read-breaks belong to `single-operand-boolean`,
`list-star-to-cons`, `negated-if`, `cons-to-list`, `append-nil`,
`if-to-unless` and `empty-let`.
takeokunn
added a commit
that referenced
this pull request
Aug 4, 2026
PR #122 fixed this in `redundant-progn` and `redundant-body-progn`. The same defect is in 38 further rules: `RuleFix::single(view.span, ...)` replaces a region that starts at the form's own reader prefix while the replacement text is built from the inner form, so a leading `'`, `` ` ``, `,`, `,@` or `#'` is deleted. The remedy is `view.content_span`, whose extent coincides with `span` on unprefixed code. A census over 5699 unique Common Lisp files, SHA-256 deduped from 16,115, found 42 rules dropping a prefix across 2650 of 17,751 replacements. Two are #122's and two -- `sharp-quoted-lambda` and `redundant-quote` -- exist to delete a prefix, so `content_span` would defeat them; they are excluded. Worst rates among the rest: `multiple-value-list-of-values` 100%, `redundant-prog1` 89.5%, `redundant-the` 80%, `if-not` 65.8%, `single-operand-boolean` 42.7%, `subseq-zero` 41.7%, `constant-if-test` 41.3%. Files SBCL could read before but not after go 29 to 2 with the two a strict subset of the 29, so nothing regressed, and prefix-dropping fixes among these rules go 706 to 0. Three rules did not take the plain swap. `empty-let` builds its fix as a sub-span inside its domain; `nested-progn` operates on a *child* node and took #122's `redundant_body_progn` treatment, a prefix requirement on the child; `constant-when-test` has two fix sites. `append-nil` showed zero drops in this corpus but carries the identical span and is named in the residual read-break list, so it is changed and the lack of corpus evidence is recorded here. The anti-over-suppression evidence is keyed on the *finding* span rather than the replacement offset, because the span change moves every replacement by construction and counting by it would report the migration as a behaviour change. On that measure the span change alone leaves fixes on unprefixed ordinary code at 14,080 before and after -- not one lost -- and the splice guard removes exactly 243 fixes, all of them `,@`, with no non-splice removed, no splice surviving, none added, and 1799 legitimate fixes retained. The oracle is SBCL's own reader with a custom readtable recording the byte offsets of each prefix, cross-validated against this parser over 80,321 regions in 4581 files with zero positional disagreements. Adding package-error recovery lifted readable coverage from 40% to 80%. Six apparent mismatches are all `,.`, where the positions agree but the parser labels unquote-dot as `Unquote` rather than `UnquoteSplicing` -- real, and separate. The two remaining read-breaks are a different bug and are reported rather than fixed: dotted-pair notation read as a call. `(append . nil)` becomes `(copy-list .)` and `(cons . (list sequence t))` becomes `(list . sequence t)`, because the parser exposes `.` as an ordinary child atom, so any rule counting operands is wrong there. It also reaches `redundant-body-progn`. Fixing two of an unknown number across 106 fixable rules would sprawl this change; it deserves its own audit. Mutation: 12 mutations, all killed, including widening the splice guard to any reader prefix and widening `nested-progn`'s guard to the enclosing form -- the two over-suppression mutations, both caught by the quasiquote-template controls. One mutation was initially invalid because `cargo fmt` had reflowed its anchor, which is why compile failure is matched on `error[E` and reported as invalid rather than as a kill.
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.
paredit fix --writewas corrupting source. Over 5,556 unique Common Lisp files (12,477 raw, deduplicated by SHA-256, 88.1 MB), of the 2,606 files a fix run changed, 206 that SBCL could read before no longer read at all — 116 fromredundant-body-progn, 98 fromredundant-progn.The cause is a span, not a missing guard
The replacement text is built from the inner form, so the prefix is deleted. Actual before/after,
loop-parser-for.lisp:78:→ SBCL:
READ-ERROR: Comma not inside a backquote.The silent variant is worse.
combin.lisp:283:`(list* ,@required (sb-c::%rest-list .rest.))→(cons ,@required (sb-c::%rest-list .rest.))— reads fine, means something else.This is systemic: 2,086 of 17,400 fixes (12.0%) across 40 rules drop a
',`,,,,@or#'.redundant-body-progn's premise was false`(progn …)is the standard shape of a multi-form macro expansion, not a redundantprognin an implicit-progn body. 197 of 204 classifiable firings (96.6%) were quasiquoted templates; combined data+template false-positive rate 98.0%.Four changes, all in
lint-control-flowview.content_span, so a prefix survives —`(progn ,x)→`,x, not,x. The two spans coincide on unprefixed code.hardhalf of the two-counterQuoteState. Quasiquote is deliberately not suppressed — a template really does become code. Reuseswith_lexical_chain(binary search + one top-level descent, neverroot_view()) and runs only once a finding exists.(progn ,@forms)as sole body form is exempt — it wraps an expansion-dependent number of forms, and`,@formsis not a well-formed backquote expression at all. Mirrors the existing reader-conditional exemption.is_prognnow requires no reader prefix — a prefixed child is a datum in that slot, not a body form.Measured effect, same corpus and method
That last row is the anti-over-suppression evidence: not one fix on real, unquoted code was lost.
redundant-progn259 → 41 firings with its code bucket unchanged at 28;redundant-body-progn204 → 5 with its bucket unchanged at 4.The oracle
Quote context was adjudicated by an oracle built from SBCL's own reader — a custom readtable recording byte ranges of
'/`/,— validated against this repo's parser over 1,194 regions on a 120-file sample: 0 prefix-char mismatches, 0 rejected byinspect check.Controls and mutation
12 new engine-level tests assert the rewritten source, not just the finding.
Positive: still fires on unquoted code; inside a quasiquote template (
`(when c (progn ,a ,b))→`(when c ,a ,b)); in adefmacrobody; on every implicit-progn head; on(progn)→nil.Negative: hard quote, long-hand
(quote …), deep nesting, comma-inside-hard-quote, plus the symmetric,(…)-inside-quasiquote case proving the guard is onhardrather than "has a data ancestor".8 mutations, all killed, each verified by
git diff --no-ext-diff --statto have actually changed the file — including M1/M2, theis_data()mutation (the obvious wrong fix), killed by the quasiquote-template controls, and M8, the over-suppression mutation widening the splice test to any prefix.Reported, not fixed
Kept out to stay reviewable:
redundant-prog189.5%,redundant-the80%,if-not60%,empty-let46.7%,constant-if-test38.8%,subseq-zero38.5%,nth-constant-index31.8%. The remedy is the samecontent_spanchange applied here.Fixablerules have no hard-quote guard — onlylint-repl-debug's 7 carry one. A 25-line adversarial file of a quoted HyperSpec-style table took 21 applied fixes, rewriting every row.single-operand-boolean(5),list-star-to-cons(4),negated-if(3),cons-to-list,append-nil,if-to-unless,empty-let.Caveat on the hard-quote figures: only 3,570 of 17,400 fixes could be quote-classified, because the sandboxed reader (
*read-eval*nil, fresh package) rejects files using#., custom readtables, or undefined packages. Those numbers are a ~20% sample and likely an under-count. The prefix-drop and read-break figures cover the whole corpus.Gates
cargo build -p/cargo test -p paredit-feature-lint-control-flow(364 passed) /clippy --all-targets -- -D warnings/fmt --check/cargo test --test cli(3,085 passed) — all exit 0, each read unpiped.No golden and no pinned count moved;
fix liststill reportsrule_count: 106. So no repo fixture contained one of these false positives.