chore(lints): gate span arithmetic, lexer indexing, and production unwrap - #1229
Merged
Conversation
Adopt two clippy lints on the paths that handle attacker-controlled input, each validated by replaying it against the tree that carried the bug it claims to catch, and drop the sixteen `expect(FEATURES_PINNED)` call sites the CLI and web crates used to discharge a widened `Result`. `clippy::arithmetic_side_effects` on the `loc` metric module. Replayed at 92680fa^ it flags both of #1051's reported panic sites — the `end - 1` in `loc/rust.rs` and the `end - start` in `add_cloc_lines`. Scoped to `loc` rather than `src/metrics/`: the other metrics contribute 244 hits, all `+=` on their own counters, which is not this bug class and would bury it. The 69 in-scope hits resolve to zero `#[allow]`s in the span code — 36 `logical_lines += 1` collapse into `Lloc::count_logical_line`, and the rest become saturating — plus one file-level carve-out in `line_set.rs`, where the arithmetic is on word indices and saturating would read the wrong row rather than prevent anything. `clippy::indexing_slicing` on `src/c_macro.rs`. The issue rated this unproven; it is not. #126 was `&DOLLARS[..(i - start)]` panicking on a 2049-byte macro identifier, and replayed at f59c5c0^ the lint flags both slices. Nine per-function carve-outs, never file-wide, so a new function is covered by default. `step_raw_string`'s delimiter comparison is hardened with `get` instead of allowed: its bound is established in `enter_raw_string` and carried through `LexState`, which is the #126 shape and the one a reader cannot check locally. `expect(FEATURES_PINNED)` was sound only while `MetricsError` had no variant it did not handle, and that enum is `#[non_exhaustive]` with a doc reserving the right to add variants in a minor release — a panic scheduled against a routine dependency bump. Both crates already had an error channel: the CLI's helpers return `io::Result`, whose runner prints a per-file line and continues, and the web handlers have an existing sanitized 500. Neither needed a new failure mode, only for the existing one to be used. No reachable behaviour change today, so both error paths are pinned by unit tests on the mapping rather than end-to-end. Metric values are unchanged: a saturating operation equals the plain one unless it would have overflowed, and none does. `dispatch_find` and `dispatch_exemptions` gain one `nexits` each — the `?` that replaced the `expect` — tripping the soft tier at 4.75. Both are baselined; the hard limit of 5 is unchanged and still met. Fixes #1152
The seven handlers each repeated `.await?.map_err(ParseError::from_metrics)?` after dropping their `expect(FEATURES_PINNED)`. That is seven chances to reach for `expect` again instead of propagating, which is the thing #1152 was cleaning up. One wrapper leaves each handler with a single `?`. The three VCS handlers keep plain `run_parse`: their closures do not return `MetricsError`.
Its callers are the per-language `loc/*.rs` modules, which are descendants of `loc` and so can already see a private item — the same reason they could write to the private `logical_lines` field before. `pub(crate)` claimed a wider surface than the helper has.
Both claimed more than the change buys, found in review. `add_cloc_lines`: `comment_diff` feeds only the two branch tests, never a row index, so both branches already hand `add_only_comment_lines` the raw span and `insert_range` already rejects an inversion. The `saturating_sub` changes which branch runs, not whether the rejection happens. `span_rows`: `sloc()` already clamps with `saturating_sub`, so a wrapped `excluded_lines` could never have escaped as `usize::MAX`. It would have escaped as `sloc: 0` for a non-empty file. Same class as the follow-up the #1051 fix itself needed on this function.
Triage of the 37 production `unwrap`/`expect` sites #1227 asked for, plus the one gate that triage justifies. The count is the finding: **0 production `unwrap()`, 37 production `expect()`**, measured with the lints enabled over lib and bin targets so `#[cfg(test)]` modules are excluded by construction rather than by a grep heuristic. `unwrap_used` therefore costs nothing to adopt today and fails CI on the first one added. It is set per crate root as `#![cfg_attr(not(test), warn(...))]`, not in `[workspace.lints]`, because a Cargo lint applies to every target of its package: the same measurement across `--all-targets` reports 1_023 `unwrap()` and 2_157 `expect()`, all of them legitimate test code. `cfg(test)` is set for integration-test crates as well as for the unit-test target, so the per-root form needs no per-file carve-out and carries zero `#[allow]`s. Verified in both directions with a cache-busted probe: a production `unwrap` is flagged, the same `unwrap` in a `#[cfg(test)]` module is not. `expect_used` is declined. All 37 sites already name their invariant in the message, which is the form `AGENTS.md` sanctions, so gating it buys 37 annotations that each restate the line above them — the "drowned in allows, reads as coverage" outcome #1152 was avoiding. The distinction that decides it is not `expect`-vs-`unwrap` but what can invalidate the invariant. The `FEATURES_PINNED` sites #1152 removed rested on a `#[non_exhaustive]` enum a dependency bump could change underneath them. These 37 rest on facts local to this repository — a constant regex compiles, a walker pushed a root before descending, a `strip_prefix` follows a `starts_with` three lines up — which review and tests already cover. No test accompanies the gate: the only shape available would grep this repository's own source for the attribute, which `.claude/rules/testing.md` bans as vacuous. The lint firing in CI is the test. Fixes #1227
#1227's gate stopped at the cargo workspace, and `enums` is not in it. The crate is excluded from `[workspace] members` for build reasons but is still first-party Rust, still CI-linted (`make enums-check` runs clippy with `-D warnings` and its tests), and held 7 production `unwrap()` calls that `cargo clippy --workspace` never saw. So the "0 production `unwrap()`" figure was true of the workspace and not of the repository, which is the reading the gate invites. One of the 7 was a live latent panic rather than a style issue: the Go generator's `names.iter().map(|x| x.0.len()).max().unwrap()` panics on a grammar that contributes no token names. It becomes `unwrap_or(0)`, which is the identity here — with no names the `map` below yields nothing, so the padding width is never read. The rest propagate onto the `io::Result<()>` every generator already returned, via a shared `render_error` lift for the `askama` failures. `env::var("CARGO_MANIFEST_DIR").unwrap()` becomes `env!(...)`: the compile-time macro is both infallible and more correct here, since it resolves the enums crate's own source tree rather than whatever environment the binary is invoked from. Both `enums` roots (lib and bin) now carry the same `#![cfg_attr(not(test), warn(clippy::unwrap_used))]` as the eight workspace roots, so the gate's claim now matches its scope. Verified by the existing codegen-drift gate rather than by assertion: `utils/check-enums-codegen-drift.sh` regenerates in both `rust` and `c_macros` modes and diffs against the checked-in output. `c_macros` is the mode that goes through the `CARGO_MANIFEST_DIR` path, and the output is byte-identical. The new `render_error` lift carries a unit test; the empty-`names` case has no reachable input through any real grammar, so `unwrap_or(0)` is correct-by-construction rather than a fixed live bug with a regression test. Follow-up to 8cf09bc (#1227).
The scripted insertion in 7ba15b1 placed the attribute between the first `//!` line and the rest of the crate doc in two files. In `big-code-analysis-cli/src/main.rs` that split a sentence across it. rustdoc concatenates the blocks so the rendered output was unaffected, and rustfmt does not move attributes, so neither gate could see it. Also drops `use std::env;`, left dead by the `env::var` -> `env!` change. rustc does not warn: the `env!` macro path marks the name used, which is why `-D warnings` stayed green. Verified by compiling without the import. Both moved attributes are still in effect — a probe `unwrap()` in `main.rs` is flagged.
Found in review. The changelog and the go.rs comment described the `max().unwrap()` conversion as fixing a live latent panic. It is not reachable: `get_token_names` walks `0..node_kind_count()` and every real tree-sitter grammar has at least the ERROR sentinel, so `names` is never empty and `.max()` is never `None`. The conversion is still right, for the reason #1227 rests on rather than the one claimed: an `unwrap()` states no invariant, which is what separates it from the 37 `expect` sites left alone. Both now say that instead. Third time this session that a guard comment claimed more than the change buys.
All comment and prose only; no behaviour change. `run_parse_fallible` was inserted between `run_parse`'s doc block and `run_parse` itself, so the block documented the wrong function and left `run_parse` undocumented. This is the third time in this branch that an insertion-based edit landed between a doc comment and its item — the other two were the crate-root attributes fixed in 7f8efe5. Neither rustfmt nor clippy can see this class, and `missing_docs` does not fire because `run_parse` is private. "Sixteen `.expect(FEATURES_PINNED)` call sites" is fifteen: 8 in the CLI dispatch helpers and 7 in the web handlers, plus the two constants, which the original wording folded into the site count. Corrected in CHANGELOG.md and lessons-learned; the commit messages in this branch still say sixteen and are left as history. `dispatch_dump` still carried "the `expect` documents that invariant" above a line that now propagates with `?`. The `add_cloc_lines` rationale, itself already corrected once in a0925a7, was still imprecise: the `== 0` arm does not call `add_only_comment_lines` at all, and the `> 0` arm passes `start + 1` rather than the raw span. It now says what the `saturating_sub` actually buys — a truthful branch classification, not an observable metric change.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1229 +/- ##
==========================================
+ Coverage 98.33% 98.35% +0.01%
==========================================
Files 278 278
Lines 72043 72068 +25
Branches 71613 71638 +25
==========================================
+ Hits 70845 70879 +34
+ Misses 788 779 -9
Partials 410 410
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Codecov reported 95.52% patch coverage against 98.33% project, with three missing lines: the `PreprocArg` range-insert in `loc/c.rs`, `loc/mozcpp.rs` and `loc/objc.rs`. C++'s identical copy was already covered, which is the per-language blind spot `AGENTS.md` warns about — these four modules are deliberate clones, so one passing language said nothing about the other three. A backslash-continued `#define` body parses as one `PreprocArg` node spanning every continuation row, and each of those rows is PLOC because `tree-sitter-cpp` does not expand macros. The fixture is one macro across three rows plus a `main`, swept over all four languages through `metrics_verbatim`, which takes a `LANG` — the only way to reach `Mozcpp`, which owns no file extension. Values are measured, not guessed, and the test is confirmed discriminating per language: deleting the arm from each module in turn fails it, each time compiling first so the failure is the assertion and not the build. `ploc` is 4 with the arm and 3 without. This closes the patch-coverage gap; the three lines were the only ones in the PR that any test could reach. My earlier "zero coverable lines" claim was scoped to the lint-gate commits alone, not the whole PR.
Owner
Author
Patch coverage: the three missing lines are coveredCodecov reported 95.52% patch vs 98.33% project, with three missing lines — C++'s identical copy of that arm was already covered. That is exactly the
Values are measured rather than guessed, and the test is confirmed
That is stronger than the coverage number asks for: a line can be executed Correcting my own earlier claimI wrote in the PR body that the change has "zero coverable lines". That was |
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.
Closes #1152. Closes #1227.
Two clippy lints adopted on the paths that handle attacker-controlled input,
each validated by replaying it against the tree that carried the bug it
claims to catch, plus removal of the fifteen
.expect(FEATURES_PINNED)callsites the CLI and web crates used to discharge a widened
Result.Validation, not argument
#1152 asked for this explicitly: "A lint adopted on the strength of an
argument and then drowned in
#[allow]s is worse than no lint, because itreads as coverage." Both adoptions replay a real historical bug.
arithmetic_side_effects92680fa8^(pre-#1051)loc/rust.rs:48(end - 1) andloc/shared.rs:46(end - start)indexing_slicingf59c5c06^(pre-#126)&DOLLARS[..(i - start)]slicesunwrap_usedThe issue was wrong about the second one. #1152 rated
indexing_slicing"plausible but unproven; ~20 raw indexes in
c_macro.rs, none of which hasproduced a bug." But #126 — "DOLLARS buffer overflow panics on macro
identifiers longer than 2048 bytes" — was a slicing panic in that exact
file, reachable from user source plus a user-supplied macro set. It was adopted
on replayable evidence rather than the weaker prevention footing the issue
assumed.
Scope was decided by measurement
arithmetic_side_effectson the issue's suggestedsrc/metrics/gives 313hits. The distribution is what decides it:
+=(counter increments)+-*All 15 subtractions are in
src/metrics/loc/, andlocis the only module insrc/metrics/that computes on tree-sitter span coordinates rather than itsown counters. The other 244 hits are not this bug class and would bury it.
Total
#[allow]count across both lints: 10, each naming its invariant —nine per-function in
c_macro.rs(so a newly added function is covered bydefault, verified with a throwaway unguarded index) and one file-level in
line_set.rs. Zero in thelocspan code: 36logical_lines += 1collapsedinto
Lloc::count_logical_line, the rest made explicitly saturating.The
line_set.rscarve-out is deliberate and is the one worth a reviewer'sattention: its arithmetic is on bitset word indices, where saturating
self.words[word - self.first_word]to index 0 would read the wrong row andsilently miscount, whereas the current form panics on a corrupt offset — the
way
intersection_len's existing comment says it is meant to fail. Preventionand masking point in opposite directions there.
unwrap_usedis not a[workspace.lints]entry0 production
unwrap(), 37 productionexpect()— measured over lib andbin targets so
#[cfg(test)]modules are excluded by construction. Across--all-targetsthe same measurement is 1,023unwrap()and 2,157expect(), essentially all legitimate test code, so a Cargo lint (whichapplies to every target of its package) was not an option.
It is
#![cfg_attr(not(test), warn(clippy::unwrap_used))]at each of the tenproduction crate roots. The load-bearing fact — checked rather than assumed,
because the widely-repeated claim is the opposite — is that
cfg(test)isset for integration-test crates, not just the unit-test target. One form
therefore covers
#[cfg(test)]modules andtests/*/main.rsalike: noper-file carve-out, no footgun for the next integration test added, and zero
#[allow]s.expect_usedis declined. All 37 sites already name their invariant in themessage, the form
AGENTS.mdsanctions, so gating it buys 37 annotations thatrestate the line above them. The distinction that decides it is not
expect-vs-unwrapbut what can invalidate the invariant: theFEATURES_PINNEDsites rested on a#[non_exhaustive]enum a dependencybump could change underneath them; these 37 rest on facts local to this
repository. That reasoning lives in
[workspace.lints.clippy], where the nextperson will look for the absent lint.
FEATURES_PINNED— wider than the issue scoped#1152 named
dispatch.rs.big-code-analysis-web/.../handlers.rscarried theidentical latent panic at 7 more sites, so all 15 are gone. Both crates already
had an error channel, which is the tell the
expectwas never necessary: theCLI maps to
io::Error(InvalidData)that the concurrent runner reports per fileand continues past; the web folds into the existing sanitized
500, which isthe same status unwinding through
spawn_blockingalready produced — whatchanges is that the log names the real cause and no worker thread unwinds.
Lessons-learned #26, which recommended the
.expect(FEATURES_PINNED)pattern,is corrected in the same change.
Behaviour and risk
unless it would have overflowed, and none does. No snapshots move.
enumscodegen changes are verified by the existingutils/check-enums-codegen-drift.sh, which regenerates in bothrustandc_macrosmodes and diffs against the checked-in output —c_macrosis themode using the
CARGO_MANIFEST_DIRpath that changed. Byte-identical.dispatch_findanddispatch_exemptionseach gain onenexits— the?that replaced the
expect— tripping the soft tier at 4.75. Both arebaselined in the same commit per the refresh discipline; the hard limit of 5
is unchanged and still met. This is the
?-is-an-exit artifactAGENTS.mdcalls out, not new branching.
Tests and coverage
Both new error paths are unreachable end-to-end by the feature pin, so they are
pinned by unit tests on the mapping rather than by a vacuous end-to-end test.
Every mutation against them is caught — wrong
ErrorKind, cause stringifiedinstead of carried, log line deleted, wrong
ParseErrorvariant — eachconfirmed to fail on an assertion rather than a compile error.
Patch coverage cannot drop: outside
enums/**andxtask/**(both incodecov.yml'signorelist as code-generation helpers), the lint-gate diffis 30 lines that are entirely comments and the
#![cfg_attr(...)]attribute —zero coverable lines, measured.
Reviewer notes
make pre-commitis green.simplify-rust,rust-optimize,review,audit-tests) plus a final/code-review. Theirfindings are folded in; several were corrections to my own over-claiming
comments, which is why four commits here are prose-only.
is fifteen plus two constants.
CHANGELOG.mdand lessons-learned arecorrected; the commit messages are left as history.
enumsis CI-linted with-D warningsbut without the workspace pedantic set, since refactor(workspace): propagate pedantic clippy + missing_docs lints (carved out of #150) #158 scoped propagation to
"the three shipping crates". 23 pedantic warnings, all pre-existing,
deliberately not bundled here.