From 681d605bd99002ddd2ac265b56a8f1d2f7db5d9e Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 21 Aug 2024 21:03:39 -0400 Subject: [PATCH] Treat invalid extras as `false` in marker evaluation (#6395) ## Summary Closes https://github.com/astral-sh/uv/issues/6279. Closes https://github.com/astral-sh/uv/pull/6395. --- crates/pep508-rs/src/lib.rs | 3 +- crates/pep508-rs/src/marker/algebra.rs | 4 +- crates/pep508-rs/src/marker/mod.rs | 5 +- crates/pep508-rs/src/marker/parse.rs | 8 +-- crates/pep508-rs/src/marker/tree.rs | 64 ++++++++++++++++++--- crates/uv/tests/pip_compile.rs | 78 ++++++++++++++++++++++++++ 6 files changed, 144 insertions(+), 18 deletions(-) diff --git a/crates/pep508-rs/src/lib.rs b/crates/pep508-rs/src/lib.rs index 4ad72d623..740789e6e 100644 --- a/crates/pep508-rs/src/lib.rs +++ b/crates/pep508-rs/src/lib.rs @@ -37,6 +37,7 @@ use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; use thiserror::Error; use url::Url; +use crate::marker::MarkerValueExtra; use cursor::Cursor; pub use marker::{ ContainsMarkerTree, ExtraMarkerTree, ExtraOperator, InMarkerTree, MarkerEnvironment, @@ -408,7 +409,7 @@ impl Requirement { self.marker .and(MarkerTree::expression(MarkerExpression::Extra { operator: ExtraOperator::Equal, - name: extra.clone(), + name: MarkerValueExtra::Extra(extra.clone()), })); self diff --git a/crates/pep508-rs/src/marker/algebra.rs b/crates/pep508-rs/src/marker/algebra.rs index c627e46d3..57b707334 100644 --- a/crates/pep508-rs/src/marker/algebra.rs +++ b/crates/pep508-rs/src/marker/algebra.rs @@ -56,9 +56,9 @@ use pep440_rs::{Version, VersionSpecifier}; use pubgrub::Range; use rustc_hash::FxHashMap; use std::sync::LazyLock; -use uv_normalize::ExtraName; use uv_pubgrub::PubGrubSpecifier; +use crate::marker::MarkerValueExtra; use crate::ExtraOperator; use crate::{MarkerExpression, MarkerOperator, MarkerValueString, MarkerValueVersion}; @@ -446,7 +446,7 @@ pub(crate) enum Variable { /// /// We keep extras at the leaves of the tree, so when simplifying extras we can /// trivially remove the leaves without having to reconstruct the entire tree. - Extra(ExtraName), + Extra(MarkerValueExtra), } /// A decision node in an Algebraic Decision Diagram. diff --git a/crates/pep508-rs/src/marker/mod.rs b/crates/pep508-rs/src/marker/mod.rs index 29a078ebc..f1cdd7a8f 100644 --- a/crates/pep508-rs/src/marker/mod.rs +++ b/crates/pep508-rs/src/marker/mod.rs @@ -18,8 +18,9 @@ mod tree; pub use environment::{MarkerEnvironment, MarkerEnvironmentBuilder}; pub use tree::{ ContainsMarkerTree, ExtraMarkerTree, ExtraOperator, InMarkerTree, MarkerExpression, - MarkerOperator, MarkerTree, MarkerTreeContents, MarkerTreeKind, MarkerValue, MarkerValueString, - MarkerValueVersion, MarkerWarningKind, StringMarkerTree, StringVersion, VersionMarkerTree, + MarkerOperator, MarkerTree, MarkerTreeContents, MarkerTreeKind, MarkerValue, MarkerValueExtra, + MarkerValueString, MarkerValueVersion, MarkerWarningKind, StringMarkerTree, StringVersion, + VersionMarkerTree, }; /// `serde` helpers for [`MarkerTree`]. diff --git a/crates/pep508-rs/src/marker/parse.rs b/crates/pep508-rs/src/marker/parse.rs index b2a10961e..a45fa6c1c 100644 --- a/crates/pep508-rs/src/marker/parse.rs +++ b/crates/pep508-rs/src/marker/parse.rs @@ -4,6 +4,7 @@ use pep440_rs::{Version, VersionPattern, VersionSpecifier}; use uv_normalize::ExtraName; use crate::cursor::Cursor; +use crate::marker::MarkerValueExtra; use crate::{ ExtraOperator, MarkerExpression, MarkerOperator, MarkerTree, MarkerValue, MarkerValueVersion, MarkerWarningKind, Pep508Error, Pep508ErrorSource, Pep508Url, Reporter, @@ -427,14 +428,13 @@ fn parse_extra_expr( reporter: &mut impl Reporter, ) -> Option { let name = match ExtraName::from_str(value) { - Ok(name) => name, + Ok(name) => MarkerValueExtra::Extra(name), Err(err) => { reporter.report( MarkerWarningKind::ExtraInvalidComparison, - format!("Expected extra name, found '{value}', will be ignored: {err}"), + format!("Expected extra name (found `{value}`): {err}"), ); - - return None; + MarkerValueExtra::Arbitrary(value.to_string()) } }; diff --git a/crates/pep508-rs/src/marker/tree.rs b/crates/pep508-rs/src/marker/tree.rs index 62003dbdf..cf495a4b6 100644 --- a/crates/pep508-rs/src/marker/tree.rs +++ b/crates/pep508-rs/src/marker/tree.rs @@ -435,6 +435,33 @@ impl Deref for StringVersion { } } +/// The [`ExtraName`] value used in `extra` markers. +#[derive(Clone, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)] +pub enum MarkerValueExtra { + /// A valid [`ExtraName`]. + Extra(ExtraName), + /// An invalid name, preserved as an arbitrary string. + Arbitrary(String), +} + +impl MarkerValueExtra { + fn as_extra(&self) -> Option<&ExtraName> { + match self { + Self::Extra(extra) => Some(extra), + Self::Arbitrary(_) => None, + } + } +} + +impl Display for MarkerValueExtra { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + match self { + Self::Extra(extra) => extra.fmt(f), + Self::Arbitrary(string) => string.fmt(f), + } + } +} + /// Represents one clause such as `python_version > "3.8"`. #[derive(Clone, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)] #[allow(missing_docs)] @@ -471,7 +498,7 @@ pub enum MarkerExpression { /// `extra '...'` or `'...' extra`. Extra { operator: ExtraOperator, - name: ExtraName, + name: MarkerValueExtra, }, } @@ -885,7 +912,12 @@ impl MarkerTree { } MarkerTreeKind::Extra(marker) => { return marker - .edge(extras.contains(marker.name())) + .edge( + marker + .name() + .as_extra() + .is_some_and(|extra| extras.contains(extra)), + ) .evaluate_reporter_impl(env, extras, reporter); } } @@ -931,7 +963,12 @@ impl MarkerTree { .children() .any(|(_, tree)| tree.evaluate_extras_and_python_version(extras, python_versions)), MarkerTreeKind::Extra(marker) => marker - .edge(extras.contains(marker.name())) + .edge( + marker + .name() + .as_extra() + .is_some_and(|extra| extras.contains(extra)), + ) .evaluate_extras_and_python_version(extras, python_versions), } } @@ -956,7 +993,12 @@ impl MarkerTree { .children() .any(|(_, tree)| tree.evaluate_extras(extras)), MarkerTreeKind::Extra(marker) => marker - .edge(extras.contains(marker.name())) + .edge( + marker + .name() + .as_extra() + .is_some_and(|extra| extras.contains(extra)), + ) .evaluate_extras(extras), } } @@ -1144,9 +1186,13 @@ impl MarkerTree { } fn simplify_extras_with_impl(self, is_extra: &impl Fn(&ExtraName) -> bool) -> MarkerTree { - MarkerTree(INTERNER.lock().restrict(self.0, &|var| match var { - Variable::Extra(name) => is_extra(name).then_some(true), - _ => None, + MarkerTree(INTERNER.lock().restrict(self.0, &|var| { + match var { + Variable::Extra(name) => name + .as_extra() + .and_then(|name| is_extra(name).then_some(true)), + _ => None, + } })) } } @@ -1375,14 +1421,14 @@ impl Ord for ContainsMarkerTree<'_> { /// A node representing the existence or absence of a given extra, such as `extra == 'bar'`. #[derive(PartialEq, Eq, Clone, Debug)] pub struct ExtraMarkerTree<'a> { - name: &'a ExtraName, + name: &'a MarkerValueExtra, high: NodeId, low: NodeId, } impl ExtraMarkerTree<'_> { /// Returns the name of the extra in this expression. - pub fn name(&self) -> &ExtraName { + pub fn name(&self) -> &MarkerValueExtra { self.name } diff --git a/crates/uv/tests/pip_compile.rs b/crates/uv/tests/pip_compile.rs index 6fe2c689a..7b6af57db 100644 --- a/crates/uv/tests/pip_compile.rs +++ b/crates/uv/tests/pip_compile.rs @@ -11829,3 +11829,81 @@ fn compatible_build_constraint() -> Result<()> { Ok(()) } + +/// Ensure that we treat invalid extra markers as `false`, i.e., in projects that define +/// non-spec-compliant extras. +#[test] +fn invalid_extra() -> Result<()> { + let context = TestContext::new("3.12"); + + let setup_py = context.temp_dir.child("setup.py"); + setup_py.write_str(indoc! {r#" + from setuptools import setup + + extras_require = { + "_anyio": ["anyio"], + "config": ["jsonschema>=2.6.0"], + "encryption": ["iniconfig"], + } + + setup(name="project", install_requires=[], extras_require=extras_require) + "#})?; + + // Sync the `encryption` extra. `anyio` should be omitted. + let requirements_in = context.temp_dir.child("requirements.in"); + requirements_in.write_str(".[encryption]")?; + + uv_snapshot!(context.filters(), context.pip_compile() + .arg("requirements.in"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + # This file was autogenerated by uv via the following command: + # uv pip compile --cache-dir [CACHE_DIR] requirements.in + iniconfig==2.0.0 + # via project + . + # via -r requirements.in + + ----- stderr ----- + Resolved 2 packages in [TIME] + "###); + + // Sync the `_anyio` extra. We should reject it. + let requirements_in = context.temp_dir.child("requirements.in"); + requirements_in.write_str(".[_anyio]")?; + + uv_snapshot!(context.filters(), context.pip_compile() + .arg("requirements.in"), @r###" + success: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + error: Couldn't parse requirement in `requirements.in` at position 0 + Caused by: Expected an alphanumeric character starting the extra name, found '_' + .[_anyio] + ^ + "###); + + // Sync the `anyio` extra. We should reject it. + let requirements_in = context.temp_dir.child("requirements.in"); + requirements_in.write_str(".[anyio]")?; + + uv_snapshot!(context.filters(), context.pip_compile() + .arg("requirements.in"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + # This file was autogenerated by uv via the following command: + # uv pip compile --cache-dir [CACHE_DIR] requirements.in + . + # via -r requirements.in + + ----- stderr ----- + Resolved 1 package in [TIME] + warning: The package `project @ file://[TEMP_DIR]/` does not have an extra named `anyio` + "###); + + Ok(()) +}