From bc8002e26ec018cdaa0cf18d164f6429af716e01 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 17 Jan 2025 11:25:32 -0500 Subject: [PATCH] Avoid narrowing `requires-python` marker with disjunctions (#10704) ## Summary A bug in `requires_python` (which infers the Python requirement from a marker) was leading us to break an invariant around the relationship between the marker environment and the Python requirement. This, in turn, was leading us to drop parts of the environment space when solving. Specifically, in the linked example, we generated a fork for `python_full_version < '3.10' or platform_python_implementation != 'CPython'`, which was later split into `python_full_version == '3.8.*'` and `python_full_version == '3.9.*'`, losing the `platform_python_implementation != 'CPython'` portion. Closes https://github.com/astral-sh/uv/issues/10669. --- Cargo.lock | 1 + crates/uv-resolver/Cargo.toml | 1 + crates/uv-resolver/src/marker.rs | 156 ++++++++++++++++++++++++++---- crates/uv/tests/it/pip_compile.rs | 34 +++++++ 4 files changed, 172 insertions(+), 20 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2442f15c5..b370ecd81 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5538,6 +5538,7 @@ dependencies = [ "same-file", "schemars", "serde", + "smallvec", "textwrap", "thiserror 2.0.11", "tokio", diff --git a/crates/uv-resolver/Cargo.toml b/crates/uv-resolver/Cargo.toml index 4f0a4687a..1bf906679 100644 --- a/crates/uv-resolver/Cargo.toml +++ b/crates/uv-resolver/Cargo.toml @@ -55,6 +55,7 @@ rustc-hash = { workspace = true } same-file = { workspace = true } schemars = { workspace = true, optional = true } serde = { workspace = true } +smallvec = { workspace = true } textwrap = { workspace = true } thiserror = { workspace = true } tokio = { workspace = true } diff --git a/crates/uv-resolver/src/marker.rs b/crates/uv-resolver/src/marker.rs index 4beb29d77..089e1ad2d 100644 --- a/crates/uv-resolver/src/marker.rs +++ b/crates/uv-resolver/src/marker.rs @@ -1,4 +1,7 @@ -use pubgrub::Range; +use pubgrub::Ranges; +use smallvec::SmallVec; +use std::ops::Bound; + use uv_pep440::Version; use uv_pep508::{CanonicalMarkerValueVersion, MarkerTree, MarkerTreeKind}; @@ -6,58 +9,77 @@ use crate::requires_python::{LowerBound, RequiresPythonRange, UpperBound}; /// Returns the bounding Python versions that can satisfy the [`MarkerTree`], if it's constrained. pub(crate) fn requires_python(tree: MarkerTree) -> Option { - fn collect_python_markers(tree: MarkerTree, markers: &mut Vec>) { + /// A small vector of Python version markers. + type Markers = SmallVec<[Ranges; 3]>; + + /// Collect the Python version markers from the tree. + /// + /// Specifically, performs a DFS to collect all Python requirements on the path to every + /// `MarkerTreeKind::True` node. + fn collect_python_markers(tree: MarkerTree, markers: &mut Markers, range: &Ranges) { match tree.kind() { - MarkerTreeKind::True | MarkerTreeKind::False => {} + MarkerTreeKind::True => { + markers.push(range.clone()); + } + MarkerTreeKind::False => {} MarkerTreeKind::Version(marker) => match marker.key() { CanonicalMarkerValueVersion::PythonFullVersion => { for (range, tree) in marker.edges() { - if !tree.is_false() { - markers.push(range.clone()); - } + collect_python_markers(tree, markers, range); } } CanonicalMarkerValueVersion::ImplementationVersion => { for (_, tree) in marker.edges() { - collect_python_markers(tree, markers); + collect_python_markers(tree, markers, range); } } }, MarkerTreeKind::String(marker) => { for (_, tree) in marker.children() { - collect_python_markers(tree, markers); + collect_python_markers(tree, markers, range); } } MarkerTreeKind::In(marker) => { for (_, tree) in marker.children() { - collect_python_markers(tree, markers); + collect_python_markers(tree, markers, range); } } MarkerTreeKind::Contains(marker) => { for (_, tree) in marker.children() { - collect_python_markers(tree, markers); + collect_python_markers(tree, markers, range); } } MarkerTreeKind::Extra(marker) => { for (_, tree) in marker.children() { - collect_python_markers(tree, markers); + collect_python_markers(tree, markers, range); } } } } - let mut markers = Vec::new(); - collect_python_markers(tree, &mut markers); + if tree.is_true() || tree.is_false() { + return None; + } - // Take the union of all Python version markers. + let mut markers = Markers::new(); + collect_python_markers(tree, &mut markers, &Ranges::full()); + + // If there are no Python version markers, return `None`. + if markers.iter().all(|range| { + let Some((lower, upper)) = range.bounding_range() else { + return true; + }; + matches!((lower, upper), (Bound::Unbounded, Bound::Unbounded)) + }) { + return None; + } + + // Take the union of the intersections of the Python version markers. let range = markers .into_iter() - .fold(None, |acc: Option>, range| { - Some(match acc { - Some(acc) => acc.union(&range), - None => range.clone(), - }) - })?; + .fold(Ranges::empty(), |acc: Ranges, range| { + acc.union(&range) + }); let (lower, upper) = range.bounding_range()?; @@ -66,3 +88,97 @@ pub(crate) fn requires_python(tree: MarkerTree) -> Option { UpperBound::new(upper.cloned()), )) } + +#[cfg(test)] +mod tests { + use std::ops::Bound; + use std::str::FromStr; + + use super::*; + + #[test] + fn test_requires_python() { + // An exact version match. + let tree = MarkerTree::from_str("python_full_version == '3.8.*'").unwrap(); + let range = requires_python(tree).unwrap(); + assert_eq!( + *range.lower(), + LowerBound::new(Bound::Included(Version::from_str("3.8").unwrap())) + ); + assert_eq!( + *range.upper(), + UpperBound::new(Bound::Excluded(Version::from_str("3.9").unwrap())) + ); + + // A version range with exclusive bounds. + let tree = + MarkerTree::from_str("python_full_version > '3.8' and python_full_version < '3.9'") + .unwrap(); + let range = requires_python(tree).unwrap(); + assert_eq!( + *range.lower(), + LowerBound::new(Bound::Excluded(Version::from_str("3.8").unwrap())) + ); + assert_eq!( + *range.upper(), + UpperBound::new(Bound::Excluded(Version::from_str("3.9").unwrap())) + ); + + // A version range with inclusive bounds. + let tree = + MarkerTree::from_str("python_full_version >= '3.8' and python_full_version <= '3.9'") + .unwrap(); + let range = requires_python(tree).unwrap(); + assert_eq!( + *range.lower(), + LowerBound::new(Bound::Included(Version::from_str("3.8").unwrap())) + ); + assert_eq!( + *range.upper(), + UpperBound::new(Bound::Included(Version::from_str("3.9").unwrap())) + ); + + // A version with a lower bound. + let tree = MarkerTree::from_str("python_full_version >= '3.8'").unwrap(); + let range = requires_python(tree).unwrap(); + assert_eq!( + *range.lower(), + LowerBound::new(Bound::Included(Version::from_str("3.8").unwrap())) + ); + assert_eq!(*range.upper(), UpperBound::new(Bound::Unbounded)); + + // A version with an upper bound. + let tree = MarkerTree::from_str("python_full_version < '3.9'").unwrap(); + let range = requires_python(tree).unwrap(); + assert_eq!(*range.lower(), LowerBound::new(Bound::Unbounded)); + assert_eq!( + *range.upper(), + UpperBound::new(Bound::Excluded(Version::from_str("3.9").unwrap())) + ); + + // A disjunction with a non-Python marker (i.e., an unbounded range). + let tree = + MarkerTree::from_str("python_full_version > '3.8' or sys_platform == 'win32'").unwrap(); + let range = requires_python(tree).unwrap(); + assert_eq!(*range.lower(), LowerBound::new(Bound::Unbounded)); + assert_eq!(*range.upper(), UpperBound::new(Bound::Unbounded)); + + // A complex mix of conjunctions and disjunctions. + let tree = MarkerTree::from_str("(python_full_version >= '3.8' and python_full_version < '3.9') or (python_full_version >= '3.10' and python_full_version < '3.11')").unwrap(); + let range = requires_python(tree).unwrap(); + assert_eq!( + *range.lower(), + LowerBound::new(Bound::Included(Version::from_str("3.8").unwrap())) + ); + assert_eq!( + *range.upper(), + UpperBound::new(Bound::Excluded(Version::from_str("3.11").unwrap())) + ); + + // An unbounded range across two specifiers. + let tree = + MarkerTree::from_str("python_full_version > '3.8' or python_full_version <= '3.8'") + .unwrap(); + assert_eq!(requires_python(tree), None); + } +} diff --git a/crates/uv/tests/it/pip_compile.rs b/crates/uv/tests/it/pip_compile.rs index fe459358e..b4deda256 100644 --- a/crates/uv/tests/it/pip_compile.rs +++ b/crates/uv/tests/it/pip_compile.rs @@ -14143,6 +14143,40 @@ fn compile_lowest_extra_unpinned_warning() -> Result<()> { Ok(()) } +#[test] +fn disjoint_requires_python() -> Result<()> { + let context = TestContext::new("3.8"); + + let requirements_in = context.temp_dir.child("requirements.in"); + requirements_in.write_str(indoc::indoc! {r" + iniconfig ; platform_python_implementation == 'CPython' and python_version >= '3.10' + coverage + "})?; + + uv_snapshot!(context.filters(), context.pip_compile() + .arg("--universal") + .arg(requirements_in.path()) + .env_remove(EnvVars::UV_EXCLUDE_NEWER), @r###" + success: true + exit_code: 0 + ----- stdout ----- + # This file was autogenerated by uv via the following command: + # uv pip compile --cache-dir [CACHE_DIR] --universal [TEMP_DIR]/requirements.in + coverage==7.6.1 ; python_full_version < '3.9' + # via -r requirements.in + coverage==7.6.10 ; python_full_version >= '3.9' + # via -r requirements.in + iniconfig==2.0.0 ; python_full_version >= '3.10' and platform_python_implementation == 'CPython' + # via -r requirements.in + + ----- stderr ----- + Resolved 3 packages in [TIME] + "### + ); + + Ok(()) +} + /// Test that we use the version in the source distribution filename for compiling, even if the /// version is declared as dynamic. ///