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.

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] is_assignable_to is not reflexive #14899

Open
sharkdp opened this issue Dec 10, 2024 · 1 comment
Open

[red-knot] is_assignable_to is not reflexive #14899

sharkdp opened this issue Dec 10, 2024 · 1 comment
Labels
help wanted Contributions especially welcome red-knot Multi-file analysis & type inference

Comments

@sharkdp
Copy link
Contributor

sharkdp commented Dec 10, 2024

The is_assignable_to-is-reflexive property test, which makes sure that we can assign any type to itself, is currently failing with counter-examples like Any & T, where T is an arbitrary type (that doesn't let the intersection collapse).

The reason for this is that we can't rely on equivalence for assignability of gradual types anymore ever since #14758. We do have some special casing for gradual types like Any and Unknown, as well as for unions and tuples of these, but we do lack handling of intersections.

This is not completely trivial. We can patch this with some superficial handling like

--- a/crates/red_knot_python_semantic/src/types.rs
+++ b/crates/red_knot_python_semantic/src/types.rs
@@ -748,6 +748,14 @@ impl<'db> Type<'db> {
                 .elements(db)
                 .iter()
                 .any(|&elem_ty| ty.is_assignable_to(db, elem_ty)),
+            (Type::Intersection(intersection), ty) => intersection
+                .positive(db)
+                .iter()
+                .any(|&elem_ty| elem_ty.is_assignable_to(db, ty)),
+            (ty, Type::Intersection(intersection)) => intersection
+                .positive(db)
+                .iter()
+                .any(|&elem_ty| ty.is_assignable_to(db, elem_ty)),
             (Type::Tuple(self_tuple), Type::Tuple(target_tuple)) => {
                 let self_elements = self_tuple.elements(db);
                 let target_elements = target_tuple.elements(db);

which also works for things like T & ~Any, because we always add Any/Unknown as positive contributions to intersections.

But then quickcheck finds types like ~tuple[Any, T], for which the simple handling above breaks.

@sharkdp sharkdp added the red-knot Multi-file analysis & type inference label Dec 10, 2024
@carljm carljm added the help wanted Contributions especially welcome label Dec 14, 2024
@carljm
Copy link
Contributor

carljm commented Dec 14, 2024

Moving this out of Backlog and into Ready, and also marking help-wanted in case a contributor is interested in it, because I think having the quickcheck tests stable and green on main is quite valuable. Just now I was making another change to type relations, ran the quickcheck tests, and hit this failure, which diverted me for a while until I realized it was failing also on main and found this issue.

sharkdp pushed a commit that referenced this issue Dec 14, 2024
## Summary

Teach red-knot that `type[...]` is always disjoint from `None` and from
`LiteralString`. Fixes #14925.

This should properly be generalized to "all instances of final types
which are not subclasses of `type`", but until we support finality,
hardcoding `None` (which is known to be final) allows us to fix the
subtype transitivity property test.

## Test Plan

Existing tests pass, added new unit tests for `is_disjoint_from` and
`is_subtype_of`.

`QUICKCHECK_TESTS=100000 cargo test -p red_knot_python_semantic --
--ignored types::property_tests::stable` fails only the "assignability
is reflexive" test, which is known to fail on `main` (#14899).

The same command, with `property_tests.rs` edited to prevent generating
intersection tests (the cause of #14899), passes all quickcheck tests.
carljm pushed a commit that referenced this issue Dec 14, 2024
…<metaclass of T>)` (#14970)

## Summary

A class is an instance of its metaclass, so `ClassLiteral("ABC")` is not
disjoint from `Instance("ABCMeta")`. However, we erroneously consider
the two types disjoint on the `main` branch. This PR fixes that.

This bug was uncovered by adding some more core types to the property
tests that provide coverage for classes that have custom metaclasses.
The additions to the property tests are included in this PR.

## Test Plan

New unit tests and property tests added. Tested with:
- `cargo test -p red_knot_python_semantic`
- `QUICKCHECK_TESTS=100000 cargo test -p red_knot_python_semantic --
--ignored types::property_tests::stable`

The assignability property test fails on this branch, but that's a known
issue that exists on `main`, due to
#14899.
AlexWaygood added a commit that referenced this issue Jan 3, 2025
## Summary

We understand `sys.version_info` branches now! As such, I _believe_ this
branch is no longer required; all tests pass without it. I also ran
`QUICKCHECK_TESTS=100000 cargo test -p red_knot_python_semantic --
--ignored types::property_tests::stable`, and no tests failed except for
the known issue with `Type::is_assignable_to()`
(#14899)

## Test Plan

See above
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Contributions especially welcome red-knot Multi-file analysis & type inference
Projects
None yet
Development

No branches or pull requests

2 participants