fix(lint): suppress the eq comparison rules inside a hard quote - #119
Merged
Conversation
`eql-string-comparison` ships at `Severity::Error` and had no quote
context guard at all, so it reported quoted *data* as if it were a
comparison. Over 5388 unique Common Lisp files, every one of its findings
was a false positive:
mgl-pax `hyperspec.lisp:367-369,1052` a HyperSpec index whose rows are
three-element lists inside a `'(`, so `(eq function "f_eq")` is a data
row and not a call
sbcl-cl-cc-vm `vm-bridge-io-docs.lisp:345` `(eql . "RT-EQL")`, an
opcode-to-runtime alist pair
The obvious repair would have been worse than the defect. Suppressing
every quote also suppresses quasiquote, and in SBCL's own sources most
quasiquoted hits are macro templates that really do become code --
`` `(eq ,val 0) `` at `hashset.lisp:110`, and the same shape in
`srctran.lisp` and `ir1tran-lambda.lisp:691`. So the guard reads only the
`hard` half of the two-counter `QuoteState`, which is exactly why that
model has two counters rather than one depth. A hard-quoted form is never
evaluated, so suppressing it cannot produce a false negative.
The guard lives in the package's existing `support.rs`, which already had
`QuoteState` and `is_unevaluated_at` for four float rules; the descent is
now shared as `quote_state_at` with `is_hard_quoted_at` reading `.hard`
alone. No new quote machinery.
All three `eq` rules get it. The guard answers "is this node code at
all", which is a precondition for any of them meaning anything, and
fixing the latent case in the two siblings costs nothing.
A second guard drops a one-argument `eq`. `(eq #\a)` in trivia's pattern
DSL (`test/level2.lisp:909`) is unquoted, so the quote guard does not
reach it, and SBCL already says "The function EQ is called with one
argument, but wants exactly two" -- a one-argument form is compiler-caught
and is not a comparison. trivia's patterns are one-argument by
construction, so this removes the whole DSL class. `single-arg-comparison`
does not cover it; that rule is scoped to `< > <= >= = /=`.
Findings over the corpus go 166 to 156: ten removed, none added. The
count deliberately does not fall to zero -- `eq-number-comparison` holds
at 144, including the two genuine float bugs at `cross-float.lisp:28`,
`(eq flonum -0.0f0)` and `(eq flonum -0.0d0)`, which are the case where
`eq` really does return the wrong answer cross-compilation-unit.
Mutation-tested with each mutation checked by `diff` before running.
Replacing the guard with `is_data()` -- the obvious wrong fix -- fails
exactly the three quasiquote tests, so the suite blocks that regression
rather than merely tolerating the correct version.
Cost is unchanged on ordinary code: 20.0 ns/invocation at both file
sizes, ratio 1.00, because the guard sits after the head-and-argument
check and `root_view()` is never reached. On a pathological all-quoted
table it is 1.89, against 1.92 for the shipped
`mixed-float-precision-arithmetic` on the identical input -- a
pre-existing property of `is_unevaluated_at` rebuilding `root_view()` per
finding, shared by four shipped rules, not a regression here.
takeokunn
added a commit
that referenced
this pull request
Aug 4, 2026
…otes (#120) A survey of all 358 rules found 274 scoped to Common Lisp, 181 of those with no quote-context guard, and 67 of those misfiring on hard-quoted data over a 5,556-file corpus. These are the two worst. `equality-arity` reported 674 false positives, 34% of its output, at `Severity::Error`. It already had a guard, but `domain.rs:121` reads the node's *own* `reader_prefixes` and never the ancestor chain -- which is exactly why its self-quoted count is 0 and its ancestor-quoted count is 674. The findings are quoted CLHS type specifiers such as `(typep spec '(cons (eql or) ...))`, where a one-argument `eql` is correct Common Lisp, throughout SBCL's `checkgen.lisp`, `interr.lisp` and `sexpr.lisp`; mgl-pax's quoted HyperSpec index, the same table that produced the bug PR #119 fixed; and SBCL's quoted pprint dispatch table. `one-step-arithmetic` reported 163, and it is `Fixable`, which makes it the more dangerous of the two: the autofix rewrites source. It would turn `:cases (("1+" '(1+ n) '(+ n 1)))` -- the expected value of a test asserting what `1+` expands to -- into `'(1+ n)`, making the assertion tautological. Both guards read only the `hard` half of `QuoteState`, following PR #119, and sit inside the per-item loop so `root_view()` is unreachable unless a finding already exists. Three rules were deliberately left alone. `implementation-package-symbol` has 307 ancestor-quoted findings that are all correct: it fires on `(import '(sb-sys:sap-ref-16 ...))`, and the symbol really is in an implementation package whether or not the list is quoted, so a guard would buy a false negative. `one-armed-if` and `explicit-nil-return` were left alone for a subtler reason, and it corrects the premise this work started from. A hard quote does *not* always mean inert: `#.` read-eval resurrects quoted code, as in SBCL's `early-extensions.lisp`, where a `(progn (defun ...))` sits hard-quoted inside `#.(if *profile-hash-cache* '(progn ...))` and is spliced back as code at read time. `deftransform` templates do the same by `subst`ing a quoted body into compiled output. Measured, 95 of 1823 hard-ancestor findings (5.2%) sit under `#.`, concentrated in those two rules -- 9 of 16 and 5 of 23 -- so guarding them would suppress real findings. The two rules fixed here are clean by that measure: `one-step-arithmetic` has 0 under `#.`, and all 6 of `equality-arity`'s are type specifiers regardless of the enclosing `#.`. Findings go 117,299 to 116,407 over the corpus: 892 removed, 0 added, no other rule changed. Every removed span was re-probed individually and confirmed hard-quoted. The two rules retain 238 quasiquote-template findings and emit zero hard-quoted ones afterwards. Reported and not fixed: about 970 of `equality-arity`'s remaining 1,307 findings are one-argument `(eql X)` in *unquoted* type contexts -- EQL specializers in `defmethod` lambda lists, and `typecase`/`deftransform` clause heads. That rule is likely more than 80% false overall, but fixing it needs a type-context model rather than a quote guard.
This was referenced Aug 4, 2026
takeokunn
added a commit
that referenced
this pull request
Aug 4, 2026
) `elisp-quoted-lambda` ships at `Severity::Error` with a destructive autofix, and over 1751 GNU Emacs 31.0.91 and package files every one of its fifteen findings was false. Its predicate was head-only -- a list, carrying a `Quote` prefix, whose first child is `lambda` -- which matches a *symbol list* just as readily as a quoted function. Demonstrated with the shipped binary against `byte-opt.el`: (memq head '(lambda internal-make-closure length cons)) -- after --fix --> (memq head (lambda internal-make-closure length cons)) A membership test rewritten into a call, automatically, in GNU Emacs's own source. The same shape appears in `bind-key.el`, `cus-start.el`, `elint.el` and `calc-map.el`. Two further findings were `',(lambda ...)`, `menu-bar.el`'s idiom where the unquote evaluates the lambda so the quote applies to the resulting closure, and two were `''(...)` or `'#'(...)`. It now requires a lambda list -- `(...)` or `nil` -- in the second position, and requires the prefix to be exactly `[Quote]`. Fifteen findings become six. `elisp-obsolete-cl-alias` ships at `Severity::Error` with no context check at all: 150 findings, and 25 of 25 sampled at random were false. They are `(dolist (block blocks) ...)` and `(let (ll (do t)) ...)` binding pairs, `(defun mail-comma-list-regexp (labels) ...)` lambda lists, `(mapcar (lambda (case) ...))`, quoted data such as `(memq word '(do doing))` and `(doctor-type '(do you know Stallman \?))`, and `cl-indent.el`'s own indent-spec table. None was a call to a removed macro, which is what you would expect: Emacs 31 would not compile if it were. It now requires at least two arguments, requires the first argument to match each macro's real lambda list -- a list for `do` and `flet`, a symbol for `block` -- skips a node carrying its own quote, and finally asks `binding_table().resolve()` whether the head is a local binding. That last check removed 21 of the remaining 35 on its own: contrary to what the other dialects suggest, Emacs Lisp *does* have a modelled binding table, in `semantics/binding/service/emacs_lisp.rs`, which knows `named-let`. 150 findings become 14. Of those 14, one is a genuine unprefixed `(case command ...)` in a chibi-scheme company backend that was previously buried in noise, one is `cl.el`'s own shim, and twelve are a single remaining class: a quote or quasiquote on an *ancestor* rather than the node. `is_unevaluated_at` in `lint-form-shape/src/support.rs` solves exactly that, and PRs #119 and #120 established the pattern, but adopting it here means copying the two-counter model into a fifth package. That is a separate decision and is left alone. Mutation testing earned its place twice over. The second harness first reported five survivors, because the new arity guard masked every other guard in the test cases I had written; rebuilding those controls around the real GNU Emacs shapes took it to 7 of 7 killed. Without it this would have shipped four guards with no coverage. Reported and not fixed: `leftover-print-debug` is `Fixable` with 6560 findings and treats Scheme's `display` and Janet's `print` as debug leftovers, when they are those languages' primary output primitives -- `--fix` deletes them. And `elisp-defcustom-missing-group` fires on 3668 of 8476 `defcustom` forms; it is correct, but it relies on documented file-level `defgroup` inheritance and belongs in `RuleTag::Pedantic`.
takeokunn
added a commit
that referenced
this pull request
Aug 4, 2026
Four PRs took this rule from 1981 findings to 407 by closing three structural false-positive classes -- hard-quoted data (#119, #120), CLHS type positions (#121), and case keys and bound variables (#123). The 407 that remain are, on every sample drawn from them, still false: 120 of 120 in one adjudication and 14 of 14 in another. They cannot be closed the same way, and the investigation into a general mechanism is what settles the severity question. Suppressing findings inside any head the engine has never seen defined would silence 32,886 of 113,979 findings across 186 of the 214 rules that fire -- 28.9% of the catalogue's output. An adjudicated sample of thirty of the collateral was 23 genuine, 5 false, 2 ambiguous, which scales to roughly 25,000 real findings destroyed to remove 405 false ones, about 62 real per false. The premise behind that design is also simply untrue: `when`, `dolist`, `deftest`, `describe`, `macrolet` and `named-let` are all macros whose body arguments *are* evaluated, so knowing a head is a macro says nothing about which of its positions hold data. `OpacityCauseKind::UnknownHead`'s own documentation reaches the same conclusion -- "treating `(print x)` as opaque too would be sound as well, and would prove nothing about any real file" -- and Common Lisp has no registry of ordinary function heads to distinguish them. A configured list of unevaluated heads fares no better. `RuleSettings` carries `i64` values by explicit design, so the list needs `packages/core/lint-engine` and its config key needs `config_bridge`; a *name* list is the wrong granularity anyway, since every `deftransform` finding is at child index 2, its lambda list, while its body at index 3 and beyond is ordinary code. Even a perfect `(head, position)` table closes 312 of 407, leaving a build-blocking rule that still blocks builds on correct programs. Fourteen rules share this false-positive class, 624 findings, seven of them at `Severity::Error` for 459 build-blocking false positives, so this is not a problem peculiar to one rule. It is worth fixing properly later. Until then the honest severity for a rule measured at 407 findings and no true positives over 5556 files is `Warning`, not `Error`. Worth recording for whoever picks this up: the 407 live in three projects -- SBCL's compiler (561 of the 624), `trivia` (62) and `mgl-pax` (1) -- and the corpus holds five SBCL releases that content-hash dedup cannot collapse, so those 407 findings occupy 88 paths but only 36 canonical ones. The SBCL bucket is one codebase counted five times. `warning_count` 255 to 256 and the preset-filtered count 239 to 240; the rule is untagged, so both move together. `RULE_COUNT` and `fixable_count` are unchanged. The golden diff is seven lines across six files, all of them the severity token.
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
1 participant
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.
eql-string-comparisonships atSeverity::Errorand had no quote-context guard at all, so it reported quoted data as if it were a comparison.Over 5,388 unique Common Lisp files / 87 MB (SBCL 2.6.0/2.6.1/2.6.6 + Quicklisp + nix-store, deduplicated by SHA-256), every one of its findings was a false positive:
hyperspec.lisp:367(eq function "f_eq")'(at :7hyperspec.lisp:368(eql function "f_eql")'(at :7hyperspec.lisp:369(eql type "t_eql")'(at :7hyperspec.lisp:1052(eql "a_eql")'(at :1040vm-bridge-io-docs.lisp:345(eql . "RT-EQL")'(at :330mgl-pax's HyperSpec index stores three-element lists inside a
'(; the sbcl-cl-cc-vm hit is an opcode-to-runtime alist pair. Each quote opener was read, not inferred.The obvious repair would be worse than the defect
Suppressing every quote also suppresses quasiquote — and in SBCL's own sources most quasiquoted hits are macro templates that genuinely become code:
`(eq ,val 0)athashset.lisp:110, and the same shape insrctran.lispandir1tran-lambda.lisp:691.So the guard reads only the
hardhalf of the two-counterQuoteState. That is precisely why the model has two counters (hard: bool+quasi: u32) rather than one depth. A hard-quoted form is never evaluated, so suppressing it cannot produce a false negative.The guard lives in the package's existing
support.rs, which already carriedQuoteStateandis_unevaluated_atfor four float rules; the descent is now shared asquote_state_at, withis_hard_quoted_atreading.hardalone. No new quote machinery, noi32counter.All three
eqrules get it — the guard answers "is this node code at all", a precondition for any of them meaning anything.A second guard: one-argument
eq(eq #\a)in trivia's pattern DSL (test/level2.lisp:909) is unquoted, so the quote guard doesn't reach it. But SBCL already says "The function EQ is called with one argument, but wants exactly two" — a one-argument form is compiler-caught and isn't a comparison. trivia's patterns are one-argument by construction, so this removes the whole DSL class.single-arg-comparisondoes not cover it; verified via--explainthat it is scoped to< > <= >= = /=only.Differential: 166 → 156, ten removed, none added
The count deliberately does not fall to zero.
eq-number-comparisonholds at 144, including the two genuine float bugs atcross-float.lisp:28—(eq flonum -0.0f0)and(eq flonum -0.0d0), inside a live(defun flonum-minus-zero-p ...). Those are the case whereeqreally does return the wrong answer cross-compilation-unit. All 12 survivingeq-charfindings are real code (cffi functions.lisp:294, cl-ppcrecharset.lisp×9, SBCLshebang.lisp:181×2).Negative controls, end-to-end through the binary
12 MUST-FIRE all fire, including quasiquote templates
`(eq ,val 0),`(eq ,name "done"),`(eq ,ch #\a), a quasiquote without an unquote, and unquoted code inside adefmacrobody.Every MUST-BE-SILENT is silent: hard-quoted tables, dotted alist pairs,
(quote ...),'(a ,(eq ...))(comma inside a hard quote), one-argument patterns, and the defect spelled inside a string.Mutation testing
Each mutation verified to land via
diffbefore running.is_data()— the obvious wrong fixThe second is the important one: the suite actively blocks the quasiquote-suppressing regression rather than merely tolerating the correct version. All restored byte-identically; 410/410 green after.
Cost
Ordinary file, zero findings, 200→400 defuns: 20.0 ns/inv at both sizes, ratio 1.00 — indistinguishable from untouched
self-comparison(30.0) andredundant-quote(17.4/18.2). The guard sits after the head-and-argument check, soroot_view()is never reached.On a pathological all-quoted table: 1.89 — against 1.92 for the shipped
mixed-float-precision-arithmeticon identical input, within 2.4%. That super-linear cost is a pre-existing property ofis_unevaluated_atrebuildingroot_view()per finding, shared by four shipped rules. Not a regression here; worth flagging separately.Scope
8 files, all under
packages/feature/lint-numeric/src/. No rule added, no golden moved, no pinned count moved — so no repo fixture contained a false positive.cargo build --workspace/fmt --check/clippy --all-targets --all-features -D warnings/test --workspace/test --test cli(3085 passed) all exit 0.