fix(lint): suppress equality-arity and one-step-arithmetic in hard quotes - #120
Merged
Conversation
…otes 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.
takeokunn
added a commit
that referenced
this pull request
Aug 4, 2026
CLHS 4.2.3 makes `(eql object)` a compound *type specifier*, so in a type position one argument is not a defect but the only legal spelling. `equality-arity` reported it as a bad-arity call anyway, at `Severity::Error`. A seeded sample of 120 of the 1307 findings PR #120 left behind, each read with its source context, contained **zero true positives**. That is not surprising once stated: the corpus is shipped, compiling code, and a genuine one-argument `(eql x)` in call position is a compile-time error. Unlike the quote guard, this cannot be a local predicate. The same text is a type in one place and a call in another -- `(eql 7)` as a `defmethod` parameter versus as a `progn` body form -- so the rule has to know the node's *role*, which lives in its ancestor. `type_table()` does not answer that and could not be made to cheaply. It exposes `expression_type` and `binding_type`, both of which ask what type an expression *has*; `(eql 7)` in a specializer has no type, it *is* one. `infer_view` also returns `Type::Unknown` for anything opaque to evaluation, so a specializer is never visited. `lint-type-declaration` has no type-position finder either -- its `type_excludes` classifies a spec it has already been handed. So this is a structural ancestor walk, mirroring the existing `quote_state_at` root descent in the same file. The contexts were found by measurement rather than from CLHS: a probe crate depending on the repo's own syntax and lint-engine ran the real dispatch with only this rule enabled, joined each finding to its ancestor chain by byte span, and bucketed the nearest non-combinator ancestor. That distribution produced the set -- `typecase`/`etypecase`/`ctypecase` clause heads, `defmethod` and `defgeneric :method` specializers, `declare`/`declaim`/`proclaim` type and ftype specs, `the`, `check-type`, and slot `:type` in `defclass`/`defstruct`. Two shapes that look like contexts are traps and stay reported. `typep`, `subtypep` and `coerce` are *functions*: their type argument is evaluated, so `(typep x (eql 5))` really is a one-argument call, and only the quoted `'(eql 5)` spelling is a specifier -- which is PR #120's job. Anchoring them dropped this change's overlap with #120 from 540 findings to 3. `satisfies` and `member` are not descent paths either, since their arguments are a predicate name and objects rather than nested specifiers. Mutation testing found a defect in the draft. `(defmethod g (a &key (k (eql y))))` has the identical `(name form)` shape as a specializer, but that second element is a *default value form* -- live code, and a real one-argument call. Only required parameters may be specialized, so the walk now requires no `&`-keyword before the parameter. Corpus counts were unchanged, making it pure false-negative prevention. SBCL's own type contexts -- `defknown`, `deftransform`, `define-vop`, `specifier-type` and friends -- are deliberately excluded. Teaching a general Common Lisp linter one implementation's compiler macros would silence any user macro that happens to share a name. That leaves 384 findings unfixed and reported rather than hidden. 679 findings removed, 0 added, all of them `eql` at exactly one argument; no other operator or arity moved. Three overlap PR #120, so the net effect on its remaining 1307 is 676, a little over half. Cost on the 5097 zero-finding files is 0.767s against 0.756s, which is noise: the walk sits behind `argument_count == 1 && operator == "eql"`, and both require a finding to exist, so clean code never reaches `root_view()`. Still unfixed and worth separate changes: 136 `case`/`ecase` clause keys, where `(case kind (eql <body>))` is a key designator and never a call, and 33 `multiple-value-bind` variable lists.
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.
Follows #119. A survey of the whole catalogue found the same defect class is widespread; these are the two worst instances.
Survey
358 rules → 274 Common Lisp-only → 181 with no quote guard → 67 misfiring, over a corpus of 5,556 unique files / 92.4 MB (SBCL 2.6.0–2.6.6 + Quicklisp + 83 nix-store
sbcl-*, deduplicated by SHA-256 from 12,477 raw / 207.8 MB). 5,506 linted at exit 0, 50 unparsable, accounting asserted to sum exactly. 117,299 findings.#.What is fixed
equality-arityalready had a guard — butdomain.rs:121reads the node's ownreader_prefixes, never the ancestor chain. That is exactly why its self-quoted count is 0 and its ancestor-quoted count is 674. The findings are quoted CLHS type specifiers like(typep spec '(cons (eql or) …)), where one-argumenteqlis correct Common Lisp — throughout SBCL'scheckgen.lisp,interr.lisp,sexpr.lisp— plus mgl-pax's quoted HyperSpec index (the same table that produced #119's bug) and SBCL's quoted pprint dispatch table.one-step-arithmeticisFixable, which makes it the more dangerous. Its autofix would rewriteinto
'(1+ n)— making the assertion tautological.The premise this work started from was wrong
I briefed both audits with "a hard-quoted form is never evaluated, so suppressing it cannot produce a false negative."
#.read-eval resurrects quoted code. SBCL'searly-extensions.lisp:Hard-quoted, spliced back as code at read time.
deftransformtemplates do the same bysubsting a quoted body into compiled output.Measured: 95 of 1,823 hard-ancestor findings (5.2%) sit under
#., concentrated inone-armed-if(9/16) andexplicit-nil-return(5/23). Those two are therefore not guarded here — it would suppress real findings. The two fixed are clean by that measure:one-step-arithmetichas 0 under#., and all 6 ofequality-arity's are type specifiers regardless of the enclosing#..Also deliberately untouched:
implementation-package-symbol, whose 307 ancestor-quoted findings are all correct — it fires on(import '(sb-sys:sap-ref-16 …)), and the symbol is in an implementation package whether or not the list is quoted.Differential
0 added, no other rule changed. Every removed span re-probed individually: 837
hard_ancestor+ 55hard_self. Post-fix the two rules emitcode1,628 /quasi_ancestor225 /quasi_self13 and zero hard — 238 quasiquote-template findings correctly retained.By origin: equality-arity — SBCL 2.6.6 ×201, 2.6.0 ×148, 2.6.3 ×132, 2.6.4 ×80, 2.6.1 ×70, fare-quasiquote ×12, mgl-pax ×12, trivia ×7, clweb ×5. one-step-arithmetic — cl-cc ×81, cl-weave ×56, cl-cc-expand ×33, iterate ×8, alexandria ×8.
Method notes
The audit refused to trust an ad-hoc scanner — a previous one invalidated a whole batch by splitting
#\U+4E00in two. Instead it built a probe crate path-depending on the repo's ownparedit-core-syntax, replicatingis_unevaluated_at's descent, validated 12/12 against the quote shapes the repo pins. Findings join on byte spans, not line/col.It also caught its own measurement bug: batching by directory, 47 of 256 batches aborted wholesale because one unparsable file kills a batch. Only the asserted exit code caught it; it re-ran per-file.
And it separated
hard_selffromhard_ancestor—redundant-quote's 115 "hard" findings are the rule working correctly, since its subject is the quote. That moved totals from 2,803 to 1,823.Controls and mutation
11 tests through the real engine. Fires on: plain code; inside a quasiquote template; in a
defmacrobody; through an unquote inside a template; and a quoted sibling does not silence its unquoted neighbour. Silent on: quoted type specifier, quoted arithmetic, long-hand(quote …),'(a ,(…)), deeply nested.6/6 mutants killed, each verified non-no-op by
git diff --no-ext-diffbefore running — including M1.hard→.is_data(), the obvious wrong fix.Reported, not fixed
About 970 of
equality-arity's remaining 1,307 findings are one-argument(eql X)in unquoted type contexts — EQL specializers indefmethodlambda lists,typecase/deftransformclause heads. That rule is likely >80% false overall, but fixing it needs a type-context model, not a quote guard.Scope
No golden moved.
RULE_COUNTstill 358. No rule added or removed.cargo build --workspace/fmt --check/clippy --all-targets --all-features -D warnings/test --workspace/test --test cliall exit 0.