diff --git a/crates/weave-core/src/merge.rs b/crates/weave-core/src/merge.rs index b971f12f..fbceac08 100644 --- a/crates/weave-core/src/merge.rs +++ b/crates/weave-core/src/merge.rs @@ -1399,7 +1399,6 @@ fn merge_interstitials( let base_content = base_map.get(key).copied().unwrap_or(""); let ours_content = ours_map.get(key).copied().unwrap_or(""); let theirs_content = theirs_map.get(key).copied().unwrap_or(""); - // If all same, no merge needed if ours_content == theirs_content { merged.insert(key.to_string(), ours_content.to_string()); @@ -1630,12 +1629,14 @@ fn parse_single_line_specifiers(trimmed: &str) -> Vec { if trimmed.starts_with("import ") { if let Some(brace_start) = trimmed.find('{') { if let Some(brace_end) = trimmed.find('}') { - let inner = &trimmed[brace_start + 1..brace_end]; - return inner - .split(',') - .map(|s| s.trim().to_string()) - .filter(|s| !s.is_empty()) - .collect(); + if brace_start < brace_end { + let inner = &trimmed[brace_start + 1..brace_end]; + return inner + .split(',') + .map(|s| s.trim().to_string()) + .filter(|s| !s.is_empty()) + .collect(); + } } } } @@ -1765,8 +1766,8 @@ fn merge_imports_commutatively(base: &str, ours: &str, theirs: &str) -> String { .collect(); // Build import groups from ours (import lines only) - let mut groups: Vec> = Vec::new(); - let mut current_group: Vec<&str> = Vec::new(); + let mut groups: Vec> = Vec::new(); + let mut current_group: Vec = Vec::new(); for line in ours.lines() { if line.trim().is_empty() { @@ -1778,7 +1779,7 @@ fn merge_imports_commutatively(base: &str, ours: &str, theirs: &str) -> String { if theirs_deleted.contains(line) { continue; } - current_group.push(line); + current_group.push(line.to_string()); } } if !current_group.is_empty() { @@ -1792,19 +1793,34 @@ fn merge_imports_commutatively(base: &str, ours: &str, theirs: &str) -> String { } else { groups.len() - 1 }; + let mut matching_line = None; for (i, group) in groups.iter().enumerate() { - if group + if let Some(pos) = group .iter() - .any(|l| is_import_line(l) && import_source_prefix(l) == prefix) + .position(|l| is_import_line(l) && import_source_prefix(l) == prefix) { best_group = i; + matching_line = Some(pos); break; } } + if let Some(pos) = matching_line { + let base_specs: Vec = base_imports + .iter() + .filter(|i| i.source == prefix) + .flat_map(|i| i.specifiers.iter().cloned()) + .collect(); + if let Some(merged) = + merge_same_source_named_import_line(&groups[best_group][pos], add, &base_specs) + { + groups[best_group][pos] = merged; + continue; + } + } if best_group < groups.len() { - groups[best_group].push(add); + groups[best_group].push(add.to_string()); } else { - groups.push(vec![add]); + groups.push(vec![add.to_string()]); } } @@ -1813,12 +1829,12 @@ fn merge_imports_commutatively(base: &str, ours: &str, theirs: &str) -> String { group.sort_unstable(); } - let mut import_lines: Vec<&str> = Vec::new(); + let mut import_lines: Vec = Vec::new(); for (i, group) in groups.iter().enumerate() { if i > 0 { - import_lines.push(""); + import_lines.push(String::new()); } - import_lines.extend(group); + import_lines.extend(group.iter().cloned()); } let import_block = import_lines.join("\n"); @@ -2137,35 +2153,16 @@ fn merge_imports_with_multiline( // Non-import lines: use diffy 3-way merge so adds/deletes/edits on // either side are handled correctly (fixes #60). let extract_non_imports = |content: &str| -> String { - content - .lines() - .filter(|l| !l.trim().is_empty() && !is_import_line(l)) - .filter(|l| { - let t = l.trim(); - // Exclude multi-line import continuation lines: - // - specifier lines ending with comma (but not assignments) - // - bare closing parens/braces - // - closing lines like `} from "./foo"` or `) from "bar"` - if (t.ends_with(',') && !t.contains('=')) || t == ")" || t == "}" { - return false; - } - // Closing line of JS/TS multi-line import: `} from "..."` or `} from '...'` - if t.starts_with('}') && t.contains("from ") { - return false; - } - // Closing line of Python multi-line import: `) ` at end or just `)` - if t.starts_with(')') { - return false; - } - true - }) + let (_, non_import_lines) = parse_import_statements(content); + non_import_lines + .into_iter() + .filter(|l| !l.trim().is_empty()) .collect::>() .join("\n") }; let base_ni = extract_non_imports(_base_raw); let ours_ni = extract_non_imports(ours_raw); let theirs_ni = extract_non_imports(_theirs_raw); - if !base_ni.is_empty() || !ours_ni.is_empty() || !theirs_ni.is_empty() { let merged_ni = match diffy::merge(&base_ni, &ours_ni, &theirs_ni) { Ok(m) => m, @@ -2186,6 +2183,79 @@ fn merge_imports_with_multiline( result } +fn named_import_parts(line: &str) -> Option<(&str, &str)> { + let trimmed_start = line.len() - line.trim_start().len(); + let trimmed = &line[trimmed_start..]; + + if let Some(open) = trimmed.find('{') { + let close = trimmed.rfind('}')?; + let from = trimmed.find(" from ")?; + if open < close && close < from { + return Some(( + &line[..trimmed_start + open + 1], + &line[trimmed_start + close..], + )); + } + } + + if trimmed.starts_with("from ") { + if let Some(pos) = trimmed.find(" import ") { + let prefix_end = trimmed_start + pos + " import ".len(); + return Some((&line[..prefix_end], "")); + } + } + + None +} + +fn merge_same_source_named_import_line( + ours: &str, + theirs: &str, + base_specs: &[String], +) -> Option { + let ours_specs = parse_single_line_specifiers(ours.trim()); + let theirs_specs = parse_single_line_specifiers(theirs.trim()); + if ours_specs.is_empty() || theirs_specs.is_empty() { + return None; + } + + let (ours_prefix, ours_suffix) = named_import_parts(ours)?; + let (theirs_prefix, theirs_suffix) = named_import_parts(theirs)?; + if ours_prefix.trim() != theirs_prefix.trim() || ours_suffix.trim() != theirs_suffix.trim() { + return None; + } + + let base_set: HashSet<&str> = base_specs.iter().map(String::as_str).collect(); + let theirs_set: HashSet<&str> = theirs_specs.iter().map(String::as_str).collect(); + let theirs_removed: HashSet<&str> = base_set.difference(&theirs_set).copied().collect(); + + let mut final_specs: Vec = ours_specs + .into_iter() + .filter(|s| !theirs_removed.contains(s.as_str())) + .collect(); + + for spec in theirs_specs { + if !base_set.contains(spec.as_str()) && !final_specs.contains(&spec) { + final_specs.push(spec); + } + } + + if final_specs.is_empty() { + return None; + } + + if ours_suffix.is_empty() { + Some(format!("{}{}", ours_prefix, final_specs.join(", "))) + } else { + Some(format!( + "{} {} {}", + ours_prefix, + final_specs.join(", "), + ours_suffix.trim_start() + )) + } +} + /// Extract the source/module prefix from an import line for group matching. /// e.g. "from collections import OrderedDict" -> "collections" /// "import React from 'react'" -> "react" @@ -4079,6 +4149,14 @@ export function agentB() { assert!(!is_import_line("function foo() {}")); } + #[test] + fn test_named_import_parsing_rejects_reversed_braces() { + let malformed = "import } foo { from 'bar';"; + assert!(parse_single_line_specifiers(malformed).is_empty()); + assert!(named_import_parts(malformed).is_none()); + assert!(named_import_parts("import { foo } from 'bar';").is_some()); + } + #[test] fn test_commutative_import_merge_both_add_different() { // The key scenario: both branches add different imports diff --git a/crates/weave-core/tests/integration.rs b/crates/weave-core/tests/integration.rs index bef81442..3df96703 100644 --- a/crates/weave-core/tests/integration.rs +++ b/crates/weave-core/tests/integration.rs @@ -2860,10 +2860,10 @@ fn json_both_add_keys_in_different_sections() { } // --------------------------------------------------------------------------- -// 7. TS: both modify the same import line differently (conflict expected) +// 7. TS: both add different specifiers to the same single-line import (auto-merge) // --------------------------------------------------------------------------- #[test] -fn ts_both_modify_same_import_conflict() { +fn ts_both_modify_same_import_merges_named_specifiers() { let base = r#"import { foo } from './utils'; export function run() { @@ -2883,14 +2883,120 @@ export function run() { } "#; let result = entity_merge(base, ours, theirs, "run.ts"); - // Both modify the same import line — the commutative import merger should - // handle this (both add to same import source). The key check is that - // neither bar nor baz is silently dropped. - let has_bar = result.content.contains("bar"); - let has_baz = result.content.contains("baz"); - assert!( - has_bar && has_baz, - "Both bar and baz must be present (merged or conflicted).\nContent:\n{}", + assert!( + result.is_clean(), + "same-source import specifier additions should auto-resolve.\nConflicts: {:?}\nContent:\n{}", + result.conflicts, + result.content + ); + assert_eq!( + result.content.matches("from './utils'").count(), + 1, + "same-source import must not be duplicated.\nContent:\n{}", + result.content + ); + assert!( + result + .content + .contains("import { foo, bar, baz } from './utils';"), + "Both bar and baz must be merged into one import.\nContent:\n{}", + result.content + ); +} + +#[test] +fn ts_multiline_imports_without_trailing_commas_do_not_leak_specifiers() { + fn has_orphan_import_member(content: &str) -> bool { + let mut depth = 0_i32; + for line in content.lines() { + let t = line.trim(); + let indented = line.starts_with(' ') || line.starts_with('\t'); + let specifier_like = indented + && (t.ends_with(',') || t.starts_with("type ")) + && !t.contains('=') + && !t.contains('('); + if depth == 0 && specifier_like { + return true; + } + depth += line.matches('{').count() as i32; + depth -= line.matches('}').count() as i32; + if depth < 0 { + return true; + } + } + false + } + + let base = r#"import { + type A +} from "./file1" +import { + type B +} from "./file2" +import { + type C +} from "./file3" + +export function main() { return 1; } +"#; + let ours = r#"import { + type A, + type A2 +} from "./file1" +import { + type B +} from "./file2" +import { + type C +} from "./file3" + +export function main() { return 1; } +"#; + let theirs = r#"import { + type A +} from "./file1" +import { + type B +} from "./file2" +import { + type C, + type C2 +} from "./file3" + +export function main() { return 1; } +"#; + + let result = entity_merge(base, ours, theirs, "imports.ts"); + assert!( + result.is_clean(), + "multi-line import additions should auto-resolve.\nConflicts: {:?}\nContent:\n{}", + result.conflicts, + result.content + ); + assert!( + !result.content.contains("<<<<<<<"), + "import merge must not introduce conflict markers.\nContent:\n{}", + result.content + ); + assert!( + !has_orphan_import_member(&result.content), + "import specifier/closer leaked outside an import block.\nContent:\n{}", + result.content + ); + for source in ["./file1", "./file2", "./file3"] { + assert_eq!( + result + .content + .matches(&format!("from \"{source}\"")) + .count(), + 1, + "source {source} should appear exactly once.\nContent:\n{}", + result.content + ); + } + assert!( + result.content.contains("type A2,") && result.content.contains("type C2,"), + "both sides' added specifiers must be present.\nContent:\n{}", result.content ); }