From 8ce9051296b897fb63bae221467f7ae67380fe77 Mon Sep 17 00:00:00 2001 From: Ibraheem Ahmed Date: Tue, 14 May 2024 11:02:30 -0400 Subject: [PATCH] Parse marker tree before evaluation (#3520) ## Summary Parse `MarkerTree` expressions upfront, instead of lazily during evaluation. This makes implementing https://github.com/astral-sh/uv/issues/3355 a lot easier. --- crates/pep508-rs/src/lib.rs | 124 +- crates/pep508-rs/src/marker.rs | 1115 +++++++++++------ crates/pep508-rs/src/unnamed.rs | 16 +- crates/requirements-txt/src/requirement.rs | 5 +- ...__line-endings-poetry-with-hashes.txt.snap | 150 +-- ...t__test__parse-poetry-with-hashes.txt.snap | 150 +-- crates/uv-resolver/src/resolution.rs | 51 +- 7 files changed, 955 insertions(+), 656 deletions(-) diff --git a/crates/pep508-rs/src/lib.rs b/crates/pep508-rs/src/lib.rs index 4926e4d90..f88e5233b 100644 --- a/crates/pep508-rs/src/lib.rs +++ b/crates/pep508-rs/src/lib.rs @@ -40,8 +40,9 @@ use unicode_width::UnicodeWidthChar; use url::Url; pub use marker::{ - MarkerEnvironment, MarkerEnvironmentBuilder, MarkerExpression, MarkerOperator, MarkerTree, - MarkerValue, MarkerValueString, MarkerValueVersion, MarkerWarningKind, StringVersion, + ExtraOperator, MarkerEnvironment, MarkerEnvironmentBuilder, MarkerExpression, MarkerOperator, + MarkerTree, MarkerValue, MarkerValueString, MarkerValueVersion, MarkerWarningKind, + StringVersion, }; #[cfg(feature = "pyo3")] use pep440_rs::PyVersion; @@ -220,7 +221,7 @@ impl Serialize for Requirement { } } -type MarkerWarning = (MarkerWarningKind, String, String); +type MarkerWarning = (MarkerWarningKind, String); #[cfg(feature = "pyo3")] #[pyclass(module = "pep508", name = "Requirement")] @@ -437,16 +438,14 @@ impl Requirement { let marker = match self.marker { Some(expression) => MarkerTree::And(vec![ expression, - MarkerTree::Expression(MarkerExpression { - l_value: MarkerValue::Extra, - operator: MarkerOperator::Equal, - r_value: MarkerValue::QuotedString(extra.to_string()), + MarkerTree::Expression(MarkerExpression::Extra { + operator: ExtraOperator::Equal, + name: extra.clone(), }), ]), - None => MarkerTree::Expression(MarkerExpression { - l_value: MarkerValue::Extra, - operator: MarkerOperator::Equal, - r_value: MarkerValue::QuotedString(extra.to_string()), + None => MarkerTree::Expression(MarkerExpression::Extra { + operator: ExtraOperator::Equal, + name: extra.clone(), }), }; Self { @@ -473,19 +472,64 @@ impl Pep508Url for Url { } } +/// A reporter for warnings that occur during marker parsing or evaluation. +pub trait Reporter { + /// Report a warning. + fn report(&mut self, kind: MarkerWarningKind, warning: String); +} + +impl Reporter for F +where + F: FnMut(MarkerWarningKind, String), +{ + fn report(&mut self, kind: MarkerWarningKind, warning: String) { + (self)(kind, warning) + } +} + +/// A simple [`Reporter`] that logs to tracing when the `tracing` feature is enabled. +pub struct TracingReporter; + +impl Reporter for TracingReporter { + fn report(&mut self, _kind: MarkerWarningKind, _message: String) { + #[cfg(feature = "tracing")] + { + tracing::warn!("{}", _message); + } + } +} + impl FromStr for Requirement { type Err = Pep508Error; /// Parse a [Dependency Specifier](https://packaging.python.org/en/latest/specifications/dependency-specifiers/). fn from_str(input: &str) -> Result { - parse_pep508_requirement::(&mut Cursor::new(input), None) + parse_pep508_requirement::(&mut Cursor::new(input), None, &mut TracingReporter) } } impl Requirement { /// Parse a [Dependency Specifier](https://packaging.python.org/en/latest/specifications/dependency-specifiers/). pub fn parse(input: &str, working_dir: impl AsRef) -> Result> { - parse_pep508_requirement(&mut Cursor::new(input), Some(working_dir.as_ref())) + parse_pep508_requirement( + &mut Cursor::new(input), + Some(working_dir.as_ref()), + &mut TracingReporter, + ) + } + + /// Parse a [Dependency Specifier](https://packaging.python.org/en/latest/specifications/dependency-specifiers/) + /// with the given reporter for warnings. + pub fn parse_reporter( + input: &str, + working_dir: impl AsRef, + reporter: &mut impl Reporter, + ) -> Result> { + parse_pep508_requirement( + &mut Cursor::new(input), + Some(working_dir.as_ref()), + reporter, + ) } /// Convert a requirement with one URL type into one with another URL type. @@ -925,6 +969,7 @@ fn parse_version_specifier_parentheses( fn parse_pep508_requirement( cursor: &mut Cursor, working_dir: Option<&Path>, + reporter: &mut impl Reporter, ) -> Result, Pep508Error> { let start = cursor.pos(); @@ -997,7 +1042,7 @@ fn parse_pep508_requirement( let marker = if cursor.peek_char() == Some(';') { // Skip past the semicolon cursor.next(); - Some(marker::parse_markers_cursor(cursor)?) + Some(marker::parse_markers_cursor(cursor, reporter)?) } else { None }; @@ -1078,10 +1123,10 @@ mod tests { use crate::cursor::Cursor; use crate::marker::{ - parse_markers_cursor, MarkerExpression, MarkerOperator, MarkerTree, MarkerValue, - MarkerValueString, MarkerValueVersion, + parse_markers_cursor, MarkerExpression, MarkerOperator, MarkerTree, MarkerValueString, + MarkerValueVersion, }; - use crate::{Requirement, VerbatimUrl, VersionOrUrl}; + use crate::{Requirement, TracingReporter, VerbatimUrl, VersionOrUrl}; fn parse_pep508_err(input: &str) -> String { Requirement::::from_str(input) @@ -1171,10 +1216,13 @@ mod tests { .into_iter() .collect(), )), - marker: Some(MarkerTree::Expression(MarkerExpression { - l_value: MarkerValue::MarkerEnvVersion(MarkerValueVersion::PythonVersion), - operator: MarkerOperator::LessThan, - r_value: MarkerValue::QuotedString("2.7".to_string()), + marker: Some(MarkerTree::Expression(MarkerExpression::Version { + key: MarkerValueVersion::PythonVersion, + specifier: VersionSpecifier::from_pattern( + pep440_rs::Operator::LessThan, + "2.7".parse().unwrap(), + ) + .unwrap(), })), origin: None, }; @@ -1410,31 +1458,33 @@ mod tests { #[test] fn test_marker_parsing() { let marker = r#"python_version == "2.7" and (sys_platform == "win32" or (os_name == "linux" and implementation_name == 'cpython'))"#; - let actual = parse_markers_cursor::(&mut Cursor::new(marker)).unwrap(); + let actual = + parse_markers_cursor::(&mut Cursor::new(marker), &mut TracingReporter).unwrap(); let expected = MarkerTree::And(vec![ - MarkerTree::Expression(MarkerExpression { - l_value: MarkerValue::MarkerEnvVersion(MarkerValueVersion::PythonVersion), - operator: MarkerOperator::Equal, - r_value: MarkerValue::QuotedString("2.7".to_string()), + MarkerTree::Expression(MarkerExpression::Version { + key: MarkerValueVersion::PythonVersion, + specifier: VersionSpecifier::from_pattern( + pep440_rs::Operator::Equal, + "2.7".parse().unwrap(), + ) + .unwrap(), }), MarkerTree::Or(vec![ - MarkerTree::Expression(MarkerExpression { - l_value: MarkerValue::MarkerEnvString(MarkerValueString::SysPlatform), + MarkerTree::Expression(MarkerExpression::String { + key: MarkerValueString::SysPlatform, operator: MarkerOperator::Equal, - r_value: MarkerValue::QuotedString("win32".to_string()), + value: "win32".to_string(), }), MarkerTree::And(vec![ - MarkerTree::Expression(MarkerExpression { - l_value: MarkerValue::MarkerEnvString(MarkerValueString::OsName), + MarkerTree::Expression(MarkerExpression::String { + key: MarkerValueString::OsName, operator: MarkerOperator::Equal, - r_value: MarkerValue::QuotedString("linux".to_string()), + value: "linux".to_string(), }), - MarkerTree::Expression(MarkerExpression { - l_value: MarkerValue::MarkerEnvString( - MarkerValueString::ImplementationName, - ), + MarkerTree::Expression(MarkerExpression::String { + key: MarkerValueString::ImplementationName, operator: MarkerOperator::Equal, - r_value: MarkerValue::QuotedString("cpython".to_string()), + value: "cpython".to_string(), }), ]), ]), diff --git a/crates/pep508-rs/src/marker.rs b/crates/pep508-rs/src/marker.rs index f6981c671..fb6d5e65e 100644 --- a/crates/pep508-rs/src/marker.rs +++ b/crates/pep508-rs/src/marker.rs @@ -26,7 +26,7 @@ use pep440_rs::{Version, VersionParseError, VersionPattern, VersionSpecifier}; use uv_normalize::ExtraName; use crate::cursor::Cursor; -use crate::{Pep508Error, Pep508ErrorSource, Pep508Url}; +use crate::{Pep508Error, Pep508ErrorSource, Pep508Url, Reporter, TracingReporter}; /// Ways in which marker evaluation can fail #[derive(Debug, Eq, Hash, Ord, PartialOrd, PartialEq, Clone, Copy)] @@ -899,21 +899,388 @@ impl<'a> TryFrom> for MarkerEnvironment { } } -/// Represents one clause such as `python_version > "3.8"` in the form -/// ```text -/// -/// ``` +/// Represents one clause such as `python_version > "3.8"`. #[derive(Clone, Debug, Eq, Hash, PartialEq)] -pub struct MarkerExpression { - /// A name from the PEP508 list or a string - pub l_value: MarkerValue, - /// an operator, such as `>=` or `not in` - pub operator: MarkerOperator, - /// A name from the PEP508 list or a string - pub r_value: MarkerValue, +#[allow(missing_docs)] +pub enum MarkerExpression { + /// A version expression, e.g. ` `. + Version { + key: MarkerValueVersion, + specifier: VersionSpecifier, + }, + /// An inverted version expression, e.g ` `. + VersionInverted { + /// No star allowed here, `'3.*' == python_version` is not a valid PEP 440 comparison. + version: Version, + operator: pep440_rs::Operator, + key: MarkerValueVersion, + }, + /// An string marker comparison, e.g. `sys_platform == '...'`. + String { + key: MarkerValueString, + operator: MarkerOperator, + value: String, + }, + /// An inverted string marker comparison, e.g. `'...' == sys_platform`. + StringInverted { + value: String, + operator: MarkerOperator, + key: MarkerValueString, + }, + /// `extra '...'` or `'...' extra` + Extra { + operator: ExtraOperator, + name: ExtraName, + }, + /// An invalid or meaningless expression, such as '...' == '...'. + /// + /// Invalid expressions always evaluate to `false`, and are warned for during parsing. + Arbitrary { + l_value: MarkerValue, + operator: MarkerOperator, + r_value: MarkerValue, + }, +} + +/// The operator for an extra expression, either '==' or '!='. +#[derive(Clone, Debug, Eq, Hash, PartialEq)] +pub enum ExtraOperator { + /// `==` + Equal, + /// `!=` + NotEqual, +} + +impl ExtraOperator { + fn from_marker_operator(operator: MarkerOperator) -> Option { + match operator { + MarkerOperator::Equal => Some(ExtraOperator::Equal), + MarkerOperator::NotEqual => Some(ExtraOperator::NotEqual), + _ => None, + } + } +} + +impl Display for ExtraOperator { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.write_str(match self { + Self::Equal => "==", + Self::NotEqual => "!=", + }) + } } impl MarkerExpression { + /// Parse a [`MarkerExpression`] from a string with the given reporter. + pub fn parse_reporter(s: &str, reporter: &mut impl Reporter) -> Result { + let mut chars = Cursor::new(s); + let expression = parse_marker_key_op_value(&mut chars, reporter)?; + chars.eat_whitespace(); + if let Some((pos, unexpected)) = chars.next() { + return Err(Pep508Error { + message: Pep508ErrorSource::String(format!( + "Unexpected character '{unexpected}', expected end of input" + )), + start: pos, + len: chars.remaining(), + input: chars.to_string(), + }); + } + Ok(expression) + } + + /// Convert a <`marker_value`> <`marker_op`> <`marker_value`> expression into it's + /// typed equivalent. + fn new( + l_value: MarkerValue, + operator: MarkerOperator, + r_value: MarkerValue, + reporter: &mut impl Reporter, + ) -> MarkerExpression { + match l_value { + // The only sound choice for this is ` ` + MarkerValue::MarkerEnvVersion(key) => { + let MarkerValue::QuotedString(value) = r_value else { + reporter.report( + MarkerWarningKind::Pep440Error, + format!( + "Expected double quoted PEP 440 version to compare with {}, found {}, + will evaluate to false", + key, r_value + ), + ); + + return MarkerExpression::arbitrary( + MarkerValue::MarkerEnvVersion(key), + operator, + r_value, + ); + }; + + match MarkerExpression::version(key.clone(), operator.clone(), &value, reporter) { + Some(expr) => expr, + None => MarkerExpression::arbitrary( + MarkerValue::MarkerEnvVersion(key), + operator, + MarkerValue::QuotedString(value), + ), + } + } + // The only sound choice for this is ` ` + MarkerValue::MarkerEnvString(key) => { + let value = match r_value { + MarkerValue::Extra + | MarkerValue::MarkerEnvVersion(_) + | MarkerValue::MarkerEnvString(_) => { + reporter.report( + MarkerWarningKind::MarkerMarkerComparison, + "Comparing two markers with each other doesn't make any sense, + will evaluate to false" + .to_string(), + ); + + return MarkerExpression::arbitrary( + MarkerValue::MarkerEnvString(key), + operator, + r_value, + ); + } + MarkerValue::QuotedString(r_string) => r_string, + }; + + MarkerExpression::String { + key, + operator, + value, + } + } + // `extra == '...'` + MarkerValue::Extra => { + let value = match r_value { + MarkerValue::MarkerEnvVersion(_) + | MarkerValue::MarkerEnvString(_) + | MarkerValue::Extra => { + reporter.report( + MarkerWarningKind::ExtraInvalidComparison, + "Comparing extra with something other than a quoted string is wrong, + will evaluate to false" + .to_string(), + ); + return MarkerExpression::arbitrary(l_value, operator, r_value); + } + MarkerValue::QuotedString(value) => value, + }; + + match MarkerExpression::extra(operator.clone(), &value, reporter) { + Some(expr) => expr, + None => MarkerExpression::arbitrary( + MarkerValue::Extra, + operator, + MarkerValue::QuotedString(value), + ), + } + } + // This is either MarkerEnvVersion, MarkerEnvString or Extra inverted + MarkerValue::QuotedString(l_string) => { + match r_value { + // The only sound choice for this is ` ` + MarkerValue::MarkerEnvVersion(key) => { + match MarkerExpression::version_inverted( + &l_string, + operator.clone(), + key.clone(), + reporter, + ) { + Some(expr) => expr, + None => MarkerExpression::arbitrary( + MarkerValue::QuotedString(l_string), + operator, + MarkerValue::MarkerEnvVersion(key), + ), + } + } + // '...' == + MarkerValue::MarkerEnvString(key) => MarkerExpression::StringInverted { + key, + operator, + value: l_string, + }, + // `'...' == extra` + MarkerValue::Extra => { + match MarkerExpression::extra(operator.clone(), &l_string, reporter) { + Some(expr) => expr, + None => MarkerExpression::arbitrary( + MarkerValue::QuotedString(l_string), + operator, + MarkerValue::Extra, + ), + } + } + // `'...' == '...'`, doesn't make much sense + MarkerValue::QuotedString(_) => { + // Not even pypa/packaging 22.0 supports this + // https://github.com/pypa/packaging/issues/632 + let expr = MarkerExpression::arbitrary( + MarkerValue::QuotedString(l_string), + operator, + r_value, + ); + + reporter.report(MarkerWarningKind::StringStringComparison, format!( + "Comparing two quoted strings with each other doesn't make sense: {expr}, + will evaluate to false" + )); + + expr + } + } + } + } + } + + /// Creates an instance of `MarkerExpression::Arbitrary` with the given values. + fn arbitrary( + l_value: MarkerValue, + operator: MarkerOperator, + r_value: MarkerValue, + ) -> MarkerExpression { + MarkerExpression::Arbitrary { + l_value, + operator, + r_value, + } + } + + /// Creates an instance of `MarkerExpression::Version` with the given values. + /// + /// Reports a warning on failure, and returns `None`. + pub fn version( + key: MarkerValueVersion, + marker_operator: MarkerOperator, + value: &str, + reporter: &mut impl Reporter, + ) -> Option { + let pattern = match value.parse::() { + Ok(pattern) => pattern, + Err(err) => { + reporter.report( + MarkerWarningKind::Pep440Error, + format!( + "Expected PEP 440 version to compare with {}, found {}, will evaluate to false: {}", + key, value, err + ), + ); + + return None; + } + }; + + let Some(operator) = marker_operator.to_pep440_operator() else { + reporter.report( + MarkerWarningKind::Pep440Error, + format!( + "Expected PEP 440 version operator to compare {} with '{}', + found '{}', will evaluate to false", + key, + pattern.version(), + marker_operator + ), + ); + + return None; + }; + + let specifier = match VersionSpecifier::from_pattern(operator, pattern) { + Ok(specifier) => specifier, + Err(err) => { + reporter.report( + MarkerWarningKind::Pep440Error, + format!("Invalid operator/version combination: {err}"), + ); + return None; + } + }; + + Some(MarkerExpression::Version { key, specifier }) + } + + /// Creates an instance of `MarkerExpression::VersionInverted` with the given values. + /// + /// Reports a warning on failure, and returns `None`. + fn version_inverted( + value: &str, + marker_operator: MarkerOperator, + key: MarkerValueVersion, + reporter: &mut impl Reporter, + ) -> Option { + let version = match value.parse::() { + Ok(version) => version, + Err(err) => { + reporter.report( + MarkerWarningKind::Pep440Error, + format!( + "Expected PEP 440 version to compare with {}, found {}, will evaluate to false: {}", + key, value, err + ), + ); + + return None; + } + }; + + let Some(operator) = marker_operator.to_pep440_operator() else { + reporter.report( + MarkerWarningKind::Pep440Error, + format!( + "Expected PEP 440 version operator to compare {} with '{}', + found '{}', will evaluate to false", + key, version, marker_operator + ), + ); + + return None; + }; + + Some(MarkerExpression::VersionInverted { + version, + operator, + key, + }) + } + + /// Creates an instance of `MarkerExpression::Extra` with the given values, falling back to + /// `MarkerExpression::Arbitrary` on failure. + fn extra( + operator: MarkerOperator, + value: &str, + reporter: &mut impl Reporter, + ) -> Option { + let name = match ExtraName::from_str(value) { + Ok(name) => name, + Err(err) => { + reporter.report( + MarkerWarningKind::ExtraInvalidComparison, + format!("Expected extra name, found '{value}', will evaluate to false: {err}"), + ); + + return None; + } + }; + + match ExtraOperator::from_marker_operator(operator.clone()) { + Some(operator) => Some(MarkerExpression::Extra { operator, name }), + None => { + reporter.report( + MarkerWarningKind::ExtraInvalidComparison, + "Comparing extra with something other than a quoted string is wrong, + will evaluate to false" + .to_string(), + ); + None + } + } + } + /// Evaluate a <`marker_value`> <`marker_op`> <`marker_value`> expression /// /// When `env` is `None`, all expressions that reference the environment @@ -922,169 +1289,63 @@ impl MarkerExpression { &self, env: Option<&MarkerEnvironment>, extras: &[ExtraName], - reporter: &mut impl FnMut(MarkerWarningKind, String, &Self), + reporter: &mut impl Reporter, ) -> bool { - match &self.l_value { - // The only sound choice for this is ` ` - MarkerValue::MarkerEnvVersion(l_key) => { - let value = &self.r_value; - let r_vpat = if let MarkerValue::QuotedString(r_string) = &value { - match r_string.parse::() { - Ok(vpat) => vpat, + match self { + MarkerExpression::Version { key, specifier } => env + .map(|env| specifier.contains(env.get_version(key))) + .unwrap_or(true), + MarkerExpression::VersionInverted { + key, + operator, + version, + } => env + .map(|env| { + let r_version = VersionPattern::verbatim(env.get_version(key).clone()); + let specifier = match VersionSpecifier::from_pattern(*operator, r_version) { + Ok(specifier) => specifier, Err(err) => { - reporter(MarkerWarningKind::Pep440Error, format!( - "Expected PEP 440 version to compare with {}, found {}, evaluating to false: {}", - l_key, self.r_value, err - ), self); + reporter.report( + MarkerWarningKind::Pep440Error, + format!("Invalid operator/version combination: {err}"), + ); + return false; } - } - } else { - reporter(MarkerWarningKind::Pep440Error, format!( - "Expected double quoted PEP 440 version to compare with {}, found {}, evaluating to false", - l_key, self.r_value - ), self); - return false; - }; + }; - let operator = match self.operator.to_pep440_operator() { - None => { - reporter(MarkerWarningKind::Pep440Error, format!( - "Expected PEP 440 version operator to compare {} with '{}', found '{}', evaluating to false", - l_key, r_vpat.version(), self.operator - ), self); - return false; - } - Some(operator) => operator, - }; - - let specifier = match VersionSpecifier::from_pattern(operator, r_vpat) { - Ok(specifier) => specifier, - Err(err) => { - reporter( - MarkerWarningKind::Pep440Error, - format!("Invalid operator/version combination: {err}"), - self, - ); - return false; - } - }; - env.map_or(true, |env| { - let l_version = env.get_version(l_key); - specifier.contains(l_version) + specifier.contains(version) }) - } - // This is half the same block as above inverted - MarkerValue::MarkerEnvString(l_key) => { - let r_string = match &self.r_value { - MarkerValue::Extra - | MarkerValue::MarkerEnvVersion(_) - | MarkerValue::MarkerEnvString(_) => { - reporter(MarkerWarningKind::MarkerMarkerComparison, "Comparing two markers with each other doesn't make any sense, evaluating to false".to_string(), self); - return false; - } - MarkerValue::QuotedString(r_string) => r_string, - }; - - env.map_or(true, |env| { - let l_string = env.get_string(l_key); - self.compare_strings(l_string, r_string, reporter) + .unwrap_or(true), + MarkerExpression::String { + key, + operator, + value, + } => env + .map(|env| { + let l_string = env.get_string(key); + self.compare_strings(l_string, operator, value, reporter) }) - } - // `extra == '...'` - MarkerValue::Extra => { - let r_value_string = match &self.r_value { - MarkerValue::MarkerEnvVersion(_) - | MarkerValue::MarkerEnvString(_) - | MarkerValue::Extra => { - reporter(MarkerWarningKind::ExtraInvalidComparison, "Comparing extra with something other than a quoted string is wrong, evaluating to false".to_string(), self); - return false; - } - MarkerValue::QuotedString(r_value_string) => r_value_string, - }; - match ExtraName::from_str(r_value_string) { - Ok(r_extra) => extras.contains(&r_extra), - Err(err) => { - reporter(MarkerWarningKind::ExtraInvalidComparison, format!( - "Expected extra name, found '{r_value_string}', evaluating to false: {err}" - ), self); - false - } - } - } - // This is either MarkerEnvVersion, MarkerEnvString or Extra inverted - MarkerValue::QuotedString(l_string) => { - match &self.r_value { - // The only sound choice for this is ` ` - MarkerValue::MarkerEnvVersion(r_key) => { - let Some(env) = env else { return true }; - - let l_version = match Version::from_str(l_string) { - Ok(l_version) => l_version, - Err(err) => { - reporter(MarkerWarningKind::Pep440Error, format!( - "Expected double quoted PEP 440 version to compare with {}, found {}, evaluating to false: {}", - l_string, self.r_value, err - ), self); - return false; - } - }; - let r_version = env.get_version(r_key); - - let operator = match self.operator.to_pep440_operator() { - None => { - reporter(MarkerWarningKind::Pep440Error, format!( - "Expected PEP 440 version operator to compare '{}' with {}, found '{}', evaluating to false", - l_string, r_key, self.operator - ), self); - return false; - } - Some(operator) => operator, - }; - - let specifier = match VersionSpecifier::from_pattern( - operator, - VersionPattern::verbatim(r_version.clone()), - ) { - Ok(specifier) => specifier, - Err(err) => { - reporter( - MarkerWarningKind::Pep440Error, - format!("Invalid operator/version combination: {err}"), - self, - ); - return false; - } - }; - - specifier.contains(&l_version) - } - // This is half the same block as above inverted - MarkerValue::MarkerEnvString(r_key) => env.map_or(true, |env| { - let r_string = env.get_string(r_key); - self.compare_strings(l_string, r_string, reporter) - }), - // `'...' == extra` - MarkerValue::Extra => match ExtraName::from_str(l_string) { - Ok(l_extra) => self.marker_compare(&l_extra, extras, reporter), - Err(err) => { - reporter(MarkerWarningKind::ExtraInvalidComparison, format!( - "Expected extra name, found '{l_string}', evaluating to false: {err}" - ), self); - false - } - }, - // `'...' == '...'`, doesn't make much sense - MarkerValue::QuotedString(_) => { - // Not even pypa/packaging 22.0 supports this - // https://github.com/pypa/packaging/issues/632 - reporter(MarkerWarningKind::StringStringComparison, format!( - "Comparing two quoted strings with each other doesn't make sense: {self}, evaluating to false" - ), self); - false - } - } - } + .unwrap_or(true), + MarkerExpression::StringInverted { + key, + operator, + value, + } => env + .map(|env| { + let r_string = env.get_string(key); + self.compare_strings(value, operator, r_string, reporter) + }) + .unwrap_or(true), + MarkerExpression::Extra { + operator: ExtraOperator::Equal, + name, + } => extras.contains(name), + MarkerExpression::Extra { + operator: ExtraOperator::NotEqual, + name, + } => !extras.contains(name), + MarkerExpression::Arbitrary { .. } => true, } } @@ -1124,71 +1385,39 @@ impl MarkerExpression { extras: &HashSet, python_versions: &[Version], ) -> bool { - match (&self.l_value, &self.operator, &self.r_value) { - // `extra == '...'` - (MarkerValue::Extra, MarkerOperator::Equal, MarkerValue::QuotedString(r_string)) => { - ExtraName::from_str(r_string).is_ok_and(|r_extra| extras.contains(&r_extra)) - } - // `'...' == extra` - (MarkerValue::QuotedString(l_string), MarkerOperator::Equal, MarkerValue::Extra) => { - ExtraName::from_str(l_string).is_ok_and(|l_extra| extras.contains(&l_extra)) - } - // `extra != '...'` - (MarkerValue::Extra, MarkerOperator::NotEqual, MarkerValue::QuotedString(r_string)) => { - ExtraName::from_str(r_string).is_ok_and(|r_extra| !extras.contains(&r_extra)) - } - // `'...' != extra` - (MarkerValue::QuotedString(l_string), MarkerOperator::NotEqual, MarkerValue::Extra) => { - ExtraName::from_str(l_string).is_ok_and(|l_extra| !extras.contains(&l_extra)) - } - ( - MarkerValue::MarkerEnvVersion(MarkerValueVersion::PythonVersion), + match self { + MarkerExpression::Version { + key: MarkerValueVersion::PythonVersion, + specifier, + } => python_versions + .iter() + .any(|l_version| specifier.contains(l_version)), + MarkerExpression::VersionInverted { + key: MarkerValueVersion::PythonVersion, operator, - MarkerValue::QuotedString(r_string), - ) => { - // ignore all errors block - (|| { - // The right hand side is allowed to contain a star, e.g. `python_version == '3.*'` - let r_vpat = r_string.parse::().ok()?; - let operator = operator.to_pep440_operator()?; - // operator and right hand side make the specifier - let specifier = VersionSpecifier::from_pattern(operator, r_vpat).ok()?; + version, + } => { + python_versions.iter().any(|r_version| { + // operator and right hand side make the specifier and in this case the + // right hand is `python_version` so changes every iteration + let Ok(specifier) = VersionSpecifier::from_pattern( + *operator, + VersionPattern::verbatim(r_version.clone()), + ) else { + return true; + }; - let compatible = python_versions - .iter() - .any(|l_version| specifier.contains(l_version)); - Some(compatible) - })() - .unwrap_or(true) - } - ( - MarkerValue::QuotedString(l_string), - operator, - MarkerValue::MarkerEnvVersion(MarkerValueVersion::PythonVersion), - ) => { - // ignore all errors block - (|| { - // Not star allowed here, `'3.*' == python_version` is not a valid PEP 440 - // comparison - let l_version = Version::from_str(l_string).ok()?; - let operator = operator.to_pep440_operator()?; - - let compatible = python_versions.iter().any(|r_version| { - // operator and right hand side make the specifier and in this case the - // right hand is `python_version` so changes every iteration - match VersionSpecifier::from_pattern( - operator, - VersionPattern::verbatim(r_version.clone()), - ) { - Ok(specifier) => specifier.contains(&l_version), - Err(_) => true, - } - }); - - Some(compatible) - })() - .unwrap_or(true) + specifier.contains(version) + }) } + MarkerExpression::Extra { + operator: ExtraOperator::Equal, + name, + } => extras.contains(name), + MarkerExpression::Extra { + operator: ExtraOperator::NotEqual, + name, + } => !extras.contains(name), _ => true, } } @@ -1197,49 +1426,45 @@ impl MarkerExpression { fn compare_strings( &self, l_string: &str, + operator: &MarkerOperator, r_string: &str, - reporter: &mut impl FnMut(MarkerWarningKind, String, &Self), + reporter: &mut impl Reporter, ) -> bool { - match self.operator { + match operator { MarkerOperator::Equal => l_string == r_string, MarkerOperator::NotEqual => l_string != r_string, MarkerOperator::GreaterThan => { - reporter( + reporter.report( MarkerWarningKind::LexicographicComparison, format!("Comparing {l_string} and {r_string} lexicographically"), - self, ); l_string > r_string } MarkerOperator::GreaterEqual => { - reporter( + reporter.report( MarkerWarningKind::LexicographicComparison, format!("Comparing {l_string} and {r_string} lexicographically"), - self, ); l_string >= r_string } MarkerOperator::LessThan => { - reporter( + reporter.report( MarkerWarningKind::LexicographicComparison, format!("Comparing {l_string} and {r_string} lexicographically"), - self, ); l_string < r_string } MarkerOperator::LessEqual => { - reporter( + reporter.report( MarkerWarningKind::LexicographicComparison, format!("Comparing {l_string} and {r_string} lexicographically"), - self, ); l_string <= r_string } MarkerOperator::TildeEqual => { - reporter( + reporter.report( MarkerWarningKind::LexicographicComparison, format!("Can't compare {l_string} and {r_string} with `~=`"), - self, ); false } @@ -1247,49 +1472,59 @@ impl MarkerExpression { MarkerOperator::NotIn => !r_string.contains(l_string), } } - - // The `marker '...'` comparison - fn marker_compare( - &self, - value: &ExtraName, - extras: &[ExtraName], - reporter: &mut impl FnMut(MarkerWarningKind, String, &Self), - ) -> bool { - match self.operator { - MarkerOperator::Equal => extras.contains(value), - MarkerOperator::NotEqual => !extras.contains(value), - _ => { - reporter(MarkerWarningKind::ExtraInvalidComparison, "Comparing extra with something other than equal (`==`) or unequal (`!=`) is wrong, evaluating to false".to_string(), self); - false - } - } - } } impl FromStr for MarkerExpression { type Err = Pep508Error; fn from_str(s: &str) -> Result { - let mut chars = Cursor::new(s); - let expression = parse_marker_key_op_value(&mut chars)?; - chars.eat_whitespace(); - if let Some((pos, unexpected)) = chars.next() { - return Err(Pep508Error { - message: Pep508ErrorSource::String(format!( - "Unexpected character '{unexpected}', expected end of input" - )), - start: pos, - len: chars.remaining(), - input: chars.to_string(), - }); - } - Ok(expression) + MarkerExpression::parse_reporter(s, &mut TracingReporter) } } impl Display for MarkerExpression { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - write!(f, "{} {} {}", self.l_value, self.operator, self.r_value) + match self { + MarkerExpression::Version { key, specifier } => { + write!( + f, + "{key} {} '{}'", + specifier.operator(), + specifier.version() + ) + } + MarkerExpression::VersionInverted { + version, + operator, + key, + } => { + write!(f, "'{version}' {operator} {key}") + } + MarkerExpression::String { + key, + operator, + value, + } => { + write!(f, "{key} {operator} '{value}'") + } + MarkerExpression::StringInverted { + value, + operator, + key, + } => { + write!(f, "'{value}' {operator} {key}") + } + MarkerExpression::Extra { operator, name } => { + write!(f, "extra {operator} '{name}'") + } + MarkerExpression::Arbitrary { + l_value, + operator, + r_value, + } => { + write!(f, "{l_value} {operator} {r_value}") + } + } } } @@ -1310,11 +1545,19 @@ impl FromStr for MarkerTree { type Err = Pep508Error; fn from_str(markers: &str) -> Result { - parse_markers(markers) + parse_markers(markers, &mut TracingReporter) } } impl MarkerTree { + /// Parse a [`MarkerTree`] from a string with the given reporter. + pub fn parse_reporter( + markers: &str, + reporter: &mut impl Reporter, + ) -> Result { + parse_markers(markers, reporter) + } + /// Does this marker apply in the given environment? pub fn evaluate(&self, env: &MarkerEnvironment, extras: &[ExtraName]) -> bool { self.evaluate_optional_environment(Some(env), extras) @@ -1332,21 +1575,15 @@ impl MarkerTree { env: Option<&MarkerEnvironment>, extras: &[ExtraName], ) -> bool { - let mut reporter = |_kind, _message, _marker_expression: &MarkerExpression| { - #[cfg(feature = "tracing")] - { - tracing::warn!("{}", _message); - } - }; - self.report_deprecated_options(&mut reporter); + self.report_deprecated_options(&mut TracingReporter); match self { - Self::Expression(expression) => expression.evaluate(env, extras, &mut reporter), + Self::Expression(expression) => expression.evaluate(env, extras, &mut TracingReporter), Self::And(expressions) => expressions .iter() - .all(|x| x.evaluate_reporter_impl(env, extras, &mut reporter)), + .all(|x| x.evaluate_reporter_impl(env, extras, &mut TracingReporter)), Self::Or(expressions) => expressions .iter() - .any(|x| x.evaluate_reporter_impl(env, extras, &mut reporter)), + .any(|x| x.evaluate_reporter_impl(env, extras, &mut TracingReporter)), } } @@ -1361,23 +1598,17 @@ impl MarkerTree { pub fn simplify_extras(self, extras: &[ExtraName]) -> Option { /// Returns `true` if the given expression is always `true` given the set of extras. pub fn is_true(expression: &MarkerExpression, extras: &[ExtraName]) -> bool { - // Ex) `extra == 'dev'` - if expression.l_value == MarkerValue::Extra { - if let MarkerValue::QuotedString(r_string) = &expression.r_value { - if let Ok(r_extra) = ExtraName::from_str(r_string) { - return extras.contains(&r_extra); - } - } + match expression { + MarkerExpression::Extra { + operator: ExtraOperator::Equal, + name, + } => extras.contains(name), + MarkerExpression::Extra { + operator: ExtraOperator::NotEqual, + name, + } => !extras.contains(name), + _ => false, } - // Ex) `'dev' == extra` - if expression.r_value == MarkerValue::Extra { - if let MarkerValue::QuotedString(l_string) = &expression.l_value { - if let Ok(l_extra) = ExtraName::from_str(l_string) { - return extras.contains(&l_extra); - } - } - } - false } match self { @@ -1438,7 +1669,7 @@ impl MarkerTree { &self, env: &MarkerEnvironment, extras: &[ExtraName], - reporter: &mut impl FnMut(MarkerWarningKind, String, &MarkerExpression), + reporter: &mut impl Reporter, ) -> bool { self.report_deprecated_options(reporter); self.evaluate_reporter_impl(Some(env), extras, reporter) @@ -1448,7 +1679,7 @@ impl MarkerTree { &self, env: Option<&MarkerEnvironment>, extras: &[ExtraName], - reporter: &mut impl FnMut(MarkerWarningKind, String, &MarkerExpression), + reporter: &mut impl Reporter, ) -> bool { match self { Self::Expression(expression) => expression.evaluate(env, extras, reporter), @@ -1493,10 +1724,10 @@ impl MarkerTree { &self, env: &MarkerEnvironment, extras: &[ExtraName], - ) -> (bool, Vec<(MarkerWarningKind, String, String)>) { + ) -> (bool, Vec<(MarkerWarningKind, String)>) { let mut warnings = Vec::new(); - let mut reporter = |kind, warning, marker: &MarkerExpression| { - warnings.push((kind, warning, marker.to_string())); + let mut reporter = |kind, warning| { + warnings.push((kind, warning)); }; self.report_deprecated_options(&mut reporter); let result = self.evaluate_reporter_impl(Some(env), extras, &mut reporter); @@ -1504,68 +1735,53 @@ impl MarkerTree { } /// Report the deprecated marker from - fn report_deprecated_options( - &self, - reporter: &mut impl FnMut(MarkerWarningKind, String, &MarkerExpression), - ) { + fn report_deprecated_options(&self, reporter: &mut impl Reporter) { match self { Self::Expression(expression) => { - for value in [&expression.l_value, &expression.r_value] { - match value { - MarkerValue::MarkerEnvString(MarkerValueString::OsNameDeprecated) => { - reporter( - MarkerWarningKind::DeprecatedMarkerName, - "os.name is deprecated in favor of os_name".to_string(), - expression, - ); - } - MarkerValue::MarkerEnvString( - MarkerValueString::PlatformMachineDeprecated, - ) => { - reporter( - MarkerWarningKind::DeprecatedMarkerName, - "platform.machine is deprecated in favor of platform_machine" - .to_string(), - expression, - ); - } - MarkerValue::MarkerEnvString( - MarkerValueString::PlatformPythonImplementationDeprecated, - ) => { - reporter( + let MarkerExpression::String { key, .. } = expression else { + return; + }; + + match key { + MarkerValueString::OsNameDeprecated => { + reporter.report( + MarkerWarningKind::DeprecatedMarkerName, + "os.name is deprecated in favor of os_name".to_string(), + ); + } + MarkerValueString::PlatformMachineDeprecated => { + reporter.report( + MarkerWarningKind::DeprecatedMarkerName, + "platform.machine is deprecated in favor of platform_machine" + .to_string(), + ); + } + MarkerValueString::PlatformPythonImplementationDeprecated => { + reporter.report( MarkerWarningKind::DeprecatedMarkerName, "platform.python_implementation is deprecated in favor of platform_python_implementation".to_string(), - expression, ); - } - MarkerValue::MarkerEnvString( - MarkerValueString::PythonImplementationDeprecated, - ) => { - reporter( + } + MarkerValueString::PythonImplementationDeprecated => { + reporter.report( MarkerWarningKind::DeprecatedMarkerName, "python_implementation is deprecated in favor of platform_python_implementation".to_string(), - expression, ); - } - MarkerValue::MarkerEnvString( - MarkerValueString::PlatformVersionDeprecated, - ) => { - reporter( - MarkerWarningKind::DeprecatedMarkerName, - "platform.version is deprecated in favor of platform_version" - .to_string(), - expression, - ); - } - MarkerValue::MarkerEnvString(MarkerValueString::SysPlatformDeprecated) => { - reporter( - MarkerWarningKind::DeprecatedMarkerName, - "sys.platform is deprecated in favor of sys_platform".to_string(), - expression, - ); - } - _ => {} } + MarkerValueString::PlatformVersionDeprecated => { + reporter.report( + MarkerWarningKind::DeprecatedMarkerName, + "platform.version is deprecated in favor of platform_version" + .to_string(), + ); + } + MarkerValueString::SysPlatformDeprecated => { + reporter.report( + MarkerWarningKind::DeprecatedMarkerName, + "sys.platform is deprecated in favor of sys_platform".to_string(), + ); + } + _ => {} } } Self::And(expressions) => { @@ -1717,6 +1933,7 @@ fn parse_marker_value(cursor: &mut Cursor) -> Result( cursor: &mut Cursor, + reporter: &mut impl Reporter, ) -> Result> { cursor.eat_whitespace(); let lvalue = parse_marker_value(cursor)?; @@ -1727,25 +1944,27 @@ fn parse_marker_key_op_value( let operator = parse_marker_operator(cursor)?; cursor.eat_whitespace(); let rvalue = parse_marker_value(cursor)?; - Ok(MarkerExpression { - l_value: lvalue, - operator, - r_value: rvalue, - }) + + Ok(MarkerExpression::new(lvalue, operator, rvalue, reporter)) } /// ```text /// marker_expr = marker_var:l marker_op:o marker_var:r -> (o, l, r) /// | wsp* '(' marker:m wsp* ')' -> m /// ``` -fn parse_marker_expr(cursor: &mut Cursor) -> Result> { +fn parse_marker_expr( + cursor: &mut Cursor, + reporter: &mut impl Reporter, +) -> Result> { cursor.eat_whitespace(); if let Some(start_pos) = cursor.eat_char('(') { - let marker = parse_marker_or(cursor)?; + let marker = parse_marker_or(cursor, reporter)?; cursor.next_expect_char(')', start_pos)?; Ok(marker) } else { - Ok(MarkerTree::Expression(parse_marker_key_op_value(cursor)?)) + Ok(MarkerTree::Expression(parse_marker_key_op_value( + cursor, reporter, + )?)) } } @@ -1753,27 +1972,34 @@ fn parse_marker_expr(cursor: &mut Cursor) -> Result ('and', l, r) /// | marker_expr:m -> m /// ``` -fn parse_marker_and(cursor: &mut Cursor) -> Result> { - parse_marker_op(cursor, "and", MarkerTree::And, parse_marker_expr) +fn parse_marker_and( + cursor: &mut Cursor, + reporter: &mut impl Reporter, +) -> Result> { + parse_marker_op(cursor, "and", MarkerTree::And, parse_marker_expr, reporter) } /// ```text /// marker_or = marker_and:l wsp* 'or' marker_and:r -> ('or', l, r) /// | marker_and:m -> m /// ``` -fn parse_marker_or(cursor: &mut Cursor) -> Result> { - parse_marker_op(cursor, "or", MarkerTree::Or, parse_marker_and) +fn parse_marker_or( + cursor: &mut Cursor, + reporter: &mut impl Reporter, +) -> Result> { + parse_marker_op(cursor, "or", MarkerTree::Or, parse_marker_and, reporter) } /// Parses both `marker_and` and `marker_or` -fn parse_marker_op( +fn parse_marker_op( cursor: &mut Cursor, op: &str, op_constructor: fn(Vec) -> MarkerTree, - parse_inner: fn(&mut Cursor) -> Result>, + parse_inner: fn(&mut Cursor, &mut R) -> Result>, + reporter: &mut R, ) -> Result> { // marker_and or marker_expr - let first_element = parse_inner(cursor)?; + let first_element = parse_inner(cursor, reporter)?; // wsp* cursor.eat_whitespace(); // Check if we're done here instead of invoking the whole vec allocating loop @@ -1791,7 +2017,7 @@ fn parse_marker_op( match cursor.slice(start, len) { value if value == op => { cursor.take_while(|c| !c.is_whitespace()); - let expression = parse_inner(cursor)?; + let expression = parse_inner(cursor, reporter)?; expressions.push(expression); } _ => { @@ -1811,8 +2037,9 @@ fn parse_marker_op( /// ``` pub(crate) fn parse_markers_cursor( cursor: &mut Cursor, + reporter: &mut impl Reporter, ) -> Result> { - let marker = parse_marker_or(cursor)?; + let marker = parse_marker_or(cursor, reporter)?; cursor.eat_whitespace(); if let Some((pos, unexpected)) = cursor.next() { // If we're here, both parse_marker_or and parse_marker_and returned because the next @@ -1831,9 +2058,12 @@ pub(crate) fn parse_markers_cursor( /// Parses markers such as `python_version < '3.8'` or /// `python_version == "3.10" and (sys_platform == "win32" or (os_name == "linux" and implementation_name == 'cpython'))` -fn parse_markers(markers: &str) -> Result> { +fn parse_markers( + markers: &str, + reporter: &mut impl Reporter, +) -> Result> { let mut chars = Cursor::new(markers); - parse_markers_cursor(&mut chars) + parse_markers_cursor(&mut chars, reporter) } #[cfg(test)] @@ -1842,12 +2072,12 @@ mod test { use insta::assert_snapshot; + use pep440_rs::VersionSpecifier; use uv_normalize::ExtraName; - use crate::marker::{MarkerEnvironment, MarkerEnvironmentBuilder}; + use crate::marker::{ExtraOperator, MarkerEnvironment, MarkerEnvironmentBuilder}; use crate::{ - MarkerExpression, MarkerOperator, MarkerTree, MarkerValue, MarkerValueString, - MarkerValueVersion, + MarkerExpression, MarkerOperator, MarkerTree, MarkerValueString, MarkerValueVersion, }; fn parse_err(input: &str) -> String { @@ -1951,7 +2181,7 @@ mod test { testing_logger::validate(|captured_logs| { assert_eq!( captured_logs[0].body, - "Comparing two markers with each other doesn't make any sense, evaluating to false" + "Comparing two markers with each other doesn't make any sense, will evaluate to false" ); assert_eq!(captured_logs[0].level, log::Level::Warn); assert_eq!(captured_logs.len(), 1); @@ -1962,7 +2192,7 @@ mod test { assert_eq!( captured_logs[0].body, "Expected PEP 440 version to compare with python_version, found '3.9.', \ - evaluating to false: after parsing '3.9', found '.', which is \ + will evaluate to false: after parsing '3.9', found '.', which is \ not part of a valid version" ); assert_eq!(captured_logs[0].level, log::Level::Warn); @@ -1973,7 +2203,7 @@ mod test { testing_logger::validate(|captured_logs| { assert_eq!( captured_logs[0].body, - "Comparing two quoted strings with each other doesn't make sense: 'b' >= 'a', evaluating to false" + "Comparing two quoted strings with each other doesn't make sense: 'b' >= 'a', will evaluate to false" ); assert_eq!(captured_logs[0].level, log::Level::Warn); assert_eq!(captured_logs.len(), 1); @@ -2004,6 +2234,38 @@ mod test { MarkerTree::from_str("'posix' not in os_name").unwrap(); } + #[test] + fn test_marker_version_inverted() { + let env37 = env37(); + let (result, warnings) = MarkerTree::from_str("python_version > '3.6'") + .unwrap() + .evaluate_collect_warnings(&env37, &[]); + assert_eq!(warnings, &[]); + assert!(result); + + let (result, warnings) = MarkerTree::from_str("'3.6' > python_version") + .unwrap() + .evaluate_collect_warnings(&env37, &[]); + assert_eq!(warnings, &[]); + assert!(!result); + } + + #[test] + fn test_marker_string_inverted() { + let env37 = env37(); + let (result, warnings) = MarkerTree::from_str("'nux' in sys_platform") + .unwrap() + .evaluate_collect_warnings(&env37, &[]); + assert_eq!(warnings, &[]); + assert!(result); + + let (result, warnings) = MarkerTree::from_str("sys_platform in 'nux'") + .unwrap() + .evaluate_collect_warnings(&env37, &[]); + assert_eq!(warnings, &[]); + assert!(!result); + } + #[test] fn test_marker_version_star() { let env37 = env37(); @@ -2051,14 +2313,44 @@ mod test { fn test_marker_expression() { assert_eq!( MarkerExpression::from_str(r#"os_name == "nt""#).unwrap(), - MarkerExpression { - l_value: MarkerValue::MarkerEnvString(MarkerValueString::OsName), + MarkerExpression::String { + key: MarkerValueString::OsName, operator: MarkerOperator::Equal, - r_value: MarkerValue::QuotedString("nt".to_string()), + value: "nt".to_string(), } ); } + #[test] + fn test_marker_expression_inverted() { + assert_eq!( + MarkerTree::from_str( + r#""nt" in os_name and '3.7' >= python_version and python_full_version >= '3.7'"# + ) + .unwrap(), + MarkerTree::And(vec![ + MarkerTree::Expression(MarkerExpression::StringInverted { + value: "nt".to_string(), + operator: MarkerOperator::In, + key: MarkerValueString::OsName, + }), + MarkerTree::Expression(MarkerExpression::VersionInverted { + key: MarkerValueVersion::PythonVersion, + operator: pep440_rs::Operator::GreaterThanEqual, + version: "3.7".parse().unwrap(), + }), + MarkerTree::Expression(MarkerExpression::Version { + key: MarkerValueVersion::PythonFullVersion, + specifier: VersionSpecifier::from_pattern( + pep440_rs::Operator::GreaterThanEqual, + "3.7".parse().unwrap() + ) + .unwrap() + }), + ]) + ); + } + #[test] fn test_marker_expression_to_long() { let err = MarkerExpression::from_str(r#"os_name == "nt" and python_version >= "3.8""#) @@ -2100,10 +2392,10 @@ mod test { let simplified = markers.simplify_extras(&[ExtraName::from_str("dev").unwrap()]); assert_eq!( simplified, - Some(MarkerTree::Expression(MarkerExpression { - l_value: MarkerValue::MarkerEnvString(MarkerValueString::OsName), + Some(MarkerTree::Expression(MarkerExpression::String { + key: MarkerValueString::OsName, operator: MarkerOperator::Equal, - r_value: MarkerValue::QuotedString("nt".to_string()), + value: "nt".to_string(), })) ); @@ -2122,10 +2414,9 @@ mod test { let simplified = markers.simplify_extras(&[ExtraName::from_str("dev").unwrap()]); assert_eq!( simplified, - Some(MarkerTree::Expression(MarkerExpression { - l_value: MarkerValue::Extra, - operator: MarkerOperator::Equal, - r_value: MarkerValue::QuotedString("test".to_string()), + Some(MarkerTree::Expression(MarkerExpression::Extra { + operator: ExtraOperator::Equal, + name: ExtraName::from_str("test").unwrap(), })) ); @@ -2135,15 +2426,14 @@ mod test { assert_eq!( simplified, Some(MarkerTree::And(vec![ - MarkerTree::Expression(MarkerExpression { - l_value: MarkerValue::MarkerEnvString(MarkerValueString::OsName), + MarkerTree::Expression(MarkerExpression::String { + key: MarkerValueString::OsName, operator: MarkerOperator::Equal, - r_value: MarkerValue::QuotedString("nt".to_string()), + value: "nt".to_string(), }), - MarkerTree::Expression(MarkerExpression { - l_value: MarkerValue::Extra, - operator: MarkerOperator::Equal, - r_value: MarkerValue::QuotedString("test".to_string()), + MarkerTree::Expression(MarkerExpression::Extra { + operator: ExtraOperator::Equal, + name: ExtraName::from_str("test").unwrap(), }), ])) ); @@ -2157,10 +2447,10 @@ mod test { let simplified = markers.simplify_extras(&[ExtraName::from_str("dev").unwrap()]); assert_eq!( simplified, - Some(MarkerTree::Expression(MarkerExpression { - l_value: MarkerValue::MarkerEnvString(MarkerValueString::OsName), + Some(MarkerTree::Expression(MarkerExpression::String { + key: MarkerValueString::OsName, operator: MarkerOperator::Equal, - r_value: MarkerValue::QuotedString("nt".to_string()), + value: "nt".to_string(), })) ); @@ -2174,15 +2464,18 @@ mod test { assert_eq!( simplified, Some(MarkerTree::Or(vec![ - MarkerTree::Expression(MarkerExpression { - l_value: MarkerValue::MarkerEnvString(MarkerValueString::OsName), + MarkerTree::Expression(MarkerExpression::String { + key: MarkerValueString::OsName, operator: MarkerOperator::Equal, - r_value: MarkerValue::QuotedString("nt".to_string()), + value: "nt".to_string(), }), - MarkerTree::Expression(MarkerExpression { - l_value: MarkerValue::MarkerEnvVersion(MarkerValueVersion::PythonVersion), - operator: MarkerOperator::Equal, - r_value: MarkerValue::QuotedString("3.7".to_string()), + MarkerTree::Expression(MarkerExpression::Version { + key: MarkerValueVersion::PythonVersion, + specifier: VersionSpecifier::from_pattern( + pep440_rs::Operator::Equal, + "3.7".parse().unwrap() + ) + .unwrap(), }), ])) ); diff --git a/crates/pep508-rs/src/unnamed.rs b/crates/pep508-rs/src/unnamed.rs index 553919dff..6390cf8df 100644 --- a/crates/pep508-rs/src/unnamed.rs +++ b/crates/pep508-rs/src/unnamed.rs @@ -10,8 +10,8 @@ use uv_normalize::ExtraName; use crate::marker::parse_markers_cursor; use crate::{ expand_env_vars, parse_extras_cursor, split_extras, split_scheme, strip_host, Cursor, - MarkerEnvironment, MarkerTree, Pep508Error, Pep508ErrorSource, RequirementOrigin, Scheme, - VerbatimUrl, VerbatimUrlError, + MarkerEnvironment, MarkerTree, Pep508Error, Pep508ErrorSource, Reporter, RequirementOrigin, + Scheme, TracingReporter, VerbatimUrl, VerbatimUrlError, }; /// A PEP 508-like, direct URL dependency specifier without a package name. @@ -110,7 +110,7 @@ impl FromStr for UnnamedRequirement { /// Parse a PEP 508-like direct URL requirement without a package name. fn from_str(input: &str) -> Result { - parse_unnamed_requirement(&mut Cursor::new(input), None) + parse_unnamed_requirement(&mut Cursor::new(input), None, &mut TracingReporter) } } @@ -119,8 +119,13 @@ impl UnnamedRequirement { pub fn parse( input: &str, working_dir: impl AsRef, + reporter: &mut impl Reporter, ) -> Result> { - parse_unnamed_requirement(&mut Cursor::new(input), Some(working_dir.as_ref())) + parse_unnamed_requirement( + &mut Cursor::new(input), + Some(working_dir.as_ref()), + reporter, + ) } } @@ -130,6 +135,7 @@ impl UnnamedRequirement { fn parse_unnamed_requirement( cursor: &mut Cursor, working_dir: Option<&Path>, + reporter: &mut impl Reporter, ) -> Result> { cursor.eat_whitespace(); @@ -143,7 +149,7 @@ fn parse_unnamed_requirement( let marker = if cursor.peek_char() == Some(';') { // Skip past the semicolon cursor.next(); - Some(parse_markers_cursor(cursor)?) + Some(parse_markers_cursor(cursor, reporter)?) } else { None }; diff --git a/crates/requirements-txt/src/requirement.rs b/crates/requirements-txt/src/requirement.rs index a149a9a9b..094e9e126 100644 --- a/crates/requirements-txt/src/requirement.rs +++ b/crates/requirements-txt/src/requirement.rs @@ -3,7 +3,9 @@ use std::path::Path; use thiserror::Error; use distribution_types::ParsedUrlError; -use pep508_rs::{Pep508Error, Pep508ErrorSource, RequirementOrigin, UnnamedRequirement}; +use pep508_rs::{ + Pep508Error, Pep508ErrorSource, RequirementOrigin, TracingReporter, UnnamedRequirement, +}; /// A requirement specifier in a `requirements.txt` file. /// @@ -52,6 +54,7 @@ impl RequirementsTxtRequirement { Ok(Self::Unnamed(UnnamedRequirement::parse( input, &working_dir, + &mut TracingReporter, )?)) } _ => Err(RequirementsTxtRequirementError::Pep508(err)), diff --git a/crates/requirements-txt/src/snapshots/requirements_txt__test__line-endings-poetry-with-hashes.txt.snap b/crates/requirements-txt/src/snapshots/requirements_txt__test__line-endings-poetry-with-hashes.txt.snap index e6c891fb3..1857cd8ed 100644 --- a/crates/requirements-txt/src/snapshots/requirements_txt__test__line-endings-poetry-with-hashes.txt.snap +++ b/crates/requirements-txt/src/snapshots/requirements_txt__test__line-endings-poetry-with-hashes.txt.snap @@ -27,25 +27,21 @@ RequirementsTxt { And( [ Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: GreaterEqual, - r_value: QuotedString( - "3.8", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: GreaterThanEqual, + version: "3.8", + }, }, ), Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: LessThan, - r_value: QuotedString( - "4.0", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: LessThan, + version: "4.0", + }, }, ), ], @@ -85,25 +81,21 @@ RequirementsTxt { And( [ Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: GreaterEqual, - r_value: QuotedString( - "3.8", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: GreaterThanEqual, + version: "3.8", + }, }, ), Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: LessThan, - r_value: QuotedString( - "4", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: LessThan, + version: "4", + }, }, ), ], @@ -143,36 +135,28 @@ RequirementsTxt { And( [ Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: GreaterEqual, - r_value: QuotedString( - "3.8", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: GreaterThanEqual, + version: "3.8", + }, }, ), Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: LessThan, - r_value: QuotedString( - "4", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: LessThan, + version: "4", + }, }, ), Expression( - MarkerExpression { - l_value: MarkerEnvString( - PlatformSystem, - ), + String { + key: PlatformSystem, operator: Equal, - r_value: QuotedString( - "Windows", - ), + value: "Windows", }, ), ], @@ -212,25 +196,21 @@ RequirementsTxt { And( [ Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: GreaterEqual, - r_value: QuotedString( - "3.8", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: GreaterThanEqual, + version: "3.8", + }, }, ), Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: LessThan, - r_value: QuotedString( - "4.0", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: LessThan, + version: "4.0", + }, }, ), ], @@ -271,25 +251,21 @@ RequirementsTxt { And( [ Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: GreaterEqual, - r_value: QuotedString( - "3.8", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: GreaterThanEqual, + version: "3.8", + }, }, ), Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: LessThan, - r_value: QuotedString( - "4.0", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: LessThan, + version: "4.0", + }, }, ), ], diff --git a/crates/requirements-txt/src/snapshots/requirements_txt__test__parse-poetry-with-hashes.txt.snap b/crates/requirements-txt/src/snapshots/requirements_txt__test__parse-poetry-with-hashes.txt.snap index e6c891fb3..1857cd8ed 100644 --- a/crates/requirements-txt/src/snapshots/requirements_txt__test__parse-poetry-with-hashes.txt.snap +++ b/crates/requirements-txt/src/snapshots/requirements_txt__test__parse-poetry-with-hashes.txt.snap @@ -27,25 +27,21 @@ RequirementsTxt { And( [ Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: GreaterEqual, - r_value: QuotedString( - "3.8", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: GreaterThanEqual, + version: "3.8", + }, }, ), Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: LessThan, - r_value: QuotedString( - "4.0", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: LessThan, + version: "4.0", + }, }, ), ], @@ -85,25 +81,21 @@ RequirementsTxt { And( [ Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: GreaterEqual, - r_value: QuotedString( - "3.8", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: GreaterThanEqual, + version: "3.8", + }, }, ), Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: LessThan, - r_value: QuotedString( - "4", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: LessThan, + version: "4", + }, }, ), ], @@ -143,36 +135,28 @@ RequirementsTxt { And( [ Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: GreaterEqual, - r_value: QuotedString( - "3.8", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: GreaterThanEqual, + version: "3.8", + }, }, ), Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: LessThan, - r_value: QuotedString( - "4", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: LessThan, + version: "4", + }, }, ), Expression( - MarkerExpression { - l_value: MarkerEnvString( - PlatformSystem, - ), + String { + key: PlatformSystem, operator: Equal, - r_value: QuotedString( - "Windows", - ), + value: "Windows", }, ), ], @@ -212,25 +196,21 @@ RequirementsTxt { And( [ Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: GreaterEqual, - r_value: QuotedString( - "3.8", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: GreaterThanEqual, + version: "3.8", + }, }, ), Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: LessThan, - r_value: QuotedString( - "4.0", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: LessThan, + version: "4.0", + }, }, ), ], @@ -271,25 +251,21 @@ RequirementsTxt { And( [ Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: GreaterEqual, - r_value: QuotedString( - "3.8", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: GreaterThanEqual, + version: "3.8", + }, }, ), Expression( - MarkerExpression { - l_value: MarkerEnvVersion( - PythonVersion, - ), - operator: LessThan, - r_value: QuotedString( - "4.0", - ), + Version { + key: PythonVersion, + specifier: VersionSpecifier { + operator: LessThan, + version: "4.0", + }, }, ), ], diff --git a/crates/uv-resolver/src/resolution.rs b/crates/uv-resolver/src/resolution.rs index 75e141ddc..66251778a 100644 --- a/crates/uv-resolver/src/resolution.rs +++ b/crates/uv-resolver/src/resolution.rs @@ -18,7 +18,7 @@ use distribution_types::{ ResolvedDist, SourceAnnotations, Verbatim, VersionId, VersionOrUrlRef, }; use once_map::OnceMap; -use pep440_rs::Version; +use pep440_rs::{Version, VersionSpecifier}; use pep508_rs::MarkerEnvironment; use pypi_types::HashDigest; use uv_normalize::{ExtraName, PackageName}; @@ -378,8 +378,7 @@ impl ResolutionGraph { marker_env: &MarkerEnvironment, ) -> Result> { use pep508_rs::{ - MarkerExpression, MarkerOperator, MarkerTree, MarkerValue, MarkerValueString, - MarkerValueVersion, + MarkerExpression, MarkerOperator, MarkerTree, MarkerValueString, MarkerValueVersion, }; /// A subset of the possible marker values. @@ -395,35 +394,32 @@ impl ResolutionGraph { /// Add all marker parameters from the given tree to the given set. fn add_marker_params_from_tree(marker_tree: &MarkerTree, set: &mut FxHashSet) { - match *marker_tree { - MarkerTree::Expression(ref expr) => { - add_marker_value(&expr.l_value, set); - add_marker_value(&expr.r_value, set); + match marker_tree { + MarkerTree::Expression( + MarkerExpression::Version { key, .. } + | MarkerExpression::VersionInverted { key, .. }, + ) => { + set.insert(MarkerParam::Version(key.clone())); + } + MarkerTree::Expression( + MarkerExpression::String { key, .. } + | MarkerExpression::StringInverted { key, .. }, + ) => { + set.insert(MarkerParam::String(key.clone())); } MarkerTree::And(ref exprs) | MarkerTree::Or(ref exprs) => { for expr in exprs { add_marker_params_from_tree(expr, set); } } - } - } - - /// Add the marker value, if it's a marker parameter, to the set - /// given. - fn add_marker_value(value: &MarkerValue, set: &mut FxHashSet) { - match *value { - MarkerValue::MarkerEnvVersion(ref value_version) => { - set.insert(MarkerParam::Version(value_version.clone())); - } - MarkerValue::MarkerEnvString(ref value_string) => { - set.insert(MarkerParam::String(value_string.clone())); - } // We specifically don't care about these for the // purposes of generating a marker string for a lock // file. Quoted strings are marker values given by the // user. We don't track those here, since we're only // interested in which markers are used. - MarkerValue::Extra | MarkerValue::QuotedString(_) => {} + MarkerTree::Expression( + MarkerExpression::Extra { .. } | MarkerExpression::Arbitrary { .. }, + ) => {} } } @@ -482,18 +478,17 @@ impl ResolutionGraph { let expr = match marker_param { MarkerParam::Version(value_version) => { let from_env = marker_env.get_version(&value_version); - MarkerExpression { - l_value: MarkerValue::MarkerEnvVersion(value_version), - operator: MarkerOperator::Equal, - r_value: MarkerValue::QuotedString(from_env.to_string()), + MarkerExpression::Version { + key: value_version, + specifier: VersionSpecifier::equals_version(from_env.clone()), } } MarkerParam::String(value_string) => { let from_env = marker_env.get_string(&value_string); - MarkerExpression { - l_value: MarkerValue::MarkerEnvString(value_string), + MarkerExpression::String { + key: value_string, operator: MarkerOperator::Equal, - r_value: MarkerValue::QuotedString(from_env.to_string()), + value: from_env.to_string(), } } };