Join requirement and override markers

This is an attempt to solve #4826 by joining markers of requirements and overrides.

Say in `a` we have a requirements
```
b==1; python_version < "3.10"
c==1; extra == "feature"
```

and overrides
```
b==2; python_version < "3.10"
b==3; python_version >= "3.10"
c==2; python_version < "3.10"
c==3; python_version >= "3.10"
```

Our current strategy is to discard the markers in the original requirements. This means that on 3.12 for `a` we install `b==3`, but it also means that we add `c` to `a` without `a[feature]`, causing #4826.

This PR changes this always join markers, which has the similarly counterintuitive effect that `b` will never be installed for `a` for 3.12. Maybe the correct solution is to only copy an extra marker, since we special case that one anyway?

crates/uv-configuration/src/constraints.rs

@@ -1,6 +1,7 @@
-use rustc_hash::{FxBuildHasher, FxHashMap};
-
+use either::Either;
 use pypi_types::Requirement;
+use rustc_hash::{FxBuildHasher, FxHashMap};
+use std::borrow::Cow;
 use uv_normalize::PackageName;
 
 /// A set of constraints for a set of requirements.
@@ -38,14 +39,34 @@ impl Constraints {
     /// Apply the constraints to a set of requirements.
     pub fn apply<'a>(
         &'a self,
-        requirements: impl IntoIterator<Item = &'a Requirement>,
-    ) -> impl Iterator<Item = &Requirement> {
+        requirements: impl IntoIterator<Item = Cow<'a, Requirement>>,
+    ) -> impl Iterator<Item = Cow<'a, Requirement>> {
         requirements.into_iter().flat_map(|requirement| {
-            std::iter::once(requirement).chain(
-                self.get(&requirement.name)
-                    .into_iter()
-                    .flat_map(|constraints| constraints.iter()),
-            )
+            if let Some(overrides) = self.get(&requirement.name) {
+                // Join the marker of the original requirement and the constraint.
+                // We use the markers of the constraint to allow gating the constraint to
+                // specific marker, but we also have to add the original markers, otherwise
+                // we would activate extras that should be inactive.
+                let marker = requirement.marker.clone();
+                Either::Left(
+                    std::iter::once(requirement).chain(if let Some(marker) = marker {
+                        Either::Left(overrides.iter().map(move |constraint| {
+                            let mut joint_marker = marker.clone();
+                            if let Some(marker) = &constraint.marker {
+                                joint_marker.and(marker.clone());
+                            }
+                            Cow::Owned(Requirement {
+                                marker: Some(joint_marker.clone()),
+                                ..constraint.clone()
+                            })
+                        }))
+                    } else {
+                        Either::Right(overrides.iter().map(Cow::Borrowed))
+                    }),
+                )
+            } else {
+                Either::Right(std::iter::once(requirement))
+            }
         })
     }
 }

crates/uv-configuration/src/overrides.rs

@@ -1,7 +1,7 @@
 use either::Either;
-use rustc_hash::{FxBuildHasher, FxHashMap};
-
 use pypi_types::Requirement;
+use rustc_hash::{FxBuildHasher, FxHashMap};
+use std::borrow::Cow;
 use uv_normalize::PackageName;
 
 /// A set of overrides for a set of requirements.
@@ -36,12 +36,29 @@ impl Overrides {
     pub fn apply<'a>(
         &'a self,
         requirements: impl IntoIterator<Item = &'a Requirement>,
-    ) -> impl Iterator<Item = &Requirement> {
+    ) -> impl Iterator<Item = Cow<'a, Requirement>> {
         requirements.into_iter().flat_map(|requirement| {
             if let Some(overrides) = self.get(&requirement.name) {
-                Either::Left(overrides.iter())
+                if let Some(marker) = &requirement.marker {
+                    Either::Left(Either::Left(overrides.iter().map(|override_requirement| {
+                        // Join the marker of the original requirement and the override.
+                        // We use the markers of the override to allow gating the override to
+                        // specific marker, but we also have to add the original markers, otherwise
+                        // we would activate extras that should be inactive.
+                        let mut joint_marker = marker.clone();
+                        if let Some(marker) = &override_requirement.marker {
+                            joint_marker.and(marker.clone());
+                        }
+                        Cow::Owned(Requirement {
+                            marker: Some(joint_marker.clone()),
+                            ..override_requirement.clone()
+                        })
+                    })))
+                } else {
+                    Either::Left(Either::Right(overrides.iter().map(Cow::Borrowed)))
+                }
             } else {
-                Either::Right(std::iter::once(requirement))
+                Either::Right(std::iter::once(Cow::Borrowed(requirement)))
             }
         })
     }

crates/uv-requirements/src/lookahead.rs

@@ -109,7 +109,7 @@ impl<'a, Context: BuildContext> LookaheadResolver<'a, Context> {
             .constraints
             .apply(self.overrides.apply(self.requirements))
             .filter(|requirement| requirement.evaluate_markers(markers, &[]))
-            .cloned()
+            .map(|requirement| (*requirement).clone())
             .collect();
 
         while !queue.is_empty() || !futures.is_empty() {
@@ -128,7 +128,7 @@ impl<'a, Context: BuildContext> LookaheadResolver<'a, Context> {
                         .apply(self.overrides.apply(lookahead.requirements()))
                     {
                         if requirement.evaluate_markers(markers, lookahead.extras()) {
-                            queue.push_back(requirement.clone());
+                            queue.push_back((*requirement).clone());
                         }
                     }
                     results.push(lookahead);

crates/uv-resolver/src/manifest.rs

@@ -1,4 +1,5 @@
 use either::Either;
+use std::borrow::Cow;
 
 use pep508_rs::MarkerEnvironment;
 use pypi_types::Requirement;
@@ -97,7 +98,7 @@ impl Manifest {
         &'a self,
         markers: Option<&'a MarkerEnvironment>,
         mode: DependencyMode,
-    ) -> impl Iterator<Item = &Requirement> + 'a {
+    ) -> impl Iterator<Item = Cow<'a, Requirement>> + 'a {
         self.requirements_no_overrides(markers, mode)
             .chain(self.overrides(markers, mode))
     }
@@ -107,7 +108,7 @@ impl Manifest {
         &'a self,
         markers: Option<&'a MarkerEnvironment>,
         mode: DependencyMode,
-    ) -> impl Iterator<Item = &Requirement> + 'a {
+    ) -> impl Iterator<Item = Cow<'a, Requirement>> + 'a {
         match mode {
             // Include all direct and transitive requirements, with constraints and overrides applied.
             DependencyMode::Transitive => Either::Left(
@@ -128,14 +129,15 @@ impl Manifest {
                     .chain(
                         self.constraints
                             .requirements()
-                            .filter(move |requirement| requirement.evaluate_markers(markers, &[])),
+                            .filter(move |requirement| requirement.evaluate_markers(markers, &[]))
+                            .map(Cow::Borrowed),
                     ),
             ),
             // Include direct requirements, with constraints and overrides applied.
             DependencyMode::Direct => Either::Right(
                 self.overrides
                     .apply(&self.requirements)
-                    .chain(self.constraints.requirements())
+                    .chain(self.constraints.requirements().map(Cow::Borrowed))
                     .filter(move |requirement| requirement.evaluate_markers(markers, &[])),
             ),
         }
@@ -146,19 +148,21 @@ impl Manifest {
         &'a self,
         markers: Option<&'a MarkerEnvironment>,
         mode: DependencyMode,
-    ) -> impl Iterator<Item = &Requirement> + 'a {
+    ) -> impl Iterator<Item = Cow<'a, Requirement>> + 'a {
         match mode {
             // Include all direct and transitive requirements, with constraints and overrides applied.
             DependencyMode::Transitive => Either::Left(
                 self.overrides
                     .requirements()
-                    .filter(move |requirement| requirement.evaluate_markers(markers, &[])),
+                    .filter(move |requirement| requirement.evaluate_markers(markers, &[]))
+                    .map(Cow::Borrowed),
             ),
             // Include direct requirements, with constraints and overrides applied.
             DependencyMode::Direct => Either::Right(
                 self.overrides
                     .requirements()
-                    .filter(move |requirement| requirement.evaluate_markers(markers, &[])),
+                    .filter(move |requirement| requirement.evaluate_markers(markers, &[]))
+                    .map(Cow::Borrowed),
             ),
         }
     }
@@ -177,7 +181,7 @@ impl Manifest {
         &'a self,
         markers: Option<&'a MarkerEnvironment>,
         mode: DependencyMode,
-    ) -> impl Iterator<Item = &PackageName> + 'a {
+    ) -> impl Iterator<Item = Cow<'a, PackageName>> + 'a {
         match mode {
             // Include direct requirements, dependencies of editables, and transitive dependencies
             // of local packages.
@@ -197,15 +201,21 @@ impl Manifest {
                             .apply(&self.requirements)
                             .filter(move |requirement| requirement.evaluate_markers(markers, &[])),
                     )
-                    .map(|requirement| &requirement.name),
+                    .map(|requirement| match requirement {
+                        Cow::Borrowed(requirement) => Cow::Borrowed(&requirement.name),
+                        Cow::Owned(requirement) => Cow::Owned(requirement.name),
+                    }),
             ),
 
             // Restrict to the direct requirements.
             DependencyMode::Direct => Either::Right(
                 self.overrides
                     .apply(self.requirements.iter())
                     .filter(move |requirement| requirement.evaluate_markers(markers, &[]))
-                    .map(|requirement| &requirement.name),
+                    .map(|requirement| match requirement {
+                        Cow::Borrowed(requirement) => Cow::Borrowed(&requirement.name),
+                        Cow::Owned(requirement) => Cow::Owned(requirement.name),
+                    }),
             ),
         }
     }
@@ -217,7 +227,7 @@ impl Manifest {
     pub fn apply<'a>(
         &'a self,
         requirements: impl IntoIterator<Item = &'a Requirement>,
-    ) -> impl Iterator<Item = &Requirement> {
+    ) -> impl Iterator<Item = Cow<'a, Requirement>> {
         self.constraints.apply(self.overrides.apply(requirements))
     }
 

crates/uv-resolver/src/resolution_mode.rs

@@ -46,7 +46,7 @@ impl ResolutionStrategy {
             ResolutionMode::LowestDirect => Self::LowestDirect(
                 manifest
                     .user_requirements(markers, dependencies)
-                    .cloned()
+                    .map(|requirement| (*requirement).clone())
                     .collect(),
             ),
         }

crates/uv-resolver/src/resolver/mod.rs

@@ -1466,7 +1466,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
                 true
             })
             .flat_map(move |requirement| {
-                iter::once(Cow::Borrowed(requirement)).chain(
+                iter::once(requirement.clone()).chain(
                     self.constraints
                         .get(&requirement.name)
                         .into_iter()

crates/uv/tests/pip_compile.rs

@@ -3111,7 +3111,7 @@ fn override_dependency_from_specific_uv_toml() -> Result<()> {
 /// override it with a multi-line override.
 #[test]
 fn override_multi_dependency() -> Result<()> {
-    let context = TestContext::new("3.12");
+    let context = TestContext::new("3.10");
     let requirements_in = context.temp_dir.child("requirements.in");
     requirements_in.write_str("black==23.10.1")?;
 
@@ -3141,13 +3141,45 @@ fn override_multi_dependency() -> Result<()> {
         # via black
     platformdirs==4.2.0
         # via black
-    tomli==2.0.1
+    tomli==0.2.10
         # via
         #   --override overrides.txt
         #   black
+    typing-extensions==4.10.0
+        # via black
 
     ----- stderr -----
-    Resolved 7 packages in [TIME]
+    Resolved 8 packages in [TIME]
+    "###
+    );
+
+    Ok(())
+}
+/// `urllib3==2.2.2` has an optional dependency on `pysocks!=1.5.7,<2.0,>=1.5.6; extra == 'socks'`,
+/// So we shouldn't apply the `pysocks==1.7.1` override without the `socks` extra.
+#[test]
+fn dont_add_override_for_non_activated_extra() -> Result<()> {
+    let context = TestContext::new("3.12");
+    let requirements_in = context.temp_dir.child("requirements.in");
+    requirements_in.write_str("urllib3==2.2.1")?;
+
+    let overrides_txt = context.temp_dir.child("overrides.txt");
+    overrides_txt.write_str("pysocks==1.7.1")?;
+
+    uv_snapshot!(context.pip_compile()
+            .arg("requirements.in")
+            .arg("--override")
+            .arg("overrides.txt"), @r###"
+    success: true
+    exit_code: 0
+    ----- stdout -----
+    # This file was autogenerated by uv via the following command:
+    #    uv pip compile --cache-dir [CACHE_DIR] requirements.in --override overrides.txt
+    urllib3==2.2.1
+        # via -r requirements.in
+
+    ----- stderr -----
+    Resolved 1 package in [TIME]
     "###
     );