From d18753527fef89fd9fa840da199fad0b787c9a91 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 26 Nov 2024 09:39:36 -0500 Subject: [PATCH] Remove `python_version` from lowered marker representation (#9343) ## Summary We never construct these -- they should be impossible, since we always translate to `python_full_version`. This PR encodes that impossibility in the types. --- crates/uv-pep508/src/lib.rs | 18 +----- crates/uv-pep508/src/marker/algebra.rs | 64 +++++++++++++--------- crates/uv-pep508/src/marker/environment.rs | 1 - crates/uv-pep508/src/marker/lowering.rs | 14 ----- crates/uv-pep508/src/marker/tree.rs | 44 --------------- crates/uv-resolver/src/marker.rs | 3 +- 6 files changed, 39 insertions(+), 105 deletions(-) diff --git a/crates/uv-pep508/src/lib.rs b/crates/uv-pep508/src/lib.rs index 338c4e146e29..157c877460ee 100644 --- a/crates/uv-pep508/src/lib.rs +++ b/crates/uv-pep508/src/lib.rs @@ -16,7 +16,6 @@ #![warn(missing_docs)] -use std::collections::HashSet; use std::error::Error; use std::fmt::{Debug, Display, Formatter}; use std::path::Path; @@ -41,7 +40,7 @@ pub use uv_normalize::{ExtraName, InvalidNameError, PackageName}; /// Version and version specifiers used in requirements (reexport). // https://github.com/konstin/pep508_rs/issues/19 pub use uv_pep440; -use uv_pep440::{Version, VersionSpecifier, VersionSpecifiers}; +use uv_pep440::{VersionSpecifier, VersionSpecifiers}; pub use verbatim_url::{ expand_env_vars, split_scheme, strip_host, Scheme, VerbatimUrl, VerbatimUrlError, }; @@ -207,21 +206,6 @@ impl Requirement { self.marker.evaluate(env, extras) } - /// Returns whether the requirement would be satisfied, independent of environment markers, i.e. - /// if there is potentially an environment that could activate this requirement. - /// - /// Note that unlike [`Self::evaluate_markers`] this does not perform any checks for bogus - /// expressions but will simply return true. As caller you should separately perform a check - /// with an environment and forward all warnings. - pub fn evaluate_extras_and_python_version( - &self, - extras: &HashSet, - python_versions: &[Version], - ) -> bool { - self.marker - .evaluate_extras_and_python_version(extras, python_versions) - } - /// Return the requirement with an additional marker added, to require the given extra. /// /// For example, given `flask >= 2.0.2`, calling `with_extra_marker("dotenv")` would return diff --git a/crates/uv-pep508/src/marker/algebra.rs b/crates/uv-pep508/src/marker/algebra.rs index 4a18acd2fff0..510aff705e3e 100644 --- a/crates/uv-pep508/src/marker/algebra.rs +++ b/crates/uv-pep508/src/marker/algebra.rs @@ -158,44 +158,54 @@ impl InternerGuard<'_> { /// Returns a decision node for a single marker expression. pub(crate) fn expression(&mut self, expr: MarkerExpression) -> NodeId { let (var, children) = match expr { - // Normalize `python_version` markers to `python_full_version` nodes. - MarkerExpression::Version { - key: MarkerValueVersion::PythonVersion, - specifier, - } => match python_version_to_full_version(normalize_specifier(specifier)) { - Ok(specifier) => ( - Variable::Version(LoweredMarkerValueVersion::PythonFullVersion), + // A variable representing the output of a version key. Edges correspond + // to disjoint version ranges. + MarkerExpression::Version { key, specifier } => match key { + MarkerValueVersion::ImplementationVersion => ( + Variable::Version(LoweredMarkerValueVersion::ImplementationVersion), Edges::from_specifier(specifier), ), - Err(node) => return node, - }, - MarkerExpression::VersionIn { - key: MarkerValueVersion::PythonVersion, - versions, - negated, - } => match Edges::from_python_versions(versions, negated) { - Ok(edges) => ( + MarkerValueVersion::PythonFullVersion => ( Variable::Version(LoweredMarkerValueVersion::PythonFullVersion), - edges, + Edges::from_specifier(specifier), ), - Err(node) => return node, + // Normalize `python_version` markers to `python_full_version` nodes. + MarkerValueVersion::PythonVersion => { + match python_version_to_full_version(normalize_specifier(specifier)) { + Ok(specifier) => ( + Variable::Version(LoweredMarkerValueVersion::PythonFullVersion), + Edges::from_specifier(specifier), + ), + Err(node) => return node, + } + } }, // A variable representing the output of a version key. Edges correspond // to disjoint version ranges. - MarkerExpression::Version { key, specifier } => ( - Variable::Version(key.into()), - Edges::from_specifier(specifier), - ), - // A variable representing the output of a version key. Edges correspond - // to disjoint version ranges. MarkerExpression::VersionIn { key, versions, negated, - } => ( - Variable::Version(key.into()), - Edges::from_versions(&versions, negated), - ), + } => match key { + MarkerValueVersion::ImplementationVersion => ( + Variable::Version(LoweredMarkerValueVersion::ImplementationVersion), + Edges::from_versions(&versions, negated), + ), + MarkerValueVersion::PythonFullVersion => ( + Variable::Version(LoweredMarkerValueVersion::PythonFullVersion), + Edges::from_versions(&versions, negated), + ), + // Normalize `python_version` markers to `python_full_version` nodes. + MarkerValueVersion::PythonVersion => { + match Edges::from_python_versions(versions, negated) { + Ok(edges) => ( + Variable::Version(LoweredMarkerValueVersion::PythonFullVersion), + edges, + ), + Err(node) => return node, + } + } + }, // The `in` and `contains` operators are a bit different than other operators. // In particular, they do not represent a particular value for the corresponding // variable, and can overlap. For example, `'nux' in os_name` and `os_name == 'Linux'` diff --git a/crates/uv-pep508/src/marker/environment.rs b/crates/uv-pep508/src/marker/environment.rs index 5ae1d5fc390f..8f6a2d52add0 100644 --- a/crates/uv-pep508/src/marker/environment.rs +++ b/crates/uv-pep508/src/marker/environment.rs @@ -39,7 +39,6 @@ impl MarkerEnvironment { &self.implementation_version().version } LoweredMarkerValueVersion::PythonFullVersion => &self.python_full_version().version, - LoweredMarkerValueVersion::PythonVersion => &self.python_version().version, } } diff --git a/crates/uv-pep508/src/marker/lowering.rs b/crates/uv-pep508/src/marker/lowering.rs index c4677cd1387a..bb40275f2bef 100644 --- a/crates/uv-pep508/src/marker/lowering.rs +++ b/crates/uv-pep508/src/marker/lowering.rs @@ -12,8 +12,6 @@ pub enum LoweredMarkerValueVersion { ImplementationVersion, /// `python_full_version` PythonFullVersion, - /// `python_version` - PythonVersion, } impl Display for LoweredMarkerValueVersion { @@ -21,17 +19,6 @@ impl Display for LoweredMarkerValueVersion { match self { Self::ImplementationVersion => f.write_str("implementation_version"), Self::PythonFullVersion => f.write_str("python_full_version"), - Self::PythonVersion => f.write_str("python_version"), - } - } -} - -impl From for LoweredMarkerValueVersion { - fn from(value: MarkerValueVersion) -> Self { - match value { - MarkerValueVersion::ImplementationVersion => Self::ImplementationVersion, - MarkerValueVersion::PythonFullVersion => Self::PythonFullVersion, - MarkerValueVersion::PythonVersion => Self::PythonVersion, } } } @@ -41,7 +28,6 @@ impl From for MarkerValueVersion { match value { LoweredMarkerValueVersion::ImplementationVersion => Self::ImplementationVersion, LoweredMarkerValueVersion::PythonFullVersion => Self::PythonFullVersion, - LoweredMarkerValueVersion::PythonVersion => Self::PythonVersion, } } } diff --git a/crates/uv-pep508/src/marker/tree.rs b/crates/uv-pep508/src/marker/tree.rs index 7697751f72d8..399f17395a6a 100644 --- a/crates/uv-pep508/src/marker/tree.rs +++ b/crates/uv-pep508/src/marker/tree.rs @@ -1,5 +1,4 @@ use std::cmp::Ordering; -use std::collections::HashSet; use std::fmt::{self, Display, Formatter}; use std::ops::{Bound, Deref}; use std::str::FromStr; @@ -922,49 +921,6 @@ impl MarkerTree { false } - /// Checks if the requirement should be activated with the given set of active extras and a set - /// of possible python versions (from `requires-python`) without evaluating the remaining - /// environment markers, i.e. if there is potentially an environment that could activate this - /// requirement. - /// - /// Note that unlike [`Self::evaluate`] this does not perform any checks for bogus expressions but - /// will simply return true. As caller you should separately perform a check with an environment - /// and forward all warnings. - pub fn evaluate_extras_and_python_version( - &self, - extras: &HashSet, - python_versions: &[Version], - ) -> bool { - match self.kind() { - MarkerTreeKind::True => true, - MarkerTreeKind::False => false, - MarkerTreeKind::Version(marker) => marker.edges().any(|(range, tree)| { - if marker.key() == LoweredMarkerValueVersion::PythonVersion { - if !python_versions - .iter() - .any(|version| range.contains(version)) - { - return false; - } - } - - tree.evaluate_extras_and_python_version(extras, python_versions) - }), - MarkerTreeKind::String(marker) => marker - .children() - .any(|(_, tree)| tree.evaluate_extras_and_python_version(extras, python_versions)), - MarkerTreeKind::In(marker) => marker - .children() - .any(|(_, tree)| tree.evaluate_extras_and_python_version(extras, python_versions)), - MarkerTreeKind::Contains(marker) => marker - .children() - .any(|(_, tree)| tree.evaluate_extras_and_python_version(extras, python_versions)), - MarkerTreeKind::Extra(marker) => marker - .edge(extras.contains(marker.name().extra())) - .evaluate_extras_and_python_version(extras, python_versions), - } - } - /// Checks if the requirement should be activated with the given set of active extras without evaluating /// the remaining environment markers, i.e. if there is potentially an environment that could activate this /// requirement. diff --git a/crates/uv-resolver/src/marker.rs b/crates/uv-resolver/src/marker.rs index 9e141ef78095..e6420fd59325 100644 --- a/crates/uv-resolver/src/marker.rs +++ b/crates/uv-resolver/src/marker.rs @@ -10,8 +10,7 @@ pub(crate) fn requires_python(tree: &MarkerTree) -> Option match tree.kind() { MarkerTreeKind::True | MarkerTreeKind::False => {} MarkerTreeKind::Version(marker) => match marker.key() { - LoweredMarkerValueVersion::PythonVersion - | LoweredMarkerValueVersion::PythonFullVersion => { + LoweredMarkerValueVersion::PythonFullVersion => { for (range, tree) in marker.edges() { if !tree.is_false() { markers.push(range.clone());