Improve error message for cyclic dependencies

First reported in rust-lang/rust#65014 it looks like our error message
on cyclic dependencies may be confusing at times. It looks like this is
an issue because there are multiple paths through a graph for a
dependency, so using the generic `path_to_top` function isn't producing
the most useful path for this purpose.

We're already walking the graph though, so this commit adds an extra
parameter which collects the list of packages we've visited so far to
produce a hopefully always-accurate error message showing the chain of
dependencies end-to-end for what depends on what.

Author: alexcrichton

crates/resolver-tests/tests/resolve.rs

@@ -1451,3 +1451,21 @@ fn conflict_store_more_then_one_match() {
     let reg = registry(input);
     let _ = resolve_and_validated(vec![dep("nA")], &reg, None);
 }
+
+#[test]
+fn cyclic_good_error_message() {
+    let input = vec![
+        pkg!(("A", "0.0.0") => [dep("C")]),
+        pkg!(("B", "0.0.0") => [dep("C")]),
+        pkg!(("C", "0.0.0") => [dep("A")]),
+    ];
+    let reg = registry(input);
+    let error = resolve(vec![dep("A"), dep("B")], &reg).unwrap_err();
+    println!("{}", error);
+    assert_eq!("\
+cyclic package dependency: package `A v0.0.0 (registry `https://example.com/`)` depends on itself. Cycle:
+package `A v0.0.0 (registry `https://example.com/`)`
+    ... which is depended on by `C v0.0.0 (registry `https://example.com/`)`
+    ... which is depended on by `A v0.0.0 (registry `https://example.com/`)`\
+", error.to_string());
+}

src/cargo/core/resolver/mod.rs

@@ -993,9 +993,11 @@ fn check_cycles(resolve: &Resolve) -> CargoResult<()> {
     let mut all_packages: Vec<_> = resolve.iter().collect();
     all_packages.sort_unstable();
     let mut checked = HashSet::new();
+    let mut path = Vec::new();
+    let mut visited = HashSet::new();
     for pkg in all_packages {
         if !checked.contains(&pkg) {
-            visit(resolve, pkg, &mut HashSet::new(), &mut checked)?
+            visit(resolve, pkg, &mut visited, &mut path, &mut checked)?
         }
     }
     return Ok(());
@@ -1004,14 +1006,16 @@ fn check_cycles(resolve: &Resolve) -> CargoResult<()> {
         resolve: &Resolve,
         id: PackageId,
         visited: &mut HashSet<PackageId>,
+        path: &mut Vec<PackageId>,
         checked: &mut HashSet<PackageId>,
     ) -> CargoResult<()> {
+        path.push(id);
         // See if we visited ourselves
         if !visited.insert(id) {
             failure::bail!(
                 "cyclic package dependency: package `{}` depends on itself. Cycle:\n{}",
                 id,
-                errors::describe_path(&resolve.path_to_top(&id))
+                errors::describe_path(&path.iter().rev().collect::<Vec<_>>()),
             );
         }
 
@@ -1023,23 +1027,25 @@ fn check_cycles(resolve: &Resolve) -> CargoResult<()> {
         // visitation list as we can't induce a cycle through transitive
         // dependencies.
         if checked.insert(id) {
+            let mut empty_set = HashSet::new();
+            let mut empty_vec = Vec::new();
             for (dep, listings) in resolve.deps_not_replaced(id) {
                 let is_transitive = listings.iter().any(|d| d.is_transitive());
-                let mut empty = HashSet::new();
-                let visited = if is_transitive {
-                    &mut *visited
+                let (visited, path) = if is_transitive {
+                    (&mut *visited, &mut *path)
                 } else {
-                    &mut empty
+                    (&mut empty_set, &mut empty_vec)
                 };
-                visit(resolve, dep, visited, checked)?;
+                visit(resolve, dep, visited, path, checked)?;
 
                 if let Some(id) = resolve.replacement(dep) {
-                    visit(resolve, id, visited, checked)?;
+                    visit(resolve, id, visited, path, checked)?;
                 }
             }
         }
 
         // Ok, we're done, no longer visiting our node any more
+        path.pop();
         visited.remove(&id);
         Ok(())
     }

tests/testsuite/build.rs

@@ -1487,7 +1487,8 @@ fn self_dependency() {
         .with_stderr(
             "\
 [ERROR] cyclic package dependency: package `test v0.0.0 ([CWD])` depends on itself. Cycle:
-package `test v0.0.0 ([CWD])`",
+package `test v0.0.0 ([CWD])`
+    ... which is depended on by `test v0.0.0 ([..])`",
         )
         .run();
 }
@@ -2613,7 +2614,8 @@ fn cyclic_deps_rejected() {
         .with_stderr(
 "[ERROR] cyclic package dependency: package `a v0.0.1 ([CWD]/a)` depends on itself. Cycle:
 package `a v0.0.1 ([CWD]/a)`
-    ... which is depended on by `foo v0.0.1 ([CWD])`",
+    ... which is depended on by `foo v0.0.1 ([CWD])`
+    ... which is depended on by `a v0.0.1 ([..])`",
         ).run();
 }
 

tests/testsuite/patch.rs

@@ -1187,6 +1187,7 @@ fn cycle() {
 error: cyclic package dependency: [..]
 package `[..]`
     ... which is depended on by `[..]`
+    ... which is depended on by `[..]`
 ",
         )
         .run();