From 8bb0a55ff52bb67c74e8a21a9d647779f313fd19 Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Fri, 6 Sep 2024 13:13:54 -0400 Subject: [PATCH] pep508: remove hidden state from `MarkerTree` I split this change into its own commit because I'm hoping it crystalizes what it means when we say "a `MarkerTree` has hidden state." That is, it isn't so much that there is some explicit member of a `MarkerTree` that is omitted, but rather, the lower and upper version bounds on `python_full_version` are are rewritten as "unbounded" when traversing the ADD for display. We will actually retain this functionality, but rejigger it so that it's explicit when we do this. In particular, this simplification has been problematic for us because it fundamentally changes the truth tables of a marker expression *unless* you are extremely careful to interpret it only under the original context in which it was simplified. This is quite difficult to do generally, and in prior work in #6268, we completed a refactor where we worked around this type of simplification and moved it to the edges of uv. In subsequent commits, we'll re-implement this form of simplification as a more explicit step. --- crates/pep508-rs/src/marker/simplify.rs | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/crates/pep508-rs/src/marker/simplify.rs b/crates/pep508-rs/src/marker/simplify.rs index 737371c5c..15219d1d6 100644 --- a/crates/pep508-rs/src/marker/simplify.rs +++ b/crates/pep508-rs/src/marker/simplify.rs @@ -285,26 +285,10 @@ pub(crate) fn collect_edges<'a, T>( where T: Ord + Clone + 'a, { - let len = map.len(); - let mut paths: IndexMap<_, Range<_>, FxBuildHasher> = IndexMap::default(); - for (i, (range, tree)) in map.enumerate() { - let (mut start, mut end) = range.bounding_range().unwrap(); - match (start, end) { - (Bound::Included(v1), Bound::Included(v2)) if v1 == v2 => {} - _ => { - // Expand the range of this variable to be unbounded. - // This helps remove redundant expressions such as `python_version >= '3.7'` - // when the range has already been restricted to `['3.7', inf)` - if i == 0 { - start = Bound::Unbounded; - } - if i == len - 1 { - end = Bound::Unbounded; - } - } - } - + for (range, tree) in map { + // OK because all ranges are guaranteed to be non-empty. + let (start, end) = range.bounding_range().unwrap(); // Combine the ranges. let range = Range::from_range_bounds((start.cloned(), end.cloned())); paths