From e80135a8deb5155ddd72c05368b1cf7fafa6d7e2 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Tue, 22 Jun 2021 02:20:08 +0200 Subject: [PATCH 01/22] feat: add range_trait (tests fail) --- src/lib.rs | 2 + src/range_trait.rs | 587 +++++++++++++++++++++++++++++++++++++++++++ src/version_trait.rs | 266 ++++++++++++++++++++ 3 files changed, 855 insertions(+) create mode 100644 src/range_trait.rs create mode 100644 src/version_trait.rs diff --git a/src/lib.rs b/src/lib.rs index 7a6e1737..c98655ee 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -212,10 +212,12 @@ pub mod error; pub mod package; pub mod range; +mod range_trait; pub mod report; pub mod solver; pub mod term; pub mod type_aliases; pub mod version; +mod version_trait; mod internal; diff --git a/src/range_trait.rs b/src/range_trait.rs new file mode 100644 index 00000000..7d2694dc --- /dev/null +++ b/src/range_trait.rs @@ -0,0 +1,587 @@ +// SPDX-License-Identifier: MPL-2.0 + +//! Ranges are constraints defining sets of versions. +//! +//! Concretely, those constraints correspond to any set of versions +//! representable as the concatenation, union, and complement +//! of the ranges building blocks. +//! +//! Those building blocks are: +//! - [none()](Range::none): the empty set +//! - [any()](Range::any): the set of all possible versions +//! - [exact(v)](Range::exact): the set containing only the version v +//! - [higher_than(v)](Range::higher_than): the set defined by `v <= versions` +//! - [strictly_lower_than(v)](Range::strictly_lower_than): the set defined by `versions < v` +//! - [between(v1, v2)](Range::between): the set defined by `v1 <= versions < v2` + +use std::cmp::Ordering; +use std::fmt; +use std::marker::PhantomData; +use std::ops::Bound; +use std::ops::RangeBounds; + +use crate::internal::small_vec::SmallVec; +use crate::version_trait; + +#[derive(Debug, Clone, Eq, PartialEq)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +pub struct Ranges { + segments: SmallVec, + phantom: PhantomData, +} + +pub trait IntervalB: RangeBounds { + fn new(start_bound: Bound, end_bound: Bound) -> Self; +} + +enum SidedBound { + Left(Bound), + Right(Bound), +} + +impl SidedBound<&V> { + fn compare(&self, other: &Self) -> Ordering { + match (&self, other) { + // Handling of both left bounds. + (SidedBound::Left(Bound::Unbounded), SidedBound::Left(Bound::Unbounded)) => { + Ordering::Equal + } + (SidedBound::Left(Bound::Unbounded), SidedBound::Left(_)) => Ordering::Less, + (SidedBound::Left(Bound::Excluded(l1)), SidedBound::Left(Bound::Excluded(l2))) => { + l1.cmp(l2) + } + (SidedBound::Left(Bound::Excluded(l1)), SidedBound::Left(Bound::Included(l2))) => { + // An open left bound is greater than a closed left bound. + l1.cmp(l2).then(Ordering::Greater) + } + (SidedBound::Left(Bound::Included(l1)), SidedBound::Left(Bound::Included(l2))) => { + l1.cmp(l2) + } + (SidedBound::Left(_), SidedBound::Left(_)) => other.compare(&self).reverse(), + + // Handling of both right bounds. + (SidedBound::Right(Bound::Unbounded), SidedBound::Right(Bound::Unbounded)) => { + Ordering::Equal + } + (SidedBound::Right(Bound::Unbounded), SidedBound::Right(_)) => Ordering::Greater, + (SidedBound::Right(Bound::Excluded(r1)), SidedBound::Right(Bound::Excluded(r2))) => { + r1.cmp(r2) + } + (SidedBound::Right(Bound::Excluded(r1)), SidedBound::Right(Bound::Included(r2))) => { + // An open Right bound is smaller than a closed Right bound. + r1.cmp(r2).then(Ordering::Less) + } + (SidedBound::Right(_), SidedBound::Right(_)) => other.compare(&self).reverse(), + + // Handling of left and right bounds. + (SidedBound::Left(Bound::Unbounded), SidedBound::Right(_)) => Ordering::Less, + (SidedBound::Left(_), SidedBound::Right(Bound::Unbounded)) => Ordering::Greater, + (SidedBound::Left(Bound::Excluded(l)), SidedBound::Right(Bound::Excluded(r))) => { + // An open left bound is after an open right bound. + l.cmp(r).then(Ordering::Greater) + } + (SidedBound::Left(Bound::Excluded(l)), SidedBound::Right(Bound::Included(r))) => { + l.cmp(r) + } + (SidedBound::Left(Bound::Included(l)), SidedBound::Right(Bound::Excluded(r))) => { + l.cmp(r) + } + (SidedBound::Left(Bound::Included(l)), SidedBound::Right(Bound::Included(r))) => { + l.cmp(r).then(Ordering::Less) + } + + // Handling of right and left bounds. + (SidedBound::Right(_), SidedBound::Left(_)) => other.compare(&self).reverse(), + } + } +} + +// Ranges building blocks. +impl, V: version_trait::Version> Ranges { + /// Empty set of versions. + pub fn empty() -> Self { + Self { + segments: SmallVec::empty(), + phantom: PhantomData, + } + } + + /// Set of all possible versions. + pub fn full() -> Self { + Self { + segments: SmallVec::one(I::new(V::minimum(), V::maximum())), + phantom: PhantomData, + } + } + + /// Set containing exactly one version. + pub fn singleton(v: impl Into) -> Self { + let v = v.into(); + let start_bound = Bound::Included(v.clone()); + let end_bound = Bound::Included(v); + Self { + segments: SmallVec::one(I::new(start_bound, end_bound)), + phantom: PhantomData, + } + } + + /// Set of all versions higher or equal to some version. + pub fn higher_than(v: impl Into) -> Self { + Self { + segments: SmallVec::one(I::new(Bound::Included(v.into()), V::maximum())), + phantom: PhantomData, + } + } + + /// Set of all versions strictly lower than some version. + pub fn strictly_lower_than(v: impl Into) -> Self { + Self { + segments: SmallVec::one(I::new(V::minimum(), Bound::Excluded(v.into()))), + phantom: PhantomData, + } + } + + /// Set of all versions comprised between two given versions. + /// The lower bound is included and the higher bound excluded. + /// `v1 <= v < v2`. + pub fn between(v1: impl Into, v2: impl Into) -> Self { + let start_bound = Bound::Included(v1.into()); + let end_bound = Bound::Excluded(v2.into()); + Self { + segments: SmallVec::one(I::new(start_bound, end_bound)), + phantom: PhantomData, + } + } +} + +// Set operations. +impl, V: version_trait::Version> Ranges { + // Negate ################################################################## + + /// Compute the complement set of versions. + pub fn complement(&self) -> Self { + match self.segments.first() { + None => Self::full(), // Complement of ∅ is * + Some(seg) => { + if seg.start_bound() == Self::ref_bound(&V::minimum()) { + Self::complement_segments( + Self::owned_bound(seg.end_bound()), + &self.segments[1..], + ) + } else { + Self::complement_segments(V::minimum(), &self.segments) + } + } + } + } + + /// Helper function performing the negation of intervals in segments. + /// For example: + /// [ (v1, None) ] => [ (start, Some(v1)) ] + /// [ (v1, Some(v2)) ] => [ (start, Some(v1)), (v2, None) ] + fn complement_segments(start: Bound, segments: &[I]) -> Self { + let mut complemented_segments = SmallVec::empty(); + let mut start = start; + for seg in segments { + complemented_segments.push(I::new(start, Self::owned_bound(seg.start_bound()))); + start = Self::owned_bound(seg.end_bound()); + } + if start != V::maximum() { + complemented_segments.push(I::new(start, V::maximum())); + } + + Self { + segments: complemented_segments, + phantom: PhantomData, + } + } + + // Union and intersection ################################################## + + /// Compute the union of two sets of versions. + pub fn union(&self, other: &Self) -> Self { + self.complement() + .intersection(&other.complement()) + .complement() + } + + /// Compute the intersection of two sets of versions. + pub fn intersection(&self, other: &Self) -> Self { + let mut segments = SmallVec::empty(); + let mut left_iter = self.segments.iter(); + let mut right_iter = other.segments.iter(); + let mut left = left_iter.next(); + let mut right = right_iter.next(); + loop { + match (left, right) { + (Some(seg_left), Some(seg_right)) => { + let l1 = seg_left.start_bound(); + let l2 = seg_left.end_bound(); + let r1 = seg_right.start_bound(); + let r2 = seg_right.end_bound(); + match SidedBound::Right(l2).compare(&SidedBound::Left(r1)) { + // Disjoint intervals with left < right. + Ordering::Less => left = left_iter.next(), + Ordering::Equal => left = left_iter.next(), + // Possible intersection with left >= right. + Ordering::Greater => { + match SidedBound::Right(r2).compare(&SidedBound::Left(l1)) { + // Disjoint intervals with left < right. + Ordering::Less => right = right_iter.next(), + Ordering::Equal => right = right_iter.next(), + // Intersection for sure. + Ordering::Greater => { + let start = match SidedBound::Left(l1) + .compare(&SidedBound::Right(r1)) + { + Ordering::Less => r1, + _ => l1, + }; + match SidedBound::Right(l2).compare(&SidedBound::Right(r2)) { + Ordering::Less => { + segments.push(I::new( + Self::owned_bound(start), + Self::owned_bound(l2), + )); + left = left_iter.next(); + } + Ordering::Equal => { + segments.push(I::new( + Self::owned_bound(start), + Self::owned_bound(l2), + )); + left = left_iter.next(); + right = right_iter.next(); + } + Ordering::Greater => { + segments.push(I::new( + Self::owned_bound(start), + Self::owned_bound(r2), + )); + right = right_iter.next(); + } + } + } + } + } + } + } + // Left or right has ended. + _ => { + break; + } + } + } + + Self { + segments, + phantom: PhantomData, + } + } + + // Contains ################################################################ + + /// Check if ranges contain a given version. + pub fn contains(&self, version: &V) -> bool { + for seg in &self.segments { + match (seg.start_bound(), seg.end_bound()) { + (Bound::Unbounded, Bound::Unbounded) => return seg.contains(version), + (Bound::Unbounded, Bound::Excluded(r)) => match version.cmp(r) { + Ordering::Less => return seg.contains(version), + Ordering::Equal => return false, + Ordering::Greater => {} + }, + (Bound::Unbounded, Bound::Included(r)) => match version.cmp(r) { + Ordering::Greater => {} + _ => return seg.contains(version), + }, + (Bound::Excluded(l), Bound::Unbounded) => match version.cmp(l) { + Ordering::Greater => return seg.contains(version), + _ => return false, + }, + (Bound::Excluded(l), Bound::Excluded(r)) => match version.cmp(l) { + Ordering::Less => return false, + Ordering::Equal => return false, + Ordering::Greater => match version.cmp(r) { + Ordering::Less => return seg.contains(version), + Ordering::Equal => return false, + Ordering::Greater => {} + }, + }, + (Bound::Excluded(l), Bound::Included(r)) => match version.cmp(l) { + Ordering::Less => return false, + Ordering::Equal => return false, + Ordering::Greater => match version.cmp(r) { + Ordering::Greater => {} + _ => return seg.contains(version), + }, + }, + (Bound::Included(l), Bound::Unbounded) => match version.cmp(l) { + Ordering::Less => return false, + _ => return seg.contains(version), + }, + (Bound::Included(l), Bound::Excluded(r)) => match version.cmp(l) { + Ordering::Less => return false, + Ordering::Equal => return seg.contains(version), + Ordering::Greater => match version.cmp(r) { + Ordering::Less => return seg.contains(version), + Ordering::Equal => return false, + Ordering::Greater => {} + }, + }, + (Bound::Included(l), Bound::Included(r)) => match version.cmp(l) { + Ordering::Less => return false, + Ordering::Equal => return seg.contains(version), + Ordering::Greater => match version.cmp(r) { + Ordering::Greater => {} + _ => return seg.contains(version), + }, + }, + } + } + false + } + + // Helper ################################################################## + + /// Will be obsolete when .as_ref() hits stable. + fn ref_bound(bound: &Bound) -> Bound<&V> { + match *bound { + Bound::Included(ref x) => Bound::Included(x), + Bound::Excluded(ref x) => Bound::Excluded(x), + Bound::Unbounded => Bound::Unbounded, + } + } + + /// Will be obsolete when .cloned() hits stable. + fn owned_bound(bound: Bound<&V>) -> Bound { + match bound { + Bound::Unbounded => Bound::Unbounded, + Bound::Included(x) => Bound::Included(x.clone()), + Bound::Excluded(x) => Bound::Excluded(x.clone()), + } + } +} + +// /// A Range is a set of versions. +// #[derive(Debug, Clone, Eq, PartialEq)] +// #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +// #[cfg_attr(feature = "serde", serde(transparent))] +// pub struct Range { +// segments: SmallVec>, +// } + +// REPORT ###################################################################### + +impl, V: version_trait::Version> fmt::Display for Ranges { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self.segments.as_slice() { + [] => write!(f, "∅"), + [seg] => { + write!(f, "{}", interval_to_string(seg)) + } + more_than_one_interval => { + let string_intervals: Vec<_> = more_than_one_interval + .iter() + .map(interval_to_string) + .collect(); + write!(f, "{}", string_intervals.join(", ")) + } + } + } +} + +fn interval_to_string, V: version_trait::Version>(seg: &I) -> String { + let start = seg.start_bound(); + let end = seg.end_bound(); + if start == Ranges::::ref_bound(&V::minimum()) { + display_end_bound(end) + } else if end == Ranges::::ref_bound(&V::maximum()) { + display_start_bound(start) + } else { + format!("{}, {}", display_start_bound(start), display_end_bound(end)) + } +} + +fn display_start_bound(start: Bound<&V>) -> String { + match start { + Bound::Unbounded => "∗".to_string(), + Bound::Excluded(v) => format!("> {}", v), + Bound::Included(v) => format!(">= {}", v), + } +} + +fn display_end_bound(end: Bound<&V>) -> String { + match end { + Bound::Unbounded => "∗".to_string(), + Bound::Excluded(v) => format!("< {}", v), + Bound::Included(v) => format!("<= {}", v), + } +} + +// trait IntervalB: RangeBounds { +// fn new(start_bound: Bound, end_bound: Bound) -> Self; +// } + +// NumberInterval ############################################################## + +use crate::version_trait::NumberVersion; + +#[derive(Debug, Clone, Eq, PartialEq)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +pub struct NumberInterval { + start: NumberVersion, + end: Option, +} + +impl RangeBounds for NumberInterval { + fn start_bound(&self) -> Bound<&NumberVersion> { + Bound::Included(&self.start) + } + fn end_bound(&self) -> Bound<&NumberVersion> { + match &self.end { + None => Bound::Unbounded, + Some(v) => Bound::Excluded(v), + } + } +} + +impl IntervalB for NumberInterval { + fn new(start_bound: Bound, end_bound: Bound) -> Self { + let start = match start_bound { + Bound::Unbounded => NumberVersion(0), + Bound::Excluded(v) => v.bump(), + Bound::Included(v) => v, + }; + let end = match end_bound { + Bound::Unbounded => None, + Bound::Excluded(v) => Some(v), + Bound::Included(v) => Some(v.bump()), + }; + Self { start, end } + } +} + +// TESTS ####################################################################### + +#[cfg(test)] +pub mod tests { + use proptest::prelude::*; + + use super::*; + + pub fn strategy() -> impl Strategy> { + prop::collection::vec(any::(), 0..10).prop_map(|mut vec| { + vec.sort_unstable(); + vec.dedup(); + let mut pair_iter = vec.chunks_exact(2); + let mut segments = SmallVec::empty(); + while let Some([v1, v2]) = pair_iter.next() { + segments.push(NumberInterval { + start: NumberVersion(*v1), + end: Some(NumberVersion(*v2)), + }); + } + if let [v] = pair_iter.remainder() { + segments.push(NumberInterval { + start: NumberVersion(*v), + end: None, + }); + } + Ranges { + segments, + phantom: PhantomData, + } + }) + } + + fn version_strat() -> impl Strategy { + any::().prop_map(NumberVersion) + } + + proptest! { + + // Testing negate ---------------------------------- + + #[test] + fn negate_is_different(ranges in strategy()) { + assert_ne!(ranges.complement(), ranges); + } + + #[test] + fn double_negate_is_identity(ranges in strategy()) { + assert_eq!(ranges.complement().complement(), ranges); + } + + #[test] + fn negate_contains_opposite(ranges in strategy(), version in version_strat()) { + assert_ne!(ranges.contains(&version), ranges.complement().contains(&version)); + } + + // Testing intersection ---------------------------- + + #[test] + fn intersection_is_symmetric(r1 in strategy(), r2 in strategy()) { + assert_eq!(r1.intersection(&r2), r2.intersection(&r1)); + } + + #[test] + fn intersection_with_any_is_identity(ranges in strategy()) { + assert_eq!(Ranges::full().intersection(&ranges), ranges); + } + + #[test] + fn intersection_with_none_is_none(ranges in strategy()) { + assert_eq!(Ranges::empty().intersection(&ranges), Ranges::empty()); + } + + #[test] + fn intersection_is_idempotent(r1 in strategy(), r2 in strategy()) { + assert_eq!(r1.intersection(&r2).intersection(&r2), r1.intersection(&r2)); + } + + #[test] + fn intersection_is_associative(r1 in strategy(), r2 in strategy(), r3 in strategy()) { + assert_eq!(r1.intersection(&r2).intersection(&r3), r1.intersection(&r2.intersection(&r3))); + } + + #[test] + fn intesection_of_complements_is_none(ranges in strategy()) { + assert_eq!(ranges.complement().intersection(&ranges), Ranges::empty()); + } + + #[test] + fn intesection_contains_both(r1 in strategy(), r2 in strategy(), version in version_strat()) { + assert_eq!(r1.intersection(&r2).contains(&version), r1.contains(&version) && r2.contains(&version)); + } + + // Testing union ----------------------------------- + + #[test] + fn union_of_complements_is_any(ranges in strategy()) { + assert_eq!(ranges.complement().union(&ranges), Ranges::full()); + } + + #[test] + fn union_contains_either(r1 in strategy(), r2 in strategy(), version in version_strat()) { + assert_eq!(r1.union(&r2).contains(&version), r1.contains(&version) || r2.contains(&version)); + } + + // Testing contains -------------------------------- + + #[test] + fn always_contains_exact(version in version_strat()) { + assert!(Ranges::::singleton(version).contains(&version)); + } + + #[test] + fn contains_negation(ranges in strategy(), version in version_strat()) { + assert_ne!(ranges.contains(&version), ranges.complement().contains(&version)); + } + + #[test] + fn contains_intersection(ranges in strategy(), version in version_strat()) { + assert_eq!(ranges.contains(&version), ranges.intersection(&Ranges::singleton(version)) != Ranges::empty()); + } + } +} diff --git a/src/version_trait.rs b/src/version_trait.rs new file mode 100644 index 00000000..7eccd6ec --- /dev/null +++ b/src/version_trait.rs @@ -0,0 +1,266 @@ +// SPDX-License-Identifier: MPL-2.0 + +//! Traits and implementations to create and compare versions. + +use std::fmt::{self, Debug, Display}; +use std::ops::Bound; +use std::str::FromStr; +use thiserror::Error; + +/// New trait for versions. +pub trait Version: Clone + Ord + Debug + Display { + /// Returns the minimum version. + fn minimum() -> Bound; + /// Returns the maximum version. + fn maximum() -> Bound; +} + +/// Type for semantic versions: major.minor.patch. +#[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq, Hash)] +pub struct SemanticVersion { + major: u32, + minor: u32, + patch: u32, +} + +#[cfg(feature = "serde")] +impl serde::Serialize for SemanticVersion { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + serializer.serialize_str(&format!("{}", self)) + } +} + +#[cfg(feature = "serde")] +impl<'de> serde::Deserialize<'de> for SemanticVersion { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let s = String::deserialize(deserializer)?; + FromStr::from_str(&s).map_err(serde::de::Error::custom) + } +} + +// Constructors +impl SemanticVersion { + /// Create a version with "major", "minor" and "patch" values. + /// `version = major.minor.patch` + pub fn new(major: u32, minor: u32, patch: u32) -> Self { + Self { + major, + minor, + patch, + } + } + + /// Version 0.0.0. + pub fn zero() -> Self { + Self::new(0, 0, 0) + } + + /// Version 1.0.0. + pub fn one() -> Self { + Self::new(1, 0, 0) + } + + /// Version 2.0.0. + pub fn two() -> Self { + Self::new(2, 0, 0) + } +} + +// Convert a tuple (major, minor, patch) into a version. +impl From<(u32, u32, u32)> for SemanticVersion { + fn from(tuple: (u32, u32, u32)) -> Self { + let (major, minor, patch) = tuple; + Self::new(major, minor, patch) + } +} + +// Convert a version into a tuple (major, minor, patch). +impl From for (u32, u32, u32) { + fn from(v: SemanticVersion) -> Self { + (v.major, v.minor, v.patch) + } +} + +// Bump versions. +impl SemanticVersion { + /// Bump the patch number of a version. + pub fn bump_patch(self) -> Self { + Self::new(self.major, self.minor, self.patch + 1) + } + + /// Bump the minor number of a version. + pub fn bump_minor(self) -> Self { + Self::new(self.major, self.minor + 1, 0) + } + + /// Bump the major number of a version. + pub fn bump_major(self) -> Self { + Self::new(self.major + 1, 0, 0) + } +} + +/// Error creating [SemanticVersion] from [String]. +#[derive(Error, Debug, PartialEq)] +pub enum VersionParseError { + /// [SemanticVersion] must contain major, minor, patch versions. + #[error("version {full_version} must contain 3 numbers separated by dot")] + NotThreeParts { + /// [SemanticVersion] that was being parsed. + full_version: String, + }, + /// Wrapper around [ParseIntError](core::num::ParseIntError). + #[error("cannot parse '{version_part}' in '{full_version}' as u32: {parse_error}")] + ParseIntError { + /// [SemanticVersion] that was being parsed. + full_version: String, + /// A version part where parsing failed. + version_part: String, + /// A specific error resulted from parsing a part of the version as [u32]. + parse_error: String, + }, +} + +impl FromStr for SemanticVersion { + type Err = VersionParseError; + + fn from_str(s: &str) -> Result { + let parse_u32 = |part: &str| { + part.parse::().map_err(|e| Self::Err::ParseIntError { + full_version: s.to_string(), + version_part: part.to_string(), + parse_error: e.to_string(), + }) + }; + + let mut parts = s.split('.'); + match (parts.next(), parts.next(), parts.next(), parts.next()) { + (Some(major), Some(minor), Some(patch), None) => { + let major = parse_u32(major)?; + let minor = parse_u32(minor)?; + let patch = parse_u32(patch)?; + Ok(Self { + major, + minor, + patch, + }) + } + _ => Err(Self::Err::NotThreeParts { + full_version: s.to_string(), + }), + } + } +} + +#[test] +fn from_str_for_semantic_version() { + let parse = |str: &str| str.parse::(); + assert!(parse( + &SemanticVersion { + major: 0, + minor: 1, + patch: 0 + } + .to_string() + ) + .is_ok()); + assert!(parse("1.2.3").is_ok()); + assert_eq!( + parse("1.abc.3"), + Err(VersionParseError::ParseIntError { + full_version: "1.abc.3".to_owned(), + version_part: "abc".to_owned(), + parse_error: "invalid digit found in string".to_owned(), + }) + ); + assert_eq!( + parse("1.2.-3"), + Err(VersionParseError::ParseIntError { + full_version: "1.2.-3".to_owned(), + version_part: "-3".to_owned(), + parse_error: "invalid digit found in string".to_owned(), + }) + ); + assert_eq!( + parse("1.2.9876543210"), + Err(VersionParseError::ParseIntError { + full_version: "1.2.9876543210".to_owned(), + version_part: "9876543210".to_owned(), + parse_error: "number too large to fit in target type".to_owned(), + }) + ); + assert_eq!( + parse("1.2"), + Err(VersionParseError::NotThreeParts { + full_version: "1.2".to_owned(), + }) + ); + assert_eq!( + parse("1.2.3."), + Err(VersionParseError::NotThreeParts { + full_version: "1.2.3.".to_owned(), + }) + ); +} + +impl Display for SemanticVersion { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}.{}.{}", self.major, self.minor, self.patch) + } +} + +// Implement Version for SemanticVersion. +impl Version for SemanticVersion { + fn minimum() -> Bound { + Bound::Included(Self::zero()) + } + fn maximum() -> Bound { + Bound::Unbounded + } +} + +/// Simplest versions possible, just a positive number. +#[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq, Hash)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize,))] +#[cfg_attr(feature = "serde", serde(transparent))] +pub struct NumberVersion(pub u32); + +// Convert an usize into a version. +impl From for NumberVersion { + fn from(v: u32) -> Self { + Self(v) + } +} + +// Convert a version into an usize. +impl From for u32 { + fn from(version: NumberVersion) -> Self { + version.0 + } +} + +impl Display for NumberVersion { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.0) + } +} + +impl Version for NumberVersion { + fn minimum() -> Bound { + Bound::Included(NumberVersion(0)) + } + fn maximum() -> Bound { + Bound::Unbounded + } +} + +impl NumberVersion { + pub fn bump(&self) -> Self { + NumberVersion(self.0 + 1) + } +} From 193c897743322f1e851c9340a017c9d799aafffa Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Tue, 22 Jun 2021 12:07:09 +0200 Subject: [PATCH 02/22] fix: ranges bugs with property tests --- src/range_trait.rs | 118 ++++++++++++++++++++++++++++++--------------- 1 file changed, 80 insertions(+), 38 deletions(-) diff --git a/src/range_trait.rs b/src/range_trait.rs index 7d2694dc..c836022e 100644 --- a/src/range_trait.rs +++ b/src/range_trait.rs @@ -34,11 +34,19 @@ pub trait IntervalB: RangeBounds { fn new(start_bound: Bound, end_bound: Bound) -> Self; } +#[derive(Debug, Clone, Eq, PartialEq)] enum SidedBound { Left(Bound), Right(Bound), } +fn ref_sided_bound(sb: &SidedBound) -> SidedBound<&V> { + match sb { + SidedBound::Left(bound) => SidedBound::Left(ref_bound(bound)), + SidedBound::Right(bound) => SidedBound::Right(ref_bound(bound)), + } +} + impl SidedBound<&V> { fn compare(&self, other: &Self) -> Ordering { match (&self, other) { @@ -71,11 +79,14 @@ impl SidedBound<&V> { // An open Right bound is smaller than a closed Right bound. r1.cmp(r2).then(Ordering::Less) } + (SidedBound::Right(Bound::Included(r1)), SidedBound::Right(Bound::Included(r2))) => { + r1.cmp(r2) + } (SidedBound::Right(_), SidedBound::Right(_)) => other.compare(&self).reverse(), // Handling of left and right bounds. (SidedBound::Left(Bound::Unbounded), SidedBound::Right(_)) => Ordering::Less, - (SidedBound::Left(_), SidedBound::Right(Bound::Unbounded)) => Ordering::Greater, + (SidedBound::Left(_), SidedBound::Right(Bound::Unbounded)) => Ordering::Less, (SidedBound::Left(Bound::Excluded(l)), SidedBound::Right(Bound::Excluded(r))) => { // An open left bound is after an open right bound. l.cmp(r).then(Ordering::Greater) @@ -163,9 +174,9 @@ impl, V: version_trait::Version> Ranges { match self.segments.first() { None => Self::full(), // Complement of ∅ is * Some(seg) => { - if seg.start_bound() == Self::ref_bound(&V::minimum()) { + if seg.start_bound() == ref_bound(&V::minimum()) { Self::complement_segments( - Self::owned_bound(seg.end_bound()), + owned_bound(flip_bound(seg.end_bound())), &self.segments[1..], ) } else { @@ -183,8 +194,8 @@ impl, V: version_trait::Version> Ranges { let mut complemented_segments = SmallVec::empty(); let mut start = start; for seg in segments { - complemented_segments.push(I::new(start, Self::owned_bound(seg.start_bound()))); - start = Self::owned_bound(seg.end_bound()); + complemented_segments.push(I::new(start, owned_bound(flip_bound(seg.start_bound())))); + start = owned_bound(flip_bound(seg.end_bound())); } if start != V::maximum() { complemented_segments.push(I::new(start, V::maximum())); @@ -239,25 +250,19 @@ impl, V: version_trait::Version> Ranges { }; match SidedBound::Right(l2).compare(&SidedBound::Right(r2)) { Ordering::Less => { - segments.push(I::new( - Self::owned_bound(start), - Self::owned_bound(l2), - )); + segments + .push(I::new(owned_bound(start), owned_bound(l2))); left = left_iter.next(); } Ordering::Equal => { - segments.push(I::new( - Self::owned_bound(start), - Self::owned_bound(l2), - )); + segments + .push(I::new(owned_bound(start), owned_bound(l2))); left = left_iter.next(); right = right_iter.next(); } Ordering::Greater => { - segments.push(I::new( - Self::owned_bound(start), - Self::owned_bound(r2), - )); + segments + .push(I::new(owned_bound(start), owned_bound(r2))); right = right_iter.next(); } } @@ -341,25 +346,33 @@ impl, V: version_trait::Version> Ranges { } false } +} - // Helper ################################################################## +// Helper ################################################################## - /// Will be obsolete when .as_ref() hits stable. - fn ref_bound(bound: &Bound) -> Bound<&V> { - match *bound { - Bound::Included(ref x) => Bound::Included(x), - Bound::Excluded(ref x) => Bound::Excluded(x), - Bound::Unbounded => Bound::Unbounded, - } +fn flip_bound(bound: Bound) -> Bound { + match bound { + Bound::Unbounded => Bound::Unbounded, + Bound::Excluded(b) => Bound::Included(b), + Bound::Included(b) => Bound::Excluded(b), } +} - /// Will be obsolete when .cloned() hits stable. - fn owned_bound(bound: Bound<&V>) -> Bound { - match bound { - Bound::Unbounded => Bound::Unbounded, - Bound::Included(x) => Bound::Included(x.clone()), - Bound::Excluded(x) => Bound::Excluded(x.clone()), - } +/// Will be obsolete when .as_ref() hits stable. +fn ref_bound(bound: &Bound) -> Bound<&V> { + match *bound { + Bound::Included(ref x) => Bound::Included(x), + Bound::Excluded(ref x) => Bound::Excluded(x), + Bound::Unbounded => Bound::Unbounded, + } +} + +/// Will be obsolete when .cloned() hits stable. +fn owned_bound(bound: Bound<&V>) -> Bound { + match bound { + Bound::Unbounded => Bound::Unbounded, + Bound::Included(x) => Bound::Included(x.clone()), + Bound::Excluded(x) => Bound::Excluded(x.clone()), } } @@ -394,9 +407,9 @@ impl, V: version_trait::Version> fmt::Display for Ranges { fn interval_to_string, V: version_trait::Version>(seg: &I) -> String { let start = seg.start_bound(); let end = seg.end_bound(); - if start == Ranges::::ref_bound(&V::minimum()) { + if start == ref_bound(&V::minimum()) { display_end_bound(end) - } else if end == Ranges::::ref_bound(&V::maximum()) { + } else if end == ref_bound(&V::maximum()) { display_start_bound(start) } else { format!("{}, {}", display_start_bound(start), display_end_bound(end)) @@ -419,10 +432,6 @@ fn display_end_bound(end: Bound<&V>) -> String { } } -// trait IntervalB: RangeBounds { -// fn new(start_bound: Bound, end_bound: Bound) -> Self; -// } - // NumberInterval ############################################################## use crate::version_trait::NumberVersion; @@ -470,6 +479,39 @@ pub mod tests { use super::*; + // SidedBound tests. + + use Bound::{Excluded, Included, Unbounded}; + use SidedBound::{Left, Right}; + + fn sided_bound_strategy() -> impl Strategy> { + prop_oneof![ + bound_strategy().prop_map(Left), + bound_strategy().prop_map(Right), + ] + } + + fn bound_strategy() -> impl Strategy> { + prop_oneof![ + Just(Unbounded), + any::().prop_map(Excluded), + any::().prop_map(Included), + ] + } + + proptest! { + + #[test] + fn reverse_bounds_reverse_order(sb1 in sided_bound_strategy(), sb2 in sided_bound_strategy()) { + let s1 = ref_sided_bound(&sb1); + let s2 = ref_sided_bound(&sb2); + assert_eq!(s1.compare(&s2), s2.compare(&s1).reverse()); + } + + } + + // Ranges tests. + pub fn strategy() -> impl Strategy> { prop::collection::vec(any::(), 0..10).prop_map(|mut vec| { vec.sort_unstable(); From ad7c7beeb170be5e97be336a172840facee27bf4 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Tue, 22 Jun 2021 14:27:50 +0200 Subject: [PATCH 03/22] refactor: rename IntervalB to Interval --- src/range_trait.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/range_trait.rs b/src/range_trait.rs index c836022e..9b0fb0f4 100644 --- a/src/range_trait.rs +++ b/src/range_trait.rs @@ -30,7 +30,7 @@ pub struct Ranges { phantom: PhantomData, } -pub trait IntervalB: RangeBounds { +pub trait Interval: RangeBounds { fn new(start_bound: Bound, end_bound: Bound) -> Self; } @@ -108,7 +108,7 @@ impl SidedBound<&V> { } // Ranges building blocks. -impl, V: version_trait::Version> Ranges { +impl, V: version_trait::Version> Ranges { /// Empty set of versions. pub fn empty() -> Self { Self { @@ -166,7 +166,7 @@ impl, V: version_trait::Version> Ranges { } // Set operations. -impl, V: version_trait::Version> Ranges { +impl, V: version_trait::Version> Ranges { // Negate ################################################################## /// Compute the complement set of versions. @@ -386,7 +386,7 @@ fn owned_bound(bound: Bound<&V>) -> Bound { // REPORT ###################################################################### -impl, V: version_trait::Version> fmt::Display for Ranges { +impl, V: version_trait::Version> fmt::Display for Ranges { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self.segments.as_slice() { [] => write!(f, "∅"), @@ -404,7 +404,7 @@ impl, V: version_trait::Version> fmt::Display for Ranges { } } -fn interval_to_string, V: version_trait::Version>(seg: &I) -> String { +fn interval_to_string, V: version_trait::Version>(seg: &I) -> String { let start = seg.start_bound(); let end = seg.end_bound(); if start == ref_bound(&V::minimum()) { @@ -455,7 +455,7 @@ impl RangeBounds for NumberInterval { } } -impl IntervalB for NumberInterval { +impl Interval for NumberInterval { fn new(start_bound: Bound, end_bound: Bound) -> Self { let start = match start_bound { Bound::Unbounded => NumberVersion(0), From f07f00793e62f4c322833992a8763bd1c5c6d39d Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Tue, 22 Jun 2021 14:48:18 +0200 Subject: [PATCH 04/22] refactor: move Interval to version_trait module --- src/range_trait.rs | 82 ++++++++------------------------------------ src/version_trait.rs | 53 ++++++++++++++++++++++++++-- 2 files changed, 66 insertions(+), 69 deletions(-) diff --git a/src/range_trait.rs b/src/range_trait.rs index 9b0fb0f4..9fbd5452 100644 --- a/src/range_trait.rs +++ b/src/range_trait.rs @@ -18,10 +18,9 @@ use std::cmp::Ordering; use std::fmt; use std::marker::PhantomData; use std::ops::Bound; -use std::ops::RangeBounds; use crate::internal::small_vec::SmallVec; -use crate::version_trait; +use crate::version_trait::{Interval, NumberInterval, NumberVersion, Version}; #[derive(Debug, Clone, Eq, PartialEq)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] @@ -30,10 +29,6 @@ pub struct Ranges { phantom: PhantomData, } -pub trait Interval: RangeBounds { - fn new(start_bound: Bound, end_bound: Bound) -> Self; -} - #[derive(Debug, Clone, Eq, PartialEq)] enum SidedBound { Left(Bound), @@ -108,7 +103,7 @@ impl SidedBound<&V> { } // Ranges building blocks. -impl, V: version_trait::Version> Ranges { +impl, V: Version> Ranges { /// Empty set of versions. pub fn empty() -> Self { Self { @@ -166,7 +161,7 @@ impl, V: version_trait::Version> Ranges { } // Set operations. -impl, V: version_trait::Version> Ranges { +impl, V: Version> Ranges { // Negate ################################################################## /// Compute the complement set of versions. @@ -376,17 +371,9 @@ fn owned_bound(bound: Bound<&V>) -> Bound { } } -// /// A Range is a set of versions. -// #[derive(Debug, Clone, Eq, PartialEq)] -// #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] -// #[cfg_attr(feature = "serde", serde(transparent))] -// pub struct Range { -// segments: SmallVec>, -// } - // REPORT ###################################################################### -impl, V: version_trait::Version> fmt::Display for Ranges { +impl, V: Version> fmt::Display for Ranges { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self.segments.as_slice() { [] => write!(f, "∅"), @@ -404,7 +391,7 @@ impl, V: version_trait::Version> fmt::Display for Ranges { } } -fn interval_to_string, V: version_trait::Version>(seg: &I) -> String { +fn interval_to_string, V: Version>(seg: &I) -> String { let start = seg.start_bound(); let end = seg.end_bound(); if start == ref_bound(&V::minimum()) { @@ -416,7 +403,7 @@ fn interval_to_string, V: version_trait::Version>(seg: &I) -> Str } } -fn display_start_bound(start: Bound<&V>) -> String { +fn display_start_bound(start: Bound<&V>) -> String { match start { Bound::Unbounded => "∗".to_string(), Bound::Excluded(v) => format!("> {}", v), @@ -424,7 +411,7 @@ fn display_start_bound(start: Bound<&V>) -> String { } } -fn display_end_bound(end: Bound<&V>) -> String { +fn display_end_bound(end: Bound<&V>) -> String { match end { Bound::Unbounded => "∗".to_string(), Bound::Excluded(v) => format!("< {}", v), @@ -432,45 +419,6 @@ fn display_end_bound(end: Bound<&V>) -> String { } } -// NumberInterval ############################################################## - -use crate::version_trait::NumberVersion; - -#[derive(Debug, Clone, Eq, PartialEq)] -#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] -pub struct NumberInterval { - start: NumberVersion, - end: Option, -} - -impl RangeBounds for NumberInterval { - fn start_bound(&self) -> Bound<&NumberVersion> { - Bound::Included(&self.start) - } - fn end_bound(&self) -> Bound<&NumberVersion> { - match &self.end { - None => Bound::Unbounded, - Some(v) => Bound::Excluded(v), - } - } -} - -impl Interval for NumberInterval { - fn new(start_bound: Bound, end_bound: Bound) -> Self { - let start = match start_bound { - Bound::Unbounded => NumberVersion(0), - Bound::Excluded(v) => v.bump(), - Bound::Included(v) => v, - }; - let end = match end_bound { - Bound::Unbounded => None, - Bound::Excluded(v) => Some(v), - Bound::Included(v) => Some(v.bump()), - }; - Self { start, end } - } -} - // TESTS ####################################################################### #[cfg(test)] @@ -519,16 +467,16 @@ pub mod tests { let mut pair_iter = vec.chunks_exact(2); let mut segments = SmallVec::empty(); while let Some([v1, v2]) = pair_iter.next() { - segments.push(NumberInterval { - start: NumberVersion(*v1), - end: Some(NumberVersion(*v2)), - }); + segments.push(NumberInterval::new( + Bound::Included(NumberVersion(*v1)), + Bound::Excluded(NumberVersion(*v2)), + )); } if let [v] = pair_iter.remainder() { - segments.push(NumberInterval { - start: NumberVersion(*v), - end: None, - }); + segments.push(NumberInterval::new( + Bound::Included(NumberVersion(*v)), + Bound::Unbounded, + )); } Ranges { segments, diff --git a/src/version_trait.rs b/src/version_trait.rs index 7eccd6ec..63dc7f37 100644 --- a/src/version_trait.rs +++ b/src/version_trait.rs @@ -3,7 +3,7 @@ //! Traits and implementations to create and compare versions. use std::fmt::{self, Debug, Display}; -use std::ops::Bound; +use std::ops::{Bound, RangeBounds}; use std::str::FromStr; use thiserror::Error; @@ -15,6 +15,16 @@ pub trait Version: Clone + Ord + Debug + Display { fn maximum() -> Bound; } +/// An interval is a bounded domain containing all values +/// between its starting and ending bounds. +pub trait Interval: RangeBounds { + /// Create an interval from its starting and ending bounds. + /// It's the caller responsability to order them correctly. + fn new(start_bound: Bound, end_bound: Bound) -> Self; +} + +// SemanticVersion ############################################################# + /// Type for semantic versions: major.minor.patch. #[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq, Hash)] pub struct SemanticVersion { @@ -224,6 +234,8 @@ impl Version for SemanticVersion { } } +// NumberVersion ############################################################### + /// Simplest versions possible, just a positive number. #[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq, Hash)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize,))] @@ -260,7 +272,44 @@ impl Version for NumberVersion { } impl NumberVersion { - pub fn bump(&self) -> Self { + fn bump(&self) -> Self { NumberVersion(self.0 + 1) } } + +// NumberInterval ############################################################## + +#[derive(Debug, Clone, Eq, PartialEq)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +pub struct NumberInterval { + start: NumberVersion, + end: Option, +} + +impl RangeBounds for NumberInterval { + fn start_bound(&self) -> Bound<&NumberVersion> { + Bound::Included(&self.start) + } + fn end_bound(&self) -> Bound<&NumberVersion> { + match &self.end { + None => Bound::Unbounded, + Some(v) => Bound::Excluded(v), + } + } +} + +impl Interval for NumberInterval { + fn new(start_bound: Bound, end_bound: Bound) -> Self { + let start = match start_bound { + Bound::Unbounded => NumberVersion(0), + Bound::Excluded(v) => v.bump(), + Bound::Included(v) => v, + }; + let end = match end_bound { + Bound::Unbounded => None, + Bound::Excluded(v) => Some(v), + Bound::Included(v) => Some(v.bump()), + }; + Self { start, end } + } +} From e1fe982f7df4bbd502cd779507f0731649e63fa4 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Tue, 22 Jun 2021 15:15:23 +0200 Subject: [PATCH 05/22] feat: impl From RangeBounds for NumberInterval --- src/range_trait.rs | 39 +++---------------------------------- src/version_trait.rs | 46 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 36 deletions(-) diff --git a/src/range_trait.rs b/src/range_trait.rs index 9fbd5452..4690bd11 100644 --- a/src/range_trait.rs +++ b/src/range_trait.rs @@ -20,6 +20,7 @@ use std::marker::PhantomData; use std::ops::Bound; use crate::internal::small_vec::SmallVec; +use crate::version_trait::{flip_bound, owned_bound, ref_bound}; use crate::version_trait::{Interval, NumberInterval, NumberVersion, Version}; #[derive(Debug, Clone, Eq, PartialEq)] @@ -343,34 +344,6 @@ impl, V: Version> Ranges { } } -// Helper ################################################################## - -fn flip_bound(bound: Bound) -> Bound { - match bound { - Bound::Unbounded => Bound::Unbounded, - Bound::Excluded(b) => Bound::Included(b), - Bound::Included(b) => Bound::Excluded(b), - } -} - -/// Will be obsolete when .as_ref() hits stable. -fn ref_bound(bound: &Bound) -> Bound<&V> { - match *bound { - Bound::Included(ref x) => Bound::Included(x), - Bound::Excluded(ref x) => Bound::Excluded(x), - Bound::Unbounded => Bound::Unbounded, - } -} - -/// Will be obsolete when .cloned() hits stable. -fn owned_bound(bound: Bound<&V>) -> Bound { - match bound { - Bound::Unbounded => Bound::Unbounded, - Bound::Included(x) => Bound::Included(x.clone()), - Bound::Excluded(x) => Bound::Excluded(x.clone()), - } -} - // REPORT ###################################################################### impl, V: Version> fmt::Display for Ranges { @@ -467,16 +440,10 @@ pub mod tests { let mut pair_iter = vec.chunks_exact(2); let mut segments = SmallVec::empty(); while let Some([v1, v2]) = pair_iter.next() { - segments.push(NumberInterval::new( - Bound::Included(NumberVersion(*v1)), - Bound::Excluded(NumberVersion(*v2)), - )); + segments.push((v1..v2).into()); } if let [v] = pair_iter.remainder() { - segments.push(NumberInterval::new( - Bound::Included(NumberVersion(*v)), - Bound::Unbounded, - )); + segments.push((v..).into()); } Ranges { segments, diff --git a/src/version_trait.rs b/src/version_trait.rs index 63dc7f37..b911d01f 100644 --- a/src/version_trait.rs +++ b/src/version_trait.rs @@ -286,6 +286,16 @@ pub struct NumberInterval { end: Option, } +// Ease the creation of NumberInterval from classic ranges: 0..10 +impl> From for NumberInterval { + fn from(range: RB) -> Self { + Self::new( + map_bound(NumberVersion::from, owned_bound(range.start_bound())), + map_bound(NumberVersion::from, owned_bound(range.end_bound())), + ) + } +} + impl RangeBounds for NumberInterval { fn start_bound(&self) -> Bound<&NumberVersion> { Bound::Included(&self.start) @@ -313,3 +323,39 @@ impl Interval for NumberInterval { Self { start, end } } } + +// Helper ################################################################## + +pub fn map_bound T>(f: F, bound: Bound) -> Bound { + match bound { + Bound::Unbounded => Bound::Unbounded, + Bound::Excluded(b) => Bound::Included(f(b)), + Bound::Included(b) => Bound::Excluded(f(b)), + } +} + +pub fn flip_bound(bound: Bound) -> Bound { + match bound { + Bound::Unbounded => Bound::Unbounded, + Bound::Excluded(b) => Bound::Included(b), + Bound::Included(b) => Bound::Excluded(b), + } +} + +/// Will be obsolete when .as_ref() hits stable. +pub fn ref_bound(bound: &Bound) -> Bound<&V> { + match *bound { + Bound::Included(ref x) => Bound::Included(x), + Bound::Excluded(ref x) => Bound::Excluded(x), + Bound::Unbounded => Bound::Unbounded, + } +} + +/// Will be obsolete when .cloned() hits stable. +pub fn owned_bound(bound: Bound<&V>) -> Bound { + match bound { + Bound::Unbounded => Bound::Unbounded, + Bound::Included(x) => Bound::Included(x.clone()), + Bound::Excluded(x) => Bound::Excluded(x.clone()), + } +} From dcbe26fb414002d65c5fbb8068774eb9b3196214 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Tue, 22 Jun 2021 17:03:10 +0200 Subject: [PATCH 06/22] feat: add SemanticInterval --- src/version_trait.rs | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/version_trait.rs b/src/version_trait.rs index b911d01f..39c9540a 100644 --- a/src/version_trait.rs +++ b/src/version_trait.rs @@ -234,6 +234,47 @@ impl Version for SemanticVersion { } } +// SemanticInterval ############################################################ + +#[derive(Debug, Clone, Eq, PartialEq)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +pub struct SemanticInterval { + start: Bound, + end: Bound, +} + +impl RangeBounds for SemanticInterval { + fn start_bound(&self) -> Bound<&SemanticVersion> { + ref_bound(&self.start) + } + fn end_bound(&self) -> Bound<&SemanticVersion> { + ref_bound(&self.end) + } +} + +impl Interval for SemanticInterval { + fn new(start_bound: Bound, end_bound: Bound) -> Self { + let start = match &start_bound { + Bound::Unbounded => Bound::Included(SemanticVersion::zero()), + _ => start_bound, + }; + Self { + start, + end: end_bound, + } + } +} + +// Ease the creation of SemanticInterval from classic ranges: (2, 0, 0)..(2, 5, 0) +impl> From for SemanticInterval { + fn from(range: RB) -> Self { + Self::new( + map_bound(SemanticVersion::from, owned_bound(range.start_bound())), + map_bound(SemanticVersion::from, owned_bound(range.end_bound())), + ) + } +} + // NumberVersion ############################################################### /// Simplest versions possible, just a positive number. From 072f60be9879533c7892058805890dc4243b5df2 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Tue, 22 Jun 2021 17:05:58 +0200 Subject: [PATCH 07/22] refactor: rename Ranges into Range --- src/range_trait.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/range_trait.rs b/src/range_trait.rs index 4690bd11..b614ac36 100644 --- a/src/range_trait.rs +++ b/src/range_trait.rs @@ -25,7 +25,7 @@ use crate::version_trait::{Interval, NumberInterval, NumberVersion, Version}; #[derive(Debug, Clone, Eq, PartialEq)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] -pub struct Ranges { +pub struct Range { segments: SmallVec, phantom: PhantomData, } @@ -104,7 +104,7 @@ impl SidedBound<&V> { } // Ranges building blocks. -impl, V: Version> Ranges { +impl, V: Version> Range { /// Empty set of versions. pub fn empty() -> Self { Self { @@ -162,7 +162,7 @@ impl, V: Version> Ranges { } // Set operations. -impl, V: Version> Ranges { +impl, V: Version> Range { // Negate ################################################################## /// Compute the complement set of versions. @@ -346,7 +346,7 @@ impl, V: Version> Ranges { // REPORT ###################################################################### -impl, V: Version> fmt::Display for Ranges { +impl, V: Version> fmt::Display for Range { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self.segments.as_slice() { [] => write!(f, "∅"), @@ -433,7 +433,7 @@ pub mod tests { // Ranges tests. - pub fn strategy() -> impl Strategy> { + pub fn strategy() -> impl Strategy> { prop::collection::vec(any::(), 0..10).prop_map(|mut vec| { vec.sort_unstable(); vec.dedup(); @@ -445,7 +445,7 @@ pub mod tests { if let [v] = pair_iter.remainder() { segments.push((v..).into()); } - Ranges { + Range { segments, phantom: PhantomData, } @@ -484,12 +484,12 @@ pub mod tests { #[test] fn intersection_with_any_is_identity(ranges in strategy()) { - assert_eq!(Ranges::full().intersection(&ranges), ranges); + assert_eq!(Range::full().intersection(&ranges), ranges); } #[test] fn intersection_with_none_is_none(ranges in strategy()) { - assert_eq!(Ranges::empty().intersection(&ranges), Ranges::empty()); + assert_eq!(Range::empty().intersection(&ranges), Range::empty()); } #[test] @@ -504,7 +504,7 @@ pub mod tests { #[test] fn intesection_of_complements_is_none(ranges in strategy()) { - assert_eq!(ranges.complement().intersection(&ranges), Ranges::empty()); + assert_eq!(ranges.complement().intersection(&ranges), Range::empty()); } #[test] @@ -516,7 +516,7 @@ pub mod tests { #[test] fn union_of_complements_is_any(ranges in strategy()) { - assert_eq!(ranges.complement().union(&ranges), Ranges::full()); + assert_eq!(ranges.complement().union(&ranges), Range::full()); } #[test] @@ -528,7 +528,7 @@ pub mod tests { #[test] fn always_contains_exact(version in version_strat()) { - assert!(Ranges::::singleton(version).contains(&version)); + assert!(Range::::singleton(version).contains(&version)); } #[test] @@ -538,7 +538,7 @@ pub mod tests { #[test] fn contains_intersection(ranges in strategy(), version in version_strat()) { - assert_eq!(ranges.contains(&version), ranges.intersection(&Ranges::singleton(version)) != Ranges::empty()); + assert_eq!(ranges.contains(&version), ranges.intersection(&Range::singleton(version)) != Range::empty()); } } } From 1ced3f3f32cb51dd99984e188d6cddf64d089b94 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Tue, 22 Jun 2021 18:05:55 +0200 Subject: [PATCH 08/22] refactor: change to lib range_trait --- src/error.rs | 7 +- src/internal/core.rs | 37 ++++----- src/internal/incompatibility.rs | 50 ++++++------- src/internal/partial_solution.rs | 80 ++++++++++---------- src/range_trait.rs | 2 + src/report.rs | 124 ++++++++++++++++--------------- src/solver.rs | 54 +++++++------- src/term.rs | 46 ++++++------ src/version_trait.rs | 2 +- 9 files changed, 209 insertions(+), 193 deletions(-) diff --git a/src/error.rs b/src/error.rs index 0553d8de..74e79fa8 100644 --- a/src/error.rs +++ b/src/error.rs @@ -6,14 +6,15 @@ use thiserror::Error; use crate::package::Package; use crate::report::DerivationTree; -use crate::version::Version; +use crate::version_trait::{Interval, Version}; +use std::fmt::Debug; /// Errors that may occur while solving dependencies. #[derive(Error, Debug)] -pub enum PubGrubError { +pub enum PubGrubError + Debug, V: Version> { /// There is no solution for this set of dependencies. #[error("No solution")] - NoSolution(DerivationTree), + NoSolution(DerivationTree), /// Error arising when the implementer of /// [DependencyProvider](crate::solver::DependencyProvider) diff --git a/src/internal/core.rs b/src/internal/core.rs index f923850a..d348191c 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -4,6 +4,7 @@ //! to write a functional PubGrub algorithm. use std::collections::HashSet as Set; +use std::fmt::Debug; use crate::error::PubGrubError; use crate::internal::arena::Arena; @@ -17,26 +18,26 @@ use crate::package::Package; use crate::report::DerivationTree; use crate::solver::DependencyConstraints; use crate::type_aliases::Map; -use crate::version::Version; +use crate::version_trait::{Interval, Version}; /// Current state of the PubGrub algorithm. #[derive(Clone)] -pub struct State { +pub struct State, V: Version> { root_package: P, root_version: V, - incompatibilities: Map>>, + incompatibilities: Map>>, /// Store the ids of incompatibilities that are already contradicted /// and will stay that way until the next conflict and backtrack is operated. - contradicted_incompatibilities: rustc_hash::FxHashSet>, + contradicted_incompatibilities: rustc_hash::FxHashSet>, /// Partial solution. /// TODO: remove pub. - pub partial_solution: PartialSolution, + pub partial_solution: PartialSolution, /// The store is the reference storage for all incompatibilities. - pub incompatibility_store: Arena>, + pub incompatibility_store: Arena>, /// This is a stack of work to be done in `unit_propagation`. /// It can definitely be a local variable to that method, but @@ -44,7 +45,7 @@ pub struct State { unit_propagation_buffer: SmallVec

, } -impl State { +impl + Debug, V: Version> State { /// Initialization of PubGrub state. pub fn init(root_package: P, root_version: V) -> Self { let mut incompatibility_store = Arena::new(); @@ -66,7 +67,7 @@ impl State { } /// Add an incompatibility to the state. - pub fn add_incompatibility(&mut self, incompat: Incompatibility) { + pub fn add_incompatibility(&mut self, incompat: Incompatibility) { let id = self.incompatibility_store.alloc(incompat); self.merge_incompatibility(id); } @@ -76,8 +77,8 @@ impl State { &mut self, package: P, version: V, - deps: &DependencyConstraints, - ) -> std::ops::Range> { + deps: &DependencyConstraints, + ) -> std::ops::Range> { // Create incompatibilities and allocate them in the store. let new_incompats_id_range = self .incompatibility_store @@ -92,13 +93,13 @@ impl State { } /// Check if an incompatibility is terminal. - pub fn is_terminal(&self, incompatibility: &Incompatibility) -> bool { + pub fn is_terminal(&self, incompatibility: &Incompatibility) -> bool { incompatibility.is_terminal(&self.root_package, &self.root_version) } /// Unit propagation is the core mechanism of the solving algorithm. /// CF - pub fn unit_propagation(&mut self, package: P) -> Result<(), PubGrubError> { + pub fn unit_propagation(&mut self, package: P) -> Result<(), PubGrubError> { self.unit_propagation_buffer.clear(); self.unit_propagation_buffer.push(package); while let Some(current_package) = self.unit_propagation_buffer.pop() { @@ -158,8 +159,8 @@ impl State { /// CF fn conflict_resolution( &mut self, - incompatibility: IncompId, - ) -> Result<(P, IncompId), PubGrubError> { + incompatibility: IncompId, + ) -> Result<(P, IncompId), PubGrubError> { let mut current_incompat_id = incompatibility; let mut current_incompat_changed = false; loop { @@ -203,7 +204,7 @@ impl State { /// Backtracking. fn backtrack( &mut self, - incompat: IncompId, + incompat: IncompId, incompat_changed: bool, decision_level: DecisionLevel, ) { @@ -234,7 +235,7 @@ 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, id: IncompId) { for (pkg, _term) in self.incompatibility_store[id].iter() { self.incompatibilities .entry(pkg.clone()) @@ -245,12 +246,12 @@ impl State { // Error reporting ######################################################### - fn build_derivation_tree(&self, incompat: IncompId) -> DerivationTree { + fn build_derivation_tree(&self, incompat: IncompId) -> DerivationTree { let shared_ids = self.find_shared_ids(incompat); Incompatibility::build_derivation_tree(incompat, &shared_ids, &self.incompatibility_store) } - fn find_shared_ids(&self, incompat: IncompId) -> Set> { + fn find_shared_ids(&self, incompat: IncompId) -> Set> { let mut all_ids = Set::new(); let mut shared_ids = Set::new(); let mut stack = vec![incompat]; diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index acf900b8..a7d1e327 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -9,10 +9,10 @@ use std::fmt; use crate::internal::arena::{Arena, Id}; use crate::internal::small_map::SmallMap; use crate::package::Package; -use crate::range::Range; +use crate::range_trait::Range; use crate::report::{DefaultStringReporter, DerivationTree, Derived, External}; use crate::term::{self, Term}; -use crate::version::Version; +use crate::version_trait::{Interval, Version}; /// An incompatibility is a set of terms for different packages /// that should never be satisfied all together. @@ -30,26 +30,26 @@ use crate::version::Version; /// during conflict resolution. More about all this in /// [PubGrub documentation](https://github.com/dart-lang/pub/blob/master/doc/solver.md#incompatibility). #[derive(Debug, Clone)] -pub struct Incompatibility { - package_terms: SmallMap>, - kind: Kind, +pub struct Incompatibility, V: Version> { + package_terms: SmallMap>, + kind: Kind, } /// Type alias of unique identifiers for incompatibilities. -pub type IncompId = Id>; +pub type IncompId = Id>; #[derive(Debug, Clone)] -enum Kind { +enum Kind, 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), + NoVersions(P, Range), /// Dependencies of the package are unavailable for versions in that range. - UnavailableDependencies(P, Range), + UnavailableDependencies(P, Range), /// Incompatibility coming from the dependencies of a given package. - FromDependencyOf(P, Range, P, Range), + FromDependencyOf(P, Range, P, Range), /// Derived from two causes. Stores cause ids. - DerivedFrom(IncompId, IncompId), + DerivedFrom(IncompId, IncompId), } /// A Relation describes how a set of terms can be compared to an incompatibility. @@ -69,13 +69,13 @@ pub enum Relation { Inconclusive, } -impl Incompatibility { +impl, V: Version> Incompatibility { /// Create the initial "not Root" incompatibility. pub fn not_root(package: P, version: V) -> Self { Self { package_terms: SmallMap::One([( package.clone(), - Term::Negative(Range::exact(version.clone())), + Term::Negative(Range::singleton(version.clone())), )]), kind: Kind::NotRoot(package, version), } @@ -83,7 +83,7 @@ impl Incompatibility { /// Create an incompatibility to remember /// that a given range does not contain any version. - pub fn no_versions(package: P, term: Term) -> Self { + pub fn no_versions(package: P, term: Term) -> Self { let range = match &term { Term::Positive(r) => r.clone(), Term::Negative(_) => panic!("No version should have a positive term"), @@ -98,7 +98,7 @@ impl Incompatibility { /// 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); + let range = Range::singleton(version); Self { package_terms: SmallMap::One([(package.clone(), Term::Positive(range.clone()))]), kind: Kind::UnavailableDependencies(package, range), @@ -106,8 +106,8 @@ impl Incompatibility { } /// Build an incompatibility from a given dependency. - pub fn from_dependency(package: P, version: V, dep: (&P, &Range)) -> Self { - let range1 = Range::exact(version); + pub fn from_dependency(package: P, version: V, dep: (&P, &Range)) -> Self { + let range1 = Range::singleton(version); let (p2, range2) = dep; Self { package_terms: SmallMap::Two([ @@ -157,12 +157,12 @@ impl Incompatibility { } /// Get the term related to a given package (if it exists). - pub fn get(&self, package: &P) -> Option<&Term> { + pub fn get(&self, package: &P) -> Option<&Term> { self.package_terms.get(package) } /// Iterate over packages. - pub fn iter(&self) -> impl Iterator)> { + pub fn iter(&self) -> impl Iterator)> { self.package_terms.iter() } @@ -181,7 +181,7 @@ impl Incompatibility { self_id: Id, shared_ids: &Set>, store: &Arena, - ) -> DerivationTree { + ) -> DerivationTree { match &store[self_id].kind { Kind::DerivedFrom(id1, id2) => { let cause1 = Self::build_derivation_tree(*id1, shared_ids, store); @@ -215,9 +215,9 @@ impl Incompatibility { } } -impl<'a, P: Package, V: Version + 'a> Incompatibility { +impl<'a, P: Package, I: Interval + 'a, V: Version + '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(&P) -> Option<&'a Term>) -> Relation

{ let mut relation = Relation::Satisfied; for (package, incompat_term) in self.package_terms.iter() { match terms(package).map(|term| incompat_term.relation_with(&term)) { @@ -243,7 +243,7 @@ impl<'a, P: Package, V: Version + 'a> Incompatibility { } } -impl fmt::Display for Incompatibility { +impl, V: Version> fmt::Display for Incompatibility { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!( f, @@ -276,12 +276,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", Range::full()) }); let i2 = store.alloc(Incompatibility { package_terms: SmallMap::Two([("p2", t2), ("p3", t3.clone())]), - kind: Kind::UnavailableDependencies("0", Range::any()) + kind: Kind::UnavailableDependencies("0", Range::full()) }); let mut i3 = Map::default(); diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index ad454244..b5666b0f 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -7,10 +7,10 @@ use crate::internal::arena::Arena; use crate::internal::incompatibility::{IncompId, Incompatibility, Relation}; use crate::internal::small_map::SmallMap; use crate::package::Package; -use crate::range::Range; +use crate::range_trait::Range; use crate::term::Term; use crate::type_aliases::{Map, SelectedDependencies}; -use crate::version::Version; +use crate::version_trait::{Interval, Version}; use super::small_vec::SmallVec; @@ -26,47 +26,47 @@ impl DecisionLevel { /// The partial solution contains all package assignments, /// organized by package and historically ordered. #[derive(Clone, Debug)] -pub struct PartialSolution { +pub struct PartialSolution, V: Version> { next_global_index: u32, current_decision_level: DecisionLevel, - package_assignments: Map>, + package_assignments: Map>, } /// Package assignments contain the potential decision and derivations /// that have already been made for a given package, /// as well as the intersection of terms by all of these. #[derive(Clone, Debug)] -struct PackageAssignments { +struct PackageAssignments, V: Version> { smallest_decision_level: DecisionLevel, highest_decision_level: DecisionLevel, - dated_derivations: SmallVec>, - assignments_intersection: AssignmentsIntersection, + dated_derivations: SmallVec>, + assignments_intersection: AssignmentsIntersection, } #[derive(Clone, Debug)] -pub struct DatedDerivation { +pub struct DatedDerivation, V: Version> { global_index: u32, decision_level: DecisionLevel, - cause: IncompId, + cause: IncompId, } #[derive(Clone, Debug)] -enum AssignmentsIntersection { - Decision((u32, V, Term)), - Derivations(Term), +enum AssignmentsIntersection, V: Version> { + Decision((u32, V, Term)), + Derivations(Term), } #[derive(Clone, Debug)] -pub enum SatisfierSearch { +pub enum SatisfierSearch, V: Version> { DifferentDecisionLevels { previous_satisfier_level: DecisionLevel, }, SameDecisionLevels { - satisfier_cause: IncompId, + satisfier_cause: IncompId, }, } -impl PartialSolution { +impl, V: Version> PartialSolution { /// Initialize an empty PartialSolution. pub fn empty() -> Self { Self { @@ -110,8 +110,8 @@ impl PartialSolution { pub fn add_derivation( &mut self, package: P, - cause: IncompId, - store: &Arena>, + cause: IncompId, + store: &Arena>, ) { use std::collections::hash_map::Entry; let term = store[cause].get(&package).unwrap().negate(); @@ -153,7 +153,7 @@ impl PartialSolution { /// selected version (no "decision") /// and if it contains at least one positive derivation term /// in the partial solution. - pub fn potential_packages(&self) -> Option)>> { + pub fn potential_packages(&self) -> Option)>> { let mut iter = self .package_assignments .iter() @@ -190,7 +190,7 @@ impl PartialSolution { pub fn backtrack( &mut self, decision_level: DecisionLevel, - store: &Arena>, + store: &Arena>, ) { self.current_decision_level = decision_level; self.package_assignments.retain(|p, pa| { @@ -241,11 +241,11 @@ impl PartialSolution { &mut self, package: P, version: V, - new_incompatibilities: std::ops::Range>, - store: &Arena>, + new_incompatibilities: std::ops::Range>, + store: &Arena>, ) { let exact = Term::exact(version.clone()); - let not_satisfied = |incompat: &Incompatibility| { + let not_satisfied = |incompat: &Incompatibility| { incompat.relation(|p| { if p == &package { Some(&exact) @@ -263,12 +263,12 @@ impl PartialSolution { } /// Check if the terms in the partial solution satisfy the incompatibility. - pub fn relation(&self, incompat: &Incompatibility) -> Relation

{ + pub fn relation(&self, incompat: &Incompatibility) -> Relation

{ incompat.relation(|package| self.term_intersection_for_package(package)) } /// Retrieve intersection of terms related to package. - pub fn term_intersection_for_package(&self, package: &P) -> Option<&Term> { + pub fn term_intersection_for_package(&self, package: &P) -> Option<&Term> { self.package_assignments .get(package) .map(|pa| pa.assignments_intersection.term()) @@ -277,9 +277,9 @@ impl PartialSolution { /// Figure out if the satisfier and previous satisfier are of different decision levels. pub fn satisfier_search( &self, - incompat: &Incompatibility, - store: &Arena>, - ) -> (P, SatisfierSearch) { + incompat: &Incompatibility, + store: &Arena>, + ) -> (P, SatisfierSearch) { let satisfied_map = Self::find_satisfier(incompat, &self.package_assignments, store); let (satisfier_package, &(satisfier_index, _, satisfier_decision_level)) = satisfied_map .iter() @@ -318,9 +318,9 @@ 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. fn find_satisfier( - incompat: &Incompatibility, - package_assignments: &Map>, - store: &Arena>, + incompat: &Incompatibility, + package_assignments: &Map>, + store: &Arena>, ) -> SmallMap { let mut satisfied = SmallMap::Empty; for (package, incompat_term) in incompat.iter() { @@ -337,11 +337,11 @@ impl PartialSolution { /// such that incompatibility is satisfied by the partial solution up to /// and including that assignment plus satisfier. fn find_previous_satisfier( - incompat: &Incompatibility, + incompat: &Incompatibility, satisfier_package: &P, mut satisfied_map: SmallMap, - package_assignments: &Map>, - store: &Arena>, + package_assignments: &Map>, + store: &Arena>, ) -> DecisionLevel { // First, let's retrieve the previous derivations and the initial accum_term. let satisfier_pa = package_assignments.get(satisfier_package).unwrap(); @@ -375,13 +375,13 @@ impl PartialSolution { } } -impl PackageAssignments { +impl, V: Version> PackageAssignments { fn satisfier( &self, package: &P, - incompat_term: &Term, - start_term: Term, - store: &Arena>, + incompat_term: &Term, + start_term: Term, + store: &Arena>, ) -> (usize, u32, DecisionLevel) { // Term where we accumulate intersections until incompat_term is satisfied. let mut accum_term = start_term; @@ -413,9 +413,9 @@ impl PackageAssignments { } } -impl AssignmentsIntersection { +impl, V: Version> AssignmentsIntersection { /// Returns the term intersection of all assignments (decision included). - fn term(&self) -> &Term { + fn term(&self) -> &Term { match self { Self::Decision((_, _, term)) => term, Self::Derivations(term) => term, @@ -429,7 +429,7 @@ impl AssignmentsIntersection { fn potential_package_filter<'a, P: Package>( &'a self, package: &'a P, - ) -> Option<(&'a P, &'a Range)> { + ) -> Option<(&'a P, &'a Range)> { match self { Self::Decision(_) => None, Self::Derivations(term_intersection) => { diff --git a/src/range_trait.rs b/src/range_trait.rs index b614ac36..6f10497b 100644 --- a/src/range_trait.rs +++ b/src/range_trait.rs @@ -25,8 +25,10 @@ use crate::version_trait::{Interval, NumberInterval, NumberVersion, Version}; #[derive(Debug, Clone, Eq, PartialEq)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +#[cfg_attr(feature = "serde", serde(transparent))] pub struct Range { segments: SmallVec, + #[cfg_attr(feature = "serde", serde(skip_serializing))] phantom: PhantomData, } diff --git a/src/report.rs b/src/report.rs index 09815821..6ce1e891 100644 --- a/src/report.rs +++ b/src/report.rs @@ -7,50 +7,50 @@ use std::fmt; use std::ops::{Deref, DerefMut}; use crate::package::Package; -use crate::range::Range; +use crate::range_trait::Range; use crate::term::Term; use crate::type_aliases::Map; -use crate::version::Version; +use crate::version_trait::{Interval, Version}; /// Reporter trait. -pub trait Reporter { +pub trait Reporter, V: Version> { /// Output type of the report. type Output; /// Generate a report from the derivation tree /// describing the resolution failure. - fn report(derivation_tree: &DerivationTree) -> Self::Output; + fn report(derivation_tree: &DerivationTree) -> Self::Output; } /// Derivation tree resulting in the impossibility /// to solve the dependencies of our root package. #[derive(Debug, Clone)] -pub enum DerivationTree { +pub enum DerivationTree, V: Version> { /// External incompatibility. - External(External), + External(External), /// Incompatibility derived from two others. - Derived(Derived), + Derived(Derived), } /// Incompatibilities that are not derived from others, /// they have their own reason. #[derive(Debug, Clone)] -pub enum External { +pub enum External, 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), + NoVersions(P, Range), /// Dependencies of the package are unavailable for versions in that range. - UnavailableDependencies(P, Range), + UnavailableDependencies(P, Range), /// Incompatibility coming from the dependencies of a given package. - FromDependencyOf(P, Range, P, Range), + FromDependencyOf(P, Range, P, Range), } /// Incompatibility derived from two others. #[derive(Debug, Clone)] -pub struct Derived { +pub struct Derived, V: Version> { /// Terms of the incompatibility. - pub terms: Map>, + pub terms: Map>, /// Indicate if that incompatibility is present multiple times /// in the derivation tree. /// If that is the case, it has a unique id, provided in that option. @@ -58,12 +58,12 @@ pub struct Derived { /// and refer to the explanation for the other times. pub shared_id: Option, /// First cause. - pub cause1: Box>, + pub cause1: Box>, /// Second cause. - pub cause2: Box>, + pub cause2: Box>, } -impl DerivationTree { +impl, V: Version> DerivationTree { /// Merge the [NoVersions](External::NoVersions) external incompatibilities /// with the other one they are matched with /// in a derived incompatibility. @@ -100,7 +100,7 @@ impl DerivationTree { } } - fn merge_no_versions(self, package: P, range: Range) -> Option { + fn merge_no_versions(self, package: P, range: Range) -> Option { match self { // TODO: take care of the Derived case. // Once done, we can remove the Option. @@ -138,21 +138,21 @@ impl DerivationTree { } } -impl fmt::Display for External { +impl, V: Version> fmt::Display for External { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Self::NotRoot(package, version) => { write!(f, "we are solving dependencies of {} {}", package, version) } Self::NoVersions(package, range) => { - if range == &Range::any() { + if range == &Range::full() { write!(f, "there is no available version for {}", package) } else { write!(f, "there is no version of {} in {}", package, range) } } Self::UnavailableDependencies(package, range) => { - if range == &Range::any() { + if range == &Range::full() { write!(f, "dependencies of {} are unavailable", package) } else { write!( @@ -163,11 +163,11 @@ impl fmt::Display for External { } } Self::FromDependencyOf(p, range_p, dep, range_dep) => { - if range_p == &Range::any() && range_dep == &Range::any() { + if range_p == &Range::full() && range_dep == &Range::full() { write!(f, "{} depends on {}", p, dep) - } else if range_p == &Range::any() { + } else if range_p == &Range::full() { write!(f, "{} depends on {} {}", p, dep, range_dep) - } else if range_dep == &Range::any() { + } else if range_dep == &Range::full() { write!(f, "{} {} depends on {}", p, range_p, dep) } else { write!(f, "{} {} depends on {} {}", p, range_p, dep, range_dep) @@ -198,7 +198,10 @@ impl DefaultStringReporter { } } - fn build_recursive(&mut self, derived: &Derived) { + fn build_recursive, V: Version>( + &mut self, + derived: &Derived, + ) { self.build_recursive_helper(derived); if let Some(id) = derived.shared_id { if self.shared_with_ref.get(&id) == None { @@ -208,7 +211,10 @@ impl DefaultStringReporter { }; } - fn build_recursive_helper(&mut self, current: &Derived) { + fn build_recursive_helper, V: Version>( + &mut self, + current: &Derived, + ) { match (current.cause1.deref(), current.cause2.deref()) { (DerivationTree::External(external1), DerivationTree::External(external2)) => { // Simplest case, we just combine two external incompatibilities. @@ -286,11 +292,11 @@ impl DefaultStringReporter { /// /// The result will depend on the fact that the derived incompatibility /// has already been explained or not. - fn report_one_each( + fn report_one_each, V: Version>( &mut self, - derived: &Derived, - external: &External, - current_terms: &Map>, + derived: &Derived, + external: &External, + current_terms: &Map>, ) { match self.line_ref_of(derived.shared_id) { Some(ref_id) => self.lines.push(Self::explain_ref_and_external( @@ -304,11 +310,11 @@ impl DefaultStringReporter { } /// Report one derived (without a line ref yet) and one external. - fn report_recurse_one_each( + fn report_recurse_one_each, V: Version>( &mut self, - derived: &Derived, - external: &External, - current_terms: &Map>, + derived: &Derived, + external: &External, + current_terms: &Map>, ) { match (derived.cause1.deref(), derived.cause2.deref()) { // If the derived cause has itself one external prior cause, @@ -342,10 +348,10 @@ impl DefaultStringReporter { // String explanations ##################################################### /// Simplest case, we just combine two external incompatibilities. - fn explain_both_external( - external1: &External, - external2: &External, - current_terms: &Map>, + fn explain_both_external, V: Version>( + external1: &External, + external2: &External, + current_terms: &Map>, ) -> String { // TODO: order should be chosen to make it more logical. format!( @@ -357,12 +363,12 @@ impl DefaultStringReporter { } /// Both causes have already been explained so we use their refs. - fn explain_both_ref( + fn explain_both_ref, V: Version>( ref_id1: usize, - derived1: &Derived, + derived1: &Derived, ref_id2: usize, - derived2: &Derived, - current_terms: &Map>, + derived2: &Derived, + current_terms: &Map>, ) -> String { // TODO: order should be chosen to make it more logical. format!( @@ -378,11 +384,11 @@ impl DefaultStringReporter { /// One cause is derived (already explained so one-line), /// the other is a one-line external cause, /// and finally we conclude with the current incompatibility. - fn explain_ref_and_external( + fn explain_ref_and_external, V: Version>( ref_id: usize, - derived: &Derived, - external: &External, - current_terms: &Map>, + derived: &Derived, + external: &External, + current_terms: &Map>, ) -> String { // TODO: order should be chosen to make it more logical. format!( @@ -395,9 +401,9 @@ impl DefaultStringReporter { } /// Add an external cause to the chain of explanations. - fn and_explain_external( - external: &External, - current_terms: &Map>, + fn and_explain_external, V: Version>( + external: &External, + current_terms: &Map>, ) -> String { format!( "And because {}, {}.", @@ -407,10 +413,10 @@ impl DefaultStringReporter { } /// Add an already explained incompat to the chain of explanations. - fn and_explain_ref( + fn and_explain_ref, V: Version>( ref_id: usize, - derived: &Derived, - current_terms: &Map>, + derived: &Derived, + current_terms: &Map>, ) -> String { format!( "And because {} ({}), {}.", @@ -421,10 +427,10 @@ impl DefaultStringReporter { } /// Add an already explained incompat to the chain of explanations. - fn and_explain_prior_and_external( - prior_external: &External, - external: &External, - current_terms: &Map>, + fn and_explain_prior_and_external, V: Version>( + prior_external: &External, + external: &External, + current_terms: &Map>, ) -> String { format!( "And because {} and {}, {}.", @@ -435,7 +441,9 @@ impl DefaultStringReporter { } /// Try to print terms of an incompatibility in a human-readable way. - pub fn string_terms(terms: &Map>) -> String { + pub fn string_terms, V: Version>( + terms: &Map>, + ) -> String { let terms_vec: Vec<_> = terms.iter().collect(); match terms_vec.as_slice() { [] => "version solving failed".into(), @@ -470,10 +478,10 @@ impl DefaultStringReporter { } } -impl Reporter for DefaultStringReporter { +impl, V: Version> Reporter for DefaultStringReporter { type Output = String; - fn report(derivation_tree: &DerivationTree) -> Self::Output { + fn report(derivation_tree: &DerivationTree) -> Self::Output { match derivation_tree { DerivationTree::External(external) => external.to_string(), DerivationTree::Derived(derived) => { diff --git a/src/solver.rs b/src/solver.rs index 62015911..2d63d3bc 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -68,22 +68,23 @@ use std::borrow::Borrow; use std::collections::{BTreeMap, BTreeSet as Set}; use std::error::Error; +use std::fmt::Debug; use crate::error::PubGrubError; use crate::internal::core::State; use crate::internal::incompatibility::Incompatibility; use crate::package::Package; -use crate::range::Range; +use crate::range_trait::Range; use crate::type_aliases::{Map, SelectedDependencies}; -use crate::version::Version; +use crate::version_trait::{Interval, Version}; /// Main function of the library. /// Finds a set of packages satisfying dependency bounds for a given package + version pair. -pub fn resolve( - dependency_provider: &impl DependencyProvider, +pub fn resolve + Debug, V: Version>( + dependency_provider: &impl DependencyProvider, package: P, version: impl Into, -) -> Result, PubGrubError> { +) -> Result, PubGrubError> { let mut state = State::init(package.clone(), version.into()); let mut added_dependencies: Map> = Map::default(); let mut next = package; @@ -159,7 +160,8 @@ pub fn resolve( version: v.clone(), }); } - if let Some((dependent, _)) = x.iter().find(|(_, r)| r == &&Range::none()) { + if let Some((dependent, _)) = x.iter().find(|(_, r)| r == &&Range::empty()) + { return Err(PubGrubError::DependencyOnTheEmptySet { package: p.clone(), version: v.clone(), @@ -203,11 +205,11 @@ pub fn resolve( /// An enum used by [DependencyProvider] that holds information about package dependencies. /// For each [Package] there is a [Range] of concrete versions it allows as a dependency. #[derive(Clone)] -pub enum Dependencies { +pub enum Dependencies, V: Version> { /// Package dependencies are unavailable. Unknown, /// Container for all available package versions. - Known(DependencyConstraints), + Known(DependencyConstraints), } /// Subtype of [Dependencies] which holds information about @@ -216,11 +218,11 @@ pub enum Dependencies { /// inside [DependencyConstraints] and [Dependencies::Unknown]: /// the former means the package has no dependencies and it is a known fact, /// while the latter means they could not be fetched by [DependencyProvider]. -pub type DependencyConstraints = Map>; +pub type DependencyConstraints = Map>; /// Trait that allows the algorithm to retrieve available packages and their dependencies. /// An implementor needs to be supplied to the [resolve] function. -pub trait DependencyProvider { +pub trait DependencyProvider, V: Version> { /// [Decision making](https://github.com/dart-lang/pub/blob/master/doc/solver.md#decision-making) /// is the process of choosing the next package /// and version that will be appended to the partial solution. @@ -246,7 +248,7 @@ pub trait DependencyProvider { /// of the available versions in preference order for any package. /// /// Note: the type `T` ensures that this returns an item from the `packages` argument. - fn choose_package_version, U: Borrow>>( + fn choose_package_version, U: Borrow>>( &self, potential_packages: impl Iterator, ) -> Result<(T, Option), Box>; @@ -257,7 +259,7 @@ pub trait DependencyProvider { &self, package: &P, version: &V, - ) -> Result, Box>; + ) -> Result, Box>; /// This is called fairly regularly during the resolution, /// if it returns an Err then resolution will be terminated. @@ -276,15 +278,15 @@ pub trait DependencyProvider { /// The helper finds the package from the `packages` argument with the fewest versions from /// `list_available_versions` contained in the constraints. Then takes that package and finds the /// first version contained in the constraints. -pub fn choose_package_with_fewest_versions( +pub fn choose_package_with_fewest_versions, V: Version, T, U, It, F>( list_available_versions: F, potential_packages: impl Iterator, ) -> (T, Option) where T: Borrow

, - U: Borrow>, - I: Iterator, - F: Fn(&P) -> I, + U: Borrow>, + It: Iterator, + F: Fn(&P) -> It, { let count_valid = |(p, range): &(T, U)| { list_available_versions(p.borrow()) @@ -303,11 +305,11 @@ where #[derive(Debug, Clone, Default)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] #[cfg_attr(feature = "serde", serde(transparent))] -pub struct OfflineDependencyProvider { - dependencies: Map>>, +pub struct OfflineDependencyProvider, V: Version> { + dependencies: Map>>, } -impl OfflineDependencyProvider { +impl, V: Version> OfflineDependencyProvider { /// Creates an empty OfflineDependencyProvider with no dependencies. pub fn new() -> Self { Self { @@ -325,11 +327,11 @@ impl OfflineDependencyProvider { /// The API does not allow to add dependencies one at a time to uphold an assumption that /// [OfflineDependencyProvider.get_dependencies(p, v)](OfflineDependencyProvider::get_dependencies) /// provides all dependencies of a given package (p) and version (v) pair. - pub fn add_dependencies)>>( + pub fn add_dependencies)>>( &mut self, package: P, version: impl Into, - dependencies: I, + dependencies: It, ) { let package_deps = dependencies.into_iter().collect(); let v = version.into(); @@ -354,7 +356,7 @@ impl OfflineDependencyProvider { /// Lists dependencies of a given package and version. /// Returns [None] if no information is available regarding that package and version pair. - fn dependencies(&self, package: &P, version: &V) -> Option> { + fn dependencies(&self, package: &P, version: &V) -> Option> { self.dependencies.get(package)?.get(version).cloned() } } @@ -363,8 +365,10 @@ impl OfflineDependencyProvider { /// contains all dependency information available in memory. /// Packages are picked with the fewest versions contained in the constraints first. /// Versions are picked with the newest versions first. -impl DependencyProvider for OfflineDependencyProvider { - fn choose_package_version, U: Borrow>>( +impl, V: Version> DependencyProvider + for OfflineDependencyProvider +{ + fn choose_package_version, U: Borrow>>( &self, potential_packages: impl Iterator, ) -> Result<(T, Option), Box> { @@ -385,7 +389,7 @@ impl DependencyProvider for OfflineDependencyProvi &self, package: &P, version: &V, - ) -> Result, Box> { + ) -> Result, Box> { Ok(match self.dependencies(package, version) { None => Dependencies::Unknown, Some(dependencies) => Dependencies::Known(dependencies), diff --git a/src/term.rs b/src/term.rs index bc038acf..3d388ad7 100644 --- a/src/term.rs +++ b/src/term.rs @@ -3,38 +3,38 @@ //! A term is the fundamental unit of operation of the PubGrub algorithm. //! It is a positive or negative expression regarding a set of versions. -use crate::range::Range; -use crate::version::Version; +use crate::range_trait::Range; +use crate::version_trait::{Interval, Version}; use std::fmt; /// A positive or negative expression regarding a set of versions. #[derive(Debug, Clone, Eq, PartialEq)] -pub enum Term { +pub enum Term, V: Version> { /// For example, "1.0.0 <= v < 2.0.0" is a positive expression /// that is evaluated true if a version is selected /// and comprised between version 1.0.0 and version 2.0.0. - Positive(Range), + Positive(Range), /// The term "not v < 3.0.0" is a negative expression /// that is evaluated true if a version is selected >= 3.0.0 /// or if no version is selected at all. - Negative(Range), + Negative(Range), } /// Base methods. -impl Term { +impl, V: Version> Term { /// A term that is always true. pub(crate) fn any() -> Self { - Self::Negative(Range::none()) + Self::Negative(Range::empty()) } /// A term that is never true. pub(crate) fn empty() -> Self { - Self::Positive(Range::none()) + Self::Positive(Range::empty()) } /// A positive term containing exactly that version. pub(crate) fn exact(version: V) -> Self { - Self::Positive(Range::exact(version)) + Self::Positive(Range::singleton(version)) } /// Simply check if a term is positive. @@ -65,7 +65,7 @@ impl Term { /// Unwrap the range contains in a positive term. /// Will panic if used on a negative range. - pub(crate) fn unwrap_positive(&self) -> &Range { + pub(crate) fn unwrap_positive(&self) -> &Range { match self { Self::Positive(range) => range, _ => panic!("Negative term cannot unwrap positive range"), @@ -74,17 +74,17 @@ impl Term { } /// Set operations with terms. -impl Term { +impl, V: Version> Term { /// Compute the intersection of two terms. /// If at least one term is positive, the intersection is also positive. - pub(crate) fn intersection(&self, other: &Term) -> Term { + pub(crate) fn intersection(&self, other: &Term) -> Term { match (self, other) { (Self::Positive(r1), Self::Positive(r2)) => Self::Positive(r1.intersection(r2)), (Self::Positive(r1), Self::Negative(r2)) => { - Self::Positive(r1.intersection(&r2.negate())) + Self::Positive(r1.intersection(&r2.complement())) } (Self::Negative(r1), Self::Positive(r2)) => { - Self::Positive(r1.negate().intersection(r2)) + Self::Positive(r1.complement().intersection(r2)) } (Self::Negative(r1), Self::Negative(r2)) => Self::Negative(r1.union(r2)), } @@ -92,14 +92,14 @@ impl Term { /// Compute the union of two terms. /// If at least one term is negative, the union is also negative. - pub(crate) fn union(&self, other: &Term) -> Term { + pub(crate) fn union(&self, other: &Term) -> Term { (self.negate().intersection(&other.negate())).negate() } /// Indicate if this term is a subset of another term. /// Just like for sets, we say that t1 is a subset of t2 /// if and only if t1 ∩ t2 = t1. - pub(crate) fn subset_of(&self, other: &Term) -> bool { + pub(crate) fn subset_of(&self, other: &Term) -> bool { self == &self.intersection(other) } } @@ -120,7 +120,7 @@ pub(crate) enum Relation { } /// Relation between terms. -impl<'a, V: 'a + Version> Term { +impl<'a, I: Interval, V: 'a + Version> Term { /// Check if a set of terms satisfies this term. /// /// We say that a set of terms S "satisfies" a term t @@ -129,7 +129,7 @@ impl<'a, V: 'a + Version> Term { /// It turns out that this can also be expressed with set operations: /// S satisfies t if and only if ⋂ S ⊆ t #[cfg(test)] - fn satisfied_by(&self, terms_intersection: &Term) -> bool { + fn satisfied_by(&self, terms_intersection: &Term) -> bool { terms_intersection.subset_of(self) } @@ -142,13 +142,13 @@ impl<'a, V: 'a + Version> Term { /// S contradicts t if and only if ⋂ S is disjoint with t /// S contradicts t if and only if (⋂ S) ⋂ t = ∅ #[cfg(test)] - fn contradicted_by(&self, terms_intersection: &Term) -> bool { + fn contradicted_by(&self, terms_intersection: &Term) -> bool { terms_intersection.intersection(self) == Self::empty() } /// Check if a set of terms satisfies or contradicts a given term. /// Otherwise the relation is inconclusive. - pub(crate) fn relation_with(&self, other_terms_intersection: &Term) -> Relation { + pub(crate) fn relation_with(&self, other_terms_intersection: &Term) -> Relation { let full_intersection = self.intersection(other_terms_intersection); if &full_intersection == other_terms_intersection { Relation::Satisfied @@ -160,15 +160,15 @@ impl<'a, V: 'a + Version> Term { } } -impl AsRef> for Term { - fn as_ref(&self) -> &Term { +impl, V: Version> AsRef> for Term { + fn as_ref(&self) -> &Term { &self } } // REPORT ###################################################################### -impl fmt::Display for Term { +impl, V: Version + fmt::Display> fmt::Display for Term { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Self::Positive(range) => write!(f, "{}", range), diff --git a/src/version_trait.rs b/src/version_trait.rs index 39c9540a..aefb6f9a 100644 --- a/src/version_trait.rs +++ b/src/version_trait.rs @@ -17,7 +17,7 @@ pub trait Version: Clone + Ord + Debug + Display { /// An interval is a bounded domain containing all values /// between its starting and ending bounds. -pub trait Interval: RangeBounds { +pub trait Interval: RangeBounds + Debug + Clone + Eq + PartialEq { /// Create an interval from its starting and ending bounds. /// It's the caller responsability to order them correctly. fn new(start_bound: Bound, end_bound: Bound) -> Self; From 948d2c4ac3c862bca47f66b16e31d0ed376e9645 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Tue, 22 Jun 2021 18:43:11 +0200 Subject: [PATCH 09/22] refactor: update examples and tests to range_trait --- examples/branching_error_reporting.rs | 7 ++-- examples/caching_dependency_provider.rs | 28 +++++++++------ examples/doc_interface.rs | 12 +++---- examples/doc_interface_error.rs | 22 ++++++------ examples/doc_interface_semantic.rs | 18 +++++----- examples/linear_error_reporting.rs | 7 ++-- src/lib.rs | 4 +-- src/term.rs | 8 ++--- tests/examples.rs | 27 ++++++++------ tests/proptest.rs | 48 ++++++++++++++----------- tests/sat_dependency_provider.rs | 12 ++++--- tests/tests.rs | 21 ++++++----- 12 files changed, 121 insertions(+), 93 deletions(-) diff --git a/examples/branching_error_reporting.rs b/examples/branching_error_reporting.rs index c1daa1bf..64f431c4 100644 --- a/examples/branching_error_reporting.rs +++ b/examples/branching_error_reporting.rs @@ -1,14 +1,15 @@ // SPDX-License-Identifier: MPL-2.0 use pubgrub::error::PubGrubError; -use pubgrub::range::Range; +use pubgrub::range_trait::Range; use pubgrub::report::{DefaultStringReporter, Reporter}; use pubgrub::solver::{resolve, OfflineDependencyProvider}; -use pubgrub::version::SemanticVersion; +use pubgrub::version_trait::{SemanticInterval, SemanticVersion}; // https://github.com/dart-lang/pub/blob/master/doc/solver.md#branching-error-reporting fn main() { - let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); + let mut dependency_provider = + OfflineDependencyProvider::<&str, SemanticInterval, SemanticVersion>::new(); #[rustfmt::skip] // root 1.0.0 depends on foo ^1.0.0 dependency_provider.add_dependencies( diff --git a/examples/caching_dependency_provider.rs b/examples/caching_dependency_provider.rs index bac730ea..b3efbc31 100644 --- a/examples/caching_dependency_provider.rs +++ b/examples/caching_dependency_provider.rs @@ -4,18 +4,25 @@ use std::cell::RefCell; use std::error::Error; use pubgrub::package::Package; -use pubgrub::range::Range; +use pubgrub::range_trait::Range; use pubgrub::solver::{resolve, Dependencies, DependencyProvider, OfflineDependencyProvider}; -use pubgrub::version::{NumberVersion, Version}; +use pubgrub::version_trait::{Interval, NumberInterval, NumberVersion, Version}; // An example implementing caching dependency provider that will // store queried dependencies in memory and check them before querying more from remote. -struct CachingDependencyProvider> { +struct CachingDependencyProvider< + P: Package, + I: Interval, + V: Version, + DP: DependencyProvider, +> { remote_dependencies: DP, - cached_dependencies: RefCell>, + cached_dependencies: RefCell>, } -impl> CachingDependencyProvider { +impl, V: Version, DP: DependencyProvider> + CachingDependencyProvider +{ pub fn new(remote_dependencies_provider: DP) -> Self { CachingDependencyProvider { remote_dependencies: remote_dependencies_provider, @@ -24,10 +31,10 @@ impl> CachingDependencyProv } } -impl> DependencyProvider - for CachingDependencyProvider +impl, V: Version, DP: DependencyProvider> + DependencyProvider for CachingDependencyProvider { - fn choose_package_version, U: std::borrow::Borrow>>( + fn choose_package_version, U: std::borrow::Borrow>>( &self, packages: impl Iterator, ) -> Result<(T, Option), Box> { @@ -39,7 +46,7 @@ impl> DependencyProvider Result, Box> { + ) -> Result, Box> { let mut cache = self.cached_dependencies.borrow_mut(); match cache.get_dependencies(package, version) { Ok(Dependencies::Unknown) => { @@ -65,7 +72,8 @@ impl> DependencyProvider::new(); + let mut remote_dependencies_provider = + OfflineDependencyProvider::<&str, NumberInterval, NumberVersion>::new(); // Add dependencies as needed. Here only root package is added. remote_dependencies_provider.add_dependencies("root", 1, Vec::new()); diff --git a/examples/doc_interface.rs b/examples/doc_interface.rs index b3e37838..3aeea43a 100644 --- a/examples/doc_interface.rs +++ b/examples/doc_interface.rs @@ -1,8 +1,8 @@ // SPDX-License-Identifier: MPL-2.0 -use pubgrub::range::Range; +use pubgrub::range_trait::Range; use pubgrub::solver::{resolve, OfflineDependencyProvider}; -use pubgrub::version::NumberVersion; +use pubgrub::version_trait::{NumberInterval, NumberVersion}; // `root` depends on `menu` and `icons` // `menu` depends on `dropdown` @@ -10,12 +10,12 @@ use pubgrub::version::NumberVersion; // `icons` has no dependency #[rustfmt::skip] fn main() { - let mut dependency_provider = OfflineDependencyProvider::<&str, NumberVersion>::new(); + let mut dependency_provider = OfflineDependencyProvider::<&str, NumberInterval, NumberVersion>::new(); dependency_provider.add_dependencies( - "root", 1, vec![("menu", Range::any()), ("icons", Range::any())], + "root", 1, vec![("menu", Range::full()), ("icons", Range::full())], ); - dependency_provider.add_dependencies("menu", 1, vec![("dropdown", Range::any())]); - dependency_provider.add_dependencies("dropdown", 1, vec![("icons", Range::any())]); + dependency_provider.add_dependencies("menu", 1, vec![("dropdown", Range::full())]); + dependency_provider.add_dependencies("dropdown", 1, vec![("icons", Range::full())]); dependency_provider.add_dependencies("icons", 1, vec![]); // Run the algorithm. diff --git a/examples/doc_interface_error.rs b/examples/doc_interface_error.rs index 0ef0f1ec..ebfa214e 100644 --- a/examples/doc_interface_error.rs +++ b/examples/doc_interface_error.rs @@ -1,10 +1,10 @@ // SPDX-License-Identifier: MPL-2.0 use pubgrub::error::PubGrubError; -use pubgrub::range::Range; +use pubgrub::range_trait::Range; use pubgrub::report::{DefaultStringReporter, Reporter}; use pubgrub::solver::{resolve, OfflineDependencyProvider}; -use pubgrub::version::SemanticVersion; +use pubgrub::version_trait::{SemanticInterval, SemanticVersion}; // `root` depends on `menu`, `icons 1.0.0` and `intl 5.0.0` // `menu 1.0.0` depends on `dropdown < 2.0.0` @@ -15,12 +15,12 @@ use pubgrub::version::SemanticVersion; // `intl` has no dependency #[rustfmt::skip] fn main() { - let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); + let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticInterval, SemanticVersion>::new(); // Direct dependencies: menu and icons. dependency_provider.add_dependencies("root", (1, 0, 0), vec![ - ("menu", Range::any()), - ("icons", Range::exact((1, 0, 0))), - ("intl", Range::exact((5, 0, 0))), + ("menu", Range::full()), + ("icons", Range::singleton((1, 0, 0))), + ("intl", Range::singleton((5, 0, 0))), ]); // Dependencies of the menu lib. @@ -45,19 +45,19 @@ fn main() { // Dependencies of the dropdown lib. dependency_provider.add_dependencies("dropdown", (1, 8, 0), vec![ - ("intl", Range::exact((3, 0, 0))), + ("intl", Range::singleton((3, 0, 0))), ]); dependency_provider.add_dependencies("dropdown", (2, 0, 0), vec![ - ("icons", Range::exact((2, 0, 0))), + ("icons", Range::singleton((2, 0, 0))), ]); dependency_provider.add_dependencies("dropdown", (2, 1, 0), vec![ - ("icons", Range::exact((2, 0, 0))), + ("icons", Range::singleton((2, 0, 0))), ]); dependency_provider.add_dependencies("dropdown", (2, 2, 0), vec![ - ("icons", Range::exact((2, 0, 0))), + ("icons", Range::singleton((2, 0, 0))), ]); dependency_provider.add_dependencies("dropdown", (2, 3, 0), vec![ - ("icons", Range::exact((2, 0, 0))), + ("icons", Range::singleton((2, 0, 0))), ]); // Icons have no dependencies. diff --git a/examples/doc_interface_semantic.rs b/examples/doc_interface_semantic.rs index b4c352e1..4825bb16 100644 --- a/examples/doc_interface_semantic.rs +++ b/examples/doc_interface_semantic.rs @@ -1,10 +1,10 @@ // SPDX-License-Identifier: MPL-2.0 use pubgrub::error::PubGrubError; -use pubgrub::range::Range; +use pubgrub::range_trait::Range; use pubgrub::report::{DefaultStringReporter, Reporter}; use pubgrub::solver::{resolve, OfflineDependencyProvider}; -use pubgrub::version::SemanticVersion; +use pubgrub::version_trait::{SemanticInterval, SemanticVersion}; // `root` depends on `menu` and `icons 1.0.0` // `menu 1.0.0` depends on `dropdown < 2.0.0` @@ -14,11 +14,11 @@ use pubgrub::version::SemanticVersion; // `icons` has no dependency #[rustfmt::skip] fn main() { - let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); + let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticInterval, SemanticVersion>::new(); // Direct dependencies: menu and icons. dependency_provider.add_dependencies("root", (1, 0, 0), vec![ - ("menu", Range::any()), - ("icons", Range::exact((1, 0, 0))), + ("menu", Range::full()), + ("icons", Range::singleton((1, 0, 0))), ]); // Dependencies of the menu lib. @@ -44,16 +44,16 @@ fn main() { // Dependencies of the dropdown lib. dependency_provider.add_dependencies("dropdown", (1, 8, 0), vec![]); dependency_provider.add_dependencies("dropdown", (2, 0, 0), vec![ - ("icons", Range::exact((2, 0, 0))), + ("icons", Range::singleton((2, 0, 0))), ]); dependency_provider.add_dependencies("dropdown", (2, 1, 0), vec![ - ("icons", Range::exact((2, 0, 0))), + ("icons", Range::singleton((2, 0, 0))), ]); dependency_provider.add_dependencies("dropdown", (2, 2, 0), vec![ - ("icons", Range::exact((2, 0, 0))), + ("icons", Range::singleton((2, 0, 0))), ]); dependency_provider.add_dependencies("dropdown", (2, 3, 0), vec![ - ("icons", Range::exact((2, 0, 0))), + ("icons", Range::singleton((2, 0, 0))), ]); // Icons has no dependency. diff --git a/examples/linear_error_reporting.rs b/examples/linear_error_reporting.rs index 0673fe36..2b2a3173 100644 --- a/examples/linear_error_reporting.rs +++ b/examples/linear_error_reporting.rs @@ -1,14 +1,15 @@ // SPDX-License-Identifier: MPL-2.0 use pubgrub::error::PubGrubError; -use pubgrub::range::Range; +use pubgrub::range_trait::Range; use pubgrub::report::{DefaultStringReporter, Reporter}; use pubgrub::solver::{resolve, OfflineDependencyProvider}; -use pubgrub::version::SemanticVersion; +use pubgrub::version_trait::{SemanticInterval, SemanticVersion}; // https://github.com/dart-lang/pub/blob/master/doc/solver.md#linear-error-reporting fn main() { - let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); + let mut dependency_provider = + OfflineDependencyProvider::<&str, SemanticInterval, SemanticVersion>::new(); #[rustfmt::skip] // root 1.0.0 depends on foo ^1.0.0 and baz ^1.0.0 dependency_provider.add_dependencies( diff --git a/src/lib.rs b/src/lib.rs index c98655ee..13df0e6c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -212,12 +212,12 @@ pub mod error; pub mod package; pub mod range; -mod range_trait; +pub mod range_trait; pub mod report; pub mod solver; pub mod term; pub mod type_aliases; pub mod version; -mod version_trait; +pub mod version_trait; mod internal; diff --git a/src/term.rs b/src/term.rs index 3d388ad7..7a01695f 100644 --- a/src/term.rs +++ b/src/term.rs @@ -182,13 +182,13 @@ impl, V: Version + fmt::Display> fmt::Display for Term { #[cfg(test)] pub mod tests { use super::*; - use crate::version::NumberVersion; + use crate::version_trait::{NumberInterval, NumberVersion}; use proptest::prelude::*; - pub fn strategy() -> impl Strategy> { + pub fn strategy() -> impl Strategy> { prop_oneof![ - crate::range::tests::strategy().prop_map(Term::Positive), - crate::range::tests::strategy().prop_map(Term::Negative), + crate::range_trait::tests::strategy().prop_map(Term::Positive), + crate::range_trait::tests::strategy().prop_map(Term::Negative), ] } diff --git a/tests/examples.rs b/tests/examples.rs index 29901caf..f19e8310 100644 --- a/tests/examples.rs +++ b/tests/examples.rs @@ -1,14 +1,15 @@ // SPDX-License-Identifier: MPL-2.0 -use pubgrub::range::Range; +use pubgrub::range_trait::Range; use pubgrub::solver::{resolve, OfflineDependencyProvider}; use pubgrub::type_aliases::Map; -use pubgrub::version::{NumberVersion, SemanticVersion}; +use pubgrub::version_trait::{NumberInterval, NumberVersion, SemanticInterval, SemanticVersion}; #[test] /// https://github.com/dart-lang/pub/blob/master/doc/solver.md#no-conflicts fn no_conflict() { - let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); + let mut dependency_provider = + OfflineDependencyProvider::<&str, SemanticInterval, SemanticVersion>::new(); #[rustfmt::skip] dependency_provider.add_dependencies( "root", (1, 0, 0), @@ -38,7 +39,8 @@ fn no_conflict() { #[test] /// https://github.com/dart-lang/pub/blob/master/doc/solver.md#avoiding-conflict-during-decision-making fn avoiding_conflict_during_decision_making() { - let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); + let mut dependency_provider = + OfflineDependencyProvider::<&str, SemanticInterval, SemanticVersion>::new(); #[rustfmt::skip] dependency_provider.add_dependencies( "root", (1, 0, 0), @@ -73,7 +75,8 @@ fn avoiding_conflict_during_decision_making() { #[test] /// https://github.com/dart-lang/pub/blob/master/doc/solver.md#performing-conflict-resolution fn conflict_resolution() { - let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); + let mut dependency_provider = + OfflineDependencyProvider::<&str, SemanticInterval, SemanticVersion>::new(); #[rustfmt::skip] dependency_provider.add_dependencies( "root", (1, 0, 0), @@ -106,7 +109,8 @@ fn conflict_resolution() { #[test] /// https://github.com/dart-lang/pub/blob/master/doc/solver.md#conflict-resolution-with-a-partial-satisfier fn conflict_with_partial_satisfier() { - let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); + let mut dependency_provider = + OfflineDependencyProvider::<&str, SemanticInterval, SemanticVersion>::new(); #[rustfmt::skip] // root 1.0.0 depends on foo ^1.0.0 and target ^2.0.0 dependency_provider.add_dependencies( @@ -171,12 +175,13 @@ fn conflict_with_partial_satisfier() { /// /// Solution: a0, b0, c0, d0 fn double_choices() { - let mut dependency_provider = OfflineDependencyProvider::<&str, NumberVersion>::new(); - dependency_provider.add_dependencies("a", 0, vec![("b", Range::any()), ("c", Range::any())]); - dependency_provider.add_dependencies("b", 0, vec![("d", Range::exact(0))]); - dependency_provider.add_dependencies("b", 1, vec![("d", Range::exact(1))]); + let mut dependency_provider = + OfflineDependencyProvider::<&str, NumberInterval, NumberVersion>::new(); + dependency_provider.add_dependencies("a", 0, vec![("b", Range::full()), ("c", Range::full())]); + dependency_provider.add_dependencies("b", 0, vec![("d", Range::singleton(0))]); + dependency_provider.add_dependencies("b", 1, vec![("d", Range::singleton(1))]); dependency_provider.add_dependencies("c", 0, vec![]); - dependency_provider.add_dependencies("c", 1, vec![("d", Range::exact(2))]); + dependency_provider.add_dependencies("c", 1, vec![("d", Range::singleton(2))]); dependency_provider.add_dependencies("d", 0, vec![]); // Solution. diff --git a/tests/proptest.rs b/tests/proptest.rs index d2da3730..92dd7582 100644 --- a/tests/proptest.rs +++ b/tests/proptest.rs @@ -4,13 +4,13 @@ use std::{collections::BTreeSet as Set, error::Error}; use pubgrub::error::PubGrubError; use pubgrub::package::Package; -use pubgrub::range::Range; +use pubgrub::range_trait::Range; use pubgrub::report::{DefaultStringReporter, Reporter}; use pubgrub::solver::{ choose_package_with_fewest_versions, resolve, Dependencies, DependencyProvider, OfflineDependencyProvider, }; -use pubgrub::version::{NumberVersion, Version}; +use pubgrub::version_trait::{Interval, NumberInterval, NumberVersion, Version}; use proptest::collection::{btree_map, vec}; use proptest::prelude::*; @@ -24,10 +24,14 @@ mod sat_dependency_provider; /// The same as [OfflineDependencyProvider] but takes versions from the opposite end: /// if [OfflineDependencyProvider] returns versions from newest to oldest, this returns them from oldest to newest. #[derive(Clone)] -struct OldestVersionsDependencyProvider(OfflineDependencyProvider); +struct OldestVersionsDependencyProvider, V: Version>( + OfflineDependencyProvider, +); -impl DependencyProvider for OldestVersionsDependencyProvider { - fn choose_package_version, U: std::borrow::Borrow>>( +impl, V: Version> DependencyProvider + for OldestVersionsDependencyProvider +{ + fn choose_package_version, U: std::borrow::Borrow>>( &self, potential_packages: impl Iterator, ) -> Result<(T, Option), Box> { @@ -37,7 +41,7 @@ impl DependencyProvider for OldestVersionsDependen )) } - fn get_dependencies(&self, p: &P, v: &V) -> Result, Box> { + fn get_dependencies(&self, p: &P, v: &V) -> Result, Box> { self.0.get_dependencies(p, v) } } @@ -62,17 +66,17 @@ impl TimeoutDependencyProvider { } } -impl> DependencyProvider - for TimeoutDependencyProvider +impl, V: Version, DP: DependencyProvider> + DependencyProvider for TimeoutDependencyProvider { - fn choose_package_version, U: std::borrow::Borrow>>( + fn choose_package_version, U: std::borrow::Borrow>>( &self, potential_packages: impl Iterator, ) -> Result<(T, Option), Box> { self.dp.choose_package_version(potential_packages) } - fn get_dependencies(&self, p: &P, v: &V) -> Result, Box> { + fn get_dependencies(&self, p: &P, v: &V) -> Result, Box> { self.dp.get_dependencies(p, v) } @@ -88,8 +92,9 @@ impl> DependencyProvider::new(); - dependency_provider.add_dependencies(0, 0, vec![(666, Range::any())]); + let mut dependency_provider = + OfflineDependencyProvider::<_, NumberInterval, NumberVersion>::new(); + dependency_provider.add_dependencies(0, 0, vec![(666, Range::full())]); // Run the algorithm. let _ = resolve( @@ -118,7 +123,7 @@ pub fn registry_strategy( bad_name: N, ) -> impl Strategy< Value = ( - OfflineDependencyProvider, + OfflineDependencyProvider, Vec<(N, NumberVersion)>, ), > { @@ -168,7 +173,7 @@ pub fn registry_strategy( move |(crate_vers_by_name, raw_dependencies, reverse_alphabetical, complicated_len)| { let mut list_of_pkgid: Vec<( (N, NumberVersion), - Option)>>, + Option)>>, )> = crate_vers_by_name .iter() .flat_map(|(name, vers)| { @@ -196,13 +201,13 @@ pub fn registry_strategy( deps.push(( dep_name, if c == 0 && d == s_last_index { - Range::any() + Range::full() } else if c == 0 { Range::strictly_lower_than(s[d].0 + 1) } else if d == s_last_index { Range::higher_than(s[c].0) } else if c == d { - Range::exact(s[c].0) + Range::singleton(s[c].0) } else { Range::between(s[c].0, s[d].0 + 1) }, @@ -210,7 +215,8 @@ pub fn registry_strategy( } } - let mut dependency_provider = OfflineDependencyProvider::::new(); + let mut dependency_provider = + OfflineDependencyProvider::::new(); let complicated_len = std::cmp::min(complicated_len, list_of_pkgid.len()); let complicated: Vec<_> = if reverse_alphabetical { @@ -226,7 +232,7 @@ pub fn registry_strategy( dependency_provider.add_dependencies( name, ver, - deps.unwrap_or_else(|| vec![(bad_name.clone(), Range::any())]), + deps.unwrap_or_else(|| vec![(bad_name.clone(), Range::full())]), ); } @@ -373,7 +379,7 @@ proptest! { .versions(package) .unwrap().collect(); let version = version_idx.get(&versions); - let dependencies: Vec<(u16, Range)> = match dependency_provider + let dependencies: Vec<(u16, Range)> = match dependency_provider .get_dependencies(package, version) .unwrap() { @@ -432,7 +438,7 @@ proptest! { Ok(used) => { // If resolution was successful, then unpublishing a version of a crate // that was not selected should not change that. - let mut smaller_dependency_provider = OfflineDependencyProvider::<_, NumberVersion>::new(); + let mut smaller_dependency_provider = OfflineDependencyProvider::<_,NumberInterval, NumberVersion>::new(); for &(n, v) in &all_versions { if used.get(&n) == Some(&v) // it was used || to_remove.get(&(n, v)).is_none() // or it is not one to be removed @@ -455,7 +461,7 @@ proptest! { Err(_) => { // If resolution was unsuccessful, then it should stay unsuccessful // even if any version of a crate is unpublished. - let mut smaller_dependency_provider = OfflineDependencyProvider::<_, NumberVersion>::new(); + let mut smaller_dependency_provider = OfflineDependencyProvider::<_, NumberInterval, NumberVersion>::new(); for &(n, v) in &all_versions { if to_remove.get(&(n, v)).is_none() // it is not one to be removed { diff --git a/tests/sat_dependency_provider.rs b/tests/sat_dependency_provider.rs index fa964ca8..6e2da979 100644 --- a/tests/sat_dependency_provider.rs +++ b/tests/sat_dependency_provider.rs @@ -1,9 +1,11 @@ // SPDX-License-Identifier: MPL-2.0 +use std::marker::PhantomData; + use pubgrub::package::Package; use pubgrub::solver::{Dependencies, DependencyProvider, OfflineDependencyProvider}; use pubgrub::type_aliases::{Map, SelectedDependencies}; -use pubgrub::version::Version; +use pubgrub::version_trait::{Interval, Version}; use varisat::ExtendFormula; const fn num_bits() -> usize { @@ -46,13 +48,14 @@ fn sat_at_most_one(solver: &mut impl varisat::ExtendFormula, vars: &[varisat::Va /// /// The SAT library does not optimize for the newer version, /// so the selected packages may not match the real resolver. -pub struct SatResolve { +pub struct SatResolve, V: Version> { solver: varisat::Solver<'static>, all_versions_by_p: Map>, + phantom: PhantomData, } -impl SatResolve { - pub fn new(dp: &OfflineDependencyProvider) -> Self { +impl, V: Version> SatResolve { + pub fn new(dp: &OfflineDependencyProvider) -> Self { let mut cnf = varisat::CnfFormula::new(); let mut all_versions = vec![]; @@ -107,6 +110,7 @@ impl SatResolve { Self { solver, all_versions_by_p, + phantom: PhantomData, } } diff --git a/tests/tests.rs b/tests/tests.rs index 7aeed03b..d7d498d9 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -1,20 +1,21 @@ // SPDX-License-Identifier: MPL-2.0 use pubgrub::error::PubGrubError; -use pubgrub::range::Range; +use pubgrub::range_trait::Range; use pubgrub::solver::{resolve, OfflineDependencyProvider}; -use pubgrub::version::NumberVersion; +use pubgrub::version_trait::{NumberInterval, NumberVersion}; #[test] fn same_result_on_repeated_runs() { - let mut dependency_provider = OfflineDependencyProvider::<_, NumberVersion>::new(); + let mut dependency_provider = + OfflineDependencyProvider::<_, NumberInterval, NumberVersion>::new(); dependency_provider.add_dependencies("c", 0, vec![]); dependency_provider.add_dependencies("c", 2, vec![]); dependency_provider.add_dependencies("b", 0, vec![]); dependency_provider.add_dependencies("b", 1, vec![("c", Range::between(0, 1))]); - dependency_provider.add_dependencies("a", 0, vec![("b", Range::any()), ("c", Range::any())]); + dependency_provider.add_dependencies("a", 0, vec![("b", Range::full()), ("c", Range::full())]); let name = "a"; let ver = NumberVersion(0); @@ -29,14 +30,15 @@ fn same_result_on_repeated_runs() { #[test] fn should_always_find_a_satisfier() { - let mut dependency_provider = OfflineDependencyProvider::<_, NumberVersion>::new(); - dependency_provider.add_dependencies("a", 0, vec![("b", Range::none())]); + let mut dependency_provider = + OfflineDependencyProvider::<_, NumberInterval, NumberVersion>::new(); + dependency_provider.add_dependencies("a", 0, vec![("b", Range::empty())]); assert!(matches!( resolve(&dependency_provider, "a", 0), Err(PubGrubError::DependencyOnTheEmptySet { .. }) )); - dependency_provider.add_dependencies("c", 0, vec![("a", Range::any())]); + dependency_provider.add_dependencies("c", 0, vec![("a", Range::full())]); assert!(matches!( resolve(&dependency_provider, "c", 0), Err(PubGrubError::DependencyOnTheEmptySet { .. }) @@ -45,8 +47,9 @@ fn should_always_find_a_satisfier() { #[test] fn cannot_depend_on_self() { - let mut dependency_provider = OfflineDependencyProvider::<_, NumberVersion>::new(); - dependency_provider.add_dependencies("a", 0, vec![("a", Range::any())]); + let mut dependency_provider = + OfflineDependencyProvider::<_, NumberInterval, NumberVersion>::new(); + dependency_provider.add_dependencies("a", 0, vec![("a", Range::full())]); assert!(matches!( resolve(&dependency_provider, "a", 0), Err(PubGrubError::SelfDependency { .. }) From 7bb44c47266a004156a9e05ad1ea1e971cc81689 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Fri, 6 Aug 2021 12:46:27 +0200 Subject: [PATCH 10/22] feat: add logging to help debugging --- Cargo.toml | 2 ++ src/internal/core.rs | 6 ++++++ src/internal/partial_solution.rs | 7 +++++++ src/solver.rs | 3 +++ tests/examples.rs | 16 ++++++++++++++++ 5 files changed, 34 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 1f6b32fb..b1994096 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -23,12 +23,14 @@ include = ["Cargo.toml", "LICENSE", "README.md", "src/**", "tests/**", "examples thiserror = "1.0" rustc-hash = "1.1.0" serde = { version = "1.0", features = ["derive"], optional = true } +log = { version = "0.4.14", default-features = false } # for debug logs in tests [dev-dependencies] proptest = "0.10.1" ron = "0.6" varisat = "0.2.2" criterion = "0.3" +env_logger = "0.9.0" [[bench]] name = "large_case" diff --git a/src/internal/core.rs b/src/internal/core.rs index f923850a..c59472d5 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -115,6 +115,10 @@ impl State { // If the partial solution satisfies the incompatibility // we must perform conflict resolution. Relation::Satisfied => { + log::info!( + "Start conflict resolution because incompat satisfied:\n {}", + current_incompat + ); conflict_id = Some(incompat_id); break; } @@ -183,6 +187,7 @@ impl State { current_incompat_changed, previous_satisfier_level, ); + log::info!("backtrack to {:?}", previous_satisfier_level); return Ok((package, current_incompat_id)); } SameDecisionLevels { satisfier_cause } => { @@ -192,6 +197,7 @@ impl State { &package, &self.incompatibility_store, ); + log::info!("prior cause: {}", prior_cause); current_incompat_id = self.incompatibility_store.alloc(prior_cause); current_incompat_changed = true; } diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 84bfc5f8..e1093c1b 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -258,7 +258,14 @@ 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); self.add_decision(package, version); + } else { + log::info!( + "not adding {} @ {} because of its dependencies", + package, + version + ); } } diff --git a/src/solver.rs b/src/solver.rs index 62015911..9fdb9b27 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -92,6 +92,7 @@ pub fn resolve( .should_cancel() .map_err(|err| PubGrubError::ErrorInShouldCancel(err))?; + log::info!("unit_propagation: {}", &next); state.unit_propagation(next)?; let potential_packages = state.partial_solution.potential_packages(); @@ -109,6 +110,7 @@ pub fn resolve( let decision = dependency_provider .choose_package_version(potential_packages.unwrap()) .map_err(PubGrubError::ErrorChoosingPackageVersion)?; + log::info!("DP chose: {} @ {:?}", decision.0, decision.1); next = decision.0.clone(); // Pick the next compatible version. @@ -195,6 +197,7 @@ pub fn resolve( } else { // `dep_incompats` are already in `incompatibilities` so we know there are not satisfied // terms and can add the decision directly. + log::info!("add_decision (not first time): {} @ {}", &next, v); state.partial_solution.add_decision(next.clone(), v); } } diff --git a/tests/examples.rs b/tests/examples.rs index 29901caf..1add530e 100644 --- a/tests/examples.rs +++ b/tests/examples.rs @@ -5,9 +5,21 @@ use pubgrub::solver::{resolve, OfflineDependencyProvider}; use pubgrub::type_aliases::Map; use pubgrub::version::{NumberVersion, SemanticVersion}; +use log::LevelFilter; +use std::io::Write; + +fn init_log() { + let _ = env_logger::builder() + .filter_level(LevelFilter::Trace) + .format(|buf, record| writeln!(buf, "{}", record.args())) + .is_test(true) + .try_init(); +} + #[test] /// https://github.com/dart-lang/pub/blob/master/doc/solver.md#no-conflicts fn no_conflict() { + init_log(); let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); #[rustfmt::skip] dependency_provider.add_dependencies( @@ -38,6 +50,7 @@ fn no_conflict() { #[test] /// https://github.com/dart-lang/pub/blob/master/doc/solver.md#avoiding-conflict-during-decision-making fn avoiding_conflict_during_decision_making() { + init_log(); let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); #[rustfmt::skip] dependency_provider.add_dependencies( @@ -73,6 +86,7 @@ fn avoiding_conflict_during_decision_making() { #[test] /// https://github.com/dart-lang/pub/blob/master/doc/solver.md#performing-conflict-resolution fn conflict_resolution() { + init_log(); let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); #[rustfmt::skip] dependency_provider.add_dependencies( @@ -106,6 +120,7 @@ fn conflict_resolution() { #[test] /// https://github.com/dart-lang/pub/blob/master/doc/solver.md#conflict-resolution-with-a-partial-satisfier fn conflict_with_partial_satisfier() { + init_log(); let mut dependency_provider = OfflineDependencyProvider::<&str, SemanticVersion>::new(); #[rustfmt::skip] // root 1.0.0 depends on foo ^1.0.0 and target ^2.0.0 @@ -171,6 +186,7 @@ fn conflict_with_partial_satisfier() { /// /// Solution: a0, b0, c0, d0 fn double_choices() { + init_log(); let mut dependency_provider = OfflineDependencyProvider::<&str, NumberVersion>::new(); dependency_provider.add_dependencies("a", 0, vec![("b", Range::any()), ("c", Range::any())]); dependency_provider.add_dependencies("b", 0, vec![("d", Range::exact(0))]); From 30daf07d624e17e04d547b5cffe089ed8b163adf Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Fri, 6 Aug 2021 15:21:11 +0200 Subject: [PATCH 11/22] debug: impl Display for partial solution --- src/internal/partial_solution.rs | 56 ++++++++++++++++++++++++++++++++ src/solver.rs | 4 +++ 2 files changed, 60 insertions(+) diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index e72dee3b..d60311ac 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -3,6 +3,9 @@ //! A Memory acts like a structured partial solution //! where terms are regrouped by package in a [Map](crate::type_aliases::Map). +use std::collections::BTreeSet; +use std::fmt::Display; + use crate::internal::arena::Arena; use crate::internal::incompatibility::{IncompId, Incompatibility, Relation}; use crate::internal::small_map::SmallMap; @@ -32,6 +35,24 @@ pub struct PartialSolution, V: Version> { package_assignments: Map>, } +impl, V: Version> Display for PartialSolution { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let assignments: BTreeSet = self + .package_assignments + .iter() + .map(|(_, pa)| pa.to_string()) + .collect(); + let assignments: Vec<_> = assignments.into_iter().collect(); + write!( + f, + "next_global_index: {}\ncurrent_decision_level: {:?}\npackage_assignements:\n{}", + self.next_global_index, + self.current_decision_level, + assignments.join("\n") + ) + } +} + /// Package assignments contain the potential decision and derivations /// that have already been made for a given package, /// as well as the intersection of terms by all of these. @@ -43,6 +64,24 @@ struct PackageAssignments, V: Version> { assignments_intersection: AssignmentsIntersection, } +impl, V: Version> Display for PackageAssignments { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let derivations: Vec<_> = self + .dated_derivations + .iter() + .map(|dd| dd.to_string()) + .collect(); + write!( + f, + "decision range: {:?}..{:?}\nderivations:\n {}\n,assignments_intersection: {}", + self.smallest_decision_level, + self.highest_decision_level, + derivations.join("\n "), + self.assignments_intersection + ) + } +} + #[derive(Clone, Debug)] pub struct DatedDerivation, V: Version> { global_index: u32, @@ -50,12 +89,29 @@ pub struct DatedDerivation, V: Version> { cause: IncompId, } +impl, V: Version> Display for DatedDerivation { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{:?}, cause: {:?}", self.decision_level, self.cause) + } +} + #[derive(Clone, Debug)] enum AssignmentsIntersection, V: Version> { Decision((u32, V, Term)), Derivations(Term), } +impl, V: Version> Display for AssignmentsIntersection { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Decision((lvl, version, _)) => { + write!(f, "Decision: level {}, v = {}", lvl, version) + } + Self::Derivations(term) => write!(f, "Derivations term: {}", term), + } + } +} + #[derive(Clone, Debug)] pub enum SatisfierSearch, V: Version> { DifferentDecisionLevels { diff --git a/src/solver.rs b/src/solver.rs index f5afe03b..13ca7fe8 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -95,6 +95,10 @@ pub fn resolve + Debug, V: Version>( log::info!("unit_propagation: {}", &next); state.unit_propagation(next)?; + log::debug!( + "Partial solution after unit propagation: {}", + state.partial_solution + ); let potential_packages = state.partial_solution.potential_packages(); if potential_packages.is_none() { From 34428ab2519d6d8914b9884b5cd40d73e90a3776 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Fri, 6 Aug 2021 17:10:43 +0200 Subject: [PATCH 12/22] Found a bug in intersection --- src/internal/partial_solution.rs | 5 ++++- src/range_trait.rs | 11 +++++++++++ src/solver.rs | 1 + 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index d60311ac..7c150692 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -40,7 +40,7 @@ impl, V: Version> Display for PartialSolution = self .package_assignments .iter() - .map(|(_, pa)| pa.to_string()) + .map(|(p, pa)| format!("{}: {}", p, pa)) .collect(); let assignments: Vec<_> = assignments.into_iter().collect(); write!( @@ -171,6 +171,7 @@ impl, V: Version> PartialSolution { ) { use std::collections::hash_map::Entry; let term = store[cause].get(&package).unwrap().negate(); + log::debug!("Add for {} the derivation term {}", package, term); let dated_derivation = DatedDerivation { global_index: self.next_global_index, decision_level: self.current_decision_level, @@ -181,6 +182,7 @@ impl, V: Version> PartialSolution { Entry::Occupied(mut occupied) => { let mut pa = occupied.get_mut(); pa.highest_decision_level = self.current_decision_level; + log::debug!("Terms intersection before: {}", pa.assignments_intersection); match &mut pa.assignments_intersection { // Check that add_derivation is never called in the wrong context. AssignmentsIntersection::Decision(_) => { @@ -190,6 +192,7 @@ impl, V: Version> PartialSolution { *t = t.intersection(&term); } } + log::debug!("Terms intersection after: {}", pa.assignments_intersection); pa.dated_derivations.push(dated_derivation); } Entry::Vacant(v) => { diff --git a/src/range_trait.rs b/src/range_trait.rs index 6f10497b..b36ed0bf 100644 --- a/src/range_trait.rs +++ b/src/range_trait.rs @@ -401,6 +401,7 @@ pub mod tests { use proptest::prelude::*; use super::*; + use crate::version_trait::{SemanticInterval, SemanticVersion}; // SidedBound tests. @@ -435,6 +436,16 @@ pub mod tests { // Ranges tests. + #[test] + fn negate_singleton() { + let s1: Range = Range::singleton((1, 0, 0)); + let s1c = s1.complement(); + let s12 = Range::between((1, 0, 0), (2, 0, 0)); + let s1c_12 = s1c.intersection(&s12); + let s12_1c = s12.intersection(&s1c); + assert_eq!(s1c_12, s12_1c); + } + pub fn strategy() -> impl Strategy> { prop::collection::vec(any::(), 0..10).prop_map(|mut vec| { vec.sort_unstable(); diff --git a/src/solver.rs b/src/solver.rs index 13ca7fe8..a0971fb6 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -106,6 +106,7 @@ pub fn resolve + Debug, V: Version>( // The borrow checker did not like using a match on potential_packages. // This `if ... is_none ... drop` is a workaround. // I believe this is a case where Polonius could help, when and if it lands in rustc. + log::debug!("Incompatibility store: {:?}", state.incompatibility_store); return state.partial_solution.extract_solution().ok_or_else(|| { PubGrubError::Failure( "How did we end up with no package to choose but no solution?".into(), From 0e0785596dcec0c666f1b339a59f386a1ab611b2 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Sat, 7 Aug 2021 01:12:23 +0200 Subject: [PATCH 13/22] Use bounds for NumberVersion --- src/range_trait.rs | 91 +++++++++++++++++++++++++++++++++++++------- src/version_trait.rs | 32 ++++------------ 2 files changed, 86 insertions(+), 37 deletions(-) diff --git a/src/range_trait.rs b/src/range_trait.rs index b36ed0bf..6ad3f9d4 100644 --- a/src/range_trait.rs +++ b/src/range_trait.rs @@ -45,7 +45,25 @@ fn ref_sided_bound(sb: &SidedBound) -> SidedBound<&V> { } } -impl SidedBound<&V> { +impl PartialOrd for SidedBound { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for SidedBound { + fn cmp(&self, other: &Self) -> Ordering { + self.compare(other) + } +} + +impl SidedBound { + fn unwrap(self) -> Bound { + match self { + Self::Left(bound) => bound, + Self::Right(bound) => bound, + } + } fn compare(&self, other: &Self) -> Ordering { match (&self, other) { // Handling of both left bounds. @@ -360,7 +378,7 @@ impl, V: Version> fmt::Display for Range { .iter() .map(interval_to_string) .collect(); - write!(f, "{}", string_intervals.join(", ")) + write!(f, "{}", string_intervals.join(" OR ")) } } } @@ -408,18 +426,25 @@ pub mod tests { use Bound::{Excluded, Included, Unbounded}; use SidedBound::{Left, Right}; - fn sided_bound_strategy() -> impl Strategy> { + fn sided_bound_strategy() -> impl Strategy> { prop_oneof![ bound_strategy().prop_map(Left), bound_strategy().prop_map(Right), ] } - fn bound_strategy() -> impl Strategy> { + fn bound_strategy() -> impl Strategy> { prop_oneof![ Just(Unbounded), - any::().prop_map(Excluded), - any::().prop_map(Included), + any::().prop_map(|n| Excluded(n.into())), + any::().prop_map(|n| Included(n.into())), + ] + } + + fn no_unbound_strategy() -> impl Strategy> { + prop_oneof![ + any::().prop_map(|n| Excluded(n.into())), + any::().prop_map(|n| Included(n.into())), ] } @@ -443,22 +468,50 @@ pub mod tests { let s12 = Range::between((1, 0, 0), (2, 0, 0)); let s1c_12 = s1c.intersection(&s12); let s12_1c = s12.intersection(&s1c); + eprintln!("first: {}", s1c); + eprintln!("first: {:?}", s1c); + eprintln!("second: {}", s12); + eprintln!("second: {:?}", s12); assert_eq!(s1c_12, s12_1c); } pub fn strategy() -> impl Strategy> { - prop::collection::vec(any::(), 0..10).prop_map(|mut vec| { - vec.sort_unstable(); + // TODO: find a better strategy so that the "intersection_is_symmetric" test + // triggers the same failure than the negate_singleton above. + prop::collection::vec(no_unbound_strategy(), 0..20).prop_map(|mut vec| { + let extract = |b: &Bound| match b { + Unbounded => panic!("cannot happen"), + Excluded(v) => v.0, + Included(v) => v.0, + }; vec.dedup(); + vec.sort_unstable_by_key(extract); + let mut segments: SmallVec = SmallVec::empty(); + let mut last_right: i64 = -1; let mut pair_iter = vec.chunks_exact(2); - let mut segments = SmallVec::empty(); - while let Some([v1, v2]) = pair_iter.next() { - segments.push((v1..v2).into()); + while let Some([b1, b2]) = pair_iter.next() { + let v1 = extract(b1); + let v2 = extract(b2); + if v1 as i64 == last_right { + segments.push(NumberInterval::new(Excluded(v1.into()), b2.clone())); + } else if v1 == v2 { + segments.push((v1..=v1).into()); + } else { + segments.push(NumberInterval::new(b1.clone(), b2.clone())); + } + last_right = v2 as i64; } - if let [v] = pair_iter.remainder() { - segments.push((v..).into()); + if let [b1] = pair_iter.remainder() { + let v1 = extract(b1); + if v1 as i64 == last_right { + segments.push(NumberInterval::new(Excluded(v1.into()), Unbounded)); + } else { + segments.push((v1..).into()); + } } + eprintln!("generated segments length: {}", segments.len()); Range { + // segments: merge_juxtaposed_segments(segments), segments, phantom: PhantomData, } @@ -554,4 +607,16 @@ pub mod tests { assert_eq!(ranges.contains(&version), ranges.intersection(&Range::singleton(version)) != Range::empty()); } } + + // #[test] + // fn contains_negation_0() { + // let full: Range = Range::full(); + // println!("full: {:?}", full); + // let full_comp = full.complement(); + // println!("full.complement(): {:?}", full_comp); + // let v0 = NumberVersion::from(0); + // assert_eq!(&full_comp, &Range::empty()); + // assert_ne!(full.contains(&v0), full_comp.contains(&v0)); + // panic!("force fail"); + // } } diff --git a/src/version_trait.rs b/src/version_trait.rs index aefb6f9a..d842166f 100644 --- a/src/version_trait.rs +++ b/src/version_trait.rs @@ -312,19 +312,13 @@ impl Version for NumberVersion { } } -impl NumberVersion { - fn bump(&self) -> Self { - NumberVersion(self.0 + 1) - } -} - // NumberInterval ############################################################## #[derive(Debug, Clone, Eq, PartialEq)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] pub struct NumberInterval { - start: NumberVersion, - end: Option, + start: Bound, + end: Bound, } // Ease the creation of NumberInterval from classic ranges: 0..10 @@ -339,29 +333,19 @@ impl> From for NumberInterval { impl RangeBounds for NumberInterval { fn start_bound(&self) -> Bound<&NumberVersion> { - Bound::Included(&self.start) + ref_bound(&self.start) } fn end_bound(&self) -> Bound<&NumberVersion> { - match &self.end { - None => Bound::Unbounded, - Some(v) => Bound::Excluded(v), - } + ref_bound(&self.end) } } impl Interval for NumberInterval { fn new(start_bound: Bound, end_bound: Bound) -> Self { - let start = match start_bound { - Bound::Unbounded => NumberVersion(0), - Bound::Excluded(v) => v.bump(), - Bound::Included(v) => v, - }; - let end = match end_bound { - Bound::Unbounded => None, - Bound::Excluded(v) => Some(v), - Bound::Included(v) => Some(v.bump()), - }; - Self { start, end } + Self { + start: start_bound, + end: end_bound, + } } } From a0396ca25f10d76784a0b0aaf6b6aa2a360933b4 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Sat, 7 Aug 2021 10:38:17 +0200 Subject: [PATCH 14/22] Failing attempt at fixing strategy --- src/range_trait.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/range_trait.rs b/src/range_trait.rs index 6ad3f9d4..f445f561 100644 --- a/src/range_trait.rs +++ b/src/range_trait.rs @@ -495,7 +495,11 @@ pub mod tests { if v1 as i64 == last_right { segments.push(NumberInterval::new(Excluded(v1.into()), b2.clone())); } else if v1 == v2 { - segments.push((v1..=v1).into()); + // segments.push((v1..=v1).into()); + segments.push(NumberInterval::new( + Included(v1.into()), + Included(v2.into()), + )); } else { segments.push(NumberInterval::new(b1.clone(), b2.clone())); } @@ -524,6 +528,13 @@ pub mod tests { proptest! { + // #[test] + // fn long_ranges(ranges in strategy()) { + // if ranges.segments.len() > 2 { + // assert_eq!(ranges, Range::empty()); + // } + // } + // Testing negate ---------------------------------- #[test] From 74a76590c8d44edf3a1d0e1017dfff98af09cd42 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Sat, 7 Aug 2021 10:49:53 +0200 Subject: [PATCH 15/22] Simplify failing symmetric test --- src/range_trait.rs | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/range_trait.rs b/src/range_trait.rs index f445f561..79aa6d0e 100644 --- a/src/range_trait.rs +++ b/src/range_trait.rs @@ -168,6 +168,14 @@ impl, V: Version> Range { } } + /// Set of all versions strictly lower than some version. + pub fn strictly_higher_than(v: impl Into) -> Self { + Self { + segments: SmallVec::one(I::new(Bound::Excluded(v.into()), V::maximum())), + phantom: PhantomData, + } + } + /// Set of all versions comprised between two given versions. /// The lower bound is included and the higher bound excluded. /// `v1 <= v < v2`. @@ -462,17 +470,12 @@ pub mod tests { // Ranges tests. #[test] - fn negate_singleton() { - let s1: Range = Range::singleton((1, 0, 0)); - let s1c = s1.complement(); - let s12 = Range::between((1, 0, 0), (2, 0, 0)); - let s1c_12 = s1c.intersection(&s12); - let s12_1c = s12.intersection(&s1c); - eprintln!("first: {}", s1c); - eprintln!("first: {:?}", s1c); - eprintln!("second: {}", s12); - eprintln!("second: {:?}", s12); - assert_eq!(s1c_12, s12_1c); + fn failing_symmetric() { + let s1: Range = Range::strictly_higher_than(0); + let s2 = Range::higher_than(0); + eprintln!("first: {}", s1); + eprintln!("second: {}", s2); + assert_eq!(s1.intersection(&s2), s2.intersection(&s1)); } pub fn strategy() -> impl Strategy> { From 49b868135deba6435ea63f0b3f3d282e726ce83f Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Sat, 7 Aug 2021 11:06:50 +0200 Subject: [PATCH 16/22] Rename bounds in intersection() code --- src/range_trait.rs | 56 +++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/src/range_trait.rs b/src/range_trait.rs index 79aa6d0e..bca7d82b 100644 --- a/src/range_trait.rs +++ b/src/range_trait.rs @@ -243,51 +243,51 @@ impl, V: Version> Range { /// Compute the intersection of two sets of versions. pub fn intersection(&self, other: &Self) -> Self { let mut segments = SmallVec::empty(); - let mut left_iter = self.segments.iter(); - let mut right_iter = other.segments.iter(); - let mut left = left_iter.next(); - let mut right = right_iter.next(); + let mut self_iter = self.segments.iter(); + let mut other_iter = other.segments.iter(); + let mut first = self_iter.next(); + let mut second = other_iter.next(); loop { - match (left, right) { - (Some(seg_left), Some(seg_right)) => { - let l1 = seg_left.start_bound(); - let l2 = seg_left.end_bound(); - let r1 = seg_right.start_bound(); - let r2 = seg_right.end_bound(); - match SidedBound::Right(l2).compare(&SidedBound::Left(r1)) { + match (first, second) { + (Some(first_seg), Some(second_seg)) => { + let fs = first_seg.start_bound(); + let fe = first_seg.end_bound(); + let ss = second_seg.start_bound(); + let se = second_seg.end_bound(); + match SidedBound::Right(fe).compare(&SidedBound::Left(ss)) { // Disjoint intervals with left < right. - Ordering::Less => left = left_iter.next(), - Ordering::Equal => left = left_iter.next(), + Ordering::Less => first = self_iter.next(), + Ordering::Equal => first = self_iter.next(), // Possible intersection with left >= right. Ordering::Greater => { - match SidedBound::Right(r2).compare(&SidedBound::Left(l1)) { + match SidedBound::Right(se).compare(&SidedBound::Left(fs)) { // Disjoint intervals with left < right. - Ordering::Less => right = right_iter.next(), - Ordering::Equal => right = right_iter.next(), + Ordering::Less => second = other_iter.next(), + Ordering::Equal => second = other_iter.next(), // Intersection for sure. Ordering::Greater => { - let start = match SidedBound::Left(l1) - .compare(&SidedBound::Right(r1)) + let start = match SidedBound::Left(fs) + .compare(&SidedBound::Right(ss)) { - Ordering::Less => r1, - _ => l1, + Ordering::Less => ss, + _ => fs, }; - match SidedBound::Right(l2).compare(&SidedBound::Right(r2)) { + match SidedBound::Right(fe).compare(&SidedBound::Right(se)) { Ordering::Less => { segments - .push(I::new(owned_bound(start), owned_bound(l2))); - left = left_iter.next(); + .push(I::new(owned_bound(start), owned_bound(fe))); + first = self_iter.next(); } Ordering::Equal => { segments - .push(I::new(owned_bound(start), owned_bound(l2))); - left = left_iter.next(); - right = right_iter.next(); + .push(I::new(owned_bound(start), owned_bound(fe))); + first = self_iter.next(); + second = other_iter.next(); } Ordering::Greater => { segments - .push(I::new(owned_bound(start), owned_bound(r2))); - right = right_iter.next(); + .push(I::new(owned_bound(start), owned_bound(se))); + second = other_iter.next(); } } } From d15452f8f73e21c66e31b3a9e4d600a0317cdeb9 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Sat, 7 Aug 2021 11:08:00 +0200 Subject: [PATCH 17/22] Fix bug in intersection --- src/range_trait.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/range_trait.rs b/src/range_trait.rs index bca7d82b..d894238d 100644 --- a/src/range_trait.rs +++ b/src/range_trait.rs @@ -266,12 +266,11 @@ impl, V: Version> Range { Ordering::Equal => second = other_iter.next(), // Intersection for sure. Ordering::Greater => { - let start = match SidedBound::Left(fs) - .compare(&SidedBound::Right(ss)) - { - Ordering::Less => ss, - _ => fs, - }; + let start = + match SidedBound::Left(fs).compare(&SidedBound::Left(ss)) { + Ordering::Less => ss, + _ => fs, + }; match SidedBound::Right(fe).compare(&SidedBound::Right(se)) { Ordering::Less => { segments From 7ffb8696f664438bf67564e1e08ff86ff189b115 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Sat, 7 Aug 2021 11:23:38 +0200 Subject: [PATCH 18/22] Fix doc tests --- src/lib.rs | 32 ++++++++++++++++---------------- src/solver.rs | 6 +++--- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 13df0e6c..573e1469 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -47,16 +47,16 @@ //! We can model that scenario with this library as follows //! ``` //! # use pubgrub::solver::{OfflineDependencyProvider, resolve}; -//! # use pubgrub::version::NumberVersion; -//! # use pubgrub::range::Range; +//! # use pubgrub::version_trait::{NumberInterval, NumberVersion}; +//! # use pubgrub::range_trait::Range; //! # -//! let mut dependency_provider = OfflineDependencyProvider::<&str, NumberVersion>::new(); +//! let mut dependency_provider = OfflineDependencyProvider::<&str, NumberInterval, NumberVersion>::new(); //! //! dependency_provider.add_dependencies( -//! "root", 1, vec![("menu", Range::any()), ("icons", Range::any())], +//! "root", 1, vec![("menu", Range::full()), ("icons", Range::full())], //! ); -//! dependency_provider.add_dependencies("menu", 1, vec![("dropdown", Range::any())]); -//! dependency_provider.add_dependencies("dropdown", 1, vec![("icons", Range::any())]); +//! dependency_provider.add_dependencies("menu", 1, vec![("dropdown", Range::full())]); +//! dependency_provider.add_dependencies("dropdown", 1, vec![("icons", Range::full())]); //! dependency_provider.add_dependencies("icons", 1, vec![]); //! //! // Run the algorithm. @@ -76,16 +76,16 @@ //! This may be done quite easily by implementing the two following functions. //! ``` //! # use pubgrub::solver::{DependencyProvider, Dependencies}; -//! # use pubgrub::version::SemanticVersion; -//! # use pubgrub::range::Range; +//! # use pubgrub::version_trait::{SemanticInterval, SemanticVersion}; +//! # use pubgrub::range_trait::Range; //! # use pubgrub::type_aliases::Map; //! # use std::error::Error; //! # use std::borrow::Borrow; //! # //! # struct MyDependencyProvider; //! # -//! impl DependencyProvider for MyDependencyProvider { -//! fn choose_package_version, U: Borrow>>(&self,packages: impl Iterator) -> Result<(T, Option), Box> { +//! impl DependencyProvider for MyDependencyProvider { +//! fn choose_package_version, U: Borrow>>(&self,packages: impl Iterator) -> Result<(T, Option), Box> { //! unimplemented!() //! } //! @@ -93,7 +93,7 @@ //! &self, //! package: &String, //! version: &SemanticVersion, -//! ) -> Result, Box> { +//! ) -> Result, Box> { //! unimplemented!() //! } //! } @@ -153,13 +153,13 @@ //! [Output](crate::report::Reporter::Output) type and a single method. //! ``` //! # use pubgrub::package::Package; -//! # use pubgrub::version::Version; +//! # use pubgrub::version_trait::{Version, Interval}; //! # use pubgrub::report::DerivationTree; //! # -//! pub trait Reporter { +//! pub trait Reporter, V: Version> { //! type Output; //! -//! fn report(derivation_tree: &DerivationTree) -> Self::Output; +//! fn report(derivation_tree: &DerivationTree) -> Self::Output; //! } //! ``` //! Implementing a [Reporter](crate::report::Reporter) may involve a lot of heuristics @@ -172,9 +172,9 @@ //! # use pubgrub::solver::{resolve, OfflineDependencyProvider}; //! # use pubgrub::report::{DefaultStringReporter, Reporter}; //! # use pubgrub::error::PubGrubError; -//! # use pubgrub::version::NumberVersion; +//! # use pubgrub::version_trait::{NumberInterval, NumberVersion}; //! # -//! # let dependency_provider = OfflineDependencyProvider::<&str, NumberVersion>::new(); +//! # let dependency_provider = OfflineDependencyProvider::<&str, NumberInterval, NumberVersion>::new(); //! # let root_package = "root"; //! # let root_version = 1; //! # diff --git a/src/solver.rs b/src/solver.rs index a0971fb6..af2a7fd8 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -42,11 +42,11 @@ //! //! ``` //! # use pubgrub::solver::{resolve, OfflineDependencyProvider}; -//! # use pubgrub::version::NumberVersion; +//! # use pubgrub::version_trait::{NumberInterval, NumberVersion}; //! # use pubgrub::error::PubGrubError; //! # -//! # fn try_main() -> Result<(), PubGrubError<&'static str, NumberVersion>> { -//! # let dependency_provider = OfflineDependencyProvider::<&str, NumberVersion>::new(); +//! # fn try_main() -> Result<(), PubGrubError<&'static str, NumberInterval, NumberVersion>> { +//! # let dependency_provider = OfflineDependencyProvider::<&str, NumberInterval, NumberVersion>::new(); //! # let package = "root"; //! # let version = 1; //! let solution = resolve(&dependency_provider, package, version)?; From a93959222bdaa414f14ba552d078f91b3eefecdf Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Sat, 7 Aug 2021 11:36:01 +0200 Subject: [PATCH 19/22] Temporary deactivate serde feature on tests --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bf7df723..49526756 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -40,7 +40,7 @@ jobs: run: cargo build --verbose - name: Run tests - run: cargo test --features=serde --verbose + run: cargo test --verbose clippy: name: No warnings from Clippy From 1573dd1b0aea38d91637c60061e82f6738cfdbb6 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Sat, 7 Aug 2021 11:49:01 +0200 Subject: [PATCH 20/22] Make clippy happy --- src/internal/core.rs | 1 + src/internal/incompatibility.rs | 4 +- src/internal/partial_solution.rs | 2 +- src/range_trait.rs | 101 +++++++++++++------------------ src/solver.rs | 60 +++++++++--------- src/term.rs | 2 +- src/version_trait.rs | 4 ++ 7 files changed, 80 insertions(+), 94 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index 01e3bd9a..57c708eb 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -161,6 +161,7 @@ impl + Debug, V: Version> State { /// Return the root cause and the backtracked model. /// CF + #[allow(clippy::type_complexity)] fn conflict_resolution( &mut self, incompatibility: IncompId, diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index a7d1e327..a1bce222 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -152,7 +152,7 @@ impl, V: Version> Incompatibility { 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) } } @@ -220,7 +220,7 @@ impl<'a, P: Package, I: Interval + 'a, V: Version + 'a> Incompatibility Option<&'a Term>) -> Relation

{ let mut relation = Relation::Satisfied; for (package, incompat_term) in self.package_terms.iter() { - match terms(package).map(|term| incompat_term.relation_with(&term)) { + match terms(package).map(|term| incompat_term.relation_with(term)) { Some(term::Relation::Satisfied) => {} Some(term::Relation::Contradicted) => { return Relation::Contradicted(package.clone()); diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 7c150692..f863011e 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -282,7 +282,7 @@ impl, V: Version> PartialSolution { pa.dated_derivations .iter() .fold(Term::any(), |acc, dated_derivation| { - let term = store[dated_derivation.cause].get(&p).unwrap().negate(); + let term = store[dated_derivation.cause].get(p).unwrap().negate(); acc.intersection(&term) }), ); diff --git a/src/range_trait.rs b/src/range_trait.rs index d894238d..d4fa9dbf 100644 --- a/src/range_trait.rs +++ b/src/range_trait.rs @@ -21,11 +21,12 @@ use std::ops::Bound; use crate::internal::small_vec::SmallVec; use crate::version_trait::{flip_bound, owned_bound, ref_bound}; -use crate::version_trait::{Interval, NumberInterval, NumberVersion, Version}; +use crate::version_trait::{Interval, Version}; #[derive(Debug, Clone, Eq, PartialEq)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] #[cfg_attr(feature = "serde", serde(transparent))] +/// New range with intervals. pub struct Range { segments: SmallVec, #[cfg_attr(feature = "serde", serde(skip_serializing))] @@ -38,12 +39,12 @@ enum SidedBound { Right(Bound), } -fn ref_sided_bound(sb: &SidedBound) -> SidedBound<&V> { - match sb { - SidedBound::Left(bound) => SidedBound::Left(ref_bound(bound)), - SidedBound::Right(bound) => SidedBound::Right(ref_bound(bound)), - } -} +// fn ref_sided_bound(sb: &SidedBound) -> SidedBound<&V> { +// match sb { +// SidedBound::Left(bound) => SidedBound::Left(ref_bound(bound)), +// SidedBound::Right(bound) => SidedBound::Right(ref_bound(bound)), +// } +// } impl PartialOrd for SidedBound { fn partial_cmp(&self, other: &Self) -> Option { @@ -58,12 +59,6 @@ impl Ord for SidedBound { } impl SidedBound { - fn unwrap(self) -> Bound { - match self { - Self::Left(bound) => bound, - Self::Right(bound) => bound, - } - } fn compare(&self, other: &Self) -> Ordering { match (&self, other) { // Handling of both left bounds. @@ -81,7 +76,7 @@ impl SidedBound { (SidedBound::Left(Bound::Included(l1)), SidedBound::Left(Bound::Included(l2))) => { l1.cmp(l2) } - (SidedBound::Left(_), SidedBound::Left(_)) => other.compare(&self).reverse(), + (SidedBound::Left(_), SidedBound::Left(_)) => other.compare(self).reverse(), // Handling of both right bounds. (SidedBound::Right(Bound::Unbounded), SidedBound::Right(Bound::Unbounded)) => { @@ -98,7 +93,7 @@ impl SidedBound { (SidedBound::Right(Bound::Included(r1)), SidedBound::Right(Bound::Included(r2))) => { r1.cmp(r2) } - (SidedBound::Right(_), SidedBound::Right(_)) => other.compare(&self).reverse(), + (SidedBound::Right(_), SidedBound::Right(_)) => other.compare(self).reverse(), // Handling of left and right bounds. (SidedBound::Left(Bound::Unbounded), SidedBound::Right(_)) => Ordering::Less, @@ -118,7 +113,7 @@ impl SidedBound { } // Handling of right and left bounds. - (SidedBound::Right(_), SidedBound::Left(_)) => other.compare(&self).reverse(), + (SidedBound::Right(_), SidedBound::Left(_)) => other.compare(self).reverse(), } } } @@ -247,57 +242,45 @@ impl, V: Version> Range { let mut other_iter = other.segments.iter(); let mut first = self_iter.next(); let mut second = other_iter.next(); - loop { - match (first, second) { - (Some(first_seg), Some(second_seg)) => { - let fs = first_seg.start_bound(); - let fe = first_seg.end_bound(); - let ss = second_seg.start_bound(); - let se = second_seg.end_bound(); - match SidedBound::Right(fe).compare(&SidedBound::Left(ss)) { + while let (Some(first_seg), Some(second_seg)) = (first, second) { + let fs = first_seg.start_bound(); + let fe = first_seg.end_bound(); + let ss = second_seg.start_bound(); + let se = second_seg.end_bound(); + match SidedBound::Right(fe).compare(&SidedBound::Left(ss)) { + // Disjoint intervals with left < right. + Ordering::Less => first = self_iter.next(), + Ordering::Equal => first = self_iter.next(), + // Possible intersection with left >= right. + Ordering::Greater => { + match SidedBound::Right(se).compare(&SidedBound::Left(fs)) { // Disjoint intervals with left < right. - Ordering::Less => first = self_iter.next(), - Ordering::Equal => first = self_iter.next(), - // Possible intersection with left >= right. + Ordering::Less => second = other_iter.next(), + Ordering::Equal => second = other_iter.next(), + // Intersection for sure. Ordering::Greater => { - match SidedBound::Right(se).compare(&SidedBound::Left(fs)) { - // Disjoint intervals with left < right. - Ordering::Less => second = other_iter.next(), - Ordering::Equal => second = other_iter.next(), - // Intersection for sure. + let start = match SidedBound::Left(fs).compare(&SidedBound::Left(ss)) { + Ordering::Less => ss, + _ => fs, + }; + match SidedBound::Right(fe).compare(&SidedBound::Right(se)) { + Ordering::Less => { + segments.push(I::new(owned_bound(start), owned_bound(fe))); + first = self_iter.next(); + } + Ordering::Equal => { + segments.push(I::new(owned_bound(start), owned_bound(fe))); + first = self_iter.next(); + second = other_iter.next(); + } Ordering::Greater => { - let start = - match SidedBound::Left(fs).compare(&SidedBound::Left(ss)) { - Ordering::Less => ss, - _ => fs, - }; - match SidedBound::Right(fe).compare(&SidedBound::Right(se)) { - Ordering::Less => { - segments - .push(I::new(owned_bound(start), owned_bound(fe))); - first = self_iter.next(); - } - Ordering::Equal => { - segments - .push(I::new(owned_bound(start), owned_bound(fe))); - first = self_iter.next(); - second = other_iter.next(); - } - Ordering::Greater => { - segments - .push(I::new(owned_bound(start), owned_bound(se))); - second = other_iter.next(); - } - } + segments.push(I::new(owned_bound(start), owned_bound(se))); + second = other_iter.next(); } } } } } - // Left or right has ended. - _ => { - break; - } } } diff --git a/src/solver.rs b/src/solver.rs index af2a7fd8..014f8702 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -145,39 +145,37 @@ pub fn resolve + Debug, V: Version>( { // Retrieve that package dependencies. let p = &next; - let dependencies = - match dependency_provider - .get_dependencies(&p, &v) - .map_err(|err| PubGrubError::ErrorRetrievingDependencies { - package: p.clone(), - version: v.clone(), - source: err, - })? { - Dependencies::Unknown => { - state.add_incompatibility(Incompatibility::unavailable_dependencies( - p.clone(), - v.clone(), - )); - continue; + let dependencies = match dependency_provider.get_dependencies(p, &v).map_err(|err| { + PubGrubError::ErrorRetrievingDependencies { + package: p.clone(), + version: v.clone(), + source: err, + } + })? { + Dependencies::Unknown => { + state.add_incompatibility(Incompatibility::unavailable_dependencies( + p.clone(), + v.clone(), + )); + continue; + } + Dependencies::Known(x) => { + if x.contains_key(p) { + return Err(PubGrubError::SelfDependency { + package: p.clone(), + version: v.clone(), + }); } - Dependencies::Known(x) => { - if x.contains_key(&p) { - return Err(PubGrubError::SelfDependency { - package: p.clone(), - version: v.clone(), - }); - } - if let Some((dependent, _)) = x.iter().find(|(_, r)| r == &&Range::empty()) - { - return Err(PubGrubError::DependencyOnTheEmptySet { - package: p.clone(), - version: v.clone(), - dependent: dependent.clone(), - }); - } - x + if let Some((dependent, _)) = x.iter().find(|(_, r)| r == &&Range::empty()) { + return Err(PubGrubError::DependencyOnTheEmptySet { + package: p.clone(), + version: v.clone(), + dependent: dependent.clone(), + }); } - }; + x + } + }; // Add that package and version if the dependencies are not problematic. let dep_incompats = diff --git a/src/term.rs b/src/term.rs index 7a01695f..82ad293e 100644 --- a/src/term.rs +++ b/src/term.rs @@ -162,7 +162,7 @@ impl<'a, I: Interval, V: 'a + Version> Term { impl, V: Version> AsRef> for Term { fn as_ref(&self) -> &Term { - &self + self } } diff --git a/src/version_trait.rs b/src/version_trait.rs index d842166f..a33a398c 100644 --- a/src/version_trait.rs +++ b/src/version_trait.rs @@ -236,6 +236,7 @@ impl Version for SemanticVersion { // SemanticInterval ############################################################ +/// Interval for semantic versions. #[derive(Debug, Clone, Eq, PartialEq)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] pub struct SemanticInterval { @@ -314,6 +315,7 @@ impl Version for NumberVersion { // NumberInterval ############################################################## +/// Interval for number versions. #[derive(Debug, Clone, Eq, PartialEq)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] pub struct NumberInterval { @@ -351,6 +353,7 @@ impl Interval for NumberInterval { // Helper ################################################################## +/// Map function inside a Bound. pub fn map_bound T>(f: F, bound: Bound) -> Bound { match bound { Bound::Unbounded => Bound::Unbounded, @@ -359,6 +362,7 @@ pub fn map_bound T>(f: F, bound: Bound) -> Bound { } } +/// Flip a bound by exchanging Excluded and Included. pub fn flip_bound(bound: Bound) -> Bound { match bound { Bound::Unbounded => Bound::Unbounded, From f7f223a5797eeb1cc70dc603c209823754051f19 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Sat, 7 Aug 2021 11:56:24 +0200 Subject: [PATCH 21/22] Fix code after clippy fixes --- src/range_trait.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/range_trait.rs b/src/range_trait.rs index d4fa9dbf..8b76158c 100644 --- a/src/range_trait.rs +++ b/src/range_trait.rs @@ -39,13 +39,6 @@ enum SidedBound { Right(Bound), } -// fn ref_sided_bound(sb: &SidedBound) -> SidedBound<&V> { -// match sb { -// SidedBound::Left(bound) => SidedBound::Left(ref_bound(bound)), -// SidedBound::Right(bound) => SidedBound::Right(ref_bound(bound)), -// } -// } - impl PartialOrd for SidedBound { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) @@ -409,13 +402,20 @@ pub mod tests { use proptest::prelude::*; use super::*; - use crate::version_trait::{SemanticInterval, SemanticVersion}; + use crate::version_trait::{NumberInterval, NumberVersion}; // SidedBound tests. use Bound::{Excluded, Included, Unbounded}; use SidedBound::{Left, Right}; + fn ref_sided_bound(sb: &SidedBound) -> SidedBound<&V> { + match sb { + SidedBound::Left(bound) => SidedBound::Left(ref_bound(bound)), + SidedBound::Right(bound) => SidedBound::Right(ref_bound(bound)), + } + } + fn sided_bound_strategy() -> impl Strategy> { prop_oneof![ bound_strategy().prop_map(Left), From 32fae2a02790595e4a0a9e81479109d7585ddee7 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Sat, 7 Aug 2021 12:01:49 +0200 Subject: [PATCH 22/22] Fix doc on range_trait --- src/range_trait.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/range_trait.rs b/src/range_trait.rs index 8b76158c..179e5a9c 100644 --- a/src/range_trait.rs +++ b/src/range_trait.rs @@ -7,9 +7,9 @@ //! of the ranges building blocks. //! //! Those building blocks are: -//! - [none()](Range::none): the empty set -//! - [any()](Range::any): the set of all possible versions -//! - [exact(v)](Range::exact): the set containing only the version v +//! - [empty()](Range::empty): the empty set +//! - [full()](Range::full): the set of all possible versions +//! - [singleton(v)](Range::singleton): the set containing only the version v //! - [higher_than(v)](Range::higher_than): the set defined by `v <= versions` //! - [strictly_lower_than(v)](Range::strictly_lower_than): the set defined by `versions < v` //! - [between(v1, v2)](Range::between): the set defined by `v1 <= versions < v2`