Auto merge of #11770 - Eh2406:non-active-conflict, r=weihanglo

patch can conflict on not activated packages

### What does this PR try to resolve?

In the resolver there is a data structure called a `conflicting_activations`, which records all the reasons a "better" version was not picked for the package being resolved. Normally, these are packages that are currently active that for one reason or another block one of the other versions. Several optimizations assumed that this was always true, even going so far as to use `.expect`. This was a mistake because when there's a `patch` involved there can be conflicts on a version that is not selected. So the correct behavior is to fall back to skip the optimizations and try all versions when a `conflicting_activations` are not active.

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

This adds two automated tests based on the reproductions in #7463 and #11336. So the test suite now covers this case. It can also be tested by reconstructing the repositories originally reported in those issues.

### Additional information

It could be that in the long term the correct fix is to figure out how to support patch without having a conflicting activation that is not activated. But that would be a much bigger change. And for now this assumption is only used in optimizations, so this gets people unstuck.

src/cargo/core/resolver/mod.rs

@@ -843,10 +843,8 @@ impl RemainingCandidates {
     }
 }
 
-/// Attempts to find a new conflict that allows a `find_candidate` feather then the input one.
+/// Attempts to find a new conflict that allows a `find_candidate` better then the input one.
 /// It will add the new conflict to the cache if one is found.
-///
-/// Panics if the input conflict is not all active in `cx`.
 fn generalize_conflicting(
     cx: &Context,
     registry: &mut RegistryQueryer<'_>,
@@ -855,15 +853,12 @@ fn generalize_conflicting(
     dep: &Dependency,
     conflicting_activations: &ConflictMap,
 ) -> Option<ConflictMap> {
-    if conflicting_activations.is_empty() {
-        return None;
-    }
     // We need to determine the `ContextAge` that this `conflicting_activations` will jump to, and why.
-    let (backtrack_critical_age, backtrack_critical_id) = conflicting_activations
-        .keys()
-        .map(|&c| (cx.is_active(c).expect("not currently active!?"), c))
-        .max()
-        .unwrap();
+    let (backtrack_critical_age, backtrack_critical_id) = shortcircuit_max(
+        conflicting_activations
+            .keys()
+            .map(|&c| cx.is_active(c).map(|a| (a, c))),
+    )?;
     let backtrack_critical_reason: ConflictReason =
         conflicting_activations[&backtrack_critical_id].clone();
 
@@ -958,6 +953,19 @@ fn generalize_conflicting(
     None
 }
 
+/// Returns Some of the largest item in the iterator.
+/// Returns None if any of the items are None or the iterator is empty.
+fn shortcircuit_max<I: Ord>(iter: impl Iterator<Item = Option<I>>) -> Option<I> {
+    let mut out = None;
+    for i in iter {
+        if i.is_none() {
+            return None;
+        }
+        out = std::cmp::max(out, i);
+    }
+    out
+}
+
 /// Looks through the states in `backtrack_stack` for dependencies with
 /// remaining candidates. For each one, also checks if rolling back
 /// could change the outcome of the failed resolution that caused backtracking
@@ -984,12 +992,10 @@ fn find_candidate(
     // the cause of that backtrack, so we do not update it.
     let age = if !backtracked {
         // we don't have abnormal situations. So we can ask `cx` for how far back we need to go.
-        let a = cx.is_conflicting(Some(parent.package_id()), conflicting_activations);
-        // If the `conflicting_activations` does not apply to `cx`, then something went very wrong
-        // in building it. But we will just fall back to laboriously trying all possibilities witch
-        // will give us the correct answer so only `assert` if there is a developer to debug it.
-        debug_assert!(a.is_some());
-        a
+        // If the `conflicting_activations` does not apply to `cx`,
+        // we will just fall back to laboriously trying all possibilities witch
+        // will give us the correct answer.
+        cx.is_conflicting(Some(parent.package_id()), conflicting_activations)
     } else {
         None
     };

tests/testsuite/patch.rs

@@ -2541,3 +2541,105 @@ foo v0.1.0 [..]
         ))
         .run();
 }
+
+// From https://github.com/rust-lang/cargo/issues/7463
+#[cargo_test]
+fn patch_eq_conflict_panic() {
+    Package::new("bar", "0.1.0").publish();
+    Package::new("bar", "0.1.1").publish();
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+
+                [dependencies]
+                bar = "=0.1.0"
+
+                [dev-dependencies]
+                bar = "=0.1.1"
+
+                [patch.crates-io]
+                bar = {path="bar"}
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .file("bar/Cargo.toml", &basic_manifest("bar", "0.1.1"))
+        .file("bar/src/lib.rs", "")
+        .build();
+
+    p.cargo("generate-lockfile")
+        .with_status(101)
+        .with_stderr(
+            r#"[UPDATING] `dummy-registry` index
+[ERROR] failed to select a version for `bar`.
+    ... required by package `foo v0.1.0 ([..])`
+versions that meet the requirements `=0.1.1` are: 0.1.1
+
+all possible versions conflict with previously selected packages.
+
+  previously selected package `bar v0.1.0`
+    ... which satisfies dependency `bar = "=0.1.0"` of package `foo v0.1.0 ([..])`
+
+failed to select a version for `bar` which could resolve this conflict
+"#,
+        )
+        .run();
+}
+
+// From https://github.com/rust-lang/cargo/issues/11336
+#[cargo_test]
+fn mismatched_version2() {
+    Package::new("qux", "0.1.0-beta.1").publish();
+    Package::new("qux", "0.1.0-beta.2").publish();
+    Package::new("bar", "0.1.0")
+        .dep("qux", "=0.1.0-beta.1")
+        .publish();
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                 [package]
+                 name = "foo"
+                 version = "0.1.0"
+
+                 [dependencies]
+                 bar = "0.1.0"
+                 qux = "0.1.0-beta.2"
+
+                 [patch.crates-io]
+                 qux = { path = "qux" }
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .file(
+            "qux/Cargo.toml",
+            r#"
+                [package]
+                name = "qux"
+                version = "0.1.0-beta.1"
+            "#,
+        )
+        .file("qux/src/lib.rs", "")
+        .build();
+
+    p.cargo("generate-lockfile")
+        .with_status(101)
+        .with_stderr(
+            r#"[UPDATING] `dummy-registry` index
+[ERROR] failed to select a version for `qux`.
+    ... required by package `bar v0.1.0`
+    ... which satisfies dependency `bar = "^0.1.0"` of package `foo v0.1.0 ([..])`
+versions that meet the requirements `=0.1.0-beta.1` are: 0.1.0-beta.1
+
+all possible versions conflict with previously selected packages.
+
+  previously selected package `qux v0.1.0-beta.2`
+    ... which satisfies dependency `qux = "^0.1.0-beta.2"` of package `foo v0.1.0 ([..])`
+
+failed to select a version for `qux` which could resolve this conflict"#,
+        )
+        .run();
+}