Skip to content

Commit ad5b493

Browse files
MuscraftMuscraft
authored
fix(resolver): Report unmatched versions, rather than saying no package (#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
2 parents 298e403 + 0ed5c21 commit ad5b493

File tree

9 files changed

+193
-80
lines changed

9 files changed

+193
-80
lines changed

‎crates/resolver-tests/src/lib.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,8 @@ pub fn resolve_with_global_context_raw(
143143
for summary in self.list.iter() {
144144
let matched = match kind {
145145
QueryKind::Exact => dep.matches(summary),
146-
QueryKind::Alternatives => true,
146+
QueryKind::AlternativeVersions => dep.matches(summary),
147+
QueryKind::AlternativeNames => true,
147148
QueryKind::Normalized => true,
148149
};
149150
if matched {

‎src/cargo/core/registry.rs

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -674,9 +674,10 @@ impl<'gctx> Registry for PackageRegistry<'gctx> {
674674
let patch = patches.remove(0);
675675
match override_summary {
676676
Some(override_summary) => {
677-
let override_summary = override_summary.into_summary();
678-
self.warn_bad_override(&override_summary, &patch)?;
679-
f(IndexSummary::Candidate(self.lock(override_summary)));
677+
self.warn_bad_override(override_summary.as_summary(), &patch)?;
678+
let override_summary =
679+
override_summary.map_summary(|summary| self.lock(summary));
680+
f(override_summary);
680681
}
681682
None => f(IndexSummary::Candidate(patch)),
682683
}
@@ -733,8 +734,8 @@ impl<'gctx> Registry for PackageRegistry<'gctx> {
733734
return;
734735
}
735736
}
736-
let summary = summary.into_summary();
737-
f(IndexSummary::Candidate(lock(locked, all_patches, summary)))
737+
let summary = summary.map_summary(|summary| lock(locked, all_patches, summary));
738+
f(summary)
738739
};
739740
return source.query(dep, kind, callback);
740741
}
@@ -760,11 +761,11 @@ impl<'gctx> Registry for PackageRegistry<'gctx> {
760761
"found an override with a non-locked list"
761762
)));
762763
}
763-
let override_summary = override_summary.into_summary();
764764
if let Some(to_warn) = to_warn {
765-
self.warn_bad_override(&override_summary, to_warn.as_summary())?;
765+
self.warn_bad_override(override_summary.as_summary(), to_warn.as_summary())?;
766766
}
767-
f(IndexSummary::Candidate(self.lock(override_summary)));
767+
let override_summary = override_summary.map_summary(|summary| self.lock(summary));
768+
f(override_summary);
768769
}
769770
}
770771

‎src/cargo/core/resolver/errors.rs

Lines changed: 90 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
use std::fmt;
2+
use std::fmt::Write as _;
23
use std::task::Poll;
34

45
use crate::core::{Dependency, PackageId, Registry, Summary};
56
use crate::sources::source::QueryKind;
7+
use crate::sources::IndexSummary;
68
use crate::util::edit_distance::edit_distance;
79
use crate::util::{GlobalContext, OptVersionReq, VersionExt};
810
use anyhow::Error;
@@ -301,10 +303,23 @@ pub(super) fn activation_error(
301303

302304
msg
303305
} else {
306+
// Maybe something is wrong with the available versions
307+
let mut version_candidates = loop {
308+
match registry.query_vec(&new_dep, QueryKind::AlternativeVersions) {
309+
Poll::Ready(Ok(candidates)) => break candidates,
310+
Poll::Ready(Err(e)) => return to_resolve_err(e),
311+
Poll::Pending => match registry.block_until_ready() {
312+
Ok(()) => continue,
313+
Err(e) => return to_resolve_err(e),
314+
},
315+
}
316+
};
317+
version_candidates.sort_unstable_by_key(|a| a.as_summary().version().clone());
318+
304319
// Maybe the user mistyped the name? Like `dep-thing` when `Dep_Thing`
305320
// was meant. So we try asking the registry for a `fuzzy` search for suggestions.
306-
let candidates = loop {
307-
match registry.query_vec(&new_dep, QueryKind::Alternatives) {
321+
let name_candidates = loop {
322+
match registry.query_vec(&new_dep, QueryKind::AlternativeNames) {
308323
Poll::Ready(Ok(candidates)) => break candidates,
309324
Poll::Ready(Err(e)) => return to_resolve_err(e),
310325
Poll::Pending => match registry.block_until_ready() {
@@ -313,58 +328,102 @@ pub(super) fn activation_error(
313328
},
314329
}
315330
};
316-
317-
let mut candidates: Vec<_> = candidates.into_iter().map(|s| s.into_summary()).collect();
318-
319-
candidates.sort_unstable_by_key(|a| a.name());
320-
candidates.dedup_by(|a, b| a.name() == b.name());
321-
let mut candidates: Vec<_> = candidates
331+
let mut name_candidates: Vec<_> = name_candidates
332+
.into_iter()
333+
.map(|s| s.into_summary())
334+
.collect();
335+
name_candidates.sort_unstable_by_key(|a| a.name());
336+
name_candidates.dedup_by(|a, b| a.name() == b.name());
337+
let mut name_candidates: Vec<_> = name_candidates
322338
.iter()
323339
.filter_map(|n| Some((edit_distance(&*new_dep.package_name(), &*n.name(), 3)?, n)))
324340
.collect();
325-
candidates.sort_by_key(|o| o.0);
326-
let mut msg: String;
327-
if candidates.is_empty() {
328-
msg = format!("no matching package named `{}` found\n", dep.package_name());
329-
} else {
330-
msg = format!(
331-
"no matching package found\nsearched package name: `{}`\n",
341+
name_candidates.sort_by_key(|o| o.0);
342+
343+
let mut msg = String::new();
344+
if !version_candidates.is_empty() {
345+
let _ = writeln!(
346+
&mut msg,
347+
"no matching versions for `{}` found",
332348
dep.package_name()
333349
);
334-
let mut names = candidates
350+
for candidate in version_candidates {
351+
match candidate {
352+
IndexSummary::Candidate(summary) => {
353+
// HACK: If this was a real candidate, we wouldn't hit this case.
354+
// so it must be a patch which get normalized to being a candidate
355+
let _ =
356+
writeln!(&mut msg, " version {} is unavailable", summary.version());
357+
}
358+
IndexSummary::Yanked(summary) => {
359+
let _ = writeln!(&mut msg, " version {} is yanked", summary.version());
360+
}
361+
IndexSummary::Offline(summary) => {
362+
let _ = writeln!(&mut msg, " version {} is not cached", summary.version());
363+
}
364+
IndexSummary::Unsupported(summary, schema_version) => {
365+
if let Some(rust_version) = summary.rust_version() {
366+
// HACK: technically its unsupported and we shouldn't make assumptions
367+
// about the entry but this is limited and for diagnostics purposes
368+
let _ = writeln!(
369+
&mut msg,
370+
" version {} requires cargo {}",
371+
summary.version(),
372+
rust_version
373+
);
374+
} else {
375+
let _ = writeln!(
376+
&mut msg,
377+
" version {} requires a Cargo version that supports index version {}",
378+
summary.version(),
379+
schema_version
380+
);
381+
}
382+
}
383+
}
384+
}
385+
} else if !name_candidates.is_empty() {
386+
let _ = writeln!(&mut msg, "no matching package found",);
387+
let _ = writeln!(&mut msg, "searched package name: `{}`", dep.package_name());
388+
let mut names = name_candidates
335389
.iter()
336390
.take(3)
337391
.map(|c| c.1.name().as_str())
338392
.collect::<Vec<_>>();
339393

340-
if candidates.len() > 3 {
394+
if name_candidates.len() > 3 {
341395
names.push("...");
342396
}
343397
// Vertically align first suggestion with missing crate name
344398
// so a typo jumps out at you.
345-
msg.push_str("perhaps you meant: ");
346-
msg.push_str(&names.iter().enumerate().fold(
347-
String::default(),
348-
|acc, (i, el)| match i {
399+
let suggestions = names
400+
.iter()
401+
.enumerate()
402+
.fold(String::default(), |acc, (i, el)| match i {
349403
0 => acc + el,
350-
i if names.len() - 1 == i && candidates.len() <= 3 => acc + " or " + el,
404+
i if names.len() - 1 == i && name_candidates.len() <= 3 => acc + " or " + el,
351405
_ => acc + ", " + el,
352-
},
353-
));
354-
msg.push('\n');
406+
});
407+
let _ = writeln!(&mut msg, "perhaps you meant: {suggestions}");
408+
} else {
409+
let _ = writeln!(
410+
&mut msg,
411+
"no matching package named `{}` found",
412+
dep.package_name()
413+
);
355414
}
356415

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

362-
msg.push_str(&format!("location searched: {}\n", location_searched_msg));
363-
msg.push_str("required by ");
364-
msg.push_str(&describe_path_in_context(
365-
resolver_ctx,
366-
&parent.package_id(),
367-
));
421+
let _ = writeln!(&mut msg, "location searched: {}", location_searched_msg);
422+
let _ = write!(
423+
&mut msg,
424+
"required by {}",
425+
describe_path_in_context(resolver_ctx, &parent.package_id()),
426+
);
368427

369428
msg
370429
};

‎src/cargo/sources/directory.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,8 +108,8 @@ impl<'gctx> Source for DirectorySource<'gctx> {
108108
}
109109
let packages = self.packages.values().map(|p| &p.0);
110110
let matches = packages.filter(|pkg| match kind {
111-
QueryKind::Exact => dep.matches(pkg.summary()),
112-
QueryKind::Alternatives => true,
111+
QueryKind::Exact | QueryKind::AlternativeVersions => dep.matches(pkg.summary()),
112+
QueryKind::AlternativeNames => true,
113113
QueryKind::Normalized => dep.matches(pkg.summary()),
114114
});
115115
for summary in matches.map(|pkg| pkg.summary().clone()) {

‎src/cargo/sources/path.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -145,8 +145,8 @@ impl<'gctx> Source for PathSource<'gctx> {
145145
self.load()?;
146146
if let Some(s) = self.package.as_ref().map(|p| p.summary()) {
147147
let matched = match kind {
148-
QueryKind::Exact => dep.matches(s),
149-
QueryKind::Alternatives => true,
148+
QueryKind::Exact | QueryKind::AlternativeVersions => dep.matches(s),
149+
QueryKind::AlternativeNames => true,
150150
QueryKind::Normalized => dep.matches(s),
151151
};
152152
if matched {
@@ -332,8 +332,8 @@ impl<'gctx> Source for RecursivePathSource<'gctx> {
332332
.map(|p| p.summary())
333333
{
334334
let matched = match kind {
335-
QueryKind::Exact => dep.matches(s),
336-
QueryKind::Alternatives => true,
335+
QueryKind::Exact | QueryKind::AlternativeVersions => dep.matches(s),
336+
QueryKind::AlternativeNames => true,
337337
QueryKind::Normalized => dep.matches(s),
338338
};
339339
if matched {

‎src/cargo/sources/registry/index/mod.rs

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ use std::collections::HashMap;
3737
use std::path::Path;
3838
use std::str;
3939
use std::task::{ready, Poll};
40-
use tracing::{debug, info};
40+
use tracing::info;
4141

4242
mod cache;
4343
use self::cache::CacheManager;
@@ -370,21 +370,7 @@ impl<'gctx> RegistryIndex<'gctx> {
370370
.filter_map(move |(k, v)| if req.matches(k) { Some(v) } else { None })
371371
.filter_map(move |maybe| {
372372
match maybe.parse(raw_data, source_id, bindeps) {
373-
Ok(sum @ IndexSummary::Candidate(_) | sum @ IndexSummary::Yanked(_)) => {
374-
Some(sum)
375-
}
376-
Ok(IndexSummary::Unsupported(summary, v)) => {
377-
debug!(
378-
"unsupported schema version {} ({} {})",
379-
v,
380-
summary.name(),
381-
summary.version()
382-
);
383-
None
384-
}
385-
Ok(IndexSummary::Offline(_)) => {
386-
unreachable!("We do not check for off-line until later")
387-
}
373+
Ok(sum) => Some(sum),
388374
Err(e) => {
389375
info!("failed to parse `{}` registry package: {}", name, e);
390376
None

‎src/cargo/sources/registry/mod.rs

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -779,7 +779,9 @@ impl<'gctx> Source for RegistrySource<'gctx> {
779779
ready!(self
780780
.index
781781
.query_inner(dep.package_name(), &req, &mut *self.ops, &mut |s| {
782-
if dep.matches(s.as_summary()) {
782+
if matches!(s, IndexSummary::Candidate(_) | IndexSummary::Yanked(_))
783+
&& dep.matches(s.as_summary())
784+
{
783785
// We are looking for a package from a lock file so we do not care about yank
784786
callback(s)
785787
}
@@ -797,14 +799,14 @@ impl<'gctx> Source for RegistrySource<'gctx> {
797799
.index
798800
.query_inner(dep.package_name(), &req, &mut *self.ops, &mut |s| {
799801
let matched = match kind {
800-
QueryKind::Exact => {
802+
QueryKind::Exact | QueryKind::AlternativeVersions => {
801803
if req.is_precise() && self.gctx.cli_unstable().unstable_options {
802804
dep.matches_prerelease(s.as_summary())
803805
} else {
804806
dep.matches(s.as_summary())
805807
}
806808
}
807-
QueryKind::Alternatives => true,
809+
QueryKind::AlternativeNames => true,
808810
QueryKind::Normalized => true,
809811
};
810812
if !matched {
@@ -813,13 +815,28 @@ impl<'gctx> Source for RegistrySource<'gctx> {
813815
// Next filter out all yanked packages. Some yanked packages may
814816
// leak through if they're in a whitelist (aka if they were
815817
// previously in `Cargo.lock`
816-
if !s.is_yanked() {
817-
callback(s);
818-
} else if self.yanked_whitelist.contains(&s.package_id()) {
819-
callback(s);
820-
} else if req.is_precise() {
821-
precise_yanked_in_use = true;
822-
callback(s);
818+
match s {
819+
s @ _ if kind == QueryKind::AlternativeVersions => callback(s),
820+
s @ IndexSummary::Candidate(_) => callback(s),
821+
s @ IndexSummary::Yanked(_) => {
822+
if self.yanked_whitelist.contains(&s.package_id()) {
823+
callback(s);
824+
} else if req.is_precise() {
825+
precise_yanked_in_use = true;
826+
callback(s);
827+
}
828+
}
829+
IndexSummary::Unsupported(summary, v) => {
830+
tracing::debug!(
831+
"unsupported schema version {} ({} {})",
832+
v,
833+
summary.name(),
834+
summary.version()
835+
);
836+
}
837+
IndexSummary::Offline(summary) => {
838+
tracing::debug!("offline ({} {})", summary.name(), summary.version());
839+
}
823840
}
824841
}))?;
825842
if precise_yanked_in_use {
@@ -839,7 +856,7 @@ impl<'gctx> Source for RegistrySource<'gctx> {
839856
return Poll::Ready(Ok(()));
840857
}
841858
let mut any_pending = false;
842-
if kind == QueryKind::Alternatives || kind == QueryKind::Normalized {
859+
if kind == QueryKind::AlternativeNames || kind == QueryKind::Normalized {
843860
// Attempt to handle misspellings by searching for a chain of related
844861
// names to the original name. The resolver will later
845862
// reject any candidates that have the wrong name, and with this it'll
@@ -859,7 +876,7 @@ impl<'gctx> Source for RegistrySource<'gctx> {
859876
.query_inner(name_permutation, &req, &mut *self.ops, &mut |s| {
860877
if !s.is_yanked() {
861878
f(s);
862-
} else if kind == QueryKind::Alternatives {
879+
} else if kind == QueryKind::AlternativeNames {
863880
f(s);
864881
}
865882
})?

‎src/cargo/sources/source.rs

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,9 +183,16 @@ pub enum QueryKind {
183183
/// Each source gets to define what `close` means for it.
184184
///
185185
/// Path/Git sources may return all dependencies that are at that URI,
186+
/// whereas an `Registry` source may return dependencies that are yanked or invalid.
187+
AlternativeVersions,
188+
/// A query for packages close to the given dependency requirement.
189+
///
190+
/// Each source gets to define what `close` means for it.
191+
///
192+
/// Path/Git sources may return all dependencies that are at that URI,
186193
/// whereas an `Registry` source may return dependencies that have the same
187194
/// canonicalization.
188-
Alternatives,
195+
AlternativeNames,
189196
/// Match a dependency in all ways and will normalize the package name.
190197
/// Each source defines what normalizing means.
191198
Normalized,

0 commit comments

Comments
 (0)