Skip to content

Commit

Permalink
Add support for python_version in ... markers
Browse files Browse the repository at this point in the history
  • Loading branch information
zanieb committed Aug 18, 2024
1 parent 0091adf commit 0335efd
Show file tree
Hide file tree
Showing 6 changed files with 286 additions and 17 deletions.
65 changes: 65 additions & 0 deletions crates/pep508-rs/src/marker/algebra.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,11 +168,32 @@ impl InternerGuard<'_> {
),
Err(node) => return node,
},
MarkerExpression::VersionIn {
key: MarkerValueVersion::PythonVersion,
versions,
negated,
} => match Edges::from_python_versions(versions, negated) {
Ok(edges) => (
Variable::Version(MarkerValueVersion::PythonFullVersion),
edges,
),
Err(node) => return node,
},
// A variable representing the output of a version key. Edges correspond
// to disjoint version ranges.
MarkerExpression::Version { key, specifier } => {
(Variable::Version(key), Edges::from_specifier(specifier))
}
// A variable representing the output of a version key. Edges correspond
// to disjoint version ranges.
MarkerExpression::VersionIn {
key,
versions,
negated,
} => (
Variable::Version(key),
Edges::from_versions(&versions, negated),
),
// The `in` and `contains` operators are a bit different than other operators.
// In particular, they do not represent a particular value for the corresponding
// variable, and can overlap. For example, `'nux' in os_name` and `os_name == 'Linux'`
Expand Down Expand Up @@ -587,6 +608,50 @@ impl Edges {
}
}

/// Returns an [`Edges`] where values in the given range are `true`.
///
/// Only for use when the `key` is a `PythonVersion`. Casts to `PythonFullVersion`.
fn from_python_versions(versions: Vec<Version>, negated: bool) -> Result<Edges, NodeId> {
let mut range = Range::empty();

// TODO(zanieb): We need to make sure this is performant, repeated unions like this do not
// seem efficient.
for version in versions {
let specifier = VersionSpecifier::equals_version(version.clone());
let specifier = python_version_to_full_version(specifier)?;
let pubgrub_specifier =
PubGrubSpecifier::from_release_specifier(&normalize_specifier(specifier)).unwrap();
range = range.union(&pubgrub_specifier.into());
}

if negated {
range = range.complement();
}

Ok(Edges::Version {
edges: Edges::from_range(&range),
})
}

/// Returns an [`Edges`] where values in the given range are `true`.
fn from_versions(versions: &Vec<Version>, negated: bool) -> Edges {
let mut range = Range::empty();

// TODO(zanieb): We need to make sure this is performant, repeated unions like this do not
// seem efficient.
for version in versions {
range = range.union(&Range::singleton(version.clone()));
}

if negated {
range = range.complement();
}

Edges::Version {
edges: Edges::from_range(&range),
}
}

/// Returns an [`Edges`] where values in the given range are `true`.
fn from_range<T>(range: &Range<T>) -> SmallVec<(Range<T>, NodeId)>
where
Expand Down
76 changes: 75 additions & 1 deletion crates/pep508-rs/src/marker/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,9 @@ pub(crate) fn parse_marker_key_op_value<T: Pep508Url>(
// Convert a `<marker_value> <marker_op> <marker_value>` expression into it's
// typed equivalent.
let expr = match l_value {
// The only sound choice for this is `<version key> <version op> <quoted PEP 440 version>`
// Either:
// - `<version key> <version op> <quoted PEP 440 version>`
// - `<version key> in <list of quoted PEP 440 versions>` and ("not in")
MarkerValue::MarkerEnvVersion(key) => {
let MarkerValue::QuotedString(value) = r_value else {
reporter.report(
Expand All @@ -146,6 +148,12 @@ pub(crate) fn parse_marker_key_op_value<T: Pep508Url>(
return Ok(None);
};

// Check for `in` and `not in` expressions
if let Some(expr) = parse_version_in_expr(key.clone(), operator, &value, reporter) {
return Ok(Some(expr));
}

// Otherwise, it's a normal version expression
parse_version_expr(key.clone(), operator, &value, reporter)
}
// The only sound choice for this is `<env key> <op> <string>`
Expand Down Expand Up @@ -238,6 +246,72 @@ pub(crate) fn parse_marker_key_op_value<T: Pep508Url>(
Ok(expr)
}

/// Creates an instance of [`MarkerExpression::VersionIn`] with the given values.
///
/// Some important caveats apply here.
///
/// While the specification defines this operation as a substring search, for versions, we use a
/// version-aware match so we can perform algebra on the expressions. This means that some markers
/// will not be evaluated according to the specification, but these marker expressions are
/// relatively rare so the trade-off is acceptable.
///
/// The following limited expression is supported:
///
/// [not] in '<version> [additional versions]'
///
/// where the version is PEP 440 compliant. Arbitrary whitespace is allowed between versions.
///
/// Returns `None` if the [`MarkerOperator`] is not relevant.
/// Reports a warning if an invalid version is encountered, and returns `None`.
fn parse_version_in_expr(
key: MarkerValueVersion,
operator: MarkerOperator,
value: &str,
reporter: &mut impl Reporter,
) -> Option<MarkerExpression> {
if !matches!(operator, MarkerOperator::In | MarkerOperator::NotIn) {
return None;
}
let negated = matches!(operator, MarkerOperator::NotIn);

let mut cursor = Cursor::new(value);
let mut versions = Vec::new();

// Parse all of the values in the list as versions
loop {
// Allow arbitrary whitespace between versions
cursor.eat_whitespace();

let (start, len) = cursor.take_while(|c| !c.is_whitespace());
if len == 0 {
break;
}

let version = match Version::from_str(cursor.slice(start, len)) {
Ok(version) => version,
Err(err) => {
reporter.report(
MarkerWarningKind::Pep440Error,
format!(
"Expected PEP 440 versions to compare with {key}, found {value},
will be ignored: {err}"
),
);

return None;
}
};

versions.push(version);
}

Some(MarkerExpression::VersionIn {
key,
versions,
negated,
})
}

/// Creates an instance of [`MarkerExpression::Version`] with the given values.
///
/// Reports a warning on failure, and returns `None`.
Expand Down
16 changes: 16 additions & 0 deletions crates/pep508-rs/src/marker/simplify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,22 @@ fn is_negation(left: &MarkerExpression, right: &MarkerExpression) -> bool {
.negate()
.is_some_and(|negated| negated == *specifier2.operator())
}
MarkerExpression::VersionIn {
key,
versions,
negated,
} => {
let MarkerExpression::VersionIn {
key: key2,
versions: versions2,
negated: negated2,
} = right
else {
return false;
};

key == key2 && versions == versions2 && negated != negated2
}
MarkerExpression::String {
key,
operator,
Expand Down
129 changes: 129 additions & 0 deletions crates/pep508-rs/src/marker/tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::fmt::{self, Display, Formatter};
use std::ops::{Bound, Deref};
use std::str::FromStr;

use itertools::Itertools;
use pubgrub::Range;
#[cfg(feature = "pyo3")]
use pyo3::{basic::CompareOp, pyclass, pymethods};
Expand Down Expand Up @@ -436,6 +437,19 @@ pub enum MarkerExpression {
key: MarkerValueVersion,
specifier: VersionSpecifier,
},
/// A version in list expression, e.g. `<version key> in <quoted list of PEP 440 versions>`.
///
/// A special case of [`MarkerExpression::String`] with the [`MarkerOperator::In`] operator for
/// [`MarkerValueVersion`] values.
///
/// See [`parse::parse_version_in_expr`] for details on the supported syntax.
///
/// Negated expressions, using "not in" are represented using `negated = true`.
VersionIn {
key: MarkerValueVersion,
versions: Vec<Version>,
negated: bool,
},
/// An string marker comparison, e.g. `sys_platform == '...'`.
///
/// Inverted string expressions, e.g `'...' == sys_platform`, are also normalized to this form.
Expand Down Expand Up @@ -534,6 +548,15 @@ impl Display for MarkerExpression {
}
write!(f, "{key} {op} '{version}'")
}
MarkerExpression::VersionIn {
key,
versions,
negated,
} => {
let op = if *negated { "not in" } else { "in" };
let versions = versions.iter().map(ToString::to_string).join(" ");
write!(f, "{key} {op} '{versions}'")
}
MarkerExpression::String {
key,
operator,
Expand Down Expand Up @@ -1542,6 +1565,69 @@ mod test {
assert!(!marker3.evaluate(&env37, &[]));
}

#[test]
fn test_version_in_evaluation() {
let env27 = MarkerEnvironment::try_from(MarkerEnvironmentBuilder {
implementation_name: "",
implementation_version: "2.7",
os_name: "linux",
platform_machine: "",
platform_python_implementation: "",
platform_release: "",
platform_system: "",
platform_version: "",
python_full_version: "2.7",
python_version: "2.7",
sys_platform: "linux",
})
.unwrap();
let env37 = env37();

let marker = MarkerTree::from_str("python_version in \"2.7 3.2 3.3\"").unwrap();
assert!(marker.evaluate(&env27, &[]));
assert!(!marker.evaluate(&env37, &[]));

let marker = MarkerTree::from_str("python_version in \"2.7 3.7\"").unwrap();
assert!(marker.evaluate(&env27, &[]));
assert!(marker.evaluate(&env37, &[]));

let marker = MarkerTree::from_str("python_version in \"2.4 3.8 4.0\"").unwrap();
assert!(!marker.evaluate(&env27, &[]));
assert!(!marker.evaluate(&env37, &[]));

let marker = MarkerTree::from_str("python_version not in \"2.7 3.2 3.3\"").unwrap();
assert!(!marker.evaluate(&env27, &[]));
assert!(marker.evaluate(&env37, &[]));

let marker = MarkerTree::from_str("python_version not in \"2.7 3.7\"").unwrap();
assert!(!marker.evaluate(&env27, &[]));
assert!(!marker.evaluate(&env37, &[]));

let marker = MarkerTree::from_str("python_version not in \"2.4 3.8 4.0\"").unwrap();
assert!(marker.evaluate(&env27, &[]));
assert!(marker.evaluate(&env37, &[]));

let marker = MarkerTree::from_str("python_full_version in \"2.7\"").unwrap();
assert!(marker.evaluate(&env27, &[]));
assert!(!marker.evaluate(&env37, &[]));

let marker = MarkerTree::from_str("implementation_version in \"2.7 3.2 3.3\"").unwrap();
assert!(marker.evaluate(&env27, &[]));
assert!(!marker.evaluate(&env37, &[]));

let marker = MarkerTree::from_str("implementation_version in \"2.7 3.7\"").unwrap();
assert!(marker.evaluate(&env27, &[]));
assert!(marker.evaluate(&env37, &[]));

let marker = MarkerTree::from_str("implementation_version not in \"2.7 3.7\"").unwrap();
assert!(!marker.evaluate(&env27, &[]));
assert!(!marker.evaluate(&env37, &[]));

let marker = MarkerTree::from_str("implementation_version not in \"2.4 3.8 4.0\"").unwrap();
assert!(marker.evaluate(&env27, &[]));
assert!(marker.evaluate(&env37, &[]));
}

#[test]
#[cfg(feature = "tracing")]
fn warnings() {
Expand Down Expand Up @@ -1808,12 +1894,55 @@ mod test {
assert_false("python_version == '3.9.0.*'");
assert_true("python_version != '3.9.1'");

// Technically these is are valid substring comparisosn, but we do not allow them.
// e.g., using a version with patch components with `python_version` is considered
// impossible to satisfy since it is truncated at the minor version
assert_false("python_version in '3.9.0'");
// ... unless there are other versions that could match
assert_true("python_version in '3.* 3.9.0'");
// e.g., using a version that is not PEP 440 compliant is considered arbitrary
assert_true("python_version in 'foo'");

assert_simplifies("python_version == '3.9'", "python_full_version == '3.9.*'");
assert_simplifies(
"python_version == '3.9.0'",
"python_full_version == '3.9.*'",
);

// `<version> in`
// e.g., when the range is not contiguous
assert_simplifies(
"python_version in '3.9 3.11'",
"python_full_version == '3.9.*' or python_full_version == '3.11.*'",
);
// e.g., when the range is contiguous
assert_simplifies(
"python_version in '3.9 3.10 3.11'",
"python_full_version >= '3.9' and python_full_version < '3.12'",
);
// e.g., with `implementation_version` instead of `python_version`
assert_simplifies(
"implementation_version in '3.9 3.11'",
"implementation_version == '3.9' or implementation_version == '3.11'",
);

// '<version> not in'
// e.g., when the range is not contiguous
assert_simplifies(
"python_version not in '3.9 3.11'",
"python_full_version < '3.9' or python_full_version == '3.10.*' or python_full_version >= '3.12'",
);
// e.g, when the range is contiguous
assert_simplifies(
"python_version not in '3.9 3.10 3.11'",
"python_full_version < '3.9' or python_full_version >= '3.12'",
);
// e.g., with `implementation_version` instead of `python_version`
assert_simplifies(
"implementation_version not in '3.9 3.11'",
"implementation_version != '3.9' and implementation_version != '3.11'",
);

assert_simplifies("python_version != '3.9'", "python_full_version != '3.9.*'");

assert_simplifies("python_version >= '3.9.0'", "python_full_version >= '3.9'");
Expand Down
Loading

0 comments on commit 0335efd

Please sign in to comment.