Avoid need for universal markers in requirements.txt export (#10171)

crates/uv-resolver/src/graph_ops.rs

@@ -5,6 +5,7 @@ use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet};
 use std::collections::hash_map::Entry;
 
 use uv_normalize::{ExtraName, GroupName, PackageName};
+use uv_pep508::MarkerTree;
 use uv_pypi_types::{ConflictItem, Conflicts};
 
 use crate::resolution::ResolutionGraphNode;
@@ -17,10 +18,10 @@ use crate::universal_marker::UniversalMarker;
 /// marker), we re-queue the node and update all its children. This implicitly handles cycles,
 /// whenever we re-reach a node through a cycle the marker we have is a more
 /// specific marker/longer path, so we don't update the node and don't re-queue it.
-pub(crate) fn marker_reachability<T>(
-    graph: &Graph<T, UniversalMarker>,
-    fork_markers: &[UniversalMarker],
-) -> FxHashMap<NodeIndex, UniversalMarker> {
+pub(crate) fn marker_reachability<Node, Edge: Reachable + Copy + PartialEq>(
+    graph: &Graph<Node, Edge>,
+    fork_markers: &[Edge],
+) -> FxHashMap<NodeIndex, Edge> {
     // Note that we build including the virtual packages due to how we propagate markers through
     // the graph, even though we then only read the markers for base packages.
     let mut reachability = FxHashMap::with_capacity_and_hasher(graph.node_count(), FxBuildHasher);
@@ -42,11 +43,11 @@ pub(crate) fn marker_reachability<T>(
     // The root nodes are always applicable, unless the user has restricted resolver
     // environments with `tool.uv.environments`.
     let root_markers = if fork_markers.is_empty() {
-        UniversalMarker::TRUE
+        Edge::true_marker()
     } else {
         fork_markers
             .iter()
-            .fold(UniversalMarker::FALSE, |mut acc, marker| {
+            .fold(Edge::false_marker(), |mut acc, marker| {
                 acc.or(*marker);
                 acc
             })
@@ -264,3 +265,54 @@ pub(crate) fn simplify_conflict_markers(
         }
     }
 }
+
+/// A trait for types that can be used as markers in the dependency graph.
+pub(crate) trait Reachable {
+    /// The marker representing the "true" value.
+    fn true_marker() -> Self;
+
+    /// The marker representing the "false" value.
+    fn false_marker() -> Self;
+
+    /// Perform a logical AND operation with another marker.
+    fn and(&mut self, other: Self);
+
+    /// Perform a logical OR operation with another marker.
+    fn or(&mut self, other: Self);
+}
+
+impl Reachable for UniversalMarker {
+    fn true_marker() -> Self {
+        Self::TRUE
+    }
+
+    fn false_marker() -> Self {
+        Self::FALSE
+    }
+
+    fn and(&mut self, other: Self) {
+        self.and(other);
+    }
+
+    fn or(&mut self, other: Self) {
+        self.or(other);
+    }
+}
+
+impl Reachable for MarkerTree {
+    fn true_marker() -> Self {
+        Self::TRUE
+    }
+
+    fn false_marker() -> Self {
+        Self::FALSE
+    }
+
+    fn and(&mut self, other: Self) {
+        self.and(other);
+    }
+
+    fn or(&mut self, other: Self) {
+        self.or(other);
+    }
+}

crates/uv-resolver/src/lock/requirements_txt.rs

@@ -20,7 +20,6 @@ use uv_pypi_types::{ParsedArchiveUrl, ParsedGitUrl};
 
 use crate::graph_ops::marker_reachability;
 use crate::lock::{Package, PackageId, Source};
-use crate::universal_marker::{ConflictMarker, UniversalMarker};
 use crate::{Installable, LockError};
 
 /// An export of a [`Lock`] that renders in `requirements.txt` format.
@@ -70,7 +69,7 @@ impl<'lock> RequirementsTxtExport<'lock> {
 
                 // Add an edge from the root.
                 let index = inverse[&dist.id];
-                petgraph.add_edge(root, index, UniversalMarker::TRUE);
+                petgraph.add_edge(root, index, MarkerTree::TRUE);
 
                 // Push its dependencies on the queue.
                 queue.push_back((dist, None));
@@ -110,17 +109,7 @@ impl<'lock> RequirementsTxtExport<'lock> {
                 petgraph.add_edge(
                     root,
                     dep_index,
-                    UniversalMarker::new(
-                        dep.simplified_marker.as_simplified_marker_tree(),
-                        // OK because we've verified above that we do not have any
-                        // conflicting extras/groups.
-                        //
-                        // So why use `UniversalMarker`? Because that's what
-                        // `marker_reachability` wants and it (probably) isn't
-                        // worth inventing a new abstraction so that it can accept
-                        // graphs with either `MarkerTree` or `UniversalMarker`.
-                        ConflictMarker::TRUE,
-                    ),
+                    dep.simplified_marker.as_simplified_marker_tree(),
                 );
 
                 // Push its dependencies on the queue.
@@ -205,21 +194,7 @@ impl<'lock> RequirementsTxtExport<'lock> {
 
                     // Add an edge from the root.
                     let dep_index = inverse[&dist.id];
-                    petgraph.add_edge(
-                        root,
-                        dep_index,
-                        UniversalMarker::new(
-                            marker,
-                            // OK because we've verified above that we do not have any
-                            // conflicting extras/groups.
-                            //
-                            // So why use `UniversalMarker`? Because that's what
-                            // `marker_reachability` wants and it (probably) isn't
-                            // worth inventing a new abstraction so that it can accept
-                            // graphs with either `MarkerTree` or `UniversalMarker`.
-                            ConflictMarker::TRUE,
-                        ),
-                    );
+                    petgraph.add_edge(root, dep_index, marker);
 
                     // Push its dependencies on the queue.
                     if seen.insert((&dist.id, None)) {
@@ -267,12 +242,7 @@ impl<'lock> RequirementsTxtExport<'lock> {
                 petgraph.add_edge(
                     index,
                     dep_index,
-                    UniversalMarker::new(
-                        dep.simplified_marker.as_simplified_marker_tree(),
-                        // See note above for other `UniversalMarker::new` for
-                        // why this is OK.
-                        ConflictMarker::TRUE,
-                    ),
+                    dep.simplified_marker.as_simplified_marker_tree(),
                 );
 
                 // Push its dependencies on the queue.
@@ -305,9 +275,7 @@ impl<'lock> RequirementsTxtExport<'lock> {
             })
             .map(|(index, package)| Requirement {
                 package,
-                // As above, we've verified that there are no conflicting extras/groups
-                // specified, so it's safe to completely ignore the conflict marker.
-                marker: reachability.remove(&index).unwrap_or_default().pep508(),
+                marker: reachability.remove(&index).unwrap_or_default(),
             })
             .collect::<Vec<_>>();