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.

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()

crates/resolver-tests/tests/resolve.rs

@@ -1562,3 +1562,36 @@ 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")], &registry(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 `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\
+    ",
+        error.to_string()
+    );
+}

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 {} {}",

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<N: Clone, E: Clone> {
@@ -87,57 +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;
-        }
-        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).or(self.edge(n2, n1)).unwrap(),
+                    *e12
+                ));
+            }
+            let last = result.last().unwrap().0;
+            // 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()
@@ -162,6 +212,36 @@ 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)]
+    #[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::<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) = ();
+            }
+        }
+        eprintln!("Graph for tests = {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)