From 7e0ae1007caaa44a8c062ca14b685b98d65492c0 Mon Sep 17 00:00:00 2001 From: Tauan Binato <11513929+tauanbinato@users.noreply.github.com> Date: Sat, 3 Oct 2026 23:32:15 -0300 Subject: [PATCH] Point a repeat at the change's own copy and name the copies it left untouched In a check of changed lines, and in an agent's turn, a shared-logic finding whose change touched only some copies now sits at the first copy the change touched, in that copy's file, and names each untouched copy and whether its file is one the change edits. Its next step is to fix the change's copy, or mark the finding `later --note "#issue"` when sharing the logic would rewrite the untouched copies. The JSON report lists them under `untouched`. The fingerprint stays the one the repeat has in a check of whole files, so baselines still accept it, and what fails the gate is unchanged; the hook holds a turn only for the change's own copy. A file's findings are reordered and its status recomputed by one helper after grouping and after moving. --- CHANGELOG.md | 2 + jevgate-baseline.json | 95 +++++++++++- site/src/coding-agents.md | 2 +- .../src/rules/maintainability/shared-logic.md | 4 + src/catalog.rs | 2 +- src/changes.rs | 1 + src/evaluate.rs | 15 +- src/hook/tests/mod.rs | 58 ++++++++ src/schema/mod.rs | 2 +- src/schema/report.rs | 14 ++ src/tests/mod.rs | 1 + src/units/compose/comments.rs | 1 + src/units/compose/mod.rs | 17 ++- src/units/compose/redundant.rs | 1 + src/units/grouping.rs | 7 +- src/units/mod.rs | 1 + src/units/plan/mod.rs | 11 +- src/units/tests/changed.rs | 138 ++++++++++++++++++ src/units/untouched.rs | 122 ++++++++++++++++ src/units/wording/maintainability.rs | 40 +++++ src/units/wording/mod.rs | 2 +- tests/cli/changes.rs | 77 ++++++++++ 22 files changed, 587 insertions(+), 26 deletions(-) create mode 100644 src/units/untouched.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 69c68df..02c44fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ Notable changes to JevGate. Versions follow [Semantic Versioning](https://semver ## [Unreleased] +- A shared-logic finding of a `--base` check or an agent's turn whose change left some copies untouched points at the change's own copy and names each untouched copy, saying whether its file is one the change edits. Its next step is to fix the change's copy, or mark the finding `later --note "#issue"` when sharing the logic would rewrite the untouched copies; the JSON report lists them under `untouched`. The agent hook holds a turn only for the change's copy. Fingerprints, and what fails the gate, are unchanged. + ## [0.34.0] - 2026-10-03 A dismissal can say where it will be fixed: `baseline mark` takes a short `--note`, such as the issue that will fix a `later` finding, and `baseline list` prints the accepted findings by reason, as text, JSON or a Markdown checklist to paste into a cleanup issue. Findings, rules and fingerprints are unchanged. diff --git a/jevgate-baseline.json b/jevgate-baseline.json index ac288ae..40cd928 100644 --- a/jevgate-baseline.json +++ b/jevgate-baseline.json @@ -709,6 +709,17 @@ "message": "This file may do several separate kinds of work, such as separate features, layers or integrations.", "reason": "later" }, + { + "fingerprint": "7be216d324b25af08a32ae15ef77369d0bff302b27ea61aa8b48ddb0d7923986", + "rule": "maintainability/function-simplification", + "path": "src/evaluate.rs", + "line": 297, + "unit": "Session::evaluate", + "strength": "review", + "message": "`Session::evaluate` could likely be made simpler to read or change: it may be long, deeply nested, repetitive or mix separate jobs.", + "reason": "intended", + "note": "as on main; this PR only changes the compose_files call" + }, { "fingerprint": "eb77e4f652658c21a450d81c491bebefa62143f89a1776ec4871d369f950a70a", "rule": "maintainability/function-simplification", @@ -716,7 +727,8 @@ "line": 297, "strength": "review", "message": "`Session::evaluate` could likely be made simpler to read or change: it may be long, deeply nested, repetitive or mix separate jobs.", - "reason": "intended" + "reason": "intended", + "note": "as on main; this PR only changes the compose_files call" }, { "fingerprint": "f9f7ae0fc216c908682f9d8e1924d78e1bd15579a8ab2063b4f1c6c703c89493", @@ -817,6 +829,17 @@ "message": "This test file may test several separate subjects.", "reason": "later" }, + { + "fingerprint": "dae22b41c22054543983b0b7b16491335b908b0f6771909f4fa2f2598469ad57", + "rule": "maintainability/shared-logic", + "path": "src/hook/tests/mod.rs", + "line": 371, + "unit": "`a_copy_the_turn_left_untouched_never_holds_it_but_its_own_copy_does` (src/hook/tests/mod.rs:371) and `loader` (tests/cli/changes.rs:212)", + "strength": "review", + "message": "`a_copy_the_turn_left_untouched_never_holds_it_but_its_own_copy_does` (src/hook/tests/mod.rs:371) and `loader` (tests/cli/changes.rs:212) may repeat one piece of logic, so a change to it would have to be made in each place.", + "reason": "intended", + "note": "the binary's unit tests and the CLI test crate cannot share a fixture" + }, { "fingerprint": "deb9de5dfe24d73e82b3e26115ff16eaf9c982aa964adf7ffd0edda5aec4000d", "rule": "maintainability/function-simplification", @@ -1198,7 +1221,19 @@ "line": 44, "strength": "review", "message": "`comment_findings` could likely be made simpler to read or change: it may be long, deeply nested, repetitive or mix separate jobs.", - "reason": "intended" + "reason": "intended", + "note": "as on main; this PR only adds the new empty untouched field" + }, + { + "fingerprint": "0c5401505451733a846b4ef8cb1d253875e5638b656350ce74fdf1e843570218", + "rule": "maintainability/function-simplification", + "path": "src/units/compose/comments.rs", + "line": 44, + "unit": "comment_findings", + "strength": "review", + "message": "`comment_findings` could likely be made simpler to read or change: it may be long, deeply nested, repetitive or mix separate jobs.", + "reason": "intended", + "note": "as on main; this PR only adds the new empty untouched field" }, { "fingerprint": "0426cf29bc82927d5ba0d17e12226e080d2c849266ad2a8909303cd6fc633099", @@ -1227,6 +1262,17 @@ "message": "`finding` could likely be made simpler to read or change: it may be long, deeply nested, repetitive or mix separate jobs.", "reason": "intended" }, + { + "fingerprint": "c6aa055e22c707ebef59cf4a80880610aa6ca65847bfc395313b52f878a8e1c1", + "rule": "maintainability/function-simplification", + "path": "src/units/compose/mod.rs", + "line": 672, + "unit": "finding", + "strength": "review", + "message": "`finding` could likely be made simpler to read or change: it may be long, deeply nested, repetitive or mix separate jobs.", + "reason": "intended", + "note": "as on main; this PR only adds the new empty untouched field" + }, { "fingerprint": "7b9c45a11bf97f35d12cf672422f6bb47560176fa5a5a91cd0618f98fed57922", "rule": "maintainability/shared-logic", @@ -1254,6 +1300,17 @@ "message": "`locates` could likely be made simpler to read or change: it may be long, deeply nested, repetitive or mix separate jobs.", "reason": "intended" }, + { + "fingerprint": "32deda72f86d2190a95c4e251f268c89eacb26f4ee32ab2e0cf7910d52af0487", + "rule": "maintainability/shared-logic", + "path": "src/units/grouping.rs", + "line": 30, + "unit": "`group_repeats` (src/units/grouping.rs:30) and `move_findings` (src/units/untouched.rs:119)", + "strength": "review", + "message": "`group_repeats` (src/units/grouping.rs:30) and `move_findings` (src/units/untouched.rs:119) may repeat one piece of logic, so a change to it would have to be made in each place.", + "reason": "wrong", + "note": "a two-line loop calling the shared reorder helper" + }, { "fingerprint": "368fb85e44e3230070b584f9350b9ab7288309c69c105e3b3e37d6064c3b5235", "rule": "maintainability/function-simplification", @@ -1389,6 +1446,17 @@ "message": "This file may do several separate kinds of work, such as separate features, layers or integrations.", "reason": "later" }, + { + "fingerprint": "0fda0298cc8e2a6e108ca53c0f79d940f40c64d562ee309b132146e9b05fcb6e", + "rule": "maintainability/shared-logic", + "path": "src/units/plan/mod.rs", + "line": 196, + "unit": "`keep_changed` (src/units/plan/mod.rs:196) and `Changed::touches` (src/units/untouched.rs:35)", + "strength": "review", + "message": "`keep_changed` (src/units/plan/mod.rs:196) and `Changed::touches` (src/units/untouched.rs:35) may repeat one piece of logic, so a change to it would have to be made in each place.", + "reason": "wrong", + "note": "one lines.touch call on different spans: left-out code and a copy's location" + }, { "fingerprint": "c5a9f017983841bd20189be39ba271545b3a7735726e4c7c6f31eb1f06be5621", "rule": "maintainability/function-simplification", @@ -1551,6 +1619,16 @@ "message": "`setup_hooks` could likely be made simpler to read or change: it may be long, deeply nested, repetitive or mix separate jobs.", "reason": "wrong" }, + { + "fingerprint": "9d9627483b789c6a5777deb22e3d782551b8f364c26d9c14de4d3449ca88a3a5", + "rule": "maintainability/file-organization", + "path": "src/units/tests/changed.rs", + "line": 7, + "strength": "review", + "message": "This test file may test several separate subjects.", + "reason": "wrong", + "note": "every test checks what a --base check judges; the copy tests are about that too" + }, { "fingerprint": "4eab3103044d7bfdeff38c62232428c8badd13b0db6ebfe106f74f49346b1b3a", "rule": "maintainability/file-organization", @@ -1693,7 +1771,18 @@ "line": 5, "strength": "review", "message": "This test file may test several separate subjects.", - "reason": "later" + "reason": "later", + "note": "as on main: the file mixes --base, --config and GitHub format tests; split by subject in a cleanup" + }, + { + "fingerprint": "85a1fccf6670fd22ce4311acc7161f7fc94448188e5ef2180b68f13c042dd3e7", + "rule": "maintainability/file-organization", + "path": "tests/cli/changes.rs", + "line": 5, + "strength": "review", + "message": "This test file may test several separate subjects.", + "reason": "later", + "note": "as on main: the file mixes --base, --config and GitHub format tests; split by subject in a cleanup" }, { "fingerprint": "28ca0d566f63b03b435c74be6ae470766cc42b7c0c9cc1047e2bf48ca47e56ec", diff --git a/site/src/coding-agents.md b/site/src/coding-agents.md index 0a76b6d..4a4b87f 100644 --- a/site/src/coding-agents.md +++ b/site/src/coding-agents.md @@ -12,7 +12,7 @@ finding and fix it when it is right; when it is mistaken, intended or left for later, dismiss it with `jevgate baseline mark wrong|intended|later PATH:LINE`. ``` -`--base` limits the review to what changed since that revision, uncommitted and untracked changes included: the functions, tests and comments on changed lines, and copies where either copy changed. A check asks about and reports only what the change touches, and cached answers make reruns free. The exit code says what to do next: +`--base` limits the review to what changed since that revision, uncommitted and untracked changes included: the functions, tests and comments on changed lines, and copies where either copy changed. A repeat points at the change's own copy and names the [copies it left untouched](rules/maintainability/shared-logic.md#copies-a-change-left-untouched), which never hold a turn: fix the change's copy, or mark the finding `later --note "#issue"`. A check asks about and reports only what the change touches, and cached answers make reruns free. The exit code says what to do next: | Exit code | Meaning for the agent | |---|---| diff --git a/site/src/rules/maintainability/shared-logic.md b/site/src/rules/maintainability/shared-logic.md index 90ebab9..8524a20 100644 --- a/site/src/rules/maintainability/shared-logic.md +++ b/site/src/rules/maintainability/shared-logic.md @@ -8,6 +8,10 @@ A finding says two or more places may repeat one piece of logic, so a change to Copies are compared within a package and across packages linked by a local dependency, copies inside example code are notes, which are not reported, and copies in code marked deprecated are not compared. A repeat whose every site lies inside tests a test-redundancy finding names is reported by that finding alone. +## Copies a change left untouched + +With `--base`, and in an agent's turn, a repeat is asked about when the change touched at least one copy. When it left some copies untouched, the finding points at the change's own copy and names each untouched one, saying whether its file is one the change edits; the JSON report lists them under `untouched`, with `file_changed`. The change fixes its own copy, for example by reusing an untouched one; when sharing the logic would rewrite the untouched copies, it marks the finding `later --note "#issue"` and leaves them. The agent hook holds the end of a turn for the change's copy until it is fixed or marked, never for the untouched ones. The finding keeps the fingerprint its repeat has in a check of whole files, so a baseline accepts it in both. + ## How it is measured A look-here finding has not been labeled yet, so it says `Not yet measured.` and never fails the check by default. Each one a coding agent or person dismisses with a reason (`jevgate baseline mark wrong|intended|later PATH:LINE`) is counted by `jevgate baseline stats`, which is how its share of noise shows in daily use. diff --git a/src/catalog.rs b/src/catalog.rs index c5d4ffd..646747d 100644 --- a/src/catalog.rs +++ b/src/catalog.rs @@ -271,7 +271,7 @@ pub fn rule_version(key: &str) -> &'static str { match key { FILE_ORGANIZATION => "23", FUNCTION_SIMPLIFICATION => "16", - SHARED_LOGIC => "23", + SHARED_LOGIC => "24", TEST_VALUE => "7", TEST_REDUNDANCY => "4", INJECTION => "13", diff --git a/src/changes.rs b/src/changes.rs index b49f2fc..1e0427a 100644 --- a/src/changes.rs +++ b/src/changes.rs @@ -166,6 +166,7 @@ mod tests { gate: None, precision: None, preview: None, + untouched: Vec::new(), } } diff --git a/src/evaluate.rs b/src/evaluate.rs index f5196e3..a614f20 100644 --- a/src/evaluate.rs +++ b/src/evaluate.rs @@ -343,7 +343,7 @@ impl Session<'_> { } } crate::progress::phase("composing findings"); - compose_files(&plan, report); + compose_files(&plan, (inputs, self.args), report); self.guard(&plan, report); self.calibrate()?; self.progress(report) @@ -696,8 +696,13 @@ fn add_metrics(stage: &mut crate::schema::StageMetrics, m: &crate::schema::Stage } /// Compose each planned file's recorded judgments into dimensions and -/// findings, with the requests its units were first asked in. -fn compose_files(plan: &crate::units::Plan, report: &mut Report) { +/// findings, with the requests its units were first asked in. With a +/// change, a repeat whose copies it did not all touch points at its own. +fn compose_files( + plan: &crate::units::Plan, + (inputs, args): (&[Input], &CheckArgs), + report: &mut Report, +) { let mut first = BTreeMap::>::new(); for planned in &plan.requests { first.entry(planned.owner).or_default().push(planned); @@ -716,6 +721,10 @@ fn compose_files(plan: &crate::units::Plan, report: &mut Report) { } crate::units::grouping::group_repeats(&mut report.files); crate::units::compose::one_level(&mut report.files); + if args.changed_lines() { + let changed = crate::units::untouched::Changed::of(inputs); + crate::units::untouched::anchor(&changed, &mut report.files); + } } fn apply_classification(file: &mut FileResult, class: crate::file_kind::Classification) { diff --git a/src/hook/tests/mod.rs b/src/hook/tests/mod.rs index 021ad0b..ebdb921 100644 --- a/src/hook/tests/mod.rs +++ b/src/hook/tests/mod.rs @@ -363,6 +363,64 @@ fn a_turn_is_judged_on_what_it_changed_not_on_the_rest_of_its_files() { assert_eq!(send(&project, &host, stop(false))["decision"], "block"); } +#[test] +fn a_copy_the_turn_left_untouched_never_holds_it_but_its_own_copy_does() { + // Two files repeat one loader; the turn edits `b.rs`'s copy and only + // the tail of `a.rs`, whose copy it leaves alone. + let load = |name: &str, field: &str, default: &str| { + format!( + "fn {name}(path: &str) -> Result {{\n let text = std::fs::read_to_string(path)?;\n let value: Value = serde_json::from_str(&text)?;\n let name = value[\"{field}\"].as_str().unwrap_or(\"{default}\").trim().to_string();\n Ok(User {{ name }})\n}}\n" + ) + }; + let project = repository(); + project.write("jevgate.toml", "rules = [\"shared-logic\"]\n"); + project.write( + "a.rs", + &format!( + "{}\n{}", + load("load_user", "name", "anonymous"), + function("tail_a") + ), + ); + project.write("b.rs", &load("load_team", "title", "anonymous")); + project.commit_all(); + let host = reviewing(); + send(&project, &host, prompt("load teams")); + let tail = function("tail_a").replace("doubled + 1", "doubled + 2"); + project.write( + "a.rs", + &format!("{}\n{tail}", load("load_user", "name", "anonymous")), + ); + project.write("b.rs", &load("load_team", "title", "nobody")); + let blocked = send(&project, &host, stop(false)); + let reason = blocked["reason"].as_str().unwrap_or_default(); + assert!( + reason.contains("\n- b.rs:2 review maintainability/shared-logic: `load_team` (b.rs:2), which this change touched, may repeat logic that copies it left untouched also hold: `load_user` (a.rs:2, in a file this change edits)."), + "{blocked}" + ); + assert!( + reason.contains("mark the finding `later --note"), + "{reason}" + ); + // Marked for later, the change's own copy no longer holds the turn. + let marked = crate::baseline::mark( + &project.0, + &crate::baseline::Mark { + reason: crate::options::Disposition::Later, + note: Some(Some("#192".into())), + targets: &["b.rs:2".into()], + rules: &[], + }, + ); + assert_eq!(marked.unwrap(), 1); + let stopped = send(&project, &host, stop(true)); + assert!(stopped.get("decision").is_none(), "{stopped}"); + assert!( + message(&stopped).contains("b.rs:2 maintainability/shared-logic as later (#192)"), + "{stopped}" + ); +} + #[test] fn a_new_review_blocks_until_it_is_fixed_or_dismissed_with_a_reason() { // The default gate still measures hardcoded values: their reviews do diff --git a/src/schema/mod.rs b/src/schema/mod.rs index f88f1e6..bb04f0c 100644 --- a/src/schema/mod.rs +++ b/src/schema/mod.rs @@ -9,7 +9,7 @@ pub use report::*; pub const RUBRIC: &str = "jevgate-units-v1"; /// Changes how saved answers become a status. Included in the report identity /// and not in the judgment cache, so unchanged questions are not sent again. -pub const COMPOSITION: &str = "unit-composition-v13"; +pub const COMPOSITION: &str = "unit-composition-v14"; pub const SCHEMA_VERSION: u32 = 2; #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] diff --git a/src/schema/report.rs b/src/schema/report.rs index d6ca3a6..bad0626 100644 --- a/src/schema/report.rs +++ b/src/schema/report.rs @@ -194,6 +194,20 @@ pub struct Finding { /// gate never fails on it. None elsewhere and for custom questions. #[serde(default, skip_serializing_if = "Option::is_none")] pub preview: Option, + /// For a shared-logic finding of a check of changed lines, the copies + /// the change did not touch; the finding points at a copy it did. + /// Empty when the change touched every copy, or judged whole files. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub untouched: Vec, +} + +/// A copy of a shared-logic finding that its change did not touch. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct Untouched { + #[serde(flatten)] + pub location: Location, + /// Whether its file is one the change edits, elsewhere. + pub file_changed: bool, } impl Finding { diff --git a/src/tests/mod.rs b/src/tests/mod.rs index eb3f5cd..6585091 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -106,6 +106,7 @@ pub(super) fn finding(strength: crate::schema::Strength) -> crate::schema::Findi gate: None, precision: None, preview: None, + untouched: Vec::new(), } } diff --git a/src/units/compose/comments.rs b/src/units/compose/comments.rs index d92385f..056ce37 100644 --- a/src/units/compose/comments.rs +++ b/src/units/compose/comments.rs @@ -96,6 +96,7 @@ pub(super) fn comment_findings( gate: None, precision: None, preview: None, + untouched: Vec::new(), } }) .collect() diff --git a/src/units/compose/mod.rs b/src/units/compose/mod.rs index a618181..788204c 100644 --- a/src/units/compose/mod.rs +++ b/src/units/compose/mod.rs @@ -98,8 +98,7 @@ pub fn compose(plan: &FilePlan, judgments: &[Judgment], first: &[&Planned]) -> C (rule.to_string(), dimension) }) .collect(); - // Strongest first, so a note never sits above a review or consider. - findings.sort_by(|a, b| b.strength.cmp(&a.strength).then(b.rank.total_cmp(&a.rank))); + strongest_first(&mut findings); let status = file_status(&dimensions, &findings); Composed { dimensions, @@ -509,6 +508,19 @@ fn one_level_counts(rule: &str, count: &mut UnitCounts) { } } +/// Strongest first, so a note never sits above a review or consider, then +/// by rank. +fn strongest_first(findings: &mut [Finding]) { + findings.sort_by(|a, b| b.strength.cmp(&a.strength).then(b.rank.total_cmp(&a.rank))); +} + +/// A file's findings in order and its status again, after findings were +/// grouped or moved between files. +pub(super) fn reorder(file: &mut crate::schema::FileResult) { + strongest_first(&mut file.findings); + file.status = file_status(&file.dimensions, &file.findings); +} + pub(super) fn file_status( dimensions: &BTreeMap, findings: &[Finding], @@ -791,5 +803,6 @@ fn finding( gate: None, precision: None, preview: None, + untouched: Vec::new(), } } diff --git a/src/units/compose/redundant.rs b/src/units/compose/redundant.rs index f8d659e..4e96729 100644 --- a/src/units/compose/redundant.rs +++ b/src/units/compose/redundant.rs @@ -148,5 +148,6 @@ pub(super) fn group_finding(plan: &FilePlan, cluster: Cluster<'_>) -> Finding { gate: None, precision: None, preview: None, + untouched: Vec::new(), } } diff --git a/src/units/grouping.rs b/src/units/grouping.rs index 33cc29e..e5687f7 100644 --- a/src/units/grouping.rs +++ b/src/units/grouping.rs @@ -4,7 +4,7 @@ //! being added or removed. A section repeated in several documents: the //! repetition findings that share a section are one finding at the section //! most of them name, identified by it, and the others point at it. -use super::compose::{counted_status, file_status}; +use super::compose::{counted_status, reorder}; use crate::{ catalog, schema::{FileResult, Finding, Location, Strength}, @@ -28,10 +28,7 @@ pub fn group_repeats(files: &mut [FileResult]) { let mut changed = group_finished_plans(files); changed.extend(group_repeated_sections(files)); for g in changed { - let file = &mut files[g]; - file.findings - .sort_by(|a, b| b.strength.cmp(&a.strength).then(b.rank.total_cmp(&a.rank))); - file.status = file_status(&file.dimensions, &file.findings); + reorder(&mut files[g]); } } diff --git a/src/units/mod.rs b/src/units/mod.rs index 10f6d12..9955206 100644 --- a/src/units/mod.rs +++ b/src/units/mod.rs @@ -30,6 +30,7 @@ mod security; mod spacetimedb; mod sveltekit; mod test_units; +pub mod untouched; mod wording; mod workflows; diff --git a/src/units/plan/mod.rs b/src/units/plan/mod.rs index 6fd0223..1c094bd 100644 --- a/src/units/plan/mod.rs +++ b/src/units/plan/mod.rs @@ -183,15 +183,8 @@ pub fn plan( /// the parser could not read where the change touched it: a grammar gap in /// a function the change left alone was named on every change to its file. fn keep_changed(inputs: &[Input], plan: &mut Plan) { - let changes: BTreeMap<&Path, Option<&crate::revision::FileChange>> = inputs - .iter() - .map(|input| (input.result.path.as_path(), input.changed.as_ref())) - .collect(); - let touched = |location: &crate::schema::Location| match changes.get(location.path.as_path()) { - Some(Some(change)) => change.lines.touch(location.start_line, location.end_line), - Some(None) => true, - None => false, - }; + let changed = super::untouched::Changed::of(inputs); + let touched = |location: &crate::schema::Location| changed.touches(location); let mut kept = BTreeMap::>::new(); for (&owner, file) in &mut plan.files { let Some(change) = &inputs[owner].changed else { diff --git a/src/units/tests/changed.rs b/src/units/tests/changed.rs index 5bd4199..9079003 100644 --- a/src/units/tests/changed.rs +++ b/src/units/tests/changed.rs @@ -1,6 +1,7 @@ //! `--base` judging what a change touched: which units are asked and //! reported, and that `--whole-files` asks what a check of the files asks. use super::*; +use std::path::Path; /// A check of what changed since `base`, for `rules`. fn since(base: &str, rules: &[&str]) -> CheckArgs { @@ -223,6 +224,143 @@ fn a_copy_pair_is_asked_when_either_copy_changed() { assert_eq!(file_plan(&plan, "a.rs").units.len(), 1); } +/// `LOAD` loading `what` from `field`, then a tail that is no copy, +/// edited beside the copy when `tail_edited`, and with the copy's default +/// changed when `copy_edited`: a literal, so the copies still match. +fn load_copy( + what: &str, + field: &str, + tail: String, + (copy_edited, tail_edited): (bool, bool), +) -> String { + let mut copy = super::duplicates::LOAD + .replace("load_user", what) + .replace("\"name\"", field); + if copy_edited { + copy = copy.replace("anonymous", "nobody"); + } + let mut tail = tail; + if tail_edited { + tail = tail + .replace("doubled + 1", "doubled + 2") + .replace("positive += 1", "positive += 2"); + } + format!("{copy}\n{tail}") +} + +/// The shared-logic findings of `report`, by the file each is in. +fn copies_found(report: &Report) -> Vec<(&Path, &crate::schema::Finding)> { + report + .files + .iter() + .flat_map(|file| file.findings.iter().map(move |f| (file.path.as_path(), f))) + .filter(|(_, f)| f.rule == catalog::id(catalog::SHARED_LOGIC)) + .collect() +} + +#[test] +fn a_change_to_one_copy_points_at_it_and_names_the_copies_it_left_untouched() { + let other = crate::tests::other_function; + let write = |project: &Project, edited: [(bool, bool); 3]| { + project.write( + "a.rs", + &load_copy("load_user", "\"name\"", function("tail_a"), edited[0]), + ); + project.write( + "b.rs", + &load_copy("load_team", "\"title\"", other("tail_b"), edited[1]), + ); + project.write( + "c.rs", + &load_copy("load_org", "\"label\"", String::new(), edited[2]), + ); + }; + let project = Project::new(); + write(&project, [(false, false); 3]); + project.commit_all(); + // `a.rs` and `b.rs` change beside their copies, `c.rs` its copy. + write(&project, [(false, true), (false, true), (true, false)]); + let options = since("HEAD", &[catalog::SHARED_LOGIC]); + let report = run(&project, &options, &mut scripted(2)); + let found = copies_found(&report); + assert_eq!(found.len(), 1, "{found:?}"); + let (path, finding) = found[0]; + assert_eq!(path, Path::new("c.rs"), "the change's own copy"); + assert_eq!( + (finding.locations[0].path.as_path(), finding.line), + (Path::new("c.rs"), 2) + ); + let untouched: Vec<(&Path, bool)> = finding + .untouched + .iter() + .map(|u| (u.location.path.as_path(), u.file_changed)) + .collect(); + assert_eq!( + untouched, + [(Path::new("a.rs"), true), (Path::new("b.rs"), true)] + ); + assert_eq!( + finding.message, + "`load_org` (c.rs:2), which this change touched, may repeat logic that copies it left untouched also hold: `load_user` (a.rs:2, in a file this change edits); `load_team` (b.rs:2, in a file this change edits)." + ); + assert!( + finding.action.contains("later --note"), + "{}", + finding.action + ); + // A check of the whole files finds the same group, by the same + // fingerprint, at its first copy: a baseline accepts both. + let mut whole = since("HEAD", &[catalog::SHARED_LOGIC]); + whole.base = None; + let all = run(&project, &whole, &mut scripted(2)); + let found_whole = copies_found(&all); + assert_eq!(found_whole.len(), 1); + assert_eq!(found_whole[0].0, Path::new("a.rs")); + assert!(found_whole[0].1.untouched.is_empty()); + assert_eq!(found_whole[0].1.fingerprint, finding.fingerprint); + + // With every copy changed, the finding is the group's, worded as before. + write(&project, [(true, false), (true, false), (true, false)]); + let report = run(&project, &options, &mut scripted(2)); + let found = copies_found(&report); + assert_eq!(found.len(), 1); + assert_eq!(found[0].1.locations.len(), 3); + assert_eq!(found[0].0, found[0].1.locations[0].path.as_path()); + assert!(found[0].1.untouched.is_empty()); + assert!(found[0].1.message.contains("may repeat one piece of logic")); +} + +#[test] +fn a_copy_outside_the_changed_files_is_named_as_outside_the_change() { + let project = Project::new(); + project.write("a.rs", super::duplicates::LOAD); + project.write( + "c.rs", + &load_copy("load_org", "\"label\"", String::new(), (false, false)), + ); + project.commit_all(); + project.write( + "c.rs", + &load_copy("load_org", "\"label\"", String::new(), (true, false)), + ); + let mut options = since("HEAD", &[catalog::SHARED_LOGIC]); + options.context = vec!["a.rs".into()]; + let report = run(&project, &options, &mut scripted(2)); + let found = copies_found(&report); + assert_eq!(found.len(), 1, "{found:?}"); + assert_eq!(found[0].0, Path::new("c.rs")); + assert_eq!(found[0].1.untouched.len(), 1); + assert!(!found[0].1.untouched[0].file_changed); + assert!( + found[0] + .1 + .message + .ends_with("`load_user` (a.rs:2, in a file outside this change)."), + "{}", + found[0].1.message + ); +} + #[test] fn only_the_constants_and_comments_a_change_touched_are_asked() { let source = format!( diff --git a/src/units/untouched.rs b/src/units/untouched.rs new file mode 100644 index 0000000..837fa90 --- /dev/null +++ b/src/units/untouched.rs @@ -0,0 +1,122 @@ +//! What a check of changed lines touched, and the shared-logic findings +//! whose copies it did not all touch: such a finding points at a copy the +//! change touched and names the untouched ones, so that a change fixes its +//! own copy and leaves the others alone. +use super::compose::reorder; +use crate::{ + catalog, + inventory::Input, + revision::FileChange, + schema::{FileResult, Finding, Location, Untouched}, +}; +use std::{ + collections::{BTreeMap, BTreeSet}, + path::{Path, PathBuf}, +}; + +/// The files a check of changed lines judged, with the lines its change +/// touched in each, or none for a file it judged whole, such as an added one. +pub(crate) struct Changed<'a>(BTreeMap<&'a Path, Option<&'a FileChange>>); + +impl<'a> Changed<'a> { + pub fn of(inputs: &'a [Input]) -> Self { + Self( + inputs + .iter() + .map(|input| (input.result.path.as_path(), input.changed.as_ref())) + .collect(), + ) + } + + /// Whether the change touched lines at `location`. A copy in a file the + /// check did not select, such as explicit context, is unchanged. + pub fn touches(&self, location: &Location) -> bool { + match self.0.get(location.path.as_path()) { + Some(Some(change)) => change.lines.touch(location.start_line, location.end_line), + Some(None) => true, + None => false, + } + } + + /// Whether the change edits the file at `path`. + fn edits(&self, path: &Path) -> bool { + self.0.contains_key(path) + } +} + +/// Point each shared-logic finding whose copies the change did not all +/// touch at the first copy it did, in that copy's file, and name the +/// untouched copies in its message and `untouched`. The fingerprint stays +/// the one its pair was found with, so a baseline still accepts it. +pub(crate) fn anchor(changed: &Changed<'_>, files: &mut [FileResult]) { + let rule = catalog::id(catalog::SHARED_LOGIC); + let at: BTreeMap = files + .iter() + .enumerate() + .map(|(f, file)| (file.path.clone(), f)) + .collect(); + let mut moves = Vec::new(); + for (f, file) in files.iter_mut().enumerate() { + for (i, finding) in file.findings.iter_mut().enumerate() { + if finding.rule != rule || !finding.untouched.is_empty() { + continue; + } + let (touched, untouched): (Vec, Vec) = finding + .locations + .iter() + .cloned() + .partition(|l| changed.touches(l)); + // A touched copy is in a file the check judged. + let Some(&to) = touched.first().and_then(|own| at.get(&own.path)) else { + continue; + }; + if untouched.is_empty() { + continue; + } + if to != f { + moves.push((f, i, to)); + } + reword(finding, touched, untouched, changed); + } + } + move_findings(files, moves); +} + +fn reword( + finding: &mut Finding, + touched: Vec, + untouched: Vec, + changed: &Changed<'_>, +) { + finding.untouched = untouched + .into_iter() + .map(|location| Untouched { + file_changed: changed.edits(&location.path), + location, + }) + .collect(); + let (message, action) = super::wording::untouched_wording(&touched, &finding.untouched); + finding.message = message; + finding.action = action.into(); + finding.line = touched[0].start_line; + finding.locations = touched; + finding + .locations + .extend(finding.untouched.iter().map(|u| u.location.clone())); +} + +/// Move each finding `(file, index, to)` to the file `to`, then restore +/// the order and status of the files whose findings changed. +fn move_findings(files: &mut [FileResult], mut moves: Vec<(usize, usize, usize)>) { + // From the last index down, so earlier indexes stay valid. + moves.sort_by_key(|&(from, index, _)| std::cmp::Reverse((from, index))); + let mut changed = BTreeSet::new(); + for (from, index, to) in moves { + let finding = files[from].findings.remove(index); + files[to].findings.push(finding); + changed.extend([from, to]); + } + for f in changed { + reorder(&mut files[f]); + } +} diff --git a/src/units/wording/maintainability.rs b/src/units/wording/maintainability.rs index bf2fbf4..46cafcb 100644 --- a/src/units/wording/maintainability.rs +++ b/src/units/wording/maintainability.rs @@ -108,3 +108,43 @@ pub(in crate::units) fn look_wording(unit: &crate::units::UnitPlan) -> Wording { ), } } + +/// A shared-logic finding whose change touched only some copies: the +/// change's copies first, then each untouched copy and whether its file is +/// one the change edits. The change fixes its own copy and leaves the others. +pub(in crate::units) fn untouched_wording( + touched: &[crate::schema::Location], + untouched: &[crate::schema::Untouched], +) -> Wording { + let own: Vec = touched.iter().map(|l| copy(l, None)).collect(); + let others: Vec = untouched + .iter() + .map(|u| { + let file = if u.file_changed { + "in a file this change edits" + } else { + "in a file outside this change" + }; + copy(&u.location, Some(file)) + }) + .collect(); + ( + format!( + "{}, which this change touched, may repeat logic that copies it left untouched also hold: {}.", + crate::output::join(&own), + others.join("; ") + ), + "Fix this change's copy, such as by reusing an untouched one; if sharing the logic would rewrite the untouched copies, mark the finding `later --note \"#issue\"`", + ) +} + +/// A copy by its function and place, with what else there is to say of it. +fn copy(location: &crate::schema::Location, more: Option<&str>) -> String { + let at = format!("{}:{}", location.path.display(), location.start_line); + match (&location.symbol, more) { + (Some(symbol), Some(more)) => format!("`{symbol}` ({at}, {more})"), + (Some(symbol), None) => format!("`{symbol}` ({at})"), + (None, Some(more)) => format!("{at} ({more})"), + (None, None) => at, + } +} diff --git a/src/units/wording/mod.rs b/src/units/wording/mod.rs index d4a6121..aa0d7b0 100644 --- a/src/units/wording/mod.rs +++ b/src/units/wording/mod.rs @@ -22,7 +22,7 @@ pub(super) use documentation::{ comment_reason, comment_wording, doc_pair_wording, document_wording, plan_wording, section_wording, stale_wording, }; -pub(super) use maintainability::{function_wording, look_wording}; +pub(super) use maintainability::{function_wording, look_wording, untouched_wording}; pub(super) use security::{handler_wording, module_wording, privilege_wording, security_wording}; pub(super) use test_rules::{law_wording, test_pair_wording, test_wording}; diff --git a/tests/cli/changes.rs b/tests/cli/changes.rs index 43b830f..9bfba78 100644 --- a/tests/cli/changes.rs +++ b/tests/cli/changes.rs @@ -206,3 +206,80 @@ fn github_format_writes_a_job_summary_and_the_agent_text() { let text = std::fs::read_to_string(summary).unwrap(); assert!(text.starts_with("### JevGate: "), "{text}"); } + +/// A loader repeated in each file, named `name`, reading `field`. +fn loader(name: &str, field: &str, default: &str) -> String { + format!( + "fn {name}(path: &str) -> Result {{\n let text = std::fs::read_to_string(path)?;\n let value: Value = serde_json::from_str(&text)?;\n let name = value[\"{field}\"].as_str().unwrap_or(\"{default}\").trim().to_string();\n Ok(User {{ name }})\n}}\n" + ) +} + +#[test] +fn base_points_a_repeat_at_the_copy_the_change_touched() { + use mock_provider::{MockProvider, Reply, answer}; + let provider = MockProvider::start(|received| Reply::json(200, &answer(&received.json(), 2))); + let tail = "\nfn tail(values: &[i32]) -> i32 {\n let mut total = 0;\n for value in values {\n total += value;\n }\n total + 1\n}\n"; + let project = Project::committed_with(&[ + ( + "a.rs", + &format!("{}{tail}", loader("load_user", "name", "anonymous")), + ), + ("b.rs", &loader("load_team", "title", "anonymous")), + ]); + let check = |project: &Project| -> serde_json::Value { + let output = project + .asking(&provider, "key") + .args(["check", "--base", "HEAD", "--format", "json"]) + .args(["--rule", "maintainability/shared-logic"]) + .output() + .unwrap(); + serde_json::from_slice(&output.stdout) + .unwrap_or_else(|_| panic!("{}", String::from_utf8_lossy(&output.stderr))) + }; + let repeats = |report: &serde_json::Value| -> Vec<(String, serde_json::Value)> { + report["files"] + .as_array() + .unwrap() + .iter() + .flat_map(|file| { + file["findings"] + .as_array() + .unwrap() + .iter() + .map(move |f| (file["path"].as_str().unwrap().to_string(), f.clone())) + }) + .collect() + }; + // The change edits `a.rs` beside its copy and `b.rs`'s copy. + let write = |a_default: &str, b_default: &str, tail_end: &str| { + let a = format!( + "{}{}", + loader("load_user", "name", a_default), + tail.replace("total + 1", tail_end) + ); + std::fs::write(project.0.join("a.rs"), a).unwrap(); + std::fs::write( + project.0.join("b.rs"), + loader("load_team", "title", b_default), + ) + .unwrap(); + }; + write("anonymous", "nobody", "total + 2"); + let found = repeats(&check(&project)); + assert_eq!(found.len(), 1, "{found:?}"); + let (path, finding) = &found[0]; + assert_eq!( + (path.as_str(), &finding["line"]), + ("b.rs", &serde_json::json!(2)) + ); + assert_eq!( + finding["untouched"], + serde_json::json!([{"path": "a.rs", "start_line": 2, "end_line": 5, "symbol": "load_user", "file_changed": true}]) + ); + // A change to both copies keeps the repeat as it was found. + write("nobody", "nobody", "total + 1"); + let found = repeats(&check(&project)); + assert_eq!(found.len(), 1, "{found:?}"); + assert_eq!(found[0].0, "a.rs"); + assert!(found[0].1.get("untouched").is_none(), "{:?}", found[0].1); +}