shortest path is probably more informative than random path for error messages

Author: Eh2406

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\

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,84 +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;
-        }
-        #[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<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).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<N: Eq + Ord + Clone, E: Default + Clone> Default for Graph<N, E> {
     fn default() -> Graph<N, E> {
         Graph::new()
@@ -189,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)