Auto merge of #12772 - Eh2406:Locked-means-locked, r=epage

If there's a version in the lock file only use that exact version

### What does this PR try to resolve?

Generally, cargo is of the opinion that semver metadata should be ignored.
It's is relevant to dependency resolution as any other description of the version, just like the description field in cargo.toml.
If your registry has two versions that only differing metadata you get the bugs you deserve.
We implement functionality to make it easier for our users or for us to maintain.

~~So let's use `==` because it's less code to write and test.~~

We also believe that lock files should ensure reproducibility
 and protect against mutations from the registry.
 In this circumstance these two goals are in conflict, and this PR picks reproducibility.
 If the lock file tells us that there is a version called `1.0.0+bar` then
 we should not silently use `1.0.0+foo` even though they have the same version.

This is one of the alternatives/follow-ups discussed in #11447.
It was separated from #12749, to allow for separate discussion and because I was being too clever by half.

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

All tests still pass except for the ones that were removed. The removed tests were only added to verify the on intuitive behavior of the older implementation in #9847.

Author: bors

src/cargo/core/registry.rs

@@ -116,7 +116,7 @@ enum Kind {
 /// directive that we found in a lockfile, if present.
 pub struct LockedPatchDependency {
     /// The original `Dependency` directive, except "locked" so it's version
-    /// requirement is `=foo` and its `SourceId` has a "precise" listed.
+    /// requirement is Locked to `foo` and its `SourceId` has a "precise" listed.
     pub dependency: Dependency,
     /// The `PackageId` that was previously found in a lock file which
     /// `dependency` matches.

src/cargo/sources/registry/index.rs

@@ -439,11 +439,9 @@ impl<'cfg> RegistryIndex<'cfg> {
     /// checking the integrity of a downloaded package matching the checksum in
     /// the index file, aka [`IndexSummary`].
     pub fn hash(&mut self, pkg: PackageId, load: &mut dyn RegistryData) -> Poll<CargoResult<&str>> {
-        let req = OptVersionReq::exact(pkg.version());
+        let req = OptVersionReq::lock_to_exact(pkg.version());
         let summary = self.summaries(pkg.name(), &req, load)?;
-        let summary = ready!(summary)
-            .filter(|s| s.package_id().version() == pkg.version())
-            .next();
+        let summary = ready!(summary).next();
         Poll::Ready(Ok(summary
             .ok_or_else(|| internal(format!("no hash listed for {}", pkg)))?
             .as_summary()
@@ -697,10 +695,8 @@ impl<'cfg> RegistryIndex<'cfg> {
         pkg: PackageId,
         load: &mut dyn RegistryData,
     ) -> Poll<CargoResult<bool>> {
-        let req = OptVersionReq::exact(pkg.version());
-        let found = ready!(self.summaries(pkg.name(), &req, load))?
-            .filter(|s| s.package_id().version() == pkg.version())
-            .any(|s| s.is_yanked());
+        let req = OptVersionReq::lock_to_exact(pkg.version());
+        let found = ready!(self.summaries(pkg.name(), &req, load))?.any(|s| s.is_yanked());
         Poll::Ready(Ok(found))
     }
 }

src/cargo/util/semver_ext.rs

@@ -1,6 +1,5 @@
 use semver::{Comparator, Op, Version, VersionReq};
 use serde_untagged::UntaggedEnumVisitor;
-use std::cmp::Ordering;
 use std::fmt::{self, Display};
 
 #[derive(PartialEq, Eq, Hash, Clone, Debug)]
@@ -44,6 +43,13 @@ impl OptVersionReq {
         OptVersionReq::Req(VersionReq::exact(version))
     }
 
+    // Since some registries have allowed crate versions to differ only by build metadata,
+    // A query using OptVersionReq::exact return nondeterministic results.
+    // So we `lock_to` the exact version were interested in.
+    pub fn lock_to_exact(version: &Version) -> Self {
+        OptVersionReq::Locked(version.clone(), VersionReq::exact(version))
+    }
+
     pub fn is_exact(&self) -> bool {
         match self {
             OptVersionReq::Any => false,
@@ -84,7 +90,16 @@ impl OptVersionReq {
         match self {
             OptVersionReq::Any => true,
             OptVersionReq::Req(req) => req.matches(version),
-            OptVersionReq::Locked(v, _) => v.cmp_precedence(version) == Ordering::Equal,
+            OptVersionReq::Locked(v, _) => {
+                // Generally, cargo is of the opinion that semver metadata should be ignored.
+                // If your registry has two versions that only differing metadata you get the bugs you deserve.
+                // We also believe that lock files should ensure reproducibility
+                // and protect against mutations from the registry.
+                // In this circumstance these two goals are in conflict, and we pick reproducibility.
+                // If the lock file tells us that there is a version called `1.0.0+bar` then
+                // we should not silently use `1.0.0+foo` even though they have the same version.
+                v == version
+            }
         }
     }
 }
@@ -316,40 +331,3 @@ fn is_req(value: &str) -> bool {
     };
     "<>=^~".contains(first) || value.contains('*') || value.contains(',')
 }
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    #[test]
-    fn locked_has_the_same_with_exact() {
-        fn test_versions(target_ver: &str, vers: &[&str]) {
-            let ver = Version::parse(target_ver).unwrap();
-            let exact = OptVersionReq::exact(&ver);
-            let mut locked = exact.clone();
-            locked.lock_to(&ver);
-            for v in vers {
-                let v = Version::parse(v).unwrap();
-                assert_eq!(exact.matches(&v), locked.matches(&v));
-            }
-        }
-
-        test_versions(
-            "1.0.0",
-            &["1.0.0", "1.0.1", "0.9.9", "0.10.0", "0.1.0", "1.0.0-pre"],
-        );
-        test_versions("0.9.0", &["0.9.0", "0.9.1", "1.9.0", "0.0.9", "0.9.0-pre"]);
-        test_versions("0.0.2", &["0.0.2", "0.0.1", "0.0.3", "0.0.2-pre"]);
-        test_versions(
-            "0.1.0-beta2.a",
-            &[
-                "0.1.0-beta2.a",
-                "0.9.1",
-                "0.1.0",
-                "0.1.1-beta2.a",
-                "0.1.0-beta2",
-            ],
-        );
-        test_versions("0.1.0+meta", &["0.1.0", "0.1.0+meta", "0.1.0+any"]);
-    }
-}

src/cargo/util/toml/mod.rs

@@ -1274,8 +1274,7 @@ impl TomlManifest {
                 replacement.unused_keys(),
                 &mut cx.warnings,
             );
-            dep.set_version_req(OptVersionReq::exact(&version))
-                .lock_version(&version);
+            dep.set_version_req(OptVersionReq::exact(&version));
             replace.push((spec, dep));
         }
         Ok(replace)

tests/testsuite/registry.rs

@@ -3597,6 +3597,57 @@ fn differ_only_by_metadata() {
 [CHECKING] baz v0.0.1+b
 [CHECKING] foo v0.0.1 ([CWD])
 [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s
+",
+        )
+        .run();
+
+    Package::new("baz", "0.0.1+d").publish();
+
+    p.cargo("clean").run();
+    p.cargo("check")
+        .with_stderr_contains("[CHECKING] baz v0.0.1+b")
+        .run();
+}
+
+#[cargo_test]
+fn differ_only_by_metadata_with_lockfile() {
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.0.1"
+                authors = []
+
+                [dependencies]
+                baz = "=0.0.1"
+            "#,
+        )
+        .file("src/main.rs", "fn main() {}")
+        .build();
+
+    Package::new("baz", "0.0.1+a").publish();
+    Package::new("baz", "0.0.1+b").publish();
+    Package::new("baz", "0.0.1+c").publish();
+
+    p.cargo("update --package baz --precise 0.0.1+b")
+        .with_stderr(
+            "\
+[UPDATING] [..] index
+[..] baz v0.0.1+c -> v0.0.1+b
+",
+        )
+        .run();
+
+    p.cargo("check")
+        .with_stderr(
+            "\
+[DOWNLOADING] crates ...
+[DOWNLOADED] [..] v0.0.1+b (registry `dummy-registry`)
+[CHECKING] baz v0.0.1+b
+[CHECKING] foo v0.0.1 ([CWD])
+[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s
 ",
         )
         .run();