Skip to content
Merged
58 changes: 58 additions & 0 deletions .claude/skills/ci-gates/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -16426,3 +16426,61 @@ does not travel" -- has a shorter form here: it did not travel four lines.**
The mutant that reverts the numerator to `claims_broken.len()` survived three fresh unit tests on
`distinct_claims`. Killed only by a structural test reading the call site: **tenth pass in a row that
the wiring outlived the function.**
### The detector I wrote could not find what the compiler was already saying

`tri types redef` ends with a sentence that reads as a partition of the rows it just
printed: *N state different NUMBERS; N differ in which fields they state, N in the text
of a field, N only in prose, N identical.* It summed to **345**. The command had printed
**346** rows.

The dropped row was the only `SIGNATURE` one — two definitions of `delete` in one spec,
one taking a line range and one taking a path. The second-most-severe class the command
has, and the tally is the sentence a reader carries away.

`signature` was incremented in the `match` and left out of the `println!` argument list.

The part worth keeping is not the bug. That pass opened by building a detector for a
neighbouring shape — *a guard checked for reachability rather than correctness* — which
narrowed 69 candidates to 16 by requiring that the variable be **read by a control-flow
test**. That detector could not have found this defect, because the defect **is the
absence of any read**. Its shape requirement excluded the thing it was hunting.

The instrument that answers "nothing reads this" ships with the compiler and had been
printing it on every build for as long as the line existed:

```
warning: variable `signature` is assigned to, but never used
warning: value assigned to `signature` is never read
```

Every build in that session was run as `cargo build 2>&1 | grep -E '^error'`, or with
its output piped to `/dev/null` outright. The output was discarded because 23 warnings
of mostly-cosmetic noise is not worth reading — which is exactly how the one warning
that mattered stayed invisible for as long as it did.

Three consequences.

**Before writing a detector, ask what the toolchain already reports.** A bespoke matcher
competes with `rustc`, `clippy`, the type checker, and the linter — all of which run on
every build and none of which need a population argument. Write the detector for what
they *cannot* say.

**Classify the noise rather than silencing it.** Not all warnings are equal.
*A value computed and never read* is a different claim from *an item nothing calls*: the
first means work was performed and dropped, which usually means a result that was meant
to reach somewhere and does not. `tri gates warnings` splits them into
`DISCARDED` / `dead` / `cosmetic` / `other`, and `--gate` holds only the first class at
zero. The 16 dead and 6 cosmetic warnings stay visible and ungated.

**A warning report must force the work.** A cached compilation unit emits *no warnings
at all*, so a report run against a warm `target/` reads clean no matter what the code
says — the same broken-ruler shape as a gate whose subject has been deleted.
`tri gates warnings` touches the crate root before checking, and says so in its output
when it finds zero.

And the test for a defect of this shape goes at the **call site**. The omission was from
an argument list, so a test of any counting helper would have passed. Removing
`signature` from the arguments alone does not compile — killed by `rustc`, not by the
test, and reporting that as a kill would have been false. The mutation that *compiles* —
dropping it from both the format string and the arguments — is the one the test must
fail on, and it does, by name.
195 changes: 195 additions & 0 deletions cli/tri/src/gates.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,23 @@ use std::process::Command;

#[derive(Subcommand, Debug)]
pub enum GatesCmd {
/// Classify the compiler's own warnings. `rustc` already answers "is this
/// value ever read" and "does anything call this" for the whole crate; the
/// answer only helps if somebody reads it.
///
/// T106: `signature` in `types_dup.rs` was incremented for every name whose
/// copies disagreed in signature and then left out of the summary's
/// argument list. The tally printed 345 for 346 rows. rustc had said
/// `value assigned to `signature` is never read` on every build for as long
/// as the line existed, and every build in that session was run with its
/// output piped to /dev/null.
Warnings {
/// Exit 1 if any warning falls in the discarded-computation class --
/// a value computed and never read. That is the T106 shape.
#[arg(long)]
gate: bool,
},

/// Run every gate script and its negative control; name the ones with none.
Sweep {
/// Skip the gates themselves and only report which have no control.
Expand Down Expand Up @@ -2622,6 +2639,154 @@ fn is_ident_byte(b: u8) -> bool {
b.is_ascii_alphanumeric() || b == b'_'
}

/// What a compiler warning says about the code, coarser than rustc's lint name.
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
pub enum WarnClass {
/// A value was computed and stored, and nothing ever reads it. This is the
/// T106 shape: work performed and dropped, which usually means a result
/// that was meant to reach somewhere and does not.
Discarded,
/// An item exists and nothing calls it.
Dead,
/// Cosmetic: an unused binding, an unnecessary `mut`.
Cosmetic,
/// Anything else rustc chose to say.
Other,
}

pub fn classify_warning(msg: &str) -> WarnClass {
if msg.starts_with("value assigned to") || msg.contains("is assigned to, but never used") {
return WarnClass::Discarded;
}
if msg.contains("is never used") || msg.contains("is never read") {
return WarnClass::Dead;
}
if msg.starts_with("unused ") || msg.starts_with("variable does not need to be mutable") {
return WarnClass::Cosmetic;
}
WarnClass::Other
}

fn warnings_gate(gate: bool) -> Result<()> {
let root = repo_root()?;
// A cached unit prints no warnings at all, so a report run against a warm
// target directory reads clean no matter what the code says. Touch the
// crate root to force this one crate to be re-checked; deps stay cached.
let main_rs = root.join("cli/tri/src/main.rs");
if !main_rs.is_file() {
anyhow::bail!("{} is missing -- nothing to re-check", main_rs.display());
}
let touched = Command::new("touch")
.arg(&main_rs)
.current_dir(&root)
.status();
match touched {
Ok(st) if st.success() => {}
_ => anyhow::bail!(
"could not touch {} -- without it a cached build reports zero warnings",
main_rs.display()
),
}

let out = Command::new("cargo")
.args([
"check",
"-p",
"tri",
"--bin",
"tri",
"--message-format=json",
])
.current_dir(&root)
.output()
.context("running cargo check")?;
let text = String::from_utf8_lossy(&out.stdout);

let mut rows: Vec<(WarnClass, String, String)> = Vec::new();
for line in text.lines() {
let v: serde_json::Value = match serde_json::from_str(line) {
Ok(v) => v,
Err(_) => continue,
};
if v.get("reason").and_then(|r| r.as_str()) != Some("compiler-message") {
continue;
}
let m = match v.get("message") {
Some(m) => m,
None => continue,
};
if m.get("level").and_then(|l| l.as_str()) != Some("warning") {
continue;
}
let msg = m.get("message").and_then(|x| x.as_str()).unwrap_or("");
// The per-crate summary ("`tri` generated 23 warnings") is not a finding.
if msg.contains("generated") && msg.contains("warning") {
continue;
}
let at = m
.get("spans")
.and_then(|s| s.as_array())
.and_then(|a| a.first())
.map(|sp| {
format!(
"{}:{}",
sp.get("file_name").and_then(|f| f.as_str()).unwrap_or("?"),
sp.get("line_start").and_then(|l| l.as_u64()).unwrap_or(0)
)
})
.unwrap_or_else(|| "?".into());
rows.push((classify_warning(msg), at, msg.to_string()));
}

rows.sort();
let discarded = rows.iter().filter(|r| r.0 == WarnClass::Discarded).count();
for (class, at, msg) in &rows {
let tag = match class {
WarnClass::Discarded => "DISCARDED",
WarnClass::Dead => "dead ",
WarnClass::Cosmetic => "cosmetic ",
WarnClass::Other => "other ",
};
println!(" {tag} {at:<34} {msg}");
}
println!();
// `cargo check -p tri` also checks the workspace crates tri depends on, and
// replays their cached warnings, so this population is not one crate. Say
// whose it is: an unlabelled total is read as belonging to the crate named
// in the command, which is the same defect this command exists to catch.
let mut by_crate: std::collections::BTreeMap<String, usize> = Default::default();
for (_, at, _) in &rows {
let krate = at.split('/').take(2).collect::<Vec<_>>().join("/");
*by_crate.entry(krate).or_default() += 1;
}
let spread = by_crate
.iter()
.map(|(k, n)| format!("{k} {n}"))
.collect::<Vec<_>>()
.join(", ");
println!(
"{} warning(s) across {} crate(s) [{}]; {} in the discarded-computation class.",
rows.len(),
by_crate.len(),
spread,
discarded
);
if rows.is_empty() {
println!();
println!(
"Zero warnings from a crate this size is more likely a cached unit\n\
than a clean one. This command touches {} before checking for\n\
exactly that reason -- if that stopped working, this reads clean\n\
while saying nothing.",
main_rs.display()
);
}
if gate && discarded > 0 {
anyhow::bail!("{discarded} value(s) computed and never read -- see DISCARDED above");
}
Ok(())
}

fn tests_gate(gate: bool) -> Result<()> {
let root = repo_root()?;
let mut files: Vec<std::path::PathBuf> = Vec::new();
Expand Down Expand Up @@ -2996,6 +3161,7 @@ pub fn run(cmd: &GatesCmd) -> Result<()> {
),
GatesCmd::Prs { repo } => prs(repo.as_deref()),
GatesCmd::Tests { gate } => tests_gate(*gate),
GatesCmd::Warnings { gate } => warnings_gate(*gate),
GatesCmd::Unmeasured { repos, stale_days } => {
let list: Vec<String> = if repos.is_empty() {
vec![current_repo()?]
Expand Down Expand Up @@ -4271,6 +4437,35 @@ mod tests {
mod sweep_tests {
use super::*;

#[test]
fn the_t106_warning_shape_is_its_own_class() {
// Both of these are what rustc printed for `signature` in types_dup.rs
// while the summary undercounted 346 rows as 345.
use super::{classify_warning, WarnClass};
assert_eq!(
classify_warning("value assigned to `signature` is never read"),
WarnClass::Discarded
);
assert_eq!(
classify_warning("variable `signature` is assigned to, but never used"),
WarnClass::Discarded
);
// "never read" also appears in dead-field warnings, which are a
// different problem: nothing computed them in the first place.
assert_eq!(
classify_warning("field `start_ts` is never read"),
WarnClass::Dead
);
assert_eq!(
classify_warning("function `plan` is never used"),
WarnClass::Dead
);
assert_eq!(
classify_warning("unused variable: `t0`"),
WarnClass::Cosmetic
);
}

#[test]
fn mutation_never_touches_the_control_itself() {
// The mistake this guards against was made by hand first: one regex
Expand Down
Loading
Loading