Feat/test sut cohesion#34
Merged
Merged
Conversation
…shared Move FileVisitor/visit_all_files, DeclaredFunction, ProjectScope (with ScopeCollector, helpers, and tests) and the test-reference collector into adapters/shared, and re-point every importer. This removes the analyzer-to-analyzer reach-ins (srp->dry, tq->dry, tq->iosp, srp->iosp): each dimension now imports the shared concept instead of another analyzer's internals. No behaviour change.
The SRP module-length check used a baseline/ceiling pair, but the ceiling only shaped a cosmetic `length_score` ramp: the SRP dimension score is count-based (report/mod.rs), so the ceiling drove no decision — not firing, counts, the fail gate, or the score. Collapse `[srp].file_length_baseline` and `file_length_ceiling` (plus the `[tests]` equivalents) into a single `file_length` threshold (default 300); a file is flagged when its production lines strictly exceed it. `length_score` is now the `production_lines / file_length` ratio, reported as metadata only. BREAKING: the `file_length_baseline` / `file_length_ceiling` config keys (under both [srp] and [tests]) are removed in favour of `file_length`.
Add the grammar for targeting ONE finding-kind within a dimension instead of the whole dimension. A target is named by its config field (metric) or rule name (boolean): metric targets require a mandatory pin value (a value-less threshold suppression would be blind forever) and a reason, boolean targets take no value. The target vocabulary lives in domain::suppression_target (target_kind / target_names) as the single source of truth that the per-dimension marking code will consume. A single classify_qual_allow drives both parse_qual_allow and detect_invalid_qual_allow so they can never diverge; unknown / malformed targets surface as ORPHAN_SUPPRESSION whose reason lists the valid targets (this is what stops an agent inventing a non-existent target like `srp_params`). Suppression gains an optional `target`; bare allow(dim) is unchanged and marking does not yet consume the target — that lands per dimension next.
SRP suppression marking now honours targets: a blanket allow(srp) silences every srp finding, while allow(srp, file_length=N) / max_independent_clusters=N / max_parameters=N / god_struct each silence one finding-kind. Metric pins suppress only while the value is <= the pin and re-fire above it, and a file_length pin can never hide a co-occurring cohesion finding (each active module component must be covered independently). Adds Suppression::suppresses() as the matching primitive and extracts the SRP marking into app/srp_suppressions.rs (mirroring dry_suppressions, keeping metrics.rs under the file-length cap). Existing blanket allow(srp) markers are unchanged. The 10% too-loose-pin orphan and the fix-first finding output land next; both are cross-cutting across every dimension's pins / findings.
Complexity suppression is now per finding-kind: a blanket allow(complexity) still silences everything, but allow(complexity, max_cognitive=18) / max_cyclomatic / max_nesting_depth / max_function_lines (metric pins, honouring their value) and magic_numbers / error_handling / unsafe (boolean) each silence exactly one kind, leaving the other complexity findings on the same function active. complexity_suppressed becomes the blanket-only flag (set only by a non-targeted allow(complexity)); per-kind decisions live in app/complexity_suppressions.rs (cx_target_suppressed / metric_warns / bool_warns), threaded into the two complexity warning passes. apply_extended_warnings is split into a thin orchestrator plus apply_extended_to_fn (with an ExtendedCx thresholds struct) to stay within the complexity/length caps. Blanket markers are unchanged.
DRY suppression is now per finding-kind: allow(dry, duplicate) / fragment / boilerplate / repeated_matches / wildcard_imports each silence exactly one kind, while a blanket allow(dry) still silences all of them. The DrySuppressible trait gains a TARGET const (the allow(dry, <target>) name), consumed by mark_dry_suppressions via Suppression::suppresses; mark_wildcard_suppressions targets "wildcard_imports". dead_code (DRY-002) is intentionally NOT a dry target — it is excluded via qual:api / qual:test_helper, not allow(dry), so a stray allow(dry) near dead_code stays an orphan. Vocabulary corrected: boilerplate added, dead_code removed.
Coupling suppression is now per finding-kind: a blanket allow(coupling) silences everything for the module, while allow(coupling, max_fan_in=N) / max_fan_out / max_instability (metric pins honouring their value — instability exercises the f64 pin) and sdp (boolean) each silence exactly one kind. Cycles are never suppressible, so cycle is not a target. Markers are MODULE-GLOBAL: new app/coupling_suppressions.rs groups coupling suppressions by module (via file_to_module) and exposes blanket() / suppresses(). m.suppressed becomes the blanket-only flag (it drives SDP inheritance + leaf handling); count_coupling_warnings gates each metric via suppresses(), and mark_sdp_target_suppressions applies targeted sdp on top of the blanket inheritance from populate_sdp_violations. Vocabulary: cycle removed.
Architecture suppression is now per rule family: a blanket allow(architecture) silences any family, while allow(architecture, layer) / forbidden / call_parity / pattern / trait_contract silence only that family. The family is the first segment after `architecture/` in the finding's rule id (architecture/layer/unmatched -> layer), extracted by arch_family; mark_architecture_suppressions routes through Suppression::suppresses. The cfg_test rules (inline_cfg_test_module / top_level_cfg_test_item) live under the `pattern` family. Window-scoping is unchanged; blanket markers keep working.
TQ suppression is now per finding-kind: a blanket allow(test_quality) (or the legacy allow(test)) silences everything, while allow(test_quality, no_assertion) / no_sut / untested / uncovered / untested_logic each silence exactly one kind. mark_tq_suppressions maps each TqWarningKind to its target name (tq_target_name) and routes through Suppression::suppresses; blanket markers are unchanged. This completes per-dimension targeted suppression across complexity, srp, dry, coupling, architecture, and test_quality. iosp stays single-kind (bare only).
The agent reaches for the tool before the README, so teach the suppression grammar from the tool itself. `--explain allow` prints the targeted-suppression guide: the grammar (metric value mandatory, reason mandatory), every dimension's valid targets split into metric (pinnable) vs boolean — sourced from the shared target_names/target_kind vocabulary so it can't drift — and the pin / 10%-orphan rationale. Dispatched by special-casing the literal `allow` for the existing `--explain <file>` flag; the builder (suppression_guide) returns a String so the content is unit-tested.
When findings remain, the text report ends with a one-line footer framing fixing as the expectation and suppression as a last resort, pointing at `rustqual --explain allow`. Deliberately prints no paste-ready suppression, so silencing a finding is never the path of least resistance.
Lifting these blanket markers (in app, pipeline, report, tq, structural) surfaces no coupling finding at all — their modules are leaf (afferent=0, so instability-exempt) or sit under the fan/instability thresholds. The markers suppressed dead air, so they're removed outright rather than migrated. First step of the Slice I cleanup: question every suppression and eliminate the cause where possible instead of re-pinning.
…(complexity) The blanket allow(complexity) on check_trivial_from masked a 68-line LONG_FN. Extract the per-item detection (trivial_from_find) and the trivial-wrap test (is_trivial_wrap) into named helpers; check_trivial_from becomes a thin per-file scan. The marker is eliminated — no complexity finding remains.
…ow(complexity) The blanket allow(complexity) masked nesting depth 6 + cyclomatic 13. Replace the nested closure-and-loop walk with iterator chains over small helpers (item_fn_bodies, clone_heavy_find, stmt_value, bp008); the marker is eliminated with no complexity finding remaining.
…omplexity) The blanket allow(complexity) masked cyclomatic 16 + a 68-line LONG_FN. Split the getter/setter classification into small predicates (is_trivial_getter / is_trivial_setter / is_getter_or_setter) and the per-impl detection into impl_getter_setters, with impl_struct_name + bp003 builders. The marker is eliminated with no complexity finding remaining.
…(complexity) The blanket allow(complexity) masked cyclomatic 15 + a 74-line LONG_FN. Extract the From-impl grouping (count_from_impls / trivial_from_impl / is_single_expr_from / bp007) and reuse the existing trait_name_of + self_type_of super-helpers instead of re-deriving them. The marker is eliminated with no complexity finding.
…tems The blanket allow(complexity) on check_builder_boilerplate masked cyclomatic 14, nesting 5, and a 76-line LONG_FN. Extracting the builder-method predicate (is_builder_method / method_returns_self / is_field_assign / is_self_expr) fixed those, but uniforming the per-item scan made check_builder_boilerplate and check_trivial_from structural duplicates (DRY-001/003 caught it). Add a shared boilerplate::scan_items (enable-guard + per-item filter_map) and route both single-find-per-item detectors through it, so each wrapper is a one-line delegation. Marker eliminated; no complexity or duplicate finding remains.
…exity) The blanket allow(complexity) masked cyclomatic 19, nesting 6, and an 81-line LONG_FN. Add a shared boilerplate::item_fns (signature+body of each function in an item) and route the detection through small helpers (is_conversion_fn / repetitive_match_find / stmt_match / bp006). The marker is eliminated with no complexity finding remaining.
The blanket allow(complexity) masked cyclomatic 12 + an 85-line LONG_FN on the interval-merge algorithm. Split it into phases (canonicalize_pairs / group_end / merge_group / run_end / make_fragment_group / stmt_line_span). The FragmentEntry literals are kept nested inside the FragmentGroup so the necessary owned string copies remain a layer-boundary detail (a flattened entry-builder tripped BP-008 clone-heavy). Marker eliminated, no finding remains.
The blanket allow(complexity) masked cyclomatic 15, nesting 5, and a 91-line LONG_FN. Split into collect_modules / crate_dep_modules / use_paths (the stack walk) / pushed / add_edges / sort_adjacency. The marker is eliminated with no complexity finding remaining.
The blanket allow(complexity) masked cyclomatic 15 + a 74-line LONG_FN on Kosaraju's algorithm. Split into its three passes (finish_order / dfs_finish, reverse_graph, collect_sccs / collect_component / scc_report). The marker is eliminated with no complexity finding remaining. This clears the last allow(complexity) marker — all nine were removed by refactoring rather than re-pinned.
project_function (BP-008 clone-heavy) and classify (BP-006 repetitive match) are the legacy→typed layer boundary: a full-field copy and a 4-arm enum bridge. Both are intrinsic to the decoupling (domain can't import the adapter type; a From impl wouldn't change the construction the detectors flag), so they are the legitimately-irreducible cases. Migrate the blanket allow(dry) to the targeted allow(dry, boilerplate) — keeping the existing rationale — so they can no longer mask an unrelated dry finding (lifting them surfaced only the boilerplate).
build_function_analysis took 7 positional args, tripping SRP_PARAMS (>5). Bundle them into a FunctionParts parameter object; the sole caller classify_and_build builds the struct literal (no param limit). Eliminates the last allow(srp) on this factory via real refactoring, not suppression.
push_metric_threshold took 7 positional args (SRP_PARAMS). Replace the six near-identical calls with a tuple data table iterated by a dispatch loop; push_metric_threshold now takes one MetricThreshold tuple (3 params). A tuple (not a struct) keeps the uniform per-metric table clear of BP-009, matching the repo convention for data tables. Eliminates the suppression by refactoring.
TQ-003 reachability could not follow syn's trait dispatch (visit_block → visit_expr), so visitor override methods and their helpers looked untested. The previous fix seeded all ignored (visit_*) functions as "implicitly tested" — a blanket assumption baked into the analyzer. Replace that with real edges: for each visitor type T, record the helper methods its syn::Visit overrides call, and at each drive-site (x.visit_block(..), syn::visit::visit_*(&mut x, ..), visit_all_files(.., &mut x)) resolve the driven type locally and add driver_fn → helpers(T) edges. Testedness now flows only when a test actually drives the visitor; an undriven visitor stays untested. Drops the is_ignored_function seed entirely. Isolated change — visit_* remains in ignore_functions, so complexity/SRP exemptions are unaffected. Self-analysis stays 100%/0; +4 dispatch unit tests.
…cluded SRP cohesion (LCOM4) excluded ALL trait-impl methods, so a struct whose core logic lives in a behavior trait (a syn::Visit walker) fragmented into false god-structs: the visit_* methods that tie its helpers together were invisible, leaving the helpers as disconnected responsibilities. Fix: non-mechanical trait methods now act as BRIDGES — their field footprint (direct accesses + one-level self-call expansion) unions the inherent methods they tie together, WITHOUT being counted as responsibility nodes. This is monotonic (only merges components, never splits), so it can only clear false positives, never manufacture new ones. Mechanical traits (Display/Debug/From/Into/Serialize/PartialEq/Hash/Default/ Clone/…) stay fully excluded: their methods touch all fields by rote and would mask genuine god-structs. Stateless trait methods (e.g. reporter render_*) touch disjoint fields, so they bridge nothing and reporters stay unflagged. Clears the calls.rs CanonicalCallCollector god_struct (its visit_* bridge record_call/calls to the scope helpers → LCOM4 → 1) and unblocks refactoring visitor methods (extracting helpers no longer fabricates a god-struct). More faithful to canonical LCOM4 (which counts all methods) while keeping the all-field-masking guard where it is justified. +1 bridge test.
…exity
Remove the blanket "visit_*" entry from ignore_functions — the single biggest
hidden suppression in the project (133 methods got a total free pass on every
dimension, far beyond its stated call-graph rationale). The call-graph
dimensions (TQ-003, dead-code) already exempt trait impls structurally, and the
dispatch-edge fix handles their reachability, so the name-ignore was redundant
there; only its (illegitimate) complexity/IOSP free pass remained.
Refactor every fat visitor to per-node category dispatch so it passes rustqual
own rules:
- Normalizer::visit_expr (cyclo 53, 246 ln) + visit_pat → category handlers
- BodyVisitor::visit_expr (cyclo 48, 297 ln, IOSP violation) → free walk_*
functions (walk steps, not struct responsibilities — keeps the
metrics-visitor SRP footprint at its pre-refactor size)
- WildcardCollector::visit_item_use, MutationChecker::visit_expr,
CallTargetCollector::visit_field → extracted decision helpers
Call-detection predicates are hidden in closures (lenient mode) to keep the
dispatch arms Operations, not IOSP Violations.
Also refine bridge-only cohesion: a bridge now ties in the methods it directly
calls (not only those sharing its field footprint), so a pure-delegation
dispatch arm that touches no field still coheres with the walk.
main/run stay ignored (handled next). Self-analysis 100%/0 with visit_* fully
analyzed; 1901 tests green.
The composition root run() in lib.rs mixed arg parsing, mode dispatch, and the analysis pipeline (cyclo 15, 86 lines, IOSP violation) — which is why it was name-ignored. Decompose it into phase operations (parse_cli_args, try_preconfig_modes, run_configured, try_postconfig_modes, run_full_analysis, collect_and_analyze, report_analysis, handle_baseline_and_compare); run() now orchestrates with early-exit branches in unwrap_or_else closures so it stays a clean Integration. main passes on its own. Fix the layer_rule test run() helper SRP_PARAMS (6 params) with an Externals parameter object. ignore_functions is now [] — every function in rustqual is analyzed by its own rules. Self-analysis 100%/0; 1901 tests green.
BREAKING: the ignore_functions config option is gone. It was a blunt instrument that hid functions from EVERY dimension; with the visitor-dispatch and bridge fixes in place, nothing in rustqual needs it. A rustqual.toml that still sets ignore_functions will now fail to parse (deny_unknown_fields) — remove the key; use // qual:allow, // qual:api, // qual:test_helper, or exclude_files instead. Removes the Config field, is_ignored_function, the compiled glob cache, and all consume sites (iosp classification, TQ-003 untested filter, DRY-002 dead-code exclusion) plus the init templates and tests. Also: compute_lcom4 now unions methods connected by a self.other() call, matching canonical LCOM4 (which joins methods sharing a field OR calling one another). This was a latent gap surfaced when removing the is_ignored_function field-touch left Analy zer falsely split; the fix is general and monotonic. Self-analysis 100%/0; 1894 tests green; clippy clean.
CHANGELOG entry for 1.5.0, version bump, and update the book (reference-configuration, reference-suppression), CLAUDE.md, and the migration note for the removed ignore_functions option.
…arker The per-node-dispatch refactor moved the heavy walk logic into inherent norm_* handlers, so the 665-line normalizer now splits cleanly by concern: token (vocabulary), operators (lookup tables), expr + compound (expression handlers), pat (pattern handlers), and mod (public API + Normalizer state + statement walk + the Visit dispatch table). Every file is well under the SRP file-length baseline, eliminating the blanket // qual:allow(srp) marker — a real fix, not a targeted pin.
…ile-length The LCOM4=2 god_struct was a single isolated method: collect_macro_body touched no field and only delegated to self.visit_expr, so it formed its own cohesion component. Inlining its 3-line body into the sole caller (visit_macro) removes the isolated node → LCOM4=1, god_struct gone (the smell you most wanted fixed). The residual SRP_MODULE file-length (692 prod lines) is migrated from the stale blanket allow(srp) marker to a targeted allow(srp, file_length=692) pin: the file is a complete single-pass receiver-type-inference engine (scope tracking + path canonicalisation + call resolution as one cohesive walk). A module split is the documented future refactor; the targeted pin re-fires if the file grows. Self-analysis 100%/0; 1894 tests green.
…ngth pin You asked whether it was repairable — it is. The 692-line single-pass receiver- type-inference walker splits cleanly by concern into a directory module, each file well under the SRP file-length baseline: - mod.rs — FnContext, entry point, the collector struct + shared helpers - scope.rs — scope-stack management + signature/closure binding seeding - canon.rs — path canonicalisation - resolve.rs— method-call target resolution + trait/edge projection - install.rs— let/for/destructure binding installation + PatKind - visit.rs — the syn::Visit walk The targeted allow(srp, file_length=692) pin is GONE — no suppression. Combined with the earlier god_struct fix, calls.rs now carries zero srp markers. All 1894 tests green; self-analysis 100%/0; clippy clean.
…e flip) BREAKING: a bare // qual:allow(<dim>) is now an error for any dimension that has targets (srp, complexity, dry, coupling, architecture, test_quality). It would silence EVERY finding of that dimension — too blunt — so it must name a target: // qual:allow(srp, god_struct), // qual:allow(complexity, max_cyclomatic=20), etc. iosp (no targets) keeps its bare form. Multi-dimension blanket markers (allow(a, b)) are likewise gone — use one marker per dimension. Adds InvalidQualAllow::BlanketNotAllowed (surfaced as ORPHAN_SUPPRESSION with a fix-it message naming the valid targets) and blanket_or_invalid() in the parser. Migrates the in-tree markers to targeted form and updates the parser tests to the new semantics. Self-analysis 100%/0; tests green; clippy clean.
…5.0)
Make the orphan detector judge a `// qual:allow(dim, target[=N])` marker
against a finding of its *own* kind, and flag a metric pin parked too far
above the value it covers.
- target-aware matching: a targeted marker matches only a finding of its
named kind (a `file_length` pin no longer counts a god-struct finding);
blanket markers (iosp) still match any covered-dim finding.
- too-loose pins: a pin that covers a finding but sits more than
`[suppression].pin_headroom` (default 0.10) above the covered value is
emitted as a `PinTooLoose` orphan ("tighten to ~value or remove"); a
too-tight (re-firing) pin is left untouched.
- `FindingPosition` gains `target: Option<&'static str>` + pinnable
`value: Option<f64>`; every collector tags its positions. Complexity
targets/values come from `complexity_predicates::triggered_targets`,
the architecture family from the shared `app::architecture::arch_family`,
TQ targets from `TqFindingKind::meta().json_kind`.
- `OrphanKind` (Stale | PinTooLoose) on `OrphanSuppression`; `status_word()`
renders "stale"/"too-loose" consistently across every reporter.
- split `orphan_suppressions/` into `positions.rs` (enumeration) and
`mod.rs` (decision), keeping both under the SRP file-length baseline.
New `[suppression]` config section. Docs: rustqual.toml, CHANGELOG,
book/reference-{configuration,suppression}, --explain allow, CLAUDE.md.
…sts, docs
Remediation of the pre-push review (own review + pr-review-toolkit agents).
Type redesign (R1): `domain::SuppressionTarget` is now a sum type
(`Metric { name, pin } | Boolean { name }`) so the "metric ⇒ pin, boolean ⇒
no pin" invariant is unrepresentable otherwise; `suppresses()` matches the
variant directly. `name()`/`pin()` accessors replace the public fields.
Orphan-detector bugs:
- B1: `srp_finding_targets` now emits the `file_length` / `max_independent_
clusters` position only for the component that actually fired (length_score
> 1.0 / clusters > max), mirroring `module_warning_suppressed`. A pin on the
inactive component was escaping stale detection and mis-firing as too-loose.
- B2: a targeted coupling marker (max_fan_*/instability/sdp) is module-global
with no line-anchored position, so `is_verifiable` now treats it as
unverifiable instead of false-flagging it when a structural coupling finding
coexists in the file.
- B3: the `magic_numbers` position anchors at the function line, not the
literal's line, so a valid `allow(complexity, magic_numbers)` marker placed
above the function is no longer reported stale.
Tests (T1): +12 cases — cluster/param too-loose, headroom boundary (`>` vs
`>=`), config-driven `pin_headroom`, largest-covered-value fold, and arch/dry/
tq targeted-orphan kind matching. The orphan target tests were split into
`orphan_targeting` + `orphan_too_loose` (+ shared `orphan_target_helpers`) to
stay under the SRP file-length cap; `orphans_cfg` trimmed to ≤5 params.
Docs: reference-suppression.md rewritten for the flip (bare allow only for
iosp; targeted syntax + mandatory reason); DRY-003/004 swap fixed in the
TestsConfig doc; CLAUDE.md file_length / signature / flip; stale Phase-4,
ignore_functions, wildcard and explain-module comments; CHANGELOG.
…RP match Remediation of the second pre-push review (own + pr-review-toolkit ×5). The first-round fixes and the SuppressionTarget redesign were verified correct by multiple agents; this addresses the remaining findings. Docs (the main new finding — a DRY-003/004 swap that the prior round fixed only in sections.rs persisted across other files, making the repo self-contradict): - DRY-003 = fragment, DRY-004 = wildcard corrected in reference-configuration.md, rustqual.toml, and the historical CHANGELOG entries. - Stale `file_length_baseline`/`file_length_ceiling` (collapsed to a single `file_length` on this branch) fixed in reference-configuration.md and module-quality.md; added the missing v1.5.0 BREAKING-config CHANGELOG entry. - CLAUDE.md "tuple" remnant; qual_allow.rs transitional "subsequent phases" comment; arch_family flip caveat. Tests (+9): direct `suppresses()` unit tests (metric at/below/above pin, metric-with-no-value fails closed, target/dim isolation, boolean by-name, blanket); `target_kind`/`target_names` vocabulary round-trip (drift guard); parser singularity (targeted ⇒ exactly one dimension); both-module-components- active emits both targets; coupling metric pins are never too-loose-checked (pins the documented module-global blind spot). Harden: `srp_finding_targets` splits the catch-all into an explicit `(Structural, _)` arm plus a `debug_assert!`-backed mismatch arm, so a future kind/details desync fails loudly instead of silently dropping a position.
Review finding (High): mark_structural_suppressions matched on covers(dim)
only, ignoring the marker's target — so any targeted marker in the 5-line
window (e.g. `allow(coupling, sdp)`) silently suppressed unrelated structural
findings (BTC/SLM/NMS, OI/SIT/DEH/IET) of that dimension. With targeted
coupling markers also skipped by the orphan detector (module-global), the
hidden warning was not even replaced by an orphan — a silent suppression.
Structural checks are not in the suppression-target vocabulary, so only a
blanket marker may silence them: `in_window && sup.target.is_none() &&
sup.covers(dim)`. This mirrors the orphan detector, where structural positions
carry `target: None` and match blanket markers only. Since blanket SRP/Coupling
markers are rejected by the parser ("the flip"), a structural finding is
effectively unsuppressible — the honest consequence of the flip, now reflected
consistently on both the marking and orphan sides.
TDD: targeted_suppression_does_not_suppress_structural_warning (red → green);
the existing blanket-suppression path stays green.
Docs: fix stale post-flip `allow(coupling)` / `allow(<dim>)` blanket examples
in reference-suppression.md (module-level section + summary rows) and
coupling-quality.md (the structural section no longer claims OI/SIT/DEH/IET are
suppressible).
…weep Decision (owner): structural findings must remain suppressible — some are genuinely unfixable (an impl forced away by Rust's orphan rules, a single-impl trait that is a real API boundary, a `downcast` plugin seam), so leaving them unsuppressible after the flip would strand legitimate cases. Each structural check now has its own boolean suppression target named by its lowercased code — `oi`/`sit`/`deh`/`iet` (coupling), `btc`/`slm`/`nms` (SRP): // qual:allow(coupling, oi) reason: "orphan rules force this impl away" - `StructuralWarningKind::target_name` + the shared `target_name_for_code` (single source for the code→target map). - Vocabulary: the seven codes added to `target_kind`/`target_names` (Srp, Coupling) — so `--explain allow` lists them and the parser accepts them. - Marking routes through `suppresses()` (a targeted marker silences only its own kind; a metric coupling pin never silences a structural finding). - Orphan collector tags structural positions with their target; `is_verifiable` now skips only *module-global* coupling targets (max_fan_*/instability/sdp) — structural coupling targets are line-anchored and verified normally (`is_module_global_coupling`). - Tests: targeted structural marker silences its own kind / not another; orphan matches its kind / stale on the wrong kind. Book sweep: every post-flip bare `allow(<dim>)` example across the user chapters (module/function/code-reuse/test/architecture/adapter-parity quality docs + reference-configuration + reference-output-formats) is now targeted, and coupling-quality documents the structural targets.
…test Review finding (Medium): the call_parity golden example still carried a bare `// qual:allow(architecture)` on cmd_debug, which the flip rejects (BlanketNotAllowed) — so running the full pipeline on the example produced an invalid-marker orphan and left cmd_debug's no_delegation finding unsuppressed, contradicting the "exactly two findings" claim in its README and rustqual.toml. It slipped through because `examples/**` is excluded from self-analysis and the golden unit test only invokes the call_parity rule, not the suppression parser. - Marker → `// qual:allow(architecture, call_parity) reason: "CLI-only diagnostic with no MCP / REST peer"`; README + rustqual.toml prose updated. Verified end-to-end: the example now yields exactly the two documented architecture findings, cmd_debug suppressed, no invalid-marker orphan. - New guard `golden_example_has_no_invalid_suppression_markers` scans every `qual:allow` marker in the example via `detect_invalid_qual_allow`, closing the examples/-excluded blind spot for future marker rot.
Review finding (Low): book/reference-configuration.md's "Removed in 1.5.0" note recommended `// qual:allow(<dim>)` for exempting code — invalid for every multi-kind dimension since the flip. Now points at a targeted `// qual:allow(<dim>, <target>)` with reason (or bare `allow(iosp)`). (The remaining bare-dim mentions live only in docs/plan-*.md — internal, historical design/backlog records, intentionally left as written.)
Review finding 1 (Medium): after the target-aware redesign, OrphanSuppression stored only `dimensions`, so a stale `// qual:allow(srp, file_length=400)` rendered as `qual:allow(srp)` — the user couldn't tell which target was stale or too-loose, and with several markers of one dimension in a file the report was ambiguous. - `OrphanSuppression` now carries `target: Option<SuppressionTarget>` (set from the marker; `None` for blanket/invalid). New `target_spec()` (`"file_length=400"` / `"god_struct"`) and `target_suffix()` (`", file_length=400"`). - Every reporter appends the target to its scope: text/sarif/github/ai render `qual:allow(srp, file_length=400)`, html shows it in the scope cell, and JSON gains a structured `target` field. Blanket markers (suffix `""`) render unchanged, so existing snapshots hold. - `status_word` moved from `OrphanSuppression` to `OrphanKind` (where it belongs) — also keeps the DTO cohesive (self-analysis flagged LCOM4=2 from the kind-vs-target method split). Dropped `Eq` (the pin is `f64`). - Tests: orphan carries + renders its target (metric/boolean/blanket). Review finding 2 (Low): two `[architecture]` config rustdocs still suggested a bare `// qual:allow(architecture)` (rejected since the flip) — now targeted `// qual:allow(architecture, call_parity) reason: "…"`.
…ng docs Review finding (Medium): `classify_metric` only checked `value.parse::<f64>()`, which accepts `inf`/`-inf`/`NaN`. A pin `allow(srp, file_length=inf)` parsed as valid, and `value <= inf` is always true — so the pin would NEVER re-fire, losing its core promise; `NaN` silenced nothing yet was accepted as valid; a negative pin is meaningless for the non-negative metrics. A pin must now be finite and `>= 0.0`, else it is rejected as a BadPinValue (message broadened to "must be a finite, non-negative number"). TDD: inf/-inf/NaN/-1/-0.5 rejected, 0 accepted. Review finding (Low): two notes in book/reference-suppression.md still said *all* targeted coupling markers are module-global and not orphan/too-loose checked. That holds only for the module-global metrics (max_fan_*/instability/ sdp); the structural coupling targets (oi/sit/deh/iet) are line-anchored and ARE orphan-checked — clarified both spots so `allow(coupling, sit)` isn't mis-described.
…ording Review finding (Medium): `pin_headroom` was an unvalidated `f64`. TOML accepts `inf`/`nan` and negatives, and the value feeds `pin > value * (1 + headroom)` directly — so `inf`/`nan` silently disable too-loose detection and a negative headroom makes the threshold nonsensical (negative below -1). Now validated at config-setup time (`validate_suppression`): finite and `>= 0.0`, else a clear config error — mirroring the finite/non-negative guard just added to marker pins. `setup_config`'s validate step now runs both weight + suppression checks. TDD: inf/NaN/-0.1/-1.5 rejected, 0.0 accepted. Review finding (Low): `sdp` was lumped with the fan-in/out/instability metrics as a "module-global metric", but it is a boolean check. The behavior is right (sdp is module-global, not line-anchored); reworded the two book notes and the two code comments (is_verifiable + is_module_global_coupling) to "module-global target — the … metrics or the boolean sdp check".
… HTML Review finding (Medium): text/GitHub/SARIF distinguished stale vs too-loose, but the structured/CI formats dropped the new semantic — JSON had no kind, AI wrote a generic "orphan suppression …", and HTML had no status column. So the remedy (stale → delete the marker, too-loose → tighten the pin) was only recoverable by parsing the human reason string. - `OrphanKind::json_kind()` — stable snake_case token (`"stale"`/`"too_loose"`) for structured output, alongside the human `status_word()`. - JSON `JsonOrphanSuppression` gains a `kind` field. - AI: detail leads with the status word and the entry carries a `kind` field. - HTML: orphan table gains a Status column. - Tests assert kind/status across JSON (stale + too_loose + target), AI, HTML.
Review finding (Low): several descriptions still defined an orphan suppression purely as a marker that "matched no finding"/"stale". Since PinTooLoose, that is incomplete — a too-loose pin *does* match a finding but is still reported. Updated the user/API/internal docs to say "stale or too-loose": book/reference-output-formats.md (HTML section), book/reference-suppression.md (ORPHAN-001 lead), report/mod.rs (`orphan_suppressions` field), json_types.rs (JsonOrphanSuppression), and the orphan.rs module doc. No code change.
Review finding (Low): the `Summary::orphan_suppressions` counter doc still described it as markers that "did not match any finding" — but since PinTooLoose the same counter also includes pins that DO match a finding yet sit too far above its value. Updated that doc and the orphan-detector module doc (`orphan_suppressions/mod.rs`) to cover stale + too-loose. No code change.
Review finding (Low): the function doc still said "markers that do not match
any finding" — stale-only — but it now also reports too-loose pins (which DO
match a finding). Updated the summary line and the IOSP-role note ("filters
unmatched markers" → "classifies each marker"). No code change.
…e-only Review finding (Low): the `target` field doc on JsonOrphanSuppression still said "so consumers see which target is stale" — but `target` is equally relevant for a `kind: "too_loose"` pin. Reworded to "which target is being reported (stale or, for a metric pin, too-loose)". No code change.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands rustqual’s suppression system from dimension-wide qual:allow(dim) markers to targeted suppressions (qual:allow(dim, target[=N])) with shared target vocabulary, adds richer orphan-suppression reporting (stale vs too-loose pin), and refactors shared analyzer utilities (scope, file visiting, test reference collection) to improve Test Quality / SRP correctness. It also updates the config schema (removing ignore_functions, adding [suppression].pin_headroom) and introduces rustqual --explain allow documentation output.
Changes:
- Add target-aware suppressions across dimensions (metric pins + boolean targets), plus new orphan kinds (
stalevstoo_loose) and reporting/serialization updates. - Add
--explain allowoutput + tests; introduce/validate[suppression].pin_headroom; removeignore_functionsfrom config and docs. - Refactor shared analyzer infrastructure (ProjectScope, visitor driving, declared-function metadata, per-test reference collection) and update affected analyzers/tests.
Reviewed changes
Copilot reviewed 168 out of 172 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ports/tests/suppression_parser.rs | Update parser tests for new suppression fields/targets |
| src/domain/tests/suppression.rs | Add domain-level tests for targeted suppression semantics |
| src/domain/suppression.rs | Add suppression target + suppresses() logic |
| src/domain/mod.rs | Export suppression_target module + symbols |
| src/domain/findings/orphan.rs | Add orphan kind + target-aware orphan rendering helpers |
| src/domain/findings/mod.rs | Re-export OrphanKind |
| src/cli/explain/tests.rs | New tests for --explain allow guide |
| src/cli/explain.rs | Implement --explain allow guide generation |
| src/app/tq_metrics.rs | Switch TQ suppression marking to per-kind targets |
| src/app/tests/warnings/orphan_target_helpers.rs | New shared helpers for targeted orphan tests |
| src/app/tests/warnings/orphan_support.rs | Adjust SRP fixtures + add suppression target field |
| src/app/tests/warnings/orphan_module_tq_arch.rs | Add target: None to suppression fixtures |
| src/app/tests/warnings/orphan_mod_internals.rs | Add target: None to suppression fixtures |
| src/app/tests/warnings/orphan_dry_and_disabled.rs | Add target: None to suppression fixtures |
| src/app/tests/warnings/orphan_basics_and_suppression.rs | Adjust orphan tests + fixture line anchoring |
| src/app/tests/warnings/mod.rs | Wire new warning APIs (suppression map param) + add modules |
| src/app/tests/warnings/flag_application.rs | Add target: None to suppression fixtures |
| src/app/tests/warnings/extended_warnings.rs | Update calls for new apply_extended_warnings signature |
| src/app/tests/warnings/exclude_and_error_handling.rs | Update calls for new apply_extended_warnings signature |
| src/app/tests/warnings/complexity_targeted.rs | New tests for targeted complexity suppression |
| src/app/tests/warnings/complexity_flags.rs | Update complexity warnings API call signature |
| src/app/tests/tq_metrics.rs | Add tests for targeted TQ suppression per kind |
| src/app/tests/structural_metrics.rs | Add tests ensuring targeted structural suppressions are per-kind |
| src/app/tests/mod.rs | Add coupling-targeted test module + add target: None helper |
| src/app/tests/metrics/suppression.rs | Add tests for targeted DRY suppression per kind |
| src/app/tests/metrics/srp_targeted.rs | New tests for SRP module targeted suppression behavior |
| src/app/tests/metrics/srp_suppression.rs | Update SRP suppression marking signature (needs config) |
| src/app/tests/metrics/mod.rs | Re-export SRP suppression marker + add targeted SRP module |
| src/app/tests/metrics/counters.rs | Update coupling warning counting signature (module suppressions) |
| src/app/tests/coupling_targeted.rs | New tests for targeted coupling suppression per module |
| src/app/tests/architecture.rs | Add tests for targeted architecture suppression by family |
| src/app/structural_metrics.rs | Switch structural suppression marking to Suppression::suppresses |
| src/app/srp_suppressions.rs | New SRP suppression marking module (target-aware) |
| src/app/setup.rs | Expand config validation (weights + suppression) |
| src/app/secondary.rs | Refactor coupling suppression flow (module-keyed suppressions) |
| src/app/projection/data.rs | Update DRY suppressions to targeted boilerplate target |
| src/app/projection/complexity.rs | Refactor complexity projection threshold table |
| src/app/pipeline.rs | Pass suppression lines into complexity warning passes |
| src/app/orphan_suppressions/complexity_predicates.rs | Emit triggered targets + values for orphan/too-loose logic |
| src/app/mod.rs | Export new suppression modules |
| src/app/dry_suppressions.rs | Introduce per-kind DRY suppression targets via trait constant |
| src/app/coupling_suppressions.rs | New module-keyed, target-aware coupling suppression helper |
| src/app/complexity_suppressions.rs | New per-kind targeted complexity suppression helper |
| src/app/architecture.rs | Implement targeted architecture suppression by rule-family |
| src/adapters/suppression/tests/qual_allow.rs | Update suppression parsing tests for targets/rejections |
| src/adapters/suppression/tests/mod.rs | Add targeted suppression parser tests module |
| src/adapters/source/tests/filesystem.rs | Update marker fixtures to new targeted SRP syntax |
| src/adapters/shared/tests/scope/mod.rs | Switch scope tests to shared project_scope |
| src/adapters/shared/tests/scope/collection_and_ownership.rs | New tests for shared scope collector/ownership checks |
| src/adapters/shared/tests/mod.rs | Add shared scope test module |
| src/adapters/shared/test_references.rs | New shared per-test reference collector |
| src/adapters/shared/normalize/token.rs | New normalized token enum (normalizer vocabulary) |
| src/adapters/shared/normalize/pat.rs | New pattern normalization routines |
| src/adapters/shared/normalize/operators.rs | New operator-to-string lookup for normalizer |
| src/adapters/shared/normalize/compound.rs | New compound expression normalization routines |
| src/adapters/shared/mod.rs | Export new shared utilities (scope, visitor, references, etc.) |
| src/adapters/shared/file_visitor.rs | New shared file-visitor infrastructure |
| src/adapters/shared/declared_function.rs | New shared DeclaredFunction type |
| src/adapters/report/text/tests/root.rs | Update orphan test fixtures for new orphan fields |
| src/adapters/report/text/mod.rs | Add “suppression is last resort” footer when findings exist |
| src/adapters/report/tests/json/summary_fields_and_orphan.rs | Add orphan kind/target JSON projection tests |
| src/adapters/report/tests/github/rendering.rs | Update orphan fixtures with kind/target |
| src/adapters/report/tests/findings_list.rs | Update orphan fixtures with kind/target |
| src/adapters/report/tests/dot.rs | Update orphan fixtures with kind/target |
| src/adapters/report/tests/ai/smoke_and_orphan.rs | Ensure AI output includes orphan kind and status word |
| src/adapters/report/sarif/tests/root/orphan_and_architecture.rs | Update orphan SARIF fixtures with kind/target |
| src/adapters/report/sarif/mod.rs | Render orphan kind + target in SARIF messages |
| src/adapters/report/mod.rs | Update summary/orphan documentation to include too-loose pins |
| src/adapters/report/json/misc.rs | Project orphan kind + target to JSON types |
| src/adapters/report/json_types.rs | Extend JSON orphan schema with kind + target |
| src/adapters/report/html/tests/root.rs | Add HTML expectations for orphan Status column |
| src/adapters/report/html/orphan_suppressions.rs | Render orphan status + target in HTML orphan table |
| src/adapters/report/github/format.rs | Render orphan kind + target in GitHub annotations |
| src/adapters/report/findings_list/mod.rs | Include orphan kind + target suffix in findings list |
| src/adapters/report/ai/output.rs | Include orphan kind + target suffix in AI output entries |
| src/adapters/report/ai/details.rs | Update SRP file-length display to new config field |
| src/adapters/config/tests/sections.rs | Update SRP/tests config defaults + add suppression config tests |
| src/adapters/config/tests/root.rs | Remove ignore_functions tests + add suppression validation tests |
| src/adapters/config/tests/init.rs | Update init config expectations post ignore_functions removal |
| src/adapters/config/sections.rs | Replace SRP baseline/ceiling with file_length; add suppression config |
| src/adapters/config/mod.rs | Remove ignore_functions; add [suppression] + validation |
| src/adapters/config/init.rs | Update generated config templates to new schema |
| src/adapters/config/architecture.rs | Update docs/examples to targeted architecture suppression |
| src/adapters/analyzers/tq/untested.rs | Remove ignore-functions dependency; use shared DeclaredFunction |
| src/adapters/analyzers/tq/tests/sut.rs | Switch to shared scope/declared-function types |
| src/adapters/analyzers/tq/tests/mod.rs | Add dispatch tests module |
| src/adapters/analyzers/tq/tests/dispatch.rs | New tests for visitor-dispatch edge modeling |
| src/adapters/analyzers/tq/sut.rs | Replace local collector with shared test reference collection |
| src/adapters/analyzers/tq/mod.rs | Add visitor-dispatch edges to call graph; adjust tested seeding |
| src/adapters/analyzers/structural/nms.rs | Refactor self-mutation detection into helper |
| src/adapters/analyzers/structural/mod.rs | Add structural code→target mapping (target_name_for_code) |
| src/adapters/analyzers/srp/tests/root.rs | Switch to shared file visitor; update module threshold API |
| src/adapters/analyzers/srp/tests/module/scoring_and_analysis.rs | Update SRP module scoring tests to new ratio-based score |
| src/adapters/analyzers/srp/tests/module/clusters.rs | Extract fixture + update module threshold API |
| src/adapters/analyzers/srp/tests/cohesion/warnings.rs | Add “bridge” tests for visitor-style cohesion |
| src/adapters/analyzers/srp/tests/cohesion/scoring.rs | Update LCOM4 scoring signature (bridges param) |
| src/adapters/analyzers/srp/tests/cohesion/mod.rs | Use shared file visitor + bridge plumbing |
| src/adapters/analyzers/srp/tests/cohesion/lcom4_and_collector.rs | Update SRP cohesion APIs + module threshold API |
| src/adapters/analyzers/srp/module.rs | Replace baseline/ceiling with single threshold + ratio score |
| src/adapters/analyzers/iosp/visitor/tests/root/mod.rs | Switch to shared ProjectScope |
| src/adapters/analyzers/iosp/visitor/mod.rs | Switch to shared ProjectScope |
| src/adapters/analyzers/iosp/tests/root/mod.rs | Switch to shared ProjectScope |
| src/adapters/analyzers/iosp/tests/root/classify_basics.rs | Remove ignore_functions-related test |
| src/adapters/analyzers/iosp/tests/mod.rs | Remove iosp scope test module |
| src/adapters/analyzers/iosp/tests/classify.rs | Switch to shared ProjectScope |
| src/adapters/analyzers/iosp/mod.rs | Remove ignore_functions filtering; refactor FunctionAnalysis builder |
| src/adapters/analyzers/iosp/classify.rs | Switch to shared ProjectScope |
| src/adapters/analyzers/dry/wildcards.rs | Move visitor infra to shared + refactor glob handling |
| src/adapters/analyzers/dry/mod.rs | Move FileVisitor + DeclaredFunction to shared |
| src/adapters/analyzers/dry/match_patterns.rs | Switch visitor infra to shared |
| src/adapters/analyzers/dry/functions.rs | Switch FileVisitor import to shared |
| src/adapters/analyzers/dry/dead_code.rs | Remove ignore_functions dependency; use shared DeclaredFunction |
| src/adapters/analyzers/dry/call_targets.rs | Refactor test-vs-prod target routing helper |
| src/adapters/analyzers/dry/boilerplate/trivial_from.rs | Refactor per-item scanning via shared helper |
| src/adapters/analyzers/dry/boilerplate/mod.rs | Add reusable per-item scanning helpers |
| src/adapters/analyzers/dry/boilerplate/clone_conversion.rs | Refactor clone-heavy detection into helpers |
| src/adapters/analyzers/architecture/tests/layer_rule.rs | Refactor externals parameters into cohesive struct |
| src/adapters/analyzers/architecture/tests/forbidden_rule.rs | Update imports due to shared ProjectScope move |
| src/adapters/analyzers/architecture/tests/call_parity_golden.rs | Update example suppression + validate markers in excluded examples |
| rustqual.toml | Update config schema docs + add suppression headroom section |
| examples/architecture/call_parity/src/cli/handlers.rs | Update architecture suppression marker to targeted form |
| examples/architecture/call_parity/rustqual.toml | Update example docs for targeted architecture marker |
| examples/architecture/call_parity/README.md | Document new architecture suppression requirements |
| Cargo.toml | Bump crate version to 1.5.0 |
| Cargo.lock | Bump lockfile version entry |
| book/test-quality.md | Update suppression examples to targeted TQ forms |
| book/reference-output-formats.md | Update orphan marker examples + HTML orphan status mention |
| book/reference-configuration.md | Document schema changes (remove ignore_functions, add suppression) |
| book/module-quality.md | Update SRP module-length semantics + new suppression examples |
| book/function-quality.md | Update complexity suppression example to targeted pin form |
| book/coupling-quality.md | Update coupling suppression docs to targeted forms |
| book/code-reuse.md | Update DRY suppression docs to targeted forms |
| book/architecture-rules.md | Update architecture suppression docs to targeted family form |
| book/adapter-parity.md | Update architecture suppression references to targeted form |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…side-effect
- fmt_pin/fmt_num: format whole pins with `{:.0}` instead of `as i64`, which
would saturate a large finite pin (e.g. 1e15) to i64::MAX and misreport the
target in orphan output. Test asserts a 1e15 pin renders in full.
- wildcards.rs: replace `record.then(|| self.push_glob_warning(..))` (using
bool::then for a side effect, dropping the Option) with a match guard
`UseTree::Glob(_) if !self.skip_glob(..) =>` — clearer, no must-use Option.
- moved the orphan-rendering unit test to orphan_too_loose.rs to keep
orphan_targeting.rs under the SRP test file-length cap.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.