Skip to content

Commit

Permalink
Start using the version ranges crate
Browse files Browse the repository at this point in the history
See pubgrub-rs/pubgrub#262 for prior discussion.

In most parts of uv, we don't want to depend on pubgrub, but we want to use its powerful version ranges arithmetic. pubgrub-rs/pubgrub#262 split out the ranges into a separate, small crate (that can be published separately to crates.io).

With this first in a series of changes, we only depend on pubgrub in the uv-resolver crate.

Note that the name is now `Ranges`, since the type formerly known as `Range` was a misnormer, it contains multiple ranges.
  • Loading branch information
konstin committed Oct 29, 2024
1 parent a56e521 commit f7435c6
Show file tree
Hide file tree
Showing 8 changed files with 92 additions and 82 deletions.
15 changes: 12 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,8 @@ pathdiff = { version = "0.2.1" }
petgraph = { version = "0.6.5" }
platform-info = { version = "2.0.3" }
proc-macro2 = { version = "1.0.86" }
pubgrub = { git = "https://github.com/astral-sh/pubgrub", rev = "7243f4faf8e54837aa8a401a18406e7173de4ad5" }
pubgrub = { git = "https://github.com/astral-sh/pubgrub", rev = "95e1390399cdddee986b658be19587eb1fdb2d79" }
version-ranges = { git = "https://github.com/astral-sh/pubgrub", rev = "95e1390399cdddee986b658be19587eb1fdb2d79" }
quote = { version = "1.0.37" }
rayon = { version = "1.10.0" }
reflink-copy = { version = "0.1.19" }
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-pep508/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ uv-pubgrub = { workspace = true }
boxcar = { workspace = true }
indexmap = { workspace = true }
itertools = { workspace = true }
pubgrub = { workspace = true }
regex = { workspace = true }
rustc-hash = { workspace = true }
schemars = { workspace = true, optional = true }
Expand All @@ -40,6 +39,7 @@ thiserror = { workspace = true }
tracing = { workspace = true, optional = true }
unicode-width = { workspace = true }
url = { workspace = true, features = ["serde"] }
version-ranges = { workspace = true }

[dev-dependencies]
insta = { version = "1.40.0" }
Expand Down
66 changes: 33 additions & 33 deletions crates/uv-pep508/src/marker/algebra.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,12 @@ use std::sync::Mutex;
use std::sync::MutexGuard;

use itertools::Either;
use pubgrub::Range;
use rustc_hash::FxHashMap;
use std::sync::LazyLock;
use uv_pep440::Operator;
use uv_pep440::{Version, VersionSpecifier};
use uv_pubgrub::PubGrubSpecifier;
use version_ranges::Ranges;

use crate::marker::MarkerValueExtra;
use crate::ExtraOperator;
Expand Down Expand Up @@ -403,7 +403,7 @@ impl InternerGuard<'_> {
});
return self.create_node(node.var.clone(), children);
};
let py_range = Range::from_range_bounds((py_lower.cloned(), py_upper.cloned()));
let py_range = Ranges::from_range_bounds((py_lower.cloned(), py_upper.cloned()));
if py_range.is_empty() {
// Oops, the bounds imply there is nothing that can match,
// so we always evaluate to false.
Expand All @@ -428,12 +428,12 @@ impl InternerGuard<'_> {
// are known to be satisfied.
let &(ref first_range, first_node_id) = new.first().unwrap();
let first_upper = first_range.bounding_range().unwrap().1;
let clipped = Range::from_range_bounds((Bound::Unbounded, first_upper.cloned()));
let clipped = Ranges::from_range_bounds((Bound::Unbounded, first_upper.cloned()));
*new.first_mut().unwrap() = (clipped, first_node_id);

let &(ref last_range, last_node_id) = new.last().unwrap();
let last_lower = last_range.bounding_range().unwrap().0;
let clipped = Range::from_range_bounds((last_lower.cloned(), Bound::Unbounded));
let clipped = Ranges::from_range_bounds((last_lower.cloned(), Bound::Unbounded));
*new.last_mut().unwrap() = (clipped, last_node_id);

self.create_node(node.var.clone(), Edges::Version { edges: new })
Expand All @@ -459,7 +459,7 @@ impl InternerGuard<'_> {
return i;
}

let py_range = Range::from_range_bounds((py_lower.cloned(), py_upper.cloned()));
let py_range = Ranges::from_range_bounds((py_lower.cloned(), py_upper.cloned()));
if py_range.is_empty() {
// Oops, the bounds imply there is nothing that can match,
// so we always evaluate to false.
Expand Down Expand Up @@ -521,14 +521,14 @@ impl InternerGuard<'_> {
// adjacent ranges map to the same node, which would not be
// a canonical representation.
if exclude_node_id == first_node_id {
let clipped = Range::from_range_bounds((Bound::Unbounded, first_upper.cloned()));
let clipped = Ranges::from_range_bounds((Bound::Unbounded, first_upper.cloned()));
*new.first_mut().unwrap() = (clipped, first_node_id);
} else {
let clipped = Range::from_range_bounds((py_lower.cloned(), first_upper.cloned()));
let clipped = Ranges::from_range_bounds((py_lower.cloned(), first_upper.cloned()));
*new.first_mut().unwrap() = (clipped, first_node_id);

let py_range_lower =
Range::from_range_bounds((py_lower.cloned(), Bound::Unbounded));
Ranges::from_range_bounds((py_lower.cloned(), Bound::Unbounded));
new.insert(0, (py_range_lower.complement(), NodeId::FALSE.negate(i)));
}
}
Expand All @@ -539,14 +539,14 @@ impl InternerGuard<'_> {
// same reasoning applies here: to maintain a canonical
// representation.
if exclude_node_id == last_node_id {
let clipped = Range::from_range_bounds((last_lower.cloned(), Bound::Unbounded));
let clipped = Ranges::from_range_bounds((last_lower.cloned(), Bound::Unbounded));
*new.last_mut().unwrap() = (clipped, last_node_id);
} else {
let clipped = Range::from_range_bounds((last_lower.cloned(), py_upper.cloned()));
let clipped = Ranges::from_range_bounds((last_lower.cloned(), py_upper.cloned()));
*new.last_mut().unwrap() = (clipped, last_node_id);

let py_range_upper =
Range::from_range_bounds((Bound::Unbounded, py_upper.cloned()));
Ranges::from_range_bounds((Bound::Unbounded, py_upper.cloned()));
new.push((py_range_upper.complement(), exclude_node_id));
}
}
Expand Down Expand Up @@ -688,15 +688,15 @@ pub(crate) enum Edges {
// Invariant: All ranges are simple, meaning they can be represented by a bounded
// interval without gaps. Additionally, there are at least two edges in the set.
Version {
edges: SmallVec<(Range<Version>, NodeId)>,
edges: SmallVec<(Ranges<Version>, NodeId)>,
},
// The edges of a string variable, representing a disjoint set of ranges that cover
// the output space.
//
// Invariant: All ranges are simple, meaning they can be represented by a bounded
// interval without gaps. Additionally, there are at least two edges in the set.
String {
edges: SmallVec<(Range<String>, NodeId)>,
edges: SmallVec<(Ranges<String>, NodeId)>,
},
// The edges of a boolean variable, representing the values `true` (the `high` child)
// and `false` (the `low` child).
Expand Down Expand Up @@ -727,13 +727,13 @@ impl Edges {
/// This function will panic for the `In` and `Contains` marker operators, which
/// should be represented as separate boolean variables.
fn from_string(operator: MarkerOperator, value: String) -> Edges {
let range: Range<String> = match operator {
MarkerOperator::Equal => Range::singleton(value),
MarkerOperator::NotEqual => Range::singleton(value).complement(),
MarkerOperator::GreaterThan => Range::strictly_higher_than(value),
MarkerOperator::GreaterEqual => Range::higher_than(value),
MarkerOperator::LessThan => Range::strictly_lower_than(value),
MarkerOperator::LessEqual => Range::lower_than(value),
let range: Ranges<String> = match operator {
MarkerOperator::Equal => Ranges::singleton(value),
MarkerOperator::NotEqual => Ranges::singleton(value).complement(),
MarkerOperator::GreaterThan => Ranges::strictly_higher_than(value),
MarkerOperator::GreaterEqual => Ranges::higher_than(value),
MarkerOperator::LessThan => Ranges::strictly_lower_than(value),
MarkerOperator::LessEqual => Ranges::lower_than(value),
MarkerOperator::TildeEqual => unreachable!("string comparisons with ~= are ignored"),
_ => unreachable!("`in` and `contains` are treated as boolean variables"),
};
Expand All @@ -756,7 +756,7 @@ impl Edges {
///
/// Only for use when the `key` is a `PythonVersion`. Normalizes to `PythonFullVersion`.
fn from_python_versions(versions: Vec<Version>, negated: bool) -> Result<Edges, NodeId> {
let mut range = Range::empty();
let mut range = Ranges::empty();

// TODO(zanieb): We need to make sure this is performant, repeated unions like this do not
// seem efficient.
Expand All @@ -779,12 +779,12 @@ impl Edges {

/// Returns an [`Edges`] where values in the given range are `true`.
fn from_versions(versions: &Vec<Version>, negated: bool) -> Edges {
let mut range = Range::empty();
let mut range = Ranges::empty();

// TODO(zanieb): We need to make sure this is performant, repeated unions like this do not
// seem efficient.
for version in versions {
range = range.union(&Range::singleton(version.clone()));
range = range.union(&Ranges::singleton(version.clone()));
}

if negated {
Expand All @@ -797,21 +797,21 @@ impl Edges {
}

/// Returns an [`Edges`] where values in the given range are `true`.
fn from_range<T>(range: &Range<T>) -> SmallVec<(Range<T>, NodeId)>
fn from_range<T>(range: &Ranges<T>) -> SmallVec<(Ranges<T>, NodeId)>
where
T: Ord + Clone,
{
let mut edges = SmallVec::new();

// Add the `true` edges.
for (start, end) in range.iter() {
let range = Range::from_range_bounds((start.clone(), end.clone()));
let range = Ranges::from_range_bounds((start.clone(), end.clone()));
edges.push((range, NodeId::TRUE));
}

// Add the `false` edges.
for (start, end) in range.complement().iter() {
let range = Range::from_range_bounds((start.clone(), end.clone()));
let range = Ranges::from_range_bounds((start.clone(), end.clone()));
edges.push((range, NodeId::FALSE));
}

Expand Down Expand Up @@ -891,12 +891,12 @@ impl Edges {
/// In that case, we drop any ranges that do not exist in the domain of both edges. Note that
/// this should not occur in practice because `requires-python` bounds are global.
fn apply_ranges<T>(
left_edges: &SmallVec<(Range<T>, NodeId)>,
left_edges: &SmallVec<(Ranges<T>, NodeId)>,
left_parent: NodeId,
right_edges: &SmallVec<(Range<T>, NodeId)>,
right_edges: &SmallVec<(Ranges<T>, NodeId)>,
right_parent: NodeId,
mut apply: impl FnMut(NodeId, NodeId) -> NodeId,
) -> SmallVec<(Range<T>, NodeId)>
) -> SmallVec<(Ranges<T>, NodeId)>
where
T: Clone + Ord,
{
Expand Down Expand Up @@ -968,9 +968,9 @@ impl Edges {

// Returns `true` if all intersecting ranges in two range maps are disjoint.
fn is_disjoint_ranges<T>(
left_edges: &SmallVec<(Range<T>, NodeId)>,
left_edges: &SmallVec<(Ranges<T>, NodeId)>,
left_parent: NodeId,
right_edges: &SmallVec<(Range<T>, NodeId)>,
right_edges: &SmallVec<(Ranges<T>, NodeId)>,
right_parent: NodeId,
interner: &mut InternerGuard<'_>,
) -> bool
Expand Down Expand Up @@ -1179,7 +1179,7 @@ fn python_version_to_full_version(specifier: VersionSpecifier) -> Result<Version
}

/// Compares the start of two ranges that are known to be disjoint.
fn compare_disjoint_range_start<T>(range1: &Range<T>, range2: &Range<T>) -> Ordering
fn compare_disjoint_range_start<T>(range1: &Ranges<T>, range2: &Ranges<T>) -> Ordering
where
T: Ord,
{
Expand All @@ -1199,7 +1199,7 @@ where
}

/// Returns `true` if two disjoint ranges can be conjoined seamlessly without introducing a gap.
fn can_conjoin<T>(range1: &Range<T>, range2: &Range<T>) -> bool
fn can_conjoin<T>(range1: &Ranges<T>, range2: &Ranges<T>) -> bool
where
T: Ord + Clone,
{
Expand Down
14 changes: 7 additions & 7 deletions crates/uv-pep508/src/marker/simplify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ use std::ops::Bound;

use indexmap::IndexMap;
use itertools::Itertools;
use pubgrub::Range;
use rustc_hash::FxBuildHasher;
use uv_pep440::{Version, VersionSpecifier};
use version_ranges::Ranges;

use crate::{ExtraOperator, MarkerExpression, MarkerOperator, MarkerTree, MarkerTreeKind};

Expand Down Expand Up @@ -280,17 +280,17 @@ fn simplify(dnf: &mut Vec<Vec<MarkerExpression>>) {

/// Merge any edges that lead to identical subtrees into a single range.
pub(crate) fn collect_edges<'a, T>(
map: impl ExactSizeIterator<Item = (&'a Range<T>, MarkerTree)>,
) -> IndexMap<MarkerTree, Range<T>, FxBuildHasher>
map: impl ExactSizeIterator<Item = (&'a Ranges<T>, MarkerTree)>,
) -> IndexMap<MarkerTree, Ranges<T>, FxBuildHasher>
where
T: Ord + Clone + 'a,
{
let mut paths: IndexMap<_, Range<_>, FxBuildHasher> = IndexMap::default();
let mut paths: IndexMap<_, Ranges<_>, FxBuildHasher> = IndexMap::default();
for (range, tree) in map {
// OK because all ranges are guaranteed to be non-empty.
let (start, end) = range.bounding_range().unwrap();
// Combine the ranges.
let range = Range::from_range_bounds((start.cloned(), end.cloned()));
let range = Ranges::from_range_bounds((start.cloned(), end.cloned()));
paths
.entry(tree)
.and_modify(|union| *union = union.union(&range))
Expand All @@ -305,7 +305,7 @@ where
///
/// For example, `os_name < 'Linux' or os_name > 'Linux'` can be simplified to
/// `os_name != 'Linux'`.
fn range_inequality<T>(range: &Range<T>) -> Option<Vec<&T>>
fn range_inequality<T>(range: &Ranges<T>) -> Option<Vec<&T>>
where
T: Ord + Clone + fmt::Debug,
{
Expand All @@ -329,7 +329,7 @@ where
///
/// For example, `python_full_version < '3.8' or python_full_version >= '3.9'` can be simplified to
/// `python_full_version != '3.8.*'`.
fn star_range_inequality(range: &Range<Version>) -> Option<VersionSpecifier> {
fn star_range_inequality(range: &Ranges<Version>) -> Option<VersionSpecifier> {
let (b1, b2) = range.iter().collect_tuple()?;

match (b1, b2) {
Expand Down
10 changes: 5 additions & 5 deletions crates/uv-pep508/src/marker/tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ use std::ops::{Bound, Deref};
use std::str::FromStr;

use itertools::Itertools;
use pubgrub::Range;
use serde::{de, Deserialize, Deserializer, Serialize, Serializer};
use uv_normalize::ExtraName;
use uv_pep440::{Version, VersionParseError, VersionSpecifier};
use version_ranges::Ranges;

use crate::cursor::Cursor;
use crate::marker::parse;
Expand Down Expand Up @@ -1396,7 +1396,7 @@ pub enum MarkerTreeKind<'a> {
pub struct VersionMarkerTree<'a> {
id: NodeId,
key: MarkerValueVersion,
map: &'a [(Range<Version>, NodeId)],
map: &'a [(Ranges<Version>, NodeId)],
}

impl VersionMarkerTree<'_> {
Expand All @@ -1406,7 +1406,7 @@ impl VersionMarkerTree<'_> {
}

/// The edges of this node, corresponding to possible output ranges of the given variable.
pub fn edges(&self) -> impl ExactSizeIterator<Item = (&Range<Version>, MarkerTree)> + '_ {
pub fn edges(&self) -> impl ExactSizeIterator<Item = (&Ranges<Version>, MarkerTree)> + '_ {
self.map
.iter()
.map(|(range, node)| (range, MarkerTree(node.negate(self.id))))
Expand All @@ -1432,7 +1432,7 @@ impl Ord for VersionMarkerTree<'_> {
pub struct StringMarkerTree<'a> {
id: NodeId,
key: MarkerValueString,
map: &'a [(Range<String>, NodeId)],
map: &'a [(Ranges<String>, NodeId)],
}

impl StringMarkerTree<'_> {
Expand All @@ -1442,7 +1442,7 @@ impl StringMarkerTree<'_> {
}

/// The edges of this node, corresponding to possible output ranges of the given variable.
pub fn children(&self) -> impl ExactSizeIterator<Item = (&Range<String>, MarkerTree)> {
pub fn children(&self) -> impl ExactSizeIterator<Item = (&Ranges<String>, MarkerTree)> {
self.map
.iter()
.map(|(range, node)| (range, MarkerTree(node.negate(self.id))))
Expand Down
Loading

0 comments on commit f7435c6

Please sign in to comment.