Skip to content

Commit 808ffa9

Browse files
committed
Auto merge of #12678 - Eh2406:shortest_path, r=epage
Shortest path ### What does this PR try to resolve? Currently error messages from the resolver are based a random path from a package that couldn't be resolved back to the root package. It was pointed out to me that the shortest route is likely to be more useful so the switches to using breadth first search to pick the path. There is also one re-factor to use let-else that I spotted while doing this work. ### How should we test and review this PR? The shortest path is is a random path, so this is not technically a behavioral change. As such the tests still pass is good evidence for being reasonable.
2 parents 976771d + 679d651 commit 808ffa9

File tree

4 files changed

+150
-38
lines changed

4 files changed

+150
-38
lines changed

crates/resolver-tests/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ pub fn resolve_with_config_raw(
164164
// we found a case that causes a panic and did not use all of the input.
165165
// lets print the part of the input that was used for minimization.
166166
eprintln!(
167-
"{:?}",
167+
"Part used befor drop: {:?}",
168168
PrettyPrintRegistry(
169169
self.list
170170
.iter()

crates/resolver-tests/tests/resolve.rs

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1562,3 +1562,36 @@ package `A v0.0.0 (registry `https://example.com/`)`
15621562
... which satisfies dependency `C = \"*\"` of package `A v0.0.0 (registry `https://example.com/`)`\
15631563
", error.to_string());
15641564
}
1565+
1566+
#[test]
1567+
fn shortest_path_in_error_message() {
1568+
let input = vec![
1569+
pkg!(("F", "0.1.2")),
1570+
pkg!(("F", "0.1.1") => [dep("bad"),]),
1571+
pkg!(("F", "0.1.0") => [dep("bad"),]),
1572+
pkg!("E" => [dep_req("F", "^0.1.2"),]),
1573+
pkg!("D" => [dep_req("F", "^0.1.2"),]),
1574+
pkg!("C" => [dep("D"),]),
1575+
pkg!("A" => [dep("C"),dep("E"),dep_req("F", "<=0.1.1"),]),
1576+
];
1577+
let error = resolve(vec![dep("A")], &registry(input)).unwrap_err();
1578+
println!("{}", error);
1579+
assert_eq!(
1580+
"\
1581+
failed to select a version for `F`.
1582+
... required by package `A v1.0.0 (registry `https://example.com/`)`
1583+
... which satisfies dependency `A = \"*\"` of package `root v1.0.0 (registry `https://example.com/`)`
1584+
versions that meet the requirements `<=0.1.1` are: 0.1.1, 0.1.0
1585+
1586+
all possible versions conflict with previously selected packages.
1587+
1588+
previously selected package `F v0.1.2 (registry `https://example.com/`)`
1589+
... which satisfies dependency `F = \"^0.1.2\"` of package `E v1.0.0 (registry `https://example.com/`)`
1590+
... which satisfies dependency `E = \"*\"` of package `A v1.0.0 (registry `https://example.com/`)`
1591+
... which satisfies dependency `A = \"*\"` of package `root v1.0.0 (registry `https://example.com/`)`
1592+
1593+
failed to select a version for `F` which could resolve this conflict\
1594+
",
1595+
error.to_string()
1596+
);
1597+
}

src/cargo/core/resolver/dep_cache.rs

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -131,9 +131,8 @@ impl<'a> RegistryQueryer<'a> {
131131
.iter()
132132
.filter(|(spec, _)| spec.matches(summary.package_id()));
133133

134-
let (spec, dep) = match potential_matches.next() {
135-
None => continue,
136-
Some(replacement) => replacement,
134+
let Some((spec, dep)) = potential_matches.next() else {
135+
continue;
137136
};
138137
debug!(
139138
"found an override for {} {}",

src/cargo/util/graph.rs

Lines changed: 114 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use std::borrow::Borrow;
2-
use std::collections::BTreeSet;
2+
use std::collections::{BTreeMap, BTreeSet, VecDeque};
33
use std::fmt;
44

55
pub struct Graph<N: Clone, E: Clone> {
@@ -87,57 +87,107 @@ impl<N: Eq + Ord + Clone, E: Default + Clone> Graph<N, E> {
8787
false
8888
}
8989

90-
/// Resolves one of the paths from the given dependent package down to
91-
/// a leaf.
90+
/// Resolves one of the paths from the given dependent package down to a leaf.
91+
///
92+
/// The path return will be the shortest path, or more accurately one of the paths with the shortest length.
9293
///
9394
/// Each element contains a node along with an edge except the first one.
9495
/// The representation would look like:
9596
///
9697
/// (Node0,) -> (Node1, Edge01) -> (Node2, Edge12)...
97-
pub fn path_to_bottom<'a>(&'a self, mut pkg: &'a N) -> Vec<(&'a N, Option<&'a E>)> {
98-
let mut result = vec![(pkg, None)];
99-
while let Some(p) = self.nodes.get(pkg).and_then(|p| {
100-
p.iter()
101-
// Note that we can have "cycles" introduced through dev-dependency
102-
// edges, so make sure we don't loop infinitely.
103-
.find(|&(node, _)| result.iter().all(|p| p.0 != node))
104-
.map(|(node, edge)| (node, Some(edge)))
105-
}) {
106-
result.push(p);
107-
pkg = p.0;
108-
}
109-
result
98+
pub fn path_to_bottom<'a>(&'a self, pkg: &'a N) -> Vec<(&'a N, Option<&'a E>)> {
99+
self.path_to(pkg, |s, p| s.edges(p))
110100
}
111101

112-
/// Resolves one of the paths from the given dependent package up to
113-
/// the root.
102+
/// Resolves one of the paths from the given dependent package up to the root.
103+
///
104+
/// The path return will be the shortest path, or more accurately one of the paths with the shortest length.
114105
///
115106
/// Each element contains a node along with an edge except the first one.
116107
/// The representation would look like:
117108
///
118109
/// (Node0,) -> (Node1, Edge01) -> (Node2, Edge12)...
119-
pub fn path_to_top<'a>(&'a self, mut pkg: &'a N) -> Vec<(&'a N, Option<&'a E>)> {
120-
// Note that this implementation isn't the most robust per se, we'll
121-
// likely have to tweak this over time. For now though it works for what
122-
// it's used for!
123-
let mut result = vec![(pkg, None)];
124-
let first_pkg_depending_on = |pkg, res: &[(&N, Option<&E>)]| {
125-
self.nodes
110+
pub fn path_to_top<'a>(&'a self, pkg: &'a N) -> Vec<(&'a N, Option<&'a E>)> {
111+
self.path_to(pkg, |s, pk| {
112+
// Note that this implementation isn't the most robust per se, we'll
113+
// likely have to tweak this over time. For now though it works for what
114+
// it's used for!
115+
s.nodes
126116
.iter()
127-
.filter(|(_, adjacent)| adjacent.contains_key(pkg))
128-
// Note that we can have "cycles" introduced through dev-dependency
129-
// edges, so make sure we don't loop infinitely.
130-
.find(|&(node, _)| !res.iter().any(|p| p.0 == node))
131-
.map(|(p, adjacent)| (p, adjacent.get(pkg)))
132-
};
133-
while let Some(p) = first_pkg_depending_on(pkg, &result) {
134-
result.push(p);
135-
pkg = p.0;
117+
.filter_map(|(p, adjacent)| adjacent.get(pk).map(|e| (p, e)))
118+
})
119+
}
120+
}
121+
122+
impl<'s, N: Eq + Ord + Clone + 's, E: Default + Clone + 's> Graph<N, E> {
123+
fn path_to<'a, F, I>(&'s self, pkg: &'a N, fn_edge: F) -> Vec<(&'a N, Option<&'a E>)>
124+
where
125+
I: Iterator<Item = (&'a N, &'a E)>,
126+
F: Fn(&'s Self, &'a N) -> I,
127+
'a: 's,
128+
{
129+
let mut back_link = BTreeMap::new();
130+
let mut queue = VecDeque::from([pkg]);
131+
let mut bottom = None;
132+
133+
while let Some(p) = queue.pop_front() {
134+
bottom = Some(p);
135+
for (child, edge) in fn_edge(&self, p) {
136+
bottom = None;
137+
back_link.entry(child).or_insert_with(|| {
138+
queue.push_back(child);
139+
(p, edge)
140+
});
141+
}
142+
if bottom.is_some() {
143+
break;
144+
}
145+
}
146+
147+
let mut result = Vec::new();
148+
let mut next =
149+
bottom.expect("the only path was a cycle, no dependency graph has this shape");
150+
while let Some((p, e)) = back_link.remove(&next) {
151+
result.push((next, Some(e)));
152+
next = p;
153+
}
154+
result.push((next, None));
155+
result.reverse();
156+
#[cfg(debug_assertions)]
157+
{
158+
for x in result.windows(2) {
159+
let [(n2, _), (n1, Some(e12))] = x else {
160+
unreachable!()
161+
};
162+
assert!(std::ptr::eq(
163+
self.edge(n1, n2).or(self.edge(n2, n1)).unwrap(),
164+
*e12
165+
));
166+
}
167+
let last = result.last().unwrap().0;
168+
// fixme: this may sometimes be wrong when there are cycles.
169+
if !fn_edge(&self, last).next().is_none() {
170+
self.print_for_test();
171+
unreachable!("The last element in the path should not have outgoing edges");
172+
}
136173
}
137174
result
138175
}
139176
}
140177

178+
#[test]
179+
fn path_to_case() {
180+
let mut new = Graph::new();
181+
new.link(0, 3);
182+
new.link(1, 0);
183+
new.link(2, 0);
184+
new.link(2, 1);
185+
assert_eq!(
186+
new.path_to_bottom(&2),
187+
vec![(&2, None), (&0, Some(&())), (&3, Some(&()))]
188+
);
189+
}
190+
141191
impl<N: Eq + Ord + Clone, E: Default + Clone> Default for Graph<N, E> {
142192
fn default() -> Graph<N, E> {
143193
Graph::new()
@@ -162,6 +212,36 @@ impl<N: fmt::Display + Eq + Ord + Clone, E: Clone> fmt::Debug for Graph<N, E> {
162212
}
163213
}
164214

215+
impl<N: Eq + Ord + Clone, E: Clone> Graph<N, E> {
216+
/// Prints the graph for constructing unit tests.
217+
///
218+
/// For purposes of graph traversal algorithms the edge values do not matter,
219+
/// and the only value of the node we care about is the order it gets compared in.
220+
/// This constructs a graph with the same topology but with integer keys and unit edges.
221+
#[cfg(debug_assertions)]
222+
#[allow(clippy::print_stderr)]
223+
fn print_for_test(&self) {
224+
// Isolate and print a test case.
225+
let names = self
226+
.nodes
227+
.keys()
228+
.chain(self.nodes.values().flat_map(|vs| vs.keys()))
229+
.collect::<BTreeSet<_>>()
230+
.into_iter()
231+
.collect::<Vec<_>>();
232+
let mut new = Graph::new();
233+
for n1 in self.nodes.keys() {
234+
let name1 = names.binary_search(&n1).unwrap();
235+
new.add(name1);
236+
for n2 in self.nodes[n1].keys() {
237+
let name2 = names.binary_search(&n2).unwrap();
238+
*new.link(name1, name2) = ();
239+
}
240+
}
241+
eprintln!("Graph for tests = {new:#?}");
242+
}
243+
}
244+
165245
impl<N: Eq + Ord + Clone, E: Eq + Clone> PartialEq for Graph<N, E> {
166246
fn eq(&self, other: &Graph<N, E>) -> bool {
167247
self.nodes.eq(&other.nodes)

0 commit comments

Comments
 (0)