From 634f003e54482bcbf05010ce556fdf3dd79bca18 Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Fri, 26 Jul 2024 14:06:34 -0400 Subject: [PATCH] uv-resolver: fix basic case of overlapping markers Consider the following packse scenario: ```toml [root] requires = [ "a>=1.0.0 ; python_version < '3.10'", "a>=1.1.0 ; python_version >= '3.10'", "a>=1.2.0 ; python_version >= '3.11'", ] [packages.a.versions."1.0.0"] [packages.a.versions."1.1.0"] [packages.a.versions."1.2.0"] ``` On current `main`, this produces a dependency on `a` that looks like this: ```toml dependencies = [ { name = "fork-overlapping-markers-basic-a", marker = "python_version < '3.10' or python_version >= '3.11'" }, ] ``` But the marker expression is clearly wrong here, since it implies that `a` isn't installed at all for Python 3.10. With this PR, the above dependency becomes: ```toml dependencies = [ { name = "fork-overlapping-markers-basic-a" }, ] ``` That is, it's unconditional. Which is I believe correct here since there aren't any other constraints on which version to select. The specific bug here is that when we found overlapping dependency specifications for the same package *within* a pre-existing fork, we intersected all of their marker expressions instead of unioning them. That in turn resulted in incorrect marker expressions. While this doesn't fix any known bug on the issue tracker (like #4640), it does appear to fix a couple of our snapshot tests. And fixes a basic test case I came up with while working on #4732. For the packse scenario test: https://github.com/astral-sh/packse/pull/206 --- crates/uv-resolver/src/marker.rs | 5 ++ crates/uv-resolver/src/resolver/mod.rs | 80 ++++++++------------------ crates/uv/tests/lock.rs | 2 +- crates/uv/tests/pip_compile.rs | 12 ++-- 4 files changed, 35 insertions(+), 64 deletions(-) diff --git a/crates/uv-resolver/src/marker.rs b/crates/uv-resolver/src/marker.rs index 9d50173a28f3..e475f40ac670 100644 --- a/crates/uv-resolver/src/marker.rs +++ b/crates/uv-resolver/src/marker.rs @@ -192,6 +192,11 @@ pub(crate) fn requires_python_marker(tree: &MarkerTree) -> Option, diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs index 961147d3a3c3..4738f41e92e2 100644 --- a/crates/uv-resolver/src/resolver/mod.rs +++ b/crates/uv-resolver/src/resolver/mod.rs @@ -2217,7 +2217,7 @@ impl ForkState { fn with_markers(mut self, markers: MarkerTree) -> Self { let combined_markers = self.markers.and(markers); let combined_markers = - normalize(combined_markers, None).expect("Fork markers are universal"); + normalize(combined_markers, None).unwrap_or_else(|| MarkerTree::And(vec![])); // If the fork contains a narrowed Python requirement, apply it. let python_requirement = requires_python_marker(&combined_markers) @@ -2709,25 +2709,20 @@ impl Dependencies { let mut new_forks_for_remaining_universe = forks.clone(); for fork in &mut new_forks_for_remaining_universe { fork.markers.and(markers.clone()); - fork.dependencies.retain(|dep| { - let Some(dep_marker) = dep.package.marker() else { - return true; - }; - // After we constrain the markers on an existing - // fork, we should ensure that any existing - // dependencies that are no longer possible in this - // fork are removed. This mirrors the check we do in - // `add_nonfork_package`. - !crate::marker::is_disjoint(&fork.markers, dep_marker) - }); + fork.remove_disjoint_packages(); } new_forks.extend(new_forks_for_remaining_universe); } + // Each group has a list of packages whose marker expressions are + // guaranteed to be overlapping. So we must union those marker + // expressions and then intersect them with each existing fork. for group in fork_groups.forks { let mut new_forks_for_group = forks.clone(); - for (index, _) in group.packages { - for fork in &mut new_forks_for_group { - fork.add_forked_package(deps[index].clone()); + for fork in &mut new_forks_for_group { + fork.markers.and(group.union()); + fork.remove_disjoint_packages(); + for &(index, _) in &group.packages { + fork.dependencies.push(deps[index].clone()); } } new_forks.extend(new_forks_for_group); @@ -2802,39 +2797,6 @@ struct Fork { } impl Fork { - /// Add the given dependency to this fork with the assumption that it - /// provoked this fork into existence. - /// - /// In particular, the markers given should correspond to the markers - /// associated with that dependency, and they are combined (via - /// conjunction) with the markers on this fork. - /// - /// Finally, and critically, any dependencies that are already in this - /// fork that are disjoint with the markers given are removed. This is - /// because a fork provoked by the given marker should not have packages - /// whose markers are disjoint with it. While it might seem harmless, this - /// can cause the resolver to explore otherwise impossible resolutions, - /// and also run into conflicts (and thus a failed resolution) that don't - /// actually exist. - fn add_forked_package(&mut self, dependency: PubGrubDependency) { - // OK because a package without a marker is unconditional and - // thus can never provoke a fork. - let marker = dependency - .package - .marker() - .cloned() - .expect("forked package always has a marker"); - self.remove_disjoint_packages(&marker); - self.dependencies.push(dependency); - // Each marker expression in a single fork is, - // by construction, overlapping with at least - // one other marker expression in this fork. - // However, the symmetric differences may be - // non-empty. Therefore, the markers need to be - // combined on the corresponding fork. - self.markers.and(marker); - } - /// Add the given dependency to this fork. /// /// This works by assuming the given package did *not* provoke a fork. @@ -2854,15 +2816,15 @@ impl Fork { } /// Removes any dependencies in this fork whose markers are disjoint with - /// the given markers. - fn remove_disjoint_packages(&mut self, fork_marker: &MarkerTree) { + /// its own markers. + fn remove_disjoint_packages(&mut self) { use crate::marker::is_disjoint; self.dependencies.retain(|dependency| { dependency .package .marker() - .map_or(true, |pkg_marker| !is_disjoint(pkg_marker, fork_marker)) + .map_or(true, |pkg_marker| !is_disjoint(pkg_marker, &self.markers)) }); } @@ -3052,12 +3014,16 @@ impl<'a> PossibleFork<'a> { /// 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(), - ) + let mut trees: Vec = self + .packages + .iter() + .map(|&(_, tree)| (*tree).clone()) + .collect(); + if trees.len() == 1 { + trees.pop().unwrap() + } else { + MarkerTree::Or(trees) + } } } diff --git a/crates/uv/tests/lock.rs b/crates/uv/tests/lock.rs index 0fcc3a58a9e8..fecc6d634cce 100644 --- a/crates/uv/tests/lock.rs +++ b/crates/uv/tests/lock.rs @@ -2902,7 +2902,7 @@ fn lock_python_version_marker_complement() -> Result<()> { source = { editable = "." } dependencies = [ { name = "attrs", marker = "(python_full_version <= '3.10' and python_version == '3.10') or (python_full_version <= '3.10' and python_version < '3.10') or (python_full_version <= '3.10' and python_version > '3.10') or (python_full_version > '3.10' and python_version == '3.10') or (python_full_version > '3.10' and python_version < '3.10') or (python_full_version > '3.10' and python_version > '3.10')" }, - { name = "iniconfig", marker = "(python_full_version <= '3.10' and python_version == '3.10') or (python_full_version <= '3.10' and python_version < '3.10') or (python_full_version <= '3.10' and python_version < '3.10' and python_version > '3.10') or (python_full_version <= '3.10' and python_version > '3.10') or (python_full_version > '3.10' and python_version == '3.10') or (python_full_version > '3.10' and python_version < '3.10') or (python_full_version > '3.10' and python_version < '3.10' and python_version > '3.10') or (python_full_version > '3.10' and python_version > '3.10')" }, + { name = "iniconfig", marker = "(python_full_version <= '3.10' and python_version == '3.10') or (python_full_version <= '3.10' and python_version < '3.10') or (python_full_version <= '3.10' and python_version > '3.10') or (python_full_version > '3.10' and python_version == '3.10') or (python_full_version > '3.10' and python_version < '3.10') or (python_full_version > '3.10' and python_version > '3.10')" }, { name = "typing-extensions", marker = "(python_full_version <= '3.10' and python_version == '3.10') or (python_full_version <= '3.10' and python_version < '3.10') or (python_full_version <= '3.10' and python_version < '3.10' and python_version > '3.10') or (python_full_version <= '3.10' and python_version > '3.10') or (python_full_version > '3.10' and python_version == '3.10') or (python_full_version > '3.10' and python_version < '3.10') or (python_full_version > '3.10' and python_version < '3.10' and python_version > '3.10') or (python_full_version > '3.10' and python_version > '3.10')" }, ] diff --git a/crates/uv/tests/pip_compile.rs b/crates/uv/tests/pip_compile.rs index 2fc3b5072fc0..f8dbbd9f21b4 100644 --- a/crates/uv/tests/pip_compile.rs +++ b/crates/uv/tests/pip_compile.rs @@ -7766,7 +7766,7 @@ fn universal_no_repeated_unconditional_distributions() -> Result<()> { # uv pip compile --cache-dir [CACHE_DIR] requirements.in -p 3.8 --universal alabaster==0.7.13 # via sphinx - astroid==3.1.0 ; python_version < '3.11' or python_version >= '3.12' + astroid==3.1.0 # via pylint babel==2.14.0 # via sphinx @@ -7778,7 +7778,7 @@ fn universal_no_repeated_unconditional_distributions() -> Result<()> { # via # pylint # sphinx - dill==0.3.8 ; python_version < '3.11' or python_version >= '3.12' + dill==0.3.8 # via pylint docutils==0.20.1 # via sphinx @@ -7788,17 +7788,17 @@ fn universal_no_repeated_unconditional_distributions() -> Result<()> { # via sphinx importlib-metadata==7.1.0 ; python_version < '3.10' # via sphinx - isort==5.13.2 ; python_version < '3.11' or python_version >= '3.12' + isort==5.13.2 # via pylint jinja2==3.1.3 # via sphinx markupsafe==2.1.5 # via jinja2 - mccabe==0.7.0 ; python_version < '3.11' or python_version >= '3.12' + mccabe==0.7.0 # via pylint packaging==24.0 # via sphinx - platformdirs==4.2.0 ; python_version < '3.11' or python_version >= '3.12' + platformdirs==4.2.0 # via pylint pygments==2.17.2 # via sphinx @@ -7826,7 +7826,7 @@ fn universal_no_repeated_unconditional_distributions() -> Result<()> { # via sphinx tomli==2.0.1 ; python_version < '3.11' # via pylint - tomlkit==0.12.4 ; python_version < '3.11' or python_version >= '3.12' + tomlkit==0.12.4 # via pylint typing-extensions==4.10.0 ; python_version < '3.11' # via