From 6bebf79ac3eadfdb65cefae31e9eff05f340241a Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 30 Nov 2024 10:36:18 -0500 Subject: [PATCH] Remove uses of `Option` in `PubGrubPackage` (#9541) ## Summary Just use `MarkerTree::TRUE` instead of `None`. --- crates/uv-resolver/src/pubgrub/package.rs | 77 ++++++++----------- .../src/resolver/batch_prefetch.rs | 7 +- crates/uv-resolver/src/resolver/mod.rs | 20 ++--- 3 files changed, 45 insertions(+), 59 deletions(-) diff --git a/crates/uv-resolver/src/pubgrub/package.rs b/crates/uv-resolver/src/pubgrub/package.rs index 19fea7be8..32d21fb67 100644 --- a/crates/uv-resolver/src/pubgrub/package.rs +++ b/crates/uv-resolver/src/pubgrub/package.rs @@ -2,7 +2,7 @@ use std::ops::Deref; use std::sync::Arc; use uv_normalize::{ExtraName, GroupName, PackageName}; -use uv_pep508::{MarkerTree, MarkerTreeContents}; +use uv_pep508::MarkerTree; use uv_pypi_types::ConflictItemRef; use crate::python_requirement::PythonRequirement; @@ -52,7 +52,7 @@ pub(crate) enum PubGrubPackageInner { name: PackageName, extra: Option, dev: Option, - marker: Option, + marker: MarkerTree, }, /// A proxy package to represent a dependency with an extra (e.g., `black[colorama]`). /// @@ -70,7 +70,7 @@ pub(crate) enum PubGrubPackageInner { Extra { name: PackageName, extra: ExtraName, - marker: Option, + marker: MarkerTree, }, /// A proxy package to represent an enabled "dependency group" (e.g., development dependencies). /// @@ -80,7 +80,7 @@ pub(crate) enum PubGrubPackageInner { Dev { name: PackageName, dev: GroupName, - marker: Option, + marker: MarkerTree, }, /// A proxy package for a base package with a marker (e.g., `black; python_version >= "3.6"`). /// @@ -105,23 +105,22 @@ impl PubGrubPackage { // extras end up having two distinct marker expressions, which in turn // makes them two distinct packages. This results in PubGrub being // unable to unify version constraints across such packages. - let tree = marker.simplify_extras_with(|_| true); - let marker = tree.contents(); + let marker = marker.simplify_extras_with(|_| true); if let Some(extra) = extra { Self(Arc::new(PubGrubPackageInner::Extra { name, extra, marker, })) - } else if marker.is_some() { - Self(Arc::new(PubGrubPackageInner::Marker { name, marker: tree })) - } else { + } else if marker.is_true() { Self(Arc::new(PubGrubPackageInner::Package { name, extra, dev: None, marker, })) + } else { + Self(Arc::new(PubGrubPackageInner::Marker { name, marker })) } } @@ -160,9 +159,7 @@ impl PubGrubPackage { PubGrubPackageInner::Root(_) | PubGrubPackageInner::Python(_) => None, PubGrubPackageInner::Package { marker, .. } | PubGrubPackageInner::Extra { marker, .. } - | PubGrubPackageInner::Dev { marker, .. } => { - marker.as_ref().map(MarkerTreeContents::as_ref) - } + | PubGrubPackageInner::Dev { marker, .. } => Some(marker), PubGrubPackageInner::Marker { marker, .. } => Some(marker), } } @@ -254,14 +251,8 @@ impl PubGrubPackage { PubGrubPackageInner::Root(_) | PubGrubPackageInner::Python(_) => {} PubGrubPackageInner::Package { ref mut marker, .. } | PubGrubPackageInner::Extra { ref mut marker, .. } - | PubGrubPackageInner::Dev { ref mut marker, .. } => { - let Some(contents) = marker.as_mut() else { - return; - }; - let tree = MarkerTree::from(contents.clone()); - *marker = python_requirement.simplify_markers(tree).contents(); - } - PubGrubPackageInner::Marker { ref mut marker, .. } => { + | PubGrubPackageInner::Dev { ref mut marker, .. } + | PubGrubPackageInner::Marker { ref mut marker, .. } => { *marker = python_requirement.simplify_markers(marker.clone()); } } @@ -303,44 +294,38 @@ impl std::fmt::Display for PubGrubPackageInner { Self::Package { name, extra: None, - marker: None, + marker, dev: None, - } => write!(f, "{name}"), + } => { + if let Some(marker) = marker.contents() { + write!(f, "{name}{{{marker}}}") + } else { + write!(f, "{name}") + } + } Self::Package { name, extra: Some(extra), - marker: None, + marker, dev: None, } => { - write!(f, "{name}[{extra}]") + if let Some(marker) = marker.contents() { + write!(f, "{name}[{extra}]{{{marker}}}") + } else { + write!(f, "{name}[{extra}]") + } } Self::Package { name, extra: None, - marker: Some(marker), - dev: None, - } => write!(f, "{name}{{{marker}}}"), - Self::Package { - name, - extra: Some(extra), - marker: Some(marker), - dev: None, - } => { - write!(f, "{name}[{extra}]{{{marker}}}") - } - Self::Package { - name, - extra: None, - marker: None, - dev: Some(dev), - } => write!(f, "{name}:{dev}"), - Self::Package { - name, - extra: None, - marker: Some(marker), + marker, dev: Some(dev), } => { - write!(f, "{name}[{dev}]{{{marker}}}") + if let Some(marker) = marker.contents() { + write!(f, "{name}:{dev}{{{marker}}}") + } else { + write!(f, "{name}:{dev}") + } } Self::Marker { name, marker, .. } => { if let Some(marker) = marker.contents() { diff --git a/crates/uv-resolver/src/resolver/batch_prefetch.rs b/crates/uv-resolver/src/resolver/batch_prefetch.rs index ea73b0250..957f53cb3 100644 --- a/crates/uv-resolver/src/resolver/batch_prefetch.rs +++ b/crates/uv-resolver/src/resolver/batch_prefetch.rs @@ -15,6 +15,7 @@ use crate::{ use uv_distribution_types::{CompatibleDist, DistributionMetadata, IndexCapabilities, IndexUrl}; use uv_normalize::PackageName; use uv_pep440::Version; +use uv_pep508::MarkerTree; enum BatchPrefetchStrategy { /// Go through the next versions assuming the existing selection and its constraints @@ -64,7 +65,7 @@ impl BatchPrefetcher { name, extra: None, dev: None, - marker: None, + marker: MarkerTree::TRUE, } = &**next else { return Ok(()); @@ -232,7 +233,7 @@ impl BatchPrefetcher { name, extra: None, dev: None, - marker: None, + marker: MarkerTree::TRUE, } = &**package else { return; @@ -248,7 +249,7 @@ impl BatchPrefetcher { name, extra: None, dev: None, - marker: None, + marker: MarkerTree::TRUE, } = &**next else { return (0, false); diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs index 789214f65..194a74507 100644 --- a/crates/uv-resolver/src/resolver/mod.rs +++ b/crates/uv-resolver/src/resolver/mod.rs @@ -822,7 +822,7 @@ impl ResolverState ResolverState { return Ok(Dependencies::Unforkable( - [None, Some(marker)] + [MarkerTree::TRUE, marker.clone()] .into_iter() .map(move |marker| PubGrubDependency { package: PubGrubPackage::from(PubGrubPackageInner::Package { name: name.clone(), extra: None, dev: None, - marker: marker.and_then(MarkerTree::contents), + marker, }), version: Range::singleton(version.clone()), url: None, @@ -1513,7 +1513,7 @@ impl ResolverState { return Ok(Dependencies::Unforkable( - [None, marker.as_ref()] + [&MarkerTree::TRUE, marker] .into_iter() .dedup() .flat_map(move |marker| { @@ -1524,7 +1524,7 @@ impl ResolverState ResolverState { return Ok(Dependencies::Unforkable( - [None, marker.as_ref()] + [&MarkerTree::TRUE, marker] .into_iter() .dedup() .flat_map(move |marker| { @@ -1549,7 +1549,7 @@ impl ResolverState