feat(lint): add 39 lint rules across nine packages - #85
Merged
Conversation
RULE_COUNT 229 -> 268, commands 413 -> 432. Four new packages plus five extended ones; only the dir-per-rule packages ship standalone `inspect` commands, so rules and commands no longer move together. New: - lint-call-shape (5): deeply nested anonymous lambdas, over-long parameter lists, stringly-typed dispatch, unreadable positional call sites, nested parameters shadowing an enclosing one. - lint-documentation (4): stale worked-example arity, over-long docstring summary lines, packages with no purpose doc, unattributed TODOs. - lint-contract-annotation (4): Typed Racket arity disagreeing with the define, vacuous Clojure :pre/:post, `%` in a :pre where it is not yet bound, check-type restating an adjacent declare. - lint-introspection (3): intern into a computed package, a function installed under a runtime-built name, an introspection probe whose nil answer is funcalled unchecked. Extended: - lint-control-flow +7, lint-safety +6, lint-conditional +4, lint-performance +3, lint-portability +3. All 39 are ReportOnly and all but one are HeadFilter::Heads, so a file with none of their heads never dispatches them. `todo-fixme-no-attribution` is WholeTree because its subject is a comment, which is not a node and has no head to filter on; the comment list is itself the guard, and it costs 0us on a comment-free file. Twenty of the fifty-nine rules proposed for this batch were dropped rather than shipped weak, each with its reasoning recorded where a maintainer will grep for it. Several were dropped because the premise was wrong, found by reading the standard rather than by testing: equal bignums *are* eql, so that half of the case-key rule was baseless; `(loop named outer ...)` establishes `outer` instead of nil, so the naive `return` rule would have fired on every ordinary loop; catch/throw is dynamic-extent by design, so a same-file check is a false-positive generator; Typed Racket's `(-> A)` is a nullary function returning A, not an annotation missing its return type; find-class and symbol-function signal rather than returning nil, and fboundp is itself the check. Others were dropped as duplicates of shipped rules, or as unimplementable under Heads without a per-definition file scan. Each package measured its own cost through the engine's RuleTimings and fixed what it found: an operator table consulted before an argument-count check (32ms of a 66ms pass), a top-level form materialized per candidate (4.59s -> 47ms), a package qualifier re-split per table entry, a quote question asked before the cheap disqualifier. Every package pins its doubling ratio at ~2.0 rather than the ~3.7 of a rule that re-scans per invocation. Mutation testing found what the suites could not: three dead guards, two tests that did not discriminate what their names claimed, and a Janet comment prefix never stripped, which had silently exempted the whole dialect. The one new golden finding is a true positive: broad.lisp has a bare top-level `(return-from blk nil)` with no enclosing block.
Verification against the primary sources, after this branch was pushed,
refuted both. Each would have reported correct code.
`check-type-redundant-with-declare` had it backwards. CLHS 3.3.1 says an
implementation "is free to ignore declaration specifiers except for the
declaration, notinline, safety, and special declaration specifiers" --
`type` is not on that list, so a conforming Lisp may discard
`(declare (type integer x))` entirely and the `check-type` is the only
portable guarantee in the pair, not a restatement of it. The Google
Common Lisp Style Guide recommends the exact code the rule flagged, and
`check-type` is correctable via `store-value` where `declare` has no
equivalent.
`clojure-pre-referencing-percent` reported an error the compiler already
raises. With `*assert*` true, the default, `{:pre [(pos? %)]}` fails at
namespace load with "Unable to resolve symbol: %". Its framing was also
wrong: `fn` injects only the bare `~'%`, so `%1` is unbound in `:post`
too, which the rule's name implied was fine.
Both refutations are recorded in the package README beside the two
earlier dropped proposals, named in full so a grep for either rule lands
on the reasoning.
Also narrows the surviving `typed-racket-arity-mismatch`. It already
declined infix arrows, keywords, rest, ellipsis, `->*`, `case->`, `All`,
curried and lambda-bound defines -- but not `opt-proposition`, so
`(-> Any Boolean : String)` against a one-parameter define reported a
mismatch on an example from the Typed Racket reference. The `#:+`/`#:-`
spelling was guarded and the bare `:` was not; both are now.
RULE_COUNT 268 -> 266. The golden diff is deletions only: both rules
carried `"count": 0` on every fixture, so nothing they reported is lost.
Five assertions across three files compared two measured durations and called that machine-independent. It is not: the ratio of two short timings has unbounded variance, worst exactly where the smaller one is smallest, which is the case these tests were about. One failed on CI at load while passing locally. They now assert invocation counts, which are deterministic and say more: "a clean file dispatches this rule zero times" is the head index doing its job, where "three times cheaper" was a proxy for it. Node-count controls sit beside them so a count of 4000 cannot be mistaken for a per-node dispatch, and dense-file counts pin the non-zero side so the zeroes cannot come from an unregistered rule. The timing probes stay, marked `#[ignore]`, still printing their numbers. They earned it: this batch's probes found a 97x, a 5.5x, a 5.3x and a 5.1x self-inflicted cost bug, and an operator table consulted before a cheap size check that cost 32ms of a 66ms pass. Only the CI-blocking assertion was wrong. The five `< 10 s` span-lookup budgets in the support modules are kept and were measured rather than assumed: 21 ms actual against a ~34 s projected regression, so the bound sits 485x above the real cost and 3.4x below what it guards. That is an absolute budget with a real window, not a ratio.
`clean/forms/*` regressed ~23% because seven of the new rules anchor on `defun`, which is all that benchmark contains, so each fired once per definition and paid real cost to conclude nothing. Four causes, in rough order of size: - `RuleSettings::get` built an owned `(String, String)` to probe a map that is empty whenever no `--rule-arg` was given, so every knob read allocated twice to learn a constant. Pre-existing; the new rules only exposed it. Now short-circuits on an empty override map. - `Path::root_child` heap-allocates, and two `support.rs` binary searches called it `log2(forms)` times per definition while their doc comments claimed the lookup did not allocate. Added `SyntaxTree::root_child_span`, an index into a slice and a field read. - Both docstring rules built a `DefinitionShape` and unescaped the docstring before the cheap test that decides the outcome. Both now pre-guard on the raw literal bytes. - `for_each_evaluated_positioned` grew its stack from one element and descended into childless nodes, which can never be a call. Every reordering is result-identical because each guard failure yields the empty result. Verified by differential run: 40 files x 15 command shapes, 600 invocations, exit code and stdout byte-identical against the pre-change binary, with each edited rule confirmed to actually fire. New-rule cost as a share of the existing catalogue's: 37.8% -> 11.3%.
This was referenced Aug 3, 2026
takeokunn
added a commit
that referenced
this pull request
Aug 3, 2026
`lint-introspection`'s `the_cost_of_each_rule_grows_linearly_with_the_ input` asserted a wall-clock doubling ratio below 3.0. It failed three CI runs on untouched code, at 4.30x and 9.24x, and passed 3/3 on re-run at load average 69. Its own docstring claimed "neither is a wall-clock threshold -- those are what make a test flaky on a loaded machine", which was wrong about itself. A ratio of two wall-clock measurements normalizes for machine *speed* but not for load changing *between* the two measurements, which is exactly what a shared box does. Split, following the precedent PR #85 set for lint-documentation, lint-contract-annotation and lint-performance: - `each_rule_is_dispatched_once_per_head_match` keeps the deterministic half -- the engine's own invocation counter -- and gains an explicit assertion that doubling the definitions doubles the dispatch count. That is the shape the ratio existed to catch (a rule re-walking the file per match), stated as a property of dispatch rather than of nanoseconds. - `ignored_bench_doubling_ratio` keeps the timing table behind `#[ignore]`, with the invocation to run it by hand. Mutation-checked: perturbing one expected invocation count by 1 fails the new test, so it still discriminates. An audit of the remaining ratio assertions found no others to fix. `lint-form-shape` and `lint-performance` are already `#[ignore]`d. `lint-package-hygiene`'s is deliberately left alone -- it normalizes against a control rule measured in the same pass, which absorbs machine load, and its comment says so; it has never flaked.
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.
Summary
RULE_COUNT229 → 268, commands 413 → 432. Four new packages plus five extended ones. Only the dir-per-rule packages ship standaloneinspectcommands, so rules and commands no longer move together — 39 rules, 19 commands.New:
lint-call-shape(5),lint-documentation(4),lint-contract-annotation(4),lint-introspection(3).Extended:
lint-control-flow+7,lint-safety+6,lint-conditional+4,lint-performance+3,lint-portability+3.All 39 are
ReportOnly, and all but one areHeadFilter::Headsso a file with none of their heads never dispatches them.todo-fixme-no-attributionisWholeTreebecause its subject is a comment — not a node, so there is no head to filter on. The comment list is itself the guard: 0 µs on a comment-free file.Twenty of the fifty-nine proposed rules were dropped rather than shipped weak
Each with its reasoning recorded where a maintainer will grep for it. Several were dropped because the premise was wrong, found by reading the standard rather than by testing:
case-key-eql-pitfalleql— CLHS says "both numbers of the same type and the same value"return-outside-implicit-nil-block, as first specified(loop named outer …)establishesouterinstead ofnil, so the naive rule fires on every ordinaryloopthrow-without-matching-catchcatch/throwis dynamic-extent by design; the catch is normally in a caller, often another filetyped-racket-missing-return-type(-> A)is a nullary function returningA— a complete annotation, not a missing return typeintrospection-probe-uncheckedfind-class/symbol-functionsignal rather than returning nil, andfboundpis the checkread-line-eof-value-unhandledfloat-format-directive-precision-unspecifiedOthers were dropped as duplicates of shipped rules (verified down to an already-passing test in the existing rule's suite) or as unimplementable under
Headswithout a per-definition file scan.Each package measured its own cost and fixed what it found
Using the engine's own
RuleTimings:Every package pins its doubling ratio at ~2.0, rather than the ~3.7 of a rule that re-scans per invocation.
Mutation testing found what the suites could not
Three dead guards, two tests that did not discriminate what their names claimed, and a Janet comment prefix never stripped — Janet uses
#where every other dialect uses;, so every Janet comment read as prose and the whole dialect was silently exempt. Caught by an engine-dispatch test, not a unit test.Several packages also ship a permanent sweep over the repo's own fixtures plus hand-written realistic-correct code, each paired with a "dangerous twin" asserting the harness can still detect findings — one package found its corpus held zero task markers and would have proved nothing, and added a candidate-count assertion before claiming zero findings.
Test plan
cargo build --workspacecargo test --workspace— 12068 passedcargo test --test cli— 3083 passedcargo fmt --check,cargo clippy --all-targets --all-features -- -D warningsinspect lint --list-rules --preset allreports 268;recommendedreports 254 (8 new pedantic rules withheld)finding_count, and the one new finding is a true positive —broad.lisphas a bare top-level(return-from blk nil)with no enclosing blockNote
nix build .#checks.aarch64-darwin.clippywas not run: it only sees git-tracked files and the four new packages were untracked at the time, which would have needed a git index write. They are tracked as of this commit, so it is runnable now if wanted.