From 563507edba60735ec82f9df18db1417dbeae8a6c Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Fri, 12 Jul 2024 11:55:40 -0400 Subject: [PATCH] uv-resolver: add support for incomplete markers In some cases, it's possible for the marker expressions on conflicting dependency specification to be disjoint but *incomplete*. That is, if one unions the disjoint markers, the result is not the complete set of marker environments possible. There may be some "gap" of marker environments not covered by the markers. This is a problem in practice because, before this commit, we only created forks in the resolver for specific marker expressions. So if a dependency happened to fall in a "gap," our resolver would never see it. This commit fixes this by adding a new split covering the negation of the union of all marker expressions in a set of forks for a specific package. Originally, I had planned on only creating this split when it was known that the gap actually existed. That is, when the negation of the marker expressions did *not* correspond to the empty set. After a lot of thought, unfortunately, this (I believe) effectively boils down to 3SAT, which is NP-complete. Instead, what we do here is *always* create an extra split unless we can definitively tell that it is empty. We look for a few cases, but otherwise throw our hands up and potentially do wasted work. This also updates the lock scenario tests to reflect the actual bug fix here. --- crates/pep508-rs/src/marker.rs | 6 +++ crates/uv-resolver/src/marker.rs | 67 ++++++++++++++++++++++++++ crates/uv-resolver/src/resolver/mod.rs | 36 ++++++++++++++ crates/uv/tests/lock_scenarios.rs | 14 +++++- 4 files changed, 122 insertions(+), 1 deletion(-) diff --git a/crates/pep508-rs/src/marker.rs b/crates/pep508-rs/src/marker.rs index 809fb758b..9115b3058 100644 --- a/crates/pep508-rs/src/marker.rs +++ b/crates/pep508-rs/src/marker.rs @@ -2515,6 +2515,12 @@ mod test { neg("os_name == 'bar' or os_name == 'foo'"), "os_name != 'bar' and os_name != 'foo'" ); + + // Always true negates to always false! + assert_eq!( + neg("python_version >= '3.6' or python_version < '3.6'"), + "python_version < '3.6' and python_version >= '3.6'" + ); } #[test] diff --git a/crates/uv-resolver/src/marker.rs b/crates/uv-resolver/src/marker.rs index c36547cfd..d2e738e88 100644 --- a/crates/uv-resolver/src/marker.rs +++ b/crates/uv-resolver/src/marker.rs @@ -15,6 +15,47 @@ use pep508_rs::{ use crate::pubgrub::PubGrubSpecifier; use crate::RequiresPythonBound; +/// Returns true when it can be proven that the given marker expression +/// evaluates to true for precisely zero marker environments. +/// +/// When this returns false, it *may* be the case that is evaluates to +/// true for precisely zero marker environments. That is, this routine +/// never has false positives but may have false negatives. +pub(crate) fn is_definitively_empty_set(tree: &MarkerTree) -> bool { + match *tree { + // A conjunction is definitively empty when it is known that + // *any* two of its conjuncts are disjoint. Since this would + // imply that the entire conjunction could never be true. + MarkerTree::And(ref trees) => { + // Since this is quadratic in the case where the + // expression is *not* empty, we limit ourselves + // to a small number of conjuncts. In practice, + // this should hopefully cover most cases. + if trees.len() > 10 { + return false; + } + for (i, tree1) in trees.iter().enumerate() { + for tree2 in &trees[i..] { + if is_disjoint(tree1, tree2) { + return true; + } + } + } + false + } + // A disjunction is definitively empty when all of its + // disjuncts are definitively empty. + MarkerTree::Or(ref trees) => trees.iter().all(is_definitively_empty_set), + // An "arbitrary" expression is always false, so we + // at least know it is definitively empty. + MarkerTree::Expression(MarkerExpression::Arbitrary { .. }) => true, + // Can't really do much with a single expression. There are maybe + // trivial cases we could check (like `python_version < '0'`), but I'm + // not sure it's worth doing? + MarkerTree::Expression(_) => false, + } +} + /// Returns `true` if there is no environment in which both marker trees can both apply, i.e. /// the expression `first and second` is always false. pub(crate) fn is_disjoint(first: &MarkerTree, second: &MarkerTree) -> bool { @@ -1079,6 +1120,27 @@ mod tests { )); } + #[test] + fn is_definitively_empty_set() { + assert!(is_empty("'wat' == 'wat'")); + assert!(is_empty( + "python_version < '3.10' and python_version >= '3.10'" + )); + assert!(is_empty( + "(python_version < '3.10' and python_version >= '3.10') \ + or (python_version < '3.9' and python_version >= '3.9')", + )); + + assert!(!is_empty("python_version < '3.10'")); + assert!(!is_empty("python_version < '0'")); + assert!(!is_empty( + "python_version < '3.10' and python_version >= '3.9'" + )); + assert!(!is_empty( + "python_version < '3.10' or python_version >= '3.11'" + )); + } + fn test_version_bounds_disjointness(version: &str) { assert!(!is_disjoint( format!("{version} > '2.7.0'"), @@ -1125,6 +1187,11 @@ mod tests { )); } + fn is_empty(tree: &str) -> bool { + let tree = MarkerTree::parse_reporter(tree, &mut TracingReporter).unwrap(); + super::is_definitively_empty_set(&tree) + } + fn is_disjoint(one: impl AsRef, two: impl AsRef) -> bool { let one = MarkerTree::parse_reporter(one.as_ref(), &mut TracingReporter).unwrap(); let two = MarkerTree::parse_reporter(two.as_ref(), &mut TracingReporter).unwrap(); diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs index 43fe644d3..89640845b 100644 --- a/crates/uv-resolver/src/resolver/mod.rs +++ b/crates/uv-resolver/src/resolver/mod.rs @@ -2476,6 +2476,13 @@ impl Dependencies { }; assert!(fork_groups.forks.len() >= 2, "expected definitive fork"); let mut new_forks: Vec = vec![]; + if let Some(markers) = fork_groups.remaining_universe() { + debug!("Adding split to cover possibly incomplete markers: {markers}"); + new_forks.push(Fork { + dependencies: vec![], + markers, + }); + } for group in fork_groups.forks { let mut new_forks_for_group = forks.clone(); for (index, _) in group.packages { @@ -2730,6 +2737,22 @@ impl<'a> PossibleForkGroups<'a> { .iter_mut() .find(|fork| fork.is_overlapping(marker)) } + + /// Returns a marker tree corresponding to the set of marker expressions + /// outside of this fork group. + /// + /// In many cases, it can be easily known that the set of marker + /// expressions referred to by this marker tree is empty. In this case, + /// `None` is returned. But note that if a marker tree is returned, it is + /// still possible for it to describe exactly zero marker environments. + fn remaining_universe(&self) -> Option { + let have = MarkerTree::Or(self.forks.iter().map(PossibleFork::union).collect()); + let missing = have.negate(); + if crate::marker::is_definitively_empty_set(&missing) { + return None; + } + Some(missing) + } } /// Intermediate state representing a single possible fork. @@ -2765,6 +2788,19 @@ impl<'a> PossibleFork<'a> { } false } + + /// Returns the union of all the marker expressions in this possible fork. + /// + /// Each marker expression in the union returned is guaranteed to be overlapping + /// with at least one other expression in the same union. + fn union(&self) -> MarkerTree { + MarkerTree::Or( + self.packages + .iter() + .map(|&(_, tree)| (*tree).clone()) + .collect(), + ) + } } /// Returns true if and only if the given requirement's marker expression has a diff --git a/crates/uv/tests/lock_scenarios.rs b/crates/uv/tests/lock_scenarios.rs index 070df62eb..5840b1a0d 100644 --- a/crates/uv/tests/lock_scenarios.rs +++ b/crates/uv/tests/lock_scenarios.rs @@ -664,7 +664,7 @@ fn fork_incomplete_markers() -> Result<()> { ----- stderr ----- warning: `uv lock` is experimental and may change without warning. - Resolved 4 packages in [TIME] + Resolved 5 packages in [TIME] "### ); @@ -699,11 +699,23 @@ fn fork_incomplete_markers() -> Result<()> { name = "package-b" version = "1.0.0" source = { registry = "https://astral-sh.github.io/packse/0.3.30/simple-html/" } + dependencies = [ + { name = "package-c", marker = "python_version == '3.10'" }, + ] sdist = { url = "https://astral-sh.github.io/packse/0.3.30/files/fork_incomplete_markers_b-1.0.0.tar.gz#sha256=c4deba44768923473d077bdc0e177033fcb6e6fd406d56830d7ee6f4ffad68c1", hash = "sha256:c4deba44768923473d077bdc0e177033fcb6e6fd406d56830d7ee6f4ffad68c1" } wheels = [ { url = "https://astral-sh.github.io/packse/0.3.30/files/fork_incomplete_markers_b-1.0.0-py3-none-any.whl#sha256=5c2a5f446580787ed7b3673431b112474237ddeaf1c81325bb30b86e7ee76adb", hash = "sha256:5c2a5f446580787ed7b3673431b112474237ddeaf1c81325bb30b86e7ee76adb" }, ] + [[distribution]] + name = "package-c" + version = "1.0.0" + source = { registry = "https://astral-sh.github.io/packse/0.3.30/simple-html/" } + sdist = { url = "https://astral-sh.github.io/packse/0.3.30/files/fork_incomplete_markers_c-1.0.0.tar.gz#sha256=ecc02ea1cc8d3b561c8dcb9d2ba1abcdae2dd32de608bf8e8ed2878118426022", hash = "sha256:ecc02ea1cc8d3b561c8dcb9d2ba1abcdae2dd32de608bf8e8ed2878118426022" } + wheels = [ + { url = "https://astral-sh.github.io/packse/0.3.30/files/fork_incomplete_markers_c-1.0.0-py3-none-any.whl#sha256=03fa287aa4cb78457211cb3df7459b99ba1ee2259aae24bc745eaab45e7eaaee", hash = "sha256:03fa287aa4cb78457211cb3df7459b99ba1ee2259aae24bc745eaab45e7eaaee" }, + ] + [[distribution]] name = "project" version = "0.1.0"