From 348795b5a96eff2c7ddaf9770e421af2bc93cc45 Mon Sep 17 00:00:00 2001
From: Jacob Finkelman <jfinkelm@amazon.com>
Date: Fri, 1 Dec 2023 18:12:36 +0000
Subject: [PATCH] separate function for clearer documentation

---
 src/internal/core.rs            | 42 +++++++++------------------------
 src/internal/incompatibility.rs | 29 +++++++++++++++++++++++
 2 files changed, 40 insertions(+), 31 deletions(-)

diff --git a/src/internal/core.rs b/src/internal/core.rs
index 25819c17..d2090cae 100644
--- a/src/internal/core.rs
+++ b/src/internal/core.rs
@@ -237,44 +237,24 @@ impl<P: Package, VS: VersionSet, Priority: Ord + Clone> State<P, VS, Priority> {
     /// (provided that no other version of foo exists between 1.0.0 and 2.0.0).
     /// We could collapse them into { foo (1.0.0 ∪ 1.1.0), not bar ^1.0.0 }
     /// without having to check the existence of other versions though.
-    ///
-    /// Here we do the simple stupid thing of just growing the Vec.
-    /// It may not be trivial since those incompatibilities
-    /// may already have derived others.
     fn merge_incompatibility(&mut self, mut id: IncompId<P, VS>) {
         if let Some((p1, p2)) = self.incompatibility_store[id].as_dependency() {
-            let vs = self.incompatibility_store[id].get(p2);
+            // If we are a dependency, there's a good chance we can be merged with a previous dependency
             let deps_lookup = self
                 .dependencies
                 .entry((p1.clone(), p2.clone()))
                 .or_default();
-            if let Some(past) = deps_lookup
-                .as_mut_slice()
-                .iter_mut()
-                .find(|past| self.incompatibility_store[**past].get(p2) == vs)
-            {
-                let incompat = &self.incompatibility_store[id];
-                let new = self
-                    .incompatibility_store
-                    .alloc(Incompatibility::from_dependency(
-                        p1.clone(),
-                        self.incompatibility_store[*past]
-                            .get(p1)
-                            .unwrap()
-                            .unwrap_positive()
-                            .union(incompat.get(p1).unwrap().unwrap_positive()), // It is safe to `simplify` here
-                        (
-                            &p2,
-                            incompat
-                                .get(p2)
-                                .map_or(&VS::empty(), |v| v.unwrap_negative()),
-                        ),
-                    ));
+            if let Some((past, mergeed)) = deps_lookup.as_mut_slice().iter_mut().find_map(|past| {
+                self.incompatibility_store[id]
+                    .merge_dependency(&self.incompatibility_store[*past])
+                    .map(|m| (past, m))
+            }) {
+                let new = self.incompatibility_store.alloc(mergeed);
                 for (pkg, _) in self.incompatibility_store[new].iter() {
-                    let ids = self.incompatibilities.entry(pkg.clone()).or_default();
-                    if let Some(slot) = ids.iter().position(|id| id == past) {
-                        ids.remove(slot);
-                    }
+                    self.incompatibilities
+                        .entry(pkg.clone())
+                        .or_default()
+                        .retain(|id| id != past);
                 }
                 *past = new;
                 id = new;
diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs
index 0b8ce35a..3876d797 100644
--- a/src/internal/incompatibility.rs
+++ b/src/internal/incompatibility.rs
@@ -129,6 +129,35 @@ impl<P: Package, VS: VersionSet> Incompatibility<P, VS> {
         }
     }
 
+    /// Merge two dependencies into one incompatibility.
+    ///
+    /// When there are two (or more) versions of a package that depend in the same way on a nother package,
+    /// both constraints can be stored in one incompatibility.
+    /// This can be thought of as a alternative constructor like [`from_dependency`].
+    /// Alternatively this can be thought of as a way to combine the two incompatibilities,
+    /// as a specialization of [`prior_cause`].
+    pub fn merge_dependency(&self, other: &Self) -> Option<Self> {
+        // It is almost certainly a bug to call this method without checking that self is a dependency
+        debug_assert!(self.as_dependency().is_some());
+        let self_pkgs = self.as_dependency()?;
+        if self_pkgs != other.as_dependency()? {
+            return None;
+        }
+        let (p1, p2) = self_pkgs;
+        let dep_term = self.get(p2);
+        if dep_term != other.get(p2) {
+            return None;
+        }
+        return Some(Self::from_dependency(
+            p1.clone(),
+            self.get(p1)
+                .unwrap()
+                .unwrap_positive()
+                .union(other.get(p1).unwrap().unwrap_positive()), // It is safe to `simplify` here
+            (&p2, dep_term.map_or(&VS::empty(), |v| v.unwrap_negative())),
+        ));
+    }
+
     /// Prior cause of two incompatibilities using the rule of resolution.
     pub fn prior_cause(
         incompat: Id<Self>,