Auto merge of #12749 - Eh2406:move_up, r=epage

move up looking at index summary enum

This builds on the work from #12643 and the discussion of the overall approach is at https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/better.20error.20messages.20for.20filtered.20versions.2E

### What does this PR try to resolve?

This uses the enum added in #12643 to delay making decisions about yanked and `–– precise` versions until higher in the stack.

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

This is an internal re-factoring and all the tests still pass. BUT, this is also tricky interleaved code where small changes may have important impacts to semantics.

### Additional information

I will try and call out potentially breaking changes as inline comments. If any of these are controversial enough to deserve separate discussion I'm happy split it into separate PR's.

Author: bors

src/cargo/sources/registry/index.rs

@@ -98,9 +98,8 @@ use cargo_util::{paths, registry::make_dep_path};
 use semver::Version;
 use serde::Deserialize;
 use std::borrow::Cow;
-use std::cmp::Ordering;
 use std::collections::BTreeMap;
-use std::collections::{HashMap, HashSet};
+use std::collections::HashMap;
 use std::fs;
 use std::io::ErrorKind;
 use std::path::Path;
@@ -574,8 +573,7 @@ impl<'cfg> RegistryIndex<'cfg> {
         name: InternedString,
         req: &OptVersionReq,
         load: &mut dyn RegistryData,
-        yanked_whitelist: &HashSet<PackageId>,
-        f: &mut dyn FnMut(Summary),
+        f: &mut dyn FnMut(IndexSummary),
     ) -> Poll<CargoResult<()>> {
         if self.config.offline() {
             // This should only return `Poll::Ready(Ok(()))` if there is at least 1 match.
@@ -592,31 +590,15 @@ impl<'cfg> RegistryIndex<'cfg> {
             let callback = &mut |s: IndexSummary| {
                 if !s.is_offline() {
                     called = true;
-                    f(s.into_summary());
+                    f(s);
                 }
             };
-            ready!(self.query_inner_with_online(
-                name,
-                req,
-                load,
-                yanked_whitelist,
-                callback,
-                false
-            )?);
+            ready!(self.query_inner_with_online(name, req, load, callback, false)?);
             if called {
                 return Poll::Ready(Ok(()));
             }
         }
-        self.query_inner_with_online(
-            name,
-            req,
-            load,
-            yanked_whitelist,
-            &mut |s| {
-                f(s.into_summary());
-            },
-            true,
-        )
+        self.query_inner_with_online(name, req, load, f, true)
     }
 
     /// Inner implementation of [`Self::query_inner`]. Returns the number of
@@ -628,15 +610,10 @@ impl<'cfg> RegistryIndex<'cfg> {
         name: InternedString,
         req: &OptVersionReq,
         load: &mut dyn RegistryData,
-        yanked_whitelist: &HashSet<PackageId>,
         f: &mut dyn FnMut(IndexSummary),
         online: bool,
     ) -> Poll<CargoResult<()>> {
-        let source_id = self.source_id;
-
-        let summaries = ready!(self.summaries(name, req, load))?;
-
-        let summaries = summaries
+        ready!(self.summaries(name, &req, load))?
             // First filter summaries for `--offline`. If we're online then
             // everything is a candidate, otherwise if we're offline we're only
             // going to consider candidates which are actually present on disk.
@@ -654,38 +631,7 @@ impl<'cfg> RegistryIndex<'cfg> {
                     IndexSummary::Offline(s.as_summary().clone())
                 }
             })
-            // 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`
-            .filter(|s| !s.is_yanked() || yanked_whitelist.contains(&s.package_id()));
-
-        // Handle `cargo update --precise` here.
-        let precise = source_id.precise_registry_version(name.as_str());
-        let summaries = summaries.filter(|s| match precise {
-            Some((current, requested)) => {
-                if req.matches(current) {
-                    // Unfortunately crates.io allows versions to differ only
-                    // by build metadata. This shouldn't be allowed, but since
-                    // it is, this will honor it if requested. However, if not
-                    // specified, then ignore it.
-                    let s_vers = s.package_id().version();
-                    match (s_vers.build.is_empty(), requested.build.is_empty()) {
-                        (true, true) => s_vers == requested,
-                        (true, false) => false,
-                        (false, true) => {
-                            // Compare disregarding the metadata.
-                            s_vers.cmp_precedence(requested) == Ordering::Equal
-                        }
-                        (false, false) => s_vers == requested,
-                    }
-                } else {
-                    true
-                }
-            }
-            None => true,
-        });
-
-        summaries.for_each(f);
+            .for_each(f);
         Poll::Ready(Ok(()))
     }
 

src/cargo/sources/registry/mod.rs

@@ -712,25 +712,33 @@ impl<'cfg> Source for RegistrySource<'cfg> {
         kind: QueryKind,
         f: &mut dyn FnMut(Summary),
     ) -> Poll<CargoResult<()>> {
-        // If this is a precise dependency, then it came from a lock file and in
+        let mut req = dep.version_req().clone();
+
+        // Handle `cargo update --precise` here.
+        if let Some((_, requested)) = self
+            .source_id
+            .precise_registry_version(dep.package_name().as_str())
+            .filter(|(c, _)| req.matches(c))
+        {
+            req.update_precise(&requested);
+        }
+
+        // If this is a locked dependency, then it came from a lock file and in
         // theory the registry is known to contain this version. If, however, we
         // come back with no summaries, then our registry may need to be
         // updated, so we fall back to performing a lazy update.
-        if kind == QueryKind::Exact && dep.source_id().has_precise() && !self.ops.is_updated() {
+        if kind == QueryKind::Exact && req.is_locked() && !self.ops.is_updated() {
             debug!("attempting query without update");
             let mut called = false;
-            ready!(self.index.query_inner(
-                dep.package_name(),
-                dep.version_req(),
-                &mut *self.ops,
-                &self.yanked_whitelist,
-                &mut |s| {
-                    if dep.matches(&s) {
+            ready!(self
+                .index
+                .query_inner(dep.package_name(), &req, &mut *self.ops, &mut |s| {
+                    if dep.matches(s.as_summary()) {
+                        // We are looking for a package from a lock file so we do not care about yank
                         called = true;
-                        f(s);
+                        f(s.into_summary());
                     }
-                },
-            ))?;
+                },))?;
             if called {
                 Poll::Ready(Ok(()))
             } else {
@@ -740,22 +748,23 @@ impl<'cfg> Source for RegistrySource<'cfg> {
             }
         } else {
             let mut called = false;
-            ready!(self.index.query_inner(
-                dep.package_name(),
-                dep.version_req(),
-                &mut *self.ops,
-                &self.yanked_whitelist,
-                &mut |s| {
+            ready!(self
+                .index
+                .query_inner(dep.package_name(), &req, &mut *self.ops, &mut |s| {
                     let matched = match kind {
-                        QueryKind::Exact => dep.matches(&s),
+                        QueryKind::Exact => dep.matches(s.as_summary()),
                         QueryKind::Fuzzy => true,
                     };
-                    if matched {
-                        f(s);
+                    // 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 matched
+                        && (!s.is_yanked() || self.yanked_whitelist.contains(&s.package_id()))
+                    {
+                        f(s.into_summary());
                         called = true;
                     }
-                }
-            ))?;
+                }))?;
             if called {
                 return Poll::Ready(Ok(()));
             }
@@ -777,13 +786,9 @@ impl<'cfg> Source for RegistrySource<'cfg> {
                     }
                     any_pending |= self
                         .index
-                        .query_inner(
-                            name_permutation,
-                            dep.version_req(),
-                            &mut *self.ops,
-                            &self.yanked_whitelist,
-                            f,
-                        )?
+                        .query_inner(name_permutation, &req, &mut *self.ops, &mut |s| {
+                            f(s.into_summary());
+                        })?
                         .is_pending();
                 }
             }

src/cargo/util/semver_ext.rs

@@ -8,6 +8,8 @@ pub enum OptVersionReq {
     Req(VersionReq),
     /// The exact locked version and the original version requirement.
     Locked(Version, VersionReq),
+    /// The exact requested version and the original version requirement.
+    UpdatePrecise(Version, VersionReq),
 }
 
 pub trait VersionExt {
@@ -53,7 +55,7 @@ impl OptVersionReq {
     pub fn is_exact(&self) -> bool {
         match self {
             OptVersionReq::Any => false,
-            OptVersionReq::Req(req) => {
+            OptVersionReq::Req(req) | OptVersionReq::UpdatePrecise(_, req) => {
                 req.comparators.len() == 1 && {
                     let cmp = &req.comparators[0];
                     cmp.op == Op::Exact && cmp.minor.is_some() && cmp.patch.is_some()
@@ -69,8 +71,24 @@ impl OptVersionReq {
         let version = version.clone();
         *self = match self {
             Any => Locked(version, VersionReq::STAR),
-            Req(req) => Locked(version, req.clone()),
-            Locked(_, req) => Locked(version, req.clone()),
+            Req(req) | Locked(_, req) | UpdatePrecise(_, req) => Locked(version, req.clone()),
+        };
+    }
+
+    pub fn update_precise(&mut self, version: &Version) {
+        assert!(
+            self.matches(version),
+            "cannot update_precise {} to {}",
+            self,
+            version
+        );
+        use OptVersionReq::*;
+        let version = version.clone();
+        *self = match self {
+            Any => UpdatePrecise(version, VersionReq::STAR),
+            Req(req) | Locked(_, req) | UpdatePrecise(_, req) => {
+                UpdatePrecise(version, req.clone())
+            }
         };
     }
 
@@ -100,6 +118,23 @@ impl OptVersionReq {
                 // we should not silently use `1.0.0+foo` even though they have the same version.
                 v == version
             }
+            OptVersionReq::UpdatePrecise(v, _) => {
+                // This is used for the `--precise` field of cargo update.
+                //
+                // Unfortunately crates.io allowed versions to differ only
+                // by build metadata. This shouldn't be allowed, but since
+                // it is, this will honor it if requested.
+                //
+                // In that context we treat a requirement that does not have
+                // build metadata as allowing any metadata. But, if a requirement
+                // has build metadata, then we only allow it to match the exact
+                // metadata.
+                v.major == version.major
+                    && v.minor == version.minor
+                    && v.patch == version.patch
+                    && v.pre == version.pre
+                    && (v.build == version.build || v.build.is_empty())
+            }
         }
     }
 }
@@ -108,8 +143,9 @@ impl Display for OptVersionReq {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
             OptVersionReq::Any => f.write_str("*"),
-            OptVersionReq::Req(req) => Display::fmt(req, f),
-            OptVersionReq::Locked(_, req) => Display::fmt(req, f),
+            OptVersionReq::Req(req)
+            | OptVersionReq::Locked(_, req)
+            | OptVersionReq::UpdatePrecise(_, req) => Display::fmt(req, f),
         }
     }
 }