From cc9f69d9cf5ed1079e06c5ab4e5829326cc1bc0d Mon Sep 17 00:00:00 2001 From: Sascha <18143567+SaschaOnTour@users.noreply.github.com> Date: Thu, 4 Jun 2026 17:48:30 +0200 Subject: [PATCH 1/2] fix(structural): count #[cfg(test)] impls in SIT so test-seam traits aren't flagged (v1.4.2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SIT (single-impl trait) counted only production impls — the structural metadata walk skips test code — so a non-pub trait with one production impl plus #[cfg(test)] test-double impls (the idiomatic DI / test-seam pattern) was wrongly flagged as an over-abstraction. It now counts #[cfg(test)] trait impls toward the trait's total implementor count and fires only when that total is 1. A genuine single-impl trait (one prod impl, no doubles) is still flagged. "Test" covers a whole #![cfg(test)] / integration-test file, an inline #[cfg(test)] mod, and an item-level #[cfg(test)] on the impl (collect_item_metadata no longer miscounts the latter as a production impl). Counting is name-based (the metadata models no trait identity), so it is scoped per module: a test impl whose trait is a bare name defined as a test-only trait in the same module is attributed to that local trait, not a same-named production trait — so an unrelated test-only `trait Clock` does not suppress the production `Clock`, while a real double in another test module still counts. Known residual: a test impl of an imported/external trait sharing a production trait's last-segment name still collides (symmetric with the pre-existing production-side name collision; full resolution needs real trait identity). Sibling detector OI is unaffected — it matches impl locations, it does not count (pinned). Regressed when v1.4.x improved #[cfg(test)] mod foo;-chain test-file recognition, which correctly excluded companion test files and shrank SIT's denominator. Four gates green: cargo fmt, 1860 nextest, self-analysis 0 findings, clippy. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 30 ++++ Cargo.lock | 2 +- Cargo.toml | 2 +- src/adapters/analyzers/structural/mod.rs | 81 ++++++++- src/adapters/analyzers/structural/sit.rs | 18 +- src/adapters/analyzers/structural/tests/oi.rs | 34 ++++ .../analyzers/structural/tests/sit.rs | 154 ++++++++++++++++++ 7 files changed, 315 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a22aa01..fa93dadd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,36 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [1.4.2] - 2026-06-04 + +Patch release: a false-positive fix found while applying 1.4.1. No change to +how production code is scored beyond removing this incorrect finding. + +### Fixed +- **SIT (single-impl trait) no longer flags traits with `#[cfg(test)]` test + doubles.** SIT counted only production impls (metadata collection skips test + code), so a non-pub trait with one production impl plus test-double impls — + the idiomatic dependency-injection / test-seam pattern — was wrongly reported + as an over-abstraction once the test impls became invisible. `#[cfg(test)]` + impls are now counted toward the trait's total implementor count — whether + they live in a whole test file, an inline `#[cfg(test)] mod`, or carry + `#[cfg(test)]` directly on the impl item — and SIT fires only when that total + is `1`. A genuine single-impl trait (one prod impl, no doubles) is still + flagged. Counting is name-based (the structural metadata models no trait + identity, on either the production or test side), so it is scoped per module: + a test impl whose trait is a bare name defined as a test-only trait in the + *same* module is attributed to that local trait, not a same-named production + trait — so an unrelated test-only `trait Clock` does not suppress the + production `Clock`, while a real double in another test module still counts. + Known residual limitation: a test impl of an *imported/external* trait that + merely shares a production trait's last-segment name still collides (a name + collision symmetric with the pre-existing production-side one; resolving it + needs real trait identity). Regressed when v1.4.x improved + `#[cfg(test)] mod foo;`-chain test-file recognition (the better classification + correctly excluded the companion test files, shrinking SIT's denominator). The + sibling detector OI is unaffected — it matches impl locations, it does not + count. + ## [1.4.1] - 2026-06-02 Patch release: **test-suite effectiveness, proven by mutation testing** (Phases diff --git a/Cargo.lock b/Cargo.lock index 5e8fb695..39f866a7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -704,7 +704,7 @@ dependencies = [ [[package]] name = "rustqual" -version = "1.4.1" +version = "1.4.2" dependencies = [ "clap", "clap_complete", diff --git a/Cargo.toml b/Cargo.toml index f7c6d7ea..63a172f7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "rustqual" -version = "1.4.1" +version = "1.4.2" edition = "2021" description = "Comprehensive Rust code quality analyzer — seven dimensions: IOSP, Complexity, DRY, SRP, Coupling, Test Quality, Architecture" license = "MIT" diff --git a/src/adapters/analyzers/structural/mod.rs b/src/adapters/analyzers/structural/mod.rs index 53805e3f..f1b32f81 100644 --- a/src/adapters/analyzers/structural/mod.rs +++ b/src/adapters/analyzers/structural/mod.rs @@ -89,8 +89,14 @@ pub(crate) struct StructuralMetadata { pub type_defs: HashMap, /// trait_name → TraitInfo pub trait_defs: HashMap, - /// trait_name → list of (impl_type, file) + /// trait_name → list of (impl_type, file) — production impls only pub trait_impls: HashMap>, + /// trait_name → number of impls living in `#[cfg(test)]` code (whole test + /// files or inline `#[cfg(test)] mod` blocks). Not in `trait_impls` + /// (metadata skips test code), but they count toward a trait's TOTAL + /// implementor count so SIT does not flag a trait whose only extra + /// implementers are test doubles — the idiomatic DI / test-seam pattern. + pub cfg_test_trait_impl_counts: HashMap, /// (type_name, impl_file, impl_block_line) for inherent impls pub inherent_impls: Vec<(String, String, usize)>, } @@ -117,6 +123,7 @@ pub(crate) fn collect_metadata( type_defs: HashMap::new(), trait_defs: HashMap::new(), trait_impls: HashMap::new(), + cfg_test_trait_impl_counts: collect_cfg_test_trait_impl_counts(parsed, cfg_test_files), inherent_impls: Vec::new(), }; parsed.iter().for_each(|(path, _, syntax)| { @@ -162,7 +169,7 @@ fn collect_item_metadata(item: &syn::Item, path: &str, meta: &mut StructuralMeta }, ); } - syn::Item::Impl(imp) => { + syn::Item::Impl(imp) if !cfg_test(&imp.attrs) => { if let Some(ref type_name) = impl_type_name(imp) { let line = imp.span().start().line; if let Some((_, ref tp, _)) = imp.trait_ { @@ -203,6 +210,76 @@ fn extract_impl_type_name(imp: &syn::ItemImpl) -> Option { use crate::adapters::shared::cfg_test::has_cfg_test as has_cfg_test_attr; +/// Count trait impls living in `#[cfg(test)]` code, keyed by trait name. The +/// main metadata walk skips test code, so these production-invisible impls +/// would otherwise vanish from a trait's implementor count — making a test +/// seam (one prod impl + test doubles) look like a single-impl over-abstraction +/// to SIT. "Test" means a whole `#![cfg(test)]` / integration-test file, an +/// inline `#[cfg(test)] mod`, or an item-level `#[cfg(test)]` on the impl. +/// Operation: iterates files, delegates per-scope collection via a closure. +fn collect_cfg_test_trait_impl_counts( + parsed: &[(String, String, syn::File)], + cfg_test_files: &HashSet, +) -> HashMap { + let mut counts = HashMap::new(); + parsed.iter().for_each(|(path, _, syntax)| { + let in_test = cfg_test_files.contains(path); + collect_test_impls_in_scope(&syntax.items, in_test, &mut counts); + }); + counts +} + +/// Tally `#[cfg(test)]` trait impls in one item scope (file or module body), +/// keyed by trait name, recursing into submodules. A test impl whose trait is a +/// **single bare segment** naming a trait DEFINED in test code in the *same* +/// scope is attributed to that local test-only trait — not a same-named +/// production trait — so it is not counted; this keeps an unrelated test-only +/// `trait Clock` from suppressing the production `Clock`, while a real double in +/// a *different* test module (or via a qualified `super::Clock` path) still +/// counts. NOTE: still name-based — a test impl of an *imported/external* trait +/// that merely shares a production trait's last-segment name collides; fully +/// resolving that needs real trait identity, which the name-keyed metadata +/// models on neither the production nor the test side. +/// Operation: builds the local test-trait set, then tallies / recurses — own +/// calls hidden in closures for IOSP. +fn collect_test_impls_in_scope( + items: &[syn::Item], + in_test: bool, + counts: &mut HashMap, +) { + let cfg_test = |attrs: &[syn::Attribute]| -> bool { has_cfg_test_attr(attrs) }; + let local_test_traits: HashSet = items + .iter() + .filter_map(|it| match it { + syn::Item::Trait(t) if in_test || cfg_test(&t.attrs) => Some(t.ident.to_string()), + _ => None, + }) + .collect(); + items.iter().for_each(|item| match item { + syn::Item::Impl(imp) if in_test || cfg_test(&imp.attrs) => { + if let Some((_, tp, _)) = &imp.trait_ { + let bare_local = tp.segments.len() == 1 + && tp + .segments + .last() + .is_some_and(|s| local_test_traits.contains(&s.ident.to_string())); + if !bare_local { + if let Some(name) = tp.segments.last().map(|s| s.ident.to_string()) { + *counts.entry(name).or_insert(0) += 1; + } + } + } + } + syn::Item::Mod(m) => { + let mod_in_test = in_test || cfg_test(&m.attrs); + if let Some((_, sub_items)) = &m.content { + collect_test_impls_in_scope(sub_items, mod_in_test, counts); + } + } + _ => {} + }); +} + /// Visit all inherent (non-trait) impl methods in parsed files, excluding test modules. /// Operation: iterates items, dispatches to callback via closure. pub(crate) fn visit_inherent_methods( diff --git a/src/adapters/analyzers/structural/sit.rs b/src/adapters/analyzers/structural/sit.rs index 04f8fb89..2288ab14 100644 --- a/src/adapters/analyzers/structural/sit.rs +++ b/src/adapters/analyzers/structural/sit.rs @@ -3,8 +3,10 @@ use crate::findings::Dimension; use super::{StructuralMetadata, StructuralWarning, StructuralWarningKind}; -/// Detect single-implementation traits: non-pub trait with exactly 1 impl. -/// Operation: compares trait definitions against impl counts. +/// Detect single-implementation traits: non-pub trait with exactly 1 impl +/// total, counting `#[cfg(test)]` impls (a test double is a real implementor, +/// so a trait with prod impl + test doubles is a DI seam, not over-abstraction). +/// Operation: compares trait definitions against total impl counts. pub(crate) fn detect_sit( warnings: &mut Vec, meta: &StructuralMetadata, @@ -32,6 +34,18 @@ pub(crate) fn detect_sit( let [(impl_type, _)] = impls.as_slice() else { return; }; + // A `#[cfg(test)]` impl is a real implementor too: one prod impl plus any + // test doubles is the idiomatic DI / test-seam pattern, not an + // over-abstraction. Only fire when the TOTAL implementor count is 1. + if meta + .cfg_test_trait_impl_counts + .get(trait_name) + .copied() + .unwrap_or(0) + != 0 + { + return; + } warnings.push(StructuralWarning { file: info.file.clone(), line: info.line, diff --git a/src/adapters/analyzers/structural/tests/oi.rs b/src/adapters/analyzers/structural/tests/oi.rs index 5f400672..edf1be1a 100644 --- a/src/adapters/analyzers/structural/tests/oi.rs +++ b/src/adapters/analyzers/structural/tests/oi.rs @@ -27,6 +27,40 @@ fn orphaned_impl_across_cfg_test_files_excluded() { ); } +#[test] +fn trait_seam_with_cfg_test_impl_does_not_trip_oi() { + // Sibling check to the SIT fix: OI is location-based (inherent impl in a + // different top-level module than its type def), it does NOT count trait + // impls, so the DI/test-seam pattern (trait + one prod impl + a cfg(test) + // double) has no analog of the SIT denominator-shrink bug. Pin that the + // exact seam shape produces no OrphanedImpl, whether the double is inline… + let inline = detect_multi(&[( + "lib.rs", + "trait Clock { fn now(&self) -> u64; } \ + struct SystemClock; impl Clock for SystemClock { fn now(&self) -> u64 { 0 } } \ + #[cfg(test)] mod tests { use super::*; \ + struct FixedClock(u64); impl Clock for FixedClock { fn now(&self) -> u64 { self.0 } } }", + )]); + assert!(inline.is_empty(), "seam pattern must not trip OI (inline)"); + // …or in a separate #![cfg(test)] companion file. + let split = detect_multi(&[ + ( + "lib.rs", + "trait Clock { fn now(&self) -> u64; } \ + struct SystemClock; impl Clock for SystemClock { fn now(&self) -> u64 { 0 } }", + ), + ( + "clock_tests.rs", + "#![cfg(test)]\nstruct FixedClock(u64); \ + impl Clock for FixedClock { fn now(&self) -> u64 { self.0 } }", + ), + ]); + assert!( + split.is_empty(), + "seam pattern must not trip OI (split file)" + ); +} + #[test] fn test_same_file_not_flagged() { let w = detect_multi(&[("lib.rs", "struct Foo {} impl Foo { fn bar() {} }")]); diff --git a/src/adapters/analyzers/structural/tests/sit.rs b/src/adapters/analyzers/structural/tests/sit.rs index bcc850e2..c6e2a67f 100644 --- a/src/adapters/analyzers/structural/tests/sit.rs +++ b/src/adapters/analyzers/structural/tests/sit.rs @@ -78,6 +78,160 @@ fn single_impl_in_non_test_module_collected() { ); } +#[test] +fn single_prod_impl_with_inline_cfg_test_impl_is_a_seam_not_sit() { + // A non-pub trait with ONE production impl plus a `#[cfg(test)]` test-double + // impl is the idiomatic DI / test-seam pattern, not an over-abstraction: the + // trait genuinely has multiple implementers and is used polymorphically. + // SIT must count the cfg(test) impl toward the total and not fire. + let w = detect_from( + "trait Clock { fn now(&self) -> u64; } \ + struct SystemClock; impl Clock for SystemClock { fn now(&self) -> u64 { 0 } } \ + #[cfg(test)] mod tests { use super::*; \ + struct FixedClock(u64); impl Clock for FixedClock { fn now(&self) -> u64 { self.0 } } }", + ); + assert!( + w.is_empty(), + "trait with a #[cfg(test)] impl is a test seam, not single-impl: {} warning(s)", + w.len() + ); +} + +#[test] +fn single_prod_impl_with_cfg_test_impl_in_separate_file_not_flagged() { + // The real-world shape: the trait + its production impl live in lib.rs, the + // test-double impl in a separate `#![cfg(test)]` companion file (so the file + // is whole-file test-classified and skipped by metadata collection). The + // cfg(test) impl must still be counted toward the trait's total. + let w = super::detect_meta( + &super::parse_multi(&[ + ( + "lib.rs", + "trait Clock { fn now(&self) -> u64; } \ + struct SystemClock; impl Clock for SystemClock { fn now(&self) -> u64 { 0 } }", + ), + ( + "clock_tests.rs", + "#![cfg(test)]\nstruct FixedClock(u64); \ + impl Clock for FixedClock { fn now(&self) -> u64 { self.0 } }", + ), + ]), + detect_sit, + ); + assert!( + w.is_empty(), + "cfg(test) impl in a separate test file must count toward the total: {} warning(s)", + w.len() + ); +} + +#[test] +fn single_prod_impl_with_two_cfg_test_impls_not_flagged() { + // Mirrors the reported crate exactly: 1 production impl + 2 cfg(test) impls. + // Multiple test doubles change nothing — total impls > 1, so no SIT. + let w = detect_from( + "trait Clock { fn now(&self) -> u64; } \ + struct SystemClock; impl Clock for SystemClock { fn now(&self) -> u64 { 0 } } \ + #[cfg(test)] mod a { use super::*; struct A; impl Clock for A { fn now(&self) -> u64 { 1 } } } \ + #[cfg(test)] mod b { use super::*; struct B; impl Clock for B { fn now(&self) -> u64 { 2 } } }", + ); + assert!( + w.is_empty(), + "1 prod + 2 test impls is not a single-impl trait" + ); +} + +#[test] +fn single_prod_impl_zero_test_impls_still_flagged() { + // Guard the fix against over-broadening: a genuine single-impl trait (one + // production impl, no test doubles anywhere) must STILL be flagged — that is + // the real over-abstraction SIT exists to catch. + let w = detect_from( + "trait Clock { fn now(&self) -> u64; } \ + struct SystemClock; impl Clock for SystemClock { fn now(&self) -> u64 { 0 } }", + ); + assert_eq!(w.len(), 1, "1 prod + 0 test impls is still SIT"); + assert_eq!(w[0].name, "Clock"); +} + +#[test] +fn cfg_test_attr_impl_is_not_a_production_impl() { + // A `#[cfg(test)]` attribute directly on the impl ITEM (not inside a + // `#[cfg(test)] mod`, not in a test file) makes it a test impl. A trait + // whose only implementor is such an impl has ZERO production impls and must + // not be flagged — previously the item-level attr was ignored and the impl + // miscounted as a single production impl. + let w = detect_from( + "trait Clock { fn now(&self) -> u64; } \ + struct Fake; #[cfg(test)] impl Clock for Fake { fn now(&self) -> u64 { 0 } }", + ); + assert!( + w.is_empty(), + "a #[cfg(test)] impl item is a test impl, not a single production impl: {} warning(s)", + w.len() + ); +} + +#[test] +fn single_prod_impl_with_cfg_test_attr_double_not_flagged() { + // The DI seam expressed via an item-level `#[cfg(test)]` double: one + // production impl plus a test double attributed directly on the impl. Total + // implementors 2 → no SIT. + let w = detect_from( + "trait Clock { fn now(&self) -> u64; } \ + struct SystemClock; impl Clock for SystemClock { fn now(&self) -> u64 { 0 } } \ + struct FixedClock; #[cfg(test)] impl Clock for FixedClock { fn now(&self) -> u64 { 1 } }", + ); + assert!( + w.is_empty(), + "1 prod + 1 #[cfg(test)] item-attr double is a seam, not SIT" + ); +} + +#[test] +fn unrelated_test_only_trait_with_same_name_does_not_suppress_sit() { + // A genuine single-impl production trait must STILL be flagged even when an + // unrelated test-only module defines its OWN trait of the same name and + // implements it. Test-impl counting keys on the trait name, so a name that + // is itself defined in test code is ambiguous — its test impls must not + // suppress the same-named production trait. + let w = detect_from( + "trait Clock { fn now(&self) -> u64; } \ + struct SystemClock; impl Clock for SystemClock { fn now(&self) -> u64 { 0 } } \ + #[cfg(test)] mod tests { \ + trait Clock { fn tick(&self); } struct TestClock; impl Clock for TestClock { fn tick(&self) {} } }", + ); + assert_eq!( + w.len(), + 1, + "an unrelated test-only trait of the same name must not suppress the production trait's SIT" + ); + assert_eq!(w[0].name, "Clock"); +} + +#[test] +fn legit_double_not_pruned_by_unrelated_same_named_test_trait() { + // Mixed collision: a production trait WITH a legitimate test double in one + // test module, AND an unrelated test-only trait of the same name in another + // test module. The real double must still count — earlier global + // name-pruning dropped every "Clock" count (including the real double), + // wrongly re-flagging the production trait. Module-scoped exclusion must + // only drop the impl that belongs to the local test-only trait. + let w = detect_from( + "trait Clock { fn now(&self) -> u64; } \ + struct SystemClock; impl Clock for SystemClock { fn now(&self) -> u64 { 0 } } \ + #[cfg(test)] mod doubles { use super::*; \ + struct FixedClock; impl Clock for FixedClock { fn now(&self) -> u64 { 1 } } } \ + #[cfg(test)] mod local { \ + trait Clock { fn tick(&self); } struct T; impl Clock for T { fn tick(&self) {} } }", + ); + assert!( + w.is_empty(), + "a real test double must still count even when an unrelated test-only trait shares the name: {} warning(s)", + w.len() + ); +} + #[test] fn test_disabled_check() { let syntax = From 91044d5250457618c0af2de33783a2487727b94d Mon Sep 17 00:00:00 2001 From: Sascha <18143567+SaschaOnTour@users.noreply.github.com> Date: Thu, 4 Jun 2026 18:56:55 +0200 Subject: [PATCH 2/2] fix(structural): honour #[cfg(test)] at every level across detectors; SIT counts impls purely by name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SIT now counts #[cfg(test)] trait impls toward a trait's total implementor count (so a test-seam trait — one prod impl plus test doubles — is not flagged), using deliberately name-based counting: it never drops a real double (no false positive on a legitimate seam), accepting a safe, documented false negative on a rare name collision with an unrelated test-only trait. Earlier scope/path heuristics were removed because resolving self::/super::/glob/absolute paths is trait identity, which the name-keyed metadata models on neither side. Making SIT count item-level #[cfg(test)] impls surfaced a broader inconsistency: the other structural detectors treated #[cfg(test)] code in a production file as production. They now honour #[cfg(test)] at every level that can occur in compiling code: - item-level #[cfg(test)] impl: BTC, SLM, NMS, DEH, IET (+ SIT/OI via metadata) - #[cfg(test)] on an impl method: BTC, SLM, NMS, DEH - #[cfg(test)] pub fn: IET, DEH consistent with whole test files and #[cfg(test)] mod blocks. Four gates green: cargo fmt, 1870 nextest, self-analysis 0 findings, clippy. --- CHANGELOG.md | 40 ++++++--- src/adapters/analyzers/structural/btc.rs | 6 +- src/adapters/analyzers/structural/deh.rs | 18 ++++ src/adapters/analyzers/structural/iet.rs | 2 +- src/adapters/analyzers/structural/mod.rs | 83 +++++++++---------- .../analyzers/structural/tests/btc.rs | 29 +++++++ .../analyzers/structural/tests/deh.rs | 29 +++++++ .../analyzers/structural/tests/iet.rs | 15 ++++ .../analyzers/structural/tests/nms.rs | 28 +++++++ .../analyzers/structural/tests/sit.rs | 77 +++++++++++------ .../analyzers/structural/tests/slm.rs | 24 ++++++ 11 files changed, 269 insertions(+), 82 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fa93dadd..20c46afb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [1.4.2] - 2026-06-04 -Patch release: a false-positive fix found while applying 1.4.1. No change to -how production code is scored beyond removing this incorrect finding. +Patch release: a false-positive fix found while applying 1.4.1. SIT stops +flagging traits that have `#[cfg(test)]` test doubles. As a deliberate, +documented trade-off it now counts test impls purely by name, so a rare name +collision with an unrelated test-only trait can instead make SIT *under-report* +a genuine single-impl production trait — the safe direction (a missed finding, +never a wrong one). Details and rationale below. ### Fixed - **SIT (single-impl trait) no longer flags traits with `#[cfg(test)]` test @@ -20,20 +24,32 @@ how production code is scored beyond removing this incorrect finding. they live in a whole test file, an inline `#[cfg(test)] mod`, or carry `#[cfg(test)]` directly on the impl item — and SIT fires only when that total is `1`. A genuine single-impl trait (one prod impl, no doubles) is still - flagged. Counting is name-based (the structural metadata models no trait - identity, on either the production or test side), so it is scoped per module: - a test impl whose trait is a bare name defined as a test-only trait in the - *same* module is attributed to that local trait, not a same-named production - trait — so an unrelated test-only `trait Clock` does not suppress the - production `Clock`, while a real double in another test module still counts. - Known residual limitation: a test impl of an *imported/external* trait that - merely shares a production trait's last-segment name still collides (a name - collision symmetric with the pre-existing production-side one; resolving it - needs real trait identity). Regressed when v1.4.x improved + flagged. Counting is deliberately *purely name-based* (the structural metadata + models no trait identity, on either the production or test side): it never + tries to resolve which trait a same-named impl targets, which guarantees a real + test double is never dropped — so a legitimate seam is never re-flagged (no + false positive, the original bug class). The accepted, documented trade-off is + a safe false *negative*: an unrelated test-only trait that happens to share a + production trait's last-segment name is counted toward it and can make SIT + under-report that production trait. Resolving that needs real trait identity + (the production side has the same name-collision limitation). Regressed when + v1.4.x improved `#[cfg(test)] mod foo;`-chain test-file recognition (the better classification correctly excluded the companion test files, shrinking SIT's denominator). The sibling detector OI is unaffected — it matches impl locations, it does not count. +- **`#[cfg(test)]` on impls, methods and fns is now honoured by all structural + detectors.** Teaching SIT to count item-level `#[cfg(test)]` impls surfaced an + inconsistency: BTC (broken trait contract), SLM (selfless method), NMS + (needless `&mut self`), DEH (downcast escape hatch) and IET (inconsistent + error types) still treated a `#[cfg(test)] impl …`, a `#[cfg(test)]` method + inside a normal impl, or a `#[cfg(test)] pub fn` sitting in a *production* file + as production code — so e.g. `#[cfg(test)] impl Foo for Mock { fn m(&self) { + todo!() } }`, or `impl S { #[cfg(test)] fn helper(&self) -> i32 { 42 } }`, + could still trip BTC / SLM. The detectors now skip `#[cfg(test)]` at every + level (impl block, impl method, and free fn), consistent with whole test files + and `#[cfg(test)] mod` blocks (OI/SIT already do, via the shared metadata + walk). ## [1.4.1] - 2026-06-02 diff --git a/src/adapters/analyzers/structural/btc.rs b/src/adapters/analyzers/structural/btc.rs index 7730b6d6..5fa6fd97 100644 --- a/src/adapters/analyzers/structural/btc.rs +++ b/src/adapters/analyzers/structural/btc.rs @@ -35,7 +35,9 @@ fn check_item(item: &syn::Item, path: &str, warnings: &mut Vec impl_check(imp, path, warnings), + syn::Item::Impl(imp) if !super::has_cfg_test_attr(&imp.attrs) => { + impl_check(imp, path, warnings) + } syn::Item::Mod(m) if !super::has_cfg_test_attr(&m.attrs) => { m.content.iter().for_each(|(_, items)| { items.iter().for_each(|i| check_item(i, path, warnings)); @@ -63,7 +65,7 @@ fn check_impl(imp: &syn::ItemImpl, path: &str, warnings: &mut Vec Some(f), + syn::ImplItem::Fn(f) if !super::has_cfg_test_attr(&f.attrs) => Some(f), _ => None, }) .collect(); diff --git a/src/adapters/analyzers/structural/deh.rs b/src/adapters/analyzers/structural/deh.rs index 50bd07eb..6c2f0756 100644 --- a/src/adapters/analyzers/structural/deh.rs +++ b/src/adapters/analyzers/structural/deh.rs @@ -66,6 +66,24 @@ impl<'ast, 'a> Visit<'ast> for DowncastVisitor<'a> { self.in_test = was_in_test; } + fn visit_item_impl(&mut self, node: &'ast syn::ItemImpl) { + let was_in_test = self.in_test; + if has_cfg_test_attr(&node.attrs) { + self.in_test = true; + } + syn::visit::visit_item_impl(self, node); + self.in_test = was_in_test; + } + + fn visit_impl_item_fn(&mut self, node: &'ast syn::ImplItemFn) { + let was_in_test = self.in_test; + if has_cfg_test_attr(&node.attrs) { + self.in_test = true; + } + syn::visit::visit_impl_item_fn(self, node); + self.in_test = was_in_test; + } + fn visit_expr_method_call(&mut self, node: &'ast syn::ExprMethodCall) { if !self.in_test { let method_name = node.method.to_string(); diff --git a/src/adapters/analyzers/structural/iet.rs b/src/adapters/analyzers/structural/iet.rs index 897da9ff..38e21728 100644 --- a/src/adapters/analyzers/structural/iet.rs +++ b/src/adapters/analyzers/structural/iet.rs @@ -53,7 +53,7 @@ fn collect_pub_error_types(file: &syn::File) -> HashSet { fn collect_item_errors(item: &syn::Item, error_types: &mut HashSet) { let extract_error = |output: &syn::ReturnType| extract_result_error_type(output); match item { - syn::Item::Fn(f) => { + syn::Item::Fn(f) if !super::has_cfg_test_attr(&f.attrs) => { if matches!(f.vis, syn::Visibility::Public(_)) { extract_error(&f.sig.output).iter().for_each(|e| { error_types.insert(e.clone()); diff --git a/src/adapters/analyzers/structural/mod.rs b/src/adapters/analyzers/structural/mod.rs index f1b32f81..f1d3686e 100644 --- a/src/adapters/analyzers/structural/mod.rs +++ b/src/adapters/analyzers/structural/mod.rs @@ -91,9 +91,10 @@ pub(crate) struct StructuralMetadata { pub trait_defs: HashMap, /// trait_name → list of (impl_type, file) — production impls only pub trait_impls: HashMap>, - /// trait_name → number of impls living in `#[cfg(test)]` code (whole test - /// files or inline `#[cfg(test)] mod` blocks). Not in `trait_impls` - /// (metadata skips test code), but they count toward a trait's TOTAL + /// trait_name → number of impls living in `#[cfg(test)]` code (a whole test + /// file, an inline `#[cfg(test)] mod` block, or an item-level `#[cfg(test)]` + /// on the impl). Not in `trait_impls` (metadata skips test code), but they + /// count toward a trait's TOTAL /// implementor count so SIT does not flag a trait whose only extra /// implementers are test doubles — the idiomatic DI / test-seam pattern. pub cfg_test_trait_impl_counts: HashMap, @@ -224,60 +225,56 @@ fn collect_cfg_test_trait_impl_counts( let mut counts = HashMap::new(); parsed.iter().for_each(|(path, _, syntax)| { let in_test = cfg_test_files.contains(path); - collect_test_impls_in_scope(&syntax.items, in_test, &mut counts); + syntax + .items + .iter() + .for_each(|item| count_cfg_test_trait_impls(item, in_test, &mut counts)); }); counts } -/// Tally `#[cfg(test)]` trait impls in one item scope (file or module body), -/// keyed by trait name, recursing into submodules. A test impl whose trait is a -/// **single bare segment** naming a trait DEFINED in test code in the *same* -/// scope is attributed to that local test-only trait — not a same-named -/// production trait — so it is not counted; this keeps an unrelated test-only -/// `trait Clock` from suppressing the production `Clock`, while a real double in -/// a *different* test module (or via a qualified `super::Clock` path) still -/// counts. NOTE: still name-based — a test impl of an *imported/external* trait -/// that merely shares a production trait's last-segment name collides; fully -/// resolving that needs real trait identity, which the name-keyed metadata -/// models on neither the production nor the test side. -/// Operation: builds the local test-trait set, then tallies / recurses — own -/// calls hidden in closures for IOSP. -fn collect_test_impls_in_scope( - items: &[syn::Item], +/// Recurse one item, counting `#[cfg(test)]` trait impls by last-path-segment +/// trait name, descending into submodules. "In test" = a whole test file, an +/// inline `#[cfg(test)] mod`, or an item-level `#[cfg(test)]` on the impl. +/// +/// Counting is deliberately PURELY name-based: it makes no attempt to resolve +/// which trait a same-named impl actually targets. That guarantees it never +/// drops a real test double — so a legitimate DI seam is never re-flagged (no +/// false positive, the original bug class). The accepted cost is a safe, +/// documented false NEGATIVE: an unrelated test-only trait that happens to share +/// a production trait's last-segment name is counted toward it and can make SIT +/// under-report that production trait. (Earlier attempts to disambiguate by +/// lexical scope / path form re-introduced false positives — e.g. a nested +/// `super::Clock` double dropped because the nested module defined its own +/// `Clock` — because correctly resolving `self::`/`super::`/glob/re-export/ +/// absolute paths IS trait identity, which the name-keyed metadata models on +/// neither the production nor the test side. The production side has the same +/// name-collision limitation.) +/// Operation: match dispatch; own calls hidden in closures for IOSP. +fn count_cfg_test_trait_impls( + item: &syn::Item, in_test: bool, counts: &mut HashMap, ) { let cfg_test = |attrs: &[syn::Attribute]| -> bool { has_cfg_test_attr(attrs) }; - let local_test_traits: HashSet = items - .iter() - .filter_map(|it| match it { - syn::Item::Trait(t) if in_test || cfg_test(&t.attrs) => Some(t.ident.to_string()), - _ => None, - }) - .collect(); - items.iter().for_each(|item| match item { + match item { syn::Item::Impl(imp) if in_test || cfg_test(&imp.attrs) => { if let Some((_, tp, _)) = &imp.trait_ { - let bare_local = tp.segments.len() == 1 - && tp - .segments - .last() - .is_some_and(|s| local_test_traits.contains(&s.ident.to_string())); - if !bare_local { - if let Some(name) = tp.segments.last().map(|s| s.ident.to_string()) { - *counts.entry(name).or_insert(0) += 1; - } + if let Some(name) = tp.segments.last().map(|s| s.ident.to_string()) { + *counts.entry(name).or_insert(0) += 1; } } } syn::Item::Mod(m) => { let mod_in_test = in_test || cfg_test(&m.attrs); - if let Some((_, sub_items)) = &m.content { - collect_test_impls_in_scope(sub_items, mod_in_test, counts); - } + m.content.iter().for_each(|(_, items)| { + items + .iter() + .for_each(|i| count_cfg_test_trait_impls(i, mod_in_test, counts)); + }); } _ => {} - }); + } } /// Visit all inherent (non-trait) impl methods in parsed files, excluding test modules. @@ -305,13 +302,15 @@ fn visit_item_methods( callback: &mut dyn FnMut(&syn::ImplItemFn, &str), ) { match item { - syn::Item::Impl(imp) => { + syn::Item::Impl(imp) if !has_cfg_test_attr(&imp.attrs) => { if imp.trait_.is_some() { return; } imp.items.iter().for_each(|i| { if let syn::ImplItem::Fn(method) = i { - callback(method, path); + if !has_cfg_test_attr(&method.attrs) { + callback(method, path); + } } }); } diff --git a/src/adapters/analyzers/structural/tests/btc.rs b/src/adapters/analyzers/structural/tests/btc.rs index e9c6da04..4076d478 100644 --- a/src/adapters/analyzers/structural/tests/btc.rs +++ b/src/adapters/analyzers/structural/tests/btc.rs @@ -21,6 +21,35 @@ fn stub_impl_in_cfg_test_file_excluded() { ); } +#[test] +fn cfg_test_attr_impl_stub_excluded() { + // An item-level `#[cfg(test)]` trait impl is test code (a mock) even in a + // production file — its stub bodies must not trip BTC, consistent with the + // SIT metadata treating the same impl as a test impl. + let w = detect_in( + "trait Foo { fn bar(&self); } struct M; #[cfg(test)] impl Foo for M { fn bar(&self) { todo!() } }", + ); + assert!( + w.is_empty(), + "a #[cfg(test)] impl's stub must be excluded from BTC: {} warning(s)", + w.len() + ); +} + +#[test] +fn cfg_test_attr_method_stub_excluded() { + // `#[cfg(test)]` directly on a trait-impl method marks it test code; its stub + // body must not trip BTC, consistent with the item-level `#[cfg(test)] impl`. + let w = detect_in( + "trait Foo { fn bar(&self); } struct M; impl Foo for M { #[cfg(test)] fn bar(&self) { todo!() } }", + ); + assert!( + w.is_empty(), + "a #[cfg(test)] method stub must be excluded from BTC: {} warning(s)", + w.len() + ); +} + #[test] fn test_all_stub_methods_flagged() { let w = detect_in("trait Foo { fn bar(&self); } impl Foo for MyType { fn bar(&self) { todo!() } } struct MyType;"); diff --git a/src/adapters/analyzers/structural/tests/deh.rs b/src/adapters/analyzers/structural/tests/deh.rs index 2249bcdc..141d7ffa 100644 --- a/src/adapters/analyzers/structural/tests/deh.rs +++ b/src/adapters/analyzers/structural/tests/deh.rs @@ -16,6 +16,35 @@ fn test_downcast_ref_flagged() { )); } +#[test] +fn downcast_in_cfg_test_attr_impl_excluded() { + // A downcast inside a method of an item-level `#[cfg(test)]` impl is test + // code even in a production file — DEH must skip it, consistent with the + // function-level `#[test]`/`#[cfg(test)]` handling and the SIT metadata. + let w = detect_in( + "struct M; #[cfg(test)] impl M { fn foo(&self, a: &dyn std::any::Any) { a.downcast_ref::(); } }", + ); + assert!( + w.is_empty(), + "a downcast in a #[cfg(test)] impl must be excluded from DEH: {} warning(s)", + w.len() + ); +} + +#[test] +fn downcast_in_cfg_test_attr_method_excluded() { + // `#[cfg(test)]` directly on an impl METHOD makes its body test code even in + // a regular production impl — DEH must skip a downcast inside it. + let w = detect_in( + "struct M; impl M { #[cfg(test)] fn foo(&self, a: &dyn std::any::Any) { a.downcast_ref::(); } }", + ); + assert!( + w.is_empty(), + "a downcast in a #[cfg(test)] method must be excluded from DEH: {} warning(s)", + w.len() + ); +} + #[test] fn test_downcast_mut_flagged() { let w = detect_in("fn foo(a: &mut dyn std::any::Any) { a.downcast_mut::(); }"); diff --git a/src/adapters/analyzers/structural/tests/iet.rs b/src/adapters/analyzers/structural/tests/iet.rs index b330c523..76c54f75 100644 --- a/src/adapters/analyzers/structural/tests/iet.rs +++ b/src/adapters/analyzers/structural/tests/iet.rs @@ -20,6 +20,21 @@ fn inconsistent_error_types_in_cfg_test_file_excluded() { ); } +#[test] +fn cfg_test_attr_pub_fn_error_type_excluded() { + // A `#[cfg(test)]` pub fn is test code even in a production file — its error + // type must not count toward IET's inconsistent-error-type check, leaving a + // single production error type (no finding). + let w = detect_in( + "pub fn a() -> Result<(), String> { Ok(()) } #[cfg(test)] pub fn b() -> Result<(), u32> { Ok(()) }", + ); + assert!( + w.is_empty(), + "a #[cfg(test)] pub fn's error type must be excluded from IET: {} warning(s)", + w.len() + ); +} + #[test] fn test_consistent_error_types_not_flagged() { let w = detect_in( diff --git a/src/adapters/analyzers/structural/tests/nms.rs b/src/adapters/analyzers/structural/tests/nms.rs index 690b0fde..e707d4c8 100644 --- a/src/adapters/analyzers/structural/tests/nms.rs +++ b/src/adapters/analyzers/structural/tests/nms.rs @@ -19,6 +19,34 @@ fn needless_mut_self_in_cfg_test_file_excluded() { ); } +#[test] +fn cfg_test_attr_impl_method_excluded() { + // An item-level `#[cfg(test)]` impl is test code even in a production file; + // its methods must not trip NMS (consistent with whole cfg-test files). + let w = detect_in( + "struct S { x: i32 } #[cfg(test)] impl S { fn foo(&mut self) -> i32 { self.x } }", + ); + assert!( + w.is_empty(), + "a #[cfg(test)] impl method must be excluded from NMS: {} warning(s)", + w.len() + ); +} + +#[test] +fn cfg_test_attr_method_excluded() { + // `#[cfg(test)]` directly on an impl METHOD makes that method test code even + // in a regular production impl — it must not trip NMS. + let w = detect_in( + "struct S { x: i32 } impl S { #[cfg(test)] fn foo(&mut self) -> i32 { self.x } }", + ); + assert!( + w.is_empty(), + "a #[cfg(test)] method must be excluded from NMS: {} warning(s)", + w.len() + ); +} + #[test] fn test_needless_mut_self_flagged() { let w = detect_in("struct S { x: i32 } impl S { fn foo(&mut self) -> i32 { self.x } }"); diff --git a/src/adapters/analyzers/structural/tests/sit.rs b/src/adapters/analyzers/structural/tests/sit.rs index c6e2a67f..78b7c9e6 100644 --- a/src/adapters/analyzers/structural/tests/sit.rs +++ b/src/adapters/analyzers/structural/tests/sit.rs @@ -189,45 +189,72 @@ fn single_prod_impl_with_cfg_test_attr_double_not_flagged() { } #[test] -fn unrelated_test_only_trait_with_same_name_does_not_suppress_sit() { - // A genuine single-impl production trait must STILL be flagged even when an - // unrelated test-only module defines its OWN trait of the same name and - // implements it. Test-impl counting keys on the trait name, so a name that - // is itself defined in test code is ambiguous — its test impls must not - // suppress the same-named production trait. +fn real_double_counts_even_with_unrelated_same_named_test_trait() { + // The critical no-false-positive guarantee: a production trait WITH a real + // test double in one test module is NOT re-flagged even when an unrelated + // test-only trait of the same name lives in another test module. Pure + // name-based counting tallies BOTH "Clock" impls, so the trait's total + // implementor count exceeds 1 and SIT stays silent — a legitimate DI seam is + // never re-flagged. (Earlier scope/path heuristics could wrongly drop the + // real double here.) let w = detect_from( "trait Clock { fn now(&self) -> u64; } \ struct SystemClock; impl Clock for SystemClock { fn now(&self) -> u64 { 0 } } \ - #[cfg(test)] mod tests { \ - trait Clock { fn tick(&self); } struct TestClock; impl Clock for TestClock { fn tick(&self) {} } }", + #[cfg(test)] mod doubles { use super::*; \ + struct FixedClock; impl Clock for FixedClock { fn now(&self) -> u64 { 1 } } } \ + #[cfg(test)] mod local { \ + trait Clock { fn tick(&self); } struct T; impl Clock for T { fn tick(&self) {} } }", ); - assert_eq!( - w.len(), - 1, - "an unrelated test-only trait of the same name must not suppress the production trait's SIT" + assert!( + w.is_empty(), + "a real test double must still count even when an unrelated test-only trait shares the name: {} warning(s)", + w.len() ); - assert_eq!(w[0].name, "Clock"); } #[test] -fn legit_double_not_pruned_by_unrelated_same_named_test_trait() { - // Mixed collision: a production trait WITH a legitimate test double in one - // test module, AND an unrelated test-only trait of the same name in another - // test module. The real double must still count — earlier global - // name-pruning dropped every "Clock" count (including the real double), - // wrongly re-flagging the production trait. Module-scoped exclusion must - // only drop the impl that belongs to the local test-only trait. +fn real_double_via_super_not_reflagged_with_same_named_nested_test_trait() { + // No-false-positive pin for the nested-`super::` case: a nested test module + // defines its own `trait Clock` AND impls `super::super::Clock` (a real + // double of the production trait). Pure name-based counting tallies that impl + // under "Clock", so the production trait is not re-flagged. (A scope/path + // heuristic wrongly dropped this double because the nested module defined a + // same-named trait — re-introducing the very false positive SIT-with-doubles + // exists to avoid.) let w = detect_from( "trait Clock { fn now(&self) -> u64; } \ struct SystemClock; impl Clock for SystemClock { fn now(&self) -> u64 { 0 } } \ - #[cfg(test)] mod doubles { use super::*; \ - struct FixedClock; impl Clock for FixedClock { fn now(&self) -> u64 { 1 } } } \ - #[cfg(test)] mod local { \ - trait Clock { fn tick(&self); } struct T; impl Clock for T { fn tick(&self) {} } }", + #[cfg(test)] mod tests { mod inner { \ + trait Clock { fn tick(&self); } struct T; \ + impl super::super::Clock for T { fn now(&self) -> u64 { 1 } } } }", ); assert!( w.is_empty(), - "a real test double must still count even when an unrelated test-only trait shares the name: {} warning(s)", + "a real super:: double must never be dropped: {} warning(s)", + w.len() + ); +} + +#[test] +fn name_collision_with_test_only_trait_under_reports_sit() { + // KNOWN, DOCUMENTED LIMITATION (deliberately the SAFE direction): counting + // is purely name-based, so an unrelated test-only `trait Clock` whose impl + // shares the production trait's last-segment name is counted toward it, + // making SIT *under-report* a genuinely single-impl production `Clock`. This + // is the accepted trade for never producing a false positive (see the two + // tests above). Precisely distinguishing the two same-named traits would need + // real trait identity, which the name-keyed metadata models on neither side + // (the production side has the same collision). Pinned so the trade-off is + // explicit and any future change to it is deliberate. + let w = detect_from( + "trait Clock { fn now(&self) -> u64; } \ + struct SystemClock; impl Clock for SystemClock { fn now(&self) -> u64 { 0 } } \ + #[cfg(test)] mod tests { \ + trait Clock { fn tick(&self); } struct TestClock; impl Clock for TestClock { fn tick(&self) {} } }", + ); + assert!( + w.is_empty(), + "name-based counting under-reports here (documented limitation), got {} warning(s)", w.len() ); } diff --git a/src/adapters/analyzers/structural/tests/slm.rs b/src/adapters/analyzers/structural/tests/slm.rs index d31e4e9d..c25800da 100644 --- a/src/adapters/analyzers/structural/tests/slm.rs +++ b/src/adapters/analyzers/structural/tests/slm.rs @@ -17,6 +17,30 @@ fn selfless_method_in_cfg_test_file_excluded() { ); } +#[test] +fn cfg_test_attr_impl_method_excluded() { + // An item-level `#[cfg(test)]` impl is test code even in a production file; + // its methods must not trip SLM (consistent with whole cfg-test files). + let w = detect_in("struct S; #[cfg(test)] impl S { fn foo(&self) -> i32 { 42 } }"); + assert!( + w.is_empty(), + "a #[cfg(test)] impl method must be excluded from SLM: {} warning(s)", + w.len() + ); +} + +#[test] +fn cfg_test_attr_method_excluded() { + // `#[cfg(test)]` directly on an impl METHOD makes that method test code even + // in a regular production impl — it must not trip SLM. + let w = detect_in("struct S; impl S { #[cfg(test)] fn foo(&self) -> i32 { 42 } }"); + assert!( + w.is_empty(), + "a #[cfg(test)] method must be excluded from SLM: {} warning(s)", + w.len() + ); +} + #[test] fn test_selfless_method_flagged() { let w = detect_in("struct S; impl S { fn foo(&self) -> i32 { 42 } }");