feat: merge dependencies for better error messages (pubgrub-rs#163)

* bad error test

* feat: merge direct dependencies

* separate function for clearer documentation

* Update doc comments for merge_dependency

* Rename merge_dependency into merge_dependants

* Use american english dependent instead of dependant

* Rename dependencies into merged_dependencies

* Typo mergeed -> merged

---------

Co-authored-by: Matthieu Pizenberg <[email protected]>

src/internal/core.rs

@@ -30,6 +30,10 @@ pub struct State<P: Package, VS: VersionSet, Priority: Ord + Clone> {
     /// and will stay that way until the next conflict and backtrack is operated.
     contradicted_incompatibilities: rustc_hash::FxHashSet<IncompId<P, VS>>,
 
+    /// All incompatibilities expressing dependencies,
+    /// with common dependents merged.
+    merged_dependencies: Map<(P, P), SmallVec<IncompId<P, VS>>>,
+
     /// Partial solution.
     /// TODO: remove pub.
     pub partial_solution: PartialSolution<P, VS, Priority>,
@@ -61,6 +65,7 @@ impl<P: Package, VS: VersionSet, Priority: Ord + Clone> State<P, VS, Priority> {
             partial_solution: PartialSolution::empty(),
             incompatibility_store,
             unit_propagation_buffer: SmallVec::Empty,
+            merged_dependencies: Map::default(),
         }
     }
 
@@ -78,11 +83,15 @@ impl<P: Package, VS: VersionSet, Priority: Ord + Clone> State<P, VS, Priority> {
         deps: &DependencyConstraints<P, VS>,
     ) -> std::ops::Range<IncompId<P, VS>> {
         // Create incompatibilities and allocate them in the store.
-        let new_incompats_id_range = self
-            .incompatibility_store
-            .alloc_iter(deps.iter().map(|dep| {
-                Incompatibility::from_dependency(package.clone(), version.clone(), dep)
-            }));
+        let new_incompats_id_range =
+            self.incompatibility_store
+                .alloc_iter(deps.iter().map(|dep| {
+                    Incompatibility::from_dependency(
+                        package.clone(),
+                        VS::singleton(version.clone()),
+                        dep,
+                    )
+                }));
         // Merge the newly created incompatibilities with the older ones.
         for id in IncompId::range_to_iter(new_incompats_id_range.clone()) {
             self.merge_incompatibility(id);
@@ -230,11 +239,31 @@ 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, id: IncompId<P, VS>) {
+    fn merge_incompatibility(&mut self, mut id: IncompId<P, VS>) {
+        if let Some((p1, p2)) = self.incompatibility_store[id].as_dependency() {
+            // If we are a dependency, there's a good chance we can be merged with a previous dependency
+            let deps_lookup = self
+                .merged_dependencies
+                .entry((p1.clone(), p2.clone()))
+                .or_default();
+            if let Some((past, merged)) = deps_lookup.as_mut_slice().iter_mut().find_map(|past| {
+                self.incompatibility_store[id]
+                    .merge_dependents(&self.incompatibility_store[*past])
+                    .map(|m| (past, m))
+            }) {
+                let new = self.incompatibility_store.alloc(merged);
+                for (pkg, _) in self.incompatibility_store[new].iter() {
+                    self.incompatibilities
+                        .entry(pkg.clone())
+                        .or_default()
+                        .retain(|id| id != past);
+                }
+                *past = new;
+                id = new;
+            } else {
+                deps_lookup.push(id);
+            }
+        }
         for (pkg, term) in self.incompatibility_store[id].iter() {
             if cfg!(debug_assertions) {
                 assert_ne!(term, &crate::term::Term::any());

src/internal/incompatibility.rs

@@ -120,22 +120,63 @@ impl<P: Package, VS: VersionSet> Incompatibility<P, VS> {
     }
 
     /// Build an incompatibility from a given dependency.
-    pub fn from_dependency(package: P, version: VS::V, dep: (&P, &VS)) -> Self {
-        let set1 = VS::singleton(version);
+    pub fn from_dependency(package: P, versions: VS, dep: (&P, &VS)) -> Self {
         let (p2, set2) = dep;
         Self {
             package_terms: if set2 == &VS::empty() {
-                SmallMap::One([(package.clone(), Term::Positive(set1.clone()))])
+                SmallMap::One([(package.clone(), Term::Positive(versions.clone()))])
             } else {
                 SmallMap::Two([
-                    (package.clone(), Term::Positive(set1.clone())),
+                    (package.clone(), Term::Positive(versions.clone())),
                     (p2.clone(), Term::Negative(set2.clone())),
                 ])
             },
-            kind: Kind::FromDependencyOf(package, set1, p2.clone(), set2.clone()),
+            kind: Kind::FromDependencyOf(package, versions, p2.clone(), set2.clone()),
         }
     }
 
+    pub fn as_dependency(&self) -> Option<(&P, &P)> {
+        match &self.kind {
+            Kind::FromDependencyOf(p1, _, p2, _) => Some((p1, p2)),
+            _ => None,
+        }
+    }
+
+    /// Merge dependant versions with the same dependency.
+    ///
+    /// When multiple versions of a package depend on the same range of another package,
+    /// we can merge the two into a single incompatibility.
+    /// For example, if a@1 depends on b and a@2 depends on b, we can say instead
+    /// a@1 and a@b depend on b.
+    ///
+    /// It is a special case of prior cause computation where the unified package
+    /// is the common dependant in the two incompatibilities expressing dependencies.
+    pub fn merge_dependents(&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());
+        // Check that both incompatibilities are of the shape p1 depends on p2,
+        // with the same p1 and p2.
+        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);
+        // The dependency range for p2 must be the same in both case
+        // to be able to merge multiple p1 ranges.
+        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>,

src/internal/small_vec.rs

@@ -28,6 +28,15 @@ impl<T> SmallVec<T> {
         }
     }
 
+    pub fn as_mut_slice(&mut self) -> &mut [T] {
+        match self {
+            Self::Empty => &mut [],
+            Self::One(v) => v,
+            Self::Two(v) => v,
+            Self::Flexible(v) => v,
+        }
+    }
+
     pub fn push(&mut self, new: T) {
         *self = match std::mem::take(self) {
             Self::Empty => Self::One([new]),

src/term.rs

@@ -70,6 +70,15 @@ impl<VS: VersionSet> Term<VS> {
             _ => panic!("Negative term cannot unwrap positive set"),
         }
     }
+
+    /// Unwrap the set contained in a negative term.
+    /// Will panic if used on a positive set.
+    pub(crate) fn unwrap_negative(&self) -> &VS {
+        match self {
+            Self::Negative(set) => set,
+            _ => panic!("Positive term cannot unwrap negative set"),
+        }
+    }
 }
 
 /// Set operations with terms.

tests/examples.rs

@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: MPL-2.0
 
+use pubgrub::error::PubGrubError;
 use pubgrub::range::Range;
+use pubgrub::report::{DefaultStringReporter, Reporter as _};
 use pubgrub::solver::{resolve, OfflineDependencyProvider};
 use pubgrub::type_aliases::Map;
 use pubgrub::version::{NumberVersion, SemanticVersion};
@@ -209,3 +211,32 @@ fn double_choices() {
     let computed_solution = resolve(&dependency_provider, "a", 0).unwrap();
     assert_eq!(expected_solution, computed_solution);
 }
+
+#[test]
+fn confusing_with_lots_of_holes() {
+    let mut dependency_provider = OfflineDependencyProvider::<&str, NumVS>::new();
+
+    // root depends on foo...
+    dependency_provider.add_dependencies("root", 1, vec![("foo", Range::full())]);
+
+    for i in 1..6 {
+        // foo depends on bar...
+        dependency_provider.add_dependencies("foo", i as u32, vec![("bar", Range::full())]);
+    }
+
+    let Err(PubGrubError::NoSolution(mut derivation_tree)) =
+        resolve(&dependency_provider, "root", 1)
+    else {
+        unreachable!()
+    };
+    assert_eq!(
+        &DefaultStringReporter::report(&derivation_tree),
+        r#"Because there is no available version for bar and foo 1 | 2 | 3 | 4 | 5 depends on bar, foo 1 | 2 | 3 | 4 | 5 is forbidden.
+And because there is no version of foo in <1 | >1, <2 | >2, <3 | >3, <4 | >4, <5 | >5 and root 1 depends on foo, root 1 is forbidden."#
+    );
+    derivation_tree.collapse_no_versions();
+    assert_eq!(
+        &DefaultStringReporter::report(&derivation_tree),
+        "Because foo depends on bar and root 1 depends on foo, root 1 is forbidden."
+    );
+}