From 0374000ec056a9bb3b145e9daffaa5cc628a1562 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 14 Jan 2024 12:16:54 -0500 Subject: [PATCH] Normalize extras when evaluating PEP 508 markers (#915) ## Summary We always normalize extra names in our requirements (e.g., `cuda12_pip` to `cuda12-pip`), but we weren't normalizing within PEP 508 markers, which meant we ended up comparing `cuda12-pip` (normalized) against `cuda12_pip` (unnormalized). Closes https://github.com/astral-sh/puffin/issues/911. --- crates/pep508-rs/src/lib.rs | 68 ++++++++++++------- crates/pep508-rs/src/marker.rs | 64 ++++++++++------- .../src/pubgrub/dependencies.rs | 4 +- 3 files changed, 84 insertions(+), 52 deletions(-) diff --git a/crates/pep508-rs/src/lib.rs b/crates/pep508-rs/src/lib.rs index feb562099..4ef989ec1 100644 --- a/crates/pep508-rs/src/lib.rs +++ b/crates/pep508-rs/src/lib.rs @@ -41,6 +41,8 @@ pub use marker::{ MarkerValueString, MarkerValueVersion, MarkerWarningKind, StringVersion, }; use pep440_rs::{Version, VersionSpecifier, VersionSpecifiers}; +#[cfg(feature = "pyo3")] +use puffin_normalize::InvalidNameError; use puffin_normalize::{ExtraName, PackageName}; pub use verbatim_url::VerbatimUrl; @@ -192,6 +194,8 @@ impl Serialize for Requirement { } } +type MarkerWarning = (MarkerWarningKind, String, String); + #[cfg(feature = "pyo3")] #[pymethods] impl Requirement { @@ -266,11 +270,18 @@ impl Requirement { /// Returns whether the markers apply for the given environment #[allow(clippy::needless_pass_by_value)] #[pyo3(name = "evaluate_markers")] - pub fn py_evaluate_markers(&self, env: &MarkerEnvironment, extras: Vec) -> bool { - self.evaluate_markers( - env, - &extras.iter().map(String::as_str).collect::>(), - ) + pub fn py_evaluate_markers( + &self, + env: &MarkerEnvironment, + extras: Vec, + ) -> PyResult { + let extras = extras + .into_iter() + .map(|extra| ExtraName::from_str(&extra)) + .collect::, InvalidNameError>>() + .map_err(|err| PyPep508Error::new_err(err.to_string()))?; + + Ok(self.evaluate_markers(env, &extras)) } /// Returns whether the requirement would be satisfied, independent of environment markers, i.e. @@ -285,14 +296,19 @@ impl Requirement { &self, extras: HashSet, python_versions: Vec, - ) -> bool { - self.evaluate_extras_and_python_version( - &extras, - &python_versions - .into_iter() - .map(|py_version| py_version.0) - .collect::>(), - ) + ) -> PyResult { + let extras = extras + .into_iter() + .map(|extra| ExtraName::from_str(&extra)) + .collect::, InvalidNameError>>() + .map_err(|err| PyPep508Error::new_err(err.to_string()))?; + + let python_versions = python_versions + .into_iter() + .map(|py_version| py_version.0) + .collect::>(); + + Ok(self.evaluate_extras_and_python_version(&extras, &python_versions)) } /// Returns whether the markers apply for the given environment @@ -302,8 +318,14 @@ impl Requirement { &self, env: &MarkerEnvironment, extras: Vec, - ) -> (bool, Vec<(MarkerWarningKind, String, String)>) { - self.evaluate_markers_and_report(env, &extras) + ) -> PyResult<(bool, Vec)> { + let extras = extras + .into_iter() + .map(|extra| ExtraName::from_str(&extra)) + .collect::, InvalidNameError>>() + .map_err(|err| PyPep508Error::new_err(err.to_string()))?; + + Ok(self.evaluate_markers_and_report(env, &extras)) } } @@ -326,7 +348,7 @@ impl Requirement { } /// Returns whether the markers apply for the given environment - pub fn evaluate_markers(&self, env: &MarkerEnvironment, extras: &[&str]) -> bool { + pub fn evaluate_markers(&self, env: &MarkerEnvironment, extras: &[ExtraName]) -> bool { if let Some(marker) = &self.marker { marker.evaluate(env, extras) } else { @@ -342,7 +364,7 @@ impl Requirement { /// with an environment and forward all warnings. pub fn evaluate_extras_and_python_version( &self, - extras: &HashSet, + extras: &HashSet, python_versions: &[Version], ) -> bool { if let Some(marker) = &self.marker { @@ -356,16 +378,10 @@ impl Requirement { pub fn evaluate_markers_and_report( &self, env: &MarkerEnvironment, - extras: &[String], - ) -> (bool, Vec<(MarkerWarningKind, String, String)>) { + extras: &[ExtraName], + ) -> (bool, Vec) { if let Some(marker) = &self.marker { - marker.evaluate_collect_warnings( - env, - &extras - .iter() - .map(std::string::String::as_str) - .collect::>(), - ) + marker.evaluate_collect_warnings(env, extras) } else { (true, Vec::new()) } diff --git a/crates/pep508-rs/src/marker.rs b/crates/pep508-rs/src/marker.rs index ec59cceef..8727dd6f8 100644 --- a/crates/pep508-rs/src/marker.rs +++ b/crates/pep508-rs/src/marker.rs @@ -11,6 +11,7 @@ use crate::{Cursor, Pep508Error, Pep508ErrorSource}; use pep440_rs::{Version, VersionPattern, VersionSpecifier}; +use puffin_normalize::ExtraName; #[cfg(feature = "pyo3")] use pyo3::{ basic::CompareOp, exceptions::PyValueError, pyclass, pymethods, PyAny, PyResult, Python, @@ -552,7 +553,7 @@ impl MarkerExpression { fn evaluate( &self, env: &MarkerEnvironment, - extras: &[&str], + extras: &[ExtraName], reporter: &mut impl FnMut(MarkerWarningKind, String, &MarkerExpression), ) -> bool { match &self.l_value { @@ -630,7 +631,15 @@ impl MarkerExpression { } MarkerValue::QuotedString(r_value_string) => r_value_string, }; - self.marker_compare(r_value_string, extras, reporter) + 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) => { @@ -652,9 +661,9 @@ impl MarkerExpression { 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); + "Expected PEP 440 version operator to compare '{}' with {}, found '{}', evaluating to false", + l_string, r_key, self.operator + ), self); return false; } Some(operator) => operator, @@ -683,7 +692,15 @@ impl MarkerExpression { self.compare_strings(l_string, r_string, reporter) } // `'...' == extra` - MarkerValue::Extra => self.marker_compare(l_string, extras, reporter), + 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 @@ -730,25 +747,25 @@ impl MarkerExpression { /// ``` fn evaluate_extras_and_python_version( &self, - extras: &HashSet, + extras: &HashSet, python_versions: &[Version], ) -> bool { match (&self.l_value, &self.operator, &self.r_value) { // `extra == '...'` (MarkerValue::Extra, MarkerOperator::Equal, MarkerValue::QuotedString(r_string)) => { - extras.contains(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) => { - extras.contains(l_string) + ExtraName::from_str(l_string).is_ok_and(|l_extra| extras.contains(&l_extra)) } // `extra != '...'` (MarkerValue::Extra, MarkerOperator::NotEqual, MarkerValue::QuotedString(r_string)) => { - !extras.contains(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) => { - !extras.contains(l_string) + ExtraName::from_str(l_string).is_ok_and(|l_extra| !extras.contains(&l_extra)) } ( MarkerValue::MarkerEnvVersion(MarkerValueVersion::PythonVersion), @@ -860,14 +877,13 @@ impl MarkerExpression { // The `marker '...'` comparison fn marker_compare( &self, - value: &str, - extras: &[&str], + value: &ExtraName, + extras: &[ExtraName], reporter: &mut impl FnMut(MarkerWarningKind, String, &MarkerExpression), ) -> bool { match self.operator { - // TODO: normalize extras - MarkerOperator::Equal => extras.contains(&value), - MarkerOperator::NotEqual => !extras.contains(&value), + 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 @@ -926,7 +942,7 @@ impl FromStr for MarkerTree { impl MarkerTree { /// Does this marker apply in the given environment? - pub fn evaluate(&self, env: &MarkerEnvironment, extras: &[&str]) -> bool { + pub fn evaluate(&self, env: &MarkerEnvironment, extras: &[ExtraName]) -> bool { let mut reporter = |_kind, message, _marker_expression: &MarkerExpression| { warn!("{}", message); }; @@ -947,7 +963,7 @@ impl MarkerTree { pub fn evaluate_reporter( &self, env: &MarkerEnvironment, - extras: &[&str], + extras: &[ExtraName], reporter: &mut impl FnMut(MarkerWarningKind, String, &MarkerExpression), ) -> bool { self.report_deprecated_options(reporter); @@ -957,7 +973,7 @@ impl MarkerTree { fn evaluate_reporter_impl( &self, env: &MarkerEnvironment, - extras: &[&str], + extras: &[ExtraName], reporter: &mut impl FnMut(MarkerWarningKind, String, &MarkerExpression), ) -> bool { match self { @@ -981,7 +997,7 @@ impl MarkerTree { /// and forward all warnings. pub fn evaluate_extras_and_python_version( &self, - extras: &HashSet, + extras: &HashSet, python_versions: &[Version], ) -> bool { match self { @@ -1002,7 +1018,7 @@ impl MarkerTree { pub fn evaluate_collect_warnings( &self, env: &MarkerEnvironment, - extras: &[&str], + extras: &[ExtraName], ) -> (bool, Vec<(MarkerWarningKind, String, String)>) { let mut warnings = Vec::new(); let mut reporter = |kind, warning, marker: &MarkerExpression| { @@ -1131,7 +1147,7 @@ fn parse_marker_operator(cursor: &mut Cursor) -> Result {} Some((pos, other)) => { @@ -1142,7 +1158,7 @@ fn parse_marker_operator(cursor: &mut Cursor) -> Result