diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a22aa0..20c46af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,52 @@ 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. 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 + 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 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 Patch release: **test-suite effectiveness, proven by mutation testing** (Phases diff --git a/Cargo.lock b/Cargo.lock index 5e8fb69..39f866a 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 f7c6d7e..63a172f 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/btc.rs b/src/adapters/analyzers/structural/btc.rs index 7730b6d..5fa6fd9 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 50bd07e..6c2f075 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 897da9f..38e2172 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 53805e3..f1d3686 100644 --- a/src/adapters/analyzers/structural/mod.rs +++ b/src/adapters/analyzers/structural/mod.rs @@ -89,8 +89,15 @@ 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 (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, /// (type_name, impl_file, impl_block_line) for inherent impls pub inherent_impls: Vec<(String, String, usize)>, } @@ -117,6 +124,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 +170,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 +211,72 @@ 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); + syntax + .items + .iter() + .for_each(|item| count_cfg_test_trait_impls(item, in_test, &mut counts)); + }); + counts +} + +/// 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) }; + match item { + syn::Item::Impl(imp) if in_test || cfg_test(&imp.attrs) => { + if let Some((_, tp, _)) = &imp.trait_ { + 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); + 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. /// Operation: iterates items, dispatches to callback via closure. pub(crate) fn visit_inherent_methods( @@ -228,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/sit.rs b/src/adapters/analyzers/structural/sit.rs index 04f8fb8..2288ab1 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/btc.rs b/src/adapters/analyzers/structural/tests/btc.rs index e9c6da0..4076d47 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 2249bcd..141d7ff 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 b330c52..76c54f7 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 690b0fd..e707d4c 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/oi.rs b/src/adapters/analyzers/structural/tests/oi.rs index 5f40067..edf1be1 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 bcc850e..78b7c9e 100644 --- a/src/adapters/analyzers/structural/tests/sit.rs +++ b/src/adapters/analyzers/structural/tests/sit.rs @@ -78,6 +78,187 @@ 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 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 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 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 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 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() + ); +} + #[test] fn test_disabled_check() { let syntax = diff --git a/src/adapters/analyzers/structural/tests/slm.rs b/src/adapters/analyzers/structural/tests/slm.rs index d31e4e9..c25800d 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 } }");