diff --git a/crates/uv-pep508/src/marker/algebra.rs b/crates/uv-pep508/src/marker/algebra.rs index 4a18acd2fff04..02e18ac44f325 100644 --- a/crates/uv-pep508/src/marker/algebra.rs +++ b/crates/uv-pep508/src/marker/algebra.rs @@ -402,7 +402,7 @@ impl InternerGuard<'_> { // Restrict this variable to the given output by merging it // with the relevant child. let node = if value { high } else { low }; - return node.negate(i); + return self.restrict(node.negate(i), f); } } @@ -411,6 +411,67 @@ impl InternerGuard<'_> { self.create_node(node.var.clone(), children) } + /// Returns a new tree where the only nodes remaining are non-`extra` + /// nodes. + /// + /// If there are only `extra` nodes, then this returns a tree that is + /// always true. + /// + /// This works by assuming all `extra` nodes are always true. + pub(crate) fn without_extras(&mut self, mut i: NodeId) -> NodeId { + if matches!(i, NodeId::TRUE | NodeId::FALSE) { + return i; + } + + let parent = i; + let node = self.shared.node(i); + if matches!(node.var, Variable::Extra(_)) { + i = NodeId::FALSE; + for child in node.children.nodes() { + i = self.or(i, child.negate(parent)); + } + if i.is_true() { + return NodeId::TRUE; + } + self.without_extras(i.negate(parent)) + } else { + // Restrict all nodes recursively. + let children = node.children.map(i, |node| self.without_extras(node)); + self.create_node(node.var.clone(), children) + } + } + + /// Returns a new tree where the only nodes remaining are `extra` nodes. + /// + /// If there are no extra nodes, then this returns a tree that is always + /// true. + /// + /// This works by assuming all non-`extra` nodes are always true. + pub(crate) fn only_extras(&mut self, mut i: NodeId) -> NodeId { + if matches!(i, NodeId::TRUE | NodeId::FALSE) { + return i; + } + + let parent = i; + let node = self.shared.node(i); + if !matches!(node.var, Variable::Extra(_)) { + i = NodeId::FALSE; + for child in node.children.nodes() { + i = self.or(i, child.negate(parent)); + } + if i.is_true() { + return NodeId::TRUE; + } + // node = self.shared.node(i.negate(parent)); + // self.only_extras(i.negate(parent)) + self.only_extras(i) + } else { + // Restrict all nodes recursively. + let children = node.children.map(i, |node| self.only_extras(node)); + self.create_node(node.var.clone(), children) + } + } + /// Simplify this tree by *assuming* that the Python version range provided /// is true and that the complement of it is false. /// diff --git a/crates/uv-pep508/src/marker/tree.rs b/crates/uv-pep508/src/marker/tree.rs index 7697751f72d83..185f16244ab6d 100644 --- a/crates/uv-pep508/src/marker/tree.rs +++ b/crates/uv-pep508/src/marker/tree.rs @@ -1108,6 +1108,21 @@ impl MarkerTree { self.simplify_extras_with(|name| extras.contains(name)) } + /// Remove negated extras from a marker, returning `None` if the marker + /// tree evaluates to `true`. + /// + /// Any negated `extra` markers that are always `true` given the provided + /// extras will be removed. Any `extra` markers that are always `false` + /// given the provided extras will be left unchanged. + /// + /// For example, if `dev` is a provided extra, given `sys_platform + /// == 'linux' and extra != 'dev'`, the marker will be simplified to + /// `sys_platform == 'linux'`. + #[must_use] + pub fn simplify_not_extras(self, extras: &[ExtraName]) -> MarkerTree { + self.simplify_not_extras_with(|name| extras.contains(name)) + } + /// Remove the extras from a marker, returning `None` if the marker tree evaluates to `true`. /// /// Any `extra` markers that are always `true` given the provided predicate will be removed. @@ -1127,12 +1142,57 @@ impl MarkerTree { self.simplify_extras_with_impl(&is_extra) } + /// Remove negated extras from a marker, returning `None` if the marker tree evaluates to + /// `true`. + /// + /// Any negated `extra` markers that are always `true` given the provided + /// predicate will be removed. Any `extra` markers that are always `false` + /// given the provided predicate will be left unchanged. + /// + /// For example, if `is_extra('dev')` is true, given + /// `sys_platform == 'linux' and extra != 'dev'`, the marker will be simplified to + /// `sys_platform == 'linux'`. + #[must_use] + pub fn simplify_not_extras_with(self, is_extra: impl Fn(&ExtraName) -> bool) -> MarkerTree { + // Because `simplify_extras_with_impl` is recursive, and we need to use + // our predicate in recursive calls, we need the predicate itself to + // have some indirection (or else we'd have to clone it). To avoid a + // recursive type at codegen time, we just introduce the indirection + // here, but keep the calling API ergonomic. + self.simplify_not_extras_with_impl(&is_extra) + } + + /// Returns a new `MarkerTree` where all `extra` expressions are removed. + /// + /// If the marker only consisted of `extra` expressions, then a marker that + /// is always true is returned. + #[must_use] + pub fn without_extras(self) -> MarkerTree { + MarkerTree(INTERNER.lock().without_extras(self.0)) + } + + /// Returns a new `MarkerTree` where only `extra` expressions are removed. + /// + /// If the marker did not contain any `extra` expressions, then a marker + /// that is always true is returned. + #[must_use] + pub fn only_extras(self) -> MarkerTree { + MarkerTree(INTERNER.lock().only_extras(self.0)) + } + fn simplify_extras_with_impl(self, is_extra: &impl Fn(&ExtraName) -> bool) -> MarkerTree { MarkerTree(INTERNER.lock().restrict(self.0, &|var| match var { Variable::Extra(name) => is_extra(name.extra()).then_some(true), _ => None, })) } + + fn simplify_not_extras_with_impl(self, is_extra: &impl Fn(&ExtraName) -> bool) -> MarkerTree { + MarkerTree(INTERNER.lock().restrict(self.0, &|var| match var { + Variable::Extra(name) => is_extra(name.extra()).then_some(false), + _ => None, + })) + } } impl fmt::Debug for MarkerTree { @@ -2115,6 +2175,59 @@ mod test { assert_eq!(simplified, expected); } + #[test] + fn test_simplify_not_extras() { + // Given `os_name == "nt" and extra != "dev"`, simplify to `os_name == "nt"`. + let markers = MarkerTree::from_str(r#"os_name == "nt" and extra != "dev""#).unwrap(); + let simplified = markers.simplify_not_extras(&[ExtraName::from_str("dev").unwrap()]); + let expected = MarkerTree::from_str(r#"os_name == "nt""#).unwrap(); + assert_eq!(simplified, expected); + + // Given `os_name == "nt" or extra != "dev"`, remove the marker entirely. + let markers = MarkerTree::from_str(r#"os_name == "nt" or extra != "dev""#).unwrap(); + let simplified = markers.simplify_not_extras(&[ExtraName::from_str("dev").unwrap()]); + assert_eq!(simplified, MarkerTree::TRUE); + + // Given `extra != "dev"`, remove the marker entirely. + let markers = MarkerTree::from_str(r#"extra != "dev""#).unwrap(); + let simplified = markers.simplify_not_extras(&[ExtraName::from_str("dev").unwrap()]); + assert_eq!(simplified, MarkerTree::TRUE); + + // Given `extra != "dev" and extra != "test"`, simplify to `extra != "test"`. + let markers = MarkerTree::from_str(r#"extra != "dev" and extra != "test""#).unwrap(); + let simplified = markers.simplify_not_extras(&[ExtraName::from_str("dev").unwrap()]); + let expected = MarkerTree::from_str(r#"extra != "test""#).unwrap(); + assert_eq!(simplified, expected); + + // Given `os_name == "nt" and extra != "test"`, don't simplify. + let markers = MarkerTree::from_str(r#"os_name != "nt" and extra != "test""#).unwrap(); + let simplified = markers + .clone() + .simplify_not_extras(&[ExtraName::from_str("dev").unwrap()]); + assert_eq!(simplified, markers); + + // Given `os_name == "nt" and (python_version == "3.7" or extra != "dev")`, simplify to + // `os_name == "nt". + let markers = MarkerTree::from_str( + r#"os_name == "nt" and (python_version == "3.7" or extra != "dev")"#, + ) + .unwrap(); + let simplified = markers.simplify_not_extras(&[ExtraName::from_str("dev").unwrap()]); + let expected = MarkerTree::from_str(r#"os_name == "nt""#).unwrap(); + assert_eq!(simplified, expected); + + // Given `os_name == "nt" or (python_version == "3.7" and extra != "dev")`, simplify to + // `os_name == "nt" or python_version == "3.7"`. + let markers = MarkerTree::from_str( + r#"os_name == "nt" or (python_version == "3.7" and extra != "dev")"#, + ) + .unwrap(); + let simplified = markers.simplify_not_extras(&[ExtraName::from_str("dev").unwrap()]); + let expected = + MarkerTree::from_str(r#"os_name == "nt" or python_version == "3.7""#).unwrap(); + assert_eq!(simplified, expected); + } + #[test] fn test_marker_simplification() { assert_false("python_version == '3.9.1'"); @@ -3055,4 +3168,64 @@ mod test { m("python_full_version >= '3.9'"), ); } + + #[test] + fn without_extras() { + assert_eq!( + m("os_name == 'Linux'").without_extras(), + m("os_name == 'Linux'"), + ); + assert!(m("extra == 'foo'").without_extras().is_true()); + assert_eq!( + m("os_name == 'Linux' and extra == 'foo'").without_extras(), + m("os_name == 'Linux'"), + ); + + assert!(m(" + (os_name == 'Linux' and extra == 'foo') + or (os_name != 'Linux' and extra == 'bar')",) + .without_extras() + .is_true()); + + assert_eq!( + m("os_name == 'Linux' and extra != 'foo'").without_extras(), + m("os_name == 'Linux'"), + ); + + assert!( + m("extra != 'extra-project-bar' and extra == 'extra-project-foo'") + .without_extras() + .is_true() + ); + } + + #[test] + fn only_extras() { + assert!(m("os_name == 'Linux'").only_extras().is_true()); + assert_eq!(m("extra == 'foo'").only_extras(), m("extra == 'foo'")); + assert_eq!( + m("os_name == 'Linux' and extra == 'foo'").only_extras(), + m("extra == 'foo'"), + ); + assert!(m(" + (os_name == 'foo' and extra == 'foo') + or (os_name == 'bar' and extra != 'foo')",) + .only_extras() + .is_true()); + assert_eq!( + m(" + (os_name == 'Linux' and extra == 'foo') + or (os_name != 'Linux' and extra == 'bar')") + .only_extras(), + m("extra == 'foo' or extra == 'bar'"), + ); + + assert_eq!( + m(" + (implementation_name != 'pypy' and os_name == 'nt' and sys_platform == 'darwin') + or (os_name == 'nt' and sys_platform == 'win32')") + .only_extras(), + m("os_name == 'Linux' or os_name != 'Linux'"), + ); + } } diff --git a/crates/uv-resolver/src/candidate_selector.rs b/crates/uv-resolver/src/candidate_selector.rs index ed6c0690602bb..aae2e4e2b0034 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 8797d7023914d..48403cdd8bac0 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 2c43f99c6be24..c2e964e611087 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 52b180c7e3298..c85f00fe72eca 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 52b180c7e3298..c85f00fe72eca 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 52b180c7e3298..c85f00fe72eca 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 46d6b4c2d94a0..b665ff870b107 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 6a956ce9d091d..5b5fd24dc6ec6 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 f3f275a829a84..78a1ddee01f6d 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 76b8dd36f122a..89589d44ea627 100644 --- a/crates/uv/src/commands/project/lock.rs +++ b/crates/uv/src/commands/project/lock.rs @@ -598,7 +598,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 47c23b10de896..b674e863f9f46 100644 --- a/crates/uv/tests/it/lock.rs +++ b/crates/uv/tests/it/lock.rs @@ -4067,7 +4067,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]