Auto merge of #6112 - alexcrichton:better-errors, r=Eh2406

Try to improve "version not found" error

This commit tweaks the wording for when versions aren't found in a
source, notably including more location information other than the
source id but rather also taking into account replaced sources and such.
It's hoped that this gives more information to make it a bit more clear
what's going on.

Closes #6111

Author: bors

src/cargo/core/registry.rs

@@ -21,6 +21,9 @@ pub trait Registry {
         self.query(dep, &mut |s| ret.push(s), fuzzy)?;
         Ok(ret)
     }
+
+    fn describe_source(&self, source: &SourceId) -> String;
+    fn is_replaced(&self, source: &SourceId) -> bool;
 }
 
 /// This structure represents a registry of known packages. It internally
@@ -528,6 +531,20 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
         f(self.lock(override_summary));
         Ok(())
     }
+
+    fn describe_source(&self, id: &SourceId) -> String {
+        match self.sources.get(id) {
+            Some(src) => src.describe(),
+            None => id.to_string(),
+        }
+    }
+
+    fn is_replaced(&self, id: &SourceId) -> bool {
+        match self.sources.get(id) {
+            Some(src) => src.is_replaced(),
+            None => false,
+        }
+    }
 }
 
 fn lock(locked: &LockedMap, patches: &HashMap<Url, Vec<PackageId>>, summary: Summary) -> Summary {

src/cargo/core/resolver/mod.rs

@@ -933,13 +933,13 @@ fn activation_error(
         };
 
         let mut msg = format!(
-            "no matching version `{}` found for package `{}`\n\
-             location searched: {}\n\
-             versions found: {}\n",
-            dep.version_req(),
+            "failed to select a version for the requirement `{} = \"{}\"`\n  \
+             candidate versions found which didn't match: {}\n  \
+             location searched: {}\n",
             dep.package_name(),
-            dep.source_id(),
-            versions
+            dep.version_req(),
+            versions,
+            registry.describe_source(dep.source_id()),
         );
         msg.push_str("required by ");
         msg.push_str(&describe_path(&graph.path_to_top(parent.package_id())));
@@ -954,6 +954,10 @@ fn activation_error(
             );
         }
 
+        if registry.is_replaced(dep.source_id()) {
+            msg.push_str("\nperhaps a crate was updated and forgotten to be re-vendored?");
+        }
+
         msg
     } else {
         // Maybe the user mistyped the name? Like `dep-thing` when `Dep_Thing`

src/cargo/core/source/mod.rs

@@ -75,6 +75,15 @@ pub trait Source {
     fn verify(&self, _pkg: &PackageId) -> CargoResult<()> {
         Ok(())
     }
+
+    /// Describes this source in a human readable fashion, used for display in
+    /// resolver error messages currently.
+    fn describe(&self) -> String;
+
+    /// Returns whether a source is being replaced by another here
+    fn is_replaced(&self) -> bool {
+        false
+    }
 }
 
 pub enum MaybePackage {
@@ -139,6 +148,14 @@ impl<'a, T: Source + ?Sized + 'a> Source for Box<T> {
     fn verify(&self, pkg: &PackageId) -> CargoResult<()> {
         (**self).verify(pkg)
     }
+
+    fn describe(&self) -> String {
+        (**self).describe()
+    }
+
+    fn is_replaced(&self) -> bool {
+        (**self).is_replaced()
+    }
 }
 
 impl<'a, T: Source + ?Sized + 'a> Source for &'a mut T {
@@ -185,6 +202,14 @@ impl<'a, T: Source + ?Sized + 'a> Source for &'a mut T {
     fn verify(&self, pkg: &PackageId) -> CargoResult<()> {
         (**self).verify(pkg)
     }
+
+    fn describe(&self) -> String {
+        (**self).describe()
+    }
+
+    fn is_replaced(&self) -> bool {
+        (**self).is_replaced()
+    }
 }
 
 /// A `HashMap` of `SourceId` -> `Box<Source>`

src/cargo/sources/directory.rs

@@ -212,4 +212,8 @@ impl<'cfg> Source for DirectorySource<'cfg> {
 
         Ok(())
     }
+
+    fn describe(&self) -> String {
+        format!("directory source `{}`", self.root.display())
+    }
 }

src/cargo/sources/git/source.rs

@@ -229,6 +229,10 @@ impl<'cfg> Source for GitSource<'cfg> {
     fn fingerprint(&self, _pkg: &Package) -> CargoResult<String> {
         Ok(self.rev.as_ref().unwrap().to_string())
     }
+
+    fn describe(&self) -> String {
+        format!("git repository {}", self.source_id)
+    }
 }
 
 #[cfg(test)]

src/cargo/sources/path.rs

@@ -558,4 +558,11 @@ impl<'cfg> Source for PathSource<'cfg> {
         let (max, max_path) = self.last_modified_file(pkg)?;
         Ok(format!("{} ({})", max, max_path.display()))
     }
+
+    fn describe(&self) -> String {
+        match self.source_id.url().to_file_path() {
+            Ok(path) => path.display().to_string(),
+            Err(_) => self.source_id.to_string(),
+        }
+    }
 }

src/cargo/sources/registry/mod.rs

@@ -585,4 +585,8 @@ impl<'cfg> Source for RegistrySource<'cfg> {
     fn fingerprint(&self, pkg: &Package) -> CargoResult<String> {
         Ok(pkg.package_id().version().to_string())
     }
+
+    fn describe(&self) -> String {
+        self.source_id.display_registry()
+    }
 }

src/cargo/sources/replaced.rs

@@ -103,4 +103,12 @@ impl<'cfg> Source for ReplacedSource<'cfg> {
         let id = id.with_source_id(&self.replace_with);
         self.inner.verify(&id)
     }
+
+    fn describe(&self) -> String {
+        format!("{} (which is replacing {})", self.inner.describe(), self.to_replace)
+    }
+
+    fn is_replaced(&self) -> bool {
+        true
+    }
 }

tests/testsuite/directory.rs

@@ -683,3 +683,45 @@ fn workspace_different_locations() {
 ",
         ).run();
 }
+
+#[test]
+fn version_missing() {
+    setup();
+
+    VendorPackage::new("foo")
+        .file("src/lib.rs", "pub fn foo() {}")
+        .build();
+
+    VendorPackage::new("bar")
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "bar"
+                version = "0.1.0"
+                authors = []
+
+                [dependencies]
+                foo = "2"
+            "#,
+        )
+        .file("src/main.rs", "fn main() {}")
+        .build();
+
+    cargo_process("install bar")
+        .with_stderr(
+            "\
+[INSTALLING] bar v0.1.0
+error: failed to compile [..]
+
+Caused by:
+  failed to select a version for the requirement `foo = \"^2\"`
+  candidate versions found which didn't match: 0.0.1
+  location searched: directory source `[..] (which is replacing registry `[..]`)
+required by package `bar v0.1.0`
+perhaps a crate was updated and forgotten to be re-vendored?
+",
+        )
+        .with_status(101)
+        .run();
+}

tests/testsuite/registry.rs

@@ -219,9 +219,9 @@ fn wrong_version() {
         .with_status(101)
         .with_stderr_contains(
             "\
-error: no matching version `>= 1.0.0` found for package `foo`
-location searched: registry [..]
-versions found: 0.0.2, 0.0.1
+error: failed to select a version for the requirement `foo = \">= 1.0.0\"`
+  candidate versions found which didn't match: 0.0.2, 0.0.1
+  location searched: `[..]` index (which is replacing registry `[..]`)
 required by package `foo v0.0.1 ([..])`
 ",
         ).run();
@@ -233,9 +233,9 @@ required by package `foo v0.0.1 ([..])`
         .with_status(101)
         .with_stderr_contains(
             "\
-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, ...
+error: failed to select a version for the requirement `foo = \">= 1.0.0\"`
+  candidate versions found which didn't match: 0.0.4, 0.0.3, 0.0.2, ...
+  location searched: `[..]` index (which is replacing registry `[..]`)
 required by package `foo v0.0.1 ([..])`
 ",
         ).run();
@@ -520,10 +520,11 @@ fn relying_on_a_yank_is_bad() {
         .with_status(101)
         .with_stderr_contains(
             "\
-error: no matching version `= 0.0.2` found for package `baz`
-location searched: registry `[..]`
-versions found: 0.0.1
+error: failed to select a version for the requirement `baz = \"= 0.0.2\"`
+  candidate versions found which didn't match: 0.0.1
+  location searched: `[..]` index (which is replacing registry `[..]`)
 required by package `bar v0.0.1`
+    ... which is depended on by `foo [..]`
 ",
         ).run();
 }

tests/testsuite/resolve.rs

@@ -17,12 +17,12 @@ use proptest::prelude::*;
 
 proptest! {
     #![proptest_config(ProptestConfig {
-        cases:
-            if env::var("CI").is_ok() {
-                256 // we have a lot of builds in CI so one or another of them will find problems
-            } else {
-                1024 // but locally try and find it in the one build
-            },
+        // Note that this is a little low in terms of cases we'd like to test,
+        // but this number affects how long this function takes. It can be
+        // increased locally to execute more tests and try to find more bugs,
+        // but for now it's semi-low to run in a small-ish amount of time on CI
+        // and locally.
+        cases: 256,
         max_shrink_iters:
             if env::var("CI").is_ok() {
                 // This attempts to make sure that CI will fail fast,

tests/testsuite/support/resolver.rs

@@ -46,6 +46,14 @@ pub fn resolve_with_config(
             }
             Ok(())
         }
+
+        fn describe_source(&self, _src: &SourceId) -> String {
+            String::new()
+        }
+
+        fn is_replaced(&self, _src: &SourceId) -> bool {
+            false
+        }
     }
     let mut registry = MyRegistry(registry);
     let summary = Summary::new(