From 7e48ee734c0012c2d6219de820f66475d6428011 Mon Sep 17 00:00:00 2001 From: konstin Date: Sun, 26 Jan 2025 17:25:08 +0100 Subject: [PATCH 1/5] Improve `VersionSet` documentation --- src/solver.rs | 7 +++++++ src/version_set.rs | 39 ++++++++++++++++++++++++++------------- 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/src/solver.rs b/src/solver.rs index d12a2b75..0b915dfe 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -276,6 +276,13 @@ pub trait DependencyProvider { /// A common choice is [`Ranges`][version_ranges::Ranges]. type VS: VersionSet; + /// The type returned from `prioritize`. The resolver does not care what type this is + /// as long as it can pick a largest one and clone it. + /// + /// [`Reverse`](std::cmp::Reverse) can be useful if you want to pick the package with + /// the fewest versions that match the outstanding constraint. + type Priority: Ord + Clone; + /// Type for custom incompatibilities. /// /// There are reasons in user code outside pubgrub that can cause packages or versions diff --git a/src/version_set.rs b/src/version_set.rs index bf6af37c..9456ffae 100644 --- a/src/version_set.rs +++ b/src/version_set.rs @@ -22,38 +22,50 @@ use std::fmt::{Debug, Display}; use crate::Ranges; -/// Trait describing sets of versions. +/// A set of versions. +/// +/// See [`Ranges`] for an implementation. +/// +/// Two version sets that contain the same versions must be equal. +/// +/// The methods with default implementations can be overwritten for better performance, but they +/// must be equal to the default implementation. pub trait VersionSet: Debug + Display + Clone + Eq { /// Version type associated with the sets manipulated. type V: Debug + Display + Clone + Ord; // Constructors - /// Constructor for an empty set containing no version. + + /// An empty set containing no version. fn empty() -> Self; - /// Constructor for a set containing exactly one version. + + /// A set containing only the given version. fn singleton(v: Self::V) -> Self; // Operations - /// Compute the complement of this set. + + /// The set of all version that are not in this set. fn complement(&self) -> Self; - /// Compute the intersection with another set. + + /// The set of all versions that are in both sets. fn intersection(&self, other: &Self) -> Self; - // Membership - /// Evaluate membership of a version in this set. + /// Whether the version is part of this set. fn contains(&self, v: &Self::V) -> bool; - // Automatically implemented functions ########################### + // Automatically implemented functions - /// Constructor for the set containing all versions. - /// Automatically implemented as `Self::empty().complement()`. + /// The set containing all versions. + /// + /// The default implementation is the complement of the empty set. fn full() -> Self { Self::empty().complement() } - /// Compute the union with another set. - /// Thanks to set properties, this is automatically implemented as: - /// `self.complement().intersection(&other.complement()).complement()` + /// The set of all versions that are either (or both) of the sets. + /// + /// The default implementation is complement of the intersection of the complements of both sets + /// (De Morgan's law). fn union(&self, other: &Self) -> Self { self.complement() .intersection(&other.complement()) @@ -71,6 +83,7 @@ pub trait VersionSet: Debug + Display + Clone + Eq { } } +/// [`Ranges`] contains optimized implementations of all operations. impl VersionSet for Ranges { type V = T; From 7be8fafb8f3bcfa9f14ac56a4374b0949c972d61 Mon Sep 17 00:00:00 2001 From: konstin Date: Sun, 26 Jan 2025 19:24:51 +0100 Subject: [PATCH 2/5] Improve report documentation --- src/report.rs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/report.rs b/src/report.rs index af232260..792865c8 100644 --- a/src/report.rs +++ b/src/report.rs @@ -26,8 +26,7 @@ pub trait Reporter ) -> Self::Output; } -/// Derivation tree resulting in the impossibility -/// to solve the dependencies of our root package. +/// Derivation tree resulting in the impossibility to solve the dependencies of our root package. #[derive(Debug, Clone)] pub enum DerivationTree { /// External incompatibility. @@ -36,8 +35,7 @@ pub enum DerivationTree), } -/// Incompatibilities that are not derived from others, -/// they have their own reason. +/// Incompatibility that is not derived from other incompatibilities. #[derive(Debug, Clone)] pub enum External { /// Initial incompatibility aiming at picking the root package for the first decision. @@ -55,11 +53,10 @@ pub enum External { pub struct Derived { /// Terms of the incompatibility. 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. - /// Then, we may want to only explain it once, - /// and refer to the explanation for the other times. + /// Indicate if the incompatibility is present multiple times in the derivation tree. + /// + /// If that is the case, the number is a unique id. We may want to only explain this + /// incompatibility once, then refer to the explanation for the other times. pub shared_id: Option, /// First cause. pub cause1: Arc>, From 4d4713ff676d5ae2c1dc59a9af16f2ad2e560869 Mon Sep 17 00:00:00 2001 From: konstin Date: Wed, 29 Jan 2025 10:07:41 +0100 Subject: [PATCH 3/5] Review --- src/report.rs | 2 +- src/version_set.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/report.rs b/src/report.rs index 792865c8..79f29c58 100644 --- a/src/report.rs +++ b/src/report.rs @@ -55,7 +55,7 @@ pub struct Derived pub terms: Map>, /// Indicate if the incompatibility is present multiple times in the derivation tree. /// - /// If that is the case, the number is a unique id. We may want to only explain this + /// If that is the case, the number is a unique id. This can be used to only explain this /// incompatibility once, then refer to the explanation for the other times. pub shared_id: Option, /// First cause. diff --git a/src/version_set.rs b/src/version_set.rs index 9456ffae..cffa037f 100644 --- a/src/version_set.rs +++ b/src/version_set.rs @@ -28,8 +28,8 @@ use crate::Ranges; /// /// Two version sets that contain the same versions must be equal. /// -/// The methods with default implementations can be overwritten for better performance, but they -/// must be equal to the default implementation. +/// The methods with default implementations can be overwritten for better performance, but their +/// output must be equal to the default implementation. pub trait VersionSet: Debug + Display + Clone + Eq { /// Version type associated with the sets manipulated. type V: Debug + Display + Clone + Ord; From 3ff1c7b8d0b2e83de7a785d98afcd50a39d68211 Mon Sep 17 00:00:00 2001 From: konstin Date: Wed, 29 Jan 2025 18:11:28 +0100 Subject: [PATCH 4/5] Improve version set contract --- src/version_set.rs | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/src/version_set.rs b/src/version_set.rs index cffa037f..fdae29ca 100644 --- a/src/version_set.rs +++ b/src/version_set.rs @@ -1,23 +1,5 @@ // SPDX-License-Identifier: MPL-2.0 -//! As its name suggests, the [VersionSet] trait describes sets of versions. -//! -//! One needs to define -//! - the associate type for versions, -//! - two constructors for the empty set and a singleton set, -//! - the complement and intersection set operations, -//! - and a function to evaluate membership of versions. -//! -//! Two functions are automatically derived, thanks to the mathematical properties of sets. -//! You can overwrite those implementations, but we highly recommend that you don't, -//! except if you are confident in a correct implementation that brings much performance gains. -//! -//! It is also extremely important that the `Eq` trait is correctly implemented. -//! In particular, you can only use `#[derive(Eq, PartialEq)]` if `Eq` is strictly equivalent to the -//! structural equality, i.e. if version sets have canonical representations. -//! Such problems may arise if your implementations of `complement()` and `intersection()` do not -//! return canonical representations so be careful there. - use std::fmt::{Debug, Display}; use crate::Ranges; @@ -26,10 +8,24 @@ use crate::Ranges; /// /// See [`Ranges`] for an implementation. /// -/// Two version sets that contain the same versions must be equal. -/// /// The methods with default implementations can be overwritten for better performance, but their /// output must be equal to the default implementation. +/// +/// # Equality +/// +/// It is important that the `Eq` trait is implemented so that if two sets contain the same +/// versions, they are equal under `Eq`. In particular, you can only use `#[derive(Eq, PartialEq)]` +/// if `Eq` is strictly equivalent to the structural equality, i.e. if version sets are always +/// stored in a canonical representations. Such problems may arise if your implementations of +/// `complement()` and `intersection()` do not return canonical representations. +/// +/// For example, `>=1,<4 || >=2,<5` and `>=1,<4 || >=3,<5` are equal, because they can both be +/// normalized to `>=1,<5`. +/// +/// Note that pubgrub does not know which versions actually exist for a package, the contract +/// is about upholding the mathematical properties of set operations, assuming all versions are +/// possible. This is required for the solver to determine the relationship of version sets to each +/// other. pub trait VersionSet: Debug + Display + Clone + Eq { /// Version type associated with the sets manipulated. type V: Debug + Display + Clone + Ord; From e19f767d3313cf3efe442ab965c1e7e527a67a51 Mon Sep 17 00:00:00 2001 From: konstin Date: Wed, 29 Jan 2025 18:13:17 +0100 Subject: [PATCH 5/5] Rebase fix --- src/solver.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/solver.rs b/src/solver.rs index 0b915dfe..91feaf78 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -295,13 +295,6 @@ pub trait DependencyProvider { /// assign [`String`] as placeholder. type M: Eq + Clone + Debug + Display; - /// The type returned from `prioritize`. The resolver does not care what type this is - /// as long as it can pick a largest one and clone it. - /// - /// [`Reverse`](std::cmp::Reverse) can be useful if you want to pick the package with - /// the fewest versions that match the outstanding constraint. - type Priority: Ord + Clone; - /// The kind of error returned from these methods. /// /// Returning this signals that resolution should fail with this error.