Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: make Kind a lot smaller #82

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 32 additions & 32 deletions src/internal/incompatibility.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,28 +43,25 @@ enum Kind<P: Package, V: Version> {
/// Initial incompatibility aiming at picking the root package for the first decision.
NotRoot(P, V),
/// There are no versions in the given range for this package.
NoVersions(P, Range<V>),
NoVersions(P),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think things like NoVersion(P) might imply that there is no version at all for package P while actually that's limited to a given range.

/// Dependencies of the package are unavailable for versions in that range.
UnavailableDependencies(P, Range<V>),
UnavailableDependencies(P),
/// Incompatibility coming from the dependencies of a given package.
FromDependencyOf(P, Range<V>, P, Range<V>),
FromDependencyOf(P, P),
/// Derived from two causes. Stores cause ids.
DerivedFrom(IncompId<P, V>, IncompId<P, V>),
}

/// A type alias for a pair of [Package] and a corresponding [Term].
pub type PackageTerm<P, V> = (P, Term<V>);

/// A Relation describes how a set of terms can be compared to an incompatibility.
/// Typically, the set of terms comes from the partial solution.
#[derive(Eq, PartialEq)]
pub enum Relation<P: Package, V: Version> {
pub enum Relation<P: Package> {
/// We say that a set of terms S satisfies an incompatibility I
/// if S satisfies every term in I.
Satisfied,
/// We say that S contradicts I
/// if S contradicts at least one term in I.
Contradicted(PackageTerm<P, V>),
Contradicted(P),
/// 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),
Expand All @@ -87,37 +84,35 @@ impl<P: Package, V: Version> Incompatibility<P, V> {
/// Create an incompatibility to remember
/// that a given range does not contain any version.
pub fn no_versions(package: P, term: Term<V>) -> Self {
let range = match &term {
Term::Positive(r) => r.clone(),
Term::Negative(_) => panic!("No version should have a positive term"),
};
assert!(term.is_positive(), "No version should have a positive term");
Self {
package_terms: SmallMap::One([(package.clone(), term)]),
kind: Kind::NoVersions(package, range),
kind: Kind::NoVersions(package),
}
}

/// Create an incompatibility to remember
/// that a package version is not selectable
/// because its list of dependencies is unavailable.
pub fn unavailable_dependencies(package: P, version: V) -> Self {
let range = Range::exact(version);
Self {
package_terms: SmallMap::One([(package.clone(), Term::Positive(range.clone()))]),
kind: Kind::UnavailableDependencies(package, range),
package_terms: SmallMap::One([(
package.clone(),
Term::Positive(Range::exact(version)),
)]),
kind: Kind::UnavailableDependencies(package),
}
}

/// Build an incompatibility from a given dependency.
pub fn from_dependency(package: P, version: V, dep: (&P, &Range<V>)) -> Self {
let range1 = Range::exact(version);
let (p2, range2) = dep;
Self {
package_terms: SmallMap::Two([
(package.clone(), Term::Positive(range1.clone())),
(package.clone(), Term::Positive(Range::exact(version))),
(p2.clone(), Term::Negative(range2.clone())),
]),
kind: Kind::FromDependencyOf(package, range1, p2.clone(), range2.clone()),
kind: Kind::FromDependencyOf(package, p2.clone()),
}
}

Expand Down Expand Up @@ -173,13 +168,13 @@ impl<P: Package, V: Version> Incompatibility<P, V> {
}

/// CF definition of Relation enum.
pub fn relation(&self, mut terms: impl FnMut(&P) -> Option<Term<V>>) -> Relation<P, V> {
pub fn relation(&self, mut terms: impl FnMut(&P) -> Option<Term<V>>) -> Relation<P> {
let mut relation = Relation::Satisfied;
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(), incompat_term.clone()));
return Relation::Contradicted(package.clone());
}
None | Some(term::Relation::Inconclusive) => {
// If a package is not present, the intersection is the same as [Term::any].
Expand Down Expand Up @@ -242,7 +237,8 @@ impl<P: Package, V: Version> Incompatibility<P, V> {
shared_ids: &Set<Id<Self>>,
store: &Arena<Self>,
) -> DerivationTree<P, V> {
match &store[self_id].kind {
let val = &store[self_id];
match &val.kind {
Kind::DerivedFrom(id1, id2) => {
let cause1 = Self::build_derivation_tree(*id1, shared_ids, store);
let cause2 = Self::build_derivation_tree(*id2, shared_ids, store);
Expand All @@ -257,18 +253,22 @@ impl<P: Package, V: Version> Incompatibility<P, V> {
Kind::NotRoot(package, version) => {
DerivationTree::External(External::NotRoot(package.clone(), version.clone()))
}
Kind::NoVersions(package, range) => {
DerivationTree::External(External::NoVersions(package.clone(), range.clone()))
Kind::NoVersions(package) => DerivationTree::External(External::NoVersions(
package.clone(),
val.package_terms[package].as_range().clone(),
)),
Kind::UnavailableDependencies(package) => {
DerivationTree::External(External::UnavailableDependencies(
package.clone(),
val.package_terms[package].as_range().clone(),
))
}
Kind::UnavailableDependencies(package, range) => DerivationTree::External(
External::UnavailableDependencies(package.clone(), range.clone()),
),
Kind::FromDependencyOf(package, range, dep_package, dep_range) => {
Kind::FromDependencyOf(package, dep_package) => {
DerivationTree::External(External::FromDependencyOf(
package.clone(),
range.clone(),
val.package_terms[package].as_range().clone(),
dep_package.clone(),
dep_range.clone(),
val.package_terms[dep_package].as_range().clone(),
))
}
}
Expand Down Expand Up @@ -308,12 +308,12 @@ pub mod tests {
let mut store = Arena::new();
let i1 = store.alloc(Incompatibility {
package_terms: SmallMap::Two([("p1", t1.clone()), ("p2", t2.negate())]),
kind: Kind::UnavailableDependencies("0", Range::any())
kind: Kind::UnavailableDependencies("0")
});

let i2 = store.alloc(Incompatibility {
package_terms: SmallMap::Two([("p2", t2), ("p3", t3.clone())]),
kind: Kind::UnavailableDependencies("0", Range::any())
kind: Kind::UnavailableDependencies("0")
});

let mut i3 = Map::default();
Expand Down
2 changes: 1 addition & 1 deletion src/internal/partial_solution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ impl<P: Package, V: Version> PartialSolution<P, V> {
}

/// Check if the terms in the partial solution satisfy the incompatibility.
pub fn relation(&mut self, incompat: &Incompatibility<P, V>) -> Relation<P, V> {
pub fn relation(&mut self, incompat: &Incompatibility<P, V>) -> Relation<P> {
incompat.relation(|package| self.memory.term_intersection_for_package(package).cloned())
}

Expand Down
9 changes: 8 additions & 1 deletion src/internal/small_map.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::type_aliases::Map;
use std::hash::Hash;
use std::{hash::Hash, ops::Index};

#[derive(Debug, Clone)]
pub enum SmallMap<K, V> {
Expand Down Expand Up @@ -146,6 +146,13 @@ impl<K, V> SmallMap<K, V> {
}
}

impl<K: PartialEq + Eq + Hash, V> Index<&K> for SmallMap<K, V> {
type Output = V;
fn index(&self, key: &K) -> &V {
&self.get(key).unwrap()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading terms[package] is nice but probably treacherous. I think it would be better if we could avoid this. Then the caller is aware of when they are calling unwrap().

}
}

impl<K: Eq + Hash + Clone, V: Clone> SmallMap<K, V> {
pub fn as_map(&self) -> Map<K, V> {
match self {
Expand Down
8 changes: 8 additions & 0 deletions src/term.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,14 @@ impl<V: Version> Term<V> {
_ => panic!("Negative term cannot unwrap positive range"),
}
}

/// Unwrap the range contains in the term.
pub(crate) fn as_range(&self) -> &Range<V> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan of this function either. as_range on a term feels like it should be negated for a Negative(range). But a term can never be converted into a range without loosing on the semantics. So I'd probably go for a name such as unwrap() or similar, meaning that some information is lost. Maybe unwrap_range or extract_range.

match self {
Self::Positive(range) => range,
Self::Negative(range) => range,
}
}
}

/// Set operations with terms.
Expand Down