Skip to content

fix(lint): stop ignore-declaration-conflict misreading lambda lists - #90

Merged
takeokunn merged 1 commit into
mainfrom
feat/fix-ignore-declaration-conflict
Aug 3, 2026
Merged

fix(lint): stop ignore-declaration-conflict misreading lambda lists#90
takeokunn merged 1 commit into
mainfrom
feat/fix-ignore-declaration-conflict

Conversation

@takeokunn

Copy link
Copy Markdown
Collaborator

Stacked on #89. A shipped rule that reports 45 findings on real code, 43 of them false positives. It now reports 2, and both are real.

What it was actually doing

The two largest false-positive classes were not body-walk bugs — the rule could not read a lambda list:

class n cause
Destructuring macro lambda lists 21 parameter_names took atom_text(p), or the first child of a sublist, so (defmacro m ((a b) …)) lost b and then reported it as referenced-but-not-bound
supplied-p variables 5 &optional (o 1 op) lost op
Quoted / templated declarations 8 (declare (ignore ,@dummies)) inside a macro template read as a real declaration
Shadowing by an inner binding form 8 an inner lambda rebinding the name counted as a use of the outer one
Lisp-2 operator position 1 (signal int) calls the CL function, a different namespace

The first two are 60% of the total and were missed by an earlier spot-check of this same rule — which is the argument for measuring over a corpus rather than reading findings.

The two true positives are real

bordeaux-threads/apiv2/impl-corman.lisp:30 and :39: (defun %thread-yield () (declare (ignore thread))) — a zero-argument definition carrying a declaration copy-pasted from its neighbour (defun %interrupt-thread (thread function) …). Exactly what the NotBound half exists for, and both are preserved.

The fix

A section-aware lambda-list parser (Required / Rest / Defaulted / Marker) that recurses into destructuring patterns only for defmacro, takes the supplied-p slot but never the default slot, and treats an unreadable lambda list as opaque for the NotBound half only. body_uses now walks with a signed quote depth, skips operator position, and prunes rebinding forms while still walking their initialisers.

The signedness is load-bearing and a saturating counter cannot replace it: at srctran.lisp:5344 the reference sits inside ,(…) in a template that the lambda itself is inside, so the unquote escapes outward — only depth exactly 0 is a real reference.

Also fixes a latent bug no finding exposed: a defmethod qualifier ((defmethod print-object :around ((x foo) s) …)) displaced the lambda list, so every declared name reported as unbound.

Self-contained in lint-conventionno dependency on another feature package, so feature_dependency_contract.rs is untouched.

Guarding against over-suppression

Over-suppression is the default failure mode here, and it's worse than the bug: a rule that silently stops reporting looks identical to one with nothing to report.

  • Differential over the corpus, denominator reported: 1291 files, 2224 (ignore occurrences, 7690 (declare, 21945 candidate heads, 25.7 MB. Removed exactly the 43 adjudicated false, zero new findings, both true positives preserved. Full before/after lists in the README.
  • 33 new tests (43 total), every suppression paired with a must-still-fire control differing only in the keyed detail.
  • 15/15 mutations killed. The first mutation run was the valuable one — it found 5 defects in my own tests: four "must still fire" controls rebound a different name than the one under test, so the shadowing branch was never entered; and one passed only because ,b keeps its prefix in the atom text. All rewritten.

Cost

HeadFilter::Heads retained. Zero-findings path is flat at 1.04–1.07×body_uses is unreachable without an ignore declaration, so clean/forms/* is unaffected. Declaration-bearing files cost ~1.7×; per-node work is up but the walk short-circuits and prunes. Measured interleaved with per-round control normalisation, because at load 56–80 the unchanged control rule moved 2.2× between windows.

No golden moved, and that is not reassurance

tests/fixtures/lint_golden/ contains no (declare (ignore …)) at all, so this rule is pinned at 0 in all four goldens before and after. The corpus differential is what carries the claim; regeneration would have proven nothing.

Verification

cargo build --workspace, cargo test --test cli (3083 passed), cargo fmt --all --check, cargo clippy --all-targets --all-features -- -D warnings — all exit 0.

cargo test --workspace has one pre-existing flaky failure unrelated to this diff: lint-introspection's the_cost_of_each_rule_grows_linearly_with_the_input, a wall-clock ratio assertion. This diff touches only lint-convention; the test passes 3/3 on re-run. It's a surviving instance of the class PR #85 replaced with deterministic dispatch counts in three other packages, and it's worth the same treatment.

@takeokunn
takeokunn force-pushed the feat/lint-batch-r8 branch from 6dd3657 to 0c76a08 Compare August 3, 2026 07:41
@takeokunn
takeokunn force-pushed the feat/fix-ignore-declaration-conflict branch from 230cb9a to f41e9e7 Compare August 3, 2026 07:42
Base automatically changed from feat/lint-batch-r8 to main August 3, 2026 08:07
Over 1291 files (SBCL src/ plus the installed Quicklisp dist, 2224
`(ignore` occurrences) the rule produced 45 findings, of which 43 were
false positives on code SBCL compiles clean. It now reports 2, and both
are real: bordeaux-threads' impl-corman.lisp:30 and :39 declare `thread`
ignored in a zero-argument `%thread-yield`, a declaration copy-pasted
from the neighbouring `%interrupt-thread`.

The two largest false-positive classes were not body-walk bugs at all --
the rule could not read a lambda list:

- **Destructuring macro lambda lists (21 findings).** `parameter_names`
  took `atom_text(p)`, or the *first* child of a sublist, so
  `(defmacro m ((a b) ...))` lost `b` entirely and then reported it as
  referenced-but-not-bound.
- **`supplied-p` variables (5).** `&optional (o 1 op)` lost `op`.

Together 60% of the false positives. The remaining 17 are the classes a
previous spot-check had identified: quoted/templated declarations (8),
shadowing by an inner binding form (8), and one Lisp-2 operator position
-- `(signal int)` calls the CL function, not the ignored variable.

Replaced with a section-aware lambda-list parser (Required / Rest /
Defaulted / Marker) that recurses into destructuring patterns only for
`defmacro`, takes the supplied-p slot but never the default slot, and
treats an unreadable lambda list as opaque for the NotBound half only.
`body_uses` now walks with a **signed** quote depth, skips operator
position, and prunes rebinding forms while still walking their
initialisers.

The signedness is load-bearing and a saturating counter cannot replace
it: at srctran.lisp:5344 the reference sits inside `,(...)` in a
template that the `lambda` itself is inside, so the unquote escapes
*outward* and only depth exactly 0 is a real reference.

Also fixes a latent bug no finding exposed: a `defmethod` qualifier
(`(defmethod print-object :around ((x foo) s) ...)`) displaced the
lambda list, so every declared name reported as unbound.

33 new tests, each suppression paired with a must-still-fire control
differing only in the keyed detail. 15/15 mutations killed. The first
mutation run found 5 defects in those tests: four controls rebound a
*different* name than the one under test, so the shadowing branch was
never entered, and one passed only because `,b` keeps its prefix in the
atom text.

Zero-findings cost is flat at 1.04-1.07x -- `body_uses` is unreachable
without an ignore declaration, so `clean/forms/*` is unaffected.

No golden moved, and that is not reassurance: `tests/fixtures/
lint_golden/` contains no `(declare (ignore ...))` at all, so this rule
is pinned at 0 there before and after. The corpus differential is what
carries the claim.
@takeokunn
takeokunn force-pushed the feat/fix-ignore-declaration-conflict branch from f41e9e7 to 743a652 Compare August 3, 2026 08:07
@takeokunn
takeokunn merged commit d110064 into main Aug 3, 2026
10 checks passed
@takeokunn
takeokunn deleted the feat/fix-ignore-declaration-conflict branch August 3, 2026 08:33
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