From 1e0c7641898e99362edbb397ad8cf998d5b43f38 Mon Sep 17 00:00:00 2001 From: Tauan BF <11513929+tauanbinato@users.noreply.github.com> Date: Sun, 27 Sep 2026 11:29:33 -0300 Subject: [PATCH 1/3] Lower short function splits and mirrored test copies, ask the kind of a rechecked split, split unit_outcome --- src/catalog.rs | 6 +- src/tests/gating.rs | 10 +- src/tests/mod.rs | 7 ++ src/units/compose.rs | 5 +- src/units/outcome/documentation.rs | 15 ++- src/units/outcome/exposure.rs | 14 +++ src/units/outcome/maintainability.rs | 56 ++++++++--- src/units/outcome/mod.rs | 142 ++++++++++----------------- src/units/outcome/security.rs | 24 +++++ src/units/outcome/test_rules.rs | 35 ++++++- src/units/tests/duplicates.rs | 16 +++ src/units/tests/functions.rs | 9 +- src/units/tests/mod.rs | 2 +- src/units/wording/mod.rs | 4 +- src/units/wording/test_rules.rs | 6 +- 15 files changed, 227 insertions(+), 124 deletions(-) diff --git a/src/catalog.rs b/src/catalog.rs index 25b7ace..d0fdd7f 100644 --- a/src/catalog.rs +++ b/src/catalog.rs @@ -287,9 +287,9 @@ pub fn rules() -> Vec { pub fn rule_version(key: &str) -> &'static str { match key { - FILE_ORGANIZATION => "20", - FUNCTION_SIMPLIFICATION => "15", - SHARED_LOGIC => "21", + FILE_ORGANIZATION => "21", + FUNCTION_SIMPLIFICATION => "16", + SHARED_LOGIC => "22", TEST_VALUE => "7", TEST_REDUNDANCY => "4", INJECTION => "10", diff --git a/src/tests/gating.rs b/src/tests/gating.rs index 0308b84..af1ff39 100644 --- a/src/tests/gating.rs +++ b/src/tests/gating.rs @@ -4,7 +4,7 @@ use super::*; #[test] fn gate_fails_only_on_the_configured_results() { let project = Project::new(); - project.write("lib.rs", &function("f")); + project.write("lib.rs", &long_function("f")); let mut options = args(); // Mock level, report status, the default gate's exit code, and the gate that fails it. let cases = [ @@ -38,7 +38,7 @@ fn gate_fails_only_on_the_configured_results() { #[test] fn a_rule_level_fails_the_gate_only_for_that_rule() { let project = Project::new(); - project.write("lib.rs", &function("f")); + project.write("lib.rs", &long_function("f")); let mut options = args(); let mut mock = Mock { level: 4, @@ -63,8 +63,8 @@ fn a_rule_level_fails_the_gate_only_for_that_rule() { #[test] fn a_scope_makes_its_paths_report_only_while_other_files_gate() { let project = Project::new(); - project.write("src/lib.rs", &function("f")); - project.write("scripts/tool.rs", &function("g")); + project.write("src/lib.rs", &long_function("f")); + project.write("scripts/tool.rs", &long_function("g")); let mut options = args(); options.fail_on = vec![options::FailOn::Consider]; let mut mock = Mock { @@ -108,7 +108,7 @@ fn publish(project: &Project, report: &schema::Report) { #[test] fn baselined_findings_do_not_fail_the_gate_but_new_ones_do() { let project = Project::new(); - project.write("lib.rs", &function("f")); + project.write("lib.rs", &long_function("f")); let options = args(); let mut review = Mock { level: 2, diff --git a/src/tests/mod.rs b/src/tests/mod.rs index 3f738d4..278bb74 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -75,6 +75,13 @@ pub(super) fn function(name: &str) -> String { ) } +/// A function longer than twenty lines, whose split can be a consider. +pub(super) fn long_function(name: &str) -> String { + format!( + "fn {name}(values: &[i32]) -> i32 {{\n let mut total = 0;\n for value in values {{\n total += value;\n }}\n let mut largest = i32::MIN;\n for value in values {{\n if *value > largest {{\n largest = *value;\n }}\n }}\n let mut smallest = i32::MAX;\n for value in values {{\n if *value < smallest {{\n smallest = *value;\n }}\n }}\n let spread = largest - smallest;\n let doubled = total * 2;\n doubled + spread + 1\n}}\n" + ) +} + /// Levels: 0 answers the bottom of every scale (clear), 1 the middle (consider, /// or a note where the middle says the code is fine), 2 the top (review), /// 3 spreads probability (uncertain), 4 leans to the top without reaching review. diff --git a/src/units/compose.rs b/src/units/compose.rs index 498ed93..da50252 100644 --- a/src/units/compose.rs +++ b/src/units/compose.rs @@ -368,7 +368,8 @@ pub fn unkinded_units(plan: &FilePlan, judgments: &[Judgment]) -> BTreeSet bool { if ![catalog::FILE_ORGANIZATION, catalog::LARGE_DOCS].contains(&unit.rule) || !answers(judgments, &unit.id, Pass::Trace).is_empty() @@ -385,7 +386,7 @@ fn unkinded_split(unit: &UnitPlan, judgments: &[Judgment]) -> bool { .get("split") .is_some_and(|a| match benefit(a) { Outcome::Uncertain(_) => true, - Outcome::Consider(_) | Outcome::Review(_) => document, + Outcome::Consider(_) | Outcome::Review(_) => document || pass == Pass::Recheck, _ => false, }) } diff --git a/src/units/outcome/documentation.rs b/src/units/outcome/documentation.rs index 785f883..613bdb3 100644 --- a/src/units/outcome/documentation.rs +++ b/src/units/outcome/documentation.rs @@ -19,9 +19,22 @@ pub(super) fn kind_share(answer: Option<&Answer>, kind: &str) -> Option { (mass > 0.0).then(|| probabilities.get(kind).copied().unwrap_or(0.0) / mass) } +/// A stale section's or finished plan's check, a cleanup at most. +pub(super) fn staleness_outcome<'a>( + get: &impl Fn(&str) -> Option<&'a Answer>, + detail: &Detail, +) -> Option { + let question = if matches!(detail, Detail::Plan { .. }) { + "plan" + } else { + "relies" + }; + get(question).map(|a| stale_outcome(cleanup(noul(a)), get("role"))) +} + /// An undecided staleness check, cleared when the section's missing names /// are clearly not a current part of the repository. -pub(super) fn stale_outcome(outcome: Outcome, role: Option<&Answer>) -> Outcome { +fn stale_outcome(outcome: Outcome, role: Option<&Answer>) -> Outcome { match outcome { Outcome::Uncertain(_) if kind_share(role, "repository").is_some_and(|share| at_least(1.0 - share)) => diff --git a/src/units/outcome/exposure.rs b/src/units/outcome/exposure.rs index a7d06d5..b95e747 100644 --- a/src/units/outcome/exposure.rs +++ b/src/units/outcome/exposure.rs @@ -192,6 +192,20 @@ fn exposure_signal(question: &str, answer: &Answer, own: Option, away: } } +/// A Django settings module's weak settings, one level lower when the +/// module applies only in development. +pub(in crate::units) fn settings_module_outcome<'a>( + get: &impl Fn(&str) -> Option<&'a Answer>, +) -> Option { + django_settings_outcome(get).map(|outcome| { + if matches!(get("dev_only").map(noul), Some(Outcome::Review(_))) { + lowered(outcome) + } else { + outcome + } + }) +} + /// Weak settings of Django code, whose checks name its settings and /// decorators: a settings module assigns dozens of settings, and the /// presence question found development settings weak (`DEBUG = True`, any diff --git a/src/units/outcome/maintainability.rs b/src/units/outcome/maintainability.rs index 7425f83..7c9e8bb 100644 --- a/src/units/outcome/maintainability.rs +++ b/src/units/outcome/maintainability.rs @@ -1,12 +1,23 @@ //! Outcomes of the maintainability rules: functions, file organization, shared logic and hardcoded values. use super::*; -/// The stronger of splitting and (for deeply nested functions only) flattening. +/// Lines a function may span and still read in one look. +const SHORT_FUNCTION_LINES: usize = 20; + +/// The stronger of splitting and (for deeply nested functions only) +/// flattening. Splitting a function of 20 lines or fewer is at most a note: +/// of 10 such considers labeled by hand, 1 was right, while the split of a +/// function over 30 lines was right in 142 of 190. pub(in crate::units) fn function_outcome( split: Option<&Answer>, flatten: Option<&Answer>, + lines: usize, ) -> Option { - let mut outcomes = vec![benefit(split?)]; + let split = match benefit(split?) { + Outcome::Consider(p) if lines <= SHORT_FUNCTION_LINES => Outcome::Note(p), + outcome => outcome, + }; + let mut outcomes = vec![split]; outcomes.extend(flatten.map(benefit)); Some(strongest(&outcomes)) } @@ -60,15 +71,20 @@ pub(in crate::units) fn values_outcome<'a>( Some(strongest(&outcomes)) } -/// The split Score, or when it stays undecided, the kind of file: the kinds -/// that serve one feature ruling a split out clear, the kinds that serve -/// several reaching the threshold a consider. +/// The split Score, weighed with the kind of file once it is asked: the +/// kinds that serve one feature reaching the threshold clear an undecided +/// split or a finding, and the kinds that serve several reaching it raise an +/// undecided split to a consider. The kind is asked of a finding only when +/// the recheck raised it from an undecided first answer. pub(in crate::units) fn organization_outcome( split: Option<&Answer>, kind: Option<&Answer>, ) -> Option { let outcome = benefit(split?); - let (Outcome::Uncertain(_), Some(Answer::Choice { probabilities, .. })) = (outcome, kind) + let ( + Outcome::Uncertain(_) | Outcome::Consider(_) | Outcome::Review(_), + Some(Answer::Choice { probabilities, .. }), + ) = (outcome, kind) else { return Some(outcome); }; @@ -83,7 +99,7 @@ pub(in crate::units) fn organization_outcome( .sum(); Some(if at_least(1.0 - several) { Outcome::Clear - } else if at_least(several) { + } else if at_least(several) && matches!(outcome, Outcome::Uncertain(_)) { Outcome::Consider(several) } else { outcome @@ -112,6 +128,10 @@ pub(in crate::units) fn several_kind<'a>( /// Lines a copy may span and still be short: sharing it saves little. const SHORT_COPY_LINES: usize = 4; +/// Lines a copy between test cases of different files may span and still +/// only mirror the other file's tests. +const MIRRORED_CASE_LINES: usize = 12; + /// Repetition the behavior requires is not a concern. Copies whose every site /// is inside test cases are one level lower: spelling out each case is how /// tests are written, so a table of cases or a fixture is a style choice. @@ -124,6 +144,11 @@ const SHORT_COPY_LINES: usize = 4; /// in test code outside its cases, in fixtures, helpers and setup, are at /// most a consider: labeled by hand on 25 projects, 13 of 19 such reviews /// were a level too strong, while 11 of 12 considers were right as they were. +/// Copies of up to twelve lines between test cases in different files are +/// notes too: tests of separate modules or rules repeat the same setup +/// because the code they test is parallel, and a helper shared across test +/// files would couple them. Of 57 such considers labeled by hand, 11 were +/// right; on the held-out projects 2 of 24. pub(in crate::units) fn shared_outcome( required: Option<&Answer>, same: Option<&Answer>, @@ -133,10 +158,17 @@ pub(in crate::units) fn shared_outcome( return Some(Outcome::Clear); } let same = score(same?); - let short = unit - .locations - .iter() - .all(|l| l.end_line + 1 - l.start_line <= SHORT_COPY_LINES); + let within = |lines: usize| { + unit.locations + .iter() + .all(|l| l.end_line + 1 - l.start_line <= lines) + }; + let short = within(SHORT_COPY_LINES); + let mirrored = within(MIRRORED_CASE_LINES) + && unit + .locations + .iter() + .any(|l| l.path != unit.locations[0].path); // Examples spell a flow out on purpose, often once per variant: // django-styleguide shows each Google login step as a DRF API and as a // plain Django view. @@ -146,7 +178,7 @@ pub(in crate::units) fn shared_outcome( .all(|l| crate::analysis::clones::example_code(&l.path)); Some(match (&unit.detail, same) { (_, Outcome::Review(p) | Outcome::Consider(p)) if examples => Outcome::Note(p), - (Detail::Pair { in_cases: true, .. }, _) if short => lowered(lowered(same)), + (Detail::Pair { in_cases: true, .. }, _) if short || mirrored => lowered(lowered(same)), // Short copies in test support, such as a run of one-line assertions. (Detail::Pair { in_tests: true, .. }, _) if short => lowered(same), (Detail::Pair { in_cases: true, .. }, _) => lowered(same), diff --git a/src/units/outcome/mod.rs b/src/units/outcome/mod.rs index d81167c..bdb0a52 100644 --- a/src/units/outcome/mod.rs +++ b/src/units/outcome/mod.rs @@ -24,9 +24,11 @@ mod test_rules; use access::access_outcome; pub(super) use comments::{comment_concern_kind, comment_outcome, comment_signals}; -use documentation::stale_outcome; +use documentation::staleness_outcome; pub(super) use documentation::{document_outcome, document_split, section_signals}; -pub(super) use exposure::{Messages, django_settings_outcome, exposure_outcome, messages}; +pub(super) use exposure::{ + Messages, django_settings_outcome, exposure_outcome, messages, settings_module_outcome, +}; pub(super) use injection::{RESOURCE_CHECKS, injection_outcome, origin_outcome}; pub(super) use maintainability::{ benign_key, function_outcome, organization_outcome, several_kind, shared_outcome, @@ -34,8 +36,8 @@ pub(super) use maintainability::{ }; use pairs::doc_pair_outcome; pub(super) use pairs::{disagreement, pair_signals, repeated}; -pub(super) use security::{checks, choice_mass, settled_checks}; -pub(super) use test_rules::{redundancy_outcome, test_value_outcome}; +pub(super) use security::{checks, choice_mass, settled_checks, workflows_outcome}; +pub(super) use test_rules::{test_pair_outcome, test_value_outcome}; #[derive(Clone, Copy, Debug, PartialEq)] pub enum Outcome { @@ -186,9 +188,17 @@ pub(super) fn choice(answer: Option<&Answer>) -> Option<(&str, f64)> { pub(super) type Answers<'a> = BTreeMap<&'a str, &'a Answer>; pub(super) fn unit_outcome(unit: &UnitPlan, answers: &Answers<'_>) -> Outcome { + let outcome = rule_outcome(unit, answers).unwrap_or(Outcome::Missing); + in_examples(unit, outcome) +} + +/// The outcome of a unit's answers under its rule's policy. +fn rule_outcome(unit: &UnitPlan, answers: &Answers<'_>) -> Option { let get = |q: &str| answers.get(q).copied(); - let result = match unit.rule { - catalog::FUNCTION_SIMPLIFICATION => function_outcome(get("split"), get("flatten")), + match unit.rule { + catalog::FUNCTION_SIMPLIFICATION => { + function_outcome(get("split"), get("flatten"), unit.lines) + } // Who calls a group is evidence in the outline, not a gate: a module // with one caller still helps a reader find a feature of a large file. // A test file's layout is advice, one level lower. @@ -203,33 +213,7 @@ pub(super) fn unit_outcome(unit: &UnitPlan, answers: &Answers<'_>) -> Outcome { } catalog::SHARED_LOGIC => shared_outcome(get("required"), get("same"), unit), catalog::TEST_VALUE => test_value_outcome(&get), - catalog::TEST_REDUNDANCY => { - let identical = matches!( - unit.detail, - Detail::TestPair { - identical: true, - .. - } - ); - let table = !matches!(unit.detail, Detail::TestPair { table: false, .. }); - // Express's `when false` and `when true` groups run one body on - // apps their `before` hooks build differently. - let unseen_setup = matches!( - unit.detail, - Detail::TestPair { - unseen_setup: true, - .. - } - ); - get("overlap").map(|overlap| { - // A suggestion to parameterize is only a note where tests cannot be. - match redundancy_outcome(overlap, &get, identical) { - Outcome::Review(p) if unseen_setup => Outcome::Consider(p), - Outcome::Consider(p) if !table => Outcome::Note(p), - outcome => outcome, - } - }) - } + catalog::TEST_REDUNDANCY => test_pair_outcome(&get, &unit.detail), catalog::HARDCODED_VALUES => values_outcome(&get, &unit.detail), // "Slightly" says the comment adds only detail: a note, as a benefit. catalog::LAWS => get("states") @@ -253,13 +237,7 @@ pub(super) fn unit_outcome(unit: &UnitPlan, answers: &Answers<'_>) -> Outcome { exposure_outcome(unit.rule, &get, &["logs_secret", "error_details"]) } catalog::UNSAFE_SETTINGS if unit.name == crate::units::security::SETTINGS_MODULE => { - django_settings_outcome(&get).map(|outcome| { - if matches!(get("dev_only").map(noul), Some(Outcome::Review(_))) { - lowered(outcome) - } else { - outcome - } - }) + settings_module_outcome(&get) } catalog::UNSAFE_SETTINGS if matches!(unit.detail, Detail::Security { django: true, .. }) => @@ -268,53 +246,25 @@ pub(super) fn unit_outcome(unit: &UnitPlan, answers: &Answers<'_>) -> Outcome { } catalog::UNSAFE_SETTINGS => exposure_outcome(unit.rule, &get, &["weakened"]), catalog::ACCESS_CONTROL => access_outcome(&get, &unit.detail), - catalog::WORKFLOWS => { - // An undecided concern is clear when its recheck Choice rules it - // out: no expression holds outside text, or the job runs only the - // base branch's code or none. - let settles = [ - ("outside", "outside_source", &["none"][..]), - ("untrusted", "pull_request_code", &["base", "none"][..]), - ]; - let asked: Vec = settles - .iter() - .filter_map(|(question, choice, clears)| { - Some(match get(question).map(noul)? { - Outcome::Uncertain(_) - if security::choice_mass(get(choice), clears).is_some_and(at_least) => - { - Outcome::Clear - } - other => other, - }) - }) - .collect(); - (!asked.is_empty()).then(|| strongest(&asked)) - } + catalog::WORKFLOWS => workflows_outcome(&get), catalog::LARGE_DOCS => document_outcome(&get), - catalog::DOC_STALENESS => { - let question = if matches!(unit.detail, Detail::Plan { .. }) { - "plan" - } else { - "relies" - }; - get(question).map(|a| stale_outcome(cleanup(noul(a)), get("role"))) - } + catalog::DOC_STALENESS => staleness_outcome(&get, &unit.detail), catalog::DOC_DUPLICATION => doc_pair_outcome(&get), catalog::AGENT_CONTEXT => { section_signals(&get).map(|s| strongest(&s.iter().map(|(_, o)| *o).collect::>())) } _ => None, - }; - let outcome = result.unwrap_or(Outcome::Missing); - // Example code is written to be read in one piece, and its settings to be - // copied and changed: debug-toolbar's example project keeps a literal - // SECRET_KEY, sqlmodel's tutorials run each step in one function, and - // express's examples keep session cookies simple. Its findings are at - // most notes; a place it passes outside input to a query or command is - // still a consider, since examples are copied, and so is a Bend 2 law - // that states less than its comment: a demo's laws show how to state - // one. + } +} + +/// Example code is written to be read in one piece, and its settings to be +/// copied and changed: debug-toolbar's example project keeps a literal +/// SECRET_KEY, sqlmodel's tutorials run each step in one function, and +/// express's examples keep session cookies simple. Its findings are at most +/// notes; a place it passes outside input to a query or command is still a +/// consider, since examples are copied, and so is a Bend 2 law that states +/// less than its comment: a demo's laws show how to state one. +fn in_examples(unit: &UnitPlan, outcome: Outcome) -> Outcome { let example = unit .locations .iter() @@ -339,19 +289,16 @@ pub(super) fn unit_outcome(unit: &UnitPlan, answers: &Answers<'_>) -> Outcome { /// law does not state hold the policy's share, or when the law likely /// checks particular inputs its comment generalizes; clear when the /// Choice's other options hold the policy's share and the inputs are not -/// particular. The particular-inputs question takes the located-part share: -/// on thirteen Bend 2 projects the 4 laws at 0.65 or more were right (two -/// of them at 0.72 and 0.78), and the highest below was a sanity check at -/// 0.51. +/// particular. fn law_recheck(relation: &Answer, fixed: Option<&Answer>) -> Outcome { - let fixed = match fixed { - Some(Answer::Noul { noul, .. }) => Some(*noul), - _ => None, + if let Some(p) = particular_inputs(fixed) { + return Outcome::Consider(p); + } + let general = match fixed { + Some(Answer::Noul { noul, .. }) => at_least(1.0 - noul), + _ => true, }; - let particular = fixed.is_some_and(|p| probability_at_least(p, LOCATION_PROBABILITY)); - let general = fixed.is_none_or(|p| at_least(1.0 - p)); match choice_mass(Some(relation), &questions::LAW_GAPS) { - _ if particular => Outcome::Consider(fixed.unwrap_or_default()), Some(gap) if at_least(gap) => Outcome::Consider(gap), Some(gap) if general && at_least(1.0 - gap) => Outcome::Clear, Some(gap) => Outcome::Uncertain(gap), @@ -359,6 +306,19 @@ fn law_recheck(relation: &Answer, fixed: Option<&Answer>) -> Outcome { } } +/// How likely a law checks particular inputs where its comment speaks of +/// any, when that reaches the located-part share. On thirteen Bend 2 +/// projects the 4 laws at 0.65 or more were right (two of them at 0.72 and +/// 0.78), and the highest below was a sanity check at 0.51. +pub(in crate::units) fn particular_inputs(fixed: Option<&Answer>) -> Option { + match fixed { + Some(Answer::Noul { noul, .. }) if probability_at_least(*noul, LOCATION_PROBABILITY) => { + Some(*noul) + } + _ => None, + } +} + /// A Score whose two lower levels are acceptable: review at its top level, /// clear when the two lower levels reach the threshold, otherwise uncertain. pub(in crate::units) fn acceptable_levels(answer: &Answer) -> Outcome { diff --git a/src/units/outcome/security.rs b/src/units/outcome/security.rs index 0914cdf..a5a8739 100644 --- a/src/units/outcome/security.rs +++ b/src/units/outcome/security.rs @@ -68,3 +68,27 @@ pub(in crate::units) fn choice_mass(answer: Option<&Answer>, options: &[&str]) - .sum() }) } + +/// A workflow's concerns. An undecided concern is clear when its recheck +/// Choice rules it out: no expression holds outside text, or the job runs +/// only the base branch's code or none. +pub(in crate::units) fn workflows_outcome<'a>( + get: &impl Fn(&str) -> Option<&'a Answer>, +) -> Option { + let settles = [ + ("outside", "outside_source", &["none"][..]), + ("untrusted", "pull_request_code", &["base", "none"][..]), + ]; + let asked: Vec = settles + .iter() + .filter_map(|(question, choice, clears)| { + Some(match get(question).map(noul)? { + Outcome::Uncertain(_) if choice_mass(get(choice), clears).is_some_and(at_least) => { + Outcome::Clear + } + other => other, + }) + }) + .collect(); + (!asked.is_empty()).then(|| strongest(&asked)) +} diff --git a/src/units/outcome/test_rules.rs b/src/units/outcome/test_rules.rs index 71bffe4..93f444f 100644 --- a/src/units/outcome/test_rules.rs +++ b/src/units/outcome/test_rules.rs @@ -59,6 +59,39 @@ fn internal_outcome(p: f64, reads: Option<&Answer>) -> Outcome { } } +/// A pair of tests: its redundancy, one level lower when their setup runs +/// where the pair does not show it (Express's `when false` and `when true` +/// groups run one body on apps their `before` hooks build differently), and a +/// suggestion to parameterize only a note where the tests cannot share a +/// table. +pub(in crate::units) fn test_pair_outcome<'a>( + get: &impl Fn(&str) -> Option<&'a Answer>, + detail: &Detail, +) -> Option { + let identical = matches!( + detail, + Detail::TestPair { + identical: true, + .. + } + ); + let table = !matches!(detail, Detail::TestPair { table: false, .. }); + let unseen_setup = matches!( + detail, + Detail::TestPair { + unseen_setup: true, + .. + } + ); + get("overlap").map( + |overlap| match redundancy_outcome(overlap, get, identical) { + Outcome::Review(p) if unseen_setup => Outcome::Consider(p), + Outcome::Consider(p) if !table => Outcome::Note(p), + outcome => outcome, + }, + ) +} + /// Two tests that check the same behavior with equivalent inputs make a /// review: one of them adds nothing. A review also needs both tests to /// exercise the same input case and expect the same outcome, each at the @@ -71,7 +104,7 @@ fn internal_outcome(p: f64, reads: Option<&Answer>) -> Outcome { /// is available" did, which deleted nothing. When asked (for Ruby) whether /// each checks something the other does not, a review also needs that ruled /// out. -pub(in crate::units) fn redundancy_outcome<'a>( +fn redundancy_outcome<'a>( overlap: &Answer, get: &impl Fn(&str) -> Option<&'a Answer>, identical: bool, diff --git a/src/units/tests/duplicates.rs b/src/units/tests/duplicates.rs index f574ee7..dba6c82 100644 --- a/src/units/tests/duplicates.rs +++ b/src/units/tests/duplicates.rs @@ -77,6 +77,22 @@ fn copies_in_test_code_are_at_most_a_consider_and_across_cases_one_level_lower() "", ); assert_eq!(strength(&short).0, Strength::Note); + // The same cases in two test files mirror each other: a note. + let project = Project::new(); + let (a, b) = cases.split_at(cases.find("\n\n#[test]").unwrap()); + project.write("tests/a.rs", a); + project.write("tests/b.rs", b.trim_start()); + let mut same = scripted(2); + same.overrides + .push(("required", json!({"type":"noul","noul":0.05}))); + let report = run(&project, &options, &mut same); + let finding = report + .files + .iter() + .flat_map(|f| &f.findings) + .next() + .unwrap(); + assert_eq!(finding.strength, Strength::Note, "{}", finding.message); } #[test] diff --git a/src/units/tests/functions.rs b/src/units/tests/functions.rs index 5d28177..c651049 100644 --- a/src/units/tests/functions.rs +++ b/src/units/tests/functions.rs @@ -170,7 +170,7 @@ fn a_torn_note_gets_the_recheck_and_takes_its_decisive_answer() { #[test] fn a_function_clears_when_the_split_level_is_ruled_out() { let project = Project::new(); - project.write("lib.rs", &function("borderline")); + project.write("lib.rs", &long_function("borderline")); let mut options = args(); options.refresh = true; only(&mut options, catalog::FUNCTION_SIMPLIFICATION); @@ -181,6 +181,13 @@ fn a_function_clears_when_the_split_level_is_ruled_out() { }; assert_eq!(status(spread(0.5, 0.35, 0.15)), Status::Clear); assert_eq!(status(spread(0.1, 0.3, 0.6)), Status::Consider); + project.write("lib.rs", &function("short")); + assert_eq!( + status(spread(0.1, 0.3, 0.6)), + Status::Note, + "a function of twenty lines or fewer reads in one look" + ); + project.write("lib.rs", &long_function("borderline")); assert_eq!( status(spread(0.1, 0.5, 0.4)), Status::Note, diff --git a/src/units/tests/mod.rs b/src/units/tests/mod.rs index 5b0e45e..f4c4b93 100644 --- a/src/units/tests/mod.rs +++ b/src/units/tests/mod.rs @@ -20,7 +20,7 @@ use crate::{ inventory::Input, options::CheckArgs, schema::{Report, Status, Strength}, - tests::{Mock, Project, answer, args, function, run}, + tests::{Mock, Project, answer, args, function, long_function, run}, token_budget::TokenBudget, }; use anyhow::Result; diff --git a/src/units/wording/mod.rs b/src/units/wording/mod.rs index 2c86c7a..67357cd 100644 --- a/src/units/wording/mod.rs +++ b/src/units/wording/mod.rs @@ -4,8 +4,8 @@ use super::{ Block, Detail, GroupInfo, outcome::{ Answers, Outcome, RESOURCE_CHECKS, benefit, choice, comment_concern_kind, comment_signals, - disagreement, document_split, noul, origin_outcome, repeated, section_signals, - settled_checks, value_signals, + disagreement, document_split, noul, origin_outcome, particular_inputs, repeated, + section_signals, settled_checks, value_signals, }, }; use crate::catalog; diff --git a/src/units/wording/test_rules.rs b/src/units/wording/test_rules.rs index c3b9f14..9b22453 100644 --- a/src/units/wording/test_rules.rs +++ b/src/units/wording/test_rules.rs @@ -67,11 +67,7 @@ pub(in crate::units) fn law_wording( "Check that the comment and the law say the same thing", ); } - let fixed = matches!( - answers.get("fixed"), - Some(Answer::Noul { noul, .. }) - if crate::policy::probability_at_least(*noul, crate::policy::LOCATION_PROBABILITY) - ); + let fixed = particular_inputs(answers.get("fixed").copied()).is_some(); let adds = match choice(answers.get("relation").copied()) { _ if fixed => ": the law checks particular inputs where the comment speaks of any", Some(("property", _)) => ": it claims a property the law does not state", From 76fa8b273d1e0d263d6bf5df58dd3c47f03f8d40 Mon Sep 17 00:00:00 2001 From: Tauan BF <11513929+tauanbinato@users.noreply.github.com> Date: Sun, 27 Sep 2026 11:52:12 -0300 Subject: [PATCH 2/3] Measure the three finding changes and drop their baseline entries Function splits of 20 lines or fewer, copies of up to twelve lines between test cases of different files, and splits the recheck raised from an undecided first answer (now asked their file's kind) were the three considers the 0.22.0 self-check got wrong. Every changed review and consider on the corpus was labeled: considers 57% to 59% right on tuned projects, 47% to 51% held out, 24% to 28% on blind Bend 2 ones. The baseline keeps only test_map's visit. --- CHANGELOG.md | 6 ++++++ jevgate-baseline.json | 27 --------------------------- src/units/outcome/maintainability.rs | 13 ++++++++----- 3 files changed, 14 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7035b6e..bd97df1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ Notable changes to JevGate. Versions follow [Semantic Versioning](https://semver ## [Unreleased] +Three changes to maintainability findings, each from the findings JevGate's own release check got wrong and measured on the corpus with every changed review and consider labeled by hand (a debatable one counting as not right): considers went from 57% to 59% right on the projects used for tuning, from 47% to 51% on the held-out ones and from 24% to 28% on 23 Bend 2 projects never used for tuning; no review changed but one wrong file-organization review. The self-check's baseline keeps one finding, down from four. + +- Function simplification: splitting a function of 20 lines or fewer is at most a note. 12 of 39 such considers were right on the tuned projects and 5 of 50 on the blind Bend 2 ones: helpers that read in one look, a dispatch over a token's cases, proofs. Function-simplification considers went from 68% to 73% right. Nothing is asked again. +- Shared logic: copies of up to twelve lines between test cases in different files are notes, like short copies inside test cases: tests of separate modules or rules repeat the same setup because the code they test is parallel, and a helper shared across test files would couple them. 30 of 87 such considers were right on the tuned projects and 2 of 24 on the held-out ones; shared-logic considers went from 53% to 57% right there and from 43% to 53% on the held-out projects. Nothing is asked again. +- File organization: a split the recheck raised from an undecided first answer is asked what kind of file it is, as an undecided one is, and a kind that serves one feature clears it. The first pass and the recheck disagreeing was a weak sign: 4 of the 18 findings it cleared were right. About $0.01 on the corpus. + ## [0.22.0] - 2026-09-27 Bend 2 ([bendlang/bend](https://github.com/bendlang/bend) 2.0.x) is a supported language, with a rule for its laws. Every review and consider on Bend code was labeled by hand from the code, a debatable one counting as not right. On the Bend repository and 40 community projects used for tuning, 58% of reviews and 43% of considers were right (62% and 44% for the default rules, without the opt-in security and documentation groups). On 23 community projects never used for tuning, 53% of reviews were right before one change made from their labels (a split of a Bend 2 file is at most a consider) and 70% with it (74% for the default rules), and about 27% of considers, from a sample of 150 of 427; hardcoded values were the weakest rule there (26% right), and law findings were right 15 times in 23. Every request to the other languages' code is unchanged on the 117 corpus projects. diff --git a/jevgate-baseline.json b/jevgate-baseline.json index 9315b80..2154c30 100644 --- a/jevgate-baseline.json +++ b/jevgate-baseline.json @@ -10,33 +10,6 @@ "strength": "consider", "message": "`visit` likely mixes separate jobs; splitting it may make it easier to understand (0.85).", "reason": "wrong" - }, - { - "fingerprint": "4e8f1ec95e2bfba8dc17ef300ba3a88fae0a1322781dec1759520aca633a5844", - "rule": "maintainability/function-simplification", - "path": "src/units/outcome/mod.rs", - "line": 346, - "strength": "consider", - "message": "`law_recheck` likely mixes separate jobs; splitting it may make it easier to understand (0.82).", - "reason": "wrong" - }, - { - "fingerprint": "30bba567a24271430ef643e3e8e4c7c87d4ecc59a9fa81906f4666e7f580b8b0", - "rule": "maintainability/file-organization", - "path": "src/units/plan/file.rs", - "line": 294, - "strength": "consider", - "message": "Some members of this file could move to a separate module (0.81). G2 (`laravel_config`, `plan_module`) would be most useful as its own module.", - "reason": "wrong" - }, - { - "fingerprint": "a100d2c1b75c85bfed70d361281c8ba3dde35f409ede013f7ae6fec3286a20f5", - "rule": "maintainability/shared-logic", - "path": "src/units/tests/comments.rs", - "line": 40, - "strength": "consider", - "message": "`each_comment_is_asked_about_with_the_code_it_is_about` (src/units/tests/comments.rs:40) and `claims_that_quantify_under_a_comment_are_asked_with_their_reading_and_defs` (src/units/tests/laws.rs:34) repeat the same steps across test cases (0.86). Differences: `comments_project`\u2192`laws_project`, `comments`\u2192`laws`.", - "reason": "wrong" } ] } diff --git a/src/units/outcome/maintainability.rs b/src/units/outcome/maintainability.rs index 7c9e8bb..85f418c 100644 --- a/src/units/outcome/maintainability.rs +++ b/src/units/outcome/maintainability.rs @@ -6,8 +6,9 @@ const SHORT_FUNCTION_LINES: usize = 20; /// The stronger of splitting and (for deeply nested functions only) /// flattening. Splitting a function of 20 lines or fewer is at most a note: -/// of 10 such considers labeled by hand, 1 was right, while the split of a -/// function over 30 lines was right in 142 of 190. +/// labeled by hand, 12 of 39 such considers were right on the projects used +/// for tuning and 5 of 50 on 23 Bend 2 projects never used for it, most of +/// them helpers that read in one look or dispatches over a token's cases. pub(in crate::units) fn function_outcome( split: Option<&Answer>, flatten: Option<&Answer>, @@ -75,7 +76,9 @@ pub(in crate::units) fn values_outcome<'a>( /// kinds that serve one feature reaching the threshold clear an undecided /// split or a finding, and the kinds that serve several reaching it raise an /// undecided split to a consider. The kind is asked of a finding only when -/// the recheck raised it from an undecided first answer. +/// the recheck raised it from an undecided first answer: of the 18 such +/// findings the kind cleared on the corpus, 4 were right, and one was the +/// proposal to split JevGate's own planner of one unit per rule. pub(in crate::units) fn organization_outcome( split: Option<&Answer>, kind: Option<&Answer>, @@ -147,8 +150,8 @@ const MIRRORED_CASE_LINES: usize = 12; /// Copies of up to twelve lines between test cases in different files are /// notes too: tests of separate modules or rules repeat the same setup /// because the code they test is parallel, and a helper shared across test -/// files would couple them. Of 57 such considers labeled by hand, 11 were -/// right; on the held-out projects 2 of 24. +/// files would couple them. Of 87 such considers labeled by hand on the +/// projects used for tuning, 30 were right; on the held-out projects 2 of 24. pub(in crate::units) fn shared_outcome( required: Option<&Answer>, same: Option<&Answer>, From 98e1b76b2cd9e1ebd8331deb6dfbe56fe4bd6790 Mon Sep 17 00:00:00 2001 From: Tauan BF <11513929+tauanbinato@users.noreply.github.com> Date: Sun, 27 Sep 2026 11:58:53 -0300 Subject: [PATCH 3/3] Split the test-case walk into class context and case, and empty the baseline visit decided in one match both which classes hold test methods and which nodes are test cases. class_context now says what a class makes of its methods, and case what a node is to the walk (a case, a function that is not one, or something to look inside); visit only walks. With it the self-check reports no review or consider, so the last baseline entry is gone. --- CHANGELOG.md | 2 +- jevgate-baseline.json | 12 +-- src/analysis/test_map.rs | 180 ++++++++++++++++++++++----------------- 3 files changed, 102 insertions(+), 92 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd97df1..2447c2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ Notable changes to JevGate. Versions follow [Semantic Versioning](https://semver ## [Unreleased] -Three changes to maintainability findings, each from the findings JevGate's own release check got wrong and measured on the corpus with every changed review and consider labeled by hand (a debatable one counting as not right): considers went from 57% to 59% right on the projects used for tuning, from 47% to 51% on the held-out ones and from 24% to 28% on 23 Bend 2 projects never used for tuning; no review changed but one wrong file-organization review. The self-check's baseline keeps one finding, down from four. +Three changes to maintainability findings, each from the findings JevGate's own release check got wrong and measured on the corpus with every changed review and consider labeled by hand (a debatable one counting as not right): considers went from 57% to 59% right on the projects used for tuning, from 47% to 51% on the held-out ones and from 24% to 28% on 23 Bend 2 projects never used for tuning; no review changed but one wrong file-organization review. The self-check's baseline is empty, down from four accepted findings. - Function simplification: splitting a function of 20 lines or fewer is at most a note. 12 of 39 such considers were right on the tuned projects and 5 of 50 on the blind Bend 2 ones: helpers that read in one look, a dispatch over a token's cases, proofs. Function-simplification considers went from 68% to 73% right. Nothing is asked again. - Shared logic: copies of up to twelve lines between test cases in different files are notes, like short copies inside test cases: tests of separate modules or rules repeat the same setup because the code they test is parallel, and a helper shared across test files would couple them. 30 of 87 such considers were right on the tuned projects and 2 of 24 on the held-out ones; shared-logic considers went from 53% to 57% right there and from 43% to 53% on the held-out projects. Nothing is asked again. diff --git a/jevgate-baseline.json b/jevgate-baseline.json index 2154c30..54dc997 100644 --- a/jevgate-baseline.json +++ b/jevgate-baseline.json @@ -1,15 +1,5 @@ { "version": 1, "created_at": 1790514454, - "findings": [ - { - "fingerprint": "9a142118d2e3410e9f6c85cfb48ffb6f9ca83a6a7667b7a97bf02223aeddc1ca", - "rule": "maintainability/function-simplification", - "path": "src/analysis/test_map.rs", - "line": 208, - "strength": "consider", - "message": "`visit` likely mixes separate jobs; splitting it may make it easier to understand (0.85).", - "reason": "wrong" - } - ] + "findings": [] } diff --git a/src/analysis/test_map.rs b/src/analysis/test_map.rs index ba3bea1..1ab031e 100644 --- a/src/analysis/test_map.rs +++ b/src/analysis/test_map.rs @@ -212,118 +212,138 @@ fn visit( in_test_class: bool, found: &mut Vec, ) { + if let Some(test_class) = class_context(node, source, pytest) { + visit_children(node, source, pytest, test_class, found); + return; + } + match case(node, source, in_test_class) { + Visit::Case { + node, + start, + name, + ruby, + } => { + push(node, start, name, source, found); + if ruby { + ruby_context(node, source, found); + } + } + Visit::Leaf => {} + Visit::Inside => visit_children(node, source, pytest, in_test_class, found), + } +} + +/// Whether a class's methods are tests, for a class that decides it: a +/// JUnit 3 `TestCase` subclass, a PHPUnit test class, a Ruby class such as +/// `class OrderTest < Minitest::Test`, and any Python class. +fn class_context(node: Node<'_>, source: &str, pytest: bool) -> Option { + match node.kind() { + "class_declaration" + if crate::test_locations::junit3_class(node, source) + || crate::analysis::php::test_class(node, source) => + { + Some(true) + } + "class_definition" => Some(crate::test_locations::python_test_class( + node, source, pytest, + )), + "class" if crate::test_locations::ruby_test_class(node, source) => Some(true), + _ => None, + } +} + +/// What a node is to the walk for test cases. +enum Visit<'a> { + /// A test case: the node, where it starts (a Rust test at its first + /// attribute), its name, and whether it is a Ruby example, which reads + /// the setup of its groups. + Case { + node: Node<'a>, + start: usize, + name: String, + ruby: bool, + }, + /// A function or method that is not a test: nothing inside it is one. + Leaf, + /// Anything else: its children may hold test cases. + Inside, +} + +fn case<'a>(node: Node<'a>, source: &str, in_test_class: bool) -> Visit<'a> { + let found = |node: Node<'a>, start, name| Visit::Case { + node, + start, + name, + ruby: false, + }; match node.kind() { "function_item" => { let marked = crate::test_locations::preceding_attributes(node, source) .iter() .any(|attribute| crate::test_locations::attribute_marks_test(attribute)); if marked { - push( - node, - attribute_start(node), - name(node, source), - source, - found, - ); + found(node, attribute_start(node), name(node, source)) + } else { + Visit::Leaf } - return; } "function_declaration" if crate::test_locations::go_test_function(node, source) => { - push(node, node.start_byte(), name(node, source), source, found); - return; - } - // PHP: a test method of a PHPUnit test class. - "method_declaration" - if in_test_class && crate::analysis::php::test_method(node, source) => - { - push(node, node.start_byte(), name(node, source), source, found); - return; + found(node, node.start_byte(), name(node, source)) } + // PHP: a test method of a PHPUnit test class; C# and Java test + // methods; `test…` methods of a JUnit 3 class. "method_declaration" => { - if crate::test_locations::csharp_test_method(node, source) + let php = in_test_class && crate::analysis::php::test_method(node, source); + if php + || crate::test_locations::csharp_test_method(node, source) || crate::test_locations::java_test_method(node, source) || in_test_class && name(node, source).starts_with("test") { - push(node, node.start_byte(), name(node, source), source, found); + found(node, node.start_byte(), name(node, source)) + } else { + Visit::Leaf } - return; - } - // A JUnit 3 `TestCase` subclass: its `test…` methods are tests. - "class_declaration" if crate::test_locations::junit3_class(node, source) => { - visit_children(node, source, pytest, true, found); - return; } "function_definition" => { - let test = name(node, source).starts_with("test"); let outer = node .parent() .filter(|p| p.kind() == "decorated_definition") .unwrap_or(node); let top_level = outer.parent().is_some_and(|p| p.kind() == "module"); - if test && (top_level || in_test_class) { - push(outer, outer.start_byte(), name(node, source), source, found); - } - return; - } - "class_definition" => { - let test_class = crate::test_locations::python_test_class(node, source, pytest); - visit_children(node, source, pytest, test_class, found); - return; - } - // Ruby: `class OrderTest < Minitest::Test` holds `test_*` methods. - "class" if crate::test_locations::ruby_test_class(node, source) => { - visit_children(node, source, pytest, true, found); - return; - } - "method" => { - let name = name(node, source); - if in_test_class && name.starts_with("test_") { - push(node, node.start_byte(), name, source, found); - ruby_context(node, source, found); + if name(node, source).starts_with("test") && (top_level || in_test_class) { + found(outer, outer.start_byte(), name(node, source)) + } else { + Visit::Leaf } - return; } + "method" if in_test_class && name(node, source).starts_with("test_") => Visit::Case { + node, + start: node.start_byte(), + name: name(node, source), + ruby: true, + }, + "method" => Visit::Leaf, "call" if crate::test_locations::ruby_test_call(node, source) && ruby::CASES.contains(&ruby::method(node, source)) => { - push( + Visit::Case { node, - node.start_byte(), - ruby_case_name(node, source), - source, - found, - ); - ruby_context(node, source, found); - return; - } - "class_declaration" if crate::analysis::php::test_class(node, source) => { - visit_children(node, source, pytest, true, found); - return; - } - "expression_statement" => { - if let Some(case) = crate::analysis::php::pest_statement(node, source) - && !case.suite - { - push(node, node.start_byte(), case.title, source, found); - return; - } - } - "call_expression" => { - if let Some(case) = javascript_case(node, source) { - push( - statement(node), - statement(node).start_byte(), - case, - source, - found, - ); - return; + start: node.start_byte(), + name: ruby_case_name(node, source), + ruby: true, } } - _ => {} + "expression_statement" => match crate::analysis::php::pest_statement(node, source) { + Some(case) if !case.suite => found(node, node.start_byte(), case.title), + _ => Visit::Inside, + }, + "call_expression" => match javascript_case(node, source) { + Some(title) => found(statement(node), statement(node).start_byte(), title), + None => Visit::Inside, + }, + _ => Visit::Inside, } - visit_children(node, source, pytest, in_test_class, found); } fn visit_children(