diff --git a/crates/uv-pep508/src/marker/tree.rs b/crates/uv-pep508/src/marker/tree.rs index e312022d9abf..9f438c058d17 100644 --- a/crates/uv-pep508/src/marker/tree.rs +++ b/crates/uv-pep508/src/marker/tree.rs @@ -702,6 +702,19 @@ impl MarkerTree { self.0 = INTERNER.lock().or(self.0, tree.0); } + /// Sets this to a marker equivalent to the implication of this one and the + /// given consequent. + /// + /// If the marker set is always `true`, then it can be said that `self` + /// implies `consequent`. + #[allow(clippy::needless_pass_by_value)] + pub fn implies(&mut self, consequent: MarkerTree) { + // This could probably be optimized, but is clearly + // correct, since logical implication is `-P or Q`. + *self = self.negate(); + self.or(consequent); + } + /// Returns `true` if there is no environment in which both marker trees can apply, /// i.e. their conjunction is always `false`. /// @@ -2484,6 +2497,47 @@ mod test { ); } + /// This tests marker implication. + /// + /// Specifically, these test cases come from a [bug] where `foo` and `bar` + /// are conflicting extras, but results in an ambiguous lock file. This + /// test takes `not(extra == 'foo' and extra == 'bar')` as world knowledge. + /// That is, since they are declared as conflicting, they cannot both be + /// true. This is in turn used to determine whether particular markers are + /// implied by this world knowledge and thus can be removed. + /// + /// [bug]: + #[test] + fn test_extra_implication() { + assert!(implies( + "extra != 'foo' or extra != 'bar'", + "extra != 'foo' or extra != 'bar'", + )); + assert!(!implies( + "extra != 'foo' or extra != 'bar'", + "extra != 'foo'", + )); + assert!(!implies( + "extra != 'foo' or extra != 'bar'", + "extra != 'bar' and extra == 'foo'", + )); + + // This simulates the case when multiple groups of conflicts + // are combined into one "world knowledge" marker. + assert!(implies( + "(extra != 'foo' or extra != 'bar') and (extra != 'baz' or extra != 'quux')", + "extra != 'foo' or extra != 'bar'", + )); + assert!(!implies( + "(extra != 'foo' or extra != 'bar') and (extra != 'baz' or extra != 'quux')", + "extra != 'foo'", + )); + assert!(!implies( + "(extra != 'foo' or extra != 'bar') and (extra != 'baz' or extra != 'quux')", + "extra != 'bar' and extra == 'foo'", + )); + } + #[test] fn test_marker_negation() { assert_eq!( @@ -2951,6 +3005,12 @@ mod test { left.is_disjoint(&right) && right.is_disjoint(&left) } + fn implies(antecedent: &str, consequent: &str) -> bool { + let mut marker = m(antecedent); + marker.implies(m(consequent)); + marker.is_true() + } + #[test] fn complexified_markers() { // Takes optional lower (inclusive) and upper (exclusive) diff --git a/crates/uv-resolver/src/resolution/output.rs b/crates/uv-resolver/src/resolution/output.rs index e9d01e171458..3882359517b8 100644 --- a/crates/uv-resolver/src/resolution/output.rs +++ b/crates/uv-resolver/src/resolution/output.rs @@ -160,18 +160,19 @@ impl ResolverOutput { let requires_python = python.target().clone(); let fork_markers: Vec = if let [resolution] = resolutions { + // In the case of a singleton marker, we only include it if it's not + // always true. Otherwise, we keep our `fork_markers` empty as there + // are no forks. resolution .env - .try_markers() - .cloned() + .try_universal_markers() .into_iter() - .map(|marker| UniversalMarker::new(marker, MarkerTree::TRUE)) + .filter(|marker| !marker.is_true()) .collect() } else { resolutions .iter() - .map(|resolution| resolution.env.try_markers().cloned().unwrap_or_default()) - .map(|marker| UniversalMarker::new(marker, MarkerTree::TRUE)) + .map(|resolution| resolution.env.try_universal_markers().unwrap_or_default()) .collect() }; diff --git a/crates/uv-resolver/src/resolver/environment.rs b/crates/uv-resolver/src/resolver/environment.rs index d8a6fccd6691..b9ba08baa5f6 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 } @@ -172,8 +187,14 @@ impl ResolverEnvironment { /// Returns the bounding Python versions that can satisfy this /// resolver environment's marker, if it's constrained. pub(crate) fn requires_python(&self) -> Option { - let marker = self.try_markers().unwrap_or(&MarkerTree::TRUE); - crate::marker::requires_python(marker) + let Kind::Universal { + markers: ref pep508_marker, + .. + } = self.kind + else { + return None; + }; + crate::marker::requires_python(pep508_marker) } /// Narrow this environment given the forking markers. @@ -195,6 +216,7 @@ impl ResolverEnvironment { Kind::Universal { ref initial_forks, markers: ref lhs, + ref include, ref exclude, } => { let mut markers = lhs.clone(); @@ -202,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 } @@ -209,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") @@ -232,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 }) } } } @@ -260,6 +306,7 @@ impl ResolverEnvironment { let Kind::Universal { ref initial_forks, markers: ref _markers, + include: ref _include, exclude: ref _exclude, } = self.kind else { @@ -292,7 +339,7 @@ impl ResolverEnvironment { &self, python_requirement: &PythonRequirement, ) -> Option { - python_requirement.narrow(&self.requires_python_range()?) + python_requirement.narrow(&self.requires_python()?) } /// Returns a message formatted for end users representing a fork in the @@ -317,23 +364,6 @@ impl ResolverEnvironment { } } - /// Returns the marker expression corresponding to the fork that is - /// represented by this resolver environment. - /// - /// This returns `None` when this does not correspond to a fork. - pub(crate) fn try_markers(&self) -> Option<&MarkerTree> { - match self.kind { - Kind::Specific { .. } => None, - Kind::Universal { ref markers, .. } => { - if markers.is_true() { - None - } else { - Some(markers) - } - } - } - } - /// Creates a universal marker expression corresponding to the fork that is /// represented by this resolver environment. A universal marker includes /// not just the standard PEP 508 marker, but also a marker based on @@ -343,25 +373,35 @@ impl ResolverEnvironment { pub(crate) fn try_universal_markers(&self) -> Option { match self.kind { Kind::Specific { .. } => None, - Kind::Universal { ref markers, .. } => { - if markers.is_true() { - None - } else { - // 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)) } } } - - /// Returns a requires-python version range derived from the marker - /// expression describing this resolver environment. - /// - /// When this isn't a fork, then there is nothing to constrain and thus - /// `None` is returned. - fn requires_python_range(&self) -> Option { - crate::marker::requires_python(self.try_markers()?) - } } /// A user visible representation of a resolver environment. diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs index 09b08debe0d1..f4e0660ff183 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) } } diff --git a/crates/uv-resolver/src/universal_marker.rs b/crates/uv-resolver/src/universal_marker.rs index 74366ad760f7..2b908cb59fd6 100644 --- a/crates/uv-resolver/src/universal_marker.rs +++ b/crates/uv-resolver/src/universal_marker.rs @@ -122,9 +122,9 @@ impl std::fmt::Display for UniversalMarker { ) { (None, None) => write!(f, "`true`"), (Some(pep508), None) => write!(f, "`{pep508}`"), - (None, Some(conflict)) => write!(f, "`true` (conflict marker: `{conflict}`"), + (None, Some(conflict)) => write!(f, "`true` (conflict marker: `{conflict}`)"), (Some(pep508), Some(conflict)) => { - write!(f, "`{pep508}` (conflict marker: `{conflict}`") + write!(f, "`{pep508}` (conflict marker: `{conflict}`)") } } } diff --git a/crates/uv/tests/it/lock.rs b/crates/uv/tests/it/lock.rs index 03a5c67fe1a9..676c5c77a0c3 100644 --- a/crates/uv/tests/it/lock.rs +++ b/crates/uv/tests/it/lock.rs @@ -19621,8 +19621,6 @@ fn lock_multiple_sources_index_disjoint_extras_with_marker() -> Result<()> { version = "3.1.3" source = { registry = "https://download.pytorch.org/whl/cu124" } resolution-markers = [ - "sys_platform == 'darwin'", - "sys_platform != 'darwin'", ] dependencies = [ { name = "markupsafe" },