From 94059e87f01451697d785220bc68841a4ab0ae5a Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Wed, 27 Nov 2024 08:31:07 -0500 Subject: [PATCH] wip --- crates/uv-resolver/src/candidate_selector.rs | 4 +- crates/uv-resolver/src/graph_ops.rs | 123 ++++++++++++++---- crates/uv-resolver/src/lock/mod.rs | 6 +- ...missing_dependency_source_unambiguous.snap | 3 +- ...dependency_source_version_unambiguous.snap | 3 +- ...issing_dependency_version_unambiguous.snap | 3 +- crates/uv-resolver/src/resolution/output.rs | 3 +- .../src/resolution/requirements_txt.rs | 2 +- crates/uv-resolver/src/universal_marker.rs | 119 ++++++++++------- crates/uv/src/commands/project/lock.rs | 1 - crates/uv/tests/it/lock.rs | 2 +- 11 files changed, 182 insertions(+), 87 deletions(-) diff --git a/crates/uv-resolver/src/candidate_selector.rs b/crates/uv-resolver/src/candidate_selector.rs index ed6c0690602b..aae2e4e2b003 100644 --- a/crates/uv-resolver/src/candidate_selector.rs +++ b/crates/uv-resolver/src/candidate_selector.rs @@ -140,10 +140,10 @@ impl CandidateSelector { // first has the matching half and then the mismatching half. let preferences_match = preferences .get(package_name) - .filter(|(marker, _index, _version)| env.included_by_marker(marker.pep508())); + .filter(|(marker, _index, _version)| env.included_by_marker(&marker.pep508())); let preferences_mismatch = preferences .get(package_name) - .filter(|(marker, _index, _version)| !env.included_by_marker(marker.pep508())); + .filter(|(marker, _index, _version)| !env.included_by_marker(&marker.pep508())); let preferences = preferences_match.chain(preferences_mismatch).filter_map( |(marker, source, version)| { // If the package is mapped to an explicit index, only consider preferences that diff --git a/crates/uv-resolver/src/graph_ops.rs b/crates/uv-resolver/src/graph_ops.rs index 8797d7023914..48403cdd8bac 100644 --- a/crates/uv-resolver/src/graph_ops.rs +++ b/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, PackageName}; +use uv_pypi_types::Conflicts; use crate::resolution::ResolutionGraphNode; use crate::universal_marker::UniversalMarker; @@ -88,11 +89,21 @@ pub(crate) fn marker_reachability( /// 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. -pub(crate) fn simplify_conflict_markers(graph: &mut Graph) { +pub(crate) fn simplify_conflict_markers( + conflicts: &Conflicts, + graph: &mut Graph, +) { + #[derive(Clone, Debug, Eq, Hash, PartialEq)] + struct Inference { + package: PackageName, + extra: ExtraName, + included: bool, + } + // The set of activated extras (and TODO, in the future, groups) // for each node. The ROOT nodes don't have any extras activated. - let mut activated: FxHashMap> = - FxHashMap::with_capacity_and_hasher(graph.node_count(), FxBuildHasher); + let mut activated: FxHashMap>> = + FxHashMap::default(); // Collect the root nodes. // @@ -108,35 +119,101 @@ pub(crate) fn simplify_conflict_markers(graph: &mut Graph> = - FxHashMap::default(); + // let mut assume_by_edge: FxHashMap> = + // FxHashMap::default(); let mut seen: FxHashSet = FxHashSet::default(); while let Some(parent_index) = queue.pop() { - for child_edge in graph.edges_directed(parent_index, Direction::Outgoing) { - // TODO: The below seems excessively clone-y. - // Consider tightening this up a bit. - let target = child_edge.target(); - let mut extras: FxHashSet<(PackageName, ExtraName)> = - activated.get(&parent_index).cloned().unwrap_or_default(); - if let Some((package, extra)) = graph[parent_index].package_extra_names() { - extras.insert((package.clone(), extra.clone())); + 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((package.clone(), extra.clone())); } - if let Some((package, extra)) = graph[target].package_extra_names() { - extras.insert((package.clone(), extra.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); } - activated.entry(target).or_default().extend(extras.clone()); - assume_by_edge - .entry(child_edge.id()) - .or_default() - .extend(extras); if seen.insert(child_edge.target()) { queue.push(child_edge.target()); } } } - for (edge_id, extras) in assume_by_edge { - for &(ref package, ref extra) in &extras { - graph[edge_id].assume_extra(package, extra); + + let mut inferences: FxHashMap>> = FxHashMap::default(); + for (node_id, sets) in activated { + let mut new_sets = vec![]; + for set in sets { + let definitive_conflict = conflicts.iter().any(|conflict_set| { + set.iter() + .filter(|(package, extra)| conflict_set.contains(package, extra)) + .count() + >= 2 + }); + if definitive_conflict { + continue; + } + + let mut new_set = FxHashSet::default(); + for (package, extra) in set { + for conflict_set in conflicts.iter() { + if !conflict_set.contains(&package, &extra) { + continue; + } + for conflict_item in conflict_set.iter() { + let Some(conflict_item_extra) = conflict_item.extra() else { + continue; + }; + if conflict_item.package() == &package && conflict_item_extra == &extra { + continue; + } + new_set.insert(Inference { + package: conflict_item.package().clone(), + extra: conflict_item_extra.clone(), + included: false, + }); + } + } + new_set.insert(Inference { + package, + extra, + 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) = inferences.get(&from_index) else { + continue; + }; + let all_paths_satisfied = inference.iter().all(|set| { + let extras = set + .iter() + .filter_map(|inf| { + if !inf.included { + return None; + } + Some((inf.package.clone(), inf.extra.clone())) + }) + .collect::>(); + graph[edge_index].conflict().evaluate(&extras) + }); + if all_paths_satisfied { + for set in inference { + for inf in set { + if inf.included { + graph[edge_index].assume_extra(&inf.package, &inf.extra); + } else { + graph[edge_index].assume_not_extra(&inf.package, &inf.extra); + } + } + } } } } diff --git a/crates/uv-resolver/src/lock/mod.rs b/crates/uv-resolver/src/lock/mod.rs index 2c43f99c6be2..c2e964e61108 100644 --- a/crates/uv-resolver/src/lock/mod.rs +++ b/crates/uv-resolver/src/lock/mod.rs @@ -3537,7 +3537,7 @@ impl Dependency { complexified_marker: UniversalMarker, ) -> Dependency { let simplified_marker = - SimplifiedMarkerTree::new(requires_python, complexified_marker.pep508().clone()); + SimplifiedMarkerTree::new(requires_python, complexified_marker.combined().clone()); Dependency { package_id, extra, @@ -3582,12 +3582,14 @@ impl Dependency { if let Some(marker) = self.simplified_marker.try_to_string() { table.insert("marker", value(marker)); } + /* if !self.complexified_marker.conflict().is_true() { table.insert( "conflict-marker", - value(self.complexified_marker.conflict().to_string()), + value(self.complexified_marker.combined().to_string()), ); } + */ table } 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 52b180c7e329..c85f00fe72ec 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 @@ -136,8 +136,7 @@ Ok( true, ), complexified_marker: UniversalMarker { - pep508_marker: python_full_version >= '3.12', - conflict_marker: ConflictMarker(true), + marker: python_full_version >= '3.12', }, }, ], 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 52b180c7e329..c85f00fe72ec 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 @@ -136,8 +136,7 @@ Ok( true, ), complexified_marker: UniversalMarker { - pep508_marker: python_full_version >= '3.12', - conflict_marker: ConflictMarker(true), + marker: python_full_version >= '3.12', }, }, ], 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 52b180c7e329..c85f00fe72ec 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 @@ -136,8 +136,7 @@ Ok( true, ), complexified_marker: UniversalMarker { - pep508_marker: python_full_version >= '3.12', - conflict_marker: ConflictMarker(true), + marker: python_full_version >= '3.12', }, }, ], diff --git a/crates/uv-resolver/src/resolution/output.rs b/crates/uv-resolver/src/resolution/output.rs index 24ca921ac2d8..102802d7676c 100644 --- a/crates/uv-resolver/src/resolution/output.rs +++ b/crates/uv-resolver/src/resolution/output.rs @@ -200,7 +200,8 @@ impl ResolverOutput { for weight in graph.edge_weights_mut() { weight.imbibe(conflicts); } - simplify_conflict_markers(&mut graph); + + simplify_conflict_markers(conflicts, &mut graph); // Discard any unreachable nodes. graph.retain_nodes(|graph, node| !graph[node].marker().is_false()); diff --git a/crates/uv-resolver/src/resolution/requirements_txt.rs b/crates/uv-resolver/src/resolution/requirements_txt.rs index 6a956ce9d091..5b5fd24dc6ec 100644 --- a/crates/uv-resolver/src/resolution/requirements_txt.rs +++ b/crates/uv-resolver/src/resolution/requirements_txt.rs @@ -180,7 +180,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/universal_marker.rs b/crates/uv-resolver/src/universal_marker.rs index f3f275a829a8..78a1ddee01f6 100644 --- a/crates/uv-resolver/src/universal_marker.rs +++ b/crates/uv-resolver/src/universal_marker.rs @@ -7,12 +7,6 @@ use uv_normalize::{ExtraName, PackageName}; use uv_pep508::{MarkerEnvironment, MarkerEnvironmentBuilder, MarkerTree}; use uv_pypi_types::Conflicts; -// BREADCRUMBS: Work on a new `ConflictMarker` type instead of using -// `MarkerTree` directly below. And instead of storing only extra/group -// names, we need to store package names too. Wire everything up. This -// will also take us toward making groups work. But keep going with just -// extras for now. - /// A representation of a marker for use in universal resolution. /// /// (This also degrades gracefully to a standard PEP 508 marker in the case of @@ -27,48 +21,51 @@ use uv_pypi_types::Conflicts; /// and its conflict marker evaluate to true. #[derive(Debug, Default, Clone, Eq, Hash, PartialEq, PartialOrd, Ord)] pub struct UniversalMarker { - pep508_marker: MarkerTree, - conflict_marker: ConflictMarker, + 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: ConflictMarker::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: ConflictMarker::FALSE, + marker: MarkerTree::FALSE, }; /// Creates a new universal marker from its constituent pieces. pub(crate) fn new( - pep508_marker: MarkerTree, + mut pep508_marker: MarkerTree, conflict_marker: ConflictMarker, ) -> UniversalMarker { + pep508_marker.and(conflict_marker.marker); UniversalMarker { - pep508_marker, - conflict_marker, + marker: pep508_marker, } } + /// Creates a new universal marker from just a PEP 508 marker. + /// + /// This sets the "conflict" marker portion of this universal marker to + /// `true`. + pub(crate) fn from_pep508(marker: MarkerTree) -> UniversalMarker { + UniversalMarker::new(marker, ConflictMarker::TRUE) + } + /// 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 = 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 = self.conflict_marker.and(other.conflict_marker); + self.marker.and(other.marker); } /// Imbibes the world knowledge expressed by `conflicts` into this marker. @@ -100,9 +97,11 @@ impl UniversalMarker { marker = marker.or(pair); } } - self.conflict_marker = marker - .negate() - .implies(std::mem::take(&mut self.conflict_marker)); + + let mut conflict_marker = marker.negate().marker; + let self_marker = std::mem::take(&mut self.marker); + conflict_marker.implies(self_marker); + self.marker = conflict_marker; } /// Assumes that a given extra for the given package is activated. @@ -110,17 +109,35 @@ impl UniversalMarker { /// This may simplify the conflicting marker component of this universal /// marker. pub(crate) fn assume_extra(&mut self, package: &PackageName, extra: &ExtraName) { - self.conflict_marker = self.conflict_marker.assume_extra(package, extra); + // self.conflict_marker = self.conflict_marker.assume_extra(package, extra); + let extra = encode_package_extra(package, extra); + self.marker = self + .marker + .clone() + .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) { + // self.conflict_marker = self.conflict_marker.assume_extra(package, extra); + let extra = encode_package_extra(package, extra); + self.marker = self + .marker + .clone() + .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() + 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() + self.marker.is_false() } /// Returns true if this universal marker is disjoint with the one given. @@ -128,22 +145,13 @@ impl UniversalMarker { /// 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) + self.marker.is_disjoint(&other.marker) } /// Returns true if this universal marker is satisfied by the given /// marker environment and list of activated extras. pub(crate) fn evaluate(&self, env: &MarkerEnvironment, extras: &[ExtraName]) -> bool { - // We specifically evaluate the conflict marker with an empty set of - // extras because this seems to best capture the intent here. Namely, - // in this context, we don't know the packages associated with the - // given extras, which means we can't really evaluate the conflict - // marker. But note that evaluation here is not vacuous. By providing - // an empty list, conflict markers like `extra != 'package[x1]'` - // will evaluate to `true`, while conflict markers like - // `extra == 'package[x2]'` will evaluate to `false`. - self.pep508_marker.evaluate(env, extras) && self.conflict_marker.evaluate(&[]) + self.marker.evaluate(env, extras) } /// Returns true if this universal marker is satisfied by the given @@ -156,16 +164,16 @@ impl UniversalMarker { activated_extras: &[(PackageName, ExtraName)], _dev: &DevGroupsManifest, ) -> bool { - if !self.pep508_marker.evaluate(env, &[]) { - return false; - } - // let extra_list = match *extras { - // // TODO(ag): This should still evaluate `dev`. - // ExtrasSpecification::All => return true, - // ExtrasSpecification::None => &[][..], - // ExtrasSpecification::Some(ref list) => list, - // }; - self.conflict_marker.evaluate(activated_extras) + let extras = activated_extras + .iter() + .map(|&(ref package, ref extra)| encode_package_extra(package, extra)) + .collect::>(); + self.marker.evaluate(env, &extras) + } + + /// TODO + pub fn combined(&self) -> &MarkerTree { + &self.marker } /// Returns the PEP 508 marker for this universal marker. @@ -176,8 +184,8 @@ impl UniversalMarker { /// producing different versions of the same package), then one should /// 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 + pub fn pep508(&self) -> MarkerTree { + self.marker.clone().without_extras() } /// Returns the non-PEP 508 marker expression that represents conflicting @@ -191,13 +199,23 @@ 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) -> &ConflictMarker { - &self.conflict_marker + pub fn conflict(&self) -> ConflictMarker { + ConflictMarker { + marker: self.marker.clone().only_extras(), + } } } impl std::fmt::Display for UniversalMarker { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + if self.is_true() { + return write!(f, "true"); + } + if self.is_false() { + return write!(f, "false"); + } + std::fmt::Display::fmt(&self.marker.contents().unwrap(), f) + /* if self.pep508_marker.is_false() || self.conflict_marker.is_false() { return write!(f, "`false`"); } @@ -216,6 +234,7 @@ impl std::fmt::Display for UniversalMarker { ) } } + */ } } diff --git a/crates/uv/src/commands/project/lock.rs b/crates/uv/src/commands/project/lock.rs index 6a095b036a64..2606e13a998e 100644 --- a/crates/uv/src/commands/project/lock.rs +++ b/crates/uv/src/commands/project/lock.rs @@ -605,7 +605,6 @@ async fn do_lock( lock.fork_markers() .iter() .map(UniversalMarker::pep508) - .cloned() .collect() }) .unwrap_or_else(|| { diff --git a/crates/uv/tests/it/lock.rs b/crates/uv/tests/it/lock.rs index 7b20b21a8e28..6fe43ad4ce0d 100644 --- a/crates/uv/tests/it/lock.rs +++ b/crates/uv/tests/it/lock.rs @@ -4194,7 +4194,7 @@ fn lock_conflicting_mixed() -> Result<()> { [package.dev-dependencies] group1 = [ - { name = "sortedcontainers", version = "2.3.0", source = { registry = "https://pypi.org/simple" }, conflict-marker = "extra != 'extra-project-extra1'" }, + { name = "sortedcontainers", version = "2.3.0", source = { registry = "https://pypi.org/simple" }, marker = "extra != 'extra-project-extra1'" }, ] [package.metadata]