Skip to content

Commit

Permalink
uv-resolver: fix basic case of overlapping markers
Browse files Browse the repository at this point in the history 
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: astral-sh/packse#206
  • Loading branch information
@BurntSushi
BurntSushi committed Jul 26, 2024
1 parent 6901a14 commit 634f003
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 64 deletions.
5 changes: 5 additions & 0 deletions crates/uv-resolver/src/marker.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,11 @@ pub(crate) fn requires_python_marker(tree: &MarkerTree) -> Option<RequiresPython
///
/// This is useful in cases where creating conjunctions or disjunctions might occur in a non-deterministic
/// order. This routine will attempt to erase the distinction created by such a construction.
///
/// This returns `None` in the case of normalization turning a `MarkerTree`
/// into an expression that is known to match all possible marker
/// environments. Note though that this may return an empty disjunction, e.g.,
/// `MarkerTree::Or(vec![])`, which matches precisely zero marker environments.
pub(crate) fn normalize(
mut tree: MarkerTree,
bound: Option<&RequiresPythonBound>,
Expand Down
80 changes: 23 additions & 57 deletions crates/uv-resolver/src/resolver/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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.
Expand All @@ -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))
});
}

Expand Down Expand Up @@ -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<MarkerTree> = self
.packages
.iter()
.map(|&(_, tree)| (*tree).clone())
.collect();
if trees.len() == 1 {
trees.pop().unwrap()
} else {
MarkerTree::Or(trees)
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/uv/tests/lock.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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')" },
]
Expand Down
12 changes: 6 additions & 6 deletions crates/uv/tests/pip_compile.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 634f003

Please sign in to comment.