mergiraf/mergiraf
mergiraf/mergiraf
9
213
Fork 45
Code Issues 51 Pull requests 7 Releases 19 Activity Actions

fix(TreeBuilder): resolve not-really-conflicts (left and right agree), take 2 #204

Merged
ada4a merged 4 commits from ada4a/mergiraf:170-not_really_a_conflict-v2 into main 2025-02-15 11:24:58 +01:00
Conversation 2 Commits 4 Files changed 2 +100 -24
2 changed files with 100 additions and 24 deletions
Download patch file Download diff file Expand all files Collapse all files

45
src/merge_3dm.rs
View file

@ -513,4 +513,49 @@ mod tests {
let _pretty_printed = merged_tree.pretty_print(&class_mapping, &DisplaySettings::default());
// assert_eq!(pretty_printed, "{}"); // TODO there should be a delete/modify conflict here!
}
fn rust_matchers() -> (TreeMatcher<'static>, TreeMatcher<'static>) {
let lang_profile = LangProfile::detect_from_filename("test.rs").unwrap();
let primary_matcher = TreeMatcher {
min_height: 0,
sim_threshold: 0.5,
max_recovery_size: 100,
use_rted: true,
lang_profile,
};
let auxiliary_matcher = TreeMatcher {
min_height: 1,
sim_threshold: 0.5,
max_recovery_size: 100,
use_rted: false,
lang_profile,
};
(primary_matcher, auxiliary_matcher)
}
#[test]
fn insert_insert_not_really_a_conflict() {
let ctx = ctx();
// both `left` and `right` add the `'s` to `&self`, so this would-be-conflict should be
// resolved during the construction of the tree. NB: The `<'s>` is added just so that
// `left` and `right` are not completely identical (which would've made the resolution trivial)
let base = ctx.parse_rust("fn foo(&self) {}");
let left = ctx.parse_rust("fn foo(&'s self) {}");
let right = ctx.parse_rust("fn foo<'s>(&'s self) {}");
let (primary_matcher, auxiliary_matcher) = rust_matchers();
let (merged_tree, class_mapping) = three_way_merge(
&base,
&left,
&right,
None,
&primary_matcher,
&auxiliary_matcher,
None,
);
let pretty_printed = merged_tree.pretty_print(&class_mapping, &DisplaySettings::default());
assert_eq!(pretty_printed, "fn foo<'s>(&'s self) {}");
}
}

79
src/tree_builder.rs
View file

@ -227,40 +227,71 @@ impl<'a, 'b> TreeBuilder<'a, 'b> {
cursor = children_map.get(&predecessor);
}
2 => {
let conflict = self.build_conflict(
let Ok((next_cursor, conflict)) = self.build_conflict(
predecessor,
children_map,
base_children_map,
&mut seen_nodes,
visiting_state,
);
match conflict {
Err(_) => {
let line_based =
self.commutative_or_line_based_local_fallback(node, visiting_state);
return line_based;
}
Ok((next_cursor, conflict)) => {
if let PCSNode::Node { node: leader, .. } = node {
if let Some(commutative_parent) = self
.lang_profile
.get_commutative_parent(leader.grammar_name())
{
let solved_conflict = self.resolve_commutative_conflict(
conflict,
commutative_parent,
visiting_state,
)?;
children.extend(solved_conflict);
} else {
children.push(conflict);
}
) else {
let line_based =
self.commutative_or_line_based_local_fallback(node, visiting_state);
return line_based;
};
let MergedTree::Conflict {
ada4a commented 2025-02-12 18:04:19 +01:00
Copy link

Since we're destructuring conflict here anyway, we could just pass the sides to resolve_commutative_conflict directly. And since destructing conflict is all the latter does besides calling commutatively_merge_list, I think we could just remove resolve_commutative_conflict and call commutatively_merge_list directly

Since we're destructuring `conflict` here anyway, we could just pass the sides to `resolve_commutative_conflict` directly. And since destructing `conflict` is all the latter does besides calling `commutatively_merge_list`, I think we could just remove `resolve_commutative_conflict` and call `commutatively_merge_list` directly
wetneb commented 2025-02-15 11:14:07 +01:00
Copy link

It sounds good to me, intuitively

It sounds good to me, intuitively
ada4a commented 2025-02-15 11:21:54 +01:00
Copy link

Great, I'll open a PR for that then

Great, I'll open a PR for that then
ref left,
ref right,
..
} = conflict
else {
unreachable!("`build_conflict` should return a conflict")
};
if left.len() == right.len()
&& std::iter::zip(left, right).all(|(l, r)| l.isomorphic_to(r))
{
// both sides of the "conflict" consist of nodes that are pairwise isomorphic.
// This means that both sides actually agree in how they change the base.
// Therefore, we resolve the conflict trivially by picking the left side (WLOG)
let not_really_a_conflict: Vec<_> = left
.iter()
.copied()
.map(|l| {
MergedTree::new_exact(
self.class_mapping
.map_to_leader(RevNode::new(Revision::Left, l)),
RevisionNESet::singleton(Revision::Left).with(Revision::Right),
self.class_mapping,
)
})
.collect();
children.extend(not_really_a_conflict);
} else {
// reason: the following two `if`s should really be `&&`-ed,
// but https://github.com/rust-lang/rust/issues/53667
// until then, collapsing one but not the other looks misleading
#[allow(clippy::collapsible_if)]
if let PCSNode::Node { node: leader, .. } = node {
if let Some(commutative_parent) = self
.lang_profile
.get_commutative_parent(leader.grammar_name())
{
let solved_conflict = self.resolve_commutative_conflict(
conflict,
commutative_parent,
visiting_state,
)?;
children.extend(solved_conflict);
} else {
children.push(conflict);
}
cursor = next_cursor;
} else {
children.push(conflict);
}
}
cursor = next_cursor;
}
_ => unreachable!("unexpected conflict size: more than two diverging sides!"),
}