From 26acf260a1050f322348715d1f2398777e5da138 Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Fri, 22 Nov 2024 15:34:28 -0500 Subject: [PATCH] uv-resolver: introduce conflict markers This commit adds a new concept to uv: conflict markers. Conflict markers resolve the ambiguity created in lock files, in some cases, when conflicting extras/groups are activated. Conflict markers allow us to make edges in the dependency graph conditional based only on which extras are and aren't enabled. Adding conflict markers themselves turned out to be somewhat easy. But what was hard was simplifying them. A good portion of the change in this commit is dedicated to simplification. We do two different kinds: 1. We imbibe "world knowledge" into the marker. That is, we take what we know about which extras/groups cannot be enabled simultaneously, then we simplify the markers based on this. 2. We walk the dependency graph and annotate which extras/groups are activated for each node for all possible paths into that node. When an extra (e.g., `foo`) is activated (or not activated) for all possible such paths, then we can replace `extra == 'foo'` (or `extra != 'foo'`) with `true` in any corresponding conflict marker expessions. Without these simplifications, it's very easy for conflict markers to appear almost everywhere in a lock file (when conflicting extras/groups are declared). Many of which are trivially true and aren't actually resolving any ambiguity and thus aren't needed. --- crates/uv-pypi-types/src/conflicts.rs | 4 +- crates/uv-resolver/src/graph_ops.rs | 161 ++++- crates/uv-resolver/src/lib.rs | 2 +- crates/uv-resolver/src/lock/mod.rs | 30 +- .../uv-resolver/src/lock/requirements_txt.rs | 6 +- ...missing_dependency_source_unambiguous.snap | 5 +- ...dependency_source_version_unambiguous.snap | 5 +- ...issing_dependency_version_unambiguous.snap | 5 +- crates/uv-resolver/src/lock/target.rs | 22 +- crates/uv-resolver/src/lock/tree.rs | 6 +- crates/uv-resolver/src/preferences.rs | 2 +- crates/uv-resolver/src/resolution/display.rs | 2 +- crates/uv-resolver/src/resolution/output.rs | 46 +- .../src/resolution/requirements_txt.rs | 4 +- .../uv-resolver/src/resolver/environment.rs | 21 +- crates/uv-resolver/src/resolver/mod.rs | 8 +- crates/uv-resolver/src/universal_marker.rs | 592 ++++++++++++++++-- 17 files changed, 795 insertions(+), 126 deletions(-) diff --git a/crates/uv-pypi-types/src/conflicts.rs b/crates/uv-pypi-types/src/conflicts.rs index 57ed4851a518..3a13399945dc 100644 --- a/crates/uv-pypi-types/src/conflicts.rs +++ b/crates/uv-pypi-types/src/conflicts.rs @@ -24,7 +24,7 @@ impl Conflicts { } /// Returns an iterator over all sets of conflicting sets. - pub fn iter(&self) -> impl Iterator + '_ { + pub fn iter(&self) -> impl Iterator + Clone + '_ { self.0.iter() } @@ -75,7 +75,7 @@ impl ConflictSet { } /// Returns an iterator over all conflicting items. - pub fn iter(&self) -> impl Iterator + '_ { + pub fn iter(&self) -> impl Iterator + Clone + '_ { self.0.iter() } diff --git a/crates/uv-resolver/src/graph_ops.rs b/crates/uv-resolver/src/graph_ops.rs index 19ef37289450..378495748df0 100644 --- a/crates/uv-resolver/src/graph_ops.rs +++ b/crates/uv-resolver/src/graph_ops.rs @@ -1,9 +1,13 @@ -use petgraph::graph::NodeIndex; +use petgraph::graph::{EdgeIndex, NodeIndex}; use petgraph::visit::EdgeRef; use petgraph::{Direction, Graph}; -use rustc_hash::{FxBuildHasher, FxHashMap}; +use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet}; use std::collections::hash_map::Entry; +use uv_normalize::{ExtraName, GroupName, PackageName}; +use uv_pypi_types::{ConflictItem, Conflicts}; + +use crate::resolution::ResolutionGraphNode; use crate::universal_marker::UniversalMarker; /// Determine the markers under which a package is reachable in the dependency tree. @@ -79,3 +83,156 @@ pub(crate) fn marker_reachability( reachability } + +/// Traverse the given dependency graph and propagate activated markers. +/// +/// For example, given an edge like `foo[x1] -> bar`, then it is known that +/// `x1` is activated. This in turn can be used to simplify any downstream +/// conflict markers with `extra == "x1"` in them (by replacing `extra == "x1"` +/// with `true`). +pub(crate) fn simplify_conflict_markers( + conflicts: &Conflicts, + graph: &mut Graph, +) { + /// An inference about whether a conflicting item is always included or + /// excluded. + /// + /// We collect these for each node in the graph after determining which + /// extras/groups are activated for each node. Once we know what's + /// activated, we can infer what must also be *inactivated* based on what's + /// conflicting with it. So for example, if we have a conflict marker like + /// `extra == 'foo' and extra != 'bar'`, and `foo` and `bar` have been + /// declared as conflicting, and we are in a part of the graph where we + /// know `foo` must be activated, then it follows that `extra != 'bar'` + /// must always be true. Because if it were false, it would imply both + /// `foo` and `bar` were activated simultaneously, which uv guarantees + /// won't happen. + /// + /// We then use these inferences to simplify the conflict markers. + #[derive(Clone, Debug, Eq, Hash, PartialEq)] + struct Inference { + item: ConflictItem, + included: bool, + } + + // The set of activated extras and groups for each node. The ROOT nodes + // don't have any extras/groups activated. + let mut activated: FxHashMap>> = FxHashMap::default(); + + // Collect the root nodes. + // + // Besides the actual virtual root node, virtual dev dependencies packages are also root + // nodes since the edges don't cover dev dependencies. + let mut queue: Vec<_> = graph + .node_indices() + .filter(|node_index| { + graph + .edges_directed(*node_index, Direction::Incoming) + .next() + .is_none() + }) + .collect(); + + let mut seen: FxHashSet = FxHashSet::default(); + while let Some(parent_index) = queue.pop() { + if let Some((package, extra)) = graph[parent_index].package_extra_names() { + for set in activated + .entry(parent_index) + .or_insert_with(|| vec![FxHashSet::default()]) + { + set.insert(ConflictItem::from((package.clone(), extra.clone()))); + } + } + if let Some((package, group)) = graph[parent_index].package_group_names() { + for set in activated + .entry(parent_index) + .or_insert_with(|| vec![FxHashSet::default()]) + { + set.insert(ConflictItem::from((package.clone(), group.clone()))); + } + } + let sets = activated.get(&parent_index).cloned().unwrap_or_default(); + for child_edge in graph.edges_directed(parent_index, Direction::Outgoing) { + for set in sets.clone() { + activated.entry(child_edge.target()).or_default().push(set); + } + if seen.insert(child_edge.target()) { + queue.push(child_edge.target()); + } + } + } + + let mut inferences: FxHashMap>> = FxHashMap::default(); + for (node_id, sets) in activated { + let mut new_sets = vec![]; + for set in sets { + let mut new_set = FxHashSet::default(); + for item in set { + for conflict_set in conflicts.iter() { + if !conflict_set.contains(item.package(), item.as_ref().conflict()) { + continue; + } + for conflict_item in conflict_set.iter() { + if conflict_item == &item { + continue; + } + new_set.insert(Inference { + item: conflict_item.clone(), + included: false, + }); + } + } + new_set.insert(Inference { + item, + included: true, + }); + } + new_sets.push(new_set); + } + inferences.insert(node_id, new_sets); + } + + for edge_index in (0..graph.edge_count()).map(EdgeIndex::new) { + let (from_index, _) = graph.edge_endpoints(edge_index).unwrap(); + let Some(inference_sets) = inferences.get(&from_index) else { + continue; + }; + // If not all possible paths (represented by our inferences) + // satisfy the conflict marker on this edge, then we can't make any + // simplifications. Namely, because it follows that out inferences + // aren't always true. Some of them may sometimes be false. + let all_paths_satisfied = inference_sets.iter().all(|set| { + let extras = set + .iter() + .filter_map(|inf| { + if !inf.included { + return None; + } + Some((inf.item.package().clone(), inf.item.extra()?.clone())) + }) + .collect::>(); + let groups = set + .iter() + .filter_map(|inf| { + if !inf.included { + return None; + } + Some((inf.item.package().clone(), inf.item.group()?.clone())) + }) + .collect::>(); + graph[edge_index].conflict().evaluate(&extras, &groups) + }); + if !all_paths_satisfied { + continue; + } + for set in inference_sets { + for inf in set { + if inf.included { + graph[edge_index].assume_conflict_item(&inf.item); + } else { + graph[edge_index].assume_not_conflict_item(&inf.item); + } + } + } + } +} diff --git a/crates/uv-resolver/src/lib.rs b/crates/uv-resolver/src/lib.rs index 90cd4634ce79..bce29ec2eff3 100644 --- a/crates/uv-resolver/src/lib.rs +++ b/crates/uv-resolver/src/lib.rs @@ -22,7 +22,7 @@ pub use resolver::{ PackageVersionsResult, Reporter as ResolverReporter, Resolver, ResolverEnvironment, ResolverProvider, VersionsResponse, WheelMetadataResult, }; -pub use universal_marker::UniversalMarker; +pub use universal_marker::{ConflictMarker, UniversalMarker}; pub use version_map::VersionMap; pub use yanks::AllowedYanks; diff --git a/crates/uv-resolver/src/lock/mod.rs b/crates/uv-resolver/src/lock/mod.rs index d965e8700f0a..f5e1b78e1272 100644 --- a/crates/uv-resolver/src/lock/mod.rs +++ b/crates/uv-resolver/src/lock/mod.rs @@ -19,7 +19,7 @@ pub use crate::lock::target::InstallTarget; pub use crate::lock::tree::TreeDisplay; use crate::requires_python::SimplifiedMarkerTree; use crate::resolution::{AnnotatedDist, ResolutionGraphNode}; -use crate::universal_marker::UniversalMarker; +use crate::universal_marker::{ConflictMarker, UniversalMarker}; use crate::{ ExcludeNewer, InMemoryIndex, MetadataResponse, PrereleaseMode, RequiresPython, ResolutionMode, ResolverOutput, @@ -63,21 +63,21 @@ static LINUX_MARKERS: LazyLock = LazyLock::new(|| { "platform_system == 'Linux' and os_name == 'posix' and sys_platform == 'linux'", ) .unwrap(); - UniversalMarker::new(pep508, MarkerTree::TRUE) + UniversalMarker::new(pep508, ConflictMarker::TRUE) }); static WINDOWS_MARKERS: LazyLock = LazyLock::new(|| { let pep508 = MarkerTree::from_str( "platform_system == 'Windows' and os_name == 'nt' and sys_platform == 'win32'", ) .unwrap(); - UniversalMarker::new(pep508, MarkerTree::TRUE) + UniversalMarker::new(pep508, ConflictMarker::TRUE) }); static MAC_MARKERS: LazyLock = LazyLock::new(|| { let pep508 = MarkerTree::from_str( "platform_system == 'Darwin' and os_name == 'posix' and sys_platform == 'darwin'", ) .unwrap(); - UniversalMarker::new(pep508, MarkerTree::TRUE) + UniversalMarker::new(pep508, ConflictMarker::TRUE) }); #[derive(Clone, Debug, serde::Deserialize)] @@ -149,7 +149,7 @@ impl Lock { resolution .fork_markers .iter() - .filter(|fork_markers| !fork_markers.is_disjoint(&dist.marker)) + .filter(|fork_markers| !fork_markers.is_disjoint(dist.marker)) .copied() .collect() } else { @@ -296,16 +296,16 @@ impl Lock { tag.starts_with(linux_tag) || tag == "linux_armv6l" || tag == "linux_armv7l" }) }) { - !graph.graph[node_index].marker().is_disjoint(&LINUX_MARKERS) + !graph.graph[node_index].marker().is_disjoint(*LINUX_MARKERS) } else if platform_tags .iter() .all(|tag| windows_tags.contains(&&**tag)) { !graph.graph[node_index] .marker() - .is_disjoint(&WINDOWS_MARKERS) + .is_disjoint(*WINDOWS_MARKERS) } else if platform_tags.iter().all(|tag| tag.starts_with("macosx_")) { - !graph.graph[node_index].marker().is_disjoint(&MAC_MARKERS) + !graph.graph[node_index].marker().is_disjoint(*MAC_MARKERS) } else { true } @@ -860,7 +860,7 @@ impl Lock { || dist .fork_markers .iter() - .any(|marker| marker.evaluate(marker_env, &[])) + .any(|marker| marker.evaluate_no_extras(marker_env)) { if found_dist.is_some() { return Err(format!("found multiple packages matching `{name}`")); @@ -1449,7 +1449,9 @@ impl TryFrom for Lock { .map(|simplified_marker| simplified_marker.into_marker(&wire.requires_python)) // TODO(ag): Consider whether this should also deserialize a conflict marker. // We currently aren't serializing. Dropping it completely is likely to be wrong. - .map(|complexified_marker| UniversalMarker::new(complexified_marker, MarkerTree::TRUE)) + .map(|complexified_marker| { + UniversalMarker::new(complexified_marker, ConflictMarker::TRUE) + }) .collect(); let lock = Lock::new( wire.version, @@ -2251,7 +2253,7 @@ impl PackageWire { // TODO(ag): Consider whether this should also deserialize a conflict marker. // We currently aren't serializing. Dropping it completely is likely to be wrong. .map(|complexified_marker| { - UniversalMarker::new(complexified_marker, MarkerTree::TRUE) + UniversalMarker::new(complexified_marker, ConflictMarker::TRUE) }) .collect(), dependencies: unwire_deps(self.dependencies)?, @@ -3541,7 +3543,7 @@ impl Dependency { complexified_marker: UniversalMarker, ) -> Dependency { let simplified_marker = - SimplifiedMarkerTree::new(requires_python, complexified_marker.pep508()); + SimplifiedMarkerTree::new(requires_python, complexified_marker.combined()); Dependency { package_id, extra, @@ -3621,7 +3623,6 @@ struct DependencyWire { extra: BTreeSet, #[serde(default)] marker: SimplifiedMarkerTree, - // FIXME: Add support for representing conflict markers. } impl DependencyWire { @@ -3635,8 +3636,7 @@ impl DependencyWire { package_id: self.package_id.unwire(unambiguous_package_ids)?, extra: self.extra, simplified_marker: self.marker, - // FIXME: Support reading conflict markers. - complexified_marker: UniversalMarker::new(complexified_marker, MarkerTree::TRUE), + complexified_marker: UniversalMarker::from_combined(complexified_marker), }) } } diff --git a/crates/uv-resolver/src/lock/requirements_txt.rs b/crates/uv-resolver/src/lock/requirements_txt.rs index b84be3e42364..0f9bca1e31b1 100644 --- a/crates/uv-resolver/src/lock/requirements_txt.rs +++ b/crates/uv-resolver/src/lock/requirements_txt.rs @@ -20,7 +20,7 @@ use uv_pypi_types::{ParsedArchiveUrl, ParsedGitUrl}; use crate::graph_ops::marker_reachability; use crate::lock::{LockErrorKind, Package, PackageId, Source}; -use crate::universal_marker::UniversalMarker; +use crate::universal_marker::{ConflictMarker, UniversalMarker}; use crate::{InstallTarget, LockError}; /// An export of a [`Lock`] that renders in `requirements.txt` format. @@ -122,7 +122,7 @@ impl<'lock> RequirementsTxtExport<'lock> { // `marker_reachability` wants and it (probably) isn't // worth inventing a new abstraction so that it can accept // graphs with either `MarkerTree` or `UniversalMarker`. - MarkerTree::TRUE, + ConflictMarker::TRUE, ), ); @@ -175,7 +175,7 @@ impl<'lock> RequirementsTxtExport<'lock> { dep.simplified_marker.as_simplified_marker_tree(), // See note above for other `UniversalMarker::new` for // why this is OK. - MarkerTree::TRUE, + ConflictMarker::TRUE, ), ); diff --git a/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_unambiguous.snap b/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_unambiguous.snap index 0ee9dabab833..205f784d82e4 100644 --- a/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_unambiguous.snap +++ b/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_unambiguous.snap @@ -135,10 +135,7 @@ Ok( simplified_marker: SimplifiedMarkerTree( true, ), - complexified_marker: UniversalMarker { - pep508_marker: python_full_version >= '3.12', - conflict_marker: true, - }, + complexified_marker: python_full_version >= '3.12', }, ], optional_dependencies: {}, diff --git a/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_version_unambiguous.snap b/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_version_unambiguous.snap index 0ee9dabab833..205f784d82e4 100644 --- a/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_version_unambiguous.snap +++ b/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_source_version_unambiguous.snap @@ -135,10 +135,7 @@ Ok( simplified_marker: SimplifiedMarkerTree( true, ), - complexified_marker: UniversalMarker { - pep508_marker: python_full_version >= '3.12', - conflict_marker: true, - }, + complexified_marker: python_full_version >= '3.12', }, ], optional_dependencies: {}, diff --git a/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_version_unambiguous.snap b/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_version_unambiguous.snap index 0ee9dabab833..205f784d82e4 100644 --- a/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_version_unambiguous.snap +++ b/crates/uv-resolver/src/lock/snapshots/uv_resolver__lock__tests__missing_dependency_version_unambiguous.snap @@ -135,10 +135,7 @@ Ok( simplified_marker: SimplifiedMarkerTree( true, ), - complexified_marker: UniversalMarker { - pep508_marker: python_full_version >= '3.12', - conflict_marker: true, - }, + complexified_marker: python_full_version >= '3.12', }, ], optional_dependencies: {}, diff --git a/crates/uv-resolver/src/lock/target.rs b/crates/uv-resolver/src/lock/target.rs index 0f8f0ab98c34..dcbae78e6df9 100644 --- a/crates/uv-resolver/src/lock/target.rs +++ b/crates/uv-resolver/src/lock/target.rs @@ -3,7 +3,6 @@ use petgraph::Graph; use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet}; use std::collections::hash_map::Entry; use std::collections::{BTreeMap, VecDeque}; -use std::slice; use uv_configuration::{BuildOptions, DevGroupsManifest, ExtrasSpecification, InstallOptions}; use uv_distribution_types::{Edge, Node, Resolution, ResolvedDist}; use uv_normalize::{ExtraName, GroupName, PackageName, DEV_DEPENDENCIES}; @@ -164,6 +163,8 @@ impl<'env> InstallTarget<'env> { let mut queue: VecDeque<(&Package, Option<&ExtraName>)> = VecDeque::new(); let mut seen = FxHashSet::default(); + let mut activated_extras: Vec<(&PackageName, &ExtraName)> = vec![]; + let mut activated_groups: Vec<(&PackageName, &GroupName)> = vec![]; let root = petgraph.add_node(Node::Root); @@ -191,10 +192,12 @@ impl<'env> InstallTarget<'env> { petgraph.add_edge(root, index, Edge::Prod(MarkerTree::TRUE)); if dev.prod() { - // Push its dependencies on the queue. + // Push its dependencies on the queue and track + // activated extras. queue.push_back((dist, None)); for extra in extras.extra_names(dist.optional_dependencies.keys()) { queue.push_back((dist, Some(extra))); + activated_extras.push((&dist.id.name, extra)); } } @@ -211,9 +214,10 @@ impl<'env> InstallTarget<'env> { }) .flatten() { - if !dep.complexified_marker.evaluate(marker_env, &[]) { + if !dep.complexified_marker.evaluate_no_extras(marker_env) { continue; } + activated_groups.push((&dist.id.name, group)); let dep_dist = self.lock().find_by_id(&dep.package_id); @@ -254,9 +258,6 @@ impl<'env> InstallTarget<'env> { // a specific marker environment and set of extras/groups. // So at this point, we know the extras/groups have been // satisfied, so we can safely drop the conflict marker. - // - // FIXME: Make the above true. We aren't actually checking - // the conflict marker yet. Edge::Dev(group.clone(), dep.complexified_marker.pep508()), ); @@ -355,10 +356,11 @@ impl<'env> InstallTarget<'env> { Either::Right(package.dependencies.iter()) }; for dep in deps { - if !dep - .complexified_marker - .evaluate(marker_env, extra.map(slice::from_ref).unwrap_or_default()) - { + if !dep.complexified_marker.evaluate( + marker_env, + &activated_extras, + &activated_groups, + ) { continue; } diff --git a/crates/uv-resolver/src/lock/tree.rs b/crates/uv-resolver/src/lock/tree.rs index 3ae9b6169031..fc913844ced1 100644 --- a/crates/uv-resolver/src/lock/tree.rs +++ b/crates/uv-resolver/src/lock/tree.rs @@ -81,7 +81,7 @@ impl<'env> TreeDisplay<'env> { if dev.prod() { for dependency in &package.dependencies { if markers.is_some_and(|markers| { - !dependency.complexified_marker.evaluate(markers, &[]) + !dependency.complexified_marker.evaluate_no_extras(markers) }) { continue; } @@ -108,7 +108,7 @@ impl<'env> TreeDisplay<'env> { for (extra, dependencies) in &package.optional_dependencies { for dependency in dependencies { if markers.is_some_and(|markers| { - !dependency.complexified_marker.evaluate(markers, &[]) + !dependency.complexified_marker.evaluate_no_extras(markers) }) { continue; } @@ -137,7 +137,7 @@ impl<'env> TreeDisplay<'env> { if dev.contains(group) { for dependency in dependencies { if markers.is_some_and(|markers| { - !dependency.complexified_marker.evaluate(markers, &[]) + !dependency.complexified_marker.evaluate_no_extras(markers) }) { continue; } diff --git a/crates/uv-resolver/src/preferences.rs b/crates/uv-resolver/src/preferences.rs index 300ebed33d0d..c20901fc5139 100644 --- a/crates/uv-resolver/src/preferences.rs +++ b/crates/uv-resolver/src/preferences.rs @@ -155,7 +155,7 @@ impl Preferences { if !preference .fork_markers .iter() - .any(|marker| marker.evaluate(markers, &[])) + .any(|marker| marker.evaluate_no_extras(markers)) { trace!( "Excluding {preference} from preferences due to unmatched fork markers" diff --git a/crates/uv-resolver/src/resolution/display.rs b/crates/uv-resolver/src/resolution/display.rs index 8c3d5e6482f4..2e8af13803f2 100644 --- a/crates/uv-resolver/src/resolution/display.rs +++ b/crates/uv-resolver/src/resolution/display.rs @@ -66,7 +66,7 @@ impl<'a> DisplayResolutionGraph<'a> { for fork_marker in &underlying.fork_markers { assert!( fork_marker.conflict().is_true(), - "found fork marker {fork_marker} with non-trivial conflicting marker, \ + "found fork marker {fork_marker:?} with non-trivial conflicting marker, \ cannot display resolver output with conflicts in requirements.txt format", ); } diff --git a/crates/uv-resolver/src/resolution/output.rs b/crates/uv-resolver/src/resolution/output.rs index fb2d1c747267..61d76488ea72 100644 --- a/crates/uv-resolver/src/resolution/output.rs +++ b/crates/uv-resolver/src/resolution/output.rs @@ -21,14 +21,14 @@ use uv_pypi_types::{ Conflicts, HashDigest, ParsedUrlError, Requirement, VerbatimParsedUrl, Yanked, }; -use crate::graph_ops::marker_reachability; +use crate::graph_ops::{marker_reachability, simplify_conflict_markers}; use crate::pins::FilePins; use crate::preferences::Preferences; use crate::redirect::url_to_precise; use crate::resolution::AnnotatedDist; use crate::resolution_mode::ResolutionStrategy; use crate::resolver::{Resolution, ResolutionDependencyEdge, ResolutionPackage}; -use crate::universal_marker::UniversalMarker; +use crate::universal_marker::{ConflictMarker, UniversalMarker}; use crate::{ InMemoryIndex, MetadataResponse, Options, PythonRequirement, RequiresPython, ResolveError, VersionsResponse, @@ -72,6 +72,26 @@ impl ResolutionGraphNode { ResolutionGraphNode::Dist(dist) => &dist.marker, } } + + pub(crate) fn package_extra_names(&self) -> Option<(&PackageName, &ExtraName)> { + match *self { + ResolutionGraphNode::Root => None, + ResolutionGraphNode::Dist(ref dist) => { + let extra = dist.extra.as_ref()?; + Some((&dist.name, extra)) + } + } + } + + pub(crate) fn package_group_names(&self) -> Option<(&PackageName, &GroupName)> { + match *self { + ResolutionGraphNode::Root => None, + ResolutionGraphNode::Dist(ref dist) => { + let group = dist.dev.as_ref()?; + Some((&dist.name, group)) + } + } + } } impl Display for ResolutionGraphNode { @@ -179,12 +199,20 @@ impl ResolverOutput { // Compute and apply the marker reachability. let mut reachability = marker_reachability(&graph, &fork_markers); - // Apply the reachability to the graph. + // Apply the reachability to the graph and imbibe world + // knowledge about conflicts. + let conflict_marker = ConflictMarker::from_conflicts(conflicts); for index in graph.node_indices() { if let ResolutionGraphNode::Dist(dist) = &mut graph[index] { dist.marker = reachability.remove(&index).unwrap_or_default(); + dist.marker.imbibe(conflict_marker); } } + for weight in graph.edge_weights_mut() { + weight.imbibe(conflict_marker); + } + + simplify_conflict_markers(conflicts, &mut graph); // Discard any unreachable nodes. graph.retain_nodes(|graph, node| !graph[node].marker().is_false()); @@ -730,8 +758,8 @@ impl ResolverOutput { } let mut dupes = vec![]; for (name, marker_trees) in name_to_markers { - for (i, (version1, marker1)) in marker_trees.iter().enumerate() { - for (version2, marker2) in &marker_trees[i + 1..] { + for (i, (version1, &marker1)) in marker_trees.iter().enumerate() { + for (version2, &marker2) in &marker_trees[i + 1..] { if version1 == version2 { continue; } @@ -740,8 +768,8 @@ impl ResolverOutput { name: name.clone(), version1: (*version1).clone(), version2: (*version2).clone(), - marker1: *(*marker1), - marker2: *(*marker2), + marker1, + marker2, }); } } @@ -780,8 +808,8 @@ impl Display for ConflictingDistributionError { write!( f, "found conflicting versions for package `{name}`: - `{marker1}` (for version `{version1}`) is not disjoint with \ - `{marker2}` (for version `{version2}`)", + `{marker1:?}` (for version `{version1}`) is not disjoint with \ + `{marker2:?}` (for version `{version2}`)", ) } } diff --git a/crates/uv-resolver/src/resolution/requirements_txt.rs b/crates/uv-resolver/src/resolution/requirements_txt.rs index a033315872b6..3c121c5e2d2f 100644 --- a/crates/uv-resolver/src/resolution/requirements_txt.rs +++ b/crates/uv-resolver/src/resolution/requirements_txt.rs @@ -170,7 +170,7 @@ impl<'dist> RequirementsTxtDist<'dist> { pub(crate) fn from_annotated_dist(annotated: &'dist AnnotatedDist) -> Self { assert!( annotated.marker.conflict().is_true(), - "found dist {annotated} with non-trivial conflicting marker {marker}, \ + "found dist {annotated} with non-trivial conflicting marker {marker:?}, \ which cannot be represented in a `requirements.txt` format", marker = annotated.marker, ); @@ -181,7 +181,7 @@ impl<'dist> RequirementsTxtDist<'dist> { // OK because we've asserted above that this dist // does not have a non-trivial conflicting marker // that we would otherwise need to care about. - markers: annotated.marker.pep508(), + markers: annotated.marker.combined(), extras: if let Some(extra) = annotated.extra.clone() { vec![extra] } else { diff --git a/crates/uv-resolver/src/resolver/environment.rs b/crates/uv-resolver/src/resolver/environment.rs index a4863f3f946b..d06d01162e69 100644 --- a/crates/uv-resolver/src/resolver/environment.rs +++ b/crates/uv-resolver/src/resolver/environment.rs @@ -6,7 +6,7 @@ use uv_pypi_types::{ConflictItem, ConflictItemRef, ResolverMarkerEnvironment}; use crate::pubgrub::{PubGrubDependency, PubGrubPackage}; use crate::requires_python::RequiresPythonRange; use crate::resolver::ForkState; -use crate::universal_marker::UniversalMarker; +use crate::universal_marker::{ConflictMarker, UniversalMarker}; use crate::PythonRequirement; /// Represents one or more marker environments for a resolution. @@ -379,24 +379,13 @@ impl ResolverEnvironment { ref exclude, .. } => { - let mut conflict_marker = MarkerTree::TRUE; + let mut conflict_marker = ConflictMarker::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); - } + conflict_marker = + conflict_marker.and(ConflictMarker::from_conflict_item(item).negate()); } 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); - } + conflict_marker = conflict_marker.and(ConflictMarker::from_conflict_item(item)); } Some(UniversalMarker::new(*markers, conflict_marker)) } diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs index f8ee9441f0d3..532b8a00ce1f 100644 --- a/crates/uv-resolver/src/resolver/mod.rs +++ b/crates/uv-resolver/src/resolver/mod.rs @@ -63,7 +63,7 @@ use crate::resolver::environment::ForkingPossibility; pub use crate::resolver::environment::ResolverEnvironment; pub(crate) use crate::resolver::fork_map::{ForkMap, ForkSet}; pub(crate) use crate::resolver::urls::Urls; -use crate::universal_marker::UniversalMarker; +use crate::universal_marker::{ConflictMarker, UniversalMarker}; pub use crate::resolver::index::InMemoryIndex; use crate::resolver::indexes::Indexes; @@ -2664,8 +2664,10 @@ pub(crate) struct ResolutionDependencyEdge { impl ResolutionDependencyEdge { pub(crate) fn universal_marker(&self) -> UniversalMarker { - // FIXME: Account for extras and groups here. - UniversalMarker::new(self.marker, MarkerTree::TRUE) + // We specifically do not account for conflict + // markers here. Instead, those are computed via + // a traversal on the resolution graph. + UniversalMarker::new(self.marker, ConflictMarker::TRUE) } } diff --git a/crates/uv-resolver/src/universal_marker.rs b/crates/uv-resolver/src/universal_marker.rs index 372328de5c4f..767edbe781b2 100644 --- a/crates/uv-resolver/src/universal_marker.rs +++ b/crates/uv-resolver/src/universal_marker.rs @@ -1,9 +1,14 @@ -use uv_normalize::ExtraName; -use uv_pep508::{MarkerEnvironment, MarkerTree}; +use std::borrow::Borrow; + +use itertools::Itertools; + +use uv_normalize::{ExtraName, GroupName, PackageName}; +use uv_pep508::{MarkerEnvironment, MarkerEnvironmentBuilder, MarkerTree}; +use uv_pypi_types::{ConflictItem, ConflictPackage, Conflicts}; /// A representation of a marker for use in universal resolution. /// -/// (This also degrades gracefully to a standard PEP 508 marker in the case of +/// (This degrades gracefully to a standard PEP 508 marker in the case of /// non-universal resolution.) /// /// This universal marker is meant to combine both a PEP 508 marker and a @@ -13,74 +18,236 @@ use uv_pep508::{MarkerEnvironment, MarkerTree}; /// /// A universal marker evaluates to true only when *both* its PEP 508 marker /// and its conflict marker evaluate to true. -#[derive(Debug, Default, Copy, Clone, Eq, Hash, PartialEq, PartialOrd, Ord)] +#[derive(Default, Copy, Clone, Eq, Hash, PartialEq, PartialOrd, Ord)] pub struct UniversalMarker { - pep508_marker: MarkerTree, - conflict_marker: MarkerTree, + /// The full combined PEP 508 and "conflict" marker. + /// + /// In the original design, the PEP 508 marker was kept separate + /// from the conflict marker, since the conflict marker is not really + /// specified by PEP 508. However, this approach turned out to be + /// bunk because the conflict marker vary depending on which part of + /// the PEP 508 marker is true. For example, you might have a different + /// conflict marker for one platform versus the other. The only way to + /// resolve this is to combine them both into one marker. + /// + /// The downside of this is that since conflict markers aren't part of + /// PEP 508, combining them is pretty weird. We could combine them into + /// a new type of marker that isn't PEP 508. But it's not clear what the + /// best design for that is, and at the time of writing, it would have + /// been a lot of additional work. (Our PEP 508 marker implementation is + /// rather sophisticated given its boolean simplification capabilities. + /// So leveraging all that work is a huge shortcut.) So to accomplish + /// this, we technically preserve PEP 508 compatibility but abuse the + /// `extra` attribute to encode conflicts. + /// + /// So for example, if a particular dependency should only be activated + /// on `Darwin` and when the extra `x1` for package `foo` is enabled, + /// then its "universal" marker looks like this: + /// + /// ```text + /// sys_platform == 'Darwin' and extra == 'extra-3-foo-x1' + /// ``` + /// + /// Then, when `uv sync --extra x1` is called, we encode that was + /// `extra-3-foo-x1` and pass it as-needed when evaluating this marker. + /// + /// Why `extra-3-foo-x1`? + /// + /// * The `extra` prefix is there to distinguish it from `group`. + /// * The `3` is there to indicate the length of the package name, + /// in bytes. This isn't strictly necessary for encoding, but + /// is required if we were ever to need to decode a package and + /// extra/group name from a conflict marker. + /// * The `foo` package name ensures we namespace the extra/group name, + /// since multiple packages can have the same extra/group name. + /// + /// We only use alphanumeric characters and hyphens in order to limit + /// ourselves to valid extra names. (If we could use other characters then + /// that would avoid the need to encode the length of the package name.) + /// + /// So while the above marker is still technically valid from a PEP 508 + /// stand-point, evaluating it requires uv's custom encoding of extras (and + /// groups). + marker: MarkerTree, } impl UniversalMarker { /// A constant universal marker that always evaluates to `true`. pub(crate) const TRUE: UniversalMarker = UniversalMarker { - pep508_marker: MarkerTree::TRUE, - conflict_marker: MarkerTree::TRUE, + marker: MarkerTree::TRUE, }; /// A constant universal marker that always evaluates to `false`. pub(crate) const FALSE: UniversalMarker = UniversalMarker { - pep508_marker: MarkerTree::FALSE, - conflict_marker: MarkerTree::FALSE, + marker: MarkerTree::FALSE, }; /// Creates a new universal marker from its constituent pieces. - pub(crate) fn new(pep508_marker: MarkerTree, conflict_marker: MarkerTree) -> UniversalMarker { - UniversalMarker { - pep508_marker, - conflict_marker, - } + pub(crate) fn new( + mut pep508_marker: MarkerTree, + conflict_marker: ConflictMarker, + ) -> UniversalMarker { + pep508_marker.and(conflict_marker.marker); + UniversalMarker::from_combined(pep508_marker) + } + + /// Creates a new universal marker from a marker that has already been + /// combined from a PEP 508 and conflict marker. + pub(crate) fn from_combined(marker: MarkerTree) -> UniversalMarker { + UniversalMarker { marker } } /// Combine this universal marker with the one given in a way that unions /// them. That is, the updated marker will evaluate to `true` if `self` or /// `other` evaluate to `true`. pub(crate) fn or(&mut self, other: UniversalMarker) { - self.pep508_marker.or(other.pep508_marker); - self.conflict_marker.or(other.conflict_marker); + self.marker.or(other.marker); } /// Combine this universal marker with the one given in a way that /// intersects them. That is, the updated marker will evaluate to `true` if /// `self` and `other` evaluate to `true`. pub(crate) fn and(&mut self, other: UniversalMarker) { - self.pep508_marker.and(other.pep508_marker); - self.conflict_marker.and(other.conflict_marker); + self.marker.and(other.marker); + } + + /// Imbibes the world knowledge expressed by `conflicts` into this marker. + /// + /// This will effectively simplify the conflict marker in this universal + /// marker. In particular, it enables simplifying based on the fact that no + /// two items from the same set in the given conflicts can be active at a + /// given time. + pub(crate) fn imbibe(&mut self, conflicts: ConflictMarker) { + let self_marker = self.marker; + self.marker = conflicts.marker; + self.marker.implies(self_marker); + } + + /// Assumes that a given extra/group for the given package is activated. + /// + /// This may simplify the conflicting marker component of this universal + /// marker. + pub(crate) fn assume_conflict_item(&mut self, item: &ConflictItem) { + match *item.conflict() { + ConflictPackage::Extra(ref extra) => self.assume_extra(item.package(), extra), + ConflictPackage::Group(ref group) => self.assume_group(item.package(), group), + } + } + + /// Assumes that a given extra/group for the given package is not + /// activated. + /// + /// This may simplify the conflicting marker component of this universal + /// marker. + pub(crate) fn assume_not_conflict_item(&mut self, item: &ConflictItem) { + match *item.conflict() { + ConflictPackage::Extra(ref extra) => self.assume_not_extra(item.package(), extra), + ConflictPackage::Group(ref group) => self.assume_not_group(item.package(), group), + } + } + + /// Assumes that a given extra for the given package is activated. + /// + /// This may simplify the conflicting marker component of this universal + /// marker. + pub(crate) fn assume_extra(&mut self, package: &PackageName, extra: &ExtraName) { + let extra = encode_package_extra(package, extra); + self.marker = self + .marker + .simplify_extras_with(|candidate| *candidate == extra); + } + + /// Assumes that a given extra for the given package is not activated. + /// + /// This may simplify the conflicting marker component of this universal + /// marker. + pub(crate) fn assume_not_extra(&mut self, package: &PackageName, extra: &ExtraName) { + let extra = encode_package_extra(package, extra); + self.marker = self + .marker + .simplify_not_extras_with(|candidate| *candidate == extra); + } + + /// Assumes that a given group for the given package is activated. + /// + /// This may simplify the conflicting marker component of this universal + /// marker. + pub(crate) fn assume_group(&mut self, package: &PackageName, group: &GroupName) { + let extra = encode_package_group(package, group); + self.marker = self + .marker + .simplify_extras_with(|candidate| *candidate == extra); + } + + /// Assumes that a given group for the given package is not activated. + /// + /// This may simplify the conflicting marker component of this universal + /// marker. + pub(crate) fn assume_not_group(&mut self, package: &PackageName, group: &GroupName) { + let extra = encode_package_group(package, group); + self.marker = self + .marker + .simplify_not_extras_with(|candidate| *candidate == extra); } /// Returns true if this universal marker will always evaluate to `true`. - pub(crate) fn is_true(&self) -> bool { - self.pep508_marker.is_true() && self.conflict_marker.is_true() + pub(crate) fn is_true(self) -> bool { + self.marker.is_true() } /// Returns true if this universal marker will always evaluate to `false`. - pub(crate) fn is_false(&self) -> bool { - self.pep508_marker.is_false() || self.conflict_marker.is_false() + pub(crate) fn is_false(self) -> bool { + self.marker.is_false() } /// Returns true if this universal marker is disjoint with the one given. /// /// Two universal markers are disjoint when it is impossible for them both /// to evaluate to `true` simultaneously. - pub(crate) fn is_disjoint(self, other: &UniversalMarker) -> bool { - self.pep508_marker.is_disjoint(other.pep508_marker) - || self.conflict_marker.is_disjoint(other.conflict_marker) + pub(crate) fn is_disjoint(self, other: UniversalMarker) -> bool { + self.marker.is_disjoint(other.marker) } - /// Returns true if this universal marker is satisfied by the given - /// marker environment and list of activated extras. + /// Returns true if this universal marker is satisfied by the given marker + /// environment. /// - /// FIXME: This also needs to accept a list of groups. - pub(crate) fn evaluate(self, env: &MarkerEnvironment, extras: &[ExtraName]) -> bool { - self.pep508_marker.evaluate(env, extras) && self.conflict_marker.evaluate(env, extras) + /// This should only be used when evaluating a marker that is known not to + /// have any extras. For example, the PEP 508 markers on a fork. + pub(crate) fn evaluate_no_extras(self, env: &MarkerEnvironment) -> bool { + self.marker.evaluate(env, &[]) + } + + /// Returns true if this universal marker is satisfied by the given marker + /// environment and list of activated extras and groups. + /// + /// The activated extras and groups should be the complete set activated + /// for a particular context. And each extra and group must be scoped to + /// the particular package that it's enabled for. + pub(crate) fn evaluate( + self, + env: &MarkerEnvironment, + extras: &[(P, E)], + groups: &[(P, G)], + ) -> bool + where + P: Borrow, + E: Borrow, + G: Borrow, + { + let extras = extras + .iter() + .map(|(package, extra)| encode_package_extra(package.borrow(), extra.borrow())); + let groups = groups + .iter() + .map(|(package, group)| encode_package_group(package.borrow(), group.borrow())); + self.marker + .evaluate(env, &extras.chain(groups).collect::>()) + } + + /// Returns the internal marker that combines both the PEP 508 + /// and conflict marker. + pub(crate) fn combined(self) -> MarkerTree { + self.marker } /// Returns the PEP 508 marker for this universal marker. @@ -92,7 +259,7 @@ impl UniversalMarker { /// always use a universal marker since it accounts for all possible ways /// for a package to be installed. pub fn pep508(self) -> MarkerTree { - self.pep508_marker + self.marker.without_extras() } /// Returns the non-PEP 508 marker expression that represents conflicting @@ -106,26 +273,359 @@ impl UniversalMarker { /// of non-trivial conflict markers and fails if any are found. (Because /// conflict markers cannot be represented in the `requirements.txt` /// format.) - pub fn conflict(self) -> MarkerTree { - self.conflict_marker + pub(crate) fn conflict(self) -> ConflictMarker { + ConflictMarker { + marker: self.marker.only_extras(), + } } } -impl std::fmt::Display for UniversalMarker { +impl std::fmt::Debug for UniversalMarker { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { - if self.pep508_marker.is_false() || self.conflict_marker.is_false() { - return write!(f, "`false`"); + std::fmt::Debug::fmt(&self.marker, f) + } +} + +/// A marker that is only for representing conflicting extras/groups. +/// +/// This encapsulates the encoding of extras and groups into PEP 508 +/// markers. +#[derive(Default, Clone, Copy, Eq, Hash, PartialEq, PartialOrd, Ord)] +pub struct ConflictMarker { + marker: MarkerTree, +} + +impl ConflictMarker { + /// A constant conflict marker that always evaluates to `true`. + pub const TRUE: ConflictMarker = ConflictMarker { + marker: MarkerTree::TRUE, + }; + + /// A constant conflict marker that always evaluates to `false`. + pub const FALSE: ConflictMarker = ConflictMarker { + marker: MarkerTree::FALSE, + }; + + /// Creates a new conflict marker from the declared conflicts provided. + pub fn from_conflicts(conflicts: &Conflicts) -> ConflictMarker { + if conflicts.is_empty() { + return ConflictMarker::TRUE; } - match ( - self.pep508_marker.contents(), - self.conflict_marker.contents(), - ) { - (None, None) => write!(f, "`true`"), - (Some(pep508), None) => write!(f, "`{pep508}`"), - (None, Some(conflict)) => write!(f, "`true` (conflict marker: `{conflict}`)"), - (Some(pep508), Some(conflict)) => { - write!(f, "`{pep508}` (conflict marker: `{conflict}`)") + let mut marker = ConflictMarker::TRUE; + for set in conflicts.iter() { + for (item1, item2) in set.iter().tuple_combinations() { + let pair = ConflictMarker::from_conflict_item(item1) + .negate() + .or(ConflictMarker::from_conflict_item(item2).negate()); + marker = marker.and(pair); } } + marker + } + + /// Create a conflict marker that is true only when the given extra or + /// group (for a specific package) is activated. + pub fn from_conflict_item(item: &ConflictItem) -> ConflictMarker { + match *item.conflict() { + ConflictPackage::Extra(ref extra) => ConflictMarker::extra(item.package(), extra), + ConflictPackage::Group(ref group) => ConflictMarker::group(item.package(), group), + } + } + + /// Create a conflict marker that is true only when the given extra for the + /// given package is activated. + pub fn extra(package: &PackageName, extra: &ExtraName) -> ConflictMarker { + let operator = uv_pep508::ExtraOperator::Equal; + let name = uv_pep508::MarkerValueExtra::Extra(encode_package_extra(package, extra)); + let expr = uv_pep508::MarkerExpression::Extra { operator, name }; + let marker = MarkerTree::expression(expr); + ConflictMarker { marker } + } + + /// Create a conflict marker that is true only when the given group for the + /// given package is activated. + pub fn group(package: &PackageName, group: &GroupName) -> ConflictMarker { + let operator = uv_pep508::ExtraOperator::Equal; + let name = uv_pep508::MarkerValueExtra::Extra(encode_package_group(package, group)); + let expr = uv_pep508::MarkerExpression::Extra { operator, name }; + let marker = MarkerTree::expression(expr); + ConflictMarker { marker } + } + + /// Returns a new conflict marker that is the negation of this one. + #[must_use] + pub fn negate(self) -> ConflictMarker { + ConflictMarker { + marker: self.marker.negate(), + } + } + + /// Returns a new conflict marker corresponding to the union of `self` and + /// `other`. + #[must_use] + pub fn or(self, other: ConflictMarker) -> ConflictMarker { + let mut marker = self.marker; + marker.or(other.marker); + ConflictMarker { marker } + } + + /// Returns a new conflict marker corresponding to the intersection of + /// `self` and `other`. + #[must_use] + pub fn and(self, other: ConflictMarker) -> ConflictMarker { + let mut marker = self.marker; + marker.and(other.marker); + ConflictMarker { marker } + } + + /// Returns a new conflict marker corresponding to the logical implication + /// of `self` and the given consequent. + /// + /// If the conflict marker returned is always `true`, then it can be said + /// that `self` implies `consequent`. + #[must_use] + pub fn implies(self, other: ConflictMarker) -> ConflictMarker { + let mut marker = self.marker; + marker.implies(other.marker); + ConflictMarker { marker } + } + + /// Returns true if this conflict marker will always evaluate to `true`. + pub fn is_true(self) -> bool { + self.marker.is_true() + } + + /// Returns true if this conflict marker will always evaluate to `false`. + pub fn is_false(self) -> bool { + self.marker.is_false() + } + + /// Returns true if this conflict marker is satisfied by the given + /// list of activated extras and groups. + pub(crate) fn evaluate( + self, + extras: &[(PackageName, ExtraName)], + groups: &[(PackageName, GroupName)], + ) -> bool { + static DUMMY: std::sync::LazyLock = std::sync::LazyLock::new(|| { + MarkerEnvironment::try_from(MarkerEnvironmentBuilder { + implementation_name: "", + implementation_version: "3.7", + os_name: "linux", + platform_machine: "", + platform_python_implementation: "", + platform_release: "", + platform_system: "", + platform_version: "", + python_full_version: "3.7", + python_version: "3.7", + sys_platform: "linux", + }) + .unwrap() + }); + let extras = extras + .iter() + .map(|(package, extra)| encode_package_extra(package, extra)); + let groups = groups + .iter() + .map(|(package, group)| encode_package_group(package, group)); + self.marker + .evaluate(&DUMMY, &extras.chain(groups).collect::>()) + } +} + +impl std::fmt::Debug for ConflictMarker { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + // This is a little more succinct than the default. + write!(f, "ConflictMarker({:?})", self.marker) + } +} + +/// Encodes the given package name and its corresponding extra into a valid +/// `extra` value in a PEP 508 marker. +fn encode_package_extra(package: &PackageName, extra: &ExtraName) -> ExtraName { + // This is OK because `PackageName` and `ExtraName` have the same + // validation rules, and we combine them in a way that always results in a + // valid name. + // + // Note also that we encode the length of the package name (in bytes) into + // the encoded extra name as well. This ensures we can parse out both the + // package and extra name if necessary. If we didn't do this, then some + // cases could be ambiguous since our field delimiter (`-`) is also a valid + // character in `package` or `extra` values. But if we know the length of + // the package name, we can always parse each field unambiguously. + let package_len = package.as_str().len(); + ExtraName::new(format!("extra-{package_len}-{package}-{extra}")).unwrap() +} + +/// Encodes the given package name and its corresponding group into a valid +/// `extra` value in a PEP 508 marker. +fn encode_package_group(package: &PackageName, group: &GroupName) -> ExtraName { + // See `encode_package_extra`, the same considerations apply here. + let package_len = package.as_str().len(); + ExtraName::new(format!("group-{package_len}-{package}-{group}")).unwrap() +} + +#[cfg(test)] +mod tests { + use super::*; + + use uv_pypi_types::ConflictSet; + + /// Creates a collection of declared conflicts from the sets + /// provided. + fn create_conflicts(it: impl IntoIterator) -> Conflicts { + let mut conflicts = Conflicts::empty(); + for set in it { + conflicts.push(set); + } + conflicts + } + + /// Creates a single set of conflicting items. + /// + /// For convenience, this always creates conflicting items with a package + /// name of `foo` and with the given string as the extra name. + fn create_set<'a>(it: impl IntoIterator) -> ConflictSet { + let items = it + .into_iter() + .map(|extra| (create_package("pkg"), create_extra(extra))) + .map(ConflictItem::from) + .collect::>(); + ConflictSet::try_from(items).unwrap() + } + + /// Shortcut for creating a package name. + fn create_package(name: &str) -> PackageName { + PackageName::new(name.to_string()).unwrap() + } + + /// Shortcut for creating an extra name. + fn create_extra(name: &str) -> ExtraName { + ExtraName::new(name.to_string()).unwrap() + } + + /// Shortcut for creating a conflict marker from an extra name. + fn create_extra_marker(name: &str) -> ConflictMarker { + ConflictMarker::extra(&create_package("pkg"), &create_extra(name)) + } + + /// Returns a string representation of the given conflict marker. + /// + /// This is just the underlying marker. And if it's `true`, then a + /// non-conforming `true` string is returned. (Which is fine since + /// this is just for tests.) + fn tostr(cm: ConflictMarker) -> String { + cm.marker + .try_to_string() + .unwrap_or_else(|| "true".to_string()) + } + + /// This tests the conversion from declared conflicts into a conflict + /// marker. This is used to describe "world knowledge" about which + /// extras/groups are and aren't allowed to be activated together. + #[test] + fn conflicts_as_marker() { + let conflicts = create_conflicts([create_set(["foo", "bar"])]); + let cm = ConflictMarker::from_conflicts(&conflicts); + assert_eq!( + tostr(cm), + "extra != 'extra-3-pkg-foo' or extra != 'extra-3-pkg-bar'" + ); + + let conflicts = create_conflicts([create_set(["foo", "bar", "baz"])]); + let cm = ConflictMarker::from_conflicts(&conflicts); + assert_eq!( + tostr(cm), + "(extra != 'extra-3-pkg-baz' and extra != 'extra-3-pkg-foo') \ + or (extra != 'extra-3-pkg-bar' and extra != 'extra-3-pkg-foo') \ + or (extra != 'extra-3-pkg-bar' and extra != 'extra-3-pkg-baz')", + ); + + let conflicts = create_conflicts([create_set(["foo", "bar"]), create_set(["fox", "ant"])]); + let cm = ConflictMarker::from_conflicts(&conflicts); + assert_eq!( + tostr(cm), + "(extra == 'extra-3-pkg-bar' and extra != 'extra-3-pkg-foo' and extra != 'extra-3-pkg-fox') \ + or (extra != 'extra-3-pkg-bar' and extra != 'extra-3-pkg-fox') \ + or (extra != 'extra-3-pkg-ant' and extra != 'extra-3-pkg-foo') \ + or (extra != 'extra-3-pkg-ant' and extra != 'extra-3-pkg-bar')", + ); + // I believe because markers are put into DNF, the marker we get here + // is a lot bigger than what we might expect. Namely, this is how it's + // constructed: + // + // (extra != 'extra-3-pkg-foo' or extra != 'extra-3-pkg-bar') + // and (extra != 'extra-3-pkg-fox' or extra != 'extra-3-pkg-ant') + // + // In other words, you can't have both `foo` and `bar` active, and you + // can't have both `fox` and `ant` active. But any other combination + // is valid. So let's step through all of them to make sure the marker + // below gives the expected result. (I did this because it's not at all + // obvious to me that the above two markers are equivalent.) + let disallowed = [ + vec!["foo", "bar"], + vec!["fox", "ant"], + vec!["foo", "fox", "bar"], + vec!["foo", "ant", "bar"], + vec!["ant", "foo", "fox"], + vec!["ant", "bar", "fox"], + vec!["foo", "bar", "fox", "ant"], + ]; + for extra_names in disallowed { + let extras = extra_names + .iter() + .copied() + .map(|name| (create_package("pkg"), create_extra(name))) + .collect::>(); + assert!( + !cm.evaluate(&extras, &[]), + "expected `{extra_names:?}` to evaluate to `false` in `{cm:?}`" + ); + } + let allowed = [ + vec![], + vec!["foo"], + vec!["bar"], + vec!["fox"], + vec!["ant"], + vec!["foo", "fox"], + vec!["foo", "ant"], + vec!["bar", "fox"], + vec!["bar", "ant"], + ]; + for extra_names in allowed { + let extras = extra_names + .iter() + .copied() + .map(|name| (create_package("pkg"), create_extra(name))) + .collect::>(); + assert!( + cm.evaluate(&extras, &[]), + "expected `{extra_names:?}` to evaluate to `true` in `{cm:?}`" + ); + } + } + + /// This tests conflict marker simplification after "imbibing" world + /// knowledge about which extras/groups cannot be activated together. + #[test] + fn imbibe() { + let conflicts = create_conflicts([create_set(["foo", "bar"])]); + let conflicts_marker = ConflictMarker::from_conflicts(&conflicts); + let foo = create_extra_marker("foo"); + let bar = create_extra_marker("bar"); + + // In this case, we simulate a dependency whose conflict marker + // is just repeating the fact that conflicting extras cannot + // both be activated. So this one simplifies to `true`. + let mut dep_conflict_marker = + UniversalMarker::new(MarkerTree::TRUE, foo.negate().or(bar.negate())); + assert_eq!( + format!("{dep_conflict_marker:?}"), + "extra != 'extra-3-pkg-foo' or extra != 'extra-3-pkg-bar'" + ); + dep_conflict_marker.imbibe(conflicts_marker); + assert_eq!(format!("{dep_conflict_marker:?}"), "true"); } }