address suggestions

Author: Eh2406

src/cargo/core/resolver/conflict_cache.rs

@@ -41,13 +41,15 @@ pub(super) struct ConflictCache {
 }
 
 impl ConflictCache {
-    pub(super) fn new() -> ConflictCache {
+    pub fn new() -> ConflictCache {
         ConflictCache {
             con_from_dep: HashMap::new(),
             dep_from_pid: HashMap::new(),
         }
     }
-    pub(super) fn filter_conflicting<F>(
+    /// Finds any known set of conflicts, if any,
+    /// which are activated in `cx` and pass the `filter` specified?
+    pub fn find_conflicting<F>(
         &self,
         cx: &Context,
         dep: &Dependency,
@@ -56,21 +58,24 @@ impl ConflictCache {
     where
         for<'r> F: FnMut(&'r &HashMap<PackageId, ConflictReason>) -> bool,
     {
-        self.con_from_dep.get(dep).and_then(|past_bad| {
-            past_bad
-                .iter()
-                .filter(filter)
-                .find(|conflicting| cx.is_conflicting(None, conflicting))
-        })
+        self.con_from_dep
+            .get(dep)?
+            .iter()
+            .filter(filter)
+            .find(|conflicting| cx.is_conflicting(None, conflicting))
     }
-    pub(super) fn conflicting(
+    pub fn conflicting(
         &self,
         cx: &Context,
         dep: &Dependency,
     ) -> Option<&HashMap<PackageId, ConflictReason>> {
-        self.filter_conflicting(cx, dep, |_| true)
+        self.find_conflicting(cx, dep, |_| true)
     }
-    pub(super) fn insert(&mut self, dep: &Dependency, con: &HashMap<PackageId, ConflictReason>) {
+
+    /// Add to the cache a conflict of the form:
+    /// `dep` is known to be unresolvable if
+    /// all the `PackageId` entries are activated
+    pub fn insert(&mut self, dep: &Dependency, con: &HashMap<PackageId, ConflictReason>) {
         let past = self.con_from_dep
             .entry(dep.clone())
             .or_insert_with(Vec::new);
@@ -85,7 +90,7 @@ impl ConflictCache {
             }
         }
     }
-    pub(super) fn get_dep_from_pid(&self, pid: &PackageId) -> Option<&HashSet<Dependency>> {
+    pub fn dependencies_conflicting_with(&self, pid: &PackageId) -> Option<&HashSet<Dependency>> {
         self.dep_from_pid.get(pid)
     }
 }

src/cargo/core/resolver/mod.rs

@@ -596,12 +596,12 @@ impl DepsFrame {
             .unwrap_or(0)
     }
 
-    fn flatten<'s>(&'s self) -> Box<Iterator<Item = (PackageId, Dependency)> + 's> {
+    fn flatten<'s>(&'s self) -> Box<Iterator<Item = (&PackageId, Dependency)> + 's> {
         // TODO: with impl Trait the Box can be removed
         Box::new(
             self.remaining_siblings
                 .clone()
-                .map(move |(_, (d, _, _))| (self.parent.package_id().clone(), d)),
+                .map(move |(_, (d, _, _))| (self.parent.package_id(), d)),
         )
     }
 }
@@ -1238,18 +1238,20 @@ fn activate_deps_loop(
                             })
                             .next()
                         {
-                            let mut conflicting = conflicting.clone();
-
-                            // If one of our deps conflicts with
+                            // If one of our deps is known unresolvable
                             // then we will not succeed.
                             // How ever if we are part of the reason that
                             // one of our deps conflicts then
                             // we can make a stronger statement
                             // because we will definitely be activated when
                             // we try our dep.
-                            conflicting.remove(&pid);
+                            conflicting_activations.extend(
+                                conflicting
+                                    .iter()
+                                    .filter(|&(p, _)| p != &pid)
+                                    .map(|(p, r)| (p.clone(), r.clone())),
+                            );
 
-                            conflicting_activations.extend(conflicting);
                             has_past_conflicting_dep = true;
                         }
                     }
@@ -1261,16 +1263,18 @@ fn activate_deps_loop(
                     // ourselves are incompatible with that dep, so we know that deps
                     // parent conflict with us.
                     if !has_past_conflicting_dep {
-                        if let Some(rel_deps) = past_conflicting_activations.get_dep_from_pid(&pid)
+                        if let Some(known_related_bad_deps) =
+                            past_conflicting_activations.dependencies_conflicting_with(&pid)
                         {
                             if let Some((other_parent, conflict)) = remaining_deps
                                 .iter()
                                 .flat_map(|other| other.flatten())
                                 // for deps related to us
-                                .filter(|&(_, ref other_dep)| rel_deps.contains(other_dep))
+                                .filter(|&(_, ref other_dep)|
+                                        known_related_bad_deps.contains(other_dep))
                                 .filter_map(|(other_parent, other_dep)| {
                                     past_conflicting_activations
-                                        .filter_conflicting(
+                                        .find_conflicting(
                                             &cx,
                                             &other_dep,
                                             |con| con.contains_key(&pid)
@@ -1279,19 +1283,22 @@ fn activate_deps_loop(
                                 })
                                 .next()
                             {
-                                let mut conflict = conflict.clone();
                                 let rel = conflict.get(&pid).unwrap().clone();
+
                                 // The conflict we found is
                                 // "other dep will not succeed if we are activated."
                                 // We want to add
                                 // "our dep will not succeed if other dep is in remaining_deps"
                                 // but that is not how the cache is set up.
                                 // So we add the less general but much faster,
                                 // "our dep will not succeed if other dep's parent is activated".
-                                conflict.insert(other_parent.clone(), rel);
-                                conflict.remove(&pid);
-
-                                conflicting_activations.extend(conflict);
+                                conflicting_activations.extend(
+                                    conflict
+                                        .iter()
+                                        .filter(|&(p, _)| p != &pid)
+                                        .map(|(p, r)| (p.clone(), r.clone())),
+                                );
+                                conflicting_activations.insert(other_parent.clone(), rel);
                                 has_past_conflicting_dep = true;
                             }
                         }

tests/testsuite/resolve.rs

@@ -825,7 +825,7 @@ fn resolving_with_constrained_cousins_backtrack() {
     reglist.push(
         pkg!((format!("level{}", DEPTH).as_str(), "1.0.0") => [dep_req("backtrack_trap0", "*"),
             dep_req("cloaking", "*")
-            ]),
+        ]),
     );
 
     let reg = registry(reglist.clone());