diff --git a/src/court.rs b/src/court.rs index f7f857d..3ac1c33 100644 --- a/src/court.rs +++ b/src/court.rs @@ -190,6 +190,8 @@ pub fn policy_key(meaning: &MeaningPolicy, visibility: &VisibilityPolicy) -> Str push_list(&mut canon, &meaning.block_on); canon.push_str(";review_on="); push_list(&mut canon, &meaning.review_on); + canon.push_str(";rename_min="); + canon.push_str(&meaning.rename_min_score.to_string()); canon.push_str(";private_paths="); push_list(&mut canon, &visibility.private_paths); canon.push_str(";private_branches="); diff --git a/src/engine/mod.rs b/src/engine/mod.rs index ac7bce8..98810db 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -14,6 +14,7 @@ pub mod language; /// Structural difference engine for code snapshots. pub struct Engine { languages: Vec, + rename_min_score: u8, } impl Engine { @@ -22,14 +23,29 @@ impl Engine { pub fn new() -> anyhow::Result { Ok(Engine { languages: registry(), + rename_min_score: 100, }) } + /// Smallest rename similarity (0-100) that still pairs a removal with + /// an addition as a rename. Values above 100 clamp down to 100. + pub fn with_rename_min_score(mut self, min_score: u8) -> Self { + self.rename_min_score = min_score.min(100); + self + } + /// The grammar configuration for `path`, if Oot can diff that language. fn config_for(&self, path: &str) -> Option<&LangConfig> { self.languages.iter().find(|c| c.supports(path)) } + /// Pairing gate: similar enough to count as a rename under this + /// engine's threshold. A zero floor still needs some similarity. + fn is_rename(&self, candidate: &str, original: &str) -> bool { + let score = rename_score(candidate, original); + score > 0 && score >= self.rename_min_score + } + /// Compare two snapshots and report Meaning disputes: functions that /// changed, were added, or were removed between base and head. pub fn diff_snapshots(&self, base: &Snapshot, head: &Snapshot) -> anyhow::Result> { @@ -98,7 +114,7 @@ impl Engine { let mut leftover_removed: Vec<&str> = Vec::new(); for (old_name, old_def) in &pending_removed { let found = pending_added.iter().enumerate().find(|(i, (_, new_def))| { - !consumed[*i] && new_def.signature == old_def.signature + !consumed[*i] && self.is_rename(&new_def.signature, &old_def.signature) }); if let Some((i, (new_name, new_def))) = found { consumed[i] = true; @@ -402,6 +418,7 @@ impl Engine { &added_ours, &removed_theirs, &added_theirs, + self.rename_min_score, ); let mut claimed_theirs: Vec<(&str, usize)> = Vec::new(); let mut claimed_base: HashSet<&str> = HashSet::new(); @@ -453,7 +470,7 @@ impl Engine { let mut leftover_removed: Vec = Vec::new(); for (old_name, old_def) in &pending_removed { let found = pending_added.iter().enumerate().find(|(i, (_, new_def))| { - !consumed[*i] && new_def.signature == old_def.signature + !consumed[*i] && self.is_rename(&new_def.signature, &old_def.signature) }); if let Some((i, (new_name, new_def))) = found { consumed[i] = true; @@ -587,9 +604,7 @@ fn without_defs(src: &str, fns: &FunctionMap) -> String { for def in fns.values().flatten() { let start = def.start_byte.min(bytes.len()); let end = def.end_byte.min(bytes.len()).max(start); - for slot in marked.iter_mut().take(end).skip(start) { - *slot = true; - } + marked[start..end].fill(true); } let kept: Vec = bytes .iter() @@ -635,16 +650,7 @@ fn align_defs(base: &[FnDef], head: &[FnDef]) -> (Vec<(usize, usize)>, Vec bool { - let mut rest: Vec<&str> = b.iter().map(|d| d.src.as_str()).collect(); - for d in a { - match rest.iter().position(|s| *s == d.src) { - Some(i) => { - rest.swap_remove(i); - } - None => return false, - } - } - true + strip_common(a, b).is_empty() } /// The defs of `a` that have no identical counterpart in `b`. @@ -652,21 +658,123 @@ fn strip_common<'a>(a: &'a [FnDef], b: &[FnDef]) -> Vec<&'a FnDef> { let mut rest: Vec<&str> = b.iter().map(|d| d.src.as_str()).collect(); let mut leftover = Vec::new(); for d in a { - match rest.iter().position(|s| *s == d.src) { - Some(i) => { - rest.swap_remove(i); - } - None => leftover.push(d), + if let Some(i) = rest.iter().position(|s| *s == d.src) { + rest.swap_remove(i); + } else { + leftover.push(d); } } leftover } -/// Rename compatibility between a removed def's signature and an added -/// def's signature: `Some` on exact equality today. A future similarity -/// metric widens this to graded scores without touching the pairing logic. -fn rename_score(candidate: &str, original: &str) -> Option<()> { - (candidate == original).then_some(()) +/// How alike two function texts are: 100 = same ignoring spaces, +/// 80 = same after forgetting param names, else 0. Bodies must match +/// word for word, even where the body repeats a renamed param. +fn rename_score(candidate: &str, original: &str) -> u8 { + if squash_ws(candidate) == squash_ws(original) { + 100 + } else if strip_param_names(candidate) == strip_param_names(original) { + 80 + } else { + 0 + } +} + +/// Same text with param names erased, plus whether any names existed. +/// A typed piece (`name: Type`, `name = 1`) keeps its type part, a bare +/// name (`a`) goes away fully. Empty stays empty. +fn strip_param_names(sig: &str) -> (String, bool) { + let squashed = squash_ws(sig); + let Some(open) = squashed.find('(') else { + return (squashed, false); + }; + // Find the `)` that closes the first `(`. + let mut depth = 0; + let mut close = None; + for (i, c) in squashed[open..].char_indices() { + if c == '(' { + depth += 1; + } else if c == ')' { + depth -= 1; + } + if depth == 0 { + close = Some(open + i); + break; + } + } + let Some(close) = close else { + return (squashed, false); + }; + let params = &squashed[open + 1..close]; + let mut out = String::with_capacity(squashed.len()); + out.push_str(&squashed[..=open]); + let mut had_names = false; + let mut segment = String::new(); + let mut seg_depth = 0; + for c in params.chars() { + match c { + '(' => { + seg_depth += 1; + segment.push(c); + } + ')' => { + seg_depth -= 1; + segment.push(c); + } + ',' if seg_depth == 0 => { + let (s, h) = strip_segment(&segment); + out.push_str(&s); + out.push(','); + had_names |= h; + segment.clear(); + } + _ => segment.push(c), + } + } + let (s, h) = strip_segment(&segment); + out.push_str(&s); + had_names |= h; + out.push_str(&squashed[close..]); + (out, had_names) +} + +/// One param with its name erased. A name at the end counts as +/// followed by `,` since the real `,` or `)` was cut off. Without a +/// type or default every word is a name, so all of them go. +fn strip_segment(segment: &str) -> (String, bool) { + let typed = segment.contains(':') || segment.contains('='); + let mut out = String::with_capacity(segment.len()); + let mut had_names = false; + let mut chars = segment.chars().peekable(); + // A `'` directly before a name opens a lifetime (`&'a str`), + // which is a name like any param. A quote elsewhere (a `'x'` + // default) leaves the value alone. + let mut lifetime = false; + while let Some(c) = chars.next() { + if c.is_alphanumeric() || c == '_' { + let mut ident = String::from(c); + while chars + .peek() + .is_some_and(|n| n.is_alphanumeric() || *n == '_') + { + ident.push(chars.next().unwrap_or_default()); + } + let follows = chars.peek().copied().unwrap_or(','); + if !typed || follows == ':' || follows == '=' || lifetime { + had_names = true; + } else { + out.push_str(&ident); + } + lifetime = false; + } else { + lifetime = c == '\'' + && chars + .peek() + .is_some_and(|n| n.is_alphanumeric() || *n == '_'); + out.push(c); + } + } + (out, had_names) } /// One side's rename evidence: a base def identified by `(name, index @@ -689,20 +797,23 @@ struct DivergentRename { theirs_row: usize, } -/// Greedily pair each removed def with the first unconsumed added def whose -/// signature matches exactly under a different name. Entries are consumed -/// once on both sides, so duplicates pair one-to-one. +/// Greedily pair each removed def with the first unconsumed added def +/// similar enough to count as a rename under a different name. Entries +/// are consumed once on both sides, so duplicates pair one-to-one. fn pair_side_renames( removed: &[(String, usize, FnDef)], added: &[(String, FnDef)], + min_score: u8, ) -> Vec { let mut used = vec![false; added.len()]; let mut pairs = Vec::new(); for (old_name, old_idx, old_def) in removed { let found = added.iter().enumerate().find(|(i, (new_name, new_def))| { - !used[*i] - && new_name != old_name - && rename_score(&new_def.signature, &old_def.signature).is_some() + if used[*i] || new_name == old_name { + return false; + } + let score = rename_score(&new_def.signature, &old_def.signature); + score > 0 && score >= min_score }); if let Some((i, (new_name, new_def))) = found { used[i] = true; @@ -726,9 +837,10 @@ fn find_divergent_renames( added_ours: &[(String, FnDef)], removed_theirs: &[(String, usize, FnDef)], added_theirs: &[(String, FnDef)], + min_score: u8, ) -> Vec { - let ours_pairs = pair_side_renames(removed_ours, added_ours); - let theirs_pairs = pair_side_renames(removed_theirs, added_theirs); + let ours_pairs = pair_side_renames(removed_ours, added_ours, min_score); + let theirs_pairs = pair_side_renames(removed_theirs, added_theirs, min_score); let mut divergent = Vec::new(); for tp in &theirs_pairs { if let Some(op) = ours_pairs.iter().find(|op| { @@ -793,11 +905,8 @@ fn file_function_summary(tree: Option<&Tree>, source: &str, config: &LangConfig) let count: usize = fns.values().map(Vec::len).sum(); let preview: Vec = names.iter().take(3).map(|s| s.to_string()).collect(); let noun = if count == 1 { "function" } else { "functions" }; - if names.len() > 3 { - format!("{} {}: {}, …", count, noun, preview.join(", ")) - } else { - format!("{} {}: {}", count, noun, preview.join(", ")) - } + let extra = if names.len() > 3 { ", …" } else { "" }; + format!("{} {}: {}{}", count, noun, preview.join(", "), extra) } /// Extract tracked functions as `name -> [(source text, 1-based row, rename @@ -949,6 +1058,48 @@ fn insert( mod tests { use super::*; + #[test] + fn test_param_rename_pairs_only_under_loose_threshold() { + let mut base = Snapshot::default(); + base.files.insert( + "src/lib.rs".into(), + "pub fn alpha(a: i32) -> i32 { 42 }\n".into(), + ); + let mut head = Snapshot::default(); + head.files.insert( + "src/lib.rs".into(), + "pub fn beta(b: i32) -> i32 { 42 }\n".into(), + ); + + let strict = Engine::new().unwrap(); + let details: Vec = strict + .diff_snapshots(&base, &head) + .unwrap() + .iter() + .map(|d| d.detail.clone()) + .collect(); + assert!( + details.iter().any(|d| d.contains("added function `beta`")) + && details + .iter() + .any(|d| d.contains("removed function `alpha`")), + "strict default must not pair a param rename, got {details:?}" + ); + + let loose = Engine::new().unwrap().with_rename_min_score(80); + let details: Vec = loose + .diff_snapshots(&base, &head) + .unwrap() + .iter() + .map(|d| d.detail.clone()) + .collect(); + assert_eq!( + details, + vec!["renamed function `alpha` to `beta`"], + "threshold 80 must pair it as one rename" + ); + } + #[test] fn test_engine_diff_functions() { let eng = Engine::new().unwrap(); @@ -1452,16 +1603,42 @@ mod tests { } #[test] - fn test_rename_score_is_exact_only() { - assert!(rename_score("fn () {}", "fn () {}").is_some()); - assert!(rename_score("fn () {}", "fn (x) {}").is_none()); + fn test_rename_score_tiers() { + // Real signatures carry the name blanked out, so only the + // body and params decide the score. + assert_eq!(rename_score("fn \0() {}", "fn \0() {}"), 100); + assert_eq!(rename_score("fn \0 () {}", "fn \0() {}"), 100); + assert_eq!( + rename_score("fn \0(a: i32) { x }", "fn \0(b: i32) { x }"), + 80 + ); + assert_eq!(rename_score("def \0(name):", "def \0(user):"), 80); + assert_eq!(rename_score("def \0(a, b=1):", "def \0(c, b=1):"), 80); + assert_eq!( + rename_score("fn \0(a: i32) { x }", "fn \0(b: String) { x }"), + 0 + ); + assert_eq!(rename_score("fn \0() { x }", "fn \0(x) { x }"), 0); + assert_eq!(rename_score("fn \0(a) { x }", "fn \0(a) { y }"), 0); + assert_eq!( + rename_score("function \0({a, b}) { x }", "function \0({c, d}) { x }"), + 80 + ); + assert_eq!( + rename_score("fn \0(a: Foo) { x }", "fn \0(c: Foo) { x }"), + 80 + ); + assert_eq!( + rename_score("fn \0(x: &'a str) { y }", "fn \0(x: &'b str) { y }"), + 80 + ); } #[test] fn test_pair_side_consumes_each_added_once() { let removed = vec![rm("f", 0, "sigA"), rm("f", 1, "sigA")]; let added = vec![ad("x", 3, "sigA")]; - let pairs = pair_side_renames(&removed, &added); + let pairs = pair_side_renames(&removed, &added, 100); assert_eq!(pairs.len(), 1, "one added def cannot serve two removals"); assert_eq!(pairs[0].old_idx, 0); assert_eq!(pairs[0].new_name, "x"); @@ -1471,19 +1648,30 @@ mod tests { fn test_pair_side_matches_duplicates_one_to_one() { let removed = vec![rm("f", 0, "sigA"), rm("f", 1, "sigB")]; let added = vec![ad("y", 5, "sigB"), ad("x", 3, "sigA")]; - let pairs = pair_side_renames(&removed, &added); + let pairs = pair_side_renames(&removed, &added, 100); assert_eq!(pairs.len(), 2); assert!(pairs.iter().any(|p| p.old_idx == 0 && p.new_name == "x")); assert!(pairs.iter().any(|p| p.old_idx == 1 && p.new_name == "y")); } + #[test] + fn test_pair_side_gates_param_rename_on_threshold() { + let removed = vec![rm("f", 0, "fn \0(a: i32) { x }")]; + let added = vec![ad("g", 3, "fn \0(b: i32) { x }")]; + assert!( + pair_side_renames(&removed, &added, 100).is_empty(), + "param rename stays unpaired at the strict default" + ); + assert_eq!(pair_side_renames(&removed, &added, 80).len(), 1); + } + #[test] fn test_pair_side_rejects_equal_names() { // Same name on both sides is no rename; it is handled by the // regular changed/gone alignment. let removed = vec![rm("f", 0, "sigA")]; let added = vec![ad("f", 3, "sigA")]; - assert!(pair_side_renames(&removed, &added).is_empty()); + assert!(pair_side_renames(&removed, &added, 100).is_empty()); } #[test] @@ -1499,12 +1687,14 @@ mod tests { &added_ours, &removed_theirs, &added_theirs_convergent, + 100, ) .is_empty()); // Theirs deleted without renaming: no join. assert!( - find_divergent_renames(&removed_ours, &added_ours, &removed_theirs, &[],).is_empty() + find_divergent_renames(&removed_ours, &added_ours, &removed_theirs, &[], 100,) + .is_empty() ); // Divergent: ours f -> g, theirs f -> k. @@ -1515,6 +1705,7 @@ mod tests { &added_ours, &removed_theirs, &added_theirs_divergent, + 100, ), vec![DivergentRename { base_name: "f".into(), diff --git a/src/main.rs b/src/main.rs index 5d35ade..6f6ac87 100644 --- a/src/main.rs +++ b/src/main.rs @@ -203,7 +203,7 @@ fn main() -> anyhow::Result { Some(v) => VisibilityPolicy::load(std::path::Path::new(&v))?, None => VisibilityPolicy::default(), }; - let eng = Engine::new()?; + let eng = Engine::new()?.with_rename_min_score(meaning_policy.rename_min_score); // Store-backed adjudication: `--change ` engages only // when an Oot store opens here and no other adjudication mode was diff --git a/src/policy.rs b/src/policy.rs index b018627..1829838 100644 --- a/src/policy.rs +++ b/src/policy.rs @@ -15,6 +15,14 @@ pub struct MeaningPolicy { /// Severity names (lowercase, e.g. `"review"`, `"high"`) that require human review. #[serde(default = "default_review_on")] pub review_on: Vec, + /// Minimum rename similarity (0-100) that pairs a removal with an + /// addition as a rename. 100 is squash-equal, 80 allows param renames. + #[serde(default = "default_rename_min_score")] + pub rename_min_score: u8, +} + +fn default_rename_min_score() -> u8 { + 100 } fn default_block_on() -> Vec { @@ -30,6 +38,7 @@ impl Default for MeaningPolicy { MeaningPolicy { block_on: default_block_on(), review_on: default_review_on(), + rename_min_score: default_rename_min_score(), } } } @@ -39,6 +48,12 @@ impl MeaningPolicy { pub fn load(path: &Path) -> anyhow::Result { let text = std::fs::read_to_string(path)?; let p: MeaningPolicy = toml::from_str(&text)?; + if p.rename_min_score > 100 { + anyhow::bail!( + "invalid rename_min_score: '{}' (expected 0-100)", + p.rename_min_score + ); + } Ok(p) }