Filter and flatten markers

[ibraheemdev]

Summary

More marker simplification:

• Filters out redundant subtrees based on outer expressions, e.g. a and (a or b) simplifies to a.
• Flattens nested trees internally, e.g. (a and b) and c

Resolves #4536.

[charliermarsh]

Are there redundant expressions that we'd only notice after normalizing (e.g., flattening)? In other words, is filter_all(normalize_all(...)) idempotent? Or is there some risk that we don't normalize everything by way of this ordering?

[ibraheemdev]

filter_all was written to handle non-flattened trees and can also leave the tree in a denormalized (not flattened) state.

Thinking about it more though, I suppose it is possible that there are duplicates that only show up after combining version ranges, so maybe we have to normalize before and after? I was trying to avoid this by writing filter_all to handle non-flattened ranges, but maybe normalizing before is unavoidable. I can try to create a test case where this happens.

[ibraheemdev]

Fixed and added a test.

[charliermarsh]

Relatedly, after this change, if we have a marker that contains an OR with MarkerTree::And(vec![]), do we map that to None (or even MarkerTree::And(vec![]))? It's relevant for a separate PR I'm working on.

[konstin]

Scroll-by Nit: // -> /// so it's rustdoc

[BurntSushi]

I might add a little more rustdoc for cases like these. As it is, I think the comment is effectively strictly redundant with the name and type signature. I think I'd write something like this:

/// For a given slice of marker trees, this collects
/// all direct leaf expressions from each marker tree,
/// and skips any marker trees that are themselves
/// not leaves.

I might also add some context as to why this routine is useful.

[ibraheemdev]

Added some more documentation.

[ibraheemdev]

@charliermarsh it does not, does that ever come up?

[charliermarsh]

@ibraheemdev -- I was considering making some changes elsewhere that would benefit from it, but it's not critical. Although I do think OR MarkerTree::And(vec![]) should be simplified to MarkerTree::And(vec![]).

What's left to do here?

[charliermarsh]

Another one we could do: #4852

[BurntSushi]

I think this overall LGTM. This does seem like playing whack-a-mole, but I think we figured out it would require significant investment to do this fully correctly right?

[BurntSushi]

I might add a little more rustdoc for cases like these. As it is, I think the comment is effectively strictly redundant with the name and type signature. I think I'd write something like this:

/// For a given slice of marker trees, this collects
/// all direct leaf expressions from each marker tree,
/// and skips any marker trees that are themselves
/// not leaves.

I might also add some context as to why this routine is useful.

[BurntSushi]

Is there a reason why this doesn't also filter them out for a direct leaf expression? I guess it would need to rewrite the marker expression as MarkerTree::And(vec![]) in that case...

Another perspective that I find helpful for routines like this is phrasing like, "returns a marker tree in which the given expression is assumed to be true in all conjunctions." I think that captures better the essence of the contract here if I'm understanding correctly.

[ibraheemdev]

Yeah, that's covered by the dedup calls in normalize_all. It's a bit trickier to do that here because we want to filter recursively, but only deduplicate from the outer conjunction or disjunction, depending on the context (we don't just want to remove the expression everywhere).

[BurntSushi]

Oh Clippy. (I agree with your stylistic choice here.)