Skip to content

Commit

Permalink
[red-knot] improve type shrinking coverage in red-knot property tests (
Browse files Browse the repository at this point in the history
…#15297)

## Summary

While looking at #14899, I looked at seeing if I could get shrinking on
the examples. It turned out to be straightforward, with a couple of
caveats.

I'm calling `clone` a lot during shrinking. Since by the shrink step
we're already looking at a test failure this feels fine? Unless I
misunderstood `quickcheck`'s core loop

When shrinking `Intersection`s, in order to just rely on `quickcheck`'s
`Vec` shrinking without thinking about it too much, the shrinking
strategy is:
- try to shrink the negative side (keeping the positive side the same)
- try to shrink the positive side (keeping the negative side the same)

This means that you can't shrink from `(A & B & ~C & ~D)` directly to
`(A & ~C)`! You would first need an intermediate failure at `(A & B &
~C)` or `(A & ~C & ~D)`. This feels good enough. Shrinking the negative
side first also has the benefit of trying to strip down negative
elements in these intersections.

## Test Plan
`cargo test -p red_knot_python_semantic -- --ignored
types::property_tests::stable` still fails as it current does on `main`,
but now the errors seem more minimal.
  • Loading branch information
rtpg authored Jan 7, 2025
1 parent 1e948f7 commit 066239f
Showing 1 changed file with 54 additions and 7 deletions.
61 changes: 54 additions & 7 deletions crates/red_knot_python_semantic/src/types/property_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,61 @@ impl Arbitrary for Ty {
}

fn shrink(&self) -> Box<dyn Iterator<Item = Self>> {
// This is incredibly naive. We can do much better here by
// trying various subsets of the elements in unions, tuples,
// and intersections. For now, we only try to shrink by
// reducing unions/tuples/intersections to a single element.
match self.clone() {
Ty::Union(types) => Box::new(types.into_iter()),
Ty::Tuple(types) => Box::new(types.into_iter()),
Ty::Intersection { pos, neg } => Box::new(pos.into_iter().chain(neg)),
Ty::Union(types) => Box::new(types.shrink().filter_map(|elts| match elts.len() {
0 => None,
1 => Some(elts.into_iter().next().unwrap()),
_ => Some(Ty::Union(elts)),
})),
Ty::Tuple(types) => Box::new(types.shrink().filter_map(|elts| match elts.len() {
0 => None,
1 => Some(elts.into_iter().next().unwrap()),
_ => Some(Ty::Tuple(elts)),
})),
Ty::Intersection { pos, neg } => {
// Shrinking on intersections is not exhaustive!
//
// We try to shrink the positive side or the negative side,
// but we aren't shrinking both at the same time.
//
// This should remove positive or negative constraints but
// won't shrink (A & B & ~C & ~D) to (A & ~C) in one shrink
// iteration.
//
// Instead, it hopes that (A & B & ~C) or (A & ~C & ~D) fails
// so that shrinking can happen there.
let pos_orig = pos.clone();
let neg_orig = neg.clone();
Box::new(
// we shrink negative constraints first, as
// intersections with only negative constraints are
// more confusing
neg.shrink()
.map(move |shrunk_neg| Ty::Intersection {
pos: pos_orig.clone(),
neg: shrunk_neg,
})
.chain(pos.shrink().map(move |shrunk_pos| Ty::Intersection {
pos: shrunk_pos,
neg: neg_orig.clone(),
}))
.filter_map(|ty| {
if let Ty::Intersection { pos, neg } = &ty {
match (pos.len(), neg.len()) {
// an empty intersection does not mean
// anything
(0, 0) => None,
// a single positive element should be
// unwrapped
(1, 0) => Some(pos[0].clone()),
_ => Some(ty),
}
} else {
unreachable!()
}
}),
)
}
_ => Box::new(std::iter::empty()),
}
}
Expand Down

0 comments on commit 066239f

Please sign in to comment.