diff --git a/crates/uv-resolver/src/lib.rs b/crates/uv-resolver/src/lib.rs index 90cd4634ce79..bce29ec2eff3 100644 --- a/crates/uv-resolver/src/lib.rs +++ b/crates/uv-resolver/src/lib.rs @@ -22,7 +22,7 @@ pub use resolver::{ PackageVersionsResult, Reporter as ResolverReporter, Resolver, ResolverEnvironment, ResolverProvider, VersionsResponse, WheelMetadataResult, }; -pub use universal_marker::UniversalMarker; +pub use universal_marker::{ConflictMarker, UniversalMarker}; pub use version_map::VersionMap; pub use yanks::AllowedYanks; diff --git a/crates/uv-resolver/src/lock/mod.rs b/crates/uv-resolver/src/lock/mod.rs index dd5b6e15f26b..6816e1387ccc 100644 --- a/crates/uv-resolver/src/lock/mod.rs +++ b/crates/uv-resolver/src/lock/mod.rs @@ -19,7 +19,7 @@ pub use crate::lock::target::InstallTarget; pub use crate::lock::tree::TreeDisplay; use crate::requires_python::SimplifiedMarkerTree; use crate::resolution::{AnnotatedDist, ResolutionGraphNode}; -use crate::universal_marker::UniversalMarker; +use crate::universal_marker::{ConflictMarker, UniversalMarker}; use crate::{ ExcludeNewer, InMemoryIndex, MetadataResponse, PrereleaseMode, RequiresPython, ResolutionMode, ResolverOutput, @@ -61,21 +61,21 @@ static LINUX_MARKERS: LazyLock = LazyLock::new(|| { "platform_system == 'Linux' and os_name == 'posix' and sys_platform == 'linux'", ) .unwrap(); - UniversalMarker::new(pep508, MarkerTree::TRUE) + UniversalMarker::new(pep508, ConflictMarker::TRUE) }); static WINDOWS_MARKERS: LazyLock = LazyLock::new(|| { let pep508 = MarkerTree::from_str( "platform_system == 'Windows' and os_name == 'nt' and sys_platform == 'win32'", ) .unwrap(); - UniversalMarker::new(pep508, MarkerTree::TRUE) + UniversalMarker::new(pep508, ConflictMarker::TRUE) }); static MAC_MARKERS: LazyLock = LazyLock::new(|| { let pep508 = MarkerTree::from_str( "platform_system == 'Darwin' and os_name == 'posix' and sys_platform == 'darwin'", ) .unwrap(); - UniversalMarker::new(pep508, MarkerTree::TRUE) + UniversalMarker::new(pep508, ConflictMarker::TRUE) }); #[derive(Clone, Debug, serde::Deserialize)] @@ -1447,7 +1447,9 @@ impl TryFrom for Lock { .map(|simplified_marker| simplified_marker.into_marker(&wire.requires_python)) // TODO(ag): Consider whether this should also deserialize a conflict marker. // We currently aren't serializing. Dropping it completely is likely to be wrong. - .map(|complexified_marker| UniversalMarker::new(complexified_marker, MarkerTree::TRUE)) + .map(|complexified_marker| { + UniversalMarker::new(complexified_marker, ConflictMarker::TRUE) + }) .collect(); let lock = Lock::new( wire.version, @@ -2244,7 +2246,7 @@ impl PackageWire { // TODO(ag): Consider whether this should also deserialize a conflict marker. // We currently aren't serializing. Dropping it completely is likely to be wrong. .map(|complexified_marker| { - UniversalMarker::new(complexified_marker, MarkerTree::TRUE) + UniversalMarker::new(complexified_marker, ConflictMarker::TRUE) }) .collect(), dependencies: unwire_deps(self.dependencies)?, @@ -3579,8 +3581,11 @@ impl Dependency { if let Some(marker) = self.simplified_marker.try_to_string() { table.insert("marker", value(marker)); } - if let Some(conflict_marker) = self.complexified_marker.conflict().try_to_string() { - table.insert("conflict-marker", value(conflict_marker)); + if !self.complexified_marker.conflict().is_true() { + table.insert( + "conflict-marker", + value(self.complexified_marker.conflict().to_string()), + ); } table @@ -3618,7 +3623,7 @@ struct DependencyWire { #[serde(default)] marker: SimplifiedMarkerTree, #[serde(rename = "conflict-marker", default)] - conflict_marker: MarkerTree, + conflict_marker: ConflictMarker, } impl DependencyWire { diff --git a/crates/uv-resolver/src/lock/requirements_txt.rs b/crates/uv-resolver/src/lock/requirements_txt.rs index b84be3e42364..0f9bca1e31b1 100644 --- a/crates/uv-resolver/src/lock/requirements_txt.rs +++ b/crates/uv-resolver/src/lock/requirements_txt.rs @@ -20,7 +20,7 @@ use uv_pypi_types::{ParsedArchiveUrl, ParsedGitUrl}; use crate::graph_ops::marker_reachability; use crate::lock::{LockErrorKind, Package, PackageId, Source}; -use crate::universal_marker::UniversalMarker; +use crate::universal_marker::{ConflictMarker, UniversalMarker}; use crate::{InstallTarget, LockError}; /// An export of a [`Lock`] that renders in `requirements.txt` format. @@ -122,7 +122,7 @@ impl<'lock> RequirementsTxtExport<'lock> { // `marker_reachability` wants and it (probably) isn't // worth inventing a new abstraction so that it can accept // graphs with either `MarkerTree` or `UniversalMarker`. - MarkerTree::TRUE, + ConflictMarker::TRUE, ), ); @@ -175,7 +175,7 @@ impl<'lock> RequirementsTxtExport<'lock> { dep.simplified_marker.as_simplified_marker_tree(), // See note above for other `UniversalMarker::new` for // why this is OK. - MarkerTree::TRUE, + ConflictMarker::TRUE, ), ); diff --git a/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_unambiguous.snap b/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_unambiguous.snap index 0ee9dabab833..52b180c7e329 100644 --- a/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_unambiguous.snap +++ b/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_unambiguous.snap @@ -137,7 +137,7 @@ Ok( ), complexified_marker: UniversalMarker { pep508_marker: python_full_version >= '3.12', - conflict_marker: true, + conflict_marker: ConflictMarker(true), }, }, ], diff --git a/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_version_unambiguous.snap b/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_version_unambiguous.snap index 0ee9dabab833..52b180c7e329 100644 --- a/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_version_unambiguous.snap +++ b/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_version_unambiguous.snap @@ -137,7 +137,7 @@ Ok( ), complexified_marker: UniversalMarker { pep508_marker: python_full_version >= '3.12', - conflict_marker: true, + conflict_marker: ConflictMarker(true), }, }, ], diff --git a/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_version_unambiguous.snap b/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_version_unambiguous.snap index 0ee9dabab833..52b180c7e329 100644 --- a/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_version_unambiguous.snap +++ b/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_version_unambiguous.snap @@ -137,7 +137,7 @@ Ok( ), complexified_marker: UniversalMarker { pep508_marker: python_full_version >= '3.12', - conflict_marker: true, + conflict_marker: ConflictMarker(true), }, }, ], diff --git a/crates/uv-resolver/src/resolver/environment.rs b/crates/uv-resolver/src/resolver/environment.rs index 3f77cc08917b..f7feb4b625c1 100644 --- a/crates/uv-resolver/src/resolver/environment.rs +++ b/crates/uv-resolver/src/resolver/environment.rs @@ -6,7 +6,7 @@ use uv_pypi_types::{ConflictItem, ConflictItemRef, ResolverMarkerEnvironment}; use crate::pubgrub::{PubGrubDependency, PubGrubPackage}; use crate::requires_python::RequiresPythonRange; use crate::resolver::ForkState; -use crate::universal_marker::UniversalMarker; +use crate::universal_marker::{ConflictMarker, UniversalMarker}; use crate::PythonRequirement; /// Represents one or more marker environments for a resolution. @@ -380,23 +380,16 @@ impl ResolverEnvironment { .. } => { // FIXME: Account for groups here in addition to extras. - let mut conflict_marker = MarkerTree::TRUE; + let mut conflict_marker = ConflictMarker::TRUE; for item in exclude.iter() { if let Some(extra) = item.extra() { - let operator = uv_pep508::ExtraOperator::NotEqual; - let name = uv_pep508::MarkerValueExtra::Extra(extra.clone()); - let expr = uv_pep508::MarkerExpression::Extra { operator, name }; - let exclude_extra_marker = MarkerTree::expression(expr); - conflict_marker.and(exclude_extra_marker); + conflict_marker = + conflict_marker.and(ConflictMarker::extra(extra.clone()).negate()); } } for item in include.iter() { if let Some(extra) = item.extra() { - let operator = uv_pep508::ExtraOperator::Equal; - let name = uv_pep508::MarkerValueExtra::Extra(extra.clone()); - let expr = uv_pep508::MarkerExpression::Extra { operator, name }; - let exclude_extra_marker = MarkerTree::expression(expr); - conflict_marker.and(exclude_extra_marker); + conflict_marker = conflict_marker.and(ConflictMarker::extra(extra.clone())); } } Some(UniversalMarker::new(*markers, conflict_marker)) diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs index f9a72fcbabb0..c4147619dca9 100644 --- a/crates/uv-resolver/src/resolver/mod.rs +++ b/crates/uv-resolver/src/resolver/mod.rs @@ -63,7 +63,7 @@ use crate::resolver::environment::ForkingPossibility; pub use crate::resolver::environment::ResolverEnvironment; pub(crate) use crate::resolver::fork_map::{ForkMap, ForkSet}; pub(crate) use crate::resolver::urls::Urls; -use crate::universal_marker::UniversalMarker; +use crate::universal_marker::{ConflictMarker, UniversalMarker}; use crate::resolver::groups::Groups; pub use crate::resolver::index::InMemoryIndex; @@ -2704,7 +2704,7 @@ impl ResolutionDependencyEdge { // We specifically do not account for conflict // markers here. Instead, those are computed via // a traversal on the resolution graph. - UniversalMarker::new(self.marker, MarkerTree::TRUE) + UniversalMarker::new(self.marker, ConflictMarker::TRUE) } } diff --git a/crates/uv-resolver/src/universal_marker.rs b/crates/uv-resolver/src/universal_marker.rs index 707df88f15ff..71c154e74455 100644 --- a/crates/uv-resolver/src/universal_marker.rs +++ b/crates/uv-resolver/src/universal_marker.rs @@ -1,10 +1,18 @@ +#![allow(warnings)] + use itertools::Itertools; use uv_configuration::{DevGroupsManifest, ExtrasSpecification}; use uv_normalize::ExtraName; -use uv_pep508::{MarkerEnvironment, MarkerTree}; +use uv_pep508::{MarkerEnvironment, MarkerEnvironmentBuilder, MarkerTree}; use uv_pypi_types::Conflicts; +// BREADCRUMBS: Work on a new `ConflictMarker` type instead of using +// `MarkerTree` directly below. And instead of storing only extra/group +// names, we need to store package names too. Wire everything up. This +// will also take us toward making groups work. But keep going with just +// extras for now. + /// A representation of a marker for use in universal resolution. /// /// (This also degrades gracefully to a standard PEP 508 marker in the case of @@ -20,24 +28,27 @@ use uv_pypi_types::Conflicts; #[derive(Debug, Default, Copy, Clone, Eq, Hash, PartialEq, PartialOrd, Ord)] pub struct UniversalMarker { pep508_marker: MarkerTree, - conflict_marker: MarkerTree, + conflict_marker: ConflictMarker, } impl UniversalMarker { /// A constant universal marker that always evaluates to `true`. pub(crate) const TRUE: UniversalMarker = UniversalMarker { pep508_marker: MarkerTree::TRUE, - conflict_marker: MarkerTree::TRUE, + conflict_marker: ConflictMarker::TRUE, }; /// A constant universal marker that always evaluates to `false`. pub(crate) const FALSE: UniversalMarker = UniversalMarker { pep508_marker: MarkerTree::FALSE, - conflict_marker: MarkerTree::FALSE, + conflict_marker: ConflictMarker::FALSE, }; /// Creates a new universal marker from its constituent pieces. - pub(crate) fn new(pep508_marker: MarkerTree, conflict_marker: MarkerTree) -> UniversalMarker { + pub(crate) fn new( + pep508_marker: MarkerTree, + conflict_marker: ConflictMarker, + ) -> UniversalMarker { UniversalMarker { pep508_marker, conflict_marker, @@ -49,7 +60,7 @@ impl UniversalMarker { /// `other` evaluate to `true`. pub(crate) fn or(&mut self, other: UniversalMarker) { self.pep508_marker.or(other.pep508_marker); - self.conflict_marker.or(other.conflict_marker); + self.conflict_marker = self.conflict_marker.or(other.conflict_marker); } /// Combine this universal marker with the one given in a way that @@ -57,7 +68,7 @@ impl UniversalMarker { /// `self` and `other` evaluate to `true`. pub(crate) fn and(&mut self, other: UniversalMarker) { self.pep508_marker.and(other.pep508_marker); - self.conflict_marker.and(other.conflict_marker); + self.conflict_marker = self.conflict_marker.and(other.conflict_marker); } /// Imbibes the world knowledge expressed by `conflicts` into this marker. @@ -76,7 +87,7 @@ impl UniversalMarker { // program. But it's doing it every time this routine // is called. We should refactor the caller to build // a marker from the `conflicts` once. - let mut marker = MarkerTree::FALSE; + let mut marker = ConflictMarker::FALSE; for set in conflicts.iter() { for (item1, item2) in set.iter().tuple_combinations() { // FIXME: Account for groups here. And extra/group @@ -84,26 +95,14 @@ impl UniversalMarker { let (Some(extra1), Some(extra2)) = (item1.extra(), item2.extra()) else { continue; }; - - let operator = uv_pep508::ExtraOperator::Equal; - let name = uv_pep508::MarkerValueExtra::Extra(extra1.clone()); - let expr = uv_pep508::MarkerExpression::Extra { operator, name }; - let marker1 = MarkerTree::expression(expr); - - let operator = uv_pep508::ExtraOperator::Equal; - let name = uv_pep508::MarkerValueExtra::Extra(extra2.clone()); - let expr = uv_pep508::MarkerExpression::Extra { operator, name }; - let marker2 = MarkerTree::expression(expr); - - let mut pair = MarkerTree::TRUE; - pair.and(marker1); - pair.and(marker2); - marker.or(pair); + let pair = ConflictMarker::extra(extra1.clone()) + .and(ConflictMarker::extra(extra2.clone())); + marker = marker.or(pair); } } - let mut marker = marker.negate(); - marker.implies(std::mem::take(&mut self.conflict_marker)); - self.conflict_marker = marker; + self.conflict_marker = marker + .negate() + .implies(std::mem::take(&mut self.conflict_marker)); } /// Assumes that a given extra is activated. @@ -111,8 +110,7 @@ impl UniversalMarker { /// This may simplify the conflicting marker component of this universal /// marker. pub(crate) fn assume_extra(&mut self, extra: &ExtraName) { - self.conflict_marker = std::mem::take(&mut self.conflict_marker) - .simplify_extras_with(|candidate| candidate == extra); + self.conflict_marker = self.conflict_marker.assume_extra(extra); } /// Returns true if this universal marker will always evaluate to `true`. @@ -137,7 +135,7 @@ impl UniversalMarker { /// Returns true if this universal marker is satisfied by the given /// marker environment and list of activated extras. pub(crate) fn evaluate(self, env: &MarkerEnvironment, extras: &[ExtraName]) -> bool { - self.pep508_marker.evaluate(env, extras) && self.conflict_marker.evaluate(env, extras) + self.pep508_marker.evaluate(env, extras) && self.conflict_marker.evaluate(extras) } /// Returns true if this universal marker is satisfied by the given @@ -159,7 +157,7 @@ impl UniversalMarker { ExtrasSpecification::None => &[][..], ExtrasSpecification::Some(ref list) => list, }; - self.conflict_marker.evaluate(env, extra_list) + self.conflict_marker.evaluate(extra_list) } /// Returns the PEP 508 marker for this universal marker. @@ -185,7 +183,7 @@ impl UniversalMarker { /// of non-trivial conflict markers and fails if any are found. (Because /// conflict markers cannot be represented in the `requirements.txt` /// format.) - pub fn conflict(self) -> MarkerTree { + pub fn conflict(self) -> ConflictMarker { self.conflict_marker } } @@ -197,14 +195,190 @@ impl std::fmt::Display for UniversalMarker { } match ( self.pep508_marker.contents(), - self.conflict_marker.contents(), + self.conflict_marker.is_true(), ) { - (None, None) => write!(f, "`true`"), - (Some(pep508), None) => write!(f, "`{pep508}`"), - (None, Some(conflict)) => write!(f, "`true` (conflict marker: `{conflict}`)"), - (Some(pep508), Some(conflict)) => { - write!(f, "`{pep508}` (conflict marker: `{conflict}`)") + (None, true) => write!(f, "`true`"), + (Some(pep508), true) => write!(f, "`{pep508}`"), + (None, false) => write!(f, "`true` (conflict marker: `{}`)", self.conflict_marker), + (Some(pep508), false) => { + write!( + f, + "`{pep508}` (conflict marker: `{}`)", + self.conflict_marker + ) } } } } + +#[derive(Default, Clone, Copy, Eq, Hash, PartialEq, PartialOrd, Ord)] +pub struct ConflictMarker { + marker: MarkerTree, +} + +impl ConflictMarker { + /// A constant conflict marker that always evaluates to `true`. + pub const TRUE: ConflictMarker = ConflictMarker { + marker: MarkerTree::TRUE, + }; + + /// A constant conflict marker that always evaluates to `false`. + pub const FALSE: ConflictMarker = ConflictMarker { + marker: MarkerTree::FALSE, + }; + + /// Create a conflict marker that is true only when the given extra is + /// activated. + pub fn extra(name: ExtraName) -> ConflictMarker { + let operator = uv_pep508::ExtraOperator::Equal; + let name = uv_pep508::MarkerValueExtra::Extra(name); + let expr = uv_pep508::MarkerExpression::Extra { operator, name }; + let marker = MarkerTree::expression(expr); + ConflictMarker { marker } + } + + /// Returns a new conflict marker that is the negation of this one. + #[must_use] + pub fn negate(self) -> ConflictMarker { + ConflictMarker { + marker: self.marker.negate(), + } + } + + /// Returns a new conflict marker corresponding to the union of `self` and + /// `other`. + #[must_use] + pub fn or(self, other: ConflictMarker) -> ConflictMarker { + let mut marker = self.marker; + marker.or(other.marker); + ConflictMarker { marker } + } + + /// Returns a new conflict marker corresponding to the intersection of + /// `self` and `other`. + #[must_use] + pub fn and(self, other: ConflictMarker) -> ConflictMarker { + let mut marker = self.marker; + marker.and(other.marker); + ConflictMarker { marker } + } + + /// Returns a new conflict marker corresponding to the logical implication + /// of `self` and the given consequent. + /// + /// If the conflict marker returned is always `true`, then it can be said + /// that `self` implies `consequent`. + #[must_use] + pub fn implies(self, other: ConflictMarker) -> ConflictMarker { + let mut marker = self.marker; + marker.implies(other.marker); + ConflictMarker { marker } + } + + /// Returns a new conflict marker with the given extra assumed to be true. + /// + /// This may simplify the marker. + #[must_use] + pub(crate) fn assume_extra(&self, extra: &ExtraName) -> ConflictMarker { + let marker = self + .marker + .clone() + .simplify_extras_with(|candidate| candidate == extra); + ConflictMarker { marker } + } + + /// Returns true if this conflict marker will always evaluate to `true`. + pub fn is_true(self) -> bool { + self.marker.is_true() + } + + /// Returns true if this conflict marker will always evaluate to `false`. + pub fn is_false(self) -> bool { + self.marker.is_false() + } + + /// Returns true if this conflict marker is disjoint with the one given. + /// + /// Two conflict markers are disjoint when it is impossible for them both + /// to evaluate to `true` simultaneously. + pub(crate) fn is_disjoint(self, other: ConflictMarker) -> bool { + self.marker.is_disjoint(other.marker) + } + + /// Returns true if this conflict marker is satisfied by the given + /// list of activated extras. + pub(crate) fn evaluate(&self, extras: &[ExtraName]) -> bool { + static DUMMY: std::sync::LazyLock = std::sync::LazyLock::new(|| { + MarkerEnvironment::try_from(MarkerEnvironmentBuilder { + implementation_name: "", + implementation_version: "3.7", + os_name: "linux", + platform_machine: "", + platform_python_implementation: "", + platform_release: "", + platform_system: "", + platform_version: "", + python_full_version: "3.7", + python_version: "3.7", + sys_platform: "linux", + }) + .unwrap() + }); + self.marker.evaluate(&DUMMY, extras) + } +} + +impl std::fmt::Display for ConflictMarker { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + // TODO: This shouldn't be exposing the internal marker, + // but instead transforming it. + if self.marker.is_false() { + return write!(f, "false"); + } + let Some(contents) = self.marker.contents() else { + return write!(f, "true"); + }; + write!(f, "{contents}") + } +} + +impl std::fmt::Debug for ConflictMarker { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + // This intentionally exposes the underlying PEP 508 marker + // representation so that programmers can debug it if there's + // something wrong with it. + write!(f, "ConflictMarker({:?})", self.marker) + } +} + +// TODO: This should parse a custom format. But we expose +// the raw PEP 508 marker for now for simplicity. We'll +// write the custom parser later. +impl std::str::FromStr for ConflictMarker { + type Err = uv_pep508::Pep508Error; + + fn from_str(markers: &str) -> Result { + Ok(ConflictMarker { + marker: markers.parse()?, + }) + } +} + +impl<'de> serde::Deserialize<'de> for ConflictMarker { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let s = String::deserialize(deserializer)?; + std::str::FromStr::from_str(&s).map_err(serde::de::Error::custom) + } +} + +impl serde::Serialize for ConflictMarker { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + serializer.serialize_str(&self.to_string()) + } +}