From 0b0f97b39e418e25a3d575b14f354adaf7bc0660 Mon Sep 17 00:00:00 2001 From: Zanie Date: Fri, 1 Dec 2023 13:28:14 -0600 Subject: [PATCH 1/8] Initial divergences from upstream (#1) --- src/internal/core.rs | 3 ++- src/internal/incompatibility.rs | 8 ++++++-- src/internal/partial_solution.rs | 18 ++++++++++++++++++ src/range.rs | 2 +- src/solver.rs | 4 ++-- src/term.rs | 2 +- tests/examples.rs | 6 +++--- 7 files changed, 33 insertions(+), 10 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index 464e6691..e443b0a5 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -25,8 +25,9 @@ pub struct State { root_package: DP::P, root_version: DP::V, + /// All incompatibilities indexed by package. #[allow(clippy::type_complexity)] - incompatibilities: Map>>, + pub incompatibilities: Map>>, /// Store the ids of incompatibilities that are already contradicted. /// For each one keep track of the decision level when it was found to be contradicted. diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index 76a310bd..4d477371 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -34,14 +34,16 @@ use crate::version_set::VersionSet; #[derive(Debug, Clone)] pub struct Incompatibility { package_terms: SmallMap>, - kind: Kind, + /// The reason for the incompatibility. + pub kind: Kind, } /// Type alias of unique identifiers for incompatibilities. pub type IncompId = Id>; +/// The reason for the incompatibility. #[derive(Debug, Clone)] -enum Kind { +pub enum Kind { /// Initial incompatibility aiming at picking the root package for the first decision. /// /// This incompatibility drives the resolution, it requires that we pick the (virtual) root @@ -152,6 +154,8 @@ impl Incompatibilit } } + /// Return the two packages where this incompatibility when the incompatibility was created + /// through a dependency edge between the two. pub fn as_dependency(&self) -> Option<(&P, &P)> { match &self.kind { Kind::FromDependencyOf(p1, _, p2, _) => Some((p1, p2)), diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 3e359413..b856f883 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -262,6 +262,24 @@ impl PartialSolution { } } + pub fn prioritized_packages(&self) -> impl Iterator { + let check_all = self.changed_this_decision_level + == self.current_decision_level.0.saturating_sub(1) as usize; + let current_decision_level = self.current_decision_level; + self.package_assignments + .get_range(self.changed_this_decision_level..) + .unwrap() + .iter() + .filter(move |(_, pa)| { + // We only actually need to update the package if its Been changed + // since the last time we called prioritize. + // Which means it's highest decision level is the current decision level, + // or if we backtracked in the mean time. + check_all || pa.highest_decision_level == current_decision_level + }) + .filter_map(|(p, pa)| pa.assignments_intersection.potential_package_filter(p)) + } + pub fn pick_highest_priority_pkg( &mut self, prioritizer: impl Fn(&DP::P, &DP::VS) -> DP::Priority, diff --git a/src/range.rs b/src/range.rs index a9426b0d..9b560821 100644 --- a/src/range.rs +++ b/src/range.rs @@ -748,7 +748,7 @@ impl Display for Range { (Included(v), Unbounded) => write!(f, ">={v}")?, (Included(v), Included(b)) => { if v == b { - write!(f, "{v}")? + write!(f, "=={v}")? } else { write!(f, ">={v}, <={b}")? } diff --git a/src/solver.rs b/src/solver.rs index 766272bd..37574db5 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -68,8 +68,8 @@ use std::error::Error; use std::fmt::{Debug, Display}; use crate::error::PubGrubError; -use crate::internal::core::State; -use crate::internal::incompatibility::Incompatibility; +pub use crate::internal::core::State; +pub use crate::internal::incompatibility::{Incompatibility, Kind}; use crate::package::Package; use crate::type_aliases::{DependencyConstraints, Map, SelectedDependencies}; use crate::version_set::VersionSet; diff --git a/src/term.rs b/src/term.rs index 55ac7518..0081933c 100644 --- a/src/term.rs +++ b/src/term.rs @@ -71,7 +71,7 @@ impl Term { /// Unwrap the set contained in a positive term. /// Will panic if used on a negative set. - pub(crate) fn unwrap_positive(&self) -> &VS { + pub fn unwrap_positive(&self) -> &VS { match self { Self::Positive(set) => set, _ => panic!("Negative term cannot unwrap positive set"), diff --git a/tests/examples.rs b/tests/examples.rs index 3e9f07bd..e15d3ed7 100644 --- a/tests/examples.rs +++ b/tests/examples.rs @@ -238,13 +238,13 @@ fn confusing_with_lots_of_holes() { }; 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."# + 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." + "Because foo depends on bar and root ==1 depends on foo, root ==1 is forbidden." ); assert_eq!( derivation_tree.packages(), From cbfa22b1f5b88e9381a40fbdec7ba978c6f0721c Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Thu, 16 Nov 2023 13:19:40 -0600 Subject: [PATCH 2/8] Add GitHub workflow to automatically tag each commit on `main` (#2) --- .github/workflows/permaref.yaml | 40 +++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 .github/workflows/permaref.yaml diff --git a/.github/workflows/permaref.yaml b/.github/workflows/permaref.yaml new file mode 100644 index 00000000..29b5dbef --- /dev/null +++ b/.github/workflows/permaref.yaml @@ -0,0 +1,40 @@ +# Automatically creates a tag for each commit to `main` so when we rebase +# changes on top of the upstream, we retain permanent references to each +# previous commit so they are not orphaned and eventually deleted. +name: Create permanent reference + +on: + push: + branches: + - "main" + +jobs: + release: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Get the permanent ref number + id: get_version + run: | + # Enable pipefail so git command failures do not result in null versions downstream + set -x + + echo ::set-output name=LAST_PERMA_NUMBER::$(\ + git ls-remote --tags --refs --sort="v:refname" \ + https://github.com/zanieb/pubgrub.git | grep "tags/perma-" | tail -n1 | sed 's/.*\/perma-//' \ + ) + + - name: Configure Git + run: | + git config user.name "$GITHUB_ACTOR" + git config user.email "$GITHUB_ACTOR@users.noreply.github.com" + + - name: Create and push the new tag + run: | + TAG="perma-$((LAST_PERMA_NUMBER + 1))" + git tag -a "$TAG" -m "Automatically created on push to `main`" + git push origin "$TAG" + env: + LAST_PERMA_NUMBER: ${{ steps.get_version.outputs.LAST_PERMA_NUMBER }} From 1a70e2132cee6313efe298799180bf4feee5c4b7 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Thu, 16 Nov 2023 13:36:19 -0600 Subject: [PATCH 3/8] Fix-ups to tag generating action (#3) * Use new GitHub output syntax * Fix tag message --- .github/workflows/permaref.yaml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/permaref.yaml b/.github/workflows/permaref.yaml index 29b5dbef..eee99e6f 100644 --- a/.github/workflows/permaref.yaml +++ b/.github/workflows/permaref.yaml @@ -9,7 +9,7 @@ on: - "main" jobs: - release: + create-permaref: runs-on: ubuntu-latest steps: - name: Checkout @@ -21,10 +21,10 @@ jobs: # Enable pipefail so git command failures do not result in null versions downstream set -x - echo ::set-output name=LAST_PERMA_NUMBER::$(\ + echo "LAST_PERMA_NUMBER=$(\ git ls-remote --tags --refs --sort="v:refname" \ https://github.com/zanieb/pubgrub.git | grep "tags/perma-" | tail -n1 | sed 's/.*\/perma-//' \ - ) + )" >> $GITHUB_OUTPUT - name: Configure Git run: | @@ -34,7 +34,7 @@ jobs: - name: Create and push the new tag run: | TAG="perma-$((LAST_PERMA_NUMBER + 1))" - git tag -a "$TAG" -m "Automatically created on push to `main`" + git tag -a "$TAG" -m 'Automatically created on push to `main`' git push origin "$TAG" env: LAST_PERMA_NUMBER: ${{ steps.get_version.outputs.LAST_PERMA_NUMBER }} From a68cbd1a26e43986a31563e1d127e83bafca3a0c Mon Sep 17 00:00:00 2001 From: konstin Date: Sat, 1 Jun 2024 17:04:00 +0200 Subject: [PATCH 4/8] Merge external custom reasons --- src/report.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/report.rs b/src/report.rs index c7d3631b..b3d761a6 100644 --- a/src/report.rs +++ b/src/report.rs @@ -147,6 +147,9 @@ impl DerivationTree // // Cannot be merged because the reason may not match DerivationTree::External(External::NoVersions(_, _)) => None, + DerivationTree::External(External::Custom(_, r, reason)) => Some( + DerivationTree::External(External::Custom(package, set.union(&r), reason)), + ), DerivationTree::External(External::FromDependencyOf(p1, r1, p2, r2)) => { if p1 == package { Some(DerivationTree::External(External::FromDependencyOf( @@ -164,8 +167,6 @@ impl DerivationTree ))) } } - // Cannot be merged because the reason may not match - DerivationTree::External(External::Custom(_, _, _)) => None, } } } From b4435e2f3af10dab2336a0345b35dcd622699d06 Mon Sep 17 00:00:00 2001 From: konsti Date: Wed, 19 Jun 2024 12:55:23 +0200 Subject: [PATCH 5/8] Add `State::add_incompatibility_from_dependencies` (#27) This wrapper avoids accessing the `incompatibility_store` directly in uv code. Before: ```rust let dep_incompats = self.pubgrub.add_version( package.clone(), version.clone(), dependencies, ); self.pubgrub.partial_solution.add_version( package.clone(), version.clone(), dep_incompats, &self.pubgrub.incompatibility_store, ); ``` After: ```rust self.pubgrub.add_incompatibility_from_dependencies(package.clone(), version.clone(), dependencies); ``` `add_incompatibility_from_dependencies` is one of the main methods for the custom interface to pubgrub. --- src/internal/core.rs | 20 ++++++++++++++++++++ src/internal/partial_solution.rs | 2 +- src/solver.rs | 10 +--------- 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index e443b0a5..40587b1a 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -74,6 +74,26 @@ impl State { } } + /// Add the dependencies for the current version of the current package as incompatibilities. + pub fn add_package_version_dependencies( + &mut self, + package: DP::P, + version: DP::V, + dependencies: impl IntoIterator, + ) { + let dep_incompats = self.add_incompatibility_from_dependencies( + package.clone(), + version.clone(), + dependencies, + ); + self.partial_solution.add_package_version_incompatibilities( + package.clone(), + version.clone(), + dep_incompats, + &self.incompatibility_store, + ) + } + /// Add an incompatibility to the state. pub fn add_incompatibility(&mut self, incompat: Incompatibility) { let id = self.incompatibility_store.alloc(incompat); diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index b856f883..a50ae40d 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -370,7 +370,7 @@ impl PartialSolution { /// In practice I think it can only produce a conflict if one of the dependencies /// (which are used to make the new incompatibilities) /// is already in the partial solution with an incompatible version. - pub fn add_version( + pub(crate) fn add_package_version_incompatibilities( &mut self, package: DP::P, version: DP::V, diff --git a/src/solver.rs b/src/solver.rs index 37574db5..82e6be54 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -168,15 +168,7 @@ pub fn resolve( }; // Add that package and version if the dependencies are not problematic. - let dep_incompats = - state.add_incompatibility_from_dependencies(p.clone(), v.clone(), dependencies); - - state.partial_solution.add_version( - p.clone(), - v.clone(), - dep_incompats, - &state.incompatibility_store, - ); + state.add_package_version_dependencies(p.clone(), v.clone(), dependencies); } else { // `dep_incompats` are already in `incompatibilities` so we know there are not satisfied // terms and can add the decision directly. From 3f0ba760951ab0deeac874b98bb18fc90103fcf7 Mon Sep 17 00:00:00 2001 From: konsti Date: Mon, 15 Jul 2024 12:24:41 +0200 Subject: [PATCH 6/8] Return `NoSolutionError` from unit propagation (#28) In uv, the only pubgrub error that can occur is a `NoSolutionError`, and the only place it can occur is `unit_propagation`. By returning `NoSolutionError` instead of `PubGrubError`, we can remove `unreachable!()` calls on uv's side. `NoSolutionError` is a type alias for `DerivationTree`. --- src/error.rs | 22 ++++++++++++++++------ src/internal/core.rs | 10 ++++------ 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/error.rs b/src/error.rs index b4921f32..9306e16f 100644 --- a/src/error.rs +++ b/src/error.rs @@ -7,15 +7,19 @@ use thiserror::Error; use crate::report::DerivationTree; use crate::solver::DependencyProvider; +/// There is no solution for this set of dependencies. +pub type NoSolutionError = DerivationTree< + ::P, + ::VS, + ::M, +>; + /// Errors that may occur while solving dependencies. #[derive(Error)] -pub enum PubGrubError -where - DP: DependencyProvider, -{ +pub enum PubGrubError { /// There is no solution for this set of dependencies. #[error("No solution")] - NoSolution(DerivationTree), + NoSolution(NoSolutionError), /// Error arising when the implementer of /// [DependencyProvider] @@ -62,13 +66,19 @@ where Failure(String), } +impl From> for PubGrubError { + fn from(err: NoSolutionError) -> Self { + Self::NoSolution(err) + } +} + impl std::fmt::Debug for PubGrubError where DP: DependencyProvider, { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - Self::NoSolution(arg0) => f.debug_tuple("NoSolution").field(arg0).finish(), + Self::NoSolution(err) => f.debug_tuple("NoSolution").field(&err).finish(), Self::ErrorRetrievingDependencies { package, version, diff --git a/src/internal/core.rs b/src/internal/core.rs index 40587b1a..42acc1e5 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -6,7 +6,7 @@ use std::collections::HashSet as Set; use std::sync::Arc; -use crate::error::PubGrubError; +use crate::error::NoSolutionError; use crate::internal::arena::Arena; use crate::internal::incompatibility::{Incompatibility, Relation}; use crate::internal::partial_solution::SatisfierSearch::{ @@ -126,7 +126,7 @@ impl State { /// Unit propagation is the core mechanism of the solving algorithm. /// CF - pub fn unit_propagation(&mut self, package: DP::P) -> Result<(), PubGrubError> { + pub fn unit_propagation(&mut self, package: DP::P) -> Result<(), NoSolutionError> { self.unit_propagation_buffer.clear(); self.unit_propagation_buffer.push(package); while let Some(current_package) = self.unit_propagation_buffer.pop() { @@ -205,16 +205,14 @@ impl State { fn conflict_resolution( &mut self, incompatibility: IncompDpId, - ) -> Result<(DP::P, IncompDpId), PubGrubError> { + ) -> Result<(DP::P, IncompDpId), NoSolutionError> { let mut current_incompat_id = incompatibility; let mut current_incompat_changed = false; loop { if self.incompatibility_store[current_incompat_id] .is_terminal(&self.root_package, &self.root_version) { - return Err(PubGrubError::NoSolution( - self.build_derivation_tree(current_incompat_id), - )); + return Err(self.build_derivation_tree(current_incompat_id)); } else { let (package, satisfier_search_result) = self.partial_solution.satisfier_search( &self.incompatibility_store[current_incompat_id], From 37a67619e4adbf31db0454c9e953c05a14d7e4b9 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Thu, 18 Jul 2024 19:20:24 +0000 Subject: [PATCH 7/8] Create an arena for package names --- benches/large_case.rs | 10 +-- src/internal/arena.rs | 53 ++++++++++-- src/internal/core.rs | 61 ++++++------- src/internal/incompatibility.rs | 141 +++++++++++++++---------------- src/internal/partial_solution.rs | 85 +++++++++---------- src/internal/small_map.rs | 21 ----- src/solver.rs | 64 ++++++++------ 7 files changed, 232 insertions(+), 203 deletions(-) diff --git a/benches/large_case.rs b/benches/large_case.rs index f02e36d7..628a35f4 100644 --- a/benches/large_case.rs +++ b/benches/large_case.rs @@ -4,11 +4,11 @@ use std::time::Duration; extern crate criterion; use self::criterion::*; -use pubgrub::package::Package; -use pubgrub::range::Range; -use pubgrub::solver::{resolve, OfflineDependencyProvider}; -use pubgrub::version::SemanticVersion; -use pubgrub::version_set::VersionSet; +use pubgrub::Package; +use pubgrub::Range; +use pubgrub::SemanticVersion; +use pubgrub::VersionSet; +use pubgrub::{resolve, OfflineDependencyProvider}; use serde::de::Deserialize; fn bench<'a, P: Package + Deserialize<'a>, VS: VersionSet + Deserialize<'a>>( diff --git a/src/internal/arena.rs b/src/internal/arena.rs index ca67661f..6e3f88a9 100644 --- a/src/internal/arena.rs +++ b/src/internal/arena.rs @@ -1,9 +1,9 @@ -use std::{ - fmt, - hash::{Hash, Hasher}, - marker::PhantomData, - ops::{Index, Range}, -}; +use std::fmt; +use std::hash::{BuildHasherDefault, Hash, Hasher}; +use std::marker::PhantomData; +use std::ops::{Index, Range}; + +type FnvIndexSet = indexmap::IndexSet>; /// The index of a value allocated in an arena that holds `T`s. /// @@ -126,3 +126,44 @@ impl Index>> for Arena { &self.data[(id.start.raw as usize)..(id.end.raw as usize)] } } + +/// Yet another index-based arena. This one de-duplicates entries by hashing. +/// +/// An arena is a kind of simple grow-only allocator, backed by a `Vec` +/// where all items have the same lifetime, making it easier +/// to have references between those items. +/// In this case the `Vec` is inside a `IndexSet` allowing fast lookup by value not just index. +/// They are all dropped at once when the arena is dropped. +#[derive(Clone, PartialEq, Eq)] +pub struct HashArena { + data: FnvIndexSet, +} + +impl fmt::Debug for HashArena { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + fmt.debug_struct("Arena") + .field("len", &self.data.len()) + .field("data", &self.data) + .finish() + } +} + +impl HashArena { + pub fn new() -> Self { + HashArena { + data: FnvIndexSet::default(), + } + } + + pub fn alloc(&mut self, value: T) -> Id { + let (raw, _) = self.data.insert_full(value); + Id::from(raw as u32) + } +} + +impl Index> for HashArena { + type Output = T; + fn index(&self, id: Id) -> &T { + &self.data[id.raw as usize] + } +} diff --git a/src/internal/core.rs b/src/internal/core.rs index 42acc1e5..7f6c70ee 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -19,15 +19,17 @@ use crate::solver::DependencyProvider; use crate::type_aliases::{IncompDpId, Map}; use crate::version_set::VersionSet; +use super::arena::{HashArena, Id}; + /// Current state of the PubGrub algorithm. #[derive(Clone)] pub struct State { - root_package: DP::P, + pub root_package: Id, root_version: DP::V, /// All incompatibilities indexed by package. #[allow(clippy::type_complexity)] - pub incompatibilities: Map>>, + pub incompatibilities: Map, Vec>>, /// Store the ids of incompatibilities that are already contradicted. /// For each one keep track of the decision level when it was found to be contradicted. @@ -37,7 +39,7 @@ pub struct State { /// All incompatibilities expressing dependencies, /// with common dependents merged. #[allow(clippy::type_complexity)] - merged_dependencies: Map<(DP::P, DP::P), SmallVec>>, + merged_dependencies: Map<(Id, Id), SmallVec>>, /// Partial solution. /// TODO: remove pub. @@ -46,22 +48,27 @@ pub struct State { /// The store is the reference storage for all incompatibilities. pub incompatibility_store: Arena>, + /// The store is the reference storage for all incompatibilities. + pub package_store: HashArena, + /// This is a stack of work to be done in `unit_propagation`. /// It can definitely be a local variable to that method, but /// this way we can reuse the same allocation for better performance. - unit_propagation_buffer: SmallVec, + unit_propagation_buffer: SmallVec>, } impl State { /// Initialization of PubGrub state. pub fn init(root_package: DP::P, root_version: DP::V) -> Self { let mut incompatibility_store = Arena::new(); + let mut package_store = HashArena::new(); + let root_package = package_store.alloc(root_package); let not_root_id = incompatibility_store.alloc(Incompatibility::not_root( - root_package.clone(), + root_package, root_version.clone(), )); let mut incompatibilities = Map::default(); - incompatibilities.insert(root_package.clone(), vec![not_root_id]); + incompatibilities.insert(root_package, vec![not_root_id]); Self { root_package, root_version, @@ -69,6 +76,7 @@ impl State { contradicted_incompatibilities: Map::default(), partial_solution: PartialSolution::empty(), incompatibility_store, + package_store, unit_propagation_buffer: SmallVec::Empty, merged_dependencies: Map::default(), } @@ -103,18 +111,19 @@ impl State { /// Add an incompatibility to the state. pub fn add_incompatibility_from_dependencies( &mut self, - package: DP::P, + package: Id, version: DP::V, deps: impl IntoIterator, ) -> std::ops::Range> { // Create incompatibilities and allocate them in the store. let new_incompats_id_range = self.incompatibility_store - .alloc_iter(deps.into_iter().map(|dep| { + .alloc_iter(deps.into_iter().map(|(dep_p, dep_vs)| { + let dep_pid = self.package_store.alloc(dep_p); Incompatibility::from_dependency( - package.clone(), + package, ::singleton(version.clone()), - dep, + (dep_pid, dep_vs), ) })); // Merge the newly created incompatibilities with the older ones. @@ -126,7 +135,7 @@ impl State { /// Unit propagation is the core mechanism of the solving algorithm. /// CF - pub fn unit_propagation(&mut self, package: DP::P) -> Result<(), NoSolutionError> { + pub fn unit_propagation(&mut self, package: Id) -> Result<(), NoSolutionError> { self.unit_propagation_buffer.clear(); self.unit_propagation_buffer.push(package); while let Some(current_package) = self.unit_propagation_buffer.pop() { @@ -147,7 +156,7 @@ impl State { // we must perform conflict resolution. Relation::Satisfied => { log::info!( - "Start conflict resolution because incompat satisfied:\n {}", + "Start conflict resolution because incompat satisfied:\n {:?}", current_incompat ); conflict_id = Some(incompat_id); @@ -160,7 +169,7 @@ impl State { // In practice the most common pathology is adding the same package repeatedly. // So we only check if it is duplicated with the last item. if self.unit_propagation_buffer.last() != Some(&package_almost) { - self.unit_propagation_buffer.push(package_almost.clone()); + self.unit_propagation_buffer.push(package_almost); } // Add (not term) to the partial solution with incompat as cause. self.partial_solution.add_derivation( @@ -182,7 +191,7 @@ impl State { if let Some(incompat_id) = conflict_id { let (package_almost, root_cause) = self.conflict_resolution(incompat_id)?; self.unit_propagation_buffer.clear(); - self.unit_propagation_buffer.push(package_almost.clone()); + self.unit_propagation_buffer.push(package_almost); // Add to the partial solution with incompat as cause. self.partial_solution.add_derivation( package_almost, @@ -205,12 +214,12 @@ impl State { fn conflict_resolution( &mut self, incompatibility: IncompDpId, - ) -> Result<(DP::P, IncompDpId), NoSolutionError> { + ) -> Result<(Id, IncompDpId), NoSolutionError> { let mut current_incompat_id = incompatibility; let mut current_incompat_changed = false; loop { if self.incompatibility_store[current_incompat_id] - .is_terminal(&self.root_package, &self.root_version) + .is_terminal(self.root_package, &self.root_version) { return Err(self.build_derivation_tree(current_incompat_id)); } else { @@ -222,7 +231,6 @@ impl State { DifferentDecisionLevels { previous_satisfier_level, } => { - let package = package.clone(); self.backtrack( current_incompat_id, current_incompat_changed, @@ -238,7 +246,7 @@ impl State { package, &self.incompatibility_store, ); - log::info!("prior cause: {}", prior_cause); + log::info!("prior cause: {:?}", prior_cause); current_incompat_id = self.incompatibility_store.alloc(prior_cause); current_incompat_changed = true; } @@ -281,19 +289,16 @@ impl State { fn merge_incompatibility(&mut self, mut id: IncompDpId) { 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(); + let deps_lookup = self.merged_dependencies.entry((p1, p2)).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() { + for (&pkg, _) in self.incompatibility_store[new].iter() { self.incompatibilities - .entry(pkg.clone()) + .entry(pkg) .or_default() .retain(|id| id != past); } @@ -303,14 +308,11 @@ impl State { deps_lookup.push(id); } } - for (pkg, term) in self.incompatibility_store[id].iter() { + for (&pkg, term) in self.incompatibility_store[id].iter() { if cfg!(debug_assertions) { assert_ne!(term, &crate::term::Term::any()); } - self.incompatibilities - .entry(pkg.clone()) - .or_default() - .push(id); + self.incompatibilities.entry(pkg).or_default().push(id); } } @@ -345,6 +347,7 @@ impl State { id, &shared_ids, &self.incompatibility_store, + &self.package_store, &precomputed, ); precomputed.insert(id, Arc::new(tree)); diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index 4d477371..22fb12d2 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -3,19 +3,19 @@ //! An incompatibility is a set of terms for different packages //! that should never be satisfied all together. -use std::fmt::{self, Debug, Display}; +use std::fmt::{Debug, Display}; use std::sync::Arc; use crate::internal::arena::{Arena, Id}; use crate::internal::small_map::SmallMap; use crate::package::Package; -use crate::report::{ - DefaultStringReportFormatter, DerivationTree, Derived, External, ReportFormatter, -}; +use crate::report::{DerivationTree, Derived, External}; use crate::term::{self, Term}; use crate::type_aliases::{Map, Set}; use crate::version_set::VersionSet; +use super::arena::HashArena; + /// An incompatibility is a set of terms for different packages /// that should never be satisfied all together. /// An incompatibility usually originates from a package dependency. @@ -33,7 +33,7 @@ use crate::version_set::VersionSet; /// [PubGrub documentation](https://github.com/dart-lang/pub/blob/master/doc/solver.md#incompatibility). #[derive(Debug, Clone)] pub struct Incompatibility { - package_terms: SmallMap>, + package_terms: SmallMap, Term>, /// The reason for the incompatibility. pub kind: Kind, } @@ -48,12 +48,12 @@ pub enum Kind { /// /// This incompatibility drives the resolution, it requires that we pick the (virtual) root /// packages. - NotRoot(P, VS::V), + NotRoot(Id

, VS::V), /// There are no versions in the given range for this package. /// /// This incompatibility is used when we tried all versions in a range and no version /// worked, so we have to backtrack - NoVersions(P, VS), + NoVersions(Id

, VS), /// Incompatibility coming from the dependencies of a given package. /// /// If a@1 depends on b>=1,<2, we create an incompatibility with terms `{a 1, b <1,>=2}` with @@ -61,7 +61,7 @@ pub enum Kind { /// /// We can merge multiple dependents with the same version. For example, if a@1 depends on b and /// a@2 depends on b, we can say instead a@1||2 depends on b. - FromDependencyOf(P, VS, P, VS), + FromDependencyOf(Id

, VS, Id

, VS), /// Derived from two causes. Stores cause ids. /// /// For example, if a -> b and b -> c, we can derive a -> c. @@ -71,7 +71,7 @@ pub enum Kind { /// Examples: /// * The version would require building the package, but builds are disabled. /// * The package is not available in the cache, but internet access has been disabled. - Custom(P, VS, M), + Custom(Id

, VS, M), } /// A Relation describes how a set of terms can be compared to an incompatibility. @@ -83,20 +83,20 @@ pub enum Relation { Satisfied, /// We say that S contradicts I /// if S contradicts at least one term in I. - Contradicted(P), + Contradicted(Id

), /// If S satisfies all but one of I's terms and is inconclusive for the remaining term, /// we say S "almost satisfies" I and we call the remaining term the "unsatisfied term". - AlmostSatisfied(P), + AlmostSatisfied(Id

), /// Otherwise, we say that their relation is inconclusive. Inconclusive, } impl Incompatibility { /// Create the initial "not Root" incompatibility. - pub fn not_root(package: P, version: VS::V) -> Self { + pub fn not_root(package: Id

, version: VS::V) -> Self { Self { package_terms: SmallMap::One([( - package.clone(), + package, Term::Negative(VS::singleton(version.clone())), )]), kind: Kind::NotRoot(package, version), @@ -104,50 +104,50 @@ impl Incompatibilit } /// Create an incompatibility to remember that a given set does not contain any version. - pub fn no_versions(package: P, term: Term) -> Self { + pub fn no_versions(package: Id

, term: Term) -> Self { let set = match &term { Term::Positive(r) => r.clone(), Term::Negative(_) => panic!("No version should have a positive term"), }; Self { - package_terms: SmallMap::One([(package.clone(), term)]), + package_terms: SmallMap::One([(package, term)]), kind: Kind::NoVersions(package, set), } } /// Create an incompatibility for a reason outside pubgrub. #[allow(dead_code)] // Used by uv - pub fn custom_term(package: P, term: Term, metadata: M) -> Self { + pub fn custom_term(package: Id

, term: Term, metadata: M) -> Self { let set = match &term { Term::Positive(r) => r.clone(), Term::Negative(_) => panic!("No version should have a positive term"), }; Self { - package_terms: SmallMap::One([(package.clone(), term)]), + package_terms: SmallMap::One([(package, term)]), kind: Kind::Custom(package, set, metadata), } } /// Create an incompatibility for a reason outside pubgrub. - pub fn custom_version(package: P, version: VS::V, metadata: M) -> Self { + pub fn custom_version(package: Id

, version: VS::V, metadata: M) -> Self { let set = VS::singleton(version); let term = Term::Positive(set.clone()); Self { - package_terms: SmallMap::One([(package.clone(), term)]), + package_terms: SmallMap::One([(package, term)]), kind: Kind::Custom(package, set, metadata), } } /// Build an incompatibility from a given dependency. - pub fn from_dependency(package: P, versions: VS, dep: (P, VS)) -> Self { + pub fn from_dependency(package: Id

, versions: VS, dep: (Id

, VS)) -> Self { let (p2, set2) = dep; Self { package_terms: if set2 == VS::empty() { - SmallMap::One([(package.clone(), Term::Positive(versions.clone()))]) + SmallMap::One([(package, Term::Positive(versions.clone()))]) } else { SmallMap::Two([ - (package.clone(), Term::Positive(versions.clone())), - (p2.clone(), Term::Negative(set2.clone())), + (package, Term::Positive(versions.clone())), + (p2, Term::Negative(set2.clone())), ]) }, kind: Kind::FromDependencyOf(package, versions, p2, set2), @@ -156,9 +156,9 @@ impl Incompatibilit /// Return the two packages where this incompatibility when the incompatibility was created /// through a dependency edge between the two. - pub fn as_dependency(&self) -> Option<(&P, &P)> { + pub fn as_dependency(&self) -> Option<(Id

, Id

)> { match &self.kind { - Kind::FromDependencyOf(p1, _, p2, _) => Some((p1, p2)), + Kind::FromDependencyOf(p1, _, p2, _) => Some((*p1, *p2)), _ => None, } } @@ -189,13 +189,13 @@ impl Incompatibilit return None; } return Some(Self::from_dependency( - p1.clone(), + p1, self.get(p1) .unwrap() .unwrap_positive() .union(other.get(p1).unwrap().unwrap_positive()), // It is safe to `simplify` here ( - p2.clone(), + p2, dep_term.map_or(VS::empty(), |v| v.unwrap_negative().clone()), ), )); @@ -205,23 +205,23 @@ impl Incompatibilit pub fn prior_cause( incompat: Id, satisfier_cause: Id, - package: &P, + package: Id

, incompatibility_store: &Arena, ) -> Self { let kind = Kind::DerivedFrom(incompat, satisfier_cause); // Optimization to avoid cloning and dropping t1 let (t1, mut package_terms) = incompatibility_store[incompat] .package_terms - .split_one(package) + .split_one(&package) .unwrap(); let satisfier_cause_terms = &incompatibility_store[satisfier_cause].package_terms; package_terms.merge( - satisfier_cause_terms.iter().filter(|(p, _)| p != &package), + satisfier_cause_terms.iter().filter(|(p, _)| p != &&package), |t1, t2| Some(t1.intersection(t2)), ); - let term = t1.union(satisfier_cause_terms.get(package).unwrap()); + let term = t1.union(satisfier_cause_terms.get(&package).unwrap()); if term != Term::any() { - package_terms.insert(package.clone(), term); + package_terms.insert(package, term); } Self { package_terms, @@ -231,24 +231,24 @@ impl Incompatibilit /// Check if an incompatibility should mark the end of the algorithm /// because it satisfies the root package. - pub fn is_terminal(&self, root_package: &P, root_version: &VS::V) -> bool { + pub fn is_terminal(&self, root_package: Id

, root_version: &VS::V) -> bool { if self.package_terms.len() == 0 { true } else if self.package_terms.len() > 1 { false } else { let (package, term) = self.package_terms.iter().next().unwrap(); - (package == root_package) && term.contains(root_version) + (package == &root_package) && term.contains(root_version) } } /// Get the term related to a given package (if it exists). - pub fn get(&self, package: &P) -> Option<&Term> { - self.package_terms.get(package) + pub fn get(&self, package: Id

) -> Option<&Term> { + self.package_terms.get(&package) } /// Iterate over packages. - pub fn iter(&self) -> impl Iterator)> { + pub fn iter(&self) -> impl Iterator, &Term)> { self.package_terms.iter() } @@ -267,12 +267,17 @@ impl Incompatibilit self_id: Id, shared_ids: &Set>, store: &Arena, + package_store: &HashArena

, precomputed: &Map, Arc>>, ) -> DerivationTree { match store[self_id].kind.clone() { Kind::DerivedFrom(id1, id2) => { - let derived = Derived { - terms: store[self_id].package_terms.as_map(), + let derived: Derived = Derived { + terms: store[self_id] + .package_terms + .iter() + .map(|(&a, b)| (package_store[a].clone(), b.clone())) + .collect(), shared_id: shared_ids.get(&self_id).map(|id| id.into_raw()), cause1: precomputed .get(&id1) @@ -286,21 +291,22 @@ impl Incompatibilit DerivationTree::Derived(derived) } Kind::NotRoot(package, version) => { - DerivationTree::External(External::NotRoot(package, version)) - } - Kind::NoVersions(package, set) => { - DerivationTree::External(External::NoVersions(package.clone(), set.clone())) + DerivationTree::External(External::NotRoot(package_store[package].clone(), version)) } + Kind::NoVersions(package, set) => DerivationTree::External(External::NoVersions( + package_store[package].clone(), + set.clone(), + )), Kind::FromDependencyOf(package, set, dep_package, dep_set) => { DerivationTree::External(External::FromDependencyOf( - package.clone(), + package_store[package].clone(), set.clone(), - dep_package.clone(), + package_store[dep_package].clone(), dep_set.clone(), )) } Kind::Custom(package, set, metadata) => DerivationTree::External(External::Custom( - package.clone(), + package_store[package].clone(), set.clone(), metadata.clone(), )), @@ -312,13 +318,13 @@ impl<'a, P: Package, VS: VersionSet + 'a, M: Eq + Clone + Debug + Display + 'a> Incompatibility { /// CF definition of Relation enum. - pub fn relation(&self, terms: impl Fn(&P) -> Option<&'a Term>) -> Relation

{ + pub fn relation(&self, terms: impl Fn(Id

) -> Option<&'a Term>) -> Relation

{ let mut relation = Relation::Satisfied; - for (package, incompat_term) in self.package_terms.iter() { + for (&package, incompat_term) in self.package_terms.iter() { match terms(package).map(|term| incompat_term.relation_with(term)) { Some(term::Relation::Satisfied) => {} Some(term::Relation::Contradicted) => { - return Relation::Contradicted(package.clone()); + return Relation::Contradicted(package); } None | Some(term::Relation::Inconclusive) => { // If a package is not present, the intersection is the same as [Term::any]. @@ -327,7 +333,7 @@ impl<'a, P: Package, VS: VersionSet + 'a, M: Eq + Clone + Debug + Display + 'a> // but we systematically remove those from incompatibilities // so we're safe on that front. if relation == Relation::Satisfied { - relation = Relation::AlmostSatisfied(package.clone()); + relation = Relation::AlmostSatisfied(package); } else { return Relation::Inconclusive; } @@ -338,21 +344,6 @@ impl<'a, P: Package, VS: VersionSet + 'a, M: Eq + Clone + Debug + Display + 'a> } } -impl fmt::Display - for Incompatibility -{ - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!( - f, - "{}", - ReportFormatter::::format_terms( - &DefaultStringReportFormatter, - &self.package_terms.as_map() - ) - ) - } -} - // TESTS ####################################################################### #[cfg(test)] @@ -374,22 +365,26 @@ pub mod tests { #[test] fn rule_of_resolution(t1 in term_strat(), t2 in term_strat(), t3 in term_strat()) { let mut store = Arena::new(); + let mut package_store = HashArena::new(); + let p1 = package_store.alloc("p1"); + let p2 = package_store.alloc("p2"); + let p3 = package_store.alloc("p3"); let i1 = store.alloc(Incompatibility { - package_terms: SmallMap::Two([("p1", t1.clone()), ("p2", t2.negate())]), - kind: Kind::<_, _, String>::FromDependencyOf("p1", Range::full(), "p2", Range::full()) + package_terms: SmallMap::Two([(p1, t1.clone()), (p2, t2.negate())]), + kind: Kind::<_, _, String>::FromDependencyOf(p1, Range::full(), p2, Range::full()) }); let i2 = store.alloc(Incompatibility { - package_terms: SmallMap::Two([("p2", t2), ("p3", t3.clone())]), - kind: Kind::<_, _, String>::FromDependencyOf("p2", Range::full(), "p3", Range::full()) + package_terms: SmallMap::Two([(p2, t2), (p3, t3.clone())]), + kind: Kind::<_, _, String>::FromDependencyOf(p2, Range::full(), p3, Range::full()) }); let mut i3 = Map::default(); - i3.insert("p1", t1); - i3.insert("p3", t3); + i3.insert(p1, t1); + i3.insert(p3, t3); - let i_resolution = Incompatibility::prior_cause(i1, i2, &"p2", &store); - assert_eq!(i_resolution.package_terms.as_map(), i3); + let i_resolution = Incompatibility::prior_cause(i1, i2, p2, &store); + assert_eq!(i_resolution.package_terms.iter().map(|(&k, v)|(k, v.clone())).collect::>(), i3); } } diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index a50ae40d..223b949a 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -15,9 +15,10 @@ use crate::internal::small_map::SmallMap; use crate::package::Package; use crate::solver::DependencyProvider; use crate::term::Term; -use crate::type_aliases::{IncompDpId, SelectedDependencies}; +use crate::type_aliases::IncompDpId; use crate::version_set::VersionSet; +use super::arena::Id; use super::small_vec::SmallVec; type FnvIndexMap = indexmap::IndexMap>; @@ -48,11 +49,11 @@ pub struct PartialSolution { /// the last time `prioritize` has bean called. The inverse is not necessarily true, some packages in the range /// did not have a change. Within this range there is no sorting. #[allow(clippy::type_complexity)] - package_assignments: FnvIndexMap>, + package_assignments: FnvIndexMap, PackageAssignments>, /// `prioritized_potential_packages` is primarily a HashMap from a package with no desition and a positive assignment /// to its `Priority`. But, it also maintains a max heap of packages by `Priority` order. prioritized_potential_packages: - PriorityQueue>, + PriorityQueue, DP::Priority, BuildHasherDefault>, changed_this_decision_level: usize, } @@ -61,7 +62,7 @@ impl Display for PartialSolution { let mut assignments: Vec<_> = self .package_assignments .iter() - .map(|(p, pa)| format!("{}: {}", p, pa)) + .map(|(p, pa)| format!("{:?}: {}", p, pa)) .collect(); assignments.sort(); write!( @@ -148,7 +149,7 @@ pub enum SatisfierSearch = SmallMap<&'i P, (Option>, u32, DecisionLevel)>; +type SatisfiedMap = SmallMap, (Option>, u32, DecisionLevel)>; impl PartialSolution { /// Initialize an empty PartialSolution. @@ -163,7 +164,7 @@ impl PartialSolution { } /// Add a decision. - pub fn add_decision(&mut self, package: DP::P, version: DP::V) { + pub fn add_decision(&mut self, package: Id, version: DP::V) { // Check that add_decision is never used in the wrong context. if cfg!(debug_assertions) { match self.package_assignments.get_mut(&package) { @@ -175,7 +176,7 @@ impl PartialSolution { AssignmentsIntersection::Derivations(term) => { debug_assert!( term.contains(&version), - "{}: {} was expected to be contained in {}", + "{:?}: {} was expected to be contained in {}", package, version, term, @@ -210,7 +211,7 @@ impl PartialSolution { /// Add a derivation. pub fn add_derivation( &mut self, - package: DP::P, + package: Id, cause: IncompDpId, store: &Arena>, ) { @@ -219,7 +220,7 @@ impl PartialSolution { global_index: self.next_global_index, decision_level: self.current_decision_level, cause, - accumulated_intersection: store[cause].get(&package).unwrap().negate(), + accumulated_intersection: store[cause].get(package).unwrap().negate(), }; self.next_global_index += 1; let pa_last_index = self.package_assignments.len().saturating_sub(1); @@ -282,8 +283,8 @@ impl PartialSolution { pub fn pick_highest_priority_pkg( &mut self, - prioritizer: impl Fn(&DP::P, &DP::VS) -> DP::Priority, - ) -> Option { + prioritizer: impl Fn(Id, &DP::VS) -> DP::Priority, + ) -> Option> { let check_all = self.changed_this_decision_level == self.current_decision_level.0.saturating_sub(1) as usize; let current_decision_level = self.current_decision_level; @@ -299,10 +300,10 @@ impl PartialSolution { // or if we backtracked in the mean time. check_all || pa.highest_decision_level == current_decision_level }) - .filter_map(|(p, pa)| pa.assignments_intersection.potential_package_filter(p)) + .filter_map(|(&p, pa)| pa.assignments_intersection.potential_package_filter(p)) .for_each(|(p, r)| { let priority = prioritizer(p, r); - prioritized_potential_packages.push(p.clone(), priority); + prioritized_potential_packages.push(p, priority); }); self.changed_this_decision_level = self.package_assignments.len(); prioritized_potential_packages.pop().map(|(p, _)| p) @@ -311,23 +312,22 @@ impl PartialSolution { /// If a partial solution has, for every positive derivation, /// a corresponding decision that satisfies that assignment, /// it's a total solution and version solving has succeeded. - pub fn extract_solution(&self) -> SelectedDependencies { + pub fn extract_solution(&self) -> impl Iterator, DP::V)> + '_ { self.package_assignments .iter() .take(self.current_decision_level.0 as usize) - .map(|(p, pa)| match &pa.assignments_intersection { - AssignmentsIntersection::Decision((_, v, _)) => (p.clone(), v.clone()), + .map(|(&p, pa)| match &pa.assignments_intersection { + AssignmentsIntersection::Decision((_, v, _)) => (p, v.clone()), AssignmentsIntersection::Derivations(_) => { panic!("Derivations in the Decision part") } }) - .collect() } /// Backtrack the partial solution to a given decision level. pub fn backtrack(&mut self, decision_level: DecisionLevel) { self.current_decision_level = decision_level; - self.package_assignments.retain(|_p, pa| { + self.package_assignments.retain(|_, pa| { if pa.smallest_decision_level > decision_level { // Remove all entries that have a smallest decision level higher than the backtrack target. false @@ -372,7 +372,7 @@ impl PartialSolution { /// is already in the partial solution with an incompatible version. pub(crate) fn add_package_version_incompatibilities( &mut self, - package: DP::P, + package: Id, version: DP::V, new_incompatibilities: std::ops::Range>, store: &Arena>, @@ -380,7 +380,7 @@ impl PartialSolution { let exact = Term::exact(version.clone()); let not_satisfied = |incompat: &Incompatibility| { incompat.relation(|p| { - if p == &package { + if p == package { Some(&exact) } else { self.term_intersection_for_package(p) @@ -391,11 +391,11 @@ impl PartialSolution { // Check none of the dependencies (new_incompatibilities) // would create a conflict (be satisfied). if store[new_incompatibilities].iter().all(not_satisfied) { - log::info!("add_decision: {} @ {}", package, version); + log::info!("add_decision: {:?} @ {}", package, version); self.add_decision(package, version); } else { log::info!( - "not adding {} @ {} because of its dependencies", + "not adding {:?} @ {} because of its dependencies", package, version ); @@ -408,19 +408,19 @@ impl PartialSolution { } /// Retrieve intersection of terms related to package. - pub fn term_intersection_for_package(&self, package: &DP::P) -> Option<&Term> { + pub fn term_intersection_for_package(&self, package: Id) -> Option<&Term> { self.package_assignments - .get(package) + .get(&package) .map(|pa| pa.assignments_intersection.term()) } /// Figure out if the satisfier and previous satisfier are of different decision levels. #[allow(clippy::type_complexity)] - pub fn satisfier_search<'i>( + pub fn satisfier_search( &self, - incompat: &'i Incompatibility, + incompat: &Incompatibility, store: &Arena>, - ) -> (&'i DP::P, SatisfierSearch) { + ) -> (Id, SatisfierSearch) { let satisfied_map = Self::find_satisfier(incompat, &self.package_assignments); let (&satisfier_package, &(satisfier_cause, _, satisfier_decision_level)) = satisfied_map .iter() @@ -455,13 +455,13 @@ impl PartialSolution { /// It would be nice if we could get rid of it, but I don't know if then it will be possible /// to return a coherent previous_satisfier_level. #[allow(clippy::type_complexity)] - fn find_satisfier<'i>( - incompat: &'i Incompatibility, - package_assignments: &FnvIndexMap>, - ) -> SatisfiedMap<'i, DP::P, DP::VS, DP::M> { + fn find_satisfier( + incompat: &Incompatibility, + package_assignments: &FnvIndexMap, PackageAssignments>, + ) -> SatisfiedMap { let mut satisfied = SmallMap::Empty; - for (package, incompat_term) in incompat.iter() { - let pa = package_assignments.get(package).expect("Must exist"); + for (&package, incompat_term) in incompat.iter() { + let pa = package_assignments.get(&package).expect("Must exist"); satisfied.insert(package, pa.satisfier(package, &incompat_term.negate())); } satisfied @@ -471,15 +471,15 @@ impl PartialSolution { /// such that incompatibility is satisfied by the partial solution up to /// and including that assignment plus satisfier. #[allow(clippy::type_complexity)] - fn find_previous_satisfier<'i>( + fn find_previous_satisfier( incompat: &Incompatibility, - satisfier_package: &'i DP::P, - mut satisfied_map: SatisfiedMap<'i, DP::P, DP::VS, DP::M>, - package_assignments: &FnvIndexMap>, + satisfier_package: Id, + mut satisfied_map: SatisfiedMap, + package_assignments: &FnvIndexMap, PackageAssignments>, store: &Arena>, ) -> DecisionLevel { // First, let's retrieve the previous derivations and the initial accum_term. - let satisfier_pa = package_assignments.get(satisfier_package).unwrap(); + let satisfier_pa = package_assignments.get(&satisfier_package).unwrap(); let (satisfier_cause, _gidx, _dl) = satisfied_map.get(&satisfier_package).unwrap(); let accum_term = if let &Some(cause) = satisfier_cause { @@ -519,7 +519,7 @@ impl PartialSolution { impl PackageAssignments { fn satisfier( &self, - package: &P, + package: Id

, start_term: &Term, ) -> (Option>, u32, DecisionLevel) { let empty = Term::empty(); @@ -541,7 +541,7 @@ impl PackageAssignm AssignmentsIntersection::Derivations(accumulated_intersection) => { unreachable!( concat!( - "while processing package {}: ", + "while processing package {:?}: ", "accum_term = {} has overlap with incompat_term = {}, ", "which means the last assignment should have been a decision, ", "but instead it was a derivation. This shouldn't be possible! ", @@ -567,10 +567,7 @@ impl AssignmentsIntersection { /// selected version (no "decision") /// and if it contains at least one positive derivation term /// in the partial solution. - fn potential_package_filter<'a, P: Package>( - &'a self, - package: &'a P, - ) -> Option<(&'a P, &'a VS)> { + fn potential_package_filter(&self, package: Id

) -> Option<(Id

, &VS)> { match self { Self::Decision(_) => None, Self::Derivations(term_intersection) => { diff --git a/src/internal/small_map.rs b/src/internal/small_map.rs index a2091fc1..89b48ecd 100644 --- a/src/internal/small_map.rs +++ b/src/internal/small_map.rs @@ -189,27 +189,6 @@ impl SmallMap { } } -impl SmallMap { - pub fn as_map(&self) -> Map { - match self { - Self::Empty => Map::default(), - Self::One([(k, v)]) => { - let mut map = Map::with_capacity_and_hasher(1, Default::default()); - map.insert(k.clone(), v.clone()); - map - } - Self::Two(data) => { - let mut map = Map::with_capacity_and_hasher(2, Default::default()); - for (k, v) in data { - map.insert(k.clone(), v.clone()); - } - map - } - Self::Flexible(data) => data.clone(), - } - } -} - enum IterSmallMap<'a, K, V> { Inline(std::slice::Iter<'a, (K, V)>), Map(std::collections::hash_map::Iter<'a, K, V>), diff --git a/src/solver.rs b/src/solver.rs index 82e6be54..2eb0c032 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -70,6 +70,7 @@ use std::fmt::{Debug, Display}; use crate::error::PubGrubError; pub use crate::internal::core::State; pub use crate::internal::incompatibility::{Incompatibility, Kind}; +use crate::internal::arena::Id; use crate::package::Package; use crate::type_aliases::{DependencyConstraints, Map, SelectedDependencies}; use crate::version_set::VersionSet; @@ -83,14 +84,14 @@ pub fn resolve( version: impl Into, ) -> Result, PubGrubError> { let mut state: State = State::init(package.clone(), version.into()); - let mut added_dependencies: Map> = Map::default(); - let mut next = package; + let mut added_dependencies: Map, Set> = Map::default(); + let mut next = state.root_package; loop { dependency_provider .should_cancel() - .map_err(PubGrubError::ErrorInShouldCancel)?; + .map_err(|err| PubGrubError::ErrorInShouldCancel(err))?; - info!("unit_propagation: {}", &next); + info!("unit_propagation: {:?}", &next); state.unit_propagation(next)?; debug!( @@ -98,29 +99,37 @@ pub fn resolve( state.partial_solution ); - let Some(highest_priority_pkg) = state - .partial_solution - .pick_highest_priority_pkg(|p, r| dependency_provider.prioritize(p, r)) + let Some(highest_priority_pkg) = + state.partial_solution.pick_highest_priority_pkg(|p, r| { + dependency_provider.prioritize(&state.package_store[p], r) + }) else { - return Ok(state.partial_solution.extract_solution()); + return Ok(state + .partial_solution + .extract_solution() + .map(|(p, v)| (state.package_store[p].clone(), v)) + .collect()); }; next = highest_priority_pkg; let term_intersection = state .partial_solution - .term_intersection_for_package(&next) + .term_intersection_for_package(next) .ok_or_else(|| { PubGrubError::Failure("a package was chosen but we don't have a term.".into()) })?; let decision = dependency_provider - .choose_version(&next, term_intersection.unwrap_positive()) + .choose_version( + &state.package_store[next], + term_intersection.unwrap_positive(), + ) .map_err(PubGrubError::ErrorChoosingPackageVersion)?; - info!("DP chose: {} @ {:?}", next, decision); + info!("DP chose: {:?} @ {:?}", next, decision); // Pick the next compatible version. let v = match decision { None => { - let inc = Incompatibility::no_versions(next.clone(), term_intersection.clone()); + let inc = Incompatibility::no_versions(next, term_intersection.clone()); state.add_incompatibility(inc); continue; } @@ -134,33 +143,33 @@ pub fn resolve( } let is_new_dependency = added_dependencies - .entry(next.clone()) + .entry(next) .or_default() .insert(v.clone()); if is_new_dependency { // Retrieve that package dependencies. - let p = &next; - let dependencies = dependency_provider.get_dependencies(p, &v).map_err(|err| { - PubGrubError::ErrorRetrievingDependencies { - package: p.clone(), + let p = next; + let dependencies = dependency_provider + .get_dependencies(&state.package_store[p], &v) + .map_err(|err| PubGrubError::ErrorRetrievingDependencies { + package: state.package_store[p].clone(), version: v.clone(), source: err, - } - })?; + })?; let dependencies = match dependencies { Dependencies::Unavailable(reason) => { state.add_incompatibility(Incompatibility::custom_version( - p.clone(), + p, v.clone(), reason, )); continue; } - Dependencies::Available(x) if x.contains_key(p) => { + Dependencies::Available(x) if x.contains_key(&state.package_store[p]) => { return Err(PubGrubError::SelfDependency { - package: p.clone(), + package: state.package_store[p].clone(), version: v, }); } @@ -168,12 +177,17 @@ pub fn resolve( }; // Add that package and version if the dependencies are not problematic. - state.add_package_version_dependencies(p.clone(), v.clone(), dependencies); + let dep_incompats = + state.add_incompatibility_from_dependencies(p, v.clone(), dependencies); + + state + .partial_solution + .add_version(p, v, dep_incompats, &state.incompatibility_store); } else { // `dep_incompats` are already in `incompatibilities` so we know there are not satisfied // terms and can add the decision directly. - info!("add_decision (not first time): {} @ {}", &next, v); - state.partial_solution.add_decision(next.clone(), v); + info!("add_decision (not first time): {:?} @ {}", &next, v); + state.partial_solution.add_decision(next, v); } } } From 181458065f056e52bb581c1dc98f3dcb0e593651 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 22 Jul 2024 21:53:50 -0400 Subject: [PATCH 8/8] Hacks --- src/internal/arena.rs | 6 ++ src/internal/core.rs | 11 +-- src/internal/partial_solution.rs | 8 +- src/solver.rs | 124 +------------------------------ 4 files changed, 17 insertions(+), 132 deletions(-) diff --git a/src/internal/arena.rs b/src/internal/arena.rs index 6e3f88a9..59e58e3a 100644 --- a/src/internal/arena.rs +++ b/src/internal/arena.rs @@ -49,6 +49,12 @@ impl fmt::Debug for Id { } } +impl fmt::Display for Id { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.raw) + } +} + impl Id { pub fn into_raw(self) -> usize { self.raw as usize diff --git a/src/internal/core.rs b/src/internal/core.rs index 7f6c70ee..1bbc11f6 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -85,17 +85,14 @@ impl State { /// Add the dependencies for the current version of the current package as incompatibilities. pub fn add_package_version_dependencies( &mut self, - package: DP::P, + package: Id, version: DP::V, dependencies: impl IntoIterator, ) { - let dep_incompats = self.add_incompatibility_from_dependencies( - package.clone(), - version.clone(), - dependencies, - ); + let dep_incompats = + self.add_incompatibility_from_dependencies(package, version.clone(), dependencies); self.partial_solution.add_package_version_incompatibilities( - package.clone(), + package, version.clone(), dep_incompats, &self.incompatibility_store, diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 223b949a..e4f9c399 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -263,7 +263,7 @@ impl PartialSolution { } } - pub fn prioritized_packages(&self) -> impl Iterator { + pub fn prioritized_packages(&self) -> impl Iterator, &DP::VS)> { let check_all = self.changed_this_decision_level == self.current_decision_level.0.saturating_sub(1) as usize; let current_decision_level = self.current_decision_level; @@ -278,7 +278,7 @@ impl PartialSolution { // or if we backtracked in the mean time. check_all || pa.highest_decision_level == current_decision_level }) - .filter_map(|(p, pa)| pa.assignments_intersection.potential_package_filter(p)) + .filter_map(|(&p, pa)| pa.assignments_intersection.potential_package_filter(p)) } pub fn pick_highest_priority_pkg( @@ -391,11 +391,11 @@ impl PartialSolution { // Check none of the dependencies (new_incompatibilities) // would create a conflict (be satisfied). if store[new_incompatibilities].iter().all(not_satisfied) { - log::info!("add_decision: {:?} @ {}", package, version); + log::info!("add_decision: {} @ {}", package, version); self.add_decision(package, version); } else { log::info!( - "not adding {:?} @ {} because of its dependencies", + "not adding {} @ {} because of its dependencies", package, version ); diff --git a/src/solver.rs b/src/solver.rs index 2eb0c032..d20cddf5 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -62,135 +62,17 @@ //! If there is no solution, the reason will be provided as clear as possible. use std::cmp::Reverse; -use std::collections::{BTreeMap, BTreeSet as Set}; +use std::collections::BTreeMap; use std::convert::Infallible; use std::error::Error; use std::fmt::{Debug, Display}; -use crate::error::PubGrubError; +pub use crate::internal::arena::Id; pub use crate::internal::core::State; pub use crate::internal::incompatibility::{Incompatibility, Kind}; -use crate::internal::arena::Id; use crate::package::Package; -use crate::type_aliases::{DependencyConstraints, Map, SelectedDependencies}; +use crate::type_aliases::{DependencyConstraints, Map}; use crate::version_set::VersionSet; -use log::{debug, info}; - -/// Main function of the library. -/// Finds a set of packages satisfying dependency bounds for a given package + version pair. -pub fn resolve( - dependency_provider: &DP, - package: DP::P, - version: impl Into, -) -> Result, PubGrubError> { - let mut state: State = State::init(package.clone(), version.into()); - let mut added_dependencies: Map, Set> = Map::default(); - let mut next = state.root_package; - loop { - dependency_provider - .should_cancel() - .map_err(|err| PubGrubError::ErrorInShouldCancel(err))?; - - info!("unit_propagation: {:?}", &next); - state.unit_propagation(next)?; - - debug!( - "Partial solution after unit propagation: {}", - state.partial_solution - ); - - let Some(highest_priority_pkg) = - state.partial_solution.pick_highest_priority_pkg(|p, r| { - dependency_provider.prioritize(&state.package_store[p], r) - }) - else { - return Ok(state - .partial_solution - .extract_solution() - .map(|(p, v)| (state.package_store[p].clone(), v)) - .collect()); - }; - next = highest_priority_pkg; - - let term_intersection = state - .partial_solution - .term_intersection_for_package(next) - .ok_or_else(|| { - PubGrubError::Failure("a package was chosen but we don't have a term.".into()) - })?; - let decision = dependency_provider - .choose_version( - &state.package_store[next], - term_intersection.unwrap_positive(), - ) - .map_err(PubGrubError::ErrorChoosingPackageVersion)?; - info!("DP chose: {:?} @ {:?}", next, decision); - - // Pick the next compatible version. - let v = match decision { - None => { - let inc = Incompatibility::no_versions(next, term_intersection.clone()); - state.add_incompatibility(inc); - continue; - } - Some(x) => x, - }; - - if !term_intersection.contains(&v) { - return Err(PubGrubError::Failure( - "choose_package_version picked an incompatible version".into(), - )); - } - - let is_new_dependency = added_dependencies - .entry(next) - .or_default() - .insert(v.clone()); - - if is_new_dependency { - // Retrieve that package dependencies. - let p = next; - let dependencies = dependency_provider - .get_dependencies(&state.package_store[p], &v) - .map_err(|err| PubGrubError::ErrorRetrievingDependencies { - package: state.package_store[p].clone(), - version: v.clone(), - source: err, - })?; - - let dependencies = match dependencies { - Dependencies::Unavailable(reason) => { - state.add_incompatibility(Incompatibility::custom_version( - p, - v.clone(), - reason, - )); - continue; - } - Dependencies::Available(x) if x.contains_key(&state.package_store[p]) => { - return Err(PubGrubError::SelfDependency { - package: state.package_store[p].clone(), - version: v, - }); - } - Dependencies::Available(x) => x, - }; - - // Add that package and version if the dependencies are not problematic. - let dep_incompats = - state.add_incompatibility_from_dependencies(p, v.clone(), dependencies); - - state - .partial_solution - .add_version(p, v, dep_incompats, &state.incompatibility_store); - } else { - // `dep_incompats` are already in `incompatibilities` so we know there are not satisfied - // terms and can add the decision directly. - info!("add_decision (not first time): {:?} @ {}", &next, v); - state.partial_solution.add_decision(next, v); - } - } -} /// An enum used by [DependencyProvider] that holds information about package dependencies. /// For each [Package] there is a set of versions allowed as a dependency.