Auto merge of #7118 - alexcrichton:patch-bug, r=Eh2406

Handle activation conflicts for `[patch]` sources

This commit updates the resolver to ensure that it recognizes conflicts
when `[patch]` is used to augment an older version of what's already in
a source, for example. Previously the deduplication based on
semver-compatible versions didn't actually work when `[patch]` was used.
This meant that when you used `[patch]` it might not transitively affect
the entire crate graph, instead just giving you a version of a
dependency and everyone else. This violates the intention of `[patch]`!

The fix here is to catch this use case happening, when a `Dependency`
source specification mismatches an activated package we need to list a
second activation in the resolver to prevent major versions from being
selected from both the original source as well as the source of the id.

Closes #7117

Author: bors

src/cargo/core/resolver/context.rs

@@ -99,8 +99,17 @@ impl Context {
 
     /// Activate this summary by inserting it into our list of known activations.
     ///
+    /// The `parent` passed in here is the parent summary/dependency edge which
+    /// cased `summary` to get activated. This may not be present for the root
+    /// crate, for example.
+    ///
     /// Returns `true` if this summary with the given method is already activated.
-    pub fn flag_activated(&mut self, summary: &Summary, method: &Method) -> CargoResult<bool> {
+    pub fn flag_activated(
+        &mut self,
+        summary: &Summary,
+        method: &Method,
+        parent: Option<(&Summary, &Dependency)>,
+    ) -> CargoResult<bool> {
         let id = summary.package_id();
         let age: ContextAge = self.age();
         match self.activations.entry(id.as_activations_key()) {
@@ -121,6 +130,30 @@ impl Context {
                     );
                 }
                 v.insert((summary.clone(), age));
+
+                // If we've got a parent dependency which activated us, *and*
+                // the dependency has a different source id listed than the
+                // `summary` itself, then things get interesting. This basically
+                // means that a `[patch]` was used to augment `dep.source_id()`
+                // with `summary`.
+                //
+                // In this scenario we want to consider the activation key, as
+                // viewed from the perspective of `dep.source_id()`, as being
+                // fulfilled. This means that we need to add a second entry in
+                // the activations map for the source that was patched, in
+                // addition to the source of the actual `summary` itself.
+                //
+                // Without this it would be possible to have both 1.0.0 and
+                // 1.1.0 "from crates.io" in a dependency graph if one of those
+                // versions came from a `[patch]` source.
+                if let Some((_, dep)) = parent {
+                    if dep.source_id() != id.source_id() {
+                        let key = (id.name(), dep.source_id(), id.version().into());
+                        let prev = self.activations.insert(key, (summary.clone(), age));
+                        assert!(prev.is_none());
+                    }
+                }
+
                 return Ok(false);
             }
         }

src/cargo/core/resolver/mod.rs

@@ -651,11 +651,16 @@ fn activate(
         }
     }
 
-    let activated = cx.flag_activated(&candidate, &method)?;
+    let activated = cx.flag_activated(&candidate, &method, parent)?;
 
     let candidate = match registry.replacement_summary(candidate_pid) {
         Some(replace) => {
-            if cx.flag_activated(replace, &method)? && activated {
+            // Note the `None` for parent here since `[replace]` is a bit wonky
+            // and doesn't activate the same things that `[patch]` typically
+            // does. TBH it basically cause panics in the test suite if
+            // `parent` is passed through here and `[replace]` is otherwise
+            // on life support so it's not critical to fix bugs anyway per se.
+            if cx.flag_activated(replace, &method, None)? && activated {
                 return Ok(None);
             }
             trace!(

tests/testsuite/patch.rs

@@ -1020,3 +1020,62 @@ fn replace_prerelease() {
 
     p.cargo("build").run();
 }
+
+#[cargo_test]
+fn patch_older() {
+    Package::new("baz", "1.0.2").publish();
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+
+                [dependencies]
+                bar = { path = 'bar' }
+                baz = "=1.0.1"
+
+                [patch.crates-io]
+                baz = { path = "./baz" }
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .file(
+            "bar/Cargo.toml",
+            r#"
+                [project]
+                name = "bar"
+                version = "0.5.0"
+                authors = []
+
+                [dependencies]
+                baz = "1.0.0"
+            "#,
+        )
+        .file("bar/src/lib.rs", "")
+        .file(
+            "baz/Cargo.toml",
+            r#"
+                [project]
+                name = "baz"
+                version = "1.0.1"
+                authors = []
+            "#,
+        )
+        .file("baz/src/lib.rs", "")
+        .build();
+
+    p.cargo("build")
+        .with_stderr(
+            "\
+[UPDATING] [..]
+[COMPILING] baz v1.0.1 [..]
+[COMPILING] bar v0.5.0 [..]
+[COMPILING] foo v0.1.0 [..]
+[FINISHED] [..]
+",
+        )
+        .run();
+}