mirrors/uv
1
0
Fork 0
mirror of https://github.com/astral-sh/uv.git synced 2025-07-07 21:35:00 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

uv-resolver: filter out sibling dependencies in a fork

Browse source 
When a fork is created from a list of dependencies, we were previously
adding all other sibling dependencies to every fork created. But this
isn't actually quite right, since the fork created is always created by
some marker expression. And while it is definitively disjoint from any
directly conflicting dependency specification, it is also possibly
disjoint with other dependencies. For example, as reported in #4414:

```toml
dependencies = [
  "anyio==4.4.0 ; python_version >= '3.12'",
  "anyio==4.3.0 ; python_version < '3.12'",
  "b1 ; python_version >= '3.12'",
  "b2 ; python_version < '3.12'",
]
```

The first two `anyio` requirements are conflicting with non-overlapping
marker expressions, and so a fork is created. Prior to this commit,
*both* `b1` and `b2` would be added to each fork. But of course, `b2` is
impossible in the `anyio==4.4.0` fork because of disjoint marker
expressions.

So in this commit, we specifically filter out any sibling dependencies
that could find their way into a fork that have disjoint markers with
that fork. We are careful to do this both when a new fork is created
from an existing set of dependencies, and when adding new dependencies
to a fork.

Fixes #4414
This commit is contained in:
Andrew Gallant 2024-06-19 11:03:17 -04:00 • committed by Andrew Gallant
parent 34c7bc5cc8
commit 9595a511cd
2 changed files with 125 additions and 20 deletions
Download patch file Download diff file Expand all files Collapse all files

28
crates/uv-resolver/src/pubgrub/package.rs
View file

@ -201,6 +201,34 @@ impl PubGrubPackage {
}))
}
}
/// Returns the name of this PubGrub package, if it has one.
pub(crate) fn name(&self) -> Option<&PackageName> {
match &**self {
// A root can never be a dependency of another package, and a `Python` pubgrub
// package is never returned by `get_dependencies`. So these cases never occur.
PubGrubPackageInner::Root(None) | PubGrubPackageInner::Python(_) => None,
PubGrubPackageInner::Root(Some(name))
| PubGrubPackageInner::Package { name, .. }
| PubGrubPackageInner::Extra { name, .. }
| PubGrubPackageInner::Dev { name, .. }
| PubGrubPackageInner::Marker { name, .. } => Some(name),
}
}
/// Returns the marker expression associated with this PubGrub package, if
/// it has one.
pub(crate) fn marker(&self) -> Option<&MarkerTree> {
match &**self {
// A root can never be a dependency of another package, and a `Python` pubgrub
// package is never returned by `get_dependencies`. So these cases never occur.
PubGrubPackageInner::Root(_) | PubGrubPackageInner::Python(_) => None,
PubGrubPackageInner::Package { marker, .. }
| PubGrubPackageInner::Extra { marker, .. }
| PubGrubPackageInner::Dev { marker, .. } => marker.as_ref(),
PubGrubPackageInner::Marker { marker, .. } => Some(marker),
}
}
}
#[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Hash, Ord)]

117
crates/uv-resolver/src/resolver/mod.rs
View file

@ -1903,15 +1903,12 @@ impl Dependencies {
let mut by_name: FxHashMap<&PackageName, PossibleForks> = FxHashMap::default();
for (index, (ref pkg, _)) in deps.iter().enumerate() {
let (name, marker) = match &**pkg {
// A root can never be a dependency of another package, and a `Python` pubgrub
// package is never returned by `get_dependencies`. So these cases never occur.
PubGrubPackageInner::Root(_) | PubGrubPackageInner::Python(_) => unreachable!(),
PubGrubPackageInner::Package { name, marker, .. }
| PubGrubPackageInner::Extra { name, marker, .. }
| PubGrubPackageInner::Dev { name, marker, .. } => (name, marker.as_ref()),
PubGrubPackageInner::Marker { name, marker, .. } => (name, Some(marker)),
};
// A root can never be a dependency of another package,
// and a `Python` pubgrub package is never returned by
// `get_dependencies`. So a pubgrub package always has a
// name in this context.
let name = pkg.name().expect("dependency always has a name");
let marker = pkg.marker();
let Some(marker) = marker else {
// When no marker is found, it implies there is a dependency on
// this package that is unconditional with respect to marker
@ -1991,32 +1988,28 @@ impl Dependencies {
}];
let mut diverging_packages = Vec::new();
for (name, possible_forks) in by_name {
let fork_groups = match possible_forks {
let fork_groups = match possible_forks.finish() {
// 'finish()' guarantees that 'PossiblyForking' implies
// 'DefinitelyForking'.
PossibleForks::PossiblyForking(fork_groups) => fork_groups,
PossibleForks::NoForkPossible(indices) => {
// No fork is provoked by this package, so just add
// everything in this group to each of the forks.
for index in indices {
for fork in &mut forks {
fork.dependencies.push(deps[index].clone());
fork.add_nonfork_package(deps[index].clone());
}
}
continue;
}
};
assert!(fork_groups.forks.len() >= 2, "expected definitive fork");
let mut new_forks: Vec<Fork> = vec![];
for group in fork_groups.forks {
let mut new_forks_for_group = forks.clone();
for (index, markers) in group.packages {
for (index, _) in group.packages {
for fork in &mut new_forks_for_group {
fork.dependencies.push(deps[index].clone());
// 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
fork.markers.and(markers.clone());
fork.add_forked_package(deps[index].clone());
}
}
new_forks.extend(new_forks_for_group);
@ -2071,6 +2064,11 @@ struct Fork {
/// The list of dependencies for this fork, guaranteed to be conflict
/// free. (i.e., There are no two packages with the same name with
/// non-overlapping marker expressions.)
///
/// Note that callers shouldn't mutate this sequence directly. Instead,
/// they should use `add_forked_package` or `add_nonfork_package`. Namely,
/// it should be impossible for a package with a marker expression that is
/// disjoint from the marker expression on this fork to be added.
dependencies: Vec<(PubGrubPackage, Range<Version>)>,
/// The markers that provoked this fork.
///
@ -2082,6 +2080,68 @@ struct Fork {
markers: MarkerTree,
}
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, (pkg, range): (PubGrubPackage, Range<Version>)) {
// OK because a package without a marker is unconditional and
// thus can never provoke a fork.
let marker = pkg
.marker()
.cloned()
.expect("forked package always has a marker");
self.remove_disjoint_packages(&marker);
self.dependencies.push((pkg, range));
// 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.
///
/// It is only added if the markers on the given package are not disjoint
/// with this fork's markers.
fn add_nonfork_package(&mut self, (pkg, range): (PubGrubPackage, Range<Version>)) {
use crate::marker::is_disjoint;
if pkg
.marker()
.map_or(true, |marker| !is_disjoint(marker, &self.markers))
{
self.dependencies.push((pkg, range));
}
}
/// Removes any dependencies in this fork whose markers are disjoint with
/// the given markers.
fn remove_disjoint_packages(&mut self, fork_marker: &MarkerTree) {
use crate::marker::is_disjoint;
self.dependencies.retain(|(pkg, _)| {
pkg.marker()
.map_or(true, |pkg_marker| !is_disjoint(pkg_marker, fork_marker))
});
}
}
/// Intermediate state that represents a *possible* grouping of forks
/// for one package name.
///
@ -2120,6 +2180,23 @@ impl<'a> PossibleForks<'a> {
fork_groups.forks.len() > 1
}
/// Consumes this possible set of forks and converts a "possibly forking"
/// variant to a "no fork possible" variant if there are no actual forks.
///
/// This should be called when all dependencies for one package have been
/// considered. It will normalize this value such that `PossiblyForking`
/// means `DefinitelyForking`.
fn finish(mut self) -> PossibleForks<'a> {
let PossibleForks::PossiblyForking(ref fork_groups) = self else {
return self;
};
if fork_groups.forks.len() == 1 {
self.make_no_forks_possible();
return self;
}
self
}
/// Pushes an unconditional index to a package.
///
/// If this previously contained possible forks, those are combined into