Skip to content

Commit

Permalink
fix(resolver): Report unmatched versions, rather than saying no packa…
Browse files Browse the repository at this point in the history
…ge (#14897)

### What does this PR try to resolve?

Instead of saying no package found when there are hidden `Summary`s,
we'll instead say why the summary was hidden in the cases of
- Yanked packages
- Schema mismatch
- Offline packages?

The schema mismatch covers part of #10623. Whats remaining is when we
can't parse the `Summary` but can parse a subset (name, version, schema
version, optionally rust-version). That will be handled in a follow up.

### How should we test and review this PR?

This has a couple of risky areas
- Moving the filtering of `IndexSummary` variants from the Index to the
Registry Source. On inspection, there were other code paths but they
seemed innocuous to not filter, like asking for a hash of a summary
- Switching `PackageRegistry` to preserve the `IndexSummary` variant for
regular sources and overrides
- I did not switch patches to preserve `IndexSummary` as that was more
invasive and the benefits seemed more minor (normally people patch over
registry dependencies and not to them)

### Additional information
  • Loading branch information
Muscraft authored Dec 11, 2024
2 parents 298e403 + 0ed5c21 commit ad5b493
Show file tree
Hide file tree
Showing 9 changed files with 193 additions and 80 deletions.
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.
// 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
// 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

0 comments on commit ad5b493

Please sign in to comment.