small refactoring in pursuit of fixing conflicting extras/groups

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]: <https://github.com/astral-sh/uv/issues/9289>
+    #[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)

crates/uv-resolver/src/resolution/output.rs

@@ -160,18 +160,19 @@ impl ResolverOutput {
         let requires_python = python.target().clone();
 
         let fork_markers: Vec<UniversalMarker> = 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()
         };
 

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<crate::FxHashbrownSet<ConflictItem>>,
         /// Conflicting group exclusions.
         exclude: Arc<crate::FxHashbrownSet<ConflictItem>>,
     },
@@ -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<RequiresPythonRange> {
-        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,55 +216,80 @@ impl ResolverEnvironment {
             Kind::Universal {
                 ref initial_forks,
                 markers: ref lhs,
+                ref include,
                 ref exclude,
             } => {
                 let mut markers = lhs.clone();
                 markers.and(rhs);
                 let kind = Kind::Universal {
                     initial_forks: Arc::clone(initial_forks),
                     markers,
+                    include: Arc::clone(include),
                     exclude: Arc::clone(exclude),
                 };
                 ResolverEnvironment { kind }
             }
         }
     }
 
-    /// 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<Item = ConflictItem>,
-    ) -> ResolverEnvironment {
+        rules: impl IntoIterator<Item = Result<ConflictItem, ConflictItem>>,
+    ) -> Option<ResolverEnvironment> {
         match self.kind {
             Kind::Specific { .. } => {
                 unreachable!("environment narrowing only happens in universal resolution")
             }
             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<PythonRequirement> {
-        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<UniversalMarker> {
         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<RequiresPythonRange> {
-        crate::marker::requires_python(self.try_markers()?)
-    }
 }
 
 /// A user visible representation of a resolver environment.