add comments to generalize_conflicting and use more general logic

Author: Eh2406

src/cargo/core/resolver/conflict_cache.rs

@@ -18,20 +18,17 @@ enum ConflictStoreTrie {
 
 impl ConflictStoreTrie {
     /// Finds any known set of conflicts, if any,
-    /// which are activated in `cx` and contain `PackageId` specified.
+    /// where all elements return some from `is_active` and contain `PackageId` specified.
     /// If more then one are activated, then it will return
     /// one that will allow for the most jump-back.
-    fn find_conflicting(
+    fn find(
         &self,
-        cx: &Context,
+        is_active: &impl Fn(PackageId) -> Option<usize>,
         must_contain: Option<PackageId>,
     ) -> Option<(&ConflictMap, usize)> {
         match self {
             ConflictStoreTrie::Leaf(c) => {
                 if must_contain.is_none() {
-                    // `is_conflicting` checks that all the elements are active,
-                    // but we have checked each one by the recursion of this function.
-                    debug_assert!(cx.is_conflicting(None, c).is_some());
                     Some((c, 0))
                 } else {
                     // We did not find `must_contain`, so we need to keep looking.
@@ -45,9 +42,9 @@ impl ConflictStoreTrie {
                     .unwrap_or_else(|| m.range(..))
                 {
                     // If the key is active, then we need to check all of the corresponding subtrie.
-                    if let Some(age_this) = cx.is_active(pid) {
+                    if let Some(age_this) = is_active(pid) {
                         if let Some((o, age_o)) =
-                            store.find_conflicting(cx, must_contain.filter(|&f| f != pid))
+                            store.find(is_active, must_contain.filter(|&f| f != pid))
                         {
                             let age = if must_contain == Some(pid) {
                                 // all the results will include `must_contain`
@@ -102,28 +99,6 @@ impl ConflictStoreTrie {
             *self = ConflictStoreTrie::Leaf(con)
         }
     }
-
-    fn contains(&self, mut iter: impl Iterator<Item = PackageId>, con: &ConflictMap) -> bool {
-        match (self, iter.next()) {
-            (ConflictStoreTrie::Leaf(c), None) => {
-                if cfg!(debug_assertions) {
-                    let a: Vec<_> = con.keys().collect();
-                    let b: Vec<_> = c.keys().collect();
-                    assert_eq!(a, b);
-                }
-                true
-            }
-            (ConflictStoreTrie::Leaf(_), Some(_)) => false,
-            (ConflictStoreTrie::Node(_), None) => false,
-            (ConflictStoreTrie::Node(m), Some(n)) => {
-                if let Some(next) = m.get(&n) {
-                    next.contains(iter, con)
-                } else {
-                    false
-                }
-            }
-        }
-    }
 }
 
 pub(super) struct ConflictCache {
@@ -172,6 +147,17 @@ impl ConflictCache {
             dep_from_pid: HashMap::new(),
         }
     }
+    pub fn find(
+        &self,
+        dep: &Dependency,
+        is_active: &impl Fn(PackageId) -> Option<usize>,
+        must_contain: Option<PackageId>,
+    ) -> Option<&ConflictMap> {
+        self.con_from_dep
+            .get(dep)?
+            .find(is_active, must_contain)
+            .map(|(c, _)| c)
+    }
     /// Finds any known set of conflicts, if any,
     /// which are activated in `cx` and contain `PackageId` specified.
     /// If more then one are activated, then it will return
@@ -182,14 +168,11 @@ impl ConflictCache {
         dep: &Dependency,
         must_contain: Option<PackageId>,
     ) -> Option<&ConflictMap> {
-        let out = self
-            .con_from_dep
-            .get(dep)?
-            .find_conflicting(cx, must_contain)
-            .map(|(c, _)| c);
+        let out = self.find(dep, &|id| cx.is_active(id), must_contain);
         if cfg!(debug_assertions) {
-            if let Some(f) = must_contain {
-                if let Some(c) = &out {
+            if let Some(c) = &out {
+                assert!(cx.is_conflicting(None, c).is_some());
+                if let Some(f) = must_contain {
                     assert!(c.contains_key(&f));
                 }
             }
@@ -229,17 +212,6 @@ impl ConflictCache {
         }
     }
 
-    /// Check if a conflict was previously added of the form:
-    /// `dep` is known to be unresolvable if
-    /// all the `PackageId` entries are activated.
-    pub fn contains(&self, dep: &Dependency, con: &ConflictMap) -> bool {
-        if let Some(cst) = self.con_from_dep.get(dep) {
-            cst.contains(con.keys().cloned(), con)
-        } else {
-            false
-        }
-    }
-
     pub fn dependencies_conflicting_with(&self, pid: PackageId) -> Option<&HashSet<Dependency>> {
         self.dep_from_pid.get(&pid)
     }

src/cargo/core/resolver/mod.rs

@@ -895,24 +895,62 @@ fn generalize_conflicting(
             // A dep is equivalent to one of the things it can resolve to.
             // Thus, if all the things it can resolve to have already ben determined
             // to be conflicting, then we can just say that we conflict with the parent.
-            if registry
+            if let Some(others) = registry
                 .query(critical_parents_dep)
                 .expect("an already used dep now error!?")
                 .iter()
                 .rev() // the last one to be tried is the least likely to be in the cache, so start with that.
-                .all(|other| {
-                    let mut con = conflicting_activations.clone();
-                    con.remove(&backtrack_critical_id);
-                    con.insert(
-                        other.summary.package_id(),
-                        backtrack_critical_reason.clone(),
-                    );
-                    past_conflicting_activations.contains(dep, &con)
+                .map(|other| {
+                    past_conflicting_activations
+                        .find(
+                            dep,
+                            &|id| {
+                                if id == other.summary.package_id() {
+                                    // we are imagining that we used other instead
+                                    Some(backtrack_critical_age)
+                                } else {
+                                    cx.is_active(id).filter(|&age|
+                                        // we only care about things that are older then critical_age
+                                        age < backtrack_critical_age)
+                                }
+                            },
+                            Some(other.summary.package_id()),
+                        )
+                        .map(|con| (other.summary.package_id(), con))
                 })
+                .collect::<Option<Vec<(PackageId, &ConflictMap)>>>()
             {
                 let mut con = conflicting_activations.clone();
-                con.remove(&backtrack_critical_id);
+                // It is always valid to combine previously inserted conflicts.
+                // A, B are both known bad states each that can never be activated.
+                // A + B is redundant but cant be activated, as if
+                // A + B is active then A is active and we know that is not ok.
+                for (_, other) in &others {
+                    con.extend(other.iter().map(|(&id, re)| (id, re.clone())));
+                }
+                // Now that we have this combined conflict, we can do a substitution:
+                // A dep is equivalent to one of the things it can resolve to.
+                // So we can remove all the things that it resolves to and replace with the parent.
+                for (other_id, _) in &others {
+                    con.remove(other_id);
+                }
                 con.insert(*critical_parent, backtrack_critical_reason);
+
+                #[cfg(debug_assertions)]
+                {
+                    // the entire point is to find an older conflict, so let's make sure we did
+                    let new_age = con
+                        .keys()
+                        .map(|&c| cx.is_active(c).expect("not currently active!?"))
+                        .max()
+                        .unwrap();
+                    assert!(
+                        new_age < backtrack_critical_age,
+                        "new_age {} < backtrack_critical_age {}",
+                        new_age,
+                        backtrack_critical_age
+                    );
+                }
                 past_conflicting_activations.insert(dep, &con);
                 return Some(con);
             }