From ab71a9f5abefb39a0c2bdd7df9d30078eabbae3b Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Fri, 1 Dec 2023 18:12:36 +0000 Subject: [PATCH 1/8] bad error test --- tests/examples.rs | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/tests/examples.rs b/tests/examples.rs index 9c11d4fe..2019e259 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,47 @@ 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 depends on bar, foo 1 is forbidden. +And because there is no version of foo in <1 | >1, <2 | >2, <3 | >3, <4 | >4, <5 | >5, foo <2 | >2, <3 | >3, <4 | >4, <5 | >5 is forbidden. (1) + +Because there is no available version for bar and foo 2 depends on bar, foo 2 is forbidden. +And because foo <2 | >2, <3 | >3, <4 | >4, <5 | >5 is forbidden (1), foo <3 | >3, <4 | >4, <5 | >5 is forbidden. (2) + +Because there is no available version for bar and foo 3 depends on bar, foo 3 is forbidden. +And because foo <3 | >3, <4 | >4, <5 | >5 is forbidden (2), foo <4 | >4, <5 | >5 is forbidden. (3) + +Because there is no available version for bar and foo 4 depends on bar, foo 4 is forbidden. +And because foo <4 | >4, <5 | >5 is forbidden (3), foo <5 | >5 is forbidden. (4) + +Because there is no available version for bar and foo 5 depends on bar, foo 5 is forbidden. +And because foo <5 | >5 is forbidden (4), foo * is forbidden. +And because root 1 depends on foo, root 1 is forbidden."# + ); + derivation_tree.collapse_no_versions(); + assert_eq!( + &DefaultStringReporter::report(&derivation_tree), + r#"Because foo <2 | >2, <3 | >3, <4 | >4, <5 | >5 depends on bar and foo 2 depends on bar, foo <3 | >3, <4 | >4, <5 | >5 is forbidden. +And because foo 3 depends on bar and foo 4 depends on bar, foo <5 | >5 is forbidden. +And because foo 5 depends on bar and root 1 depends on foo, root 1 is forbidden."# + ); +} From 8b4378e6498567651440d8d7fc2dd262c181e8c8 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Fri, 1 Dec 2023 18:12:36 +0000 Subject: [PATCH 2/8] feat: merge direct dependencies --- src/internal/core.rs | 59 +++++++++++++++++++++++++++++---- src/internal/incompatibility.rs | 16 ++++++--- src/internal/small_vec.rs | 9 +++++ src/term.rs | 9 +++++ tests/examples.rs | 21 ++---------- 5 files changed, 85 insertions(+), 29 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index 06e3ae21..25819c17 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -30,6 +30,8 @@ pub struct State { /// and will stay that way until the next conflict and backtrack is operated. contradicted_incompatibilities: rustc_hash::FxHashSet>, + dependencies: Map<(P, P), SmallVec>>, + /// Partial solution. /// TODO: remove pub. pub partial_solution: PartialSolution, @@ -61,6 +63,7 @@ impl State { partial_solution: PartialSolution::empty(), incompatibility_store, unit_propagation_buffer: SmallVec::Empty, + dependencies: Map::default(), } } @@ -78,11 +81,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); @@ -234,7 +241,47 @@ impl State { /// 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() { + let vs = self.incompatibility_store[id].get(p2); + 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()), + ), + )); + 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); + } + } + *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 49037e79..0b8ce35a 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -107,19 +107,25 @@ 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, } } 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 cf7aa6f7..3a9906bd 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 2019e259..f968bba1 100644 --- a/tests/examples.rs +++ b/tests/examples.rs @@ -231,27 +231,12 @@ fn confusing_with_lots_of_holes() { }; assert_eq!( &DefaultStringReporter::report(&derivation_tree), - r#"Because there is no available version for bar and foo 1 depends on bar, foo 1 is forbidden. -And because there is no version of foo in <1 | >1, <2 | >2, <3 | >3, <4 | >4, <5 | >5, foo <2 | >2, <3 | >3, <4 | >4, <5 | >5 is forbidden. (1) - -Because there is no available version for bar and foo 2 depends on bar, foo 2 is forbidden. -And because foo <2 | >2, <3 | >3, <4 | >4, <5 | >5 is forbidden (1), foo <3 | >3, <4 | >4, <5 | >5 is forbidden. (2) - -Because there is no available version for bar and foo 3 depends on bar, foo 3 is forbidden. -And because foo <3 | >3, <4 | >4, <5 | >5 is forbidden (2), foo <4 | >4, <5 | >5 is forbidden. (3) - -Because there is no available version for bar and foo 4 depends on bar, foo 4 is forbidden. -And because foo <4 | >4, <5 | >5 is forbidden (3), foo <5 | >5 is forbidden. (4) - -Because there is no available version for bar and foo 5 depends on bar, foo 5 is forbidden. -And because foo <5 | >5 is forbidden (4), foo * is forbidden. -And because root 1 depends on foo, root 1 is forbidden."# + 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), - r#"Because foo <2 | >2, <3 | >3, <4 | >4, <5 | >5 depends on bar and foo 2 depends on bar, foo <3 | >3, <4 | >4, <5 | >5 is forbidden. -And because foo 3 depends on bar and foo 4 depends on bar, foo <5 | >5 is forbidden. -And because foo 5 depends on bar and root 1 depends on foo, root 1 is forbidden."# + "Because foo depends on bar and root 1 depends on foo, root 1 is forbidden." ); } From 43be97ce182c9941a3289fff431250f84870b79a Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Fri, 1 Dec 2023 18:12:36 +0000 Subject: [PATCH 3/8] 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 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, mut id: IncompId) { 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..efed8f43 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -129,6 +129,35 @@ impl Incompatibility { } } + /// 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 [`Self::from_dependency`]. + /// Alternatively this can be thought of as a way to combine the two incompatibilities, + /// as a specialization of [`Self::prior_cause`]. + pub fn merge_dependency(&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()); + 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, From c646d3c2297dbcaa9ebc1fde742f40c70c1b84c8 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Fri, 15 Dec 2023 22:55:40 +0100 Subject: [PATCH 4/8] Update doc comments for merge_dependency --- src/internal/incompatibility.rs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index efed8f43..6b4138a2 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -129,22 +129,28 @@ impl Incompatibility { } } - /// Merge two dependencies into one incompatibility. + /// Merge dependant versions with the same dependency. /// - /// 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 [`Self::from_dependency`]. - /// Alternatively this can be thought of as a way to combine the two incompatibilities, - /// as a specialization of [`Self::prior_cause`]. + /// 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_dependency(&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; } From 4c5800ef058e0cbc17e4e4c482624c546404b9a0 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Fri, 15 Dec 2023 22:57:47 +0100 Subject: [PATCH 5/8] Rename merge_dependency into merge_dependants --- src/internal/core.rs | 2 +- src/internal/incompatibility.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index d2090cae..ae9fd46e 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -246,7 +246,7 @@ impl State { .or_default(); 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]) + .merge_dependants(&self.incompatibility_store[*past]) .map(|m| (past, m)) }) { let new = self.incompatibility_store.alloc(mergeed); diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index 6b4138a2..cbe5115d 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -138,7 +138,7 @@ impl Incompatibility { /// /// 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_dependency(&self, other: &Self) -> Option { + pub fn merge_dependants(&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, From c22f4986c24467b0d8e738f3574efce6aa16e76f Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Fri, 15 Dec 2023 23:13:54 +0100 Subject: [PATCH 6/8] Use american english dependent instead of dependant --- src/internal/core.rs | 2 +- src/internal/incompatibility.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index ae9fd46e..65287b12 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -246,7 +246,7 @@ impl State { .or_default(); if let Some((past, mergeed)) = deps_lookup.as_mut_slice().iter_mut().find_map(|past| { self.incompatibility_store[id] - .merge_dependants(&self.incompatibility_store[*past]) + .merge_dependents(&self.incompatibility_store[*past]) .map(|m| (past, m)) }) { let new = self.incompatibility_store.alloc(mergeed); diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index cbe5115d..1c8add0a 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -138,7 +138,7 @@ impl Incompatibility { /// /// 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_dependants(&self, other: &Self) -> Option { + 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, From 8a25f579a02751045aac40fed7c97dd06bad554c Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Fri, 15 Dec 2023 23:22:25 +0100 Subject: [PATCH 7/8] Rename dependencies into merged_dependencies --- src/internal/core.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index 65287b12..d0dcb2af 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -30,7 +30,9 @@ pub struct State { /// and will stay that way until the next conflict and backtrack is operated. contradicted_incompatibilities: rustc_hash::FxHashSet>, - dependencies: Map<(P, P), SmallVec>>, + /// All incompatibilities expressing dependencies, + /// with common dependents merged. + merged_dependencies: Map<(P, P), SmallVec>>, /// Partial solution. /// TODO: remove pub. @@ -63,7 +65,7 @@ impl State { partial_solution: PartialSolution::empty(), incompatibility_store, unit_propagation_buffer: SmallVec::Empty, - dependencies: Map::default(), + merged_dependencies: Map::default(), } } @@ -241,7 +243,7 @@ impl State { 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 - .dependencies + .merged_dependencies .entry((p1.clone(), p2.clone())) .or_default(); if let Some((past, mergeed)) = deps_lookup.as_mut_slice().iter_mut().find_map(|past| { From 5c4a6b92e736ebc044cc53fb5a407dfa68ca0b0f Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Fri, 15 Dec 2023 23:30:54 +0100 Subject: [PATCH 8/8] Typo mergeed -> merged --- src/internal/core.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index d0dcb2af..a8da0576 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -246,12 +246,12 @@ impl State { .merged_dependencies .entry((p1.clone(), p2.clone())) .or_default(); - if let Some((past, mergeed)) = deps_lookup.as_mut_slice().iter_mut().find_map(|past| { + 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(mergeed); + let new = self.incompatibility_store.alloc(merged); for (pkg, _) in self.incompatibility_store[new].iter() { self.incompatibilities .entry(pkg.clone())