From 1051246c2fc5dd32cf6f63686c5ecca771369c1b Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Tue, 19 Sep 2023 20:22:13 +0000 Subject: [PATCH] shortest path is probably more informative than random path for error messages --- crates/resolver-tests/tests/resolve.rs | 5 +- src/cargo/util/graph.rs | 156 ++++++++++++++++--------- 2 files changed, 106 insertions(+), 55 deletions(-) diff --git a/crates/resolver-tests/tests/resolve.rs b/crates/resolver-tests/tests/resolve.rs index c1b07d174336..dd21502d8fd1 100644 --- a/crates/resolver-tests/tests/resolve.rs +++ b/crates/resolver-tests/tests/resolve.rs @@ -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\ diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index b3214ee86d2d..abd0e364ad64 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -1,5 +1,5 @@ use std::borrow::Borrow; -use std::collections::BTreeSet; +use std::collections::{BTreeMap, BTreeSet, VecDeque}; use std::fmt; pub struct Graph { @@ -87,84 +87,107 @@ impl Graph { 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 { + fn path_to<'a, F, I>(&'s self, pkg: &'a N, fn_edge: F) -> Vec<(&'a N, Option<&'a E>)> + where + I: Iterator, + 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 Default for Graph { fn default() -> Graph { Graph::new() @@ -189,6 +212,35 @@ impl fmt::Debug for Graph { } } +impl Graph { + /// 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::>() + .into_iter() + .collect::>(); + 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 PartialEq for Graph { fn eq(&self, other: &Graph) -> bool { self.nodes.eq(&other.nodes)