Handle type[Any] correctly by dcreager · Pull Request #14876 · astral-sh/ruff

dcreager · 2024-12-09T15:16:32Z

This adds support for type[Any], which represents an unknown type (not an instance of an unknown type), and type, which we are choosing to interpret as type[object].

Closes #14546

github-actions · 2024-12-09T15:35:53Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

carljm

🚀

* main: [`airflow`] Add fix to remove deprecated keyword arguments (`AIR302`) (#14887) Improve mdtests style (#14884) Reference `suppress-dummy-regex-options` in documentation of rules supporting it (#14888) [`flake8-bugbear`] `itertools.batched()` without explicit `strict` (`B911`) (#14408) [`ruff`] Mark autofix for `RUF052` as always unsafe (#14824) [red-knot] Improve type inference for except handlers (#14838) More typos found by codespell (#14880) [red-knot] move standalone expression_ty to TypeInferenceBuilder::file_expression_ty (#14879) [`ruff`] Do not simplify `round()` calls (`RUF046`) (#14832) Stop referring to early ruff versions (#14862) Fix a typo in `class.rs` (#14877) [`flake8-pyi`] Also remove `self` and `cls`'s annotation (`PYI034`) (#14801) [`pyupgrade`] Remove unreachable code in `UP015` implementation (#14871) [`flake8-bugbear`] Skip `B028` if `warnings.warn` is called with `*args` or `**kwargs` (#14870)

dcreager

(Initial responses, still need to make the type ⇒ type[object] change and add the equivalent_to tests)

AlexWaygood

Nice!

* main: [red-knot] Add infrastructure to declare lints (#14873) [red-knot] Typed diagnostic id (#14869)

dcreager · 2024-12-10T18:07:16Z

-            (Type::Unknown | Type::Any | Type::Todo(_), _) => false,
-            (_, Type::Unknown | Type::Any | Type::Todo(_)) => false,


I removed these since they (and the new Subclass(Any) form) are handled by the is_fully_static checks above.

dcreager · 2024-12-10T18:09:05Z

            | Type::SliceLiteral(_)
            | Type::KnownInstance(_) => true,
-            Type::ClassLiteral(_) | Type::SubclassOf(_) | Type::Instance(_) => {
+            Type::SubclassOf(SubclassOfType { base }) => matches!(base, ClassBase::Class(_)),


SubclassOf(Any) is not fully static

carljm

Looking good! In addition to the additional unit tests I mentioned inline, this PR seems a good candidate to run the quickcheck property-based tests for core type algebra invariants. These don't run in CI because they are slow, but it would be good to see whether they catch any issues here. See https://github.com/astral-sh/ruff/blob/main/crates/red_knot_python_semantic/src/types/property_tests.rs

EDIT: just realized that in order to get value from that on this PR, we should also add SubclassOf types to the arbitrary_core_type function there!

AlexWaygood · 2024-12-10T18:18:27Z

+            (
+                Type::Instance(InstanceType { class: self_class }),
+                Type::Instance(InstanceType {
+                    class: target_class,
+                }),
+            ) if {
+                let self_known = self_class.known(db);
+                matches!(
+                    self_known,
+                    Some(KnownClass::NoneType | KnownClass::NoDefaultType)
+                ) && self_known == target_class.known(db)
+            } =>
+            {
+                true
+            }


this gets a little hard to read 😄 how about writing it like this?

Suggested change

(

Type::Instance(InstanceType { class: self_class }),

Type::Instance(InstanceType {

class: target_class,

}),

) if {

let self_known = self_class.known(db);

matches!(

self_known,

Some(KnownClass::NoneType | KnownClass::NoDefaultType)

) && self_known == target_class.known(db)

} =>

{

true

}

(Type::Instance(self_instance), Type::Instance(target_instance)) => {

let Some(self_known_class) = self_instance.class.known(db) else {

return false;

};

if !matches!(

self_known_class,

KnownClass::NoneType | KnownClass::NoDefaultType

) {

return false;

}

Some(self_known_class) == target_instance.class.known(db)

}

We have to be careful here; these return false in the suggested change are not correct. They would have to be return self == other, and I'm not sure we want to duplicate that condition in various places.

argh, great spot. thanks.

I had also considered handling each of the special cases with a separate if statement + early return. Then it would possibly be clearer that self == other is the fall-back / general case. But we had other large match blocks elsewhere that made me think that might be preferred house style. I will take a stab at the early return pattern and see if it's more readable.

I will take a stab at the early return pattern and see if it's more readable.

Now using early return. lmkwyt

AlexWaygood · 2024-12-10T18:20:01Z

+            ) if object_class.is_known(db, KnownClass::Object)
+                && type_class.is_known(db, KnownClass::Type) =>
+            {
+                true
+            }


Suggested change

) if object_class.is_known(db, KnownClass::Object)

&& type_class.is_known(db, KnownClass::Type) =>

{

true

}

) => {

object_class.is_known(db, KnownClass::Object)

&& type_class.is_known(db, KnownClass::Type)

}

This one should be safe because a Type::Instance can't be == to a Type::SubclassOf.

sharkdp · 2024-12-10T19:34:03Z

In addition to the additional unit tests I mentioned inline, this PR seems a good candidate to run the quickcheck property-based tests for core type algebra invariants. These don't run in CI because they are slow, but it would be good to see whether they catch any issues here. See https://github.com/astral-sh/ruff/blob/main/crates/red_knot_python_semantic/src/types/property_tests.rs

I already did this. I didn't mention it here because they are currently broken (surprise!). I'll put up a small PR with a fix.

Edit: #14897

* main: [red-knot] split call-outcome enums to their own submodule (#14898) [red-knot] Property tests: account non-fully-static types (#14897) ruff: add worktree support to build.rs (#14896)

sharkdp

Thank you for all the updates and the new tests!

FWIW, I did successfully run the property tests on your branch after cherry-picking my fix in #14897 and applying the following patch, where the 4=>3 change is a weird way to suppress the creation of intersection types to prevent running into #14899:

diff --git a/crates/red_knot_python_semantic/src/types/property_tests.rs b/crates/red_knot_python_semantic/src/types/property_tests.rs
index 15f4d5648..e6b223034 100644
--- a/crates/red_knot_python_semantic/src/types/property_tests.rs
+++ b/crates/red_knot_python_semantic/src/types/property_tests.rs
@@ -64,6 +64,10 @@ fn arbitrary_core_type(g: &mut Gen) -> Ty {
         Ty::BuiltinClassLiteral("int"),
         Ty::BuiltinClassLiteral("bool"),
         Ty::BuiltinClassLiteral("object"),
+        Ty::SubclassOfAny,
+        Ty::SubclassOfBuiltinClass("object"),
+        Ty::SubclassOfBuiltinClass("int"),
+        Ty::SubclassOfBuiltinClass("bool"),
     ])
     .unwrap()
     .clone()
@@ -78,7 +82,7 @@ fn arbitrary_type(g: &mut Gen, size: u32) -> Ty {
     if size == 0 {
         arbitrary_core_type(g)
     } else {
-        match u32::arbitrary(g) % 4 {
+        match u32::arbitrary(g) % 3 {
             0 => arbitrary_core_type(g),
             1 => Ty::Union(
                 (0..*g.choose(&[2, 3]).unwrap())

dcreager

I did successfully run the property tests on your branch

Thanks @sharkdp, was just looking at the property tests myself and running into the assignable bug

carljm

Looks good to me!!

AlexWaygood added the ty Multi-file analysis & type inference label Dec 9, 2024

dcreager force-pushed the dcreager/type-of-any branch from ba56f82 to 839f7a4 Compare December 9, 2024 16:57

dcreager added 2 commits December 9, 2024 14:17

Handle type[Any] correctly

cdfbfa7

Treat type as a synonym for type[Any]

4ac0df3

dcreager force-pushed the dcreager/type-of-any branch from 839f7a4 to 4ac0df3 Compare December 9, 2024 19:17

Add test cases for type[object]

b65a156

dcreager marked this pull request as ready for review December 9, 2024 19:45

dcreager requested review from AlexWaygood, MichaReiser, carljm and sharkdp as code owners December 9, 2024 19:45

carljm approved these changes Dec 9, 2024

View reviewed changes

Comment thread crates/red_knot_python_semantic/resources/mdtest/type_of/any.md

Comment thread crates/red_knot_python_semantic/resources/mdtest/type_of/any.md

sharkdp reviewed Dec 9, 2024

View reviewed changes

Comment thread crates/red_knot_python_semantic/resources/mdtest/type_of/any.md Outdated

sharkdp reviewed Dec 9, 2024

View reviewed changes

Comment thread crates/red_knot_python_semantic/src/types/mro.rs Outdated

dcreager added 5 commits December 10, 2024 09:12

Use variable assignments in tests

683f901

Handle assignability correctly

ebb3031

Remove unused function

3095cfd

Add bound method TODOs

3c1043d

carljm reviewed Dec 10, 2024

View reviewed changes

Add failing mdtests

24d1fdc

dcreager commented Dec 10, 2024

View reviewed changes

AlexWaygood reviewed Dec 10, 2024

View reviewed changes

Comment thread crates/red_knot_python_semantic/src/types.rs Outdated

Comment thread crates/red_knot_python_semantic/src/types/mro.rs Outdated

dcreager added 6 commits December 10, 2024 12:33

Inline and remove ClassBase::is_subtype_of

d47b4cc

Reuse existing clause for C ⊆ type

2ec0bcc

Fix is_fully_static for SubclassOf(Any)

e651e7d

Remove superfluous clause

42a2233

Rename SubclassOfClass

5fd4217

Subclass[object] ⇔ Instance[type]

9213222

dcreager added 3 commits December 10, 2024 12:50

Interpret type as "instance of type", aka type[object]

1f944c2

Verify that equivalence is symmetric

a6a1a3d

Merge branch 'main' into dcreager/type-of-any

113af14

* main: [red-knot] Add infrastructure to declare lints (#14873) [red-knot] Typed diagnostic id (#14869)

dcreager commented Dec 10, 2024

View reviewed changes

carljm reviewed Dec 10, 2024

View reviewed changes

Comment thread crates/red_knot_python_semantic/src/types.rs

AlexWaygood reviewed Dec 10, 2024

View reviewed changes

dcreager added 4 commits December 10, 2024 15:08

Renames per Alex

62da549

Add tests for is_fully_static

20bbaaf

Add property test constructors for new test types

72f09c2

Merge branch 'main' into dcreager/type-of-any

ae96c9f

* main: [red-knot] split call-outcome enums to their own submodule (#14898) [red-knot] Property tests: account non-fully-static types (#14897) ruff: add worktree support to build.rs (#14896)

sharkdp approved these changes Dec 10, 2024

View reviewed changes

dcreager commented Dec 10, 2024

View reviewed changes

dcreager added 2 commits December 10, 2024 15:28

Use early return in is_equivalent_to

4eddff6

Add comment

1148f96

carljm approved these changes Dec 10, 2024

View reviewed changes

dcreager mentioned this pull request Dec 10, 2024

[red-knot] support type[Any] #14546

Closed

dcreager merged commit d4126f6 into main Dec 10, 2024

dcreager deleted the dcreager/type-of-any branch December 10, 2024 21:12

		(Type::Unknown \| Type::Any \| Type::Todo(_), _) => false,
		(_, Type::Unknown \| Type::Any \| Type::Todo(_)) => false,

Conversation

dcreager commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dcreager left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carljm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sharkdp commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharkdp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcreager left a comment

Choose a reason for hiding this comment

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

dcreager commented Dec 9, 2024 •

edited

Loading

github-actions Bot commented Dec 9, 2024 •

edited

Loading

`ruff-ecosystem` results

carljm left a comment •

edited

Loading

sharkdp commented Dec 10, 2024 •

edited

Loading

sharkdp left a comment •

edited

Loading