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

fix(resolver): Report unmatched versions, rather than saying no package #14897

Merged
merged 11 commits into from
Dec 11, 2024
Merged
3 changes: 2 additions & 1 deletion crates/resolver-tests/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,8 @@ pub fn resolve_with_global_context_raw(
for summary in self.list.iter() {
let matched = match kind {
QueryKind::Exact => dep.matches(summary),
QueryKind::Alternatives => true,
QueryKind::AlternativeVersions => dep.matches(summary),
QueryKind::AlternativeNames => true,
QueryKind::Normalized => true,
};
if matched {
Expand Down
17 changes: 9 additions & 8 deletions src/cargo/core/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -674,9 +674,10 @@ impl<'gctx> Registry for PackageRegistry<'gctx> {
let patch = patches.remove(0);
match override_summary {
Some(override_summary) => {
let override_summary = override_summary.into_summary();
self.warn_bad_override(&override_summary, &patch)?;
f(IndexSummary::Candidate(self.lock(override_summary)));
self.warn_bad_override(override_summary.as_summary(), &patch)?;
let override_summary =
override_summary.map_summary(|summary| self.lock(summary));
f(override_summary);
}
None => f(IndexSummary::Candidate(patch)),
}
Expand Down Expand Up @@ -733,8 +734,8 @@ impl<'gctx> Registry for PackageRegistry<'gctx> {
return;
}
}
let summary = summary.into_summary();
f(IndexSummary::Candidate(lock(locked, all_patches, summary)))
let summary = summary.map_summary(|summary| lock(locked, all_patches, summary));
f(summary)
};
return source.query(dep, kind, callback);
}
Expand All @@ -760,11 +761,11 @@ impl<'gctx> Registry for PackageRegistry<'gctx> {
"found an override with a non-locked list"
)));
}
let override_summary = override_summary.into_summary();
if let Some(to_warn) = to_warn {
self.warn_bad_override(&override_summary, to_warn.as_summary())?;
self.warn_bad_override(override_summary.as_summary(), to_warn.as_summary())?;
}
f(IndexSummary::Candidate(self.lock(override_summary)));
let override_summary = override_summary.map_summary(|summary| self.lock(summary));
f(override_summary);
}
}

Expand Down
121 changes: 90 additions & 31 deletions src/cargo/core/resolver/errors.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
use std::fmt;
use std::fmt::Write as _;
use std::task::Poll;

use crate::core::{Dependency, PackageId, Registry, Summary};
use crate::sources::source::QueryKind;
use crate::sources::IndexSummary;
use crate::util::edit_distance::edit_distance;
use crate::util::{GlobalContext, OptVersionReq, VersionExt};
use anyhow::Error;
Expand Down Expand Up @@ -301,10 +303,23 @@ pub(super) fn activation_error(

msg
} else {
// Maybe something is wrong with the available versions
let mut version_candidates = loop {
match registry.query_vec(&new_dep, QueryKind::AlternativeVersions) {
Poll::Ready(Ok(candidates)) => break candidates,
Poll::Ready(Err(e)) => return to_resolve_err(e),
Poll::Pending => match registry.block_until_ready() {
Ok(()) => continue,
Err(e) => return to_resolve_err(e),
},
}
};
version_candidates.sort_unstable_by_key(|a| a.as_summary().version().clone());

// 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 candidates = loop {
match registry.query_vec(&new_dep, QueryKind::Alternatives) {
let name_candidates = loop {
match registry.query_vec(&new_dep, QueryKind::AlternativeNames) {
Poll::Ready(Ok(candidates)) => break candidates,
Poll::Ready(Err(e)) => return to_resolve_err(e),
Poll::Pending => match registry.block_until_ready() {
Expand All @@ -313,58 +328,102 @@ pub(super) fn activation_error(
},
}
};

let mut candidates: Vec<_> = candidates.into_iter().map(|s| s.into_summary()).collect();

candidates.sort_unstable_by_key(|a| a.name());
candidates.dedup_by(|a, b| a.name() == b.name());
let mut candidates: Vec<_> = candidates
let mut name_candidates: Vec<_> = name_candidates
.into_iter()
.map(|s| s.into_summary())
.collect();
name_candidates.sort_unstable_by_key(|a| a.name());
name_candidates.dedup_by(|a, b| a.name() == b.name());
let mut name_candidates: Vec<_> = name_candidates
.iter()
.filter_map(|n| Some((edit_distance(&*new_dep.package_name(), &*n.name(), 3)?, n)))
.collect();
candidates.sort_by_key(|o| o.0);
let mut msg: String;
if candidates.is_empty() {
msg = format!("no matching package named `{}` found\n", dep.package_name());
} else {
msg = format!(
"no matching package found\nsearched package name: `{}`\n",
name_candidates.sort_by_key(|o| o.0);

let mut msg = String::new();
if !version_candidates.is_empty() {
let _ = writeln!(
&mut msg,
"no matching versions for `{}` found",
dep.package_name()
);
let mut names = candidates
for candidate in version_candidates {
match candidate {
IndexSummary::Candidate(summary) => {
// HACK: If this was a real candidate, we wouldn't hit this case.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how this happens. It it possible to add a unit test to cover this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to hit this. This was originally an assert but I hit it because we switched everything to Candidate. That should be fixed but I figured a vague error is better for users than a crash in case I missed something or things change in the future.

// so it must be a patch which get normalized to being a candidate
let _ =
writeln!(&mut msg, " version {} is unavailable", summary.version());
}
IndexSummary::Yanked(summary) => {
let _ = writeln!(&mut msg, " version {} is yanked", summary.version());
}
IndexSummary::Offline(summary) => {
let _ = writeln!(&mut msg, " version {} is not cached", summary.version());
}
IndexSummary::Unsupported(summary, schema_version) => {
if let Some(rust_version) = summary.rust_version() {
// HACK: technically its unsupported and we shouldn't make assumptions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by technically its unsupported?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, the Unsupported branch means that we can parse the entry but the schema version was bumped. How we interpret the fields could have changed.

It'll be even worse in a follow up where I'll add "failed to parse but could get basic data".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thank you!

// about the entry but this is limited and for diagnostics purposes
let _ = writeln!(
&mut msg,
" version {} requires cargo {}",
summary.version(),
rust_version
);
} else {
let _ = writeln!(
&mut msg,
" version {} requires a Cargo version that supports index version {}",
summary.version(),
schema_version
);
}
}
}
}
} else if !name_candidates.is_empty() {
let _ = writeln!(&mut msg, "no matching package found",);
let _ = writeln!(&mut msg, "searched package name: `{}`", dep.package_name());
let mut names = name_candidates
.iter()
.take(3)
.map(|c| c.1.name().as_str())
.collect::<Vec<_>>();

if candidates.len() > 3 {
if name_candidates.len() > 3 {
names.push("...");
}
// Vertically align first suggestion with missing crate name
// so a typo jumps out at you.
msg.push_str("perhaps you meant: ");
msg.push_str(&names.iter().enumerate().fold(
String::default(),
|acc, (i, el)| match i {
let suggestions = names
.iter()
.enumerate()
.fold(String::default(), |acc, (i, el)| match i {
0 => acc + el,
i if names.len() - 1 == i && candidates.len() <= 3 => acc + " or " + el,
i if names.len() - 1 == i && name_candidates.len() <= 3 => acc + " or " + el,
_ => acc + ", " + el,
},
));
msg.push('\n');
});
let _ = writeln!(&mut msg, "perhaps you meant: {suggestions}");
} else {
let _ = writeln!(
&mut msg,
"no matching package named `{}` found",
dep.package_name()
);
}

let mut location_searched_msg = registry.describe_source(dep.source_id());
if location_searched_msg.is_empty() {
location_searched_msg = format!("{}", dep.source_id());
}

msg.push_str(&format!("location searched: {}\n", location_searched_msg));
msg.push_str("required by ");
msg.push_str(&describe_path_in_context(
resolver_ctx,
&parent.package_id(),
));
let _ = writeln!(&mut msg, "location searched: {}", location_searched_msg);
let _ = write!(
&mut msg,
"required by {}",
describe_path_in_context(resolver_ctx, &parent.package_id()),
);

msg
};
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/sources/directory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,8 @@ impl<'gctx> Source for DirectorySource<'gctx> {
}
let packages = self.packages.values().map(|p| &p.0);
let matches = packages.filter(|pkg| match kind {
QueryKind::Exact => dep.matches(pkg.summary()),
QueryKind::Alternatives => true,
QueryKind::Exact | QueryKind::AlternativeVersions => dep.matches(pkg.summary()),
QueryKind::AlternativeNames => true,
QueryKind::Normalized => dep.matches(pkg.summary()),
});
for summary in matches.map(|pkg| pkg.summary().clone()) {
Expand Down
8 changes: 4 additions & 4 deletions src/cargo/sources/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,8 @@ impl<'gctx> Source for PathSource<'gctx> {
self.load()?;
if let Some(s) = self.package.as_ref().map(|p| p.summary()) {
let matched = match kind {
QueryKind::Exact => dep.matches(s),
QueryKind::Alternatives => true,
QueryKind::Exact | QueryKind::AlternativeVersions => dep.matches(s),
QueryKind::AlternativeNames => true,
QueryKind::Normalized => dep.matches(s),
};
if matched {
Expand Down Expand Up @@ -332,8 +332,8 @@ impl<'gctx> Source for RecursivePathSource<'gctx> {
.map(|p| p.summary())
{
let matched = match kind {
QueryKind::Exact => dep.matches(s),
QueryKind::Alternatives => true,
QueryKind::Exact | QueryKind::AlternativeVersions => dep.matches(s),
QueryKind::AlternativeNames => true,
QueryKind::Normalized => dep.matches(s),
};
if matched {
Expand Down
18 changes: 2 additions & 16 deletions src/cargo/sources/registry/index/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ use std::collections::HashMap;
use std::path::Path;
use std::str;
use std::task::{ready, Poll};
use tracing::{debug, info};
use tracing::info;

mod cache;
use self::cache::CacheManager;
Expand Down Expand Up @@ -370,21 +370,7 @@ impl<'gctx> RegistryIndex<'gctx> {
.filter_map(move |(k, v)| if req.matches(k) { Some(v) } else { None })
.filter_map(move |maybe| {
match maybe.parse(raw_data, source_id, bindeps) {
Ok(sum @ IndexSummary::Candidate(_) | sum @ IndexSummary::Yanked(_)) => {
Some(sum)
}
Ok(IndexSummary::Unsupported(summary, v)) => {
debug!(
"unsupported schema version {} ({} {})",
v,
summary.name(),
summary.version()
);
None
}
Ok(IndexSummary::Offline(_)) => {
unreachable!("We do not check for off-line until later")
}
Ok(sum) => Some(sum),
Err(e) => {
info!("failed to parse `{}` registry package: {}", name, e);
None
Expand Down
41 changes: 29 additions & 12 deletions src/cargo/sources/registry/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -779,7 +779,9 @@ impl<'gctx> Source for RegistrySource<'gctx> {
ready!(self
.index
.query_inner(dep.package_name(), &req, &mut *self.ops, &mut |s| {
if dep.matches(s.as_summary()) {
if matches!(s, IndexSummary::Candidate(_) | IndexSummary::Yanked(_))
&& dep.matches(s.as_summary())
{
// We are looking for a package from a lock file so we do not care about yank
callback(s)
}
Expand All @@ -797,14 +799,14 @@ impl<'gctx> Source for RegistrySource<'gctx> {
.index
.query_inner(dep.package_name(), &req, &mut *self.ops, &mut |s| {
let matched = match kind {
QueryKind::Exact => {
QueryKind::Exact | QueryKind::AlternativeVersions => {
if req.is_precise() && self.gctx.cli_unstable().unstable_options {
dep.matches_prerelease(s.as_summary())
} else {
dep.matches(s.as_summary())
}
}
QueryKind::Alternatives => true,
QueryKind::AlternativeNames => true,
QueryKind::Normalized => true,
};
if !matched {
Expand All @@ -813,13 +815,28 @@ impl<'gctx> Source for RegistrySource<'gctx> {
// Next filter out all yanked packages. Some yanked packages may
// leak through if they're in a whitelist (aka if they were
// previously in `Cargo.lock`
if !s.is_yanked() {
callback(s);
} else if self.yanked_whitelist.contains(&s.package_id()) {
callback(s);
} else if req.is_precise() {
precise_yanked_in_use = true;
callback(s);
match s {
s @ _ if kind == QueryKind::AlternativeVersions => callback(s),
s @ IndexSummary::Candidate(_) => callback(s),
s @ IndexSummary::Yanked(_) => {
if self.yanked_whitelist.contains(&s.package_id()) {
callback(s);
} else if req.is_precise() {
precise_yanked_in_use = true;
callback(s);
}
}
IndexSummary::Unsupported(summary, v) => {
tracing::debug!(
"unsupported schema version {} ({} {})",
v,
summary.name(),
summary.version()
);
}
IndexSummary::Offline(summary) => {
tracing::debug!("offline ({} {})", summary.name(), summary.version());
}
}
}))?;
if precise_yanked_in_use {
Expand All @@ -839,7 +856,7 @@ impl<'gctx> Source for RegistrySource<'gctx> {
return Poll::Ready(Ok(()));
}
let mut any_pending = false;
if kind == QueryKind::Alternatives || kind == QueryKind::Normalized {
if kind == QueryKind::AlternativeNames || kind == QueryKind::Normalized {
// Attempt to handle misspellings by searching for a chain of related
// names to the original name. The resolver will later
// reject any candidates that have the wrong name, and with this it'll
Expand All @@ -859,7 +876,7 @@ impl<'gctx> Source for RegistrySource<'gctx> {
.query_inner(name_permutation, &req, &mut *self.ops, &mut |s| {
if !s.is_yanked() {
f(s);
} else if kind == QueryKind::Alternatives {
} else if kind == QueryKind::AlternativeNames {
f(s);
}
})?
Expand Down
9 changes: 8 additions & 1 deletion src/cargo/sources/source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,9 +183,16 @@ pub enum QueryKind {
/// Each source gets to define what `close` means for it.
///
/// Path/Git sources may return all dependencies that are at that URI,
/// whereas an `Registry` source may return dependencies that are yanked or invalid.
AlternativeVersions,
/// A query for packages close to the given dependency requirement.
///
/// Each source gets to define what `close` means for it.
///
/// Path/Git sources may return all dependencies that are at that URI,
/// whereas an `Registry` source may return dependencies that have the same
/// canonicalization.
Alternatives,
AlternativeNames,
/// Match a dependency in all ways and will normalize the package name.
/// Each source defines what normalizing means.
Normalized,
Expand Down
Loading
Loading