[ty] Fix broken property tests for disjointness of intersections (#20775)
Some checks are pending
CI / Determine changes (push) Waiting to run
CI / cargo fmt (push) Waiting to run
CI / cargo clippy (push) Blocked by required conditions
CI / cargo test (linux) (push) Blocked by required conditions
CI / cargo test (linux, release) (push) Blocked by required conditions
CI / cargo test (windows) (push) Blocked by required conditions
CI / cargo test (wasm) (push) Blocked by required conditions
CI / cargo build (release) (push) Waiting to run
CI / cargo build (msrv) (push) Blocked by required conditions
CI / cargo fuzz build (push) Blocked by required conditions
CI / fuzz parser (push) Blocked by required conditions
CI / test scripts (push) Blocked by required conditions
CI / ecosystem (push) Blocked by required conditions
CI / Fuzz for new ty panics (push) Blocked by required conditions
CI / cargo shear (push) Blocked by required conditions
CI / ty completion evaluation (push) Blocked by required conditions
CI / python package (push) Waiting to run
CI / pre-commit (push) Waiting to run
CI / mkdocs (push) Waiting to run
CI / formatter instabilities and black similarity (push) Blocked by required conditions
CI / test ruff-lsp (push) Blocked by required conditions
CI / check playground (push) Blocked by required conditions
CI / benchmarks instrumented (ruff) (push) Blocked by required conditions
CI / benchmarks instrumented (ty) (push) Blocked by required conditions
CI / benchmarks-walltime (push) Blocked by required conditions
[ty Playground] Release / publish (push) Waiting to run

## Summary

Two stable property tests are currently failing on `main`, following
f054b8a55e
(of course, I only thought to run the property tests again around 30
minutes _after_ landing that PR...). The issue is quite subtle, and took
me an annoying amount of time to pin down: we're matching over `(self,
other)` in `Type::is_disjoint_from_impl`, but `other` here is shadowed
by the binding in the `match` branch, which means that the wrong key is
inserted into the cache of the `IsDisjointFrom` cycle detector:


f054b8a55e/crates/ty_python_semantic/src/types.rs (L2408-L2435)

This PR fixes that issue, and also adds a few `Debug` implementations to
our cycle detectors, so that issues like this are easier to debug in the
future.

I'm adding the `internal` label, as this fixes a bug that hasn't yet
appeared in any released version of ty, so it doesn't deserve its own
changelog entry.

## Test Plan

`QUICKCHECK_TESTS=1000000 cargo test --release -p ty_python_semantic --
--ignored types::property_tests::stable` now once again passes on `main`

I considered adding new mdtests as well, but the examples that the
property tests were throwing at me all seemed _quite_ obscure and
somewhat unlikely to occur in the real world. I don't think it's worth
it.
This commit is contained in:
Alex Waygood 2025-10-08 22:28:56 +01:00 committed by GitHub
parent f054b8a55e
commit b0c6217e0b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194

View file

@ -182,6 +182,7 @@ pub(crate) type ApplyTypeMappingVisitor<'db> = TypeTransformer<'db, TypeMapping<
/// A [`PairVisitor`] that is used in `has_relation_to` methods.
pub(crate) type HasRelationToVisitor<'db> =
CycleDetector<TypeRelation, (Type<'db>, Type<'db>, TypeRelation), ConstraintSet<'db>>;
impl Default for HasRelationToVisitor<'_> {
fn default() -> Self {
HasRelationToVisitor::new(ConstraintSet::from(true))
@ -190,7 +191,10 @@ impl Default for HasRelationToVisitor<'_> {
/// A [`PairVisitor`] that is used in `is_disjoint_from` methods.
pub(crate) type IsDisjointVisitor<'db> = PairVisitor<'db, IsDisjoint, ConstraintSet<'db>>;
#[derive(Debug)]
pub(crate) struct IsDisjoint;
impl Default for IsDisjointVisitor<'_> {
fn default() -> Self {
IsDisjointVisitor::new(ConstraintSet::from(false))
@ -199,7 +203,10 @@ impl Default for IsDisjointVisitor<'_> {
/// A [`PairVisitor`] that is used in `is_equivalent` methods.
pub(crate) type IsEquivalentVisitor<'db> = PairVisitor<'db, IsEquivalent, ConstraintSet<'db>>;
#[derive(Debug)]
pub(crate) struct IsEquivalent;
impl Default for IsEquivalentVisitor<'_> {
fn default() -> Self {
IsEquivalentVisitor::new(ConstraintSet::from(true))
@ -208,6 +215,8 @@ impl Default for IsEquivalentVisitor<'_> {
/// A [`CycleDetector`] that is used in `find_legacy_typevars` methods.
pub(crate) type FindLegacyTypeVarsVisitor<'db> = CycleDetector<FindLegacyTypeVars, Type<'db>, ()>;
#[derive(Debug)]
pub(crate) struct FindLegacyTypeVars;
/// A [`CycleDetector`] that is used in `try_bool` methods.
@ -217,6 +226,8 @@ pub(crate) struct TryBool;
/// A [`TypeTransformer`] that is used in `normalized` methods.
pub(crate) type NormalizedVisitor<'db> = TypeTransformer<'db, Normalized>;
#[derive(Debug)]
pub(crate) struct Normalized;
/// How a generic type has been specialized.
@ -2301,7 +2312,7 @@ impl<'db> Type<'db> {
(Type::TypeAlias(alias), _) => {
let self_alias_ty = alias.value_type(db);
disjointness_visitor.visit((self_alias_ty, other), || {
disjointness_visitor.visit((self, other), || {
self_alias_ty.is_disjoint_from_impl(
db,
other,
@ -2313,7 +2324,7 @@ impl<'db> Type<'db> {
(_, Type::TypeAlias(alias)) => {
let other_alias_ty = alias.value_type(db);
disjointness_visitor.visit((self, other_alias_ty), || {
disjointness_visitor.visit((self, other), || {
self.is_disjoint_from_impl(
db,
other_alias_ty,
@ -2405,8 +2416,8 @@ impl<'db> Type<'db> {
})
}
(Type::Intersection(intersection), other)
| (other, Type::Intersection(intersection)) => {
(Type::Intersection(intersection), non_intersection)
| (non_intersection, Type::Intersection(intersection)) => {
disjointness_visitor.visit((self, other), || {
intersection
.positive(db)
@ -2414,7 +2425,7 @@ impl<'db> Type<'db> {
.when_any(db, |p| {
p.is_disjoint_from_impl(
db,
other,
non_intersection,
disjointness_visitor,
relation_visitor,
)
@ -2422,7 +2433,7 @@ impl<'db> Type<'db> {
// A & B & Not[C] is disjoint from C
.or(db, || {
intersection.negative(db).iter().when_any(db, |&neg_ty| {
other.has_relation_to_impl(
non_intersection.has_relation_to_impl(
db,
neg_ty,
TypeRelation::Subtyping,