From f7bed37a4e061695890d94f36be20a7baef9c20c Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Fri, 22 Nov 2024 11:58:47 -0500 Subject: [PATCH] uv-resolver: add "include" rules to `ResolverEnvironment` When we generate conflict markers for each resolution after the resolver runs, it turns out that generating them just from exclusion rules is not sufficient. For example, if `foo` and `bar` are declared as conflicting extras, then we end up with the following forks: A: extra != 'foo' B: extra != 'bar' C: extra != 'foo' and extra != 'bar' Now let's take an example where these forks don't share the same version for all packages. Consider a case where `idna==3.9` is in forks A and C, but where `idna==3.10` is in fork B. If we combine the markers in forks A and C through disjunction, we get the following: idna==3.9: extra != 'foo' or (extra != 'foo' and extra != 'bar') idna==3.10: extra != 'bar' Which simplifies to: idna==3.9: extra != 'foo' idna==3.10: extra != 'bar' But these are clearly not disjoint. Both dependencies could be selected, for example, when neither `foo` nor `bar` are active. We can remedy this by keeping around the inclusion rules for each fork: A: extra != 'foo' and extra == 'bar' B: extra != 'bar' and extra == 'foo' C: extra != 'foo' and extra != 'bar' And so for `idna`, we have: idna==3.9: (extra != 'foo' and extra == 'bar') or (extra != 'foo' and extra != 'bar') idna==3.10: extra != 'bar' and extra == 'foo' Which simplifies to: idna==3.9: extra != 'foo' idna==3.10: extra != 'bar' and extra == 'foo' And these *are* properly disjoint. There is no way for them both to be active. This also correctly accounts for fork C where neither `foo` nor `bar` are active, and yet, `idna==3.9` is still enabled but `idna==3.10` is not. (In the [motivating example], this comes from `baz` being enabled.) That is, this captures the idea that for `idna==3.10` to be installed, there must actually be a specific extra that is enabled. That's what makes it disjoint from `idna==3.9`. We aren't quite done yet, because this does add *too many* conflict markers to dependency edges that don't need it. In the next commit, we'll add in our world knowledge to simplify these conflict markers. [motivating example]: https://github.com/astral-sh/uv/issues/9289 --- .../uv-resolver/src/resolver/environment.rs | 88 ++++++++++++++++--- crates/uv-resolver/src/resolver/mod.rs | 39 ++++---- 2 files changed, 100 insertions(+), 27 deletions(-) diff --git a/crates/uv-resolver/src/resolver/environment.rs b/crates/uv-resolver/src/resolver/environment.rs index f1fef15c4..b9ba08baa 100644 --- a/crates/uv-resolver/src/resolver/environment.rs +++ b/crates/uv-resolver/src/resolver/environment.rs @@ -95,6 +95,20 @@ enum Kind { initial_forks: Arc<[MarkerTree]>, /// The markers associated with this resolver fork. markers: MarkerTree, + /// Conflicting group inclusions. + /// + /// Note that inclusions don't play a role in predicates + /// like `ResolverEnvironment::included_by_group`. Instead, + /// only exclusions are considered. + /// + /// We record inclusions for two reasons. First is that if + /// we somehow wind up with an inclusion and exclusion rule + /// for the same conflict item, then we treat the resulting + /// fork as impossible. (You cannot require that an extra is + /// both included and excluded. Such a rule can never be + /// satisfied.) Second is that we use the inclusion rules to + /// write conflict markers after resolution is finished. + include: Arc>, /// Conflicting group exclusions. exclude: Arc>, }, @@ -134,6 +148,7 @@ impl ResolverEnvironment { let kind = Kind::Universal { initial_forks: initial_forks.into(), markers: MarkerTree::TRUE, + include: Arc::new(crate::FxHashbrownSet::default()), exclude: Arc::new(crate::FxHashbrownSet::default()), }; ResolverEnvironment { kind } @@ -201,6 +216,7 @@ impl ResolverEnvironment { Kind::Universal { ref initial_forks, markers: ref lhs, + ref include, ref exclude, } => { let mut markers = lhs.clone(); @@ -208,6 +224,7 @@ impl ResolverEnvironment { let kind = Kind::Universal { initial_forks: Arc::clone(initial_forks), markers, + include: Arc::clone(include), exclude: Arc::clone(exclude), }; ResolverEnvironment { kind } @@ -215,22 +232,29 @@ impl ResolverEnvironment { } } - /// Returns a new resolver environment with the given groups excluded from - /// it. + /// Returns a new resolver environment with the given groups included or + /// excluded from it. An `Ok` variant indicates an include rule while an + /// `Err` variant indicates en exclude rule. /// /// When a group is excluded from a resolver environment, /// `ResolverEnvironment::included_by_group` will return false. The idea /// is that a dependency with a corresponding group should be excluded by - /// forks in the resolver with this environment. + /// forks in the resolver with this environment. (Include rules have no + /// effect in `included_by_group` since, for the purposes of conflicts + /// during resolution, we only care about what *isn't* allowed.) + /// + /// If calling this routine results in the same conflict item being both + /// included and excluded, then this returns `None` (since it would + /// otherwise result in a fork that can never be satisfied). /// /// # Panics /// /// This panics if the resolver environment corresponds to one and only one /// specific marker environment. i.e., "pip"-style resolution. - pub(crate) fn exclude_by_group( + pub(crate) fn filter_by_group( &self, - items: impl IntoIterator, - ) -> ResolverEnvironment { + rules: impl IntoIterator>, + ) -> Option { match self.kind { Kind::Specific { .. } => { unreachable!("environment narrowing only happens in universal resolution") @@ -238,18 +262,34 @@ impl ResolverEnvironment { Kind::Universal { ref initial_forks, ref markers, + ref include, ref exclude, } => { + let mut include: crate::FxHashbrownSet<_> = (**include).clone(); let mut exclude: crate::FxHashbrownSet<_> = (**exclude).clone(); - for item in items { - exclude.insert(item); + for rule in rules { + match rule { + Ok(item) => { + if exclude.contains(&item) { + return None; + } + include.insert(item); + } + Err(item) => { + if include.contains(&item) { + return None; + } + exclude.insert(item); + } + } } let kind = Kind::Universal { initial_forks: Arc::clone(initial_forks), markers: markers.clone(), + include: Arc::new(include), exclude: Arc::new(exclude), }; - ResolverEnvironment { kind } + Some(ResolverEnvironment { kind }) } } } @@ -266,6 +306,7 @@ impl ResolverEnvironment { let Kind::Universal { ref initial_forks, markers: ref _markers, + include: ref _include, exclude: ref _exclude, } = self.kind else { @@ -332,9 +373,32 @@ impl ResolverEnvironment { pub(crate) fn try_universal_markers(&self) -> Option { match self.kind { Kind::Specific { .. } => None, - Kind::Universal { ref markers, .. } => { - // FIXME: Support conflicts. - Some(UniversalMarker::new(markers.clone(), MarkerTree::TRUE)) + Kind::Universal { + ref markers, + ref include, + ref exclude, + .. + } => { + let mut conflict_marker = MarkerTree::TRUE; + for item in exclude.iter() { + if let Some(extra) = item.extra() { + let operator = uv_pep508::ExtraOperator::NotEqual; + let name = uv_pep508::MarkerValueExtra::Extra(extra.clone()); + let expr = uv_pep508::MarkerExpression::Extra { operator, name }; + let exclude_extra_marker = MarkerTree::expression(expr); + conflict_marker.and(exclude_extra_marker); + } + } + for item in include.iter() { + if let Some(extra) = item.extra() { + let operator = uv_pep508::ExtraOperator::Equal; + let name = uv_pep508::MarkerValueExtra::Extra(extra.clone()); + let expr = uv_pep508::MarkerExpression::Extra { operator, name }; + let exclude_extra_marker = MarkerTree::expression(expr); + conflict_marker.and(exclude_extra_marker); + } + } + Some(UniversalMarker::new(markers.clone(), conflict_marker)) } } } diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs index 09b08debe..f4e0660ff 100644 --- a/crates/uv-resolver/src/resolver/mod.rs +++ b/crates/uv-resolver/src/resolver/mod.rs @@ -2941,11 +2941,9 @@ impl Forks { } // Create a fork that excludes ALL extras. - let mut fork_none = fork.clone(); - for group in set.iter() { - fork_none = fork_none.exclude([group.clone()]); + if let Some(fork_none) = fork.clone().filter(set.iter().cloned().map(Err)) { + new.push(fork_none); } - new.push(fork_none); // Now create a fork for each conflicting group, where // that fork excludes every *other* conflicting group. @@ -2955,13 +2953,18 @@ impl Forks { // {foo, bar}, one that excludes {foo, baz} and one // that excludes {bar, baz}. for (i, _) in set.iter().enumerate() { - let fork_allows_group = fork.clone().exclude( - set.iter() - .enumerate() - .filter(|&(j, _)| i != j) - .map(|(_, group)| group.clone()), - ); - new.push(fork_allows_group); + let fork_allows_group = + fork.clone() + .filter(set.iter().cloned().enumerate().map(|(j, group)| { + if i == j { + Ok(group) + } else { + Err(group) + } + })); + if let Some(fork_allows_group) = fork_allows_group { + new.push(fork_allows_group); + } } } forks = new; @@ -3058,11 +3061,17 @@ impl Fork { self.conflicts.contains(&item) } - /// Exclude the given groups from this fork. + /// Include or Exclude the given groups from this fork. /// /// This removes all dependencies matching the given conflicting groups. - fn exclude(mut self, groups: impl IntoIterator) -> Fork { - self.env = self.env.exclude_by_group(groups); + /// + /// If the exclusion rules would result in a fork with an unsatisfiable + /// resolver environment, then this returns `None`. + fn filter( + mut self, + rules: impl IntoIterator>, + ) -> Option { + self.env = self.env.filter_by_group(rules)?; self.dependencies.retain(|dep| { let Some(conflicting_item) = dep.package.conflicting_item() else { return true; @@ -3075,7 +3084,7 @@ impl Fork { } false }); - self + Some(self) } }