Auto merge of rust-lang#5025 - Eh2406:error_mesges, r=alexcrichton

better resolver error messages

This is a start on beter resolver error messages. This is mostly trying to copy the `links` messages. In the process I found that we wor not testing the common case of having found candidates and still not resolving.

Any advice?

src/cargo/core/resolver/mod.rs

@@ -118,28 +118,8 @@ struct Candidate {
 impl Resolve {
     /// Resolves one of the paths from the given dependent package up to
     /// the root.
-    pub fn path_to_top<'a>(&'a self, mut pkg: &'a PackageId) -> Vec<&'a PackageId> {
-        // 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];
-        let first_pkg_depending_on = |pkg: &PackageId| {
-            self.graph.get_nodes()
-                .iter()
-                .filter(|&(_node, adjacent)| adjacent.contains(pkg))
-                .next()
-                .map(|p| p.0)
-        };
-        while let Some(p) = first_pkg_depending_on(pkg) {
-            // Note that we can have "cycles" introduced through dev-dependency
-            // edges, so make sure we don't loop infinitely.
-            if result.contains(&p) {
-                break
-            }
-            result.push(p);
-            pkg = p;
-        }
-        result
+    pub fn path_to_top<'a>(&'a self, pkg: &'a PackageId) -> Vec<&'a PackageId> {
+        self.graph.path_to_top(pkg)
     }
     pub fn register_used_patches(&mut self,
                                  patches: &HashMap<Url, Vec<Summary>>) {
@@ -806,30 +786,28 @@ fn activation_error(cx: &Context,
                     prev_active: &[Summary],
                     candidates: &[Candidate],
                     config: Option<&Config>) -> CargoError {
+    let graph = cx.graph();
+    let describe_path = |pkgid: &PackageId| -> String {
+        use std::fmt::Write;
+        let dep_path = graph.path_to_top(pkgid);
+        let mut dep_path_desc = format!("package `{}`", dep_path[0]);
+        for dep in dep_path.iter().skip(1) {
+            write!(dep_path_desc,
+                   "\n    ... which is depended on by `{}`",
+                   dep).unwrap();
+        }
+        dep_path_desc
+    };
     if !candidates.is_empty() {
-        let mut msg = format!("failed to select a version for `{}` \
-                               (required by `{}`):\n\
+        let mut msg = format!("failed to select a version for `{0}`\n\
                                all possible versions conflict with \
-                               previously selected versions of `{}`",
-                              dep.name(), parent.name(),
+                               previously selected versions of `{0}`\n",
                               dep.name());
-        let graph = cx.graph();
-        'outer: for v in prev_active.iter() {
-            for node in graph.iter() {
-                let edges = match graph.edges(node) {
-                    Some(edges) => edges,
-                    None => continue,
-                };
-                for edge in edges {
-                    if edge != v.package_id() { continue }
-
-                    msg.push_str(&format!("\n  version {} in use by {}",
-                                          v.version(), edge));
-                    continue 'outer;
-                }
-            }
-            msg.push_str(&format!("\n  version {} in use by ??",
-                                  v.version()));
+        msg.push_str("required by ");
+        msg.push_str(&describe_path(parent.package_id()));
+        for v in prev_active.iter() {
+            msg.push_str("\n  previously selected ");
+            msg.push_str(&describe_path(v.package_id()));
         }
 
         msg.push_str(&format!("\n  possible versions to select: {}",
@@ -873,15 +851,15 @@ fn activation_error(cx: &Context,
             versions.join(", ")
         };
 
-        let mut msg = format!("no matching version `{}` found for package `{}` \
-                               (required by `{}`)\n\
+        let mut msg = format!("no matching version `{}` found for package `{}`\n\
                                location searched: {}\n\
-                               versions found: {}",
+                               versions found: {}\n",
                               dep.version_req(),
                               dep.name(),
-                              parent.name(),
                               dep.source_id(),
                               versions);
+        msg.push_str("required by ");
+        msg.push_str(&describe_path(parent.package_id()));
 
         // If we have a path dependency with a locked version, then this may
         // indicate that we updated a sub-package and forgot to run `cargo
@@ -894,13 +872,13 @@ fn activation_error(cx: &Context,
 
         msg
     } else {
-        format!("no matching package named `{}` found \
-                 (required by `{}`)\n\
-                 location searched: {}\n\
-                 version required: {}",
-                dep.name(), parent.name(),
-                dep.source_id(),
-                dep.version_req())
+        let mut msg = format!("no matching package named `{}` found\n\
+                 location searched: {}\n",
+                dep.name(), dep.source_id());
+        msg.push_str("required by ");
+        msg.push_str(&describe_path(parent.package_id()));
+
+        msg
     };
 
     if let Some(config) = config {

src/cargo/util/graph.rs

@@ -67,6 +67,30 @@ impl<N: Eq + Hash + Clone> Graph<N> {
     pub fn iter(&self) -> Nodes<N> {
         self.nodes.keys()
     }
+
+    /// Resolves one of the paths from the given dependent package up to
+    /// the root.
+    pub fn path_to_top<'a>(&'a self, mut pkg: &'a N) -> Vec<&'a N> {
+        // 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];
+        let first_pkg_depending_on = |pkg: &N, res: &[&N]| {
+            self.get_nodes()
+                .iter()
+                .filter(|&(_node, adjacent)| adjacent.contains(pkg))
+                // Note that we can have "cycles" introduced through dev-dependency
+                // edges, so make sure we don't loop infinitely.
+                .filter(|&(_node, _)| !res.contains(&_node))
+                .next()
+                .map(|p| p.0)
+        };
+        while let Some(p) = first_pkg_depending_on(pkg, &result) {
+            result.push(p);
+            pkg = p;
+        }
+        result
+    }
 }
 
 impl<N: Eq + Hash + Clone> Default for Graph<N> {

tests/build.rs

@@ -784,9 +784,9 @@ fn cargo_compile_with_dep_name_mismatch() {
 
     assert_that(p.cargo("build"),
                 execs().with_status(101).with_stderr(&format!(
-r#"[ERROR] no matching package named `notquitebar` found (required by `foo`)
+r#"error: no matching package named `notquitebar` found
 location searched: {proj_dir}/bar
-version required: *
+required by package `foo v0.0.1 ({proj_dir})`
 "#, proj_dir = p.url())));
 }
 
@@ -921,9 +921,9 @@ fn cargo_compile_offline_not_try_update() {
     assert_that(p.cargo("build").masquerade_as_nightly_cargo().arg("-Zoffline"),
                 execs().with_status(101)
                     .with_stderr("\
-error: no matching package named `not_cached_dep` found (required by `bar`)
+error: no matching package named `not_cached_dep` found
 location searched: registry `[..]`
-version required: ^1.2.5
+required by package `bar v0.1.0 ([..])`
 As a reminder, you're using offline mode (-Z offline) \
 which can sometimes cause surprising resolution failures, \
 if this error is too confusing you may with to retry \
@@ -996,6 +996,49 @@ fn main(){
     );
 }
 
+#[test]
+fn incompatible_dependencies() {
+    Package::new("bad", "0.1.0").publish();
+    Package::new("bad", "1.0.0").publish();
+    Package::new("bad", "1.0.1").publish();
+    Package::new("bad", "1.0.2").publish();
+    Package::new("foo", "0.1.0").dep("bad", "0.1.0").publish();
+    Package::new("bar", "0.1.1").dep("bad", "=1.0.0").publish();
+    Package::new("bar", "0.1.0").dep("bad", "=1.0.0").publish();
+    Package::new("baz", "0.1.2").dep("bad", ">=1.0.1").publish();
+    Package::new("baz", "0.1.1").dep("bad", ">=1.0.1").publish();
+    Package::new("baz", "0.1.0").dep("bad", ">=1.0.1").publish();
+
+    let p = project("transitive_load_test")
+        .file("Cargo.toml", r#"
+            [project]
+            name = "incompatible_dependencies"
+            version = "0.0.1"
+
+            [dependencies]
+            foo = "0.1.0"
+            bar = "0.1.0"
+            baz = "0.1.0"
+        "#)
+        .file("src/main.rs", "fn main(){}")
+        .build();
+
+    assert_that(p.cargo("build"),
+        execs().with_status(101)
+            .with_stderr_contains("\
+error: failed to select a version for `bad`
+all possible versions conflict with previously selected versions of `bad`
+required by package `baz v0.1.0`
+    ... which is depended on by `incompatible_dependencies v0.0.1 ([..])`
+  previously selected package `bad v0.1.0`
+    ... which is depended on by `foo v0.1.0`
+    ... which is depended on by `incompatible_dependencies v0.0.1 ([..])`
+  previously selected package `bad v1.0.0`
+    ... which is depended on by `bar v0.1.0`
+    ... which is depended on by `incompatible_dependencies v0.0.1 ([..])`
+  possible versions to select: 1.0.2, 1.0.1"));
+}
+
 #[test]
 fn compile_offline_while_transitive_dep_not_cached() {
     let bar = Package::new("bar", "1.0.0");
@@ -1031,9 +1074,10 @@ fn compile_offline_while_transitive_dep_not_cached() {
     assert_that(p.cargo("build").masquerade_as_nightly_cargo().arg("-Zoffline"),
         execs().with_status(101)
             .with_stderr("\
-error: no matching package named `bar` found (required by `foo`)
+error: no matching package named `bar` found
 location searched: registry `[..]`
-version required: = 1.0.0
+required by package `foo v0.1.0`
+    ... which is depended on by `transitive_load_test v0.0.1 ([..]/transitive_load_test)`
 As a reminder, you're using offline mode (-Z offline) \
 which can sometimes cause surprising resolution failures, \
 if this error is too confusing you may with to retry \
@@ -1073,9 +1117,10 @@ fn compile_path_dep_then_change_version() {
 
     assert_that(p.cargo("build"),
                 execs().with_status(101).with_stderr("\
-[ERROR] no matching version `= 0.0.1` found for package `bar` (required by `foo`)
+error: no matching version `= 0.0.1` found for package `bar`
 location searched: [..]
 versions found: 0.0.2
+required by package `foo v0.0.1 ([..]/foo)`
 consider running `cargo update` to update a path dependency's locked version
 "));
 }

tests/directory.rs

@@ -195,9 +195,9 @@ fn simple_install_fail() {
 error: failed to compile `bar v0.1.0`, intermediate artifacts can be found at `[..]`
 
 Caused by:
-  no matching package named `baz` found (required by `bar`)
+  no matching package named `baz` found
 location searched: registry `https://github.com/rust-lang/crates.io-index`
-version required: ^9.8.7
+required by package `bar v0.1.0`
 "));
 }
 
@@ -276,9 +276,9 @@ fn not_there() {
 
     assert_that(p.cargo("build"),
                 execs().with_status(101).with_stderr("\
-error: no matching package named `foo` found (required by `bar`)
+error: no matching package named `foo` found
 location searched: [..]
-version required: ^0.1.0
+required by package `bar v0.1.0 ([..])`
 "));
 }
 

tests/path.rs

@@ -950,9 +950,9 @@ fn invalid_path_dep_in_workspace_with_lockfile() {
     assert_that(p.cargo("build"),
                 execs().with_status(101)
                        .with_stderr("\
-error: no matching package named `bar` found (required by `foo`)
+error: no matching package named `bar` found
 location searched: [..]
-version required: *
+required by package `foo v0.5.0 ([..])`
 "));
 }
 

tests/registry.rs

@@ -110,9 +110,9 @@ fn nonexistent() {
     assert_that(p.cargo("build"),
                 execs().with_status(101).with_stderr("\
 [UPDATING] registry [..]
-[ERROR] no matching package named `nonexistent` found (required by `foo`)
+error: no matching package named `nonexistent` found
 location searched: registry [..]
-version required: >= 0.0.0
+required by package `foo v0.0.1 ([..])`
 "));
 }
 
@@ -136,19 +136,21 @@ fn wrong_version() {
 
     assert_that(p.cargo("build"),
                 execs().with_status(101).with_stderr_contains("\
-[ERROR] no matching version `>= 1.0.0` found for package `foo` (required by `foo`)
+error: no matching version `>= 1.0.0` found for package `foo`
 location searched: registry [..]
 versions found: 0.0.2, 0.0.1
+required by package `foo v0.0.1 ([..])`
 "));
 
     Package::new("foo", "0.0.3").publish();
     Package::new("foo", "0.0.4").publish();
 
     assert_that(p.cargo("build"),
                 execs().with_status(101).with_stderr_contains("\
-[ERROR] no matching version `>= 1.0.0` found for package `foo` (required by `foo`)
+error: no matching version `>= 1.0.0` found for package `foo`
 location searched: registry [..]
 versions found: 0.0.4, 0.0.3, 0.0.2, ...
+required by package `foo v0.0.1 ([..])`
 "));
 }
 
@@ -204,9 +206,9 @@ fn update_registry() {
 
     assert_that(p.cargo("build"),
                 execs().with_status(101).with_stderr_contains("\
-[ERROR] no matching package named `notyet` found (required by `foo`)
-location searched: registry [..]
-version required: >= 0.0.0
+error: no matching package named `notyet` found
+location searched: registry `[..]`
+required by package `foo v0.0.1 ([..])`
 "));
 
     Package::new("notyet", "0.0.1").publish();
@@ -256,9 +258,9 @@ fn package_with_path_deps() {
 [ERROR] failed to verify package tarball
 
 Caused by:
-  no matching package named `notyet` found (required by `foo`)
+  no matching package named `notyet` found
 location searched: registry [..]
-version required: ^0.0.1
+required by package `foo v0.0.1 ([..])`
 "));
 
     Package::new("notyet", "0.0.1").publish();
@@ -401,9 +403,10 @@ fn relying_on_a_yank_is_bad() {
 
     assert_that(p.cargo("build"),
                 execs().with_status(101).with_stderr_contains("\
-[ERROR] no matching version `= 0.0.2` found for package `baz` (required by `bar`)
-location searched: registry [..]
+error: no matching version `= 0.0.2` found for package `baz`
+location searched: registry `[..]`
 versions found: 0.0.1
+required by package `bar v0.0.1`
 "));
 }
 
@@ -436,9 +439,9 @@ fn yanks_in_lockfiles_are_ok() {
 
     assert_that(p.cargo("update"),
                 execs().with_status(101).with_stderr_contains("\
-[ERROR] no matching package named `bar` found (required by `foo`)
+error: no matching package named `bar` found
 location searched: registry [..]
-version required: *
+required by package `foo v0.0.1 ([..])`
 "));
 }
 

tests/resolve.rs

@@ -513,9 +513,9 @@ fn resolving_but_no_exists() {
     assert!(res.is_err());
 
     assert_eq!(res.err().unwrap().to_string(), "\
-no matching package named `foo` found (required by `root`)
-location searched: registry `http://example.com/`
-version required: ^1\
+no matching package named `foo` found\n\
+location searched: registry `http://example.com/`\n\
+required by package `root v1.0.0 (registry `http://example.com/`)`\
 ");
 }