From 245e69ce10da0e63473a06c95af7cf8e1eaeab61 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Tue, 19 Sep 2023 20:22:13 +0000 Subject: [PATCH 1/3] add a test for shortest path --- crates/resolver-tests/src/lib.rs | 2 +- crates/resolver-tests/tests/resolve.rs | 34 ++++++++++++++++++++++++++ src/cargo/core/resolver/dep_cache.rs | 5 ++-- 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/crates/resolver-tests/src/lib.rs b/crates/resolver-tests/src/lib.rs index f6e93e65cde..9bdeb867415 100644 --- a/crates/resolver-tests/src/lib.rs +++ b/crates/resolver-tests/src/lib.rs @@ -164,7 +164,7 @@ pub fn resolve_with_config_raw( // we found a case that causes a panic and did not use all of the input. // lets print the part of the input that was used for minimization. eprintln!( - "{:?}", + "Part used befor drop: {:?}", PrettyPrintRegistry( self.list .iter() diff --git a/crates/resolver-tests/tests/resolve.rs b/crates/resolver-tests/tests/resolve.rs index 02486bfb5db..c1b07d17433 100644 --- a/crates/resolver-tests/tests/resolve.rs +++ b/crates/resolver-tests/tests/resolve.rs @@ -1562,3 +1562,37 @@ package `A v0.0.0 (registry `https://example.com/`)` ... which satisfies dependency `C = \"*\"` of package `A v0.0.0 (registry `https://example.com/`)`\ ", error.to_string()); } + +#[test] +fn shortest_path_in_error_message() { + let input = vec![ + pkg!(("F", "0.1.2")), + pkg!(("F", "0.1.1") => [dep("bad"),]), + pkg!(("F", "0.1.0") => [dep("bad"),]), + pkg!("E" => [dep_req("F", "^0.1.2"),]), + pkg!("D" => [dep_req("F", "^0.1.2"),]), + pkg!("C" => [dep("D"),]), + pkg!("A" => [dep("C"),dep("E"),dep_req("F", "<=0.1.1"),]), + ]; + let error = resolve(vec![dep("A")], ®istry(input)).unwrap_err(); + println!("{}", error); + assert_eq!( + "\ +failed to select a version for `F`. + ... required by package `A v1.0.0 (registry `https://example.com/`)` + ... which satisfies dependency `A = \"*\"` of package `root v1.0.0 (registry `https://example.com/`)` +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 `A = \"*\"` of package `root v1.0.0 (registry `https://example.com/`)` + +failed to select a version for `F` which could resolve this conflict\ + ", + error.to_string() + ); +} diff --git a/src/cargo/core/resolver/dep_cache.rs b/src/cargo/core/resolver/dep_cache.rs index b230254cccd..7b6e0661f17 100644 --- a/src/cargo/core/resolver/dep_cache.rs +++ b/src/cargo/core/resolver/dep_cache.rs @@ -131,9 +131,8 @@ impl<'a> RegistryQueryer<'a> { .iter() .filter(|(spec, _)| spec.matches(summary.package_id())); - let (spec, dep) = match potential_matches.next() { - None => continue, - Some(replacement) => replacement, + let Some((spec, dep)) = potential_matches.next() else { + continue; }; debug!( "found an override for {} {}", From 2284b7a8858e82873fa0c76be61967366bbcbf6c Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Tue, 19 Sep 2023 20:22:13 +0000 Subject: [PATCH 2/3] added assertions to make sure the path code is correct in graphs --- src/cargo/util/graph.rs | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index ff4018201fe..b3214ee86d2 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -106,6 +106,18 @@ impl Graph { 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 } @@ -134,6 +146,21 @@ impl Graph { result.push(p); pkg = p.0; } + #[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)); + } + 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))); + } result } } From 679d65103f3d128c3d9b1ba0bf84ea5dff84cf2c Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Tue, 19 Sep 2023 20:22:13 +0000 Subject: [PATCH 3/3] shortest path is probably more informative than random path for error messages --- crates/resolver-tests/tests/resolve.rs | 5 +- src/cargo/util/graph.rs | 157 +++++++++++++++++-------- 2 files changed, 107 insertions(+), 55 deletions(-) diff --git a/crates/resolver-tests/tests/resolve.rs b/crates/resolver-tests/tests/resolve.rs index c1b07d17433..dd21502d8fd 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 b3214ee86d2..8c4a593eafd 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,36 @@ 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)] + #[allow(clippy::print_stderr)] + 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) = (); + } + } + eprintln!("Graph for tests = {new:#?}"); + } +} + impl PartialEq for Graph { fn eq(&self, other: &Graph) -> bool { self.nodes.eq(&other.nodes)