diff --git a/README.md b/README.md index 96efdbf..c121552 100644 --- a/README.md +++ b/README.md @@ -74,7 +74,7 @@ Working seed — the engine runs, the docket renders, and git + Jujutsu ingestio - [x] Git adapter with 3-way adjudication - [x] Jujutsu adapter with 3-way adjudication (`--source jj`, revsets accepted) -**Known limitation:** functions that share a bare name within one file (e.g., a `render` method on two classes, or same-named Go methods on two types) are tracked by first occurrence only; the docket flags them as ambiguous rather than tracking each definition separately. +Same-named definitions in one file (e.g. a `render` method on two classes) are each tracked separately: diffing matches identical bodies first, then pairs the rest, so only the definition that actually changed is reported. ## License diff --git a/TODO.md b/TODO.md index de29c07..cae05a3 100644 --- a/TODO.md +++ b/TODO.md @@ -18,3 +18,44 @@ Text conversion happens only in the structural engine at parse time store unconverted bytes. Pinned by `test_cli_distinct_binaries_are_not_collapsed`. +## ~~Same-named functions tracked by first occurrence only~~ RESOLVED 2026-08-22 + +`FunctionMap` was `HashMap`, so a second same-key definition in +one file was dropped with an ambiguity note and the first occurrence's +changes shadowed the rest. The map now holds every definition +(`HashMap>`) and diffing aligns each name group by +content: exact body match first, positional pairing of leftovers, remainder +becomes added/removed. Qualified keys (`(Type).name`) stay rejected — they +are unstable under impl-block refactors +(`test_engine_impl_move_is_not_a_conflict`). Pinned by +`test_engine_duplicate_function_names_tracked_separately`, +`test_engine_go_same_name_methods_tracked_separately`, and +`test_engine_rust_impl_method_collision_tracked_separately`. + +## Rename/rename divergence is swallowed + +Base has `f`; ours renames `f -> g`, theirs renames `f -> k`. Both sides +deleted `f`, so it reads as convergent; `g` and `k` read as plain additions. +Merged result silently holds both names. Pre-existing (the old code hit its +convergent-clean branch the same way), but def-level tracking makes a fix +reachable: detect that the removed base defs survive under different names +per side, then emit High. Trigger: first real rename/rename dispute or the +hosted intent-scoring work, whichever comes first. + +## Positional pairing can swap row attribution on count asymmetry + +When a new same-named def lands *above* a modified one +(base 1x `f`, head 2x `f`), leftover pairing reports "changed" at the new +def's row and "added" at the modified one's row. Counts and severity are +right; only line numbers are swapped. Right fix: similarity-based leftover +pairing (edit distance or tree-sitter diff hash) instead of document order. +Trigger: when a docket consumer starts using dispute rows for navigation. + +## Fallback conflict message says "modified" for pure deletion divergence + +When both branches delete different copies of a same-named def, the High +dispute reuses the "both branches modified function `X` differently" +wording (severity is correct, only the verb is off). Right fix: a dedicated +"deleted differently" message variant. Trigger: first docket consumer that +pattern-matches dispute details. + diff --git a/src/engine/language.rs b/src/engine/language.rs index 843aafe..e04e003 100644 --- a/src/engine/language.rs +++ b/src/engine/language.rs @@ -7,11 +7,14 @@ //! on the declarator itself. That relationship is captured by //! [`LangConfig::wrapped_functions`]. //! -//! Function map keys are always bare names. Receiver- or impl-qualified keys -//! (`(*T).name`, `(A).name`) were tried and rejected: they make keys unstable -//! under refactoring, so moving a function between impl blocks fabricates -//! High-severity 3-way conflicts (and false Blocked verdicts). Same-named -//! functions are reported as ambiguous instead — see `Engine::diff_snapshots`. +//! Function map keys are always bare names, but every same-key definition +//! is kept and diffing aligns each group by content (exact source match +//! first, then positional pairing of leftovers — see `align_defs`). +//! Receiver- or impl-qualified keys (`(*T).name`, `(A).name`) were tried and +//! rejected: they make keys unstable under refactoring, so moving a function +//! between impl blocks fabricates High-severity 3-way conflicts (and false +//! Blocked verdicts). Content-first alignment keeps identity stable under +//! those moves while still tracking each definition separately. use tree_sitter::{Language, Node}; diff --git a/src/engine/mod.rs b/src/engine/mod.rs index 0926a29..1c61e2a 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -50,89 +50,76 @@ impl Engine { match (base_src, head_src) { (Some(b), Some(h)) => { - let (base_fns, mut dupes) = extract_functions( + let base_fns = extract_functions( parse_source(&mut parser, &config.language, &b).as_ref(), &b, config, ); - let (head_fns, head_dupes) = extract_functions( + let head_fns = extract_functions( parse_source(&mut parser, &config.language, &h).as_ref(), &h, config, ); - dupes.extend(head_dupes); - dupes.sort(); - dupes.dedup(); - for name in &dupes { - disputes.push(meaning( - &mut n, - path, - 0, - format!( - "function `{name}` is defined multiple times in this file; tracked only by first occurrence" - ), - Severity::Review, - )); - } - - let mut added: Vec<(&String, usize)> = Vec::new(); - let mut names: Vec<&String> = head_fns.keys().collect(); + let mut names: Vec<&String> = base_fns.keys().chain(head_fns.keys()).collect(); names.sort(); + names.dedup(); + + let empty: Vec = Vec::new(); + let mut pending_added: Vec<(&str, FnDef)> = Vec::new(); + let mut pending_removed: Vec<(&str, FnDef)> = Vec::new(); + for name in names { - let (h_src, h_row, _) = &head_fns[name]; - match base_fns.get(name) { - Some((b_src, _, _)) if b_src != h_src => { - disputes.push(meaning( - &mut n, - path, - *h_row, - format!("both sides changed `{}`", name), - Severity::Review, - )); - } - None => { - added.push((name, *h_row)); - } - _ => {} + let b_list = base_fns.get(name).unwrap_or(&empty); + let h_list = head_fns.get(name).unwrap_or(&empty); + let (changed, gone, fresh) = align_defs(b_list, h_list); + for (_, hi) in &changed { + disputes.push(meaning( + &mut n, + path, + h_list[*hi].row, + format!("both sides changed `{}`", name), + Severity::Review, + )); + } + for bi in gone { + pending_removed.push((name, b_list[bi].clone())); + } + for hi in fresh { + pending_added.push((name, h_list[hi].clone())); } } - let mut removed: Vec<&String> = base_fns - .keys() - .filter(|name| !head_fns.contains_key(*name)) - .collect(); - removed.sort(); - - // Pair removals with additions of identical source text: - // that is a rename, not two separate changes. - let mut consumed = vec![false; added.len()]; - let mut leftover_removed: Vec<&String> = Vec::new(); - for old in &removed { - let old_sig = &base_fns[*old].2; - let found = added - .iter() - .enumerate() - .find(|(i, (new, _))| !consumed[*i] && &head_fns[*new].2 == old_sig); - if let Some((i, (new, _))) = found { + + // Pair removals with additions of identical blanked-name + // source text: that is a rename, not two separate changes. + pending_added.sort_by(|a, b| a.0.cmp(b.0)); + pending_removed.sort_by(|a, b| a.0.cmp(b.0)); + let mut consumed = vec![false; pending_added.len()]; + 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 + }); + if let Some((i, (new_name, new_def))) = found { consumed[i] = true; disputes.push(meaning( &mut n, path, - head_fns[*new].1, - format!("renamed function `{}` to `{}`", old, new), + new_def.row, + format!("renamed function `{}` to `{}`", old_name, new_name), Severity::Review, )); } else { - leftover_removed.push(old); + leftover_removed.push(old_name); } } - for (i, (new, row)) in added.iter().enumerate() { + for (i, (new_name, new_def)) in pending_added.iter().enumerate() { if !consumed[i] { disputes.push(meaning( &mut n, path, - *row, - format!("added function `{}`", new), + new_def.row, + format!("added function `{}`", new_name), Severity::Review, )); } @@ -208,37 +195,21 @@ impl Engine { match (b_file, o_file, t_file) { // File exists in all three (Some(b_src), Some(o_src), Some(t_src)) => { - let (b_fns, mut dupes) = extract_functions( + let b_fns = extract_functions( parse_source(&mut parser, &config.language, &b_src).as_ref(), &b_src, config, ); - let (o_fns, o_dupes) = extract_functions( + let o_fns = extract_functions( parse_source(&mut parser, &config.language, &o_src).as_ref(), &o_src, config, ); - let (t_fns, t_dupes) = extract_functions( + let t_fns = extract_functions( parse_source(&mut parser, &config.language, &t_src).as_ref(), &t_src, config, ); - dupes.extend(o_dupes); - dupes.extend(t_dupes); - dupes.sort(); - dupes.dedup(); - - for name in &dupes { - disputes.push(meaning( - &mut n, - path, - 0, - format!( - "function `{name}` is defined multiple times in this file; tracked only by first occurrence" - ), - Severity::Review, - )); - } let mut all_fn_names: Vec<&String> = b_fns .keys() @@ -248,124 +219,153 @@ impl Engine { all_fn_names.sort(); all_fn_names.dedup(); - let mut pending_added: Vec<(String, usize)> = Vec::new(); - let mut pending_removed: Vec = Vec::new(); + let mut pending_added: Vec<(String, FnDef)> = Vec::new(); + let mut pending_removed: Vec<(String, FnDef)> = Vec::new(); + let empty: Vec = Vec::new(); for name in all_fn_names { - let b_fn = b_fns.get(name); - let o_fn = o_fns.get(name); - let t_fn = t_fns.get(name); - - let b_body = b_fn.map(|(s, _, _)| s.as_str()); - let o_body = o_fn.map(|(s, _, _)| s.as_str()); - let t_body = t_fn.map(|(s, _, _)| s.as_str()); - let row = t_fn - .map(|(_, r, _)| *r) - .or_else(|| o_fn.map(|(_, r, _)| *r)) - .unwrap_or(0); + let b_list = b_fns.get(name).unwrap_or(&empty); + let o_list = o_fns.get(name).unwrap_or(&empty); + let t_list = t_fns.get(name).unwrap_or(&empty); + + let (o_changed, o_gone, o_fresh) = align_defs(b_list, o_list); + let (t_changed, t_gone, t_fresh) = align_defs(b_list, t_list); + + let ours_touched = + !(o_changed.is_empty() && o_gone.is_empty() && o_fresh.is_empty()); + let theirs_touched = + !(t_changed.is_empty() && t_gone.is_empty() && t_fresh.is_empty()); + + // If both match base, unchanged + if !ours_touched && !theirs_touched { + continue; + } - // If both matches base, unchanged - if o_body == b_body && t_body == b_body { + // Neither side mutated a definition that existed in + // base. Additions stand alone unless both branches + // added conflicting same-named copies. + if !o_list.is_empty() + && !t_list.is_empty() + && o_changed.is_empty() + && o_gone.is_empty() + && t_changed.is_empty() + && t_gone.is_empty() + { + if contained_in(o_list, t_list) || contained_in(t_list, o_list) { + continue; + } + let row = t_list + .first() + .or_else(|| o_list.first()) + .map(|d| d.row) + .unwrap_or(0); + disputes.push(meaning( + &mut n, + path, + row, + format!( + "3-way conflict: both branches modified function `{}` differently", + name + ), + Severity::High, + )); continue; } - // Case 1: Unilateral change by incoming (theirs) - if o_body == b_body && t_body != b_body { - match (b_body, t_body) { - (None, Some(_)) => { - pending_added.push((name.clone(), row)); - } - (Some(_), None) => { - pending_removed.push(name.clone()); - } - (Some(_), Some(_)) => { - disputes.push(meaning( - &mut n, - path, - row, - format!("incoming branch modified function `{}`", name), - Severity::Review, - )); - } - (None, None) => {} + // Case 1: Unilateral change by incoming (theirs); + // ours at most added new definitions. + if o_changed.is_empty() && o_gone.is_empty() { + for (_, ti) in &t_changed { + disputes.push(meaning( + &mut n, + path, + t_list[*ti].row, + format!("incoming branch modified function `{}`", name), + Severity::Review, + )); + } + for bi in &t_gone { + pending_removed.push((name.clone(), b_list[*bi].clone())); + } + for ti in &t_fresh { + pending_added.push((name.clone(), t_list[*ti].clone())); } + continue; } // Case 2: Unilateral change by target (ours) - no dispute for incoming merge - else if o_body != b_body && t_body == b_body { + if t_changed.is_empty() && t_gone.is_empty() { continue; } // Case 3: Both branches modified relative to base - else { - if o_body == t_body { - // Convergent clean change - continue; - } - // Divergent modifications -> 3-way semantic conflict - match (o_body, t_body) { - (Some(_), Some(_)) => { - disputes.push(meaning( - &mut n, - path, - row, - format!( - "3-way conflict: both branches modified function `{}` differently", - name - ), - Severity::High, - )); - } - (None, Some(_)) => { - disputes.push(meaning( - &mut n, - path, - row, - format!( - "3-way conflict: function `{}` modified in incoming branch but deleted in target", - name - ), - Severity::High, - )); - } - (Some(_), None) => { - disputes.push(meaning( - &mut n, - path, - row, - format!( - "3-way conflict: function `{}` deleted in incoming branch but modified in target", - name - ), - Severity::High, - )); - } - (None, None) => {} - } + if contained_in(o_list, t_list) && contained_in(t_list, o_list) { + // Convergent clean change + continue; + } + // When neither side deleted anything, identical defs + // converged and a leftover on one side alone is an + // additive duplication, not a conflict. + if o_gone.is_empty() + && t_gone.is_empty() + && (strip_common(o_list, t_list).is_empty() + || strip_common(t_list, o_list).is_empty()) + { + continue; } + // Divergent modifications -> 3-way semantic conflict + let row = t_list + .first() + .map(|d| d.row) + .or_else(|| o_list.first().map(|d| d.row)) + .unwrap_or(0); + let detail = if !t_changed.is_empty() && !o_changed.is_empty() { + format!( + "3-way conflict: both branches modified function `{}` differently", + name + ) + } else if !t_changed.is_empty() && !o_gone.is_empty() { + format!( + "3-way conflict: function `{}` modified in incoming branch but deleted in target", + name + ) + } else if !t_gone.is_empty() && !o_changed.is_empty() { + format!( + "3-way conflict: function `{}` deleted in incoming branch but modified in target", + name + ) + } else { + format!( + "3-way conflict: both branches modified function `{}` differently", + name + ) + }; + disputes.push(meaning(&mut n, path, row, detail, Severity::High)); } // Pair incoming removals with incoming additions of - // identical source text: a rename, not two changes. + // identical blanked-name source text: a rename, not two + // changes. pending_added.sort_by(|a, b| a.0.cmp(&b.0)); - pending_removed.sort(); + pending_removed.sort_by(|a, b| a.0.cmp(&b.0)); let mut consumed = vec![false; pending_added.len()]; let mut leftover_removed: Vec = Vec::new(); - for old in &pending_removed { - let old_sig = &b_fns[old].2; - let found = pending_added - .iter() - .enumerate() - .find(|(i, (new, _))| !consumed[*i] && &t_fns[new].2 == old_sig); - if let Some((i, (new, row))) = found { + 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 + }); + if let Some((i, (new_name, new_def))) = found { consumed[i] = true; disputes.push(meaning( &mut n, path, - *row, - format!("incoming branch renamed function `{}` to `{}`", old, new), + new_def.row, + format!( + "incoming branch renamed function `{}` to `{}`", + old_name, new_name + ), Severity::Review, )); } else { - leftover_removed.push(old.clone()); + leftover_removed.push(old_name.clone()); } } for (i, (new, row)) in pending_added.iter().enumerate() { @@ -373,7 +373,7 @@ impl Engine { disputes.push(meaning( &mut n, path, - *row, + row.row, format!("incoming branch added function `{}`", new), Severity::Low, )); @@ -436,9 +436,77 @@ impl Engine { } } -/// Tracked functions keyed by name: source text, 1-based row, and a rename +/// One extracted definition: source text, 1-based row, and a rename /// signature (the body with its own name blanked out). -type FunctionMap = HashMap; +#[derive(Debug, Clone)] +struct FnDef { + src: String, + row: usize, + signature: String, +} + +/// Tracked functions grouped by bare name. A name may hold several +/// definitions in one file (same-named methods on different receivers); +/// each keeps its own entry and diffing runs a matching pass per group +/// instead of collapsing to the first occurrence. +type FunctionMap = HashMap>; + +/// Align two def lists for one name: identical source text pairs first +/// (each def consumed once, silently — those are unchanged), then remaining +/// leftovers pair by relative order and count as changed. Returns matched +/// `(base_idx, head_idx)` pairs plus the base and head leftovers. +fn align_defs(base: &[FnDef], head: &[FnDef]) -> (Vec<(usize, usize)>, Vec, Vec) { + let mut used_base = vec![false; base.len()]; + let mut used_head = vec![false; head.len()]; + for (hi, h) in head.iter().enumerate() { + for (bi, b) in base.iter().enumerate() { + if !used_base[bi] && b.src == h.src { + used_base[bi] = true; + used_head[hi] = true; + break; + } + } + } + let rest_base: Vec = (0..base.len()).filter(|&i| !used_base[i]).collect(); + let rest_head: Vec = (0..head.len()).filter(|&i| !used_head[i]).collect(); + let pairs = rest_base.len().min(rest_head.len()); + let mut changed = Vec::new(); + for i in 0..pairs { + changed.push((rest_base[i], rest_head[i])); + } + let gone = rest_base[pairs..].to_vec(); + let fresh = rest_head[pairs..].to_vec(); + (changed, gone, fresh) +} + +/// Whether every def in `a` also appears in `b` (multiplicity-aware). +fn contained_in(a: &[FnDef], b: &[FnDef]) -> 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 +} + +/// The defs of `a` that have no identical counterpart in `b`. +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), + } + } + leftover +} fn parse_source(parser: &mut Parser, language: &Language, source: &str) -> Option { parser.set_language(language).ok()?; @@ -470,13 +538,13 @@ fn meaning(n: &mut i32, path: &str, row: usize, detail: String, severity: Severi /// up to three names, so a docket reader knows what arrived without opening /// the file. fn file_function_summary(tree: Option<&Tree>, source: &str, config: &LangConfig) -> String { - let (fns, _) = extract_functions(tree, source, config); + let fns = extract_functions(tree, source, config); let mut names: Vec<&String> = fns.keys().collect(); names.sort(); if names.is_empty() { return "no functions detected".to_string(); } - let count = names.len(); + 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 count > 3 { @@ -486,36 +554,24 @@ fn file_function_summary(tree: Option<&Tree>, source: &str, config: &LangConfig) } } -/// Extract tracked functions as `name -> (source text, 1-based row, rename -/// signature with the name blanked out)`, plus the -/// list of names defined more than once (only the first occurrence is kept). -fn extract_functions( - tree: Option<&Tree>, - source: &str, - config: &LangConfig, -) -> (FunctionMap, Vec) { +/// Extract tracked functions as `name -> [(source text, 1-based row, rename +/// signature with the name blanked out)]`, in document order. +fn extract_functions(tree: Option<&Tree>, source: &str, config: &LangConfig) -> FunctionMap { let mut map = HashMap::new(); - let mut duplicates = Vec::new(); if let Some(tree) = tree { - collect(tree.root_node(), source, &mut map, &mut duplicates, config); + collect(tree.root_node(), source, &mut map, config); } - (map, duplicates) + map } -fn collect( - node: Node, - source: &str, - map: &mut FunctionMap, - duplicates: &mut Vec, - config: &LangConfig, -) { +fn collect(node: Node, source: &str, map: &mut FunctionMap, config: &LangConfig) { let mut matched = false; for kind in config.function_kinds { if node.kind() == kind.node_kind { matched = true; if let Some(key) = config.function_key(kind, node, source) { let name_node = node.child_by_field_name("name"); - insert(key, node, name_node, source, map, duplicates); + insert(key, node, name_node, source, map); } } } @@ -537,7 +593,7 @@ fn collect( .utf8_text(source.as_bytes()) .unwrap_or("") .to_string(); - insert(key, body, None, source, map, duplicates); + insert(key, body, None, source, map); } } // Do not recurse into nodes that yielded a function: nested definitions @@ -547,48 +603,38 @@ fn collect( } let mut cursor = node.walk(); for child in node.children(&mut cursor) { - collect(child, source, map, duplicates, config); + collect(child, source, map, config); } } -/// Record a named function under `key`, with `body` as its source. First -/// occurrence wins; later same-key definitions are reported as duplicates. -fn insert( - key: String, - body: Node, - name_node: Option, - source: &str, - map: &mut FunctionMap, - duplicates: &mut Vec, -) { +/// Record a named function under `key`. Every definition is kept; diffing +/// aligns same-key groups by content (see [`align_defs`]). +fn insert(key: String, body: Node, name_node: Option, source: &str, map: &mut FunctionMap) { if key.is_empty() { return; } - match map.entry(key.clone()) { - std::collections::hash_map::Entry::Occupied(_) => duplicates.push(key), - std::collections::hash_map::Entry::Vacant(slot) => { - let src = body.utf8_text(source.as_bytes()).unwrap_or("").to_string(); - let row = body.start_position().row + 1; - // Signature: the body with its own name blanked, so two functions - // that differ only by what they are called compare equal and pair - // as a rename. - let signature = match name_node { - Some(n) - if n.start_byte() >= body.start_byte() && n.end_byte() <= body.end_byte() => - { - let rel_start = n.start_byte() - body.start_byte(); - let rel_end = n.end_byte() - body.start_byte(); - let mut sig = String::with_capacity(src.len()); - sig.push_str(&src[..rel_start]); - sig.push('\u{0}'); - sig.push_str(&src[rel_end..]); - sig - } - _ => src.clone(), - }; - slot.insert((src, row, signature)); + let src = body.utf8_text(source.as_bytes()).unwrap_or("").to_string(); + let row = body.start_position().row + 1; + // Signature: the body with its own name blanked, so two functions + // that differ only by what they are called compare equal and pair + // as a rename. + let signature = match name_node { + Some(n) if n.start_byte() >= body.start_byte() && n.end_byte() <= body.end_byte() => { + let rel_start = n.start_byte() - body.start_byte(); + let rel_end = n.end_byte() - body.start_byte(); + let mut sig = String::with_capacity(src.len()); + sig.push_str(&src[..rel_start]); + sig.push('\u{0}'); + sig.push_str(&src[rel_end..]); + sig } - } + _ => src.clone(), + }; + map.entry(key).or_default().push(FnDef { + src, + row, + signature, + }); } #[cfg(test)] @@ -818,7 +864,7 @@ mod tests { } #[test] - fn test_engine_go_method_collision_flagged_ambiguous() { + fn test_engine_go_same_name_methods_tracked_separately() { let eng = Engine::new().unwrap(); let mut base = Snapshot::default(); @@ -834,17 +880,13 @@ mod tests { ); let disputes = eng.diff_snapshots(&base, &head).unwrap(); - assert!( - disputes - .iter() - .any(|d| d.detail.contains("`hit`") && d.detail.contains("multiple times")), - "same-named methods must surface an ambiguity dispute, got {:?}", - disputes - ); + // Only A's `hit` changed; B's identical `hit` must not be dragged in. + assert_eq!(disputes.len(), 1); + assert_eq!(disputes[0].detail, "both sides changed `hit`"); } #[test] - fn test_engine_rust_impl_method_collision_flagged_ambiguous() { + fn test_engine_rust_impl_method_collision_tracked_separately() { let eng = Engine::new().unwrap(); let mut base = Snapshot::default(); @@ -861,11 +903,10 @@ mod tests { ); let disputes = eng.diff_snapshots(&base, &head).unwrap(); - assert!( - disputes - .iter() - .any(|d| d.detail.contains("`hit`") && d.detail.contains("multiple times")), - "same-named impl methods must surface an ambiguity dispute, got {:?}", + assert_eq!(disputes.len(), 1); + assert_eq!( + disputes[0].detail, "both sides changed `hit`", + "only the touched impl method must be reported, got {:?}", disputes ); } @@ -898,6 +939,154 @@ mod tests { ); } + #[test] + fn test_engine_3way_overload_add_is_not_a_conflict() { + // Regression: ours adds a same-named overload while theirs edits the + // original. Git merges this cleanly, so it must stay Review-level. + let eng = Engine::new().unwrap(); + + let snap = |s: &str| { + let mut x = Snapshot::default(); + x.files.insert("src/lib.rs".into(), s.into()); + x + }; + let disputes = eng + .diff_3way( + &snap("pub fn f() -> i32 { 1 }\n"), + &snap("pub fn f() -> i32 { 1 }\npub fn f() -> i32 { 99 }\n"), + &snap("pub fn f() -> i32 { 3 }\n"), + ) + .unwrap(); + + assert!( + !disputes.iter().any(|d| d.severity == Severity::High), + "additive overload must not fabricate a conflict, got {:?}", + disputes + ); + assert!(disputes + .iter() + .any(|d| d.detail == "incoming branch modified function `f`" + && d.severity == Severity::Review)); + } + + #[test] + fn test_engine_3way_add_add_divergent_conflicts_with_old_contract_message() { + let eng = Engine::new().unwrap(); + + let snap = |s: &str| { + let mut x = Snapshot::default(); + x.files.insert("src/lib.rs".into(), s.into()); + x + }; + let disputes = eng + .diff_3way( + &snap("pub fn a() {}\n"), + &snap("pub fn a() {}\npub fn f() -> i32 { 1 }\n"), + &snap("pub fn a() {}\npub fn f() -> i32 { 2 }\n"), + ) + .unwrap(); + + assert_eq!(disputes.len(), 1); + assert_eq!(disputes[0].severity, Severity::High); + assert_eq!( + disputes[0].detail, + "3-way conflict: both branches modified function `f` differently" + ); + } + + #[test] + fn test_engine_3way_convergent_same_name_addition_is_clean() { + let eng = Engine::new().unwrap(); + + let snap = |s: &str| { + let mut x = Snapshot::default(); + x.files.insert("src/lib.rs".into(), s.into()); + x + }; + let disputes = eng + .diff_3way( + &snap("pub fn a() {}\n"), + &snap("pub fn a() {}\npub fn f() -> i32 { 7 }\n"), + &snap("pub fn a() {}\npub fn f() -> i32 { 7 }\n"), + ) + .unwrap(); + + assert!(disputes.is_empty(), "got {:?}", disputes); + } + + #[test] + fn test_engine_3way_superset_addition_is_clean() { + // Regression: ours' addition appearing verbatim inside theirs' + // additions is a trivial union, not a conflict. + let eng = Engine::new().unwrap(); + + let snap = |s: &str| { + let mut x = Snapshot::default(); + x.files.insert("src/lib.rs".into(), s.into()); + x + }; + let disputes = eng + .diff_3way( + &snap("pub fn a() {}\n"), + &snap("pub fn a() {}\npub fn f() -> i32 { 5 }\n"), + &snap("pub fn a() {}\npub fn f() -> i32 { 5 }\npub fn f() -> i32 { 6 }\n"), + ) + .unwrap(); + + assert!( + !disputes.iter().any(|d| d.severity == Severity::High), + "superset additions must not fabricate a conflict, got {:?}", + disputes + ); + } + + #[test] + fn test_engine_3way_identical_mutation_plus_duplication_is_clean() { + // Regression: both branches converge on the same body and theirs + // additionally keeps a second copy. The merge is a trivial union. + let eng = Engine::new().unwrap(); + + let snap = |s: &str| { + let mut x = Snapshot::default(); + x.files.insert("src/lib.rs".into(), s.into()); + x + }; + let disputes = eng + .diff_3way( + &snap("pub fn f() -> i32 { 1 }\n"), + &snap("pub fn f() -> i32 { 42 }\n"), + &snap("pub fn f() -> i32 { 42 }\npub fn f() -> i32 { 42 }\n"), + ) + .unwrap(); + + assert!( + !disputes.iter().any(|d| d.severity == Severity::High), + "identical mutation plus duplication must not conflict, got {:?}", + disputes + ); + } + + #[test] + fn test_engine_3way_shared_deletion_with_converged_edit_is_clean() { + // Both sides delete `f` and converge on `g`: clean, as before. + let eng = Engine::new().unwrap(); + + let snap = |s: &str| { + let mut x = Snapshot::default(); + x.files.insert("src/lib.rs".into(), s.into()); + x + }; + let disputes = eng + .diff_3way( + &snap("pub fn f() -> i32 { 1 }\npub fn g() -> i32 { 2 }\n"), + &snap("pub fn g() -> i32 { 42 }\n"), + &snap("pub fn g() -> i32 { 42 }\n"), + ) + .unwrap(); + + assert!(disputes.is_empty(), "got {:?}", disputes); + } + #[test] fn test_engine_3way_mixed_languages() { let eng = Engine::new().unwrap(); diff --git a/tests/engine_test.rs b/tests/engine_test.rs index e0a3c36..c969d83 100644 --- a/tests/engine_test.rs +++ b/tests/engine_test.rs @@ -605,7 +605,7 @@ fn test_engine_dotless_filename_is_not_source() { } #[test] -fn test_engine_duplicate_function_names_flagged() { +fn test_engine_duplicate_function_names_tracked_separately() { let engine = Engine::new().expect("Failed to initialize engine"); let mut base = Snapshot::default(); @@ -646,12 +646,8 @@ func (b B) Name() string { let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); - // Regression: last-wins overwrite used to hide A's change entirely. - assert!( - disputes - .iter() - .any(|d| d.detail.contains("`Name`") && d.detail.contains("multiple times")), - "duplicate name must surface an ambiguity dispute, got {:?}", - disputes - ); + // Regression: last-wins overwrite used to hide A's change entirely; + // first-occurrence tracking then reduced it to an ambiguity note. + assert_eq!(disputes.len(), 1, "got {:?}", disputes); + assert_eq!(disputes[0].detail, "both sides changed `Name`"); }