Auto merge of #12977 - Eh2406:bug_12941, r=epage

If the only path is a loop then counted as the shortest path.

This is a fix for #12941

This graph data structure is used to store dependency DAGs. Where each edge represents a dependency from a package to the package that fulfilled the dependency. Different parts of the resolver store this data in opposite directions, sometimes packages point at the things that depend on them other times packages point to the parents that required them. Error messages often need to report on why a package is in the graph, either by walking up toward parents or down toward children depending on how this graph is stored. #12678 unified the two different walking implementations, and replace them with a breadth first search so as to find the shortest path. This code ignored when edge pointed at a package that had already been reached, because that generally describes a longer path to an existing package.

Unfortunately, when I said this was a DAG that was a simplification. There can be cycles introduced as dev-dependencies. The existing code would reasonably ignore the cycles figuring that if we continue searching we would eventually find the root package (a package that nothing depended on). Missing the possibility that the root package created the cycle.

Now we search through the entire graph looking for a root package. If we do not find a root package we report the path to the last package we processed.

Author: bors

Diff for: src/cargo/util/graph.rs

@@ -128,30 +128,32 @@ impl<'s, N: Eq + Ord + Clone + 's, E: Default + Clone + 's> Graph<N, E> {
     {
         let mut back_link = BTreeMap::new();
         let mut queue = VecDeque::from([pkg]);
-        let mut bottom = None;
+        let mut last = pkg;
 
         while let Some(p) = queue.pop_front() {
-            bottom = Some(p);
+            last = p;
+            let mut out_edges = true;
             for (child, edge) in fn_edge(&self, p) {
-                bottom = None;
+                out_edges = false;
                 back_link.entry(child).or_insert_with(|| {
                     queue.push_back(child);
                     (p, edge)
                 });
             }
-            if bottom.is_some() {
+            if out_edges {
                 break;
             }
         }
 
         let mut result = Vec::new();
-        let mut next =
-            bottom.expect("the only path was a cycle, no dependency graph has this shape");
+        let mut next = last;
         while let Some((p, e)) = back_link.remove(&next) {
             result.push((next, Some(e)));
             next = p;
         }
-        result.push((next, None));
+        if result.iter().all(|(n, _)| n != &next) {
+            result.push((next, None));
+        }
         result.reverse();
         #[cfg(debug_assertions)]
         {
@@ -165,8 +167,12 @@ impl<'s, N: Eq + Ord + Clone + 's, E: Default + Clone + 's> Graph<N, E> {
                 ));
             }
             let last = result.last().unwrap().0;
-            // fixme: this may sometimes be wrong when there are cycles.
-            if !fn_edge(&self, last).next().is_none() {
+            let set: Vec<_> = result.iter().map(|(k, _)| k).collect();
+            if !fn_edge(&self, last)
+                .filter(|(e, _)| !set.contains(&e))
+                .next()
+                .is_none()
+            {
                 self.print_for_test();
                 unreachable!("The last element in the path should not have outgoing edges");
             }
@@ -188,6 +194,14 @@ fn path_to_case() {
     );
 }
 
+#[test]
+fn path_to_self() {
+    // Extracted from #12941
+    let mut new: Graph<i32, ()> = Graph::new();
+    new.link(0, 0);
+    assert_eq!(new.path_to_bottom(&0), vec![(&0, Some(&()))]);
+}
+
 impl<N: Eq + Ord + Clone, E: Default + Clone> Default for Graph<N, E> {
     fn default() -> Graph<N, E> {
         Graph::new()

Diff for: tests/testsuite/test.rs

@@ -3555,6 +3555,39 @@ fn cyclic_dev() {
     p.cargo("test --workspace").run();
 }
 
+#[cargo_test]
+fn cyclical_dep_with_missing_feature() {
+    // Checks for error handling when a cyclical dev-dependency specify a
+    // feature that doesn't exist.
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+
+                [dev-dependencies]
+                foo = { path = ".", features = ["missing"] }
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+    p.cargo("check")
+        .with_status(101)
+        .with_stderr(
+            "error: failed to select a version for `foo`.
+    ... required by package `foo v0.1.0 ([..]/foo)`
+versions that meet the requirements `*` are: 0.1.0
+
+the package `foo` depends on `foo`, with features: `missing` but `foo` does not have these features.
+
+
+failed to select a version for `foo` which could resolve this conflict",
+        )
+        .run();
+}
+
 #[cargo_test]
 fn publish_a_crate_without_tests() {
     Package::new("testless", "0.1.0")