Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from
Jul 16, 2018
9 changes: 4 additions & 5 deletions src/bin/cargo/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
32 changes: 22 additions & 10 deletions src/cargo/core/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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)
}
}
Expand Down
59 changes: 46 additions & 13 deletions src/cargo/core/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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,
};
Expand Down Expand Up @@ -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())));

Expand Down
4 changes: 2 additions & 2 deletions src/cargo/core/resolver/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ impl<'a> RegistryQueryer<'a> {
summary: s,
replace: None,
});
})?;
}, false)?;
for candidate in ret.iter_mut() {
let summary = &candidate.summary;

Expand All @@ -66,7 +66,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\
Expand Down
11 changes: 11 additions & 0 deletions src/cargo/core/source/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))?;
Expand Down Expand Up @@ -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()
Expand Down
8 changes: 8 additions & 0 deletions src/cargo/sources/directory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
7 changes: 7 additions & 0 deletions src/cargo/sources/git/source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
7 changes: 7 additions & 0 deletions src/cargo/sources/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
Loading