Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[red-knot] improve type shrinking coverage in red-knot property tests #15297

Merged
merged 1 commit into from
Jan 7, 2025
+54 −7
Merged

[red-knot] improve type shrinking coverage in red-knot property tests #15297

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 54 additions & 7 deletions crates/red_knot_python_semantic/src/types/property_tests.rs
View file
Open in desktop
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might not be completely unreasonable to try and return Ty::Never here?

This also makes me think: I think we currently only try shrinking for the root of the Type tree? Like if we have tuple[A | B, C], I think we would only try to remove either A | B from the tuple, or try to remove C from the tuple, i.e. we try to shrink to C or to A | B, but we would not try to shrink the A | B union.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling shrink on the vector of types actually does attempt to shrink the elements of the vector as well as trying to remove elements, so we get nice shrinking just leaning on the shrinking built into quickcheck (at least that was my read of the source)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the empty union shrinking to Ty::Never... feels fine by me! Especially now that I fully grok that Ty is a smart constructor for Type so I don't need to worried about types being in the "wrong location", so to speak

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling shrink on the vector of types actually does attempt to shrink the elements of the vector as well as trying to remove elements

👍

Regarding the empty union shrinking to Ty::Never... feels fine by me! Especially now that I fully grok that Ty is a smart constructor for Type so I don't need to worried about types being in the "wrong location", so to speak

If we try to shrink at all "depths", I think we can skip this. The idea was to simplify nested Ty unions like A | B | (C | D) to A | B | Never and then further down to A | B, but there are many other possible shrinking-paths that would lead to that result (e.g. via the single-element union or via a element-removal shrink on the outer union).

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
Loading