Shortest path

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)