pioduino/uv
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

Normalize marker expression order (#5422)

Browse Source 
## Summary

Normalize the order of marker expressions on construction. This removes
the distinction between expressions like `os_name == 'Linux'` vs.
`'Linux' == os_name` throughout the codebase. One caveat here is that
the `in` operator does not have a direct inverse, so we introduce
`MarkerOperator::Contains` to handle that case.

I wanted to land this smaller change before some more intrusive changes
as it simplifies the existing code quite a bit.
This commit is contained in:
Ibraheem Ahmed
2024-07-24 18:28:34 -04:00
committed by GitHub
parent 35b6726df5
commit 39be71f403
4 changed files with 109 additions and 264 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/pep508-rs/src/lib.rs
+4 -1
View File
@@ -1804,8 +1804,11 @@ mod tests {
#[test]
fn no_space_after_operator() {
let requirement = Requirement::<Url>::from_str("pytest;python_version<='4.0'").unwrap();
assert_eq!(requirement.to_string(), "pytest ; python_version <= '4.0'");
let requirement = Requirement::<Url>::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]
crates/pep508-rs/src/marker.rs
+87 -140
View File
@@ -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<MarkerEnvironmentBuilder<'a>> for MarkerEnvironment {
#[allow(missing_docs)]
pub enum MarkerExpression {
/// A version expression, e.g. `<version key> <version op> <quoted PEP 440 version>`.
///
/// Inverted version expressions, such as `<version> <version op> <version key>`, are also
/// normalized to this form.
Version {
key: MarkerValueVersion,
specifier: VersionSpecifier,
},
/// An inverted version expression, e.g `<quoted PEP 440 version> <version op> <version key>`.
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 <extra op> '...'` or `'...' <extra op> extra`
/// `extra <extra op> '...'` or `'...' <extra op> extra`.
Extra {
operator: ExtraOperator,
name: ExtraName,
@@ -1133,9 +1153,10 @@ impl MarkerExpression {
}
}
// '...' == <env key>
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<MarkerExpression> {
// 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::<Version>() {
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,
crates/uv-resolver/src/marker.rs
+16 -115
View File
@@ -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<Version>)> {
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<MarkerOperator> {
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;
crates/uv-resolver/src/resolution/graph.rs
+2 -8
View File
@@ -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<MarkerParam>) {
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) => {