diff --git a/crates/uv-resolver/src/lock.rs b/crates/uv-resolver/src/lock.rs index 034b876115d6..1039b4b5e05b 100644 --- a/crates/uv-resolver/src/lock.rs +++ b/crates/uv-resolver/src/lock.rs @@ -1626,12 +1626,7 @@ impl Dependency { ) -> Dependency { let distribution_id = DistributionId::from_annotated_dist(annotated_dist); let extra = annotated_dist.extra.clone(); - let mut marker = marker.cloned(); - // Markers can be combined in an unpredictable order, so normalize them - // such that the lock file output is consistent and deterministic. - if let Some(ref mut marker) = marker { - crate::marker::normalize(marker); - } + let marker = marker.cloned().and_then(crate::marker::normalize); Dependency { distribution_id, extra, diff --git a/crates/uv-resolver/src/marker.rs b/crates/uv-resolver/src/marker.rs index 76ad21882a44..01209c00dd4a 100644 --- a/crates/uv-resolver/src/marker.rs +++ b/crates/uv-resolver/src/marker.rs @@ -1,7 +1,6 @@ #![allow(clippy::enum_glob_use)] use std::collections::HashMap; -use std::mem; use std::ops::Bound::{self, *}; use std::ops::RangeBounds; @@ -16,7 +15,6 @@ use pubgrub::range::Range as PubGrubRange; /// Returns `true` if there is no environment in which both marker trees can both apply, i.e. /// the expression `first and second` is always false. -#[allow(dead_code)] pub(crate) fn is_disjoint(first: &MarkerTree, second: &MarkerTree) -> bool { let (expr1, expr2) = match (first, second) { (MarkerTree::Expression(expr1), MarkerTree::Expression(expr2)) => (expr1, expr2), @@ -94,62 +92,84 @@ fn string_is_disjoint(this: &MarkerExpression, other: &MarkerExpression) -> bool /// /// This is useful in cases where creating conjunctions or disjunctions might occur in a non-deterministic /// order. This routine will attempt to erase the distinction created by such a construction. -pub(crate) fn normalize(tree: &mut MarkerTree) { +pub(crate) fn normalize(tree: MarkerTree) -> Option { match tree { - MarkerTree::And(trees) | MarkerTree::Or(trees) => { + MarkerTree::And(trees) => { let mut reduced = Vec::new(); let mut versions: HashMap<_, Vec<_>> = HashMap::new(); - for mut tree in mem::take(trees) { + for subtree in trees { // Simplify nested expressions as much as possible first. - normalize(&mut tree); + // + // If the expression gets normalized out (e.g., `version < '3.8' and version >= '3.8'`), omit it. + let Some(subtree) = normalize(subtree) else { + continue; + }; // Extract expressions we may be able to simplify more. - if let MarkerTree::Expression(ref expr) = tree { + if let MarkerTree::Expression(ref expr) = subtree { if let Some((key, range)) = keyed_range(expr) { versions.entry(key.clone()).or_default().push(range); continue; } } + reduced.push(subtree); + } + + simplify_ranges(&mut reduced, versions, |ranges| { + ranges + .iter() + .fold(PubGrubRange::full(), |acc, range| acc.intersection(range)) + }); - reduced.push(tree); + reduced.sort(); + reduced.dedup(); + + match reduced.len() { + 0 => None, + 1 => Some(reduced.remove(0)), + _ => Some(MarkerTree::And(reduced)), } + } - match tree { - MarkerTree::And(_) => { - simplify_ranges(&mut reduced, versions, |ranges| { - ranges - .iter() - .fold(PubGrubRange::full(), |acc, range| acc.intersection(range)) - }); - - reduced.sort(); - reduced.dedup(); - - *tree = match reduced.len() { - 1 => reduced.remove(0), - _ => MarkerTree::And(reduced), - }; - } - MarkerTree::Or(_) => { - simplify_ranges(&mut reduced, versions, |ranges| { - ranges - .iter() - .fold(PubGrubRange::empty(), |acc, range| acc.union(range)) - }); - - reduced.sort(); - reduced.dedup(); - - *tree = match reduced.len() { - 1 => reduced.remove(0), - _ => MarkerTree::Or(reduced), - }; + MarkerTree::Or(trees) => { + let mut reduced = Vec::new(); + let mut versions: HashMap<_, Vec<_>> = HashMap::new(); + + for subtree in trees { + // Simplify nested expressions as much as possible first. + // + // If the expression gets normalized out (e.g., `version < '3.8' and version >= '3.8'`), return `true`. + let subtree = normalize(subtree)?; + + // Extract expressions we may be able to simplify more. + if let MarkerTree::Expression(ref expr) = subtree { + if let Some((key, range)) = keyed_range(expr) { + versions.entry(key.clone()).or_default().push(range); + continue; + } } - MarkerTree::Expression(_) => unreachable!(), + + reduced.push(subtree); + } + + simplify_ranges(&mut reduced, versions, |ranges| { + ranges + .iter() + .fold(PubGrubRange::empty(), |acc, range| acc.union(range)) + }); + + reduced.sort(); + reduced.dedup(); + + match reduced.len() { + 0 => None, + 1 => Some(reduced.remove(0)), + _ => Some(MarkerTree::Or(reduced)), } } - MarkerTree::Expression(_) => {} + + MarkerTree::Expression(_) => Some(tree), } } @@ -283,18 +303,14 @@ fn keyed_range(expr: &MarkerExpression) -> Option<(&MarkerValueVersion, PubGrubR // if the expression was inverted, we have to reverse the operator before constructing // a version specifier let operator = reverse_operator(*operator); - let Ok(specifier) = VersionSpecifier::from_version(operator, version.clone()) else { - return None; - }; + let specifier = VersionSpecifier::from_version(operator, version.clone()).ok()?; (key, specifier) } _ => return None, }; - let Ok(pubgrub_specifier) = PubGrubSpecifier::try_from(&specifier) else { - return None; - }; + let pubgrub_specifier = PubGrubSpecifier::try_from(&specifier).ok()?; Some((key, pubgrub_specifier.into())) } @@ -404,6 +420,17 @@ mod tests { "extra == 'a' and (extra == 'a' or extra == 'b')", "extra == 'a' and (extra == 'a' or extra == 'b')", ); + + assert_normalizes_out("python_version < '3.12.0rc1' or python_version >= '3.12.0rc1'"); + + assert_normalizes_out( + "extra == 'a' or (python_version < '3.12.0rc1' or python_version >= '3.12.0rc1')", + ); + + assert_normalizes_to( + "extra == 'a' and (python_version < '3.12.0rc1' or python_version >= '3.12.0rc1')", + "extra == 'a'", + ); } #[test] @@ -557,9 +584,24 @@ mod tests { } fn assert_marker_equal(one: impl AsRef, two: impl AsRef) { - let mut tree1 = MarkerTree::parse_reporter(one.as_ref(), &mut TracingReporter).unwrap(); - super::normalize(&mut tree1); + let tree1 = MarkerTree::parse_reporter(one.as_ref(), &mut TracingReporter).unwrap(); + let tree1 = normalize(tree1).unwrap(); let tree2 = MarkerTree::parse_reporter(two.as_ref(), &mut TracingReporter).unwrap(); assert_eq!(tree1.to_string(), tree2.to_string()); } + + fn assert_normalizes_to(before: impl AsRef, after: impl AsRef) { + let normalized = MarkerTree::parse_reporter(before.as_ref(), &mut TracingReporter) + .unwrap() + .clone(); + let normalized = normalize(normalized).unwrap(); + assert_eq!(normalized.to_string(), after.as_ref()); + } + + fn assert_normalizes_out(before: impl AsRef) { + let normalized = MarkerTree::parse_reporter(before.as_ref(), &mut TracingReporter) + .unwrap() + .clone(); + assert!(normalize(normalized).is_none()); + } }