Auto merge of #5691 - Eh2406:hyphen-underscore, r=alexcrichton

Make index lookup robust to _ vs -, but don't let the user get it wrong.

This does a brute force search thru combinations of hyphen and underscores to allow queries of crates to pass the wrong one.

This is a small first step of fixing #2775

Where is best to add test?

Author: bors

src/bin/cargo/main.rs

@@ -113,12 +113,11 @@ fn find_closest(config: &Config, cmd: &str) -> Option<String> {
     let cmds = list_commands(config);
     // Only consider candidates with a lev_distance of 3 or less so we don't
     // suggest out-of-the-blue options.
-    let mut filtered = cmds.iter()
-        .map(|&(ref c, _)| (lev_distance(c, cmd), c))
+    cmds.into_iter()
+        .map(|(c, _)| (lev_distance(&c, cmd), c))
         .filter(|&(d, _)| d < 4)
-        .collect::<Vec<_>>();
-    filtered.sort_by(|a, b| a.0.cmp(&b.0));
-    filtered.get(0).map(|slot| slot.1.clone())
+        .min_by_key(|a| a.0)
+        .map(|slot| slot.1)
 }
 
 fn execute_external_subcommand(config: &Config, cmd: &str, args: &[&str]) -> CliResult {

src/cargo/core/registry.rs

@@ -14,11 +14,11 @@ use sources::config::SourceConfigMap;
 /// See also `core::Source`.
 pub trait Registry {
     /// Attempt to find the packages that match a dependency request.
-    fn query(&mut self, dep: &Dependency, f: &mut FnMut(Summary)) -> CargoResult<()>;
+    fn query(&mut self, dep: &Dependency, f: &mut FnMut(Summary), fuzzy: bool) -> CargoResult<()>;
 
-    fn query_vec(&mut self, dep: &Dependency) -> CargoResult<Vec<Summary>> {
+    fn query_vec(&mut self, dep: &Dependency, fuzzy: bool) -> CargoResult<Vec<Summary>> {
         let mut ret = Vec::new();
-        self.query(dep, &mut |s| ret.push(s))?;
+        self.query(dep, &mut |s| ret.push(s), fuzzy)?;
         Ok(ret)
     }
 }
@@ -395,7 +395,7 @@ http://doc.crates.io/specifying-dependencies.html#overriding-dependencies
 }
 
 impl<'cfg> Registry for PackageRegistry<'cfg> {
-    fn query(&mut self, dep: &Dependency, f: &mut FnMut(Summary)) -> CargoResult<()> {
+    fn query(&mut self, dep: &Dependency, f: &mut FnMut(Summary), fuzzy: bool) -> CargoResult<()> {
         assert!(self.patches_locked);
         let (override_summary, n, to_warn) = {
             // Look for an override and get ready to query the real source.
@@ -476,15 +476,20 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
                         // already selected, then we skip this `summary`.
                         let locked = &self.locked;
                         let all_patches = &self.patches_available;
-                        return source.query(dep, &mut |summary| {
+                        let callback = &mut |summary: Summary| {
                             for patch in patches.iter() {
                                 let patch = patch.package_id().version();
                                 if summary.package_id().version() == patch {
                                     return;
                                 }
                             }
                             f(lock(locked, all_patches, summary))
-                        });
+                        };
+                        return if fuzzy {
+                            source.fuzzy_query(dep, callback)
+                        } else {
+                            source.query(dep, callback)
+                        };
                     }
 
                     // If we have an override summary then we query the source
@@ -496,10 +501,17 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
                         }
                         let mut n = 0;
                         let mut to_warn = None;
-                        source.query(dep, &mut |summary| {
-                            n += 1;
-                            to_warn = Some(summary);
-                        })?;
+                        {
+                            let callback = &mut |summary| {
+                                n += 1;
+                                to_warn = Some(summary);
+                            };
+                            if fuzzy {
+                                source.fuzzy_query(dep, callback)?;
+                            } else {
+                                source.query(dep, callback)?;
+                            }
+                        }
                         (override_summary, n, to_warn)
                     }
                 }

src/cargo/core/resolver/mod.rs

@@ -59,6 +59,7 @@ use core::PackageIdSpec;
 use core::{Dependency, PackageId, Registry, Summary};
 use util::config::Config;
 use util::errors::{CargoError, CargoResult};
+use util::lev_distance::lev_distance;
 use util::profile;
 
 use self::context::{Activations, Context};
@@ -230,7 +231,9 @@ fn activate_deps_loop(
         // to amortize the cost of the current time lookup.
         ticks += 1;
         if let Some(config) = config {
-            if config.shell().is_err_tty() && !printed && ticks % 1000 == 0
+            if config.shell().is_err_tty()
+                && !printed
+                && ticks % 1000 == 0
                 && start.elapsed() - deps_time > time_to_print
             {
                 printed = true;
@@ -857,12 +860,14 @@ fn activation_error(
         msg.push_str("\nversions that meet the requirements `");
         msg.push_str(&dep.version_req().to_string());
         msg.push_str("` are: ");
-        msg.push_str(&candidates
-            .iter()
-            .map(|v| v.summary.version())
-            .map(|v| v.to_string())
-            .collect::<Vec<_>>()
-            .join(", "));
+        msg.push_str(
+            &candidates
+                .iter()
+                .map(|v| v.summary.version())
+                .map(|v| v.to_string())
+                .collect::<Vec<_>>()
+                .join(", "),
+        );
 
         let mut conflicting_activations: Vec<_> = conflicting_activations.iter().collect();
         conflicting_activations.sort_unstable();
@@ -922,17 +927,16 @@ fn activation_error(
         return format_err!("{}", msg);
     }
 
-    // Once we're all the way down here, we're definitely lost in the
-    // weeds! We didn't actually find any candidates, so we need to
+    // We didn't actually find any candidates, so we need to
     // give an error message that nothing was found.
     //
-    // Note that we re-query the registry with a new dependency that
-    // allows any version so we can give some nicer error reporting
-    // which indicates a few versions that were actually found.
+    // Maybe the user mistyped the ver_req? Like `dep="2"` when `dep="0.2"`
+    // was meant. So we re-query the registry with `deb="*"` so we can
+    // list a few versions that were actually found.
     let all_req = semver::VersionReq::parse("*").unwrap();
     let mut new_dep = dep.clone();
     new_dep.set_version_req(all_req);
-    let mut candidates = match registry.query_vec(&new_dep) {
+    let mut candidates = match registry.query_vec(&new_dep, false) {
         Ok(candidates) => candidates,
         Err(e) => return e,
     };
@@ -977,12 +981,41 @@ fn activation_error(
 
         msg
     } else {
+        // Maybe the user mistyped the name? Like `dep-thing` when `Dep_Thing`
+        // was meant. So we try asking the registry for a `fuzzy` search for suggestions.
+        let mut candidates = Vec::new();
+        if let Err(e) = registry.query(&new_dep, &mut |s| candidates.push(s.name()), true) {
+            return e;
+        };
+        candidates.sort_unstable();
+        candidates.dedup();
+        let mut candidates: Vec<_> = candidates
+            .iter()
+            .map(|n| (lev_distance(&*new_dep.name(), &*n), n))
+            .filter(|&(d, _)| d < 4)
+            .collect();
+        candidates.sort_by_key(|o| o.0);
         let mut msg = format!(
             "no matching package named `{}` found\n\
              location searched: {}\n",
             dep.name(),
             dep.source_id()
         );
+        if !candidates.is_empty() {
+            let mut names = candidates
+                .iter()
+                .take(3)
+                .map(|c| c.1.as_str())
+                .collect::<Vec<_>>();
+
+            if candidates.len() > 3 {
+                names.push("...");
+            }
+
+            msg.push_str("did you mean: ");
+            msg.push_str(&names.join(", "));
+            msg.push_str("\n");
+        }
         msg.push_str("required by ");
         msg.push_str(&describe_path(&graph.path_to_top(parent.package_id())));
 

src/cargo/core/resolver/types.rs

@@ -51,7 +51,7 @@ impl<'a> RegistryQueryer<'a> {
                 summary: s,
                 replace: None,
             });
-        })?;
+        }, false)?;
         for candidate in ret.iter_mut() {
             let summary = &candidate.summary;
 
@@ -65,7 +65,7 @@ impl<'a> RegistryQueryer<'a> {
             };
             debug!("found an override for {} {}", dep.name(), dep.version_req());
 
-            let mut summaries = self.registry.query_vec(dep)?.into_iter();
+            let mut summaries = self.registry.query_vec(dep, false)?.into_iter();
             let s = summaries.next().ok_or_else(|| {
                 format_err!(
                     "no matching package for override `{}` found\n\

src/cargo/core/source/mod.rs

@@ -25,6 +25,12 @@ pub trait Source {
     /// Attempt to find the packages that match a dependency request.
     fn query(&mut self, dep: &Dependency, f: &mut FnMut(Summary)) -> CargoResult<()>;
 
+    /// Attempt to find the packages that are close to a dependency request.
+    /// Each source gets to define what `close` means for it.
+    /// path/git sources may return all dependencies that are at that uri.
+    /// where as an Index source may return dependencies that have the same canonicalization.
+    fn fuzzy_query(&mut self, dep: &Dependency, f: &mut FnMut(Summary)) -> CargoResult<()>;
+
     fn query_vec(&mut self, dep: &Dependency) -> CargoResult<Vec<Summary>> {
         let mut ret = Vec::new();
         self.query(dep, &mut |s| ret.push(s))?;
@@ -79,6 +85,11 @@ impl<'a, T: Source + ?Sized + 'a> Source for Box<T> {
         (**self).query(dep, f)
     }
 
+    /// Forwards to `Source::query`
+    fn fuzzy_query(&mut self, dep: &Dependency, f: &mut FnMut(Summary)) -> CargoResult<()> {
+        (**self).fuzzy_query(dep, f)
+    }
+
     /// Forwards to `Source::source_id`
     fn source_id(&self) -> &SourceId {
         (**self).source_id()

src/cargo/sources/directory.rs

@@ -54,6 +54,14 @@ impl<'cfg> Source for DirectorySource<'cfg> {
         Ok(())
     }
 
+    fn fuzzy_query(&mut self, _dep: &Dependency, f: &mut FnMut(Summary)) -> CargoResult<()> {
+        let packages = self.packages.values().map(|p| &p.0);
+        for summary in packages.map(|pkg| pkg.summary().clone()) {
+            f(summary);
+        }
+        Ok(())
+    }
+
     fn supports_checksums(&self) -> bool {
         true
     }

src/cargo/sources/git/source.rs

@@ -130,6 +130,13 @@ impl<'cfg> Source for GitSource<'cfg> {
         src.query(dep, f)
     }
 
+    fn fuzzy_query(&mut self, dep: &Dependency, f: &mut FnMut(Summary)) -> CargoResult<()> {
+        let src = self.path_source
+            .as_mut()
+            .expect("BUG: update() must be called before query()");
+        src.fuzzy_query(dep, f)
+    }
+
     fn supports_checksums(&self) -> bool {
         false
     }

src/cargo/sources/path.rs

@@ -508,6 +508,13 @@ impl<'cfg> Source for PathSource<'cfg> {
         Ok(())
     }
 
+    fn fuzzy_query(&mut self, _dep: &Dependency, f: &mut FnMut(Summary)) -> CargoResult<()> {
+        for s in self.packages.iter().map(|p| p.summary()) {
+            f(s.clone())
+        }
+        Ok(())
+    }
+
     fn supports_checksums(&self) -> bool {
         false
     }