From 1fbd6583d8836ff75df5e116474f1642e135ca59 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Sun, 17 Dec 2023 14:20:45 -0500 Subject: [PATCH] feat: merge dependencies for better error messages (#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 --- src/internal/core.rs | 49 ++++++++++++++++++++++++------- src/internal/incompatibility.rs | 51 +++++++++++++++++++++++++++++---- src/internal/small_vec.rs | 9 ++++++ src/term.rs | 9 ++++++ tests/examples.rs | 31 ++++++++++++++++++++ 5 files changed, 134 insertions(+), 15 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index a4a23b34..d6e20866 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -30,6 +30,10 @@ pub struct State { /// and will stay that way until the next conflict and backtrack is operated. contradicted_incompatibilities: rustc_hash::FxHashSet>, + /// All incompatibilities expressing dependencies, + /// with common dependents merged. + merged_dependencies: Map<(P, P), SmallVec>>, + /// Partial solution. /// TODO: remove pub. pub partial_solution: PartialSolution, @@ -61,6 +65,7 @@ impl State { partial_solution: PartialSolution::empty(), incompatibility_store, unit_propagation_buffer: SmallVec::Empty, + merged_dependencies: Map::default(), } } @@ -78,11 +83,15 @@ impl State { deps: &DependencyConstraints, ) -> std::ops::Range> { // 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 State { /// (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) { + fn merge_incompatibility(&mut self, mut id: IncompId) { + 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()); diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index 7f4ab1be..6e58c052 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -120,22 +120,63 @@ impl Incompatibility { } /// 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 { + // 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, diff --git a/src/internal/small_vec.rs b/src/internal/small_vec.rs index fa720435..a1217ed4 100644 --- a/src/internal/small_vec.rs +++ b/src/internal/small_vec.rs @@ -28,6 +28,15 @@ impl SmallVec { } } + 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]), diff --git a/src/term.rs b/src/term.rs index 895afdfe..964e80ee 100644 --- a/src/term.rs +++ b/src/term.rs @@ -70,6 +70,15 @@ impl Term { _ => 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. diff --git a/tests/examples.rs b/tests/examples.rs index 9c11d4fe..f968bba1 100644 --- a/tests/examples.rs +++ b/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." + ); +}