Skip to content

fix(lint): stop equality-arity reporting eql type specifiers - #121

Merged
takeokunn merged 1 commit into
mainfrom
fix/equality-arity-type-context
Aug 4, 2026
Merged

fix(lint): stop equality-arity reporting eql type specifiers#121
takeokunn merged 1 commit into
mainfrom
fix/equality-arity-type-context

Conversation

@takeokunn

Copy link
Copy Markdown
Collaborator

Follows #119 and #120. CLHS 4.2.3 makes (eql object) a compound type specifier — so in a type position one argument is the only legal spelling, not a defect. equality-arity reported it anyway, at Severity::Error.

The measured rate is worse than estimated

A seeded random sample of 120 of the 1,307 findings #120 left behind, each read with source context:

count
standard-CL type positions 71
SBCL-internal type positions 25
non-type positions (case keys, trivia patterns, multiple-value-bind) 24
true positives 0

Essentially all 1,307 are false positives (95% CI ≥ 97.6%) — against my ~74% estimate. Unsurprising once stated: the corpus is shipped, compiling code, and a genuine one-argument (eql x) in call position is a compile-time error.

Why this could not be another guard

The quote guard was a local predicate. This is not: 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 must know the node's role, which lives in its ancestor.

type_table() cannot answer it. It exposes expression_type and binding_type, both asking what type an expression has. (eql 7) in a specializer has no type — it is one. And infer_view (typing/service/inference.rs:104) returns Type::Unknown for anything is_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: a structural ancestor walk, mirroring the existing quote_state_at root descent in the same file.

The contexts were measured, not assumed

A probe crate path-depending on the repo's own paredit-core-syntax/lint-engine ran the real dispatch with only this rule enabled, joined each finding to its ancestor chain by byte span, climbed past compound-specifier combinators, and bucketed the nearest non-combinator ancestor. That distribution produced the set:

context standard CL? implemented
typecase/etypecase/ctypecase clause head yes
defmethod / defgeneric :method EQL specializer yes
declare/declaim/proclaim (type …)/(ftype …) yes
the, check-type yes
slot :type (defclass/defstruct) yes
defknown, deftransform, define-vop, specifier-type, … no — SBCL internals ❌ deliberate

Two shapes on the original list 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; only the quoted '(eql 5) spelling is a specifier, which is #120's job. Anchoring them dropped the overlap with #120 from 540 to 3. There are now tests asserting all three stay reported. satisfies and member are not descent paths either.

SBCL internals excluded on principle: teaching a general CL linter one implementation's compiler macros would silence any user macro sharing a name. That leaves 384 findings unfixed and reported rather than hidden.

Mutation testing found a bug in the draft

Dropping a shape check revealed that (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 genuine 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.

13/13 mutants killed, each verified changed by byte comparison and git diff --no-ext-diff before running. The harness itself had a flaw caught in passing: matching "error: " on stderr misread cargo's error: test failed and reported 12 false "broken" mutations; tightened to error[E/could not compile.

Differential

total hard-quoted post-#120 remainder
before 1,981 674 1,307
after 1,302 671 631

679 removed, 0 added — all eql at exactly one argument; no other operator or arity moved. Only 3 overlap #120, so the net effect on the 1,307 is 676 (−51.7%).

Removals by anchor: 318 specializer, 271 typecase-family, 41 declare/declaim, 23 slot :type, 16 the, 10 check-type.

Controls

Still fires on: (eq x), (eql x), (eql), (eql a b c), (equal a), (equalp a b c d) — as progn body forms, inside defun, and inside a quasiquote template; on (typep x (eql y))/subtypep/coerce; on (or (eql x) y) under a bare or; on a case clause head; on a typecase keyform; on the's value form; and on the specializer shape under a non-method definer and in a method body.

Silent on: defmethod specializers (including :around, (setf g), :method, shouted, cl:-qualified, quasiquoted templates), typecase-family clause heads including nested (cons (eql …))/(or …)/(and … (not …)), declare/declaim types, the, check-type, slot :type.

Cost

Whole corpus 4.749 s vs 4.493 s (+5.7%, ~0.13 ms per finding adjudicated). On the 5,097 zero-finding files: 0.767 s vs 0.756 s — noise. The walk sits behind argument_count == 1 && operator == "eql", both of which require a finding to exist, so clean code never reaches root_view().

Still unfixed, worth separate PRs

631 remain and are still ~100% false — but for reasons outside this scope: 384 SBCL-internal (recommend leaving), 136 case/ecase clause keys ((case kind (eql <body>)) is a key designator, never a call), 49 trivia patterns, 33 multiple-value-bind variable lists.

Given the measured rate, Severity::Error on this rule is hard to justify until at least the case-key class is fixed.

No golden or pinned count moved. cargo build --workspace / fmt --check / clippy --all-targets --all-features -D warnings / test --workspace all exit 0.

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.
@takeokunn
takeokunn merged commit 605d350 into main Aug 4, 2026
10 checks passed
@takeokunn
takeokunn deleted the fix/equality-arity-type-context branch August 4, 2026 06:07
takeokunn added a commit that referenced this pull request Aug 4, 2026
Third and last of the structural false-positive classes in this rule.
`(case kind (eql <body>))` names a *key* to compare `kind` against, and
`(multiple-value-bind (equal certain) ...)` binds variables called
`equal` and `certain`. Neither is a call, and CLHS 5.3 is explicit that a
`case` clause is `(keys form*)` with an atom key standing for a singleton
list.

The context set was measured rather than assumed: a probe of a 26-form
wide net of binding candidates found 224 findings in four classes, not
the 169 in two that had been estimated.

  case/ecase clause, atom key      155
  case/ecase clause, key list       40   `((eql char=) ...)`
  multiple-value-bind variables     25
  let binding list                   4
                                   ---
                                   224

The other twenty-two neighbours -- `destructuring-bind`, `dolist`,
`defun`, `lambda`, `do` and the rest -- fire zero times, which is worth
as much as the four that do: it bounds the change. Both families are
implemented anyway, 21 fixed-index lambda-list anchors and 10
binding-list heads, all standard Common Lisp, since a position that is
provably not a call cannot become one. `defmethod` is excluded: its
lambda-list index moves with qualifiers, and PR #121 already models it.

The boundary this creates is the delicate part, because a clause head and
a clause body are adjacent and take opposite verdicts. It is pinned by a
single assertion -- `count("(case kind (eql (eql x)))") == 1` -- same
operator, same form, key silenced and body reported.

Findings go 631 to 407 on a corpus of 5556 unique files. The removal set
is identical whether applied to `main` or to #121 and overlaps #121's own
676 not at all, so neither guard masks the other. Fourteen random
findings read against their real source were fourteen false positives:
`constraint.lisp` genuinely writes `(let (mark (eq (lambda-var-eq-
constraints leaf)) ...)`, a variable named `eq`, three lines above a real
two-argument `(eq other-ref ref)`.

PR #121's `a_case_clause_head_is_not_a_type_position` asserted that the
rule *reports* `(case x ((eql 5) 1))`, to prove its type anchor stops at
`typecase`. This guard now declines that shape for an unrelated reason,
so the assertion has stopped testing what it was for. It is split: the
lib test becomes `a_case_clause_head_is_not_reported_as_a_call`, and the
invariant it existed for is asserted directly against
`is_eql_type_specifier_at` in `support.rs`. Otherwise the key guard would
silently mask a future regression in the type guard.

407 findings remain and are, as far as the sampling shows, all still
false: roughly 358 SBCL-internal type contexts and 49 trivia pattern-DSL
forms. Both have one cause -- a macro's arguments are not evaluated, and
a general linter cannot know which macros those are -- so closing them
needs a mechanism rather than more tables. Until then this rule reports
only false positives on real code at `Severity::Error`, and demoting it
is worth considering.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant