Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for python_version in ... markers #6172

Merged
merged 1 commit into from
Aug 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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`. Normalizes 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
138 changes: 138 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,64 @@ mod test {
assert_false("python_version == '3.9.0.*'");
assert_true("python_version != '3.9.1'");

// Technically these is are valid substring comparison, but we do not allow them.
// e.g., using a version with patch components with `python_version` is considered
// impossible to satisfy since the value it is truncated at the minor version
assert_false("python_version in '3.9.0'");
// e.g., using a version that is not PEP 440 compliant is considered arbitrary
assert_true("python_version in 'foo'");
// e.g., including `*` versions, which would require tracking a version specifier
assert_true("python_version in '3.9.*'");
// e.g., when non-whitespace separators are present
assert_true("python_version in '3.9, 3.10'");
assert_true("python_version in '3.9,3.10'");
assert_true("python_version in '3.9 or 3.10'");

// e.g, when one of the values cannot be true
// TODO(zanieb): This seems like a quirk of the `python_full_version` normalization, this
// should just act as though the patch version isn't present
assert_false("python_version in '3.9 3.10.0 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.*'",
);

// `<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
Loading