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] type[] is disjoint from None, LiteralString #14967

Merged
merged 4 commits into from
Dec 14, 2024
Merged

Conversation

carljm
Copy link
Contributor

@carljm carljm commented 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 carljm added the red-knot Multi-file analysis & type inference label Dec 14, 2024
Copy link
Contributor

github-actions bot commented Dec 14, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link
Contributor

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thank you!

@sharkdp sharkdp merged commit ac31b26 into main Dec 14, 2024
21 checks passed
@sharkdp sharkdp deleted the cjm/disjoint branch December 14, 2024 10:02
Comment on lines +1054 to 1062
(Type::SubclassOf(_), Type::Instance(instance))
| (Type::Instance(instance), Type::SubclassOf(_)) => {
// TODO this should be `true` if the instance is of a final type which is not a
// subclass of type. (With non-final types, we never know whether a subclass might
// multiply-inherit `type` or a subclass of it, and thus not be disjoint with
// `type[...]`.) Until we support finality, hardcode None, which is known to be
// final.
matches!(instance.class.known(db), Some(KnownClass::NoneType))
}
Copy link
Member

@AlexWaygood AlexWaygood Dec 14, 2024

Choose a reason for hiding this comment

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

We could generalise this branch a little bit to cover all singleton classes that are known not to be subtypes of type, rather than singling out None for special treatment:

--- a/crates/red_knot_python_semantic/src/types.rs
+++ b/crates/red_knot_python_semantic/src/types.rs
@@ -1051,14 +1051,15 @@ impl<'db> Type<'db> {
 
             (Type::SubclassOf(_), Type::SubclassOf(_)) => false,
 
-            (Type::SubclassOf(_), Type::Instance(instance))
-            | (Type::Instance(instance), Type::SubclassOf(_)) => {
+            (Type::SubclassOf(_), instance @ Type::Instance(InstanceType { class }))
+            | (instance @ Type::Instance(InstanceType { class }), Type::SubclassOf(_)) => {
                 // TODO this should be `true` if the instance is of a final type which is not a
                 // subclass of type. (With non-final types, we never know whether a subclass might
                 // multiply-inherit `type` or a subclass of it, and thus not be disjoint with
-                // `type[...]`.) Until we support finality, hardcode None, which is known to be
-                // final.
-                matches!(instance.class.known(db), Some(KnownClass::NoneType))
+                // `type[...]`.) Until we support finality, special-case various classes
+                // known to be singletons
+                class.known(db).is_some_and(KnownClass::is_singleton)
+                    && !instance.is_subtype_of(db, KnownClass::Type.to_instance(db))
             }
 
             (

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

Successfully merging this pull request may close these issues.

[red-knot] is_subtype_of-transitivity violation
3 participants