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

chore: Better error types for TreeBuilder #563

Merged
wetneb merged 5 commits from 552-error-types into main 2025-10-03 10:40:52 +02:00
Conversation 7 Commits 5 Files changed 5 +94 -21
5 changed files with 94 additions and 21 deletions
Download patch file Download diff file Expand all files Collapse all files

25
Cargo.lock generated
View file

@ -544,6 +544,7 @@ dependencies = [
"rustc-hash 2.1.1",
"stderrlog",
"tempfile",
"thiserror 2.0.12",
"tree-edit-distance",
"tree-sitter",
"tree-sitter-c-sharp",
@ -661,7 +662,7 @@ dependencies = [
"itertools 0.10.5",
"num-traits",
"rustc-hash 1.1.0",
"thiserror",
"thiserror 1.0.69",
]
[[package]]
@ -958,7 +959,16 @@ version = "1.0.69"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "b6aaf5339b578ea85b50e080feb250a3e8ae8cfcdff9a461c9ec2904bc923f52"
dependencies = [
"thiserror-impl",
"thiserror-impl 1.0.69",
]
[[package]]
name = "thiserror"
version = "2.0.12"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "567b8a2dae586314f7be2a752ec7474332959c6460e02bde30d702a66d488708"
dependencies = [
"thiserror-impl 2.0.12",
]
[[package]]
@ -972,6 +982,17 @@ dependencies = [
"syn",
]
[[package]]
name = "thiserror-impl"
version = "2.0.12"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "7f7cf42b4507d8ea322120659672cf1b9dbb93f8f2d4ecfd6e51350ff5b17a1d"
dependencies = [
"proc-macro2",
"quote",
"syn",
]
[[package]]
name = "thread_local"
version = "1.1.9"

1
Cargo.toml
View file

@ -80,6 +80,7 @@ tree-sitter-make = "1.1.1"
tree-sitter-starlark = "1.3.0"
nonempty-collections = "1.0.1"
tempfile = {version = "3", optional = true}
thiserror = "2"
# for transitive dependencies that incorrectly specify minimal required versions of their dependencies
[target."cfg(any())".dependencies]

1
src/lib.rs
View file

@ -46,6 +46,7 @@ pub(crate) mod test_utils;
pub(crate) mod tree_builder;
pub(crate) mod tree_matcher;
pub mod util;
pub(crate) mod utils;
pub(crate) mod visualizer;
use core::fmt::Write;

68
src/tree_builder.rs
View file

@ -1,5 +1,6 @@
use std::collections::{HashMap, HashSet};
use std::fmt::Display;
use thiserror::Error;
use either::Either;
use itertools::Itertools;
@ -7,6 +8,7 @@ use log::{debug, trace};
use rustc_hash::FxHashSet;
use crate::merged_tree::Conflict;
use crate::utils::InternalError;
use crate::{
ast::AstNode,
changeset::ChangeSet,
@ -67,6 +69,27 @@ impl VisitingState<'_> {
}
}
#[derive(Error, Debug, PartialEq, Eq)]
pub enum TreeBuildingError<'a> {
// Errors that are expected to happen in certain cases.
#[error("node `{node}` encountered twice, which generates an infinite tree")]
NodeEncounteredTwice { node: Leader<'a> },
#[error("children not allowed to commute per their types")]
UncommutableChildren,
// Internal errors, which are a sign of a programming
// error and should never be allowed to happen, regardless of the input data.
// To avoid panicking in production, we still return an error for those.
#[error("more than two conflicting sides after node `{node}`")]
MoreThanTwoConflictingSides { node: PCSNode<'a> },
#[error("the virtual root needs to have a child, none found")]
NoVirtualRootChildFound,
#[error("impossible to do a line-based fallback merge for a virtual node")]
LineBasedFallbackOnVirtualNode,
#[error("impossible to build a subtree for a virtual left/right marker")]
BuildSubtreeForVirtualMarker,
}
type SuccessorsCursor<'a> = FxHashSet<(Revision, PCSNode<'a>)>;
impl<'a, 'b> TreeBuilder<'a, 'b> {
@ -86,7 +109,7 @@ impl<'a, 'b> TreeBuilder<'a, 'b> {
}
/// Build the merged tree
pub fn build_tree(&self) -> Result<MergedTree<'a>, String> {
pub fn build_tree(&self) -> Result<MergedTree<'a>, TreeBuildingError<'a>> {
let mut visiting_state = VisitingState {
// keep track of all nodes that have been deleted on one side and modified on the other
deleted_and_modified: HashSet::new(),
@ -143,11 +166,11 @@ impl<'a, 'b> TreeBuilder<'a, 'b> {
&'b self,
node: PCSNode<'a>,
visiting_state: &mut VisitingState<'a>,
) -> Result<MergedTree<'a>, String> {
) -> Result<MergedTree<'a>, TreeBuildingError<'a>> {
if let PCSNode::Node { node, .. } = node {
let visited = &mut visiting_state.visited_nodes;
if visited.contains(&node) {
return Err("node already visited".to_owned());
return Err(TreeBuildingError::NodeEncounteredTwice { node });
}
visited.insert(node);
}
@ -164,7 +187,7 @@ impl<'a, 'b> TreeBuilder<'a, 'b> {
&'b self,
node: PCSNode<'a>,
visiting_state: &mut VisitingState<'a>,
) -> Result<MergedTree<'a>, String> {
) -> Result<MergedTree<'a>, TreeBuildingError<'a>> {
// if the node has isomorphic subtrees in all revisions, that's very boring,
// so we just return a tree that matches that
if let PCSNode::Node {
@ -206,7 +229,9 @@ impl<'a, 'b> TreeBuilder<'a, 'b> {
loop {
match cursor.len() {
0 => {
// unexpected, this is a nasty conflict!
// This could be a double delete or a delete/modified conflict.
// Following the 3DM algorithm, we fall back on line-based merges in this case.
// See merge_3dm::tests::{delete_delete, commutative_conflict_delete_delete, commutative_conflict_delete_modified}.
return self.commutative_or_line_based_local_fallback(node, visiting_state);
}
1 => {
@ -275,7 +300,12 @@ impl<'a, 'b> TreeBuilder<'a, 'b> {
}
cursor = next_cursor;
}
_ => unreachable!("unexpected conflict size: more than two diverging sides!"),
_ => {
return Err(TreeBuildingError::MoreThanTwoConflictingSides {
node: predecessor,
})
.debug_panic();
}
}
}
@ -341,14 +371,16 @@ impl<'a, 'b> TreeBuilder<'a, 'b> {
}
match node {
PCSNode::VirtualRoot => children.into_iter().next().ok_or_else(|| {
"the virtual root must have exactly one child, none found".to_string()
}),
PCSNode::VirtualRoot => children
.into_iter()
.next()
.ok_or(TreeBuildingError::NoVirtualRootChildFound)
.debug_panic(),
PCSNode::LeftMarker => {
panic!("impossible to build a subtree for a virtual left marker")
Err(TreeBuildingError::BuildSubtreeForVirtualMarker).debug_panic()
}
PCSNode::RightMarker => {
panic!("impossible to build a subtree for a virtual right marker")
Err(TreeBuildingError::BuildSubtreeForVirtualMarker).debug_panic()
}
PCSNode::Node {
node: revnode,
@ -526,13 +558,11 @@ impl<'a, 'b> TreeBuilder<'a, 'b> {
&self,
node: PCSNode<'a>,
visiting_state: &mut VisitingState<'a>,
) -> Result<MergedTree<'a>, String> {
) -> Result<MergedTree<'a>, TreeBuildingError<'a>> {
let pad = visiting_state.indentation();
trace!("{pad}{node} commutative_or_line_based_local_fallback");
let PCSNode::Node { node, .. } = node else {
return Err(format!(
"impossible to do a line-based local fallback for a virtual PCS node {node}"
));
return Err(TreeBuildingError::LineBasedFallbackOnVirtualNode).debug_panic();
};
// If the root happens to be commutative, we can merge all children accordingly.
if let Some(commutative_parent) = node.commutative_parent_definition()
@ -602,7 +632,7 @@ impl<'a, 'b> TreeBuilder<'a, 'b> {
right: &[&'a AstNode<'a>],
commutative_parent: &CommutativeParent,
visiting_state: &mut VisitingState<'a>,
) -> Result<Vec<MergedTree<'a>>, String> {
) -> Result<Vec<MergedTree<'a>>, TreeBuildingError<'a>> {
let pad = visiting_state.indentation();
trace!("{pad}commutatively_merge_lists");
// TODO improve handling of comments? comments added by the right side should ideally be placed sensibly
@ -610,7 +640,7 @@ impl<'a, 'b> TreeBuilder<'a, 'b> {
// check that all the nodes involved are allowed to commute in this context
let raw_separator = commutative_parent
.child_separator(base, left, right)
.ok_or("The children are not allowed to commute")?;
.ok_or(TreeBuildingError::UncommutableChildren)?;
let trimmed_sep = raw_separator.trim();
let trimmed_left_delim = commutative_parent.left_delim.unwrap_or_default().trim();
let trimmed_right_delim = commutative_parent.right_delim.unwrap_or_default().trim();
@ -676,7 +706,7 @@ impl<'a, 'b> TreeBuilder<'a, 'b> {
)?;
Ok((revnode, subtree))
})
.collect::<Result<_, String>>()?;
.collect::<Result<_, TreeBuildingError<'a>>>()?;
let right_removed_and_not_modified: HashSet<_> = right_removed_content
.into_iter()
.filter(|(_, result_tree)| match result_tree {
@ -804,7 +834,7 @@ impl<'a, 'b> TreeBuilder<'a, 'b> {
leader: &Leader<'a>,
commutative_parent: &CommutativeParent,
visiting_state: &mut VisitingState<'a>,
) -> Result<Vec<MergedTree<'a>>, String> {
) -> Result<Vec<MergedTree<'a>>, TreeBuildingError<'a>> {
let children_base = self
.class_mapping
.children_at_revision(leader, Revision::Base)

20
src/utils.rs Normal file
View file

@ -0,0 +1,20 @@
use std::error::Error;
pub(crate) trait InternalError {
fn debug_panic(self) -> Self;
}
impl<V, E: Error> InternalError for Result<V, E> {
/// Panic if this result is an error and we are in debug mode.
/// This is useful for internal errors that are meant to be never reached,
/// but that we want to be able to gracefully recover from in release mode.
#[track_caller]
#[inline]
ada4a marked this conversation as resolved
ada4a commented 2025-10-01 14:07:39 +02:00
Copy link

I think this would benefit from #[track_caller], which would make it so that the reported panic location is at the caller of this function and not inside this function

I think this would benefit from `#[track_caller]`, which would make it so that the reported panic location is at the caller of this function and not inside this function
fn debug_panic(self) -> Self {
if cfg!(debug_assertions) {
Ok(self.unwrap())
} else {
self
}
}
}