diff --git a/crates/pep508-rs/src/lib.rs b/crates/pep508-rs/src/lib.rs index 8ec32d04e200..06bad39387e3 100644 --- a/crates/pep508-rs/src/lib.rs +++ b/crates/pep508-rs/src/lib.rs @@ -1804,8 +1804,11 @@ mod tests { #[test] fn no_space_after_operator() { + let requirement = Requirement::::from_str("pytest;python_version<='4.0'").unwrap(); + assert_eq!(requirement.to_string(), "pytest ; python_version <= '4.0'"); + let requirement = Requirement::::from_str("pytest;'4.0'>=python_version").unwrap(); - assert_eq!(requirement.to_string(), "pytest ; '4.0' >= python_version"); + assert_eq!(requirement.to_string(), "pytest ; python_version <= '4.0'"); } #[test] diff --git a/crates/pep508-rs/src/marker.rs b/crates/pep508-rs/src/marker.rs index 7e95f6529a1f..8061ddbb5a30 100644 --- a/crates/pep508-rs/src/marker.rs +++ b/crates/pep508-rs/src/marker.rs @@ -234,6 +234,16 @@ pub enum MarkerOperator { In, /// `not in` NotIn, + /// The inverse of the `in` operator. + /// + /// This is not a valid operator when parsing but is used for normalizing + /// marker trees. + Contains, + /// The inverse of the `not in` operator. + /// + /// This is not a valid operator when parsing but is used for normalizing + /// marker trees. + NotContains, } impl MarkerOperator { @@ -247,8 +257,7 @@ impl MarkerOperator { Self::LessThan => Some(pep440_rs::Operator::LessThan), Self::LessEqual => Some(pep440_rs::Operator::LessThanEqual), Self::TildeEqual => Some(pep440_rs::Operator::TildeEqual), - Self::In => None, - Self::NotIn => None, + _ => None, } } @@ -267,8 +276,27 @@ impl MarkerOperator { Self::GreaterEqual => Self::LessThan, Self::In => Self::NotIn, Self::NotIn => Self::In, + Self::Contains => Self::NotContains, + Self::NotContains => Self::Contains, }) } + + /// Inverts this marker operator. + fn invert(self) -> MarkerOperator { + match self { + Self::LessThan => Self::GreaterThan, + Self::LessEqual => Self::GreaterEqual, + Self::GreaterThan => Self::LessThan, + Self::GreaterEqual => Self::LessEqual, + Self::Equal => Self::Equal, + Self::NotEqual => Self::NotEqual, + Self::TildeEqual => Self::TildeEqual, + Self::In => Self::Contains, + Self::NotIn => Self::NotContains, + Self::Contains => Self::In, + Self::NotContains => Self::NotIn, + } + } } impl FromStr for MarkerOperator { @@ -312,8 +340,8 @@ impl Display for MarkerOperator { Self::LessThan => "<", Self::LessEqual => "<=", Self::TildeEqual => "~=", - Self::In => "in", - Self::NotIn => "not in", + Self::In | Self::Contains => "in", + Self::NotIn | Self::NotContains => "not in", }) } } @@ -931,30 +959,22 @@ impl<'a> TryFrom> for MarkerEnvironment { #[allow(missing_docs)] pub enum MarkerExpression { /// A version expression, e.g. ` `. + /// + /// Inverted version expressions, such as ` `, are also + /// normalized to this form. 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 == '...'`. + /// + /// Inverted string expressions, e.g `'...' == sys_platform`, are also normalized to this form. 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 '...'` or `'...' extra`. Extra { operator: ExtraOperator, name: ExtraName, @@ -1133,9 +1153,10 @@ impl MarkerExpression { } } // '...' == - MarkerValue::MarkerEnvString(key) => MarkerExpression::StringInverted { + MarkerValue::MarkerEnvString(key) => MarkerExpression::String { key, - operator, + // Invert the operator to normalize the expression order. + operator: operator.invert(), value: l_string, }, // `'...' == extra` @@ -1187,7 +1208,7 @@ impl MarkerExpression { /// Creates an instance of [`MarkerExpression::Version`] with the given values. /// /// Reports a warning on failure, and returns `None`. - pub fn version( + fn version( key: MarkerValueVersion, marker_operator: MarkerOperator, value: &str, @@ -1211,11 +1232,9 @@ impl MarkerExpression { reporter.report( MarkerWarningKind::Pep440Error, format!( - "Expected PEP 440 version operator to compare {} with '{}', - found '{}', will evaluate to false", - key, - pattern.version(), - marker_operator + "Expected PEP 440 version operator to compare {key} with '{version}', + found '{marker_operator}', will evaluate to false", + version = pattern.version() ), ); @@ -1236,7 +1255,7 @@ impl MarkerExpression { Some(MarkerExpression::Version { key, specifier }) } - /// Creates an instance of [`MarkerExpression::VersionInverted`] with the given values. + /// Creates an instance of [`MarkerExpression::Version`] from an inverted expression. /// /// Reports a warning on failure, and returns `None`. fn version_inverted( @@ -1245,6 +1264,10 @@ impl MarkerExpression { key: MarkerValueVersion, reporter: &mut impl Reporter, ) -> Option { + // Invert the operator to normalize the expression order. + let marker_operator = marker_operator.invert(); + + // Not star allowed here, `'3.*' == python_version` is not a valid PEP 440 comparison. let version = match value.parse::() { Ok(version) => version, Err(err) => { @@ -1271,11 +1294,18 @@ impl MarkerExpression { return None; }; - Some(MarkerExpression::VersionInverted { - version, - operator, - key, - }) + let specifier = match VersionSpecifier::from_version(operator, version) { + 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::Extra`] with the given values, falling back to @@ -1342,24 +1372,6 @@ impl MarkerExpression { } } } - MarkerExpression::VersionInverted { - ref version, - ref operator, - ref key, - } => { - let version = version.clone(); - match operator.negate() { - None => negate_compatible_version(key.clone(), version), - Some(op) => { - let expr = MarkerExpression::VersionInverted { - version: version.without_local(), - operator: op, - key: key.clone(), - }; - MarkerTree::Expression(expr) - } - } - } MarkerExpression::String { ref key, ref operator, @@ -1375,21 +1387,6 @@ impl MarkerExpression { }; MarkerTree::Expression(expr) } - MarkerExpression::StringInverted { - ref value, - ref operator, - ref key, - } => { - let expr = MarkerExpression::StringInverted { - value: value.clone(), - // negating ~= doesn't make sense in this context, but - // I believe it is technically allowed, so we just leave - // it as-is. - operator: operator.negate().unwrap_or(MarkerOperator::TildeEqual), - key: key.clone(), - }; - MarkerTree::Expression(expr) - } MarkerExpression::Extra { ref operator, ref name, @@ -1436,28 +1433,6 @@ impl MarkerExpression { 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.report( - MarkerWarningKind::Pep440Error, - format!("Invalid operator/version combination: {err}"), - ); - - return false; - } - }; - - specifier.contains(version) - }) - .unwrap_or(true), MarkerExpression::String { key, operator, @@ -1468,16 +1443,6 @@ impl MarkerExpression { Self::compare_strings(l_string, *operator, value, reporter) }) .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, @@ -1533,24 +1498,6 @@ impl MarkerExpression { } => python_versions .iter() .any(|l_version| specifier.contains(l_version)), - MarkerExpression::VersionInverted { - key: MarkerValueVersion::PythonVersion, - operator, - 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; - }; - - specifier.contains(version) - }) - } MarkerExpression::Extra { operator: ExtraOperator::Equal, name, @@ -1610,6 +1557,8 @@ impl MarkerExpression { } MarkerOperator::In => r_string.contains(l_string), MarkerOperator::NotIn => !r_string.contains(l_string), + MarkerOperator::Contains => l_string.contains(r_string), + MarkerOperator::NotContains => !l_string.contains(r_string), } } } @@ -1633,31 +1582,20 @@ impl Display for MarkerExpression { } write!(f, "{key} {op} '{version}'") } - MarkerExpression::VersionInverted { - version, - operator: op, - key, - } => { - if op == &pep440_rs::Operator::EqualStar || op == &pep440_rs::Operator::NotEqualStar - { - return write!(f, "'{version}.*' {op} {key}"); - } - write!(f, "'{version}' {op} {key}") - } MarkerExpression::String { key, operator, value, } => { + if matches!( + operator, + MarkerOperator::Contains | MarkerOperator::NotContains + ) { + return write!(f, "'{value}' {} {key}", operator.invert()); + } + write!(f, "{key} {operator} '{value}'") } - MarkerExpression::StringInverted { - value, - operator, - key, - } => { - write!(f, "'{value}' {operator} {key}") - } MarkerExpression::Extra { operator, name } => { write!(f, "extra {operator} '{name}'") } @@ -2469,7 +2407,7 @@ mod test { }; assert_eq!(neg("python_version > '3.6'"), "python_version <= '3.6'"); - assert_eq!(neg("'3.6' < python_version"), "'3.6' >= python_version"); + assert_eq!(neg("'3.6' < python_version"), "python_version <= '3.6'"); assert_eq!( neg("python_version == '3.6.*'"), @@ -2494,7 +2432,7 @@ mod test { ); assert_eq!(neg("sys_platform == 'linux'"), "sys_platform != 'linux'"); - assert_eq!(neg("'linux' == sys_platform"), "'linux' != sys_platform"); + assert_eq!(neg("'linux' == sys_platform"), "sys_platform != 'linux'"); // ~= is nonsense on string markers. Evaluation always returns false // in this case, so technically negation would be an expression that @@ -2633,6 +2571,12 @@ mod test { .evaluate_collect_warnings(&env37, &[]); assert_eq!(warnings, &[]); assert!(!result); + + let (result, warnings) = MarkerTree::from_str("'3.*' == python_version") + .unwrap() + .evaluate_collect_warnings(&env37, &[]); + assert_eq!(warnings, &[]); + assert!(!result); } #[test] @@ -2714,15 +2658,18 @@ mod test { ) .unwrap(), MarkerTree::And(vec![ - MarkerTree::Expression(MarkerExpression::StringInverted { + MarkerTree::Expression(MarkerExpression::String { value: "nt".to_string(), - operator: MarkerOperator::In, + operator: MarkerOperator::Contains, key: MarkerValueString::OsName, }), - MarkerTree::Expression(MarkerExpression::VersionInverted { + MarkerTree::Expression(MarkerExpression::Version { key: MarkerValueVersion::PythonVersion, - operator: pep440_rs::Operator::GreaterThanEqual, - version: "3.7".parse().unwrap(), + specifier: VersionSpecifier::from_pattern( + pep440_rs::Operator::LessThanEqual, + "3.7".parse().unwrap() + ) + .unwrap() }), MarkerTree::Expression(MarkerExpression::Version { key: MarkerValueVersion::PythonFullVersion, diff --git a/crates/uv-resolver/src/marker.rs b/crates/uv-resolver/src/marker.rs index d2e738e88acf..9d50173a28f3 100644 --- a/crates/uv-resolver/src/marker.rs +++ b/crates/uv-resolver/src/marker.rs @@ -6,7 +6,7 @@ use std::ops::RangeBounds; use pubgrub::range::{Range as PubGrubRange, Range}; use rustc_hash::FxHashMap; -use pep440_rs::{Operator, Version, VersionSpecifier}; +use pep440_rs::{Version, VersionSpecifier}; use pep508_rs::{ ExtraName, ExtraOperator, MarkerExpression, MarkerOperator, MarkerTree, MarkerValueString, MarkerValueVersion, @@ -74,12 +74,8 @@ pub(crate) fn is_disjoint(first: &MarkerTree, second: &MarkerTree) -> bool { match (expr1, expr2) { // `Arbitrary` expressions always evaluate to `false`, and are thus always disjoint. (MarkerExpression::Arbitrary { .. }, _) | (_, MarkerExpression::Arbitrary { .. }) => true, - (MarkerExpression::Version { .. } | MarkerExpression::VersionInverted { .. }, expr2) => { - version_is_disjoint(expr1, expr2) - } - (MarkerExpression::String { .. } | MarkerExpression::StringInverted { .. }, expr2) => { - string_is_disjoint(expr1, expr2) - } + (MarkerExpression::Version { .. }, expr2) => version_is_disjoint(expr1, expr2), + (MarkerExpression::String { .. }, expr2) => string_is_disjoint(expr1, expr2), (MarkerExpression::Extra { operator, name }, expr2) => { extra_is_disjoint(operator, name, expr2) } @@ -90,43 +86,6 @@ pub(crate) fn is_disjoint(first: &MarkerTree, second: &MarkerTree) -> bool { fn string_is_disjoint(this: &MarkerExpression, other: &MarkerExpression) -> bool { use MarkerOperator::*; - // The `in` and `not in` operators are not reversible, so we have to ensure the expressions - // match exactly. Notably, `'a' in env` and `env not in 'a'` are not disjoint given `env == 'ab'`. - match (this, other) { - ( - MarkerExpression::String { - key, - operator, - value, - }, - MarkerExpression::String { - key: key2, - operator: operator2, - value: value2, - }, - ) - | ( - MarkerExpression::StringInverted { - key, - operator, - value, - }, - MarkerExpression::StringInverted { - key: key2, - operator: operator2, - value: value2, - }, - ) if key == key2 => match (operator, operator2) { - // The only disjoint expressions involving these operators are `key in value` - // and `key not in value`, or reversed. - (In, NotIn) | (NotIn, In) => return value == value2, - // Anything else cannot be disjoint. - (In | NotIn, _) | (_, In | NotIn) => return false, - _ => {} - }, - _ => {} - } - // Extract the normalized string expressions. let Some((key, operator, value)) = extract_string_expression(this) else { return false; @@ -144,6 +103,12 @@ fn string_is_disjoint(this: &MarkerExpression, other: &MarkerExpression) -> bool // The only disjoint expressions involving strict inequality are `key != value` and `key == value`. (NotEqual, Equal) | (Equal, NotEqual) => return value == value2, (NotEqual, _) | (_, NotEqual) => return false, + // Similarly for `in` and `not in`. + (In, NotIn) | (NotIn, In) => return value == value2, + (In | NotIn, _) | (_, In | NotIn) => return false, + // As well as the inverse. + (Contains, NotContains) | (NotContains, Contains) => return value == value2, + (Contains | NotContains, _) | (_, Contains | NotContains) => return false, _ => {} } @@ -639,14 +604,6 @@ fn extract_string_expression( operator, value, } => Some((key, *operator, value)), - MarkerExpression::StringInverted { - value, - operator, - key, - } => { - // If the expression was inverted, we have to reverse the marker operator. - reverse_marker_operator(*operator).map(|operator| (key, operator, value.as_str())) - } _ => None, } } @@ -675,7 +632,7 @@ fn string_bounds(value: &str, operator: MarkerOperator) -> (Bound<&str>, Bound<& GreaterEqual => (Included(value), Unbounded), LessThan => (Unbounded, Excluded(value)), LessEqual => (Unbounded, Included(value)), - NotEqual | In | NotIn => unreachable!(), + NotEqual | In | NotIn | Contains | NotContains => unreachable!(), } } @@ -717,21 +674,15 @@ fn version_is_disjoint(this: &MarkerExpression, other: &MarkerExpression) -> boo fn contains_complements(trees: &[MarkerTree]) -> bool { let mut terms = FxHashMap::default(); for tree in trees { - let MarkerTree::Expression( - MarkerExpression::String { - key, - operator, - value, - } - | MarkerExpression::StringInverted { - value, - operator, - key, - }, - ) = tree + let MarkerTree::Expression(MarkerExpression::String { + key, + operator, + value, + }) = tree else { continue; }; + match operator { MarkerOperator::Equal => { if let Some(MarkerOperator::NotEqual) = terms.insert((key, value), operator) { @@ -753,18 +704,6 @@ fn contains_complements(trees: &[MarkerTree]) -> bool { fn keyed_range(expr: &MarkerExpression) -> Option<(&MarkerValueVersion, PubGrubRange)> { let (key, specifier) = match expr { MarkerExpression::Version { key, specifier } => (key, specifier.clone()), - MarkerExpression::VersionInverted { - version, - operator, - key, - } => { - // if the expression was inverted, we have to reverse the operator before constructing - // a version specifier - let operator = reverse_operator(*operator); - let specifier = VersionSpecifier::from_version(operator, version.clone()).ok()?; - - (key, specifier) - } _ => return None, }; @@ -778,50 +717,12 @@ fn keyed_range(expr: &MarkerExpression) -> Option<(&MarkerValueVersion, PubGrubR key: MarkerValueVersion::PythonFullVersion, .. } => PubGrubSpecifier::from_pep440_specifier(&specifier).ok()?, - MarkerExpression::VersionInverted { - key: MarkerValueVersion::PythonVersion, - .. - } => PubGrubSpecifier::from_release_specifier(&specifier).ok()?, - MarkerExpression::VersionInverted { - key: MarkerValueVersion::PythonFullVersion, - .. - } => PubGrubSpecifier::from_pep440_specifier(&specifier).ok()?, _ => return None, }; Some((key, pubgrub_specifier.into())) } -/// Reverses a binary operator. -fn reverse_operator(operator: Operator) -> Operator { - use Operator::*; - - match operator { - LessThan => GreaterThan, - LessThanEqual => GreaterThanEqual, - GreaterThan => LessThan, - GreaterThanEqual => LessThanEqual, - _ => operator, - } -} - -/// Reverses a marker operator, if possible. -fn reverse_marker_operator(operator: MarkerOperator) -> Option { - use MarkerOperator::*; - - Some(match operator { - LessThan => GreaterThan, - LessEqual => GreaterEqual, - GreaterThan => LessThan, - GreaterEqual => LessEqual, - Equal => Equal, - NotEqual => NotEqual, - TildeEqual => TildeEqual, - // The `in` and `not in` operators are not reversible. - In | NotIn => return None, - }) -} - #[cfg(test)] mod tests { use pep508_rs::TracingReporter; diff --git a/crates/uv-resolver/src/resolution/graph.rs b/crates/uv-resolver/src/resolution/graph.rs index 37b8bc0181eb..b482d92ac21a 100644 --- a/crates/uv-resolver/src/resolution/graph.rs +++ b/crates/uv-resolver/src/resolution/graph.rs @@ -391,16 +391,10 @@ 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 IndexSet) { match marker_tree { - MarkerTree::Expression( - MarkerExpression::Version { key, .. } - | MarkerExpression::VersionInverted { key, .. }, - ) => { + MarkerTree::Expression(MarkerExpression::Version { key, .. }) => { set.insert(MarkerParam::Version(key.clone())); } - MarkerTree::Expression( - MarkerExpression::String { key, .. } - | MarkerExpression::StringInverted { key, .. }, - ) => { + MarkerTree::Expression(MarkerExpression::String { key, .. }) => { set.insert(MarkerParam::String(key.clone())); } MarkerTree::And(ref exprs) | MarkerTree::Or(ref exprs) => {