Skip to content

Commit

Permalink
shortest path is probably more informative than random path for error…
Browse files Browse the repository at this point in the history 
… messages
  • Loading branch information
@Eh2406
Eh2406 committed Sep 19, 2023
1 parent 2284b7a commit 1051246
Show file tree
Hide file tree
Showing 2 changed files with 106 additions and 55 deletions.
5 changes: 2 additions & 3 deletions crates/resolver-tests/tests/resolve.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1586,9 +1586,8 @@ versions that meet the requirements `<=0.1.1` are: 0.1.1, 0.1.0
all possible versions conflict with previously selected packages.
previously selected package `F v0.1.2 (registry `https://example.com/`)`
... which satisfies dependency `F = \"^0.1.2\"` of package `D v1.0.0 (registry `https://example.com/`)`
... which satisfies dependency `D = \"*\"` of package `C v1.0.0 (registry `https://example.com/`)`
... which satisfies dependency `C = \"*\"` of package `A v1.0.0 (registry `https://example.com/`)`
... which satisfies dependency `F = \"^0.1.2\"` of package `E v1.0.0 (registry `https://example.com/`)`
... which satisfies dependency `E = \"*\"` of package `A v1.0.0 (registry `https://example.com/`)`
... which satisfies dependency `A = \"*\"` of package `root v1.0.0 (registry `https://example.com/`)`
failed to select a version for `F` which could resolve this conflict\
Expand Down
156 changes: 104 additions & 52 deletions src/cargo/util/graph.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use std::borrow::Borrow;
use std::collections::BTreeSet;
use std::collections::{BTreeMap, BTreeSet, VecDeque};
use std::fmt;

pub struct Graph<N: Clone, E: Clone> {
Expand Down Expand Up @@ -87,84 +87,107 @@ impl<N: Eq + Ord + Clone, E: Default + Clone> Graph<N, E> {
false
}

/// Resolves one of the paths from the given dependent package down to
/// a leaf.
/// Resolves one of the paths from the given dependent package down to a leaf.
///
/// The path return will be the shortest path, or more accurately one of the paths with the shortest length.
///
/// Each element contains a node along with an edge except the first one.
/// The representation would look like:
///
/// (Node0,) -> (Node1, Edge01) -> (Node2, Edge12)...
pub fn path_to_bottom<'a>(&'a self, mut pkg: &'a N) -> Vec<(&'a N, Option<&'a E>)> {
let mut result = vec![(pkg, None)];
while let Some(p) = self.nodes.get(pkg).and_then(|p| {
p.iter()
// Note that we can have "cycles" introduced through dev-dependency
// edges, so make sure we don't loop infinitely.
.find(|&(node, _)| result.iter().all(|p| p.0 != node))
.map(|(node, edge)| (node, Some(edge)))
}) {
result.push(p);
pkg = p.0;
}
#[cfg(debug_assertions)]
{
for x in result.windows(2) {
let [(n1, _), (n2, Some(e12))] = x else {
unreachable!()
};
assert!(std::ptr::eq(self.edge(n1, n2).unwrap(), *e12));
}
let last = result.last().unwrap().0;
// fixme: this may be wrong when there are cycles, but we dont have them in tests.
assert!(!self.nodes.contains_key(last));
}
result
pub fn path_to_bottom<'a>(&'a self, pkg: &'a N) -> Vec<(&'a N, Option<&'a E>)> {
self.path_to(pkg, |s, p| s.edges(p))
}

/// Resolves one of the paths from the given dependent package up to
/// the root.
/// Resolves one of the paths from the given dependent package up to the root.
///
/// The path return will be the shortest path, or more accurately one of the paths with the shortest length.
///
/// Each element contains a node along with an edge except the first one.
/// The representation would look like:
///
/// (Node0,) -> (Node1, Edge01) -> (Node2, Edge12)...
pub fn path_to_top<'a>(&'a self, mut pkg: &'a N) -> Vec<(&'a N, Option<&'a E>)> {
// Note that this implementation isn't the most robust per se, we'll
// likely have to tweak this over time. For now though it works for what
// it's used for!
let mut result = vec![(pkg, None)];
let first_pkg_depending_on = |pkg, res: &[(&N, Option<&E>)]| {
self.nodes
pub fn path_to_top<'a>(&'a self, pkg: &'a N) -> Vec<(&'a N, Option<&'a E>)> {
self.path_to(pkg, |s, pk| {
// Note that this implementation isn't the most robust per se, we'll
// likely have to tweak this over time. For now though it works for what
// it's used for!
s.nodes
.iter()
.filter(|(_, adjacent)| adjacent.contains_key(pkg))
// Note that we can have "cycles" introduced through dev-dependency
// edges, so make sure we don't loop infinitely.
.find(|&(node, _)| !res.iter().any(|p| p.0 == node))
.map(|(p, adjacent)| (p, adjacent.get(pkg)))
};
while let Some(p) = first_pkg_depending_on(pkg, &result) {
result.push(p);
pkg = p.0;
.filter_map(|(p, adjacent)| adjacent.get(pk).map(|e| (p, e)))
})
}
}

impl<'s, N: Eq + Ord + Clone + 's, E: Default + Clone + 's> Graph<N, E> {
fn path_to<'a, F, I>(&'s self, pkg: &'a N, fn_edge: F) -> Vec<(&'a N, Option<&'a E>)>
where
I: Iterator<Item = (&'a N, &'a E)>,
F: Fn(&'s Self, &'a N) -> I,
'a: 's,
{
let mut back_link = BTreeMap::new();
let mut queue = VecDeque::from([pkg]);
let mut bottom = None;

while let Some(p) = queue.pop_front() {
bottom = Some(p);
for (child, edge) in fn_edge(&self, p) {
bottom = None;
back_link.entry(child).or_insert_with(|| {
queue.push_back(child);
(p, edge)
});
}
if bottom.is_some() {
break;
}
}

let mut result = Vec::new();
let mut next =
bottom.expect("the only path was a cycle, no dependency graph has this shape");
while let Some((p, e)) = back_link.remove(&next) {
result.push((next, Some(e)));
next = p;
}
result.push((next, None));
result.reverse();
#[cfg(debug_assertions)]
{
for x in result.windows(2) {
let [(n2, _), (n1, Some(e12))] = x else {
unreachable!()
};
assert!(std::ptr::eq(self.edge(n1, n2).unwrap(), *e12));
assert!(std::ptr::eq(
self.edge(n1, n2).or(self.edge(n2, n1)).unwrap(),
*e12
));
}
let last = result.last().unwrap().0;
// fixme: this may be wrong when there are cycles, but we dont have them in tests.
assert!(!self
.nodes
.iter()
.any(|(_, adjacent)| adjacent.contains_key(last)));
// fixme: this may sometimes be wrong when there are cycles.
if !fn_edge(&self, last).next().is_none() {
self.print_for_test();
unreachable!("The last element in the path should not have outgoing edges");
}
}
result
}
}

#[test]
fn path_to_case() {
let mut new = Graph::new();
new.link(0, 3);
new.link(1, 0);
new.link(2, 0);
new.link(2, 1);
assert_eq!(
new.path_to_bottom(&2),
vec![(&2, None), (&0, Some(&())), (&3, Some(&()))]
);
}

impl<N: Eq + Ord + Clone, E: Default + Clone> Default for Graph<N, E> {
fn default() -> Graph<N, E> {
Graph::new()
Expand All @@ -189,6 +212,35 @@ impl<N: fmt::Display + Eq + Ord + Clone, E: Clone> fmt::Debug for Graph<N, E> {
}
}

impl<N: Eq + Ord + Clone, E: Clone> Graph<N, E> {
/// Prints the graph for constructing unit tests.
///
/// For purposes of graph traversal algorithms the edge values do not matter,
/// and the only value of the node we care about is the order it gets compared in.
/// This constructs a graph with the same topology but with integer keys and unit edges.
#[cfg(debug_assertions)]
fn print_for_test(&self) {
// Isolate and print a test case.
let names = self
.nodes
.keys()
.chain(self.nodes.values().flat_map(|vs| vs.keys()))
.collect::<BTreeSet<_>>()
.into_iter()
.collect::<Vec<_>>();
let mut new = Graph::new();
for n1 in self.nodes.keys() {
let name1 = names.binary_search(&n1).unwrap();
new.add(name1);
for n2 in self.nodes[n1].keys() {
let name2 = names.binary_search(&n2).unwrap();
*new.link(name1, name2) = ();
}
}
dbg!(new);
}
}

impl<N: Eq + Ord + Clone, E: Eq + Clone> PartialEq for Graph<N, E> {
fn eq(&self, other: &Graph<N, E>) -> bool {
self.nodes.eq(&other.nodes)
Expand Down

0 comments on commit 1051246

Please sign in to comment.