From 69872d46f0abb7a00260b21891651c7a30b3e85d Mon Sep 17 00:00:00 2001 From: Sascha <18143567+SaschaOnTour@users.noreply.github.com> Date: Sat, 13 Jun 2026 02:06:40 +0200 Subject: [PATCH 1/7] feat(explain): rule-card registry as single source of truth + --explain One card per catalog rule (id, title, detects, why, fix, suppress, config) in src/domain/rule_cards/; the SARIF rules table renders from it (sync test pins set equality) and --explain now resolves a rule id case-insensitively to its card before falling back to the file-path architecture diagnostics. cli/explain tests moved into tests/ (directory form) so the production-only unwrap/expect pattern rule exempts them like every other test module. --- src/adapters/report/sarif/rules.rs | 114 +--------------- src/adapters/report/sarif/tests/rules.rs | 18 +++ src/cli/explain.rs | 51 +++++++- src/cli/explain/{tests.rs => tests/mod.rs} | 27 ++++ src/domain/mod.rs | 1 + src/domain/rule_cards/architecture.rs | 145 +++++++++++++++++++++ src/domain/rule_cards/boilerplate.rs | 144 ++++++++++++++++++++ src/domain/rule_cards/complexity.rs | 103 +++++++++++++++ src/domain/rule_cards/coupling.rs | 53 ++++++++ src/domain/rule_cards/dry.rs | 77 +++++++++++ src/domain/rule_cards/governance.rs | 40 ++++++ src/domain/rule_cards/iosp.rs | 23 ++++ src/domain/rule_cards/mod.rs | 66 ++++++++++ src/domain/rule_cards/srp.rs | 59 +++++++++ src/domain/rule_cards/structural.rs | 99 ++++++++++++++ src/domain/rule_cards/tq.rs | 81 ++++++++++++ src/domain/tests/mod.rs | 1 + src/domain/tests/rule_cards.rs | 115 ++++++++++++++++ src/lib.rs | 8 +- 19 files changed, 1107 insertions(+), 118 deletions(-) rename src/cli/explain/{tests.rs => tests/mod.rs} (57%) create mode 100644 src/domain/rule_cards/architecture.rs create mode 100644 src/domain/rule_cards/boilerplate.rs create mode 100644 src/domain/rule_cards/complexity.rs create mode 100644 src/domain/rule_cards/coupling.rs create mode 100644 src/domain/rule_cards/dry.rs create mode 100644 src/domain/rule_cards/governance.rs create mode 100644 src/domain/rule_cards/iosp.rs create mode 100644 src/domain/rule_cards/mod.rs create mode 100644 src/domain/rule_cards/srp.rs create mode 100644 src/domain/rule_cards/structural.rs create mode 100644 src/domain/rule_cards/tq.rs create mode 100644 src/domain/tests/rule_cards.rs diff --git a/src/adapters/report/sarif/rules.rs b/src/adapters/report/sarif/rules.rs index aad23a81..3239de1f 100644 --- a/src/adapters/report/sarif/rules.rs +++ b/src/adapters/report/sarif/rules.rs @@ -69,114 +69,14 @@ fn structural_rule(code: &str) -> &'static str { } } -/// Build the SARIF rules array with all known rule definitions. -/// Operation: static data construction, no own calls. +/// Build the SARIF rules array from the rule-card registry — the single +/// source of truth for catalog ids and titles (pattern + trait_contract +/// dynamic sub-kinds are added at emit time, not registered here). +/// Operation: registry map, no own calls outside the registry read. pub(super) fn sarif_rules() -> Vec { - vec![ - rule( - "iosp/violation", - "Function violates the Integration Operation Segregation Principle", - ), - rule("CX-001", "Cognitive complexity exceeds threshold"), - rule("CX-002", "Cyclomatic complexity exceeds threshold"), - rule("CX-003", "Magic number literal in non-const context"), - rule("CP-001", "Circular module dependency"), - rule("DRY-001", "Duplicate function detected"), - rule("DRY-002", "Dead code detected"), - rule("DRY-003", "Duplicate code fragment"), - rule( - "SRP-001", - "Struct may violate Single Responsibility Principle", - ), - rule("SRP-002", "Module has excessive production code length"), - rule( - "SRP-003", - "Function has too many parameters — reduce parameter count", - ), - rule("CX-004", "Function length exceeds threshold"), - rule("CX-005", "Nesting depth exceeds threshold"), - rule("CX-006", "Unsafe block detected"), - rule("A20", "Error handling issue (unwrap/expect/panic/todo)"), - rule("DRY-004", "Wildcard import (use module::*)"), - rule("CP-002", "Stable Dependencies Principle violation"), - rule("CP-003", "Module instability exceeds configured threshold"), - rule("TQ-001", "Test function has no assertions"), - rule( - "TQ-002", - "Test function does not call any production function", - ), - rule("TQ-003", "Production function is untested"), - rule("TQ-004", "Production function has no coverage"), - rule("TQ-005", "Untested logic branches (uncovered lines)"), - rule("BTC", "Broken trait contract: all methods are stubs"), - rule("SLM", "Selfless method: takes self but never references it"), - rule( - "NMS", - "Needless &mut self: takes &mut self but never mutates", - ), - rule( - "OI", - "Orphaned impl: impl block in different file than type", - ), - rule( - "SIT", - "Single-impl trait: non-pub trait with exactly one implementation", - ), - rule("DEH", "Downcast escape hatch: use of Any::downcast"), - rule("IET", "Inconsistent error types in module"), - rule("DRY-005", "Repeated match pattern across functions"), - rule("BP-001", "Trivial From implementation (derivable)"), - rule("BP-002", "Trivial Display implementation (derivable)"), - rule( - "BP-003", - "Trivial getter/setter (consider field visibility)", - ), - rule("BP-004", "Builder pattern (consider derive macro)"), - rule("BP-005", "Manual Default implementation (derivable)"), - rule("BP-006", "Repetitive match mapping"), - rule("BP-007", "Error enum boilerplate (consider thiserror)"), - rule("BP-008", "Clone-heavy conversion"), - rule("BP-009", "Struct update boilerplate"), - rule("BP-010", "Format string repetition"), - rule("SUP-001", "Suppression ratio exceeds configured maximum"), - rule( - "ORPHAN-001", - "Stale qual:allow marker: no finding in the annotation window", - ), - // Architecture dimension — hierarchical rule IDs. - // Pattern + trait_contract have dynamic sub-kinds (the user-defined - // rule's name / check string); only the base IDs registered here. - rule("architecture/layer", "Layer rule violation"), - rule( - "architecture/layer/unmatched", - "File doesn't match any configured layer", - ), - rule( - "architecture/forbidden", - "Forbidden-edge violation between layers", - ), - rule( - "architecture/pattern", - "Symbol-pattern violation (path/method/macro)", - ), - rule("architecture/trait_contract", "Trait-contract violation"), - rule( - "architecture/call_parity/no_delegation", - "Adapter pub fn does not reach the target layer", - ), - rule( - "architecture/call_parity/missing_adapter", - "Target pub fn is not reached by every adapter (or is orphaned)", - ), - rule( - "architecture/call_parity/multi_touchpoint", - "Adapter pub fn has multiple target touchpoints", - ), - rule( - "architecture/call_parity/multiplicity_mismatch", - "Target pub fn reached with divergent handler counts across adapters", - ), - ] + crate::domain::rule_cards::all_rule_cards() + .map(|c| rule(c.id, c.title)) + .collect() } /// Build a single SARIF rule entry. diff --git a/src/adapters/report/sarif/tests/rules.rs b/src/adapters/report/sarif/tests/rules.rs index 7209ec46..b3d0389c 100644 --- a/src/adapters/report/sarif/tests/rules.rs +++ b/src/adapters/report/sarif/tests/rules.rs @@ -283,3 +283,21 @@ fn srp_kind(kind: SrpFindingKind, suppressed: bool) -> SrpFinding { }, } } + +#[test] +fn sarif_rules_render_from_the_rule_card_registry() { + // Single source of truth: the SARIF rules table and the rule-card + // registry must agree exactly — a rule added to one but not the other + // would drift the catalog. + let rules = sarif_rules(); + let sarif_ids: HashSet<&str> = rules.iter().filter_map(|r| r["id"].as_str()).collect(); + let card_ids: HashSet<&str> = crate::domain::rule_cards::all_rule_cards() + .map(|c| c.id) + .collect(); + let missing_cards: Vec<&&str> = sarif_ids.difference(&card_ids).collect(); + let missing_sarif: Vec<&&str> = card_ids.difference(&sarif_ids).collect(); + assert!( + missing_cards.is_empty() && missing_sarif.is_empty(), + "registry drift — sarif-only: {missing_cards:?}, cards-only: {missing_sarif:?}" + ); +} diff --git a/src/cli/explain.rs b/src/cli/explain.rs index c377fcdf..d986979a 100644 --- a/src/cli/explain.rs +++ b/src/cli/explain.rs @@ -1,19 +1,62 @@ //! CLI-level entry points for the `--explain` flag. //! -//! Two entry points share this module: -//! - `--explain ` → `handle_explain`, which loads the file, compiles the -//! architecture config, runs the rule checks on that single file, and prints -//! the rendered report to stdout. +//! Three modes share this module, routed by `dispatch_explain`: //! - `--explain allow` → `explain_allow` / `suppression_guide`, which print the //! `// qual:allow(...)` suppression guide (grammar + per-dimension targets //! sourced from the shared vocabulary + the pin/orphan rationale). +//! - `--explain ` → `rule_card_text`, which renders the rule card +//! (detects/why/fix/suppress/config) from the domain registry, +//! case-insensitively (`bp-009` works like `BP-009`). +//! - `--explain ` → `handle_explain`, which loads the file, compiles the +//! architecture config, runs the rule checks on that single file, and prints +//! the rendered report to stdout. use crate::adapters::analyzers::architecture::compiled::compile_architecture; use crate::adapters::analyzers::architecture::explain::explain_file; use crate::config::Config; +use crate::domain::rule_cards::{find_rule_card, RuleCard}; use crate::domain::{target_kind, target_names, Dimension, TargetKind}; use std::path::Path; +/// Route `--explain ` to its mode: the literal `allow` → suppression +/// guide; a known catalog rule id → rule card; anything else → file-path +/// architecture diagnostics. Operation: mode dispatch. +// qual:api +pub fn dispatch_explain(target: &Path, config: &Config) -> Result<(), i32> { + if target.as_os_str() == "allow" { + explain_allow(); + return Ok(()); + } + if let Some(text) = rule_card_text(&target.to_string_lossy()) { + print!("{text}"); + return Ok(()); + } + handle_explain(target, config) +} + +/// Render the rule card for `id` (case-insensitive), or `None` when `id` is +/// not a catalog rule. Integration: registry lookup + render. +pub(crate) fn rule_card_text(id: &str) -> Option { + find_rule_card(id).map(render_rule_card) +} + +/// Format one rule card for the terminal: title line + the five sections a +/// consumer needs to act (what fired, why, fix, suppression, config knob). +/// Operation: string assembly. +fn render_rule_card(card: &RuleCard) -> String { + format!( + "\n{id} — {title}\n\n Detects: {detects}\n Why: {why}\n \ + Fix: {fix}\n Suppress: {suppress}\n Config: {config}\n", + id = card.id, + title = card.title, + detects = card.detects, + why = card.why, + fix = card.fix, + suppress = card.suppress, + config = card.config, + ) +} + /// Print the `// qual:allow(...)` suppression guide (`rustqual --explain allow`). /// Operation: delegates to the testable builder + prints. // qual:api diff --git a/src/cli/explain/tests.rs b/src/cli/explain/tests/mod.rs similarity index 57% rename from src/cli/explain/tests.rs rename to src/cli/explain/tests/mod.rs index 8d47c40a..9cd35010 100644 --- a/src/cli/explain/tests.rs +++ b/src/cli/explain/tests/mod.rs @@ -27,3 +27,30 @@ fn suppression_guide_covers_grammar_targets_and_rationale() { assert!(g.contains("10%"), "10% headroom rationale"); assert!(g.contains("last resort"), "last-resort framing"); } + +#[test] +fn rule_card_text_renders_full_card_for_known_id() { + let text = super::rule_card_text("bp-009").expect("BP-009 must render (case-insensitive)"); + assert!(text.contains("BP-009"), "card must show the canonical id"); + assert!( + text.contains("Struct update boilerplate"), + "card must show the title: {text}" + ); + for section in ["Detects:", "Why:", "Fix:", "Suppress:", "Config:"] { + assert!(text.contains(section), "card must have section {section}"); + } + assert!( + text.contains("qual:allow(dry, boilerplate)"), + "card must cite the copyable suppression marker" + ); + assert!( + text.contains("[boilerplate]"), + "card must point at the config section" + ); +} + +#[test] +fn rule_card_text_is_none_for_unknown_id() { + assert!(super::rule_card_text("BP-999").is_none()); + assert!(super::rule_card_text("src/lib.rs").is_none()); +} diff --git a/src/domain/mod.rs b/src/domain/mod.rs index 8a579ef3..0bd3df48 100644 --- a/src/domain/mod.rs +++ b/src/domain/mod.rs @@ -12,6 +12,7 @@ pub mod analysis_data; pub mod dimension; pub mod finding; pub mod findings; +pub mod rule_cards; pub mod score; pub mod severity; pub mod source_unit; diff --git a/src/domain/rule_cards/architecture.rs b/src/domain/rule_cards/architecture.rs new file mode 100644 index 00000000..c1a09105 --- /dev/null +++ b/src/domain/rule_cards/architecture.rs @@ -0,0 +1,145 @@ +//! Rule cards for the Architecture dimension (hierarchical +//! `architecture/[/]` ids; only base ids carry cards — +//! pattern/trait_contract sub-kinds are user-defined rule names). + +use super::RuleCard; + +pub(super) const CARDS: &[RuleCard] = &[ + RuleCard { + id: "architecture/layer", + title: "Layer rule violation", + detects: "A file importing against its layer's allowed direction, \ + as defined by the [architecture.layers] ranks (inner layers \ + must not depend on outer ones).", + why: "Layering only protects the core if every edge respects it; \ + one inward-pointing import couples the domain to its \ + adapters.", + fix: "Move the needed code inward, invert the dependency with a \ + port (trait) defined in the inner layer, or relocate the file \ + to the layer it actually belongs to.", + suppress: "// qual:allow(architecture, layer) reason: \"…\".", + config: "[architecture.layers] defines globs and ranks; \ + [architecture.reexport_points] exempts composition roots.", + }, + RuleCard { + id: "architecture/layer/unmatched", + title: "File doesn't match any configured layer", + detects: "A production file whose path matches no \ + [architecture.layers] glob, under unmatched_behavior = \ + \"strict_error\".", + why: "An unmatched file is outside the architecture: its \ + dependencies are checked against nothing, so the layer rule \ + silently does not apply to it.", + fix: "Move the file into a layer directory, extend the layer's \ + glob, or register it under [architecture.reexport_points] if \ + it is composition-root code.", + suppress: "// qual:allow(architecture, layer) reason: \"…\" (the \ + layer family covers unmatched findings).", + config: "[architecture] unmatched_behavior (strict_error | warn | \ + ignore); [architecture.layers] globs.", + }, + RuleCard { + id: "architecture/forbidden", + title: "Forbidden-edge violation between layers", + detects: "An import matching a [[architecture.forbidden]] rule — \ + an explicitly banned from-glob → to-glob edge.", + why: "Forbidden edges encode project-specific decisions the \ + generic layer ranks cannot express; a hit is a documented \ + decision being violated.", + fix: "Follow the rule's own message (each forbidden rule carries \ + its rationale); typically: route through the sanctioned \ + module instead of importing the banned one directly.", + suppress: "// qual:allow(architecture, forbidden) reason: \"…\".", + config: "[[architecture.forbidden]] rules (from/to globs + \ + message).", + }, + RuleCard { + id: "architecture/pattern", + title: "Symbol-pattern violation (path/method/macro)", + detects: "A use of a forbidden symbol matching a \ + [[architecture.pattern]] rule: forbid_path_prefix, \ + forbid_method_call, forbid_macro_call, or forbid_glob_import \ + (prelude globs exempt unless allow_prelude_glob = false). \ + Macro bodies are scanned via structured recovery.", + why: "Pattern rules ban concrete escape hatches (panic in \ + production, println debugging, direct DB access) that layer \ + globs cannot see.", + fix: "Each pattern rule carries its own message naming the \ + sanctioned alternative — use that. The finding's sub-id names \ + the violated rule.", + suppress: "// qual:allow(architecture, pattern) reason: \"…\".", + config: "[[architecture.pattern]] rules (name, kind, value, \ + message, allow_prelude_glob).", + }, + RuleCard { + id: "architecture/trait_contract", + title: "Trait-contract violation", + detects: "A trait failing a [[architecture.trait_contract]] check \ + (e.g. object_safety, required method set) configured for it.", + why: "Contract checks pin design properties of key traits — losing \ + object safety or a required method breaks downstream \ + consumers at a distance.", + fix: "Restore the property the check names (the sub-id carries \ + the check kind), or update the contract rule if the design \ + legitimately moved on.", + suppress: "// qual:allow(architecture, trait_contract) reason: \ + \"…\".", + config: "[[architecture.trait_contract]] rules (trait name + \ + check).", + }, + RuleCard { + id: "architecture/call_parity/no_delegation", + title: "Adapter pub fn does not reach the target layer", + detects: "Check A: an adapter's pub fn whose call graph never \ + reaches the configured target layer.", + why: "An adapter entry point that does its own work instead of \ + delegating duplicates business logic outside the layer that \ + owns it.", + fix: "Route the entry point through the target layer's API; move \ + any local logic inward.", + suppress: "// qual:allow(architecture, call_parity) reason: \"…\".", + config: "[architecture.call_parity] (adapter globs, target layer; \ + see book/adapter-parity.md).", + }, + RuleCard { + id: "architecture/call_parity/missing_adapter", + title: "Target pub fn is not reached by every adapter (or is orphaned)", + detects: "Check B: a target-layer pub fn present in one adapter's \ + coverage but missing from another, or transitively unreachable \ + from any adapter touchpoint (dead island).", + why: "Parity gaps mean one interface (CLI, MCP, HTTP, …) silently \ + lacks a capability the others expose.", + fix: "Wire the missing adapter route to the target fn, or remove \ + the orphaned fn if no adapter should expose it.", + suppress: "// qual:allow(architecture, call_parity) reason: \"…\".", + config: "[architecture.call_parity] (see book/adapter-parity.md).", + }, + RuleCard { + id: "architecture/call_parity/multi_touchpoint", + title: "Adapter pub fn has multiple target touchpoints", + detects: "Check C: an adapter entry point calling into the target \ + layer at more than one place (severity configurable via \ + single_touchpoint, default warn).", + why: "Several touchpoints mean the adapter is composing business \ + steps itself — orchestration that belongs in the target \ + layer.", + fix: "Introduce one target-layer function that composes the steps; \ + call only it from the adapter.", + suppress: "// qual:allow(architecture, call_parity) reason: \"…\".", + config: "[architecture.call_parity] single_touchpoint (error | \ + warn | off).", + }, + RuleCard { + id: "architecture/call_parity/multiplicity_mismatch", + title: "Target pub fn reached with divergent handler counts across adapters", + detects: "Check D: every adapter reaches the target fn, but with \ + different handler counts (e.g. cli=2, mcp=1).", + why: "Divergent multiplicity usually means one adapter splits a \ + capability the others treat as one — the interfaces have \ + quietly stopped being parallel.", + fix: "Align the adapters on one route per capability, or document \ + the deliberate asymmetry in a suppression reason.", + suppress: "// qual:allow(architecture, call_parity) reason: \"…\".", + config: "[architecture.call_parity] (see book/adapter-parity.md).", + }, +]; diff --git a/src/domain/rule_cards/boilerplate.rs b/src/domain/rule_cards/boilerplate.rs new file mode 100644 index 00000000..0cc4bacc --- /dev/null +++ b/src/domain/rule_cards/boilerplate.rs @@ -0,0 +1,144 @@ +//! Rule cards for the boilerplate patterns (BP-*), part of the DRY dimension. + +use super::RuleCard; + +/// Shared suppress line: every BP pattern is one finding-kind (`boilerplate`). +const BP_SUPPRESS: &str = "// qual:allow(dry, boilerplate) reason: \"…\"."; + +/// Shared config line: patterns are individually selectable. +const BP_CONFIG: &str = "[boilerplate] patterns — list only the BP ids you \ + want active (empty list = all); suggest_crates toggles crate \ + recommendations in suggestions."; + +pub(super) const CARDS: &[RuleCard] = &[ + RuleCard { + id: "BP-001", + title: "Trivial From implementation (derivable)", + detects: "An impl From for B whose from() only maps fields \ + one-to-one, with no logic.", + why: "Hand-written field mapping drifts when either struct changes; \ + a declarative form keeps the conversion total by construction.", + fix: "derive_more::From, or struct-update / field-shorthand \ + construction. Keep a manual impl only when it documents a real \ + conversion contract.", + suppress: BP_SUPPRESS, + config: BP_CONFIG, + }, + RuleCard { + id: "BP-002", + title: "Trivial Display implementation (derivable)", + detects: "An impl Display whose single fmt method is one \ + write!/writeln! call with no further logic.", + why: "The same effect is available declaratively; N hand-rolled \ + trivial Displays drift in style and bury the one impl that \ + actually formats.", + fix: "derive_more::Display with #[display(\"…\")], or a project-wide \ + dependency-free idiom (e.g. f.write_str(&self.0) for \ + string-backed newtypes, or one local macro_rules! emitting the \ + impl for all newtypes).", + suppress: BP_SUPPRESS, + config: BP_CONFIG, + }, + RuleCard { + id: "BP-003", + title: "Trivial getter/setter (consider field visibility)", + detects: "Methods that only return or assign a single field, with no \ + validation or transformation.", + why: "Accessor ceremony without invariants adds indirection but no \ + encapsulation value.", + fix: "Make the field pub(crate)/pub where appropriate, or use a \ + getter/setter derive; keep manual accessors only where they \ + guard an invariant.", + suppress: BP_SUPPRESS, + config: BP_CONFIG, + }, + RuleCard { + id: "BP-004", + title: "Builder pattern (consider derive macro)", + detects: "A struct with many builder-style methods (set one field, \ + return self).", + why: "Hand-rolled builders are dozens of lines of mechanical code \ + that must be extended for every new field.", + fix: "derive_builder / typed-builder, or a constructor taking the \ + required fields plus Default for the rest.", + suppress: BP_SUPPRESS, + config: BP_CONFIG, + }, + RuleCard { + id: "BP-005", + title: "Manual Default implementation (derivable)", + detects: "An impl Default whose default() only fills fields with \ + literals or nested Default::default() calls.", + why: "The derive expresses the same thing shorter and stays correct \ + when fields are added.", + fix: "#[derive(Default)]; for non-zero defaults use field-level \ + #[default] (enums) or keep only the divergent fields manual via \ + a const.", + suppress: BP_SUPPRESS, + config: BP_CONFIG, + }, + RuleCard { + id: "BP-006", + title: "Repetitive match mapping", + detects: "A match that maps enum variants one-to-one onto values or \ + another enum's variants, arm after arm with the same shape.", + why: "Mechanical mappings hide in plain sight among real logic and \ + must be extended for every new variant.", + fix: "A lookup table (const array / match in one dedicated fn), a \ + strum derive, or storing the mapped value in the variant itself.", + suppress: BP_SUPPRESS, + config: BP_CONFIG, + }, + RuleCard { + id: "BP-007", + title: "Error enum boilerplate (consider thiserror)", + detects: "An error enum accompanied by several trivial From impls \ + (and manual Display/Error plumbing).", + why: "thiserror generates exactly this plumbing from attributes, \ + keeping the enum readable as a list of failure cases.", + fix: "#[derive(thiserror::Error)] with #[error(\"…\")] and #[from] \ + on the source fields.", + suppress: BP_SUPPRESS, + config: BP_CONFIG, + }, + RuleCard { + id: "BP-008", + title: "Clone-heavy conversion", + detects: "A struct construction cloning many fields from another \ + value.", + why: "Field-by-field cloning usually signals a conversion that \ + should own or borrow its input — each clone is a hidden \ + allocation and a sign the ownership design fights the data flow.", + fix: "Take the source by value and move the fields, implement \ + From/Into, or borrow with lifetimes where the copy is not \ + needed.", + suppress: BP_SUPPRESS, + config: BP_CONFIG, + }, + RuleCard { + id: "BP-009", + title: "Struct update boilerplate", + detects: "A function constructing the same struct type several times \ + with mostly overlapping fields, instead of deriving the variants \ + from a base value.", + why: "Repeating the unchanged fields obscures which field actually \ + differs between the constructions — the one thing the reader \ + needs to see.", + fix: "Build one base value and use struct-update syntax for the \ + variants: Type { changed, ..base } (Clone the base if needed).", + suppress: BP_SUPPRESS, + config: BP_CONFIG, + }, + RuleCard { + id: "BP-010", + title: "Format string repetition", + detects: "The same format string literal used in many \ + format!/println!-family calls across a file.", + why: "A repeated format string is a de-facto template; copies drift \ + in wording and there is no single place to change the format.", + fix: "Extract the string into a const, or wrap the formatting in a \ + small helper function that owns the template.", + suppress: BP_SUPPRESS, + config: BP_CONFIG, + }, +]; diff --git a/src/domain/rule_cards/complexity.rs b/src/domain/rule_cards/complexity.rs new file mode 100644 index 00000000..69534c93 --- /dev/null +++ b/src/domain/rule_cards/complexity.rs @@ -0,0 +1,103 @@ +//! Rule cards for the Complexity dimension (CX-*, A20). + +use super::RuleCard; + +pub(super) const CARDS: &[RuleCard] = &[ + RuleCard { + id: "CX-001", + title: "Cognitive complexity exceeds threshold", + detects: "A function whose cognitive-complexity score (branching, \ + weighted by nesting depth) exceeds max_cognitive (default 15).", + why: "Cognitive complexity measures how hard a body is to follow; \ + deeply nested decisions are where defects hide and reviews stall.", + fix: "Extract decision branches into small named functions, flatten \ + nesting with early returns / let-else, or replace conditional \ + ladders with match dispatch or table lookups.", + suppress: "// qual:allow(complexity, max_cognitive=N) reason: \"…\" — \ + pin at the current value; re-fires above N.", + config: "[complexity] max_cognitive (default 15).", + }, + RuleCard { + id: "CX-002", + title: "Cyclomatic complexity exceeds threshold", + detects: "A function with more independent paths (branches, loops, \ + guards) than max_cyclomatic (default 10). Trivial match arms do \ + not increment the count.", + why: "Every path needs a test; past the threshold the function can no \ + longer be covered or reasoned about exhaustively.", + fix: "Split the function along its decision points; collapse repeated \ + branching into data-driven dispatch (match → lookup table) or \ + extract validation chains into helpers returning Result.", + suppress: "// qual:allow(complexity, max_cyclomatic=N) reason: \"…\" — \ + pin at the current value; re-fires above N.", + config: "[complexity] max_cyclomatic (default 10).", + }, + RuleCard { + id: "CX-003", + title: "Magic number literal in non-const context", + detects: "A numeric literal outside const/static context in \ + production code (test functions are exempt). Negative literals \ + count once; allowed_magic_numbers are skipped.", + why: "An unnamed constant hides intent and invites divergent copies \ + of the same value across the codebase.", + fix: "Name the value as a const next to its use (or in the module's \ + constants block) with a domain-meaningful name.", + suppress: "// qual:allow(complexity, magic_numbers) reason: \"…\".", + config: "[complexity] detect_magic_numbers, allowed_magic_numbers.", + }, + RuleCard { + id: "CX-004", + title: "Function length exceeds threshold", + detects: "A function body longer than max_function_lines (default \ + 60). Applies to test functions too, at [tests].max_function_lines \ + (which defaults to the production value).", + why: "Long functions bundle several jobs; they resist naming, reuse, \ + and review, and their tests blur what is actually asserted.", + fix: "Extract cohesive steps into named helpers. In tests: pull setup \ + into builder/helper functions or split into focused tests.", + suppress: "// qual:allow(complexity, max_function_lines=N) reason: \ + \"…\" — pin at the current value; re-fires above N.", + config: "[complexity] max_function_lines; [tests] max_function_lines \ + overrides it for test code.", + }, + RuleCard { + id: "CX-005", + title: "Nesting depth exceeds threshold", + detects: "A function whose deepest block nesting exceeds \ + max_nesting_depth (default 4).", + why: "Each nesting level multiplies the conditions a reader must hold \ + in their head to understand the innermost line.", + fix: "Invert conditions into early returns, use let-else for \ + fallible bindings, extract the inner loop/match body into a \ + function.", + suppress: "// qual:allow(complexity, max_nesting_depth=N) reason: \ + \"…\" — pin at the current value; re-fires above N.", + config: "[complexity] max_nesting_depth (default 4).", + }, + RuleCard { + id: "CX-006", + title: "Unsafe block detected", + detects: "An unsafe block or unsafe fn in analyzed code.", + why: "Each unsafe block is a manual proof obligation the compiler no \ + longer checks; the score keeps visible where those live.", + fix: "Encapsulate the unsafe behind a small, audited safe API with a \ + // SAFETY: comment, or replace it with a safe alternative.", + suppress: "// qual:allow(unsafe) reason: \"…\" — the dedicated narrow \ + marker for CX-006 (equivalently // qual:allow(complexity, unsafe)).", + config: "[complexity] detect_unsafe (default true).", + }, + RuleCard { + id: "A20", + title: "Error handling issue (unwrap/expect/panic/todo)", + detects: "unwrap(), expect() (unless allow_expect = true), panic!, \ + todo!, or unimplemented! in production code (test functions are \ + exempt).", + why: "Each is a crash path that bypasses Result-based error flow — an \ + input the author did not handle, not an error strategy.", + fix: "Propagate with `?`, convert into a typed error (thiserror), or \ + handle the None/Err branch explicitly. For true invariants, \ + document via expect(\"invariant: …\") and set allow_expect = true.", + suppress: "// qual:allow(complexity, error_handling) reason: \"…\".", + config: "[complexity] detect_error_handling, allow_expect.", + }, +]; diff --git a/src/domain/rule_cards/coupling.rs b/src/domain/rule_cards/coupling.rs new file mode 100644 index 00000000..6a8ffcf8 --- /dev/null +++ b/src/domain/rule_cards/coupling.rs @@ -0,0 +1,53 @@ +//! Rule cards for the Coupling dimension (CP-*). + +use super::RuleCard; + +pub(super) const CARDS: &[RuleCard] = &[ + RuleCard { + id: "CP-001", + title: "Circular module dependency", + detects: "A cycle in the module dependency graph (A uses B uses … \ + uses A).", + why: "Modules in a cycle cannot be understood, tested, or replaced \ + independently — the cycle is effectively one large module.", + fix: "Extract what both sides need into a third module, or invert \ + one edge with a trait defined on the consumer side (dependency \ + inversion).", + suppress: "Not suppressible — circular dependencies always count \ + toward the score (there is deliberately no allow(coupling, \ + cycle) target).", + config: "[coupling] enabled — the check has no threshold; a cycle \ + either exists or not.", + }, + RuleCard { + id: "CP-002", + title: "Stable Dependencies Principle violation", + detects: "A module depending on a module that is less stable \ + (higher instability I = Ce/(Ca+Ce)) than itself.", + why: "When stable code depends on volatile code, every change in \ + the volatile module ripples into the code everyone else relies \ + on.", + fix: "Invert the dependency with an interface owned by the stable \ + side, or move the volatile part out of the stable module's \ + dependency path.", + suppress: "// qual:allow(coupling, sdp) reason: \"…\".", + config: "[coupling] check_sdp (default true).", + }, + RuleCard { + id: "CP-003", + title: "Module instability exceeds configured threshold", + detects: "A module whose instability I = Ce/(Ca+Ce) exceeds \ + max_instability (default 0.8), or whose fan-in/fan-out exceed \ + max_fan_in/max_fan_out.", + why: "A module that depends on many others while many depend on it \ + is a change amplifier: edits anywhere reach it, and its edits \ + reach everywhere.", + fix: "Reduce outgoing dependencies (split the module, inject \ + dependencies via traits) or reduce incoming ones (move the \ + widely-used parts into a smaller, stable core).", + suppress: "// qual:allow(coupling, max_instability=N) reason: \"…\" \ + (also max_fan_in=N / max_fan_out=N). Module-global metrics: the \ + marker is not line-anchored, so orphan detection skips it.", + config: "[coupling] max_instability, max_fan_in, max_fan_out.", + }, +]; diff --git a/src/domain/rule_cards/dry.rs b/src/domain/rule_cards/dry.rs new file mode 100644 index 00000000..5ab34802 --- /dev/null +++ b/src/domain/rule_cards/dry.rs @@ -0,0 +1,77 @@ +//! Rule cards for the DRY dimension (DRY-*). + +use super::RuleCard; + +pub(super) const CARDS: &[RuleCard] = &[ + RuleCard { + id: "DRY-001", + title: "Duplicate function detected", + detects: "Two or more functions whose normalized bodies are at least \ + similarity_threshold alike (default 0.85; exact duplicates score \ + 1.0). Runs on test code too.", + why: "Parallel copies drift: a fix lands in one and not the other, \ + and reviewers must diff by eye to see whether they still match.", + fix: "Extract one shared helper and call it from every site. For \ + deliberate mirror pairs (encode/decode, serialize/deserialize), \ + mark one side with // qual:inverse(other_fn) instead.", + suppress: "// qual:allow(dry, duplicate) reason: \"…\".", + config: "[duplicates] similarity_threshold, min_tokens, \ + ignore_trait_impls.", + }, + RuleCard { + id: "DRY-002", + title: "Dead code detected", + detects: "A production function never called from production code: \ + 'uncalled' (no callers at all) or 'testonly' (only tests reach \ + it). Macro bodies are scanned, so calls inside vec!/format!/rsx! \ + count as references.", + why: "Dead code is maintained, reviewed, and refactored without ever \ + running — pure carrying cost that also misleads readers about \ + what the system does.", + fix: "Delete the function, or wire it to its intended caller. Public \ + API entry points: mark // qual:api. Integration-test helpers \ + living in src/: mark // qual:test_helper.", + suppress: "// qual:api (public API) or // qual:test_helper — DRY-002 \ + is deliberately NOT suppressible via qual:allow(dry, …).", + config: "[duplicates] detect_dead_code (default true).", + }, + RuleCard { + id: "DRY-003", + title: "Duplicate code fragment", + detects: "The same statement sequence (at least min_lines lines, \ + default 5) repeated across function bodies, in production and \ + test code.", + why: "Fragment-level copies are the early stage of DRY-001: each \ + repetition is a future divergence point.", + fix: "Extract the fragment into a named function; if the copies vary \ + only in values, parameterize them.", + suppress: "// qual:allow(dry, fragment) reason: \"…\".", + config: "[duplicates] min_lines, min_statements.", + }, + RuleCard { + id: "DRY-004", + title: "Wildcard import (use module::*)", + detects: "A glob import in production code. Prelude globs (any path \ + with a ::prelude segment) are skipped by this check's own logic; \ + test files are exempt.", + why: "Globs hide where names come from, create silent collisions, \ + and let an upstream addition change this module's meaning.", + fix: "Import the used names explicitly; group with braces if the \ + list is long.", + suppress: "// qual:allow(dry, wildcard_imports) reason: \"…\".", + config: "[duplicates] detect_wildcard_imports (default true).", + }, + RuleCard { + id: "DRY-005", + title: "Repeated match pattern across functions", + detects: "The same enum matched arm-by-arm in several functions \ + (3+ arms in 3+ places), in production and test code.", + why: "Scattered matches over one enum mean every new variant demands \ + a synchronized edit in N places — a missed one is a logic bug.", + fix: "Centralize the dispatch: one function owning the match, or move \ + the behavior into the enum as a method (one impl arm per \ + variant).", + suppress: "// qual:allow(dry, repeated_matches) reason: \"…\".", + config: "[duplicates] detect_repeated_matches (default true).", + }, +]; diff --git a/src/domain/rule_cards/governance.rs b/src/domain/rule_cards/governance.rs new file mode 100644 index 00000000..c3f15e16 --- /dev/null +++ b/src/domain/rule_cards/governance.rs @@ -0,0 +1,40 @@ +//! Rule cards for suppression governance (SUP-*, ORPHAN-*). + +use super::RuleCard; + +pub(super) const CARDS: &[RuleCard] = &[ + RuleCard { + id: "SUP-001", + title: "Suppression ratio exceeds configured maximum", + detects: "The ratio of suppression markers (qual:allow + #[allow]) \ + to functions exceeds max_suppression_ratio (default 0.05).", + why: "Each marker is debt; past a ratio the tool is being silenced \ + rather than satisfied, and the score stops meaning anything.", + fix: "Fix findings instead of allowing them; delete stale markers \ + (ORPHAN-001 lists them). Raising max_suppression_ratio is a \ + deliberate, visible decision — make it in review, not ad hoc.", + suppress: "Not suppressible — this is the meta-check on \ + suppressions themselves. It warns by default and fails only \ + with --fail-on-warnings.", + config: "max_suppression_ratio (top level, default 0.05).", + }, + RuleCard { + id: "ORPHAN-001", + title: "Stale qual:allow marker: no finding in the annotation window", + detects: "A qual:allow marker that matches no finding of its kind \ + in its comment window (stale), or a metric pin sitting more \ + than [suppression].pin_headroom above the value it covers \ + (too-loose).", + why: "A stale marker silently pre-authorizes a future regression; \ + a too-loose pin absorbs growth up to its ceiling without \ + anyone deciding that.", + fix: "Delete the stale marker; tighten a too-loose pin to the \ + current metric value. If the marker was a typo (wrong target \ + name), write the intended one — rustqual --explain allow lists \ + the vocabulary.", + suppress: "Not suppressible — the marker itself is the problem; \ + the only fix is editing or removing it.", + config: "[suppression] pin_headroom (default 0.10) governs the \ + too-loose threshold.", + }, +]; diff --git a/src/domain/rule_cards/iosp.rs b/src/domain/rule_cards/iosp.rs new file mode 100644 index 00000000..171d9622 --- /dev/null +++ b/src/domain/rule_cards/iosp.rs @@ -0,0 +1,23 @@ +//! Rule card for the IOSP dimension (single-kind). + +use super::RuleCard; + +pub(super) const CARDS: &[RuleCard] = &[RuleCard { + id: "iosp/violation", + title: "Function violates the Integration Operation Segregation Principle", + detects: "A function that both calls other project functions AND contains \ + its own logic (if/for/match/computation) — neither a pure Integration \ + (orchestration only) nor a pure Operation (logic only).", + why: "Mixed functions interleave decisions with delegation, so neither the \ + flow nor the logic can be read, tested, or reused on its own; defects \ + hide in the seams between the two.", + fix: "Extract the logic into Operation functions and keep this function as \ + a pure orchestrator — or inline the orchestration so the function \ + becomes a pure Operation. In lenient mode (default), closures and \ + iterator chains do not count as logic; for-loops and matches whose \ + every arm only delegates count as dispatch.", + suppress: "// qual:allow(iosp) reason: \"…\" — the bare form (iosp has no \ + sub-targets); legacy alias // iosp:allow.", + config: "[weights] iosp sets the dimension weight; --strict-closures / \ + --strict-iterators tighten the leniency rules.", +}]; diff --git a/src/domain/rule_cards/mod.rs b/src/domain/rule_cards/mod.rs new file mode 100644 index 00000000..865a4cd6 --- /dev/null +++ b/src/domain/rule_cards/mod.rs @@ -0,0 +1,66 @@ +//! Rule cards — the single source of truth for per-rule metadata. +//! +//! Every reporter that names a rule renders from this registry: the SARIF +//! `tool.driver.rules` table, `--explain `, and the text summary's +//! rule titles. One card per stable catalog id; adding a rule means adding a +//! card here (the SARIF sync test enforces set equality). + +mod architecture; +mod boilerplate; +mod complexity; +mod coupling; +mod dry; +mod governance; +mod iosp; +mod srp; +mod structural; +mod tq; + +/// Everything a consumer needs to act on a finding of one rule: what fired, +/// why it matters, how to fix it, how to suppress it as a last resort, and +/// which config knob governs it. All fields are mandatory prose — a rule +/// without a config knob or suppression says so explicitly instead of +/// leaving the reader to guess. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct RuleCard { + /// Stable catalog id (`BP-009`, `architecture/layer`, …). + pub id: &'static str, + /// One-line title, shared with the SARIF `shortDescription`. + pub title: &'static str, + /// What pattern the analyzer matches. + pub detects: &'static str, + /// Why the pattern is worth flagging. + pub why: &'static str, + /// Concrete fix forms, most preferred first. + pub fix: &'static str, + /// The copyable suppression marker (or why none exists). + pub suppress: &'static str, + /// The governing `rustqual.toml` knob (or why none exists). + pub config: &'static str, +} + +/// Card groups in catalog order (mirrors the SARIF rules table). +const GROUPS: &[&[RuleCard]] = &[ + iosp::CARDS, + complexity::CARDS, + dry::CARDS, + boilerplate::CARDS, + srp::CARDS, + coupling::CARDS, + structural::CARDS, + tq::CARDS, + architecture::CARDS, + governance::CARDS, +]; + +/// Every rule card, in catalog order. +/// Operation: flattens the static groups. +pub fn all_rule_cards() -> impl Iterator { + GROUPS.iter().flat_map(|g| g.iter()) +} + +/// Look up one card by catalog id, case-insensitively (`bp-009` resolves +/// like `BP-009`). Integration: scans `all_rule_cards`. +pub fn find_rule_card(id: &str) -> Option<&'static RuleCard> { + all_rule_cards().find(|c| c.id.eq_ignore_ascii_case(id)) +} diff --git a/src/domain/rule_cards/srp.rs b/src/domain/rule_cards/srp.rs new file mode 100644 index 00000000..773939fd --- /dev/null +++ b/src/domain/rule_cards/srp.rs @@ -0,0 +1,59 @@ +//! Rule cards for the SRP dimension (SRP-*). + +use super::RuleCard; + +pub(super) const CARDS: &[RuleCard] = &[ + RuleCard { + id: "SRP-001", + title: "Struct may violate Single Responsibility Principle", + detects: "A struct whose composite smell score crosses \ + smell_threshold — inputs: field count (max_fields), method count \ + (max_methods), fan-out (max_fan_out), and LCOM4 cohesion \ + (lcom4_threshold). Also fires on test-file structs (a \ + god-fixture is a real smell), at production thresholds.", + why: "A struct accumulating fields, methods, and dependencies has \ + several reasons to change; every change risks the unrelated \ + responsibilities sharing its state.", + fix: "Split along the LCOM4 method clusters: each disconnected \ + cluster of methods+fields is a candidate type. Extract \ + collaborating field groups into sub-structs the original \ + composes.", + suppress: "// qual:allow(srp, god_struct) reason: \"…\".", + config: "[srp] smell_threshold, max_fields, max_methods, \ + max_fan_out, lcom4_threshold, weights.", + }, + RuleCard { + id: "SRP-002", + title: "Module has excessive production code length", + detects: "A file longer than file_length (default 300 lines). Test \ + files are checked too, at [tests].file_length (defaults to the \ + production value). Production files additionally fire when they \ + contain more than max_independent_clusters unrelated \ + free-function groups (module cohesion; production-only).", + why: "A long file is where unrelated concerns quietly co-locate; \ + independent clusters in one module are separate modules wearing \ + one name.", + fix: "Split along behavior seams into focused modules; the \ + independent clusters named by the finding are the natural cut \ + lines.", + suppress: "// qual:allow(srp, file_length=N) reason: \"…\" — or the \ + max_independent_clusters=N target for the cluster variant; pins \ + re-fire above N.", + config: "[srp] file_length, max_independent_clusters, \ + min_cluster_statements; [tests] file_length for test files.", + }, + RuleCard { + id: "SRP-003", + title: "Function has too many parameters — reduce parameter count", + detects: "A function signature with more than max_parameters \ + parameters (default 5).", + why: "Long parameter lists are a missing type: callers pass loose \ + values whose relationships and invariants nothing checks.", + fix: "Group related parameters into a struct (often revealing a \ + domain concept), or split the function if the parameters serve \ + different jobs.", + suppress: "// qual:allow(srp, max_parameters=N) reason: \"…\" — pin \ + re-fires above N.", + config: "[srp] max_parameters (default 5).", + }, +]; diff --git a/src/domain/rule_cards/structural.rs b/src/domain/rule_cards/structural.rs new file mode 100644 index 00000000..3ca06e23 --- /dev/null +++ b/src/domain/rule_cards/structural.rs @@ -0,0 +1,99 @@ +//! Rule cards for the structural binary checks — part of SRP (BTC, SLM, +//! NMS) and Coupling (OI, SIT, DEH, IET). + +use super::RuleCard; + +pub(super) const CARDS: &[RuleCard] = &[ + RuleCard { + id: "BTC", + title: "Broken trait contract: all methods are stubs", + detects: "An impl Trait block where every method body is a stub \ + (todo!/unimplemented!/empty).", + why: "A contract implemented by stubs satisfies the compiler but \ + lies to callers: the type promises behavior it does not have.", + fix: "Implement the methods, or remove the impl. If the trait \ + demands methods this type cannot honor, the trait is too wide — \ + split it.", + suppress: "// qual:allow(srp, btc) reason: \"…\".", + config: "[structural] check_btc (default true).", + }, + RuleCard { + id: "SLM", + title: "Selfless method: takes self but never references it", + detects: "A method with a self receiver whose body never uses self \ + — macro bodies are scanned, so self inside format!(…) counts as \ + a reference.", + why: "A receiver that is never read misstates the method's \ + dependencies and forces callers to have an instance they don't \ + need.", + fix: "Make it an associated function (drop self) or a free \ + function. If a trait signature forces the receiver, name that \ + in the suppression reason.", + suppress: "// qual:allow(srp, slm) reason: \"…\".", + config: "[structural] check_slm (default true).", + }, + RuleCard { + id: "NMS", + title: "Needless &mut self: takes &mut self but never mutates", + detects: "A method declaring &mut self whose body never mutates \ + self.", + why: "The needless mut blocks callers holding shared references and \ + overstates the method's effects.", + fix: "Change the receiver to &self.", + suppress: "// qual:allow(srp, nms) reason: \"…\".", + config: "[structural] check_nms (default true).", + }, + RuleCard { + id: "OI", + title: "Orphaned impl: impl block in different file than type", + detects: "An inherent impl Foo block living in a different file \ + than the struct/enum Foo definition.", + why: "A type's behavior scattered across files defeats the reader \ + who opens the type to learn what it does.", + fix: "Move the impl next to its type. For deliberate splits (e.g. \ + one file per concern of a large type), name the reason in a \ + suppression.", + suppress: "// qual:allow(coupling, oi) reason: \"…\".", + config: "[structural] check_oi (default true).", + }, + RuleCard { + id: "SIT", + title: "Single-impl trait: non-pub trait with exactly one implementation", + detects: "A non-pub trait implemented exactly once (#[cfg(test)] \ + impls count, so test seams with a test double are not flagged).", + why: "An abstraction with one realization adds indirection without \ + offering choice — readers must follow the trait to find the one \ + place the behavior lives.", + fix: "Inline the trait into its single implementor. Keep it only as \ + a deliberate seam (future second impl, test boundary) and say \ + so.", + suppress: "// qual:allow(coupling, sit) reason: \"…\".", + config: "[structural] check_sit (default true).", + }, + RuleCard { + id: "DEH", + title: "Downcast escape hatch: use of Any::downcast", + detects: "A call to Any::downcast/downcast_ref/downcast_mut.", + why: "Downcasting reintroduces the type knowledge the abstraction \ + was supposed to hide; each cast is a place the design leaks.", + fix: "Model the alternatives explicitly — an enum over the known \ + variants, or a trait method that moves the behavior to where \ + the concrete type is known.", + suppress: "// qual:allow(coupling, deh) reason: \"…\" (e.g. a plugin \ + boundary that genuinely requires dynamic typing).", + config: "[structural] check_deh (default true).", + }, + RuleCard { + id: "IET", + title: "Inconsistent error types in module", + detects: "A module whose public functions return several unrelated \ + error types.", + why: "Callers must learn one error contract per function instead of \ + one per module; conversions multiply at every call site.", + fix: "Define one module error enum (thiserror) with From \ + conversions for the underlying sources; return it from every \ + pub fn.", + suppress: "// qual:allow(coupling, iet) reason: \"…\".", + config: "[structural] check_iet (default true).", + }, +]; diff --git a/src/domain/rule_cards/tq.rs b/src/domain/rule_cards/tq.rs new file mode 100644 index 00000000..81196ac7 --- /dev/null +++ b/src/domain/rule_cards/tq.rs @@ -0,0 +1,81 @@ +//! Rule cards for the Test-Quality dimension (TQ-*). + +use super::RuleCard; + +pub(super) const CARDS: &[RuleCard] = &[ + RuleCard { + id: "TQ-001", + title: "Test function has no assertions", + detects: "A test function without any assert-family macro, \ + should_panic expectation, or configured extra assertion macro. \ + Tests inside proptest!/quickcheck! bodies are recognized too.", + why: "A test that asserts nothing can only fail by panicking — it \ + documents that the code ran, not that it was right.", + fix: "Assert on observable behavior (return value, state change, \ + emitted output). If the point is 'does not panic', still assert \ + the result so regressions surface as failures, not silence.", + suppress: "// qual:allow(test_quality, no_assertion) reason: \"…\".", + config: "[test_quality] extra_assertion_macros extends what counts \ + as an assertion (e.g. verify!, check!).", + }, + RuleCard { + id: "TQ-002", + title: "Test function does not call any production function", + detects: "A test that never calls into production code — macro \ + bodies are scanned, so a SUT call inside assert!(…) or rsx!{…} \ + counts.", + why: "A test without a system-under-test only tests its own \ + fixtures; it passes forever and verifies nothing.", + fix: "Call the unit under test directly and assert on its result; \ + if the test exists to exercise a helper, test the production \ + caller instead.", + suppress: "// qual:allow(test_quality, no_sut) reason: \"…\".", + config: "[test_quality] enabled — the check itself has no \ + threshold.", + }, + RuleCard { + id: "TQ-003", + title: "Production function is untested", + detects: "A production function no test reaches through the call \ + graph (visitor-dispatch edges and macro-embedded calls are \ + resolved).", + why: "Unreached code has no safety net: refactorings and \ + regressions in it are invisible to the suite.", + fix: "Add a test calling it directly, or through a covered caller. \ + Public API entry points: mark // qual:api. Integration-test \ + helpers in src/: // qual:test_helper. Functions referenced only \ + dynamically (e.g. serde default = \"fn\") need a direct test \ + call — the call graph cannot see string references.", + suppress: "// qual:allow(test_quality, untested) reason: \"…\".", + config: "[test_quality] enabled; // qual:api and // qual:test_helper \ + exclude entry points without ratio cost.", + }, + RuleCard { + id: "TQ-004", + title: "Production function has no coverage", + detects: "A production function with zero covered lines in the LCOV \ + file passed via --coverage (functions absent from the LCOV data \ + are skipped, not flagged).", + why: "Call-graph reachability says a test could reach the function; \ + coverage says none actually did — the stronger signal.", + fix: "Exercise the function in a test that actually runs it; if it \ + is only reachable behind a cfg/feature, run the suite with that \ + configuration.", + suppress: "// qual:allow(test_quality, uncovered) reason: \"…\".", + config: "--coverage enables the check; [test_quality] \ + coverage_file sets a default path.", + }, + RuleCard { + id: "TQ-005", + title: "Untested logic branches (uncovered lines)", + detects: "A covered production function that still has uncovered \ + lines — branches the suite never takes (requires --coverage).", + why: "The uncovered branches are usually the error paths and edge \ + cases — exactly where defects live.", + fix: "Add cases for the uncovered branches: error returns, else \ + arms, early exits. The finding lists the uncovered lines.", + suppress: "// qual:allow(test_quality, untested_logic) reason: \"…\".", + config: "--coverage enables the check; [test_quality] \ + coverage_file sets a default path.", + }, +]; diff --git a/src/domain/tests/mod.rs b/src/domain/tests/mod.rs index 27ab2090..56d2ad7b 100644 --- a/src/domain/tests/mod.rs +++ b/src/domain/tests/mod.rs @@ -1,5 +1,6 @@ mod dimension; mod finding; +mod rule_cards; mod severity; mod source_unit; mod suppression; diff --git a/src/domain/tests/rule_cards.rs b/src/domain/tests/rule_cards.rs new file mode 100644 index 00000000..9bcef0ee --- /dev/null +++ b/src/domain/tests/rule_cards.rs @@ -0,0 +1,115 @@ +//! Tests for the rule-card registry — the single source of truth every +//! reporter (SARIF, `--explain`, summary titles) renders rule metadata from. + +use crate::domain::rule_cards::{all_rule_cards, find_rule_card}; + +#[test] +fn find_is_case_insensitive() { + let lower = find_rule_card("bp-009").expect("bp-009 must resolve"); + let upper = find_rule_card("BP-009").expect("BP-009 must resolve"); + assert_eq!(lower.id, "BP-009"); + assert_eq!(upper.id, "BP-009"); +} + +#[test] +fn unknown_id_is_none() { + assert!(find_rule_card("BP-999").is_none()); + assert!(find_rule_card("not-a-rule").is_none()); + assert!(find_rule_card("").is_none()); +} + +#[test] +fn ids_are_unique() { + let mut seen = std::collections::HashSet::new(); + for card in all_rule_cards() { + assert!(seen.insert(card.id), "duplicate rule-card id {}", card.id); + } +} + +#[test] +fn every_card_is_fully_filled() { + // Every field must carry substantive content — an empty `suppress` or + // `config` would leave a consumer without a next step, which is exactly + // the failure mode the cards exist to prevent. "Not suppressible" etc. + // must be said explicitly. + for card in all_rule_cards() { + for (name, value) in [ + ("title", card.title), + ("detects", card.detects), + ("why", card.why), + ("fix", card.fix), + ("suppress", card.suppress), + ("config", card.config), + ] { + assert!( + value.len() >= 10, + "card {} field `{name}` must be substantive, got {value:?}", + card.id + ); + } + } +} + +#[test] +fn every_rule_family_is_represented() { + // One spot-check per family so a forgotten module can't slip through. + for id in [ + "iosp/violation", + "CX-001", + "A20", + "DRY-002", + "BP-002", + "SRP-001", + "CP-003", + "TQ-003", + "SLM", + "IET", + "architecture/pattern", + "architecture/call_parity/no_delegation", + "SUP-001", + "ORPHAN-001", + ] { + assert!(find_rule_card(id).is_some(), "missing rule card for {id}"); + } +} + +#[test] +fn cards_name_real_suppression_targets() { + // The suppress field must cite the actual `qual:allow` vocabulary, not an + // invented name — an agent copies this line verbatim. + let bp = find_rule_card("BP-002").unwrap(); + assert!( + bp.suppress.contains("qual:allow(dry, boilerplate)"), + "BP-002 suppress must cite the dry/boilerplate target: {}", + bp.suppress + ); + let cx = find_rule_card("CX-001").unwrap(); + assert!( + cx.suppress + .contains("qual:allow(complexity, max_cognitive="), + "CX-001 suppress must cite the pinned metric form: {}", + cx.suppress + ); + let dead = find_rule_card("DRY-002").unwrap(); + assert!( + dead.suppress.contains("qual:api"), + "DRY-002 is excluded via qual:api, not allow(dry): {}", + dead.suppress + ); +} + +#[test] +fn cards_name_their_config_knob() { + let bp = find_rule_card("BP-009").unwrap(); + assert!( + bp.config.contains("[boilerplate]"), + "BP-009 config must point at the [boilerplate] section: {}", + bp.config + ); + let cx = find_rule_card("CX-004").unwrap(); + assert!( + cx.config.contains("max_function_lines"), + "CX-004 config must name the threshold field: {}", + cx.config + ); +} diff --git a/src/lib.rs b/src/lib.rs index ae800426..50429f55 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -96,13 +96,7 @@ fn run_configured(cli: &Cli) -> Result<(), i32> { /// Operation: mode checks. fn try_postconfig_modes(cli: &Cli, config: &config::Config) -> Option> { if let Some(ref target) = cli.explain { - // `--explain allow` prints the suppression guide instead of explaining - // a file's architecture rules. - if target.as_os_str() == "allow" { - crate::cli::explain::explain_allow(); - return Some(Ok(())); - } - return Some(crate::cli::explain::handle_explain(target, config)); + return Some(crate::cli::explain::dispatch_explain(target, config)); } if cli.watch { let output_format = determine_output_format(cli); From df592e60729d03a82a43f4f438dc170b9eee4dec Mon Sep 17 00:00:00 2001 From: Sascha <18143567+SaschaOnTour@users.noreply.github.com> Date: Sat, 13 Jun 2026 02:09:10 +0200 Subject: [PATCH 2/7] =?UTF-8?q?feat(explain):=20never=20dead-end=20?= =?UTF-8?q?=E2=80=94=20fallback=20usage=20guide=20+=20target-name=20hint?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --explain with an argument that is no rule id, not 'allow', and not a readable file now prints the three explain modes with copyable examples (and where rule ids come from) instead of stopping at the bare OS error. An argument that matches a qual:allow target name (the real flailing-agent guess: --explain boilerplate) gets a hint naming its dimension and the allow guide. Clap help for --explain now names all three modes. --- src/cli/explain.rs | 58 +++++++++++++++++++++++++++++------- src/cli/explain/tests/mod.rs | 37 +++++++++++++++++++++++ src/cli/mod.rs | 6 ++-- 3 files changed, 89 insertions(+), 12 deletions(-) diff --git a/src/cli/explain.rs b/src/cli/explain.rs index d986979a..17d0e0dd 100644 --- a/src/cli/explain.rs +++ b/src/cli/explain.rs @@ -40,6 +40,51 @@ pub(crate) fn rule_card_text(id: &str) -> Option { find_rule_card(id).map(render_rule_card) } +/// The `--explain` usage guide shown when the argument matches no mode — an +/// unknown argument must never dead-end without a next step. +/// Integration: optional target hint + the modes block. +pub(crate) fn explain_fallback_text(arg: &str) -> String { + let hint = target_name_hint(arg).unwrap_or_default(); + format!( + "`{arg}` is not a rule id, `allow`, or a readable file.\n{hint}{}", + EXPLAIN_MODES + ) +} + +const EXPLAIN_MODES: &str = "\n--explain modes:\n\ +\x20 rustqual --explain rule card: what it detects, why, how to fix,\n\ +\x20 suppression, config knob. Rule ids appear in\n\ +\x20 the findings output, e.g. --explain BP-009\n\ +\x20 rustqual --explain allow the qual:allow suppression guide\n\ +\x20 (grammar + per-dimension targets)\n\ +\x20 rustqual --explain architecture-rule diagnostics for one file\n"; + +/// All dimensions, for vocabulary scans. +const ALL_DIMS: [Dimension; 7] = [ + Dimension::Iosp, + Dimension::Complexity, + Dimension::Dry, + Dimension::Srp, + Dimension::Coupling, + Dimension::TestQuality, + Dimension::Architecture, +]; + +/// A hint line when `arg` is a suppression *target* name (`boilerplate`, +/// `untested`, …) — a real flailing-agent guess: the word belongs to the +/// qual:allow vocabulary, not the rule catalog. Operation: vocabulary scan. +fn target_name_hint(arg: &str) -> Option { + let dim = ALL_DIMS.iter().find(|d| { + target_names(**d) + .iter() + .any(|t| t.eq_ignore_ascii_case(arg)) + })?; + Some(format!( + "Note: `{arg}` is a suppression target of the `{dim}` dimension — \ + usable as // qual:allow({dim}, {arg}); see rustqual --explain allow.\n" + )) +} + /// Format one rule card for the terminal: title line + the five sections a /// consumer needs to act (what fired, why, fix, suppression, config knob). /// Operation: string assembly. @@ -85,16 +130,8 @@ finding first; suppression is the last resort.\n"; /// Operation: header + per-dimension blocks + footer via a closure. pub(crate) fn suppression_guide() -> String { let mut out = String::from(GUIDE_HEADER); - let dims = [ - Dimension::Iosp, - Dimension::Complexity, - Dimension::Dry, - Dimension::Srp, - Dimension::Coupling, - Dimension::TestQuality, - Dimension::Architecture, - ]; - dims.iter() + ALL_DIMS + .iter() .for_each(|&dim| out.push_str(&dim_targets_block(dim))); out.push_str(GUIDE_FOOTER); out @@ -143,6 +180,7 @@ pub fn handle_explain(target: &Path, config: &Config) -> Result<(), i32> { Ok(s) => s, Err(e) => { eprintln!("Error reading {}: {e}", target.display()); + eprintln!("{}", explain_fallback_text(&target.to_string_lossy())); return Err(1); } }; diff --git a/src/cli/explain/tests/mod.rs b/src/cli/explain/tests/mod.rs index 9cd35010..b27709b2 100644 --- a/src/cli/explain/tests/mod.rs +++ b/src/cli/explain/tests/mod.rs @@ -54,3 +54,40 @@ fn rule_card_text_is_none_for_unknown_id() { assert!(super::rule_card_text("BP-999").is_none()); assert!(super::rule_card_text("src/lib.rs").is_none()); } + +#[test] +fn fallback_text_lists_all_three_modes_with_examples() { + // The no-match case must never dead-end: it names the argument and shows + // every mode with a copyable example, including where rule ids come from. + let g = super::explain_fallback_text("BP-999"); + assert!(g.contains("BP-999"), "must name the unrecognized argument"); + assert!(g.contains("--explain allow"), "must show the allow mode"); + assert!( + g.contains("--explain BP-009"), + "must show a rule-id example: {g}" + ); + assert!( + g.contains(".rs"), + "must show the file-diagnostics mode: {g}" + ); + assert!( + g.contains("findings output"), + "must say where rule ids appear: {g}" + ); +} + +#[test] +fn fallback_text_hints_when_arg_is_a_suppression_target() { + // `--explain boilerplate` was a real flailing agent's guess: the word is + // a suppression target, not a rule id — say so and point at both paths. + let g = super::explain_fallback_text("boilerplate"); + assert!( + g.contains("suppression target"), + "must identify the arg as a target name: {g}" + ); + assert!(g.contains("dry"), "must name the owning dimension: {g}"); + assert!( + g.contains("--explain allow"), + "must point at the guide: {g}" + ); +} diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 289b1a7f..2e32d2af 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -132,7 +132,9 @@ pub(crate) struct Cli { #[arg(long, value_name = "LCOV_FILE")] pub coverage: Option, - /// Diagnostic mode: explain architecture-rule classification for one file. - #[arg(long, value_name = "FILE")] + /// Explain one thing: a rule id ("BP-009" prints its rule card), "allow" + /// (the qual:allow suppression guide), or a .rs file (architecture-rule + /// classification for that file). + #[arg(long, value_name = "RULE-ID|allow|FILE")] pub explain: Option, } From d3cb5f8f675898d7b7a88141e9da951e1d8d438d Mon Sep 17 00:00:00 2001 From: Sascha <18143567+SaschaOnTour@users.noreply.github.com> Date: Sat, 13 Jun 2026 02:12:20 +0200 Subject: [PATCH 3/7] feat(report): tail-proof footer + rule titles in compact finding lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The findings epilogue now names both next steps — rule cards (rustqual --explain ) and suppression syntax (--explain allow) — and renders last, so it is what survives an agent's '| tail' truncation. Boilerplate entries in the compact findings/summary view carry the registry title (BP-009 Struct update boilerplate) instead of the bare id. --- .../report/findings_list/categories.rs | 13 +++++- .../report/tests/findings_list_categories.rs | 4 +- src/adapters/report/text/mod.rs | 11 +++-- src/adapters/report/text/tests/root.rs | 42 +++++++++++++++++++ 4 files changed, 64 insertions(+), 6 deletions(-) diff --git a/src/adapters/report/findings_list/categories.rs b/src/adapters/report/findings_list/categories.rs index 4ab881d4..dcb9a8a9 100644 --- a/src/adapters/report/findings_list/categories.rs +++ b/src/adapters/report/findings_list/categories.rs @@ -63,7 +63,7 @@ pub(super) fn dry_category_detail(f: &DryFinding) -> (&'static str, String) { ("WILDCARD", module_path.clone()) } (DryFindingKind::Boilerplate, DryFindingDetails::Boilerplate { pattern_id, .. }) => { - ("BOILERPLATE", pattern_id.clone()) + ("BOILERPLATE", boilerplate_detail(pattern_id)) } (DryFindingKind::RepeatedMatch, DryFindingDetails::RepeatedMatch { enum_name, .. }) => { ("REPEATED_MATCH", enum_name.clone()) @@ -72,6 +72,17 @@ pub(super) fn dry_category_detail(f: &DryFinding) -> (&'static str, String) { } } +/// `BP-009 Struct update boilerplate` — id plus registry title. The compact +/// summary line is often the only view a consumer sees (agents tail the +/// output), so the bare pattern id is not enough to act on. +/// Operation: registry lookup with id fallback. +fn boilerplate_detail(pattern_id: &str) -> String { + match crate::domain::rule_cards::find_rule_card(pattern_id) { + Some(card) => format!("{} {}", card.id, card.title), + None => pattern_id.to_string(), + } +} + pub(super) fn srp_category_detail(f: &SrpFinding) -> (&'static str, String) { match (&f.kind, &f.details) { ( diff --git a/src/adapters/report/tests/findings_list_categories.rs b/src/adapters/report/tests/findings_list_categories.rs index 0b3a9f40..31b4a15e 100644 --- a/src/adapters/report/tests/findings_list_categories.rs +++ b/src/adapters/report/tests/findings_list_categories.rs @@ -140,7 +140,9 @@ fn dry_cases() -> Vec { suggestion: "x".into(), }, "BOILERPLATE", - "BP-007", + // id + registry title: the compact summary line is often the only + // view an agent sees, so the bare pattern id is not enough. + "BP-007 Error enum boilerplate (consider thiserror)", ), ( DryFindingKind::RepeatedMatch, diff --git a/src/adapters/report/text/mod.rs b/src/adapters/report/text/mod.rs index 3ca24955..00989ced 100644 --- a/src/adapters/report/text/mod.rs +++ b/src/adapters/report/text/mod.rs @@ -146,13 +146,16 @@ impl<'a> ReporterImpl for TextReporter<'a> { } /// Footer shown whenever findings remain: fixing is the expectation, -/// suppression the last resort. Points at `--explain allow` (the agent reaches -/// for the tool before the README), and deliberately prints no paste-ready -/// suppression so silencing isn't the path of least resistance. +/// suppression the last resort. Points at `--explain ` (the rule +/// card with fix forms) and `--explain allow` — it renders last, so it is +/// what survives an agent's `| tail` truncation. Deliberately prints no +/// paste-ready suppression so silencing isn't the path of least resistance. /// Operation: constant string. fn suppression_footer() -> String { "\n── Fix the findings above. Suppression is a last resort — it needs a written\n \ - reason and re-fires when the code worsens. Syntax: rustqual --explain allow\n" + reason and re-fires when the code worsens.\n \ + Rule details: rustqual --explain (e.g. rustqual --explain BP-009)\n \ + Suppression syntax: rustqual --explain allow\n" .to_string() } diff --git a/src/adapters/report/text/tests/root.rs b/src/adapters/report/text/tests/root.rs index f7a75310..a83bce9c 100644 --- a/src/adapters/report/text/tests/root.rs +++ b/src/adapters/report/text/tests/root.rs @@ -191,3 +191,45 @@ fn test_print_report_suppressed_verbose_marks_function() { "verbose mode must list the function even when suppressed; got {out}" ); } + +#[test] +fn footer_with_findings_points_at_rule_cards_and_allow_guide() { + // The epilogue is the only part guaranteed to survive an agent's + // `| tail -30` — it must carry the next step for BOTH paths: rule + // lookup (--explain ) and suppression syntax (--explain allow). + use crate::domain::{AnalysisData, AnalysisFindings}; + use crate::ports::Reporter; + use crate::report::findings_list::FindingEntry; + let entries = [FindingEntry::new( + "src/x.rs", + 1, + "BOILERPLATE", + "BP-009".into(), + "build_thing".into(), + )]; + let summary = Summary::from_results(&[]); + let reporter = TextReporter { + summary: &summary, + function_analyses: &[], + findings_entries: &entries, + verbose: false, + suggestions_text: None, + }; + let out = reporter.render(&AnalysisFindings::default(), &AnalysisData::default()); + assert!( + out.contains("rustqual --explain "), + "footer must point at rule cards: {out}" + ); + assert!( + out.contains("rustqual --explain allow"), + "footer must keep pointing at the allow guide: {out}" + ); + let findings_heading = out.find("Finding").expect("findings list rendered"); + let hint = out + .rfind("--explain ") + .expect("rule-card hint rendered"); + assert!( + hint > findings_heading, + "the explain hint must render after the findings list so tail keeps it" + ); +} From 580b866153018074455e037e4b3a0a33348dc4a3 Mon Sep 17 00:00:00 2001 From: Sascha <18143567+SaschaOnTour@users.noreply.github.com> Date: Sat, 13 Jun 2026 02:15:48 +0200 Subject: [PATCH 4/7] feat(config): [boilerplate].accepted_display_idioms with fail-loud vocabulary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New field declaring the project's trivial-Display house idioms (fixed vocabulary: write_str, write_char, write_macro, delegation; default empty). Validation runs at startup next to weights/suppression: an unknown entry is a config error naming the offender and the valid vocabulary — declared policy must parse or error, never silently mean something else. Not yet consumed by BP-002 (that lands with the semantic-matching slice). --- src/adapters/config/mod.rs | 21 +++++++++++++ src/adapters/config/sections.rs | 12 ++++++++ src/adapters/config/tests/root.rs | 51 +++++++++++++++++++++++++++++++ src/app/setup.rs | 5 +-- 4 files changed, 87 insertions(+), 2 deletions(-) diff --git a/src/adapters/config/mod.rs b/src/adapters/config/mod.rs index 1e308a7b..8c6c8966 100644 --- a/src/adapters/config/mod.rs +++ b/src/adapters/config/mod.rs @@ -219,6 +219,27 @@ pub fn validate_weights(config: &Config) -> Result<(), String> { Ok(()) } +/// Validate `[boilerplate].accepted_display_idioms` against the fixed idiom +/// vocabulary. Fail-loud by design: a typo like `"write_st"` would otherwise +/// accept nothing and report nothing — declared policy must either parse or +/// error, never silently mean something else. +/// Operation: vocabulary scan, no own calls. +pub fn validate_boilerplate(config: &Config) -> Result<(), String> { + let unknown = config + .boilerplate + .accepted_display_idioms + .iter() + .find(|idiom| !sections::DISPLAY_IDIOM_VOCABULARY.contains(&idiom.as_str())); + match unknown { + Some(bad) => Err(format!( + "[boilerplate].accepted_display_idioms contains unknown idiom '{bad}'. \ + Valid idioms: {}. Check rustqual.toml.", + sections::DISPLAY_IDIOM_VOCABULARY.join(", ") + )), + None => Ok(()), + } +} + /// Validate `[suppression]` settings. `pin_headroom` feeds the too-loose check /// `pin > value * (1 + headroom)`, so a non-finite value (`inf`/`NaN`, both /// accepted by TOML float parsing) would silently disable too-loose detection diff --git a/src/adapters/config/sections.rs b/src/adapters/config/sections.rs index 0c384b4a..6c69b3e8 100644 --- a/src/adapters/config/sections.rs +++ b/src/adapters/config/sections.rs @@ -147,6 +147,13 @@ impl Default for DuplicatesConfig { } } +/// The fixed vocabulary for `[boilerplate].accepted_display_idioms` — the +/// trivial-Display forms a project may declare as its house idiom (BP-002 +/// then treats them as policy, not boilerplate). Validation is fail-loud: +/// an entry outside this list is a config error, never a silent no-op. +pub const DISPLAY_IDIOM_VOCABULARY: &[&str] = + &["write_str", "write_char", "write_macro", "delegation"]; + /// Configuration for boilerplate detection. #[derive(Debug, Deserialize, Clone)] #[serde(default, deny_unknown_fields)] @@ -154,6 +161,10 @@ pub struct BoilerplateConfig { pub enabled: bool, pub patterns: Vec, pub suggest_crates: bool, + /// Trivial-Display forms declared as the project's house idiom; BP-002 + /// does not flag an fmt body whose every write op is an accepted idiom. + /// Entries must come from [`DISPLAY_IDIOM_VOCABULARY`]. + pub accepted_display_idioms: Vec, } impl Default for BoilerplateConfig { @@ -162,6 +173,7 @@ impl Default for BoilerplateConfig { enabled: DEFAULT_BOILERPLATE_ENABLED, patterns: vec![], suggest_crates: true, + accepted_display_idioms: vec![], } } } diff --git a/src/adapters/config/tests/root.rs b/src/adapters/config/tests/root.rs index 8b6cc033..1f00f251 100644 --- a/src/adapters/config/tests/root.rs +++ b/src/adapters/config/tests/root.rs @@ -173,3 +173,54 @@ fn test_validate_suppression_rejects_non_finite_and_negative() { cfg.suppression.pin_headroom = 0.0; assert!(validate_suppression(&cfg).is_ok()); } + +#[test] +fn test_validate_boilerplate_default_ok() { + assert!(validate_boilerplate(&Config::default()).is_ok()); +} + +#[test] +fn test_validate_boilerplate_rejects_unknown_idiom_fail_loud() { + // A typo like "write_st" would otherwise accept nothing and report + // nothing — the silent-config failure mode. The error must name the + // offender AND list the valid vocabulary so the fix is copyable. + let mut cfg = Config::default(); + cfg.boilerplate.accepted_display_idioms = vec!["write_st".into()]; + let result = validate_boilerplate(&cfg); + assert!(result.is_err(), "unknown idiom must be a config error"); + let msg = result.unwrap_err(); + assert!(msg.contains("write_st"), "must name the offender: {msg}"); + assert!(msg.contains("write_str"), "must list the vocabulary: {msg}"); + assert!( + msg.contains("delegation"), + "must list the vocabulary: {msg}" + ); +} + +#[test] +fn test_validate_boilerplate_accepts_full_vocabulary() { + let mut cfg = Config::default(); + cfg.boilerplate.accepted_display_idioms = + ["write_str", "write_char", "write_macro", "delegation"] + .iter() + .map(|s| s.to_string()) + .collect(); + assert!(validate_boilerplate(&cfg).is_ok()); +} + +#[test] +fn test_boilerplate_accepted_display_idioms_parse_and_default_empty() { + let cfg: Config = + toml::from_str("[boilerplate]\naccepted_display_idioms = [\"write_str\"]\n").unwrap(); + assert_eq!( + cfg.boilerplate.accepted_display_idioms, + vec!["write_str".to_string()] + ); + assert!( + Config::default() + .boilerplate + .accepted_display_idioms + .is_empty(), + "default must be empty (all trivial forms flagged)" + ); +} diff --git a/src/app/setup.rs b/src/app/setup.rs index 7d04e1a4..b09a401d 100644 --- a/src/app/setup.rs +++ b/src/app/setup.rs @@ -48,11 +48,12 @@ fn load_auto_config(path: &Path) -> Result { Config::load(path).map_err(report_config_error) } -/// Validate config invariants (weight sum, suppression headroom). -/// Integration: delegates to the per-section validators. +/// Validate config invariants (weight sum, suppression headroom, boilerplate +/// idiom vocabulary). Integration: delegates to the per-section validators. fn validate_config(config: &Config) -> Result<(), i32> { config::validate_weights(config) .and_then(|()| config::validate_suppression(config)) + .and_then(|()| config::validate_boilerplate(config)) .map_err(report_config_error) } From 037acbc74da01d4e6746801a46a7a9be54688ee4 Mon Sep 17 00:00:00 2001 From: Sascha <18143567+SaschaOnTour@users.noreply.github.com> Date: Sat, 13 Jun 2026 02:25:52 +0200 Subject: [PATCH 5/7] feat(bp002)!: semantic trivial-Display matching + accepted-idiom policy gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BP-002 now matches what the fmt body MEANS, not which syntax it uses: any branch-free body consisting only of formatter write ops (write!/writeln!, f.write_str, f.write_char, Display::fmt delegation), multi-statement included — so the observed evasion (one write! split into two write_char statements) is the same finding, and the semantically identical write_str newtype form no longer slips through unmatched. [boilerplate].accepted_display_idioms gates findings as declared policy: a body whose every used idiom is accepted is silent, any other trivial form keeps firing — the rule re-targets to house-idiom consistency enforcement instead of going quiet. The suggestion (and the BP-002 rule card) always names all three fix forms regardless of workspace deps: derive (derive_more::Display under suggest_crates), the accepted-idiom config, and a local macro_rules! as the dependency-free DRY option. Behavior change: projects with hand-rolled write_str/write_char/delegation Displays get new BP-002 findings until they pick a form (minor bump follows in the release slice). --- .../boilerplate/tests/root/bp002_semantics.rs | 213 +++++++++++++++ .../dry/boilerplate/tests/root/mod.rs | 1 + .../dry/boilerplate/trivial_display.rs | 249 ++++++++++++++---- src/domain/rule_cards/boilerplate.rs | 22 +- 4 files changed, 433 insertions(+), 52 deletions(-) create mode 100644 src/adapters/analyzers/dry/boilerplate/tests/root/bp002_semantics.rs diff --git a/src/adapters/analyzers/dry/boilerplate/tests/root/bp002_semantics.rs b/src/adapters/analyzers/dry/boilerplate/tests/root/bp002_semantics.rs new file mode 100644 index 00000000..82e42267 --- /dev/null +++ b/src/adapters/analyzers/dry/boilerplate/tests/root/bp002_semantics.rs @@ -0,0 +1,213 @@ +//! BP-002 semantic-triviality tests: the check matches what the body *means* +//! (branch-free, write-only fmt), not which surface syntax it uses — plus the +//! accepted_display_idioms policy gate and the all-fix-forms suggestion. + +use super::*; + +/// A `BoilerplateConfig` with the given accepted Display idioms. +fn cfg_accepting(idioms: &[&str]) -> BoilerplateConfig { + BoilerplateConfig { + accepted_display_idioms: idioms.iter().map(|s| s.to_string()).collect(), + ..Default::default() + } +} + +const WRITE_STR_NEWTYPE: &str = r#" + use std::fmt; + struct Id(String); + impl fmt::Display for Id { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(&self.0) + } + } +"#; + +const WRITE_MACRO_NEWTYPE: &str = r#" + use std::fmt; + struct Id(String); + impl fmt::Display for Id { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.0) + } + } +"#; + +#[test] +fn test_bp002_write_str_newtype_detected() { + // f.write_str(&self.0) is semantically the same trivial Display as + // write!(f, "{}", self.0) — surface syntax must not decide the finding. + let findings = detect_boilerplate(&parse(WRITE_STR_NEWTYPE), &BoilerplateConfig::default()); + assert!( + findings.iter().any(|f| f.pattern_id == "BP-002"), + "a write_str-only fmt body is trivial Display" + ); +} + +#[test] +fn test_bp002_multi_statement_write_char_body_detected() { + // The mechanical evasion rewrite observed in the wild: one write! split + // into two write_char statements. Still branch-free, still write-only — + // must still be BP-002 so the rule forces a decision, not a workaround. + let code = r#" + use std::fmt; + struct Brackets; + impl fmt::Display for Brackets { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_char('[')?; + f.write_char(']') + } + } + "#; + let findings = detect_boilerplate(&parse(code), &BoilerplateConfig::default()); + assert!( + findings.iter().any(|f| f.pattern_id == "BP-002"), + "a multi-statement write_char body is still trivial Display" + ); +} + +#[test] +fn test_bp002_accepted_idiom_is_policy_not_finding() { + // Declaring write_str as the house idiom silences exactly that form … + let cfg = cfg_accepting(&["write_str"]); + let findings = detect_boilerplate(&parse(WRITE_STR_NEWTYPE), &cfg); + assert!( + !findings.iter().any(|f| f.pattern_id == "BP-002"), + "the declared house idiom is policy, not boilerplate" + ); + // … while the write! form keeps firing under the same config — the rule + // re-targets to consistency enforcement instead of going silent. + let findings = detect_boilerplate(&parse(WRITE_MACRO_NEWTYPE), &cfg); + assert!( + findings.iter().any(|f| f.pattern_id == "BP-002"), + "a non-accepted trivial form must still fire" + ); +} + +#[test] +fn test_bp002_mixed_idioms_fire_unless_all_accepted() { + let code = r#" + use std::fmt; + struct Tagged(String); + impl fmt::Display for Tagged { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str("tag:")?; + write!(f, "{}", self.0) + } + } + "#; + let only_write_str = cfg_accepting(&["write_str"]); + let findings = detect_boilerplate(&parse(code), &only_write_str); + assert!( + findings.iter().any(|f| f.pattern_id == "BP-002"), + "a body mixing an accepted and a non-accepted idiom must fire" + ); + let both = cfg_accepting(&["write_str", "write_macro"]); + let findings = detect_boilerplate(&parse(code), &both); + assert!( + !findings.iter().any(|f| f.pattern_id == "BP-002"), + "every used idiom accepted ⇒ policy, no finding" + ); +} + +#[test] +fn test_bp002_delegation_form_detected_and_acceptable() { + let code = r#" + use std::fmt; + struct Wrapper(inner::Type); + impl fmt::Display for Wrapper { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Display::fmt(&self.0, f) + } + } + "#; + let findings = detect_boilerplate(&parse(code), &BoilerplateConfig::default()); + assert!( + findings.iter().any(|f| f.pattern_id == "BP-002"), + "a pure Display::fmt delegation is trivial Display" + ); + let findings = detect_boilerplate(&parse(code), &cfg_accepting(&["delegation"])); + assert!( + !findings.iter().any(|f| f.pattern_id == "BP-002"), + "delegation declared as house idiom ⇒ no finding" + ); +} + +#[test] +fn test_bp002_local_binding_makes_body_non_trivial() { + // A let-binding means the body computes something — not pure write-only + // boilerplate, so the (conservative) check stays silent. + let code = r#" + use std::fmt; + struct Id(String); + impl fmt::Display for Id { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let upper = self.0.to_uppercase(); + f.write_str(&upper) + } + } + "#; + let findings = detect_boilerplate(&parse(code), &BoilerplateConfig::default()); + assert!( + !findings.iter().any(|f| f.pattern_id == "BP-002"), + "a body with a local binding is not trivial" + ); +} + +#[test] +fn test_bp002_write_on_non_formatter_receiver_not_trivial() { + // self.buf.write_str(…) writes somewhere else — that is real logic, not + // a formatter-forwarding idiom; guards the receiver check. + let code = r#" + use std::fmt; + struct Tee { buf: String } + impl fmt::Display for Tee { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.buf.write_str("x") + } + } + "#; + let findings = detect_boilerplate(&parse(code), &BoilerplateConfig::default()); + assert!( + !findings.iter().any(|f| f.pattern_id == "BP-002"), + "write_str on a non-formatter receiver is not the trivial idiom" + ); +} + +#[test] +fn test_bp002_suggestion_names_all_fix_forms() { + // Workspace availability ≠ usability under a dependency policy: the + // suggestion must always name every fix form so the consumer can pick + // the one their project allows. + let findings = detect_boilerplate(&parse(WRITE_MACRO_NEWTYPE), &BoilerplateConfig::default()); + let bp002 = findings + .iter() + .find(|f| f.pattern_id == "BP-002") + .expect("BP-002 fires"); + for needle in ["derive_more", "accepted_display_idioms", "macro_rules"] { + assert!( + bp002.suggestion.contains(needle), + "suggestion must name fix form `{needle}`: {}", + bp002.suggestion + ); + } + // suggest_crates = false drops the crate name but keeps the derive form. + let no_crates = BoilerplateConfig { + suggest_crates: false, + ..Default::default() + }; + let findings = detect_boilerplate(&parse(WRITE_MACRO_NEWTYPE), &no_crates); + let bp002 = findings + .iter() + .find(|f| f.pattern_id == "BP-002") + .expect("BP-002 fires"); + assert!( + !bp002.suggestion.contains("derive_more"), + "suggest_crates = false must not name crates: {}", + bp002.suggestion + ); + assert!( + bp002.suggestion.to_lowercase().contains("derive"), + "the derive form itself must stay named: {}", + bp002.suggestion + ); +} diff --git a/src/adapters/analyzers/dry/boilerplate/tests/root/mod.rs b/src/adapters/analyzers/dry/boilerplate/tests/root/mod.rs index d429cf1a..5b9206e2 100644 --- a/src/adapters/analyzers/dry/boilerplate/tests/root/mod.rs +++ b/src/adapters/analyzers/dry/boilerplate/tests/root/mod.rs @@ -6,6 +6,7 @@ pub(super) use crate::adapters::analyzers::dry::boilerplate::*; pub(super) use crate::config::sections::BoilerplateConfig; mod bp001_to_005; +mod bp002_semantics; mod bp006_to_008; mod bp009_onward; mod bp_predicates; diff --git a/src/adapters/analyzers/dry/boilerplate/trivial_display.rs b/src/adapters/analyzers/dry/boilerplate/trivial_display.rs index 56376f48..aa04335b 100644 --- a/src/adapters/analyzers/dry/boilerplate/trivial_display.rs +++ b/src/adapters/analyzers/dry/boilerplate/trivial_display.rs @@ -1,24 +1,39 @@ +//! BP-002: trivial `Display` — semantic matching. A body is trivial when it +//! is branch-free and every statement is a formatter write op: `write!` / +//! `writeln!`, `f.write_str(…)`, `f.write_char(…)`, or a `Display::fmt` +//! delegation. Multi-statement bodies count too, so the rule cannot be +//! dodged by splitting one `write!` into two `write_char` statements — the +//! historical evasion this check was blind to when it matched surface +//! syntax (one `write!` macro) instead of meaning. +//! +//! `[boilerplate].accepted_display_idioms` turns forms into declared policy: +//! a body whose every used idiom is accepted is no finding, while any other +//! trivial form keeps firing — the rule then enforces the house idiom +//! instead of going silent. + +use std::collections::BTreeSet; use syn::spanned::Spanned; -use super::{self_type_of, single_return_expr, trait_name_of, BoilerplateFind}; +use super::{self_type_of, trait_name_of, BoilerplateFind}; use crate::config::sections::BoilerplateConfig; -/// Check whether an expression is a `write!` or `writeln!` macro call. -/// Operation: AST pattern matching logic, no own calls. -fn is_write_macro(expr: &syn::Expr) -> bool { - if let syn::Expr::Macro(m) = expr { - m.mac - .path - .segments - .last() - .map(|s| s.ident == "write" || s.ident == "writeln") - .unwrap_or(false) - } else { - false - } +/// One recognized statement in a trivial fmt body: a write op tagged with +/// its idiom (vocabulary name), or neutral filler (`Ok(())`). +enum WriteOp { + Idiom(&'static str), + Neutral, } -/// Detect trivial `impl Display` with a single write!/writeln! call. +const SUGGEST_WITH_CRATES: &str = "Derive it (derive_more::Display), declare the \ + project's house idiom in [boilerplate] accepted_display_idioms, or generate \ + the impls with one local macro_rules! — details: rustqual --explain BP-002"; + +const SUGGEST_NEUTRAL: &str = "Derive it with a derive macro, declare the \ + project's house idiom in [boilerplate] accepted_display_idioms, or generate \ + the impls with one local macro_rules! — details: rustqual --explain BP-002"; + +/// Detect trivial `impl Display` bodies (branch-free, write-only) whose +/// idioms are not all declared as accepted policy. /// Operation: AST pattern matching logic; helper calls in closures. pub(super) fn check_trivial_display( parsed: &[(String, String, syn::File)], @@ -26,17 +41,16 @@ pub(super) fn check_trivial_display( ) -> Vec { pattern_guard!("BP-002", config); let suggest = if config.suggest_crates { - "Consider using derive_more::Display" + SUGGEST_WITH_CRATES } else { - "Consider using a derive macro for simple Display implementations" + SUGGEST_NEUTRAL }; - let is_write = |e: &syn::Expr| is_write_macro(e); + let accepted = &config.accepted_display_idioms; parsed .iter() .flat_map(|(file, _, syntax)| { syntax.items.iter().filter_map({ let file = file.clone(); - let suggest = suggest.to_string(); move |item| { let syn::Item::Impl(imp) = item else { return None; @@ -44,36 +58,181 @@ pub(super) fn check_trivial_display( if trait_name_of(imp)? != "Display" { return None; } - let methods: Vec<_> = imp - .items - .iter() - .filter_map(|i| { - if let syn::ImplItem::Fn(m) = i { - Some(m) - } else { - None - } - }) - .collect(); - if methods.len() != 1 || methods[0].sig.ident != "fmt" { - return None; - } - let expr = single_return_expr(&methods[0].block)?; - if !is_write(expr) { + let idioms = trivial_display_idioms(single_fmt_method(imp)?)?; + if idioms.iter().all(|i| accepted.iter().any(|a| a == i)) { return None; } - Some(BoilerplateFind { - pattern_id: "BP-002".to_string(), - file: file.clone(), - line: imp.self_ty.span().start().line, - struct_name: self_type_of(imp), - description: "Trivial Display implementation with a single write! call" - .to_string(), - suggestion: suggest.clone(), - suppressed: false, - }) + Some(build_find(imp, &idioms, &file, suggest)) } }) }) .collect() } + +/// Build the BP-002 find for one trivial impl, naming the idioms it uses. +/// Operation: struct construction. +fn build_find( + imp: &syn::ItemImpl, + idioms: &BTreeSet<&'static str>, + file: &str, + suggest: &str, +) -> BoilerplateFind { + let used: Vec<&str> = idioms.iter().copied().collect(); + BoilerplateFind { + pattern_id: "BP-002".to_string(), + file: file.to_string(), + line: imp.self_ty.span().start().line, + struct_name: self_type_of(imp), + description: format!( + "Trivial Display implementation (branch-free, {} only)", + used.join("/") + ), + suggestion: suggest.to_string(), + suppressed: false, + } +} + +/// The impl's single method, when it is named `fmt`. +/// Operation: iterator logic, no own calls. +fn single_fmt_method(imp: &syn::ItemImpl) -> Option<&syn::ImplItemFn> { + let mut fns = imp.items.iter().filter_map(|i| match i { + syn::ImplItem::Fn(m) => Some(m), + _ => None, + }); + let first = fns.next()?; + if fns.next().is_some() || first.sig.ident != "fmt" { + return None; + } + Some(first) +} + +/// The set of write idioms a trivial fmt body uses — `None` when any +/// statement is not a recognized write op (branching, locals, computation): +/// the conservative direction, the check stays silent. +/// Integration: formatter lookup + idiom collection. +fn trivial_display_idioms(method: &syn::ImplItemFn) -> Option> { + formatter_ident(method).and_then(|fmt| collect_idioms(&method.block, &fmt)) +} + +/// The formatter parameter's ident (the second fmt argument, usually `f`). +/// Operation: signature pattern match, no own calls. +fn formatter_ident(method: &syn::ImplItemFn) -> Option { + let syn::FnArg::Typed(pat_type) = method.sig.inputs.iter().nth(1)? else { + return None; + }; + let syn::Pat::Ident(pi) = &*pat_type.pat else { + return None; + }; + Some(pi.ident.to_string()) +} + +/// Collect the idioms of every statement, or `None` on the first statement +/// that is not a recognized write op. +/// Operation: iterator over statements; classification calls in closures. +fn collect_idioms(block: &syn::Block, fmt_ident: &str) -> Option> { + let ops: Option> = block + .stmts + .iter() + .map(|s| classify_stmt(s, fmt_ident)) + .collect(); + let idioms: BTreeSet<&'static str> = ops? + .into_iter() + .filter_map(|op| match op { + WriteOp::Idiom(i) => Some(i), + WriteOp::Neutral => None, + }) + .collect(); + (!idioms.is_empty()).then_some(idioms) +} + +/// Classify one statement as a write op. +/// Integration: dispatch on statement shape. +fn classify_stmt(stmt: &syn::Stmt, fmt_ident: &str) -> Option { + match stmt { + syn::Stmt::Expr(e, _) => classify_expr(unwrap_try(e), fmt_ident), + syn::Stmt::Macro(m) => classify_macro(&m.mac), + _ => None, + } +} + +/// Strip any `?` layers: `f.write_char('[')?` classifies like the inner call. +/// Operation: loop unwrap, no own calls. +fn unwrap_try(expr: &syn::Expr) -> &syn::Expr { + let mut e = expr; + while let syn::Expr::Try(t) = e { + e = &t.expr; + } + e +} + +/// Classify one expression as a write op. +/// Integration: dispatch on expression shape. +fn classify_expr(expr: &syn::Expr, fmt_ident: &str) -> Option { + match expr { + syn::Expr::Macro(m) => classify_macro(&m.mac), + syn::Expr::MethodCall(mc) => classify_method_call(mc, fmt_ident), + syn::Expr::Call(c) => classify_call(c, fmt_ident), + _ => None, + } +} + +/// `write!` / `writeln!` → the `write_macro` idiom. +/// Operation: path check, no own calls. +fn classify_macro(mac: &syn::Macro) -> Option { + let last = mac.path.segments.last()?; + if last.ident == "write" || last.ident == "writeln" { + Some(WriteOp::Idiom("write_macro")) + } else { + None + } +} + +/// `f.write_str(…)` / `f.write_char(…)` on the formatter, or `self.x.fmt(f)` +/// delegation. A write on any other receiver is NOT the trivial idiom — it +/// writes somewhere else, which is real logic. +/// Operation: method-shape checks; receiver test in a closure. +fn classify_method_call(mc: &syn::ExprMethodCall, fmt_ident: &str) -> Option { + let is_fmt = |e: &syn::Expr| is_path_ident(e, fmt_ident); + let method = mc.method.to_string(); + if method == "write_str" && is_fmt(&mc.receiver) { + return Some(WriteOp::Idiom("write_str")); + } + if method == "write_char" && is_fmt(&mc.receiver) { + return Some(WriteOp::Idiom("write_char")); + } + if method == "fmt" && mc.args.len() == 1 && is_fmt(&mc.args[0]) { + return Some(WriteOp::Idiom("delegation")); + } + None +} + +/// `Display::fmt(&self.x, f)` → delegation; `Ok(())` → neutral filler. +/// Operation: path/arg-shape checks; formatter test in a closure. +fn classify_call(call: &syn::ExprCall, fmt_ident: &str) -> Option { + let is_fmt = |e: &syn::Expr| is_path_ident(e, fmt_ident); + let syn::Expr::Path(p) = &*call.func else { + return None; + }; + let segs: Vec<&syn::Ident> = p.path.segments.iter().map(|s| &s.ident).collect(); + let unit_arg = matches!(call.args.first(), Some(syn::Expr::Tuple(t)) if t.elems.is_empty()); + if segs.last().is_some_and(|s| *s == "Ok") && call.args.len() == 1 && unit_arg { + return Some(WriteOp::Neutral); + } + let display_fmt = + segs.len() >= 2 && *segs[segs.len() - 2] == "Display" && *segs[segs.len() - 1] == "fmt"; + if display_fmt && call.args.len() == 2 && is_fmt(&call.args[1]) { + return Some(WriteOp::Idiom("delegation")); + } + None +} + +/// True when `expr` is exactly the ident `name`, possibly `&`-referenced. +/// Operation: shape match, no own calls. +fn is_path_ident(expr: &syn::Expr, name: &str) -> bool { + let inner = if let syn::Expr::Reference(r) = expr { + &*r.expr + } else { + expr + }; + matches!(inner, syn::Expr::Path(p) if p.path.is_ident(name)) +} diff --git a/src/domain/rule_cards/boilerplate.rs b/src/domain/rule_cards/boilerplate.rs index 0cc4bacc..991d902d 100644 --- a/src/domain/rule_cards/boilerplate.rs +++ b/src/domain/rule_cards/boilerplate.rs @@ -27,17 +27,25 @@ pub(super) const CARDS: &[RuleCard] = &[ RuleCard { id: "BP-002", title: "Trivial Display implementation (derivable)", - detects: "An impl Display whose single fmt method is one \ - write!/writeln! call with no further logic.", + detects: "An impl Display whose fmt body is branch-free and consists \ + only of formatter write ops — write!/writeln!, f.write_str, \ + f.write_char, or Display::fmt delegation — in any number of \ + statements. Matching is semantic, so rewriting one write! into \ + two write_char statements is still the same finding.", why: "The same effect is available declaratively; N hand-rolled \ trivial Displays drift in style and bury the one impl that \ actually formats.", - fix: "derive_more::Display with #[display(\"…\")], or a project-wide \ - dependency-free idiom (e.g. f.write_str(&self.0) for \ - string-backed newtypes, or one local macro_rules! emitting the \ - impl for all newtypes).", + fix: "Three forms — pick what your dependency policy allows: (1) \ + derive_more::Display with #[display(\"…\")]; (2) declare the \ + project's house idiom in [boilerplate] accepted_display_idioms \ + (e.g. \"write_str\") so the trivial impl becomes stated policy; \ + (3) one local macro_rules! emitting the impl for all newtypes \ + (dependency-free, single definition point).", suppress: BP_SUPPRESS, - config: BP_CONFIG, + config: "[boilerplate] accepted_display_idioms (write_str, \ + write_char, write_macro, delegation) declares house idioms; \ + patterns can disable BP-002 entirely; suggest_crates toggles \ + crate recommendations.", }, RuleCard { id: "BP-003", From 93075204e81a60b42cb705f5ed938720766667ac Mon Sep 17 00:00:00 2001 From: Sascha <18143567+SaschaOnTour@users.noreply.github.com> Date: Sat, 13 Jun 2026 02:31:38 +0200 Subject: [PATCH 6/7] =?UTF-8?q?docs:=201.6.0=20=E2=80=94=20changelog,=20co?= =?UTF-8?q?nfig/book=20reference,=20CLAUDE.md=20design=20notes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CHANGELOG entry for the self-explaining-CLI + semantic-BP-002 release; accepted_display_idioms documented in book/reference-configuration.md, rustqual.toml, and the BP-002 rows of reference-rules.md / code-reuse.md; reference-rules.md points at --explain ; CLAUDE.md records the rule-card and BP-002 design decisions. Version bump to 1.6.0 (two new features + a finding-set expansion — not a patch). --- CHANGELOG.md | 56 +++++++++++++++++++++++++++++++++ Cargo.lock | 2 +- Cargo.toml | 2 +- book/code-reuse.md | 2 +- book/reference-configuration.md | 19 +++++++++++ book/reference-rules.md | 12 +++++-- rustqual.toml | 6 ++++ 7 files changed, 93 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 273cb737..a527745d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,62 @@ 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.6.0] - 2026-06-13 + +Release making rustqual **self-explaining for human and agent consumers** +(rule cards, `--explain `, a never-dead-end CLI) and making **BP-002 +honest**: trivial-`Display` detection is now semantic (what the body means) +instead of syntactic (which macro it uses), with a config-declared house-idiom +policy. Driven by two field reports: an agent flailing through +`--explain BP-009` → `os error 2` with no next step, and a BP-002 finding +"fixed" by mechanically rewriting one `write!` into two `write_char` calls — +syntax-based matching invited evasion instead of a decision. + +### Added +- **Rule cards — one registry, every reporter** (`src/domain/rule_cards/`). + One card per catalog rule (id, title, what it detects, why it matters, fix + forms, the copyable suppression marker, the governing config knob). The + SARIF `tool.driver.rules` table renders from it (a sync test pins set + equality), `--explain` renders from it, and the compact findings view takes + its titles from it. +- **`--explain `** prints the rule card, case-insensitively + (`rustqual --explain bp-009` works). The natural first guess of every + consumer now succeeds. +- **`--explain` never dead-ends.** An argument that is no rule id, not + `allow`, and not a readable file prints the three explain modes with + copyable examples instead of stopping at the bare OS error; an argument + matching a `qual:allow` *target* name (`--explain boilerplate`) gets a hint + naming its dimension and the allow guide. The clap help text names all + three modes. +- **`[boilerplate].accepted_display_idioms`** (fixed vocabulary: `write_str`, + `write_char`, `write_macro`, `delegation`; default empty) declares the + project's trivial-`Display` house idiom as policy. Validation is fail-loud + at startup: an unknown entry is a config error naming the offender and the + valid vocabulary — never a silent no-op. + +### Changed +- **BP-002 matches semantically.** Any branch-free `fmt` body consisting only + of formatter write ops — `write!`/`writeln!`, `f.write_str`, `f.write_char`, + `Display::fmt` delegation — is trivial, multi-statement bodies included. + The evasion rewrite (two `write_char` statements) is the same finding; the + semantically identical `f.write_str(&self.0)` newtype form no longer slips + through unmatched. With accepted idioms declared, the rule enforces + house-idiom consistency (a stray `write!` still fires) instead of going + silent. *Projects with hand-rolled write-only Displays get new BP-002 + findings until they pick a form — declare the idiom, derive, or generate + via a local `macro_rules!`.* +- **BP-002's suggestion names all fix forms** regardless of workspace + dependencies (availability ≠ usability under a dependency policy): derive + (`derive_more::Display` when `suggest_crates`), the accepted-idiom config, + and a local `macro_rules!` as the dependency-free DRY option; it ends with + `rustqual --explain BP-002`. +- **The findings epilogue is tail-proof.** It now names both next steps — + `rustqual --explain ` (rule details) and `rustqual --explain allow` + (suppression syntax) — and renders last, so it survives an agent's + `| tail` truncation. Boilerplate entries in the compact findings view carry + the registry title (`BP-009 Struct update boilerplate`) instead of the bare + pattern id. + ## [1.5.1] - 2026-06-09 Release fixing a class of **macro-token blindness** that ran across the diff --git a/Cargo.lock b/Cargo.lock index 38816cca..585c987b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -704,7 +704,7 @@ dependencies = [ [[package]] name = "rustqual" -version = "1.5.1" +version = "1.6.0" dependencies = [ "clap", "clap_complete", diff --git a/Cargo.toml b/Cargo.toml index e0fc44d3..581ceed5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "rustqual" -version = "1.5.1" +version = "1.6.0" edition = "2021" description = "Comprehensive Rust code quality analyzer — seven dimensions: IOSP, Complexity, DRY, SRP, Coupling, Test Quality, Architecture" license = "MIT" diff --git a/book/code-reuse.md b/book/code-reuse.md index e8a69af7..43eae3ed 100644 --- a/book/code-reuse.md +++ b/book/code-reuse.md @@ -26,7 +26,7 @@ rustqual covers four families: | `DRY-004` | Wildcard import (`use module::*;`) | | `DRY-005` | Repeated match pattern across functions (≥3 arms identical, ≥3 instances) | | `BP-001` | Trivial `From` impl (derivable) | -| `BP-002` | Trivial `Display` impl | +| `BP-002` | Trivial `Display` impl — semantic: any branch-free, write-only `fmt` body; house idioms declarable via `[boilerplate].accepted_display_idioms` | | `BP-003` | Trivial getter/setter (consider field visibility) | | `BP-004` | Builder pattern — consider `derive_builder` or similar | | `BP-005` | Manual `Default` impl (derivable) | diff --git a/book/reference-configuration.md b/book/reference-configuration.md index 218b071e..d6de9da5 100644 --- a/book/reference-configuration.md +++ b/book/reference-configuration.md @@ -59,10 +59,29 @@ enabled = true ```toml [boilerplate] enabled = true +# patterns = ["BP-002", "BP-007"] # enable-list; empty = all BP-* active +# suggest_crates = true # name crates (derive_more, thiserror) in suggestions +# accepted_display_idioms = ["write_str"] # declare the trivial-Display house idiom ``` The full `BP-*` family. Disable if your project deliberately avoids derive macros. +`accepted_display_idioms` declares which trivial-`Display` forms are your +project's *stated policy* rather than boilerplate — useful under a +dependency policy that rules out `derive_more`. BP-002 matches semantically +(any branch-free, write-only `fmt` body); a body whose every used idiom is +accepted is silent, while any other trivial form keeps firing, so the rule +enforces the declared idiom instead of going quiet. Fixed vocabulary +(validated fail-loud at startup — a typo is a config error, not a silent +no-op): + +| Idiom | Covers | +|---|---| +| `write_str` | `f.write_str(…)` on the formatter | +| `write_char` | `f.write_char(…)` on the formatter | +| `write_macro` | `write!(f, …)` / `writeln!(f, …)` | +| `delegation` | `Display::fmt(&self.x, f)` / `self.x.fmt(f)` | + ## `[srp]` | Key | Default | Meaning | diff --git a/book/reference-rules.md b/book/reference-rules.md index 3b3b0bf1..b2c52125 100644 --- a/book/reference-rules.md +++ b/book/reference-rules.md @@ -1,7 +1,13 @@ # Reference: rule catalog -Every rule rustqual emits, grouped by dimension. Codes are stable -identifiers — but where they actually surface differs by reporter: +Every rule rustqual emits, grouped by dimension. For any code below, +`rustqual --explain ` (case-insensitive) prints its full rule card: +what it detects, why it matters, the fix forms, the copyable suppression +marker, and the governing config knob — the catalog and the cards render +from the same registry. + +Codes are stable identifiers — but where they actually surface differs by +reporter: - **SARIF**: every result carries the catalog code as `ruleId` and is registered in `tool.driver.rules` (architecture also registers @@ -57,7 +63,7 @@ For dimension intent and refactor patterns, see the use-case guides linked at th | Code | Meaning | |---|---| | `BP-001` | Trivial `From` impl (derivable) | -| `BP-002` | Trivial `Display` impl (derivable) | +| `BP-002` | Trivial `Display` impl — semantic match: any branch-free, write-only `fmt` body (`write!`/`writeln!`, `f.write_str`, `f.write_char`, `Display::fmt` delegation, multi-statement included); gate forms via `[boilerplate].accepted_display_idioms` | | `BP-003` | Trivial getter/setter (consider field visibility) | | `BP-004` | Builder pattern (consider derive macro) | | `BP-005` | Manual `Default` impl (derivable) | diff --git a/rustqual.toml b/rustqual.toml index 1e847500..c78409f4 100644 --- a/rustqual.toml +++ b/rustqual.toml @@ -45,9 +45,15 @@ allow_expect = false enabled = true # ── Boilerplate Detection ─────────────────────────────────────────────── +# +# BP-002 matches semantically (any branch-free, write-only fmt body). +# accepted_display_idioms declares the house idiom as policy; vocabulary: +# write_str, write_char, write_macro, delegation (validated fail-loud). [boilerplate] enabled = true +# patterns = [] # enable-list; empty = all BP-* active +# accepted_display_idioms = [] # e.g. ["write_str"] # ── SRP (Single Responsibility) ───────────────────────────────────────── From 757b93bdca833f79f3a534b7ff01c8e789c9687d Mon Sep 17 00:00:00 2001 From: Sascha <18143567+SaschaOnTour@users.noreply.github.com> Date: Sat, 13 Jun 2026 02:43:02 +0200 Subject: [PATCH 7/7] fix(explain,bp002): resolve dynamic architecture ids; write_macro must target the formatter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings. (1) Findings print dynamic hierarchical ids (architecture/pattern/, architecture/trait_contract/) and the footer says --explain — find_rule_card now resolves an id to its longest registered prefix card, while ids with their own card (architecture/layer/unmatched, the call_parity checks) keep matching exactly. (2) classify_macro only counts write!/writeln! as the write_macro idiom when the first macro argument IS the formatter (recovered via the shared macro_tokens helper) — write!(self.buf, …) writes somewhere else: real logic, neither trivial nor blessable via accepted_display_idioms. --- CHANGELOG.md | 11 +++-- .../boilerplate/tests/root/bp002_semantics.rs | 42 +++++++++++++++++++ .../dry/boilerplate/trivial_display.rs | 19 ++++++--- src/domain/rule_cards/mod.rs | 23 +++++++++- src/domain/tests/rule_cards.rs | 30 +++++++++++++ 5 files changed, 115 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a527745d..fad8c181 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,7 +24,10 @@ syntax-based matching invited evasion instead of a decision. equality), `--explain` renders from it, and the compact findings view takes its titles from it. - **`--explain `** prints the rule card, case-insensitively - (`rustqual --explain bp-009` works). The natural first guess of every + (`rustqual --explain bp-009` works). Dynamic hierarchical ids resolve to + their longest registered prefix card — `architecture/pattern/` and + `architecture/trait_contract/` are exactly what findings print, so + they explain as their family card. The natural first guess of every consumer now succeeds. - **`--explain` never dead-ends.** An argument that is no rule id, not `allow`, and not a readable file prints the three explain modes with @@ -40,8 +43,10 @@ syntax-based matching invited evasion instead of a decision. ### Changed - **BP-002 matches semantically.** Any branch-free `fmt` body consisting only - of formatter write ops — `write!`/`writeln!`, `f.write_str`, `f.write_char`, - `Display::fmt` delegation — is trivial, multi-statement bodies included. + of formatter write ops — `write!(f, …)`/`writeln!(f, …)` (the first macro + argument must be the formatter; writing to any other target is real + logic), `f.write_str`, `f.write_char`, `Display::fmt` delegation — is + trivial, multi-statement bodies included. The evasion rewrite (two `write_char` statements) is the same finding; the semantically identical `f.write_str(&self.0)` newtype form no longer slips through unmatched. With accepted idioms declared, the rule enforces diff --git a/src/adapters/analyzers/dry/boilerplate/tests/root/bp002_semantics.rs b/src/adapters/analyzers/dry/boilerplate/tests/root/bp002_semantics.rs index 82e42267..9921acd4 100644 --- a/src/adapters/analyzers/dry/boilerplate/tests/root/bp002_semantics.rs +++ b/src/adapters/analyzers/dry/boilerplate/tests/root/bp002_semantics.rs @@ -211,3 +211,45 @@ fn test_bp002_suggestion_names_all_fix_forms() { bp002.suggestion ); } + +#[test] +fn test_bp002_write_macro_on_non_formatter_target_not_trivial() { + // write!(self.buf, …) writes somewhere else — real logic, not the + // documented write!(f, …) idiom. It must neither fire BP-002 (not + // derivable) nor count as `write_macro` (else + // accepted_display_idioms = ["write_macro"] would silently bless it). + let code = r#" + use std::fmt; + struct Tee { buf: String } + impl fmt::Display for Tee { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(self.buf, "x") + } + } + "#; + let findings = detect_boilerplate(&parse(code), &BoilerplateConfig::default()); + assert!( + !findings.iter().any(|f| f.pattern_id == "BP-002"), + "write! on a non-formatter target is not the trivial idiom" + ); +} + +#[test] +fn test_bp002_write_macro_checks_formatter_even_when_renamed() { + // The formatter param is positional, not nominal: `fmt` instead of `f` + // must still classify write!(fmt, …) as the write_macro idiom. + let code = r#" + use std::fmt; + struct Id(String); + impl fmt::Display for Id { + fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(fmt, "{}", self.0) + } + } + "#; + let findings = detect_boilerplate(&parse(code), &BoilerplateConfig::default()); + assert!( + findings.iter().any(|f| f.pattern_id == "BP-002"), + "write!(renamed_formatter, …) is still the trivial idiom" + ); +} diff --git a/src/adapters/analyzers/dry/boilerplate/trivial_display.rs b/src/adapters/analyzers/dry/boilerplate/trivial_display.rs index aa04335b..849c69b8 100644 --- a/src/adapters/analyzers/dry/boilerplate/trivial_display.rs +++ b/src/adapters/analyzers/dry/boilerplate/trivial_display.rs @@ -150,7 +150,7 @@ fn collect_idioms(block: &syn::Block, fmt_ident: &str) -> Option Option { match stmt { syn::Stmt::Expr(e, _) => classify_expr(unwrap_try(e), fmt_ident), - syn::Stmt::Macro(m) => classify_macro(&m.mac), + syn::Stmt::Macro(m) => classify_macro(&m.mac, fmt_ident), _ => None, } } @@ -169,18 +169,25 @@ fn unwrap_try(expr: &syn::Expr) -> &syn::Expr { /// Integration: dispatch on expression shape. fn classify_expr(expr: &syn::Expr, fmt_ident: &str) -> Option { match expr { - syn::Expr::Macro(m) => classify_macro(&m.mac), + syn::Expr::Macro(m) => classify_macro(&m.mac, fmt_ident), syn::Expr::MethodCall(mc) => classify_method_call(mc, fmt_ident), syn::Expr::Call(c) => classify_call(c, fmt_ident), _ => None, } } -/// `write!` / `writeln!` → the `write_macro` idiom. -/// Operation: path check, no own calls. -fn classify_macro(mac: &syn::Macro) -> Option { +/// `write!(f, …)` / `writeln!(f, …)` → the `write_macro` idiom — only when +/// the first macro argument IS the formatter. `write!(self.buf, …)` writes +/// somewhere else: real logic, neither the idiom nor derivable. +/// Operation: path check; arg recovery + ident test in a closure. +fn classify_macro(mac: &syn::Macro, fmt_ident: &str) -> Option { + let first_arg_is_fmt = || { + crate::adapters::shared::macro_tokens::recover_exprs(&mac.tokens) + .first() + .is_some_and(|e| is_path_ident(e, fmt_ident)) + }; let last = mac.path.segments.last()?; - if last.ident == "write" || last.ident == "writeln" { + if (last.ident == "write" || last.ident == "writeln") && first_arg_is_fmt() { Some(WriteOp::Idiom("write_macro")) } else { None diff --git a/src/domain/rule_cards/mod.rs b/src/domain/rule_cards/mod.rs index 865a4cd6..36c14c68 100644 --- a/src/domain/rule_cards/mod.rs +++ b/src/domain/rule_cards/mod.rs @@ -60,7 +60,28 @@ pub fn all_rule_cards() -> impl Iterator { } /// Look up one card by catalog id, case-insensitively (`bp-009` resolves -/// like `BP-009`). Integration: scans `all_rule_cards`. +/// like `BP-009`). Dynamic hierarchical ids resolve to their **longest +/// registered prefix**: findings emit `architecture/pattern/` / +/// `architecture/trait_contract/` with user-defined sub-kinds, and +/// every id a finding prints must be explainable — while an id with its own +/// card (`architecture/layer/unmatched`) never falls back to its family. +/// Integration: prefix candidates × exact lookup. pub fn find_rule_card(id: &str) -> Option<&'static RuleCard> { + id_prefixes(id).into_iter().find_map(|p| exact_card(p)) +} + +/// The exact-id lookup over the registry. +/// Operation: scan with a comparison closure. +fn exact_card(id: &str) -> Option<&'static RuleCard> { all_rule_cards().find(|c| c.id.eq_ignore_ascii_case(id)) } + +/// `a/b/c` → `["a/b/c", "a/b", "a"]` — the id and each `/`-prefix, longest +/// first, so exact matches always win over family fallbacks. +/// Operation: index arithmetic, no own calls. +fn id_prefixes(id: &str) -> Vec<&str> { + let mut ends: Vec = vec![id.len()]; + ends.extend(id.match_indices('/').map(|(i, _)| i)); + ends.sort_unstable_by(|a, b| b.cmp(a)); + ends.into_iter().map(|e| &id[..e]).collect() +} diff --git a/src/domain/tests/rule_cards.rs b/src/domain/tests/rule_cards.rs index 9bcef0ee..3f5ae68d 100644 --- a/src/domain/tests/rule_cards.rs +++ b/src/domain/tests/rule_cards.rs @@ -113,3 +113,33 @@ fn cards_name_their_config_knob() { cx.config ); } + +#[test] +fn dynamic_architecture_sub_ids_resolve_to_their_family_card() { + // Findings emit dynamic hierarchical ids (architecture/pattern/, + // architecture/trait_contract/) and the footer says + // `--explain ` — so exactly these ids must resolve, to the + // longest registered prefix card. + let pattern = find_rule_card("architecture/pattern/no_glob_imports") + .expect("dynamic pattern sub-id must resolve"); + assert_eq!(pattern.id, "architecture/pattern"); + let contract = find_rule_card("architecture/trait_contract/object_safety") + .expect("dynamic trait_contract sub-id must resolve"); + assert_eq!(contract.id, "architecture/trait_contract"); +} + +#[test] +fn registered_sub_ids_still_resolve_exactly() { + // Longest-prefix means an id with its own card never falls back to the + // family: layer/unmatched and the call_parity checks stay specific. + let unmatched = find_rule_card("architecture/layer/unmatched").unwrap(); + assert_eq!(unmatched.id, "architecture/layer/unmatched"); + let parity = find_rule_card("architecture/call_parity/no_delegation").unwrap(); + assert_eq!(parity.id, "architecture/call_parity/no_delegation"); +} + +#[test] +fn prefix_fallback_does_not_invent_cards() { + assert!(find_rule_card("not/a/rule").is_none()); + assert!(find_rule_card("src/cli/explain.rs").is_none()); +}