Auto merge of #7303 - alexcrichton:duplicate-patch, r=ehuss

Fixes around multiple `[patch]` per crate

This commit fixes a few bugs in handling of `[patch]` where multiple
version of the same crate name have been patched in. Two sub-issues were
discovered when investigating #7264:

* The first issue is that the source of the assertion, the logic in
  `lock`, revealed a fundamental flaw in the logic. The `lock` function
  serves the purpose of applying a lock file to a dependency candidate
  and ensure that it stays within locked versions of dependencies, if
  they're previously found. The logic here to handle `[patch]`, however,
  happened a bit too late and was a bit too zealous to simply lock a
  dependency by name instead of accounting for the version as well.

  The updated logic is to move the locking of dependencies here to
  during the normal iteration over the list of dependencies. Adjacent to
  `matches_id` we check `matches_ignoring_source`. If the latter returns
  `true` then we double-check that the previous dependency is still in
  `[patch]`, and then we let it through. This means that patches with
  multiple versions should be correctly handled where edges drawn with
  `[patch]` are preserved.

* The second issue, after fixing this, was found where if the exact same
  version was listed in `[patch]` multiple times then we would
  continuously update the original source since one of the replacements
  gets lost along the way. This commit adds a first-class warning
  disallowing patches pointing to the exact same crate version, since we
  don't have a way to prioritize amongst them anyway.

Closes #7264

Author: bors

src/cargo/core/registry.rs

@@ -1,5 +1,6 @@
 use std::collections::{HashMap, HashSet};
 
+use failure::bail;
 use log::{debug, trace};
 use semver::VersionReq;
 use url::Url;
@@ -79,7 +80,28 @@ pub struct PackageRegistry<'cfg> {
     patches_available: HashMap<Url, Vec<PackageId>>,
 }
 
-type LockedMap = HashMap<SourceId, HashMap<String, Vec<(PackageId, Vec<PackageId>)>>>;
+/// A map of all "locked packages" which is filled in when parsing a lock file
+/// and is used to guide dependency resolution by altering summaries as they're
+/// queried from this source.
+///
+/// This map can be thought of as a glorified `Vec<MySummary>` where `MySummary`
+/// has a `PackageId` for which package it represents as well as a list of
+/// `PackageId` for the resolved dependencies. The hash map is otherwise
+/// structured though for easy access throughout this registry.
+type LockedMap = HashMap<
+    // The first level of key-ing done in this hash map is the source that
+    // dependencies come from, identified by a `SourceId`.
+    SourceId,
+    HashMap<
+        // This next level is keyed by the name of the package...
+        String,
+        // ... and the value here is a list of tuples. The first element of each
+        // tuple is a package which has the source/name used to get to this
+        // point. The second element of each tuple is the list of locked
+        // dependencies that the first element has.
+        Vec<(PackageId, Vec<PackageId>)>,
+    >,
+>;
 
 #[derive(PartialEq, Eq, Clone, Copy)]
 enum Kind {
@@ -275,6 +297,20 @@ impl<'cfg> PackageRegistry<'cfg> {
             .collect::<CargoResult<Vec<_>>>()
             .chain_err(|| failure::format_err!("failed to resolve patches for `{}`", url))?;
 
+        let mut name_and_version = HashSet::new();
+        for summary in unlocked_summaries.iter() {
+            let name = summary.package_id().name();
+            let version = summary.package_id().version();
+            if !name_and_version.insert((name, version)) {
+                bail!(
+                    "cannot have two `[patch]` entries which both resolve \
+                     to `{} v{}`",
+                    name,
+                    version
+                );
+            }
+        }
+
         // Note that we do not use `lock` here to lock summaries! That step
         // happens later once `lock_patches` is invoked. In the meantime though
         // we want to fill in the `patches_available` map (later used in the
@@ -579,7 +615,7 @@ fn lock(locked: &LockedMap, patches: &HashMap<Url, Vec<PackageId>>, summary: Sum
 
     // Lock the summary's ID if possible
     let summary = match pair {
-        Some(&(ref precise, _)) => summary.override_id(precise.clone()),
+        Some((precise, _)) => summary.override_id(precise.clone()),
         None => summary,
     };
     summary.map_dependencies(|dep| {
@@ -603,18 +639,56 @@ fn lock(locked: &LockedMap, patches: &HashMap<Url, Vec<PackageId>>, summary: Sum
         //    locked version because the dependency needs to be
         //    re-resolved.
         //
-        // 3. We don't have a lock entry for this dependency, in which
+        // 3. We have a lock entry for this dependency, but it's from a
+        //    different source than what's listed. This lock though happens
+        //    through `[patch]`, so we want to preserve it.
+        //
+        // 4. We don't have a lock entry for this dependency, in which
         //    case it was likely an optional dependency which wasn't
         //    included previously so we just pass it through anyway.
         //
-        // Cases 1/2 are handled by `matches_id` and case 3 is handled by
-        // falling through to the logic below.
-        if let Some(&(_, ref locked_deps)) = pair {
-            let locked = locked_deps.iter().find(|&&id| dep.matches_id(id));
+        // Cases 1/2 are handled by `matches_id`, case 3 is handled specially,
+        // and case 4 is handled by falling through to the logic below.
+        if let Some((_, locked_deps)) = pair {
+            let locked = locked_deps.iter().find(|&&id| {
+                // If the dependency matches the package id exactly then we've
+                // found a match, this is the id the dependency was previously
+                // locked to.
+                if dep.matches_id(id) {
+                    return true;
+                }
+
+                // If the name/version doesn't match, then we definitely don't
+                // have a match whatsoever. Otherwise we need to check
+                // `[patch]`...
+                if !dep.matches_ignoring_source(id) {
+                    return false;
+                }
+
+                // ... so here we look up the dependency url in the patches
+                // map, and we see if `id` is contained in the list of patches
+                // for that url. If it is then this lock is still valid,
+                // otherwise the lock is no longer valid.
+                match patches.get(dep.source_id().url()) {
+                    Some(list) => list.contains(&id),
+                    None => false,
+                }
+            });
+
             if let Some(&locked) = locked {
                 trace!("\tfirst hit on {}", locked);
                 let mut dep = dep;
-                dep.lock_to(locked);
+
+                // If we found a locked version where the sources match, then
+                // we can `lock_to` to get an exact lock on this dependency.
+                // Otherwise we got a lock via `[patch]` so we only lock the
+                // version requirement, not the source.
+                if locked.source_id() == dep.source_id() {
+                    dep.lock_to(locked);
+                } else {
+                    let req = VersionReq::exact(locked.version());
+                    dep.set_version_req(req);
+                }
                 return dep;
             }
         }
@@ -633,33 +707,6 @@ fn lock(locked: &LockedMap, patches: &HashMap<Url, Vec<PackageId>>, summary: Sum
             return dep;
         }
 
-        // Finally we check to see if any registered patches correspond to
-        // this dependency.
-        let v = patches.get(dep.source_id().url()).map(|vec| {
-            let dep2 = dep.clone();
-            let mut iter = vec
-                .iter()
-                .filter(move |&&p| dep2.matches_ignoring_source(p));
-            (iter.next(), iter)
-        });
-        if let Some((Some(patch_id), mut remaining)) = v {
-            assert!(remaining.next().is_none());
-            let patch_source = patch_id.source_id();
-            let patch_locked = locked
-                .get(&patch_source)
-                .and_then(|m| m.get(&*patch_id.name()))
-                .map(|list| list.iter().any(|&(ref id, _)| id == patch_id))
-                .unwrap_or(false);
-
-            if patch_locked {
-                trace!("\tthird hit on {}", patch_id);
-                let req = VersionReq::exact(patch_id.version());
-                let mut dep = dep;
-                dep.set_version_req(req);
-                return dep;
-            }
-        }
-
         trace!("\tnope, unlocked");
         dep
     })

tests/testsuite/patch.rs

@@ -1179,3 +1179,162 @@ fn multipatch() {
 
     p.cargo("build").run();
 }
+
+#[cargo_test]
+fn patch_same_version() {
+    let bar = git::repo(&paths::root().join("override"))
+        .file("Cargo.toml", &basic_manifest("bar", "0.1.0"))
+        .file("src/lib.rs", "")
+        .build();
+
+    crate::support::registry::init();
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            &format!(
+                r#"
+                [package]
+                name = "foo"
+                version = "0.0.1"
+                [dependencies]
+                bar = "0.1"
+                [patch.crates-io]
+                bar = {{ path = "bar" }}
+                bar2 = {{ git = '{}', package = 'bar' }}
+            "#,
+                bar.url(),
+            ),
+        )
+        .file("src/lib.rs", "")
+        .file(
+            "bar/Cargo.toml",
+            r#"
+                [package]
+                name = "bar"
+                version = "0.1.0"
+            "#,
+        )
+        .file("bar/src/lib.rs", "")
+        .build();
+
+    p.cargo("build")
+        .with_status(101)
+        .with_stderr(
+            "\
+[UPDATING] [..]
+error: cannot have two `[patch]` entries which both resolve to `bar v0.1.0`
+",
+        )
+        .run();
+}
+
+#[cargo_test]
+fn two_semver_compatible() {
+    let bar = git::repo(&paths::root().join("override"))
+        .file("Cargo.toml", &basic_manifest("bar", "0.1.1"))
+        .file("src/lib.rs", "")
+        .build();
+
+    crate::support::registry::init();
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            &format!(
+                r#"
+                [package]
+                name = "foo"
+                version = "0.0.1"
+                [dependencies]
+                bar = "0.1"
+                [patch.crates-io]
+                bar = {{ path = "bar" }}
+                bar2 = {{ git = '{}', package = 'bar' }}
+            "#,
+                bar.url(),
+            ),
+        )
+        .file("src/lib.rs", "pub fn foo() { bar::foo() }")
+        .file(
+            "bar/Cargo.toml",
+            r#"
+                [package]
+                name = "bar"
+                version = "0.1.2"
+            "#,
+        )
+        .file("bar/src/lib.rs", "pub fn foo() {}")
+        .build();
+
+    // assert the build succeeds and doesn't panic anywhere, and then afterwards
+    // assert that the build succeeds again without updating anything or
+    // building anything else.
+    p.cargo("build").run();
+    p.cargo("build")
+        .with_stderr(
+            "\
+warning: Patch `bar v0.1.1 [..]` was not used in the crate graph.
+Check that [..]
+with the [..]
+what is [..]
+version. [..]
+[FINISHED] [..]",
+        )
+        .run();
+}
+
+#[cargo_test]
+fn multipatch_select_big() {
+    let bar = git::repo(&paths::root().join("override"))
+        .file("Cargo.toml", &basic_manifest("bar", "0.1.0"))
+        .file("src/lib.rs", "")
+        .build();
+
+    crate::support::registry::init();
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            &format!(
+                r#"
+                [package]
+                name = "foo"
+                version = "0.0.1"
+                [dependencies]
+                bar = "*"
+                [patch.crates-io]
+                bar = {{ path = "bar" }}
+                bar2 = {{ git = '{}', package = 'bar' }}
+            "#,
+                bar.url(),
+            ),
+        )
+        .file("src/lib.rs", "pub fn foo() { bar::foo() }")
+        .file(
+            "bar/Cargo.toml",
+            r#"
+                [package]
+                name = "bar"
+                version = "0.2.0"
+            "#,
+        )
+        .file("bar/src/lib.rs", "pub fn foo() {}")
+        .build();
+
+    // assert the build succeeds, which is only possible if 0.2.0 is selected
+    // since 0.1.0 is missing the function we need. Afterwards assert that the
+    // build succeeds again without updating anything or building anything else.
+    p.cargo("build").run();
+    p.cargo("build")
+        .with_stderr(
+            "\
+warning: Patch `bar v0.1.0 [..]` was not used in the crate graph.
+Check that [..]
+with the [..]
+what is [..]
+version. [..]
+[FINISHED] [..]",
+        )
+        .run();
+}