[red-knot] Add narrowing for `issubclass` checks by sharkdp · Pull Request #14128 · astral-sh/ruff

sharkdp · 2024-11-06T10:35:58Z

Summary

Adds basic support for type[C] as a red knot Type. Some things might not be supported yet, like type[Any].
Adds type narrowing for issubclass checks.

closes #14117

Test Plan

New Markdown-based tests

github-actions · 2024-11-06T10:50:07Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

crates/red_knot_python_semantic/src/types/narrow.rs

crates/red_knot_python_semantic/src/types.rs

sharkdp · 2024-11-06T15:04:42Z

crates/red_knot_python_semantic/src/types.rs

            Type::Tuple(_) => KnownClass::Tuple.to_class(db),
            // TODO not accurate if there's a custom metaclass...
            Type::ClassLiteral(_) => KnownClass::Type.to_class(db),
+            Type::Type(_) => Type::Todo,


This is too meta for me. Should this be Type::Type('type')?

Should this be Type::Type('type')?

yes, I think so

For now, sure. But with a TODO comment. Because with Charlie's metaclass PR, it should become something like:

Suggested change

Type::Type(_) => Type::Todo,

Type::Type(class) => Type::Type(class.class.metaclass(db).expect_literal_class())

but with more proper handling of odd cases and less expect

I tried something, but I'm not sure what kind of "odd cases" would need to be handled. Let me know if something should be changed here (post-merge).

crates/red_knot_python_semantic/src/types.rs

AlexWaygood

Very cool!

crates/red_knot_python_semantic/src/types.rs

AlexWaygood · 2024-11-06T15:20:20Z

crates/red_knot_python_semantic/src/types.rs

            Type::Tuple(_) => KnownClass::Tuple.to_class(db),
            // TODO not accurate if there's a custom metaclass...
            Type::ClassLiteral(_) => KnownClass::Type.to_class(db),
+            Type::Type(_) => Type::Todo,


Should this be Type::Type('type')?

yes, I think so

crates/red_knot_python_semantic/src/types.rs

crates/red_knot_python_semantic/src/types/narrow.rs

crates/red_knot_python_semantic/resources/mdtest/narrow/issubclass.md

carljm

(Haven't finished review yet but need to run to lunch and an errand, will complete review when I get back.)

crates/red_knot_python_semantic/src/types.rs

carljm · 2024-11-06T20:50:03Z

crates/red_knot_python_semantic/src/types.rs

            Type::Tuple(_) => KnownClass::Tuple.to_class(db),
            // TODO not accurate if there's a custom metaclass...
            Type::ClassLiteral(_) => KnownClass::Type.to_class(db),
+            Type::Type(_) => Type::Todo,


For now, sure. But with a TODO comment. Because with Charlie's metaclass PR, it should become something like:

Suggested change

Type::Type(_) => Type::Todo,

Type::Type(class) => Type::Type(class.class.metaclass(db).expect_literal_class())

but with more proper handling of odd cases and less expect

carljm

This is very nice!

crates/red_knot_python_semantic/src/types.rs

crates/red_knot_python_semantic/src/types/infer.rs

AlexWaygood

Great work!

crates/red_knot_python_semantic/resources/mdtest/narrow/issubclass.md

crates/red_knot_python_semantic/src/types/infer.rs

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

carljm · 2024-11-07T13:53:08Z

crates/red_knot_python_semantic/src/types.rs

+            ) if self_class
+                .metaclass(db)
+                .into_class_literal()
+                .map(|meta| meta.class.is_subclass_of(db, target_class))
+                .unwrap_or(false) =>
+            {
+                true


Looks good! My comment was vague because I hadn't thought through yet the handling of a non-class-literal metaclass here. But I think the only options here are class literal or dynamic type (Any/Unknown/Todo), and since this is is_subtype_of, ignoring dynamic type is correct.

carljm · 2024-11-07T13:56:01Z

crates/red_knot_python_semantic/src/types.rs

+                class
+                    .try_metaclass(db)
+                    .ok()
+                    .and_then(Type::into_class_literal)
+                    .unwrap_or(KnownClass::Type.to_class(db).expect_class_literal())
+                    .to_subclass_of_type(),


This looks great, too! Falling back to Type::SubclassOf(builtins.type) in the case of any non-specific metaclass is the right answer here, I think.

sharkdp added the ty Multi-file analysis & type inference label Nov 6, 2024

MichaReiser reviewed Nov 6, 2024

View reviewed changes

crates/red_knot_python_semantic/src/types/narrow.rs Outdated Show resolved Hide resolved

sharkdp marked this pull request as ready for review November 6, 2024 14:57

sharkdp requested review from AlexWaygood and carljm as code owners November 6, 2024 14:57

sharkdp commented Nov 6, 2024

View reviewed changes

crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved

sharkdp commented Nov 6, 2024

View reviewed changes

crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved

sharkdp commented Nov 6, 2024

View reviewed changes

crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved

sharkdp commented Nov 6, 2024

View reviewed changes

crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved

AlexWaygood reviewed Nov 6, 2024

View reviewed changes

crates/red_knot_python_semantic/resources/mdtest/narrow/issubclass.md Show resolved Hide resolved

carljm reviewed Nov 6, 2024

View reviewed changes

carljm approved these changes Nov 7, 2024

View reviewed changes

crates/red_knot_python_semantic/src/types.rs Show resolved Hide resolved

crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved

sharkdp added 15 commits November 7, 2024 10:28

[red-knot] Add narrowing for issubclass checks

4de507c

Add Type::Type(…) variant

7296010

Code simplification

9afbe38

More unit tests

4008814

More precise behavior for Type::Type

bd6450c

Adapt to_meta_type

ea11c9e

Remove empty comment

6236ee4

Minor clarifications

929066c

Remove TODO comment

7f1827e

Error handling for to_meta_type

70660f7

Add TODO comment

c0954ce

Fix Type::to_instance

91ccf0c

Rename Type::Type to Type::SubclassOf

1aba6ac

Fixes for is_disjoint_from

51b32e2

Add test for !(Literal[int] <: Literal[object])

0884352

sharkdp force-pushed the david/issubclass-narrowing branch from 7787933 to 0884352 Compare November 7, 2024 10:27

sharkdp added 3 commits November 7, 2024 11:29

Comment clarification

dd1422c

Model constraint functions inside KnownFunction

a31988b

Move name=>KnownFunction to separate method

0967e01

AlexWaygood approved these changes Nov 7, 2024

View reviewed changes

Understand type[…] annotations

400e101

sharkdp force-pushed the david/issubclass-narrowing branch from 7cc2599 to 400e101 Compare November 7, 2024 12:34

AlexWaygood reviewed Nov 7, 2024

View reviewed changes

crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved

AlexWaygood approved these changes Nov 7, 2024

View reviewed changes

crates/red_knot_python_semantic/resources/mdtest/narrow/issubclass.md Show resolved Hide resolved

crates/red_knot_python_semantic/src/types/infer.rs Show resolved Hide resolved

sharkdp and others added 4 commits November 7, 2024 13:45

Add description to test

0791afb

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

Add TODO comment

4e56308

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

Nested match

be1691d

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

Fix Markdown formatting

4e35aec

sharkdp merged commit f2546c5 into main Nov 7, 2024

sharkdp deleted the david/issubclass-narrowing branch November 7, 2024 13:15

carljm reviewed Nov 7, 2024

View reviewed changes

	Type::Type(_) => Type::Todo,
	Type::Type(class) => Type::Type(class.class.metaclass(db).expect_literal_class())

Conversation

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

Uh oh!

Summary

Test Plan

Uh oh!

github-actions bot commented Nov 6, 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!

Uh oh!

Uh oh!

sharkdp Nov 6, 2024

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Nov 6, 2024

Choose a reason for hiding this comment

Uh oh!

carljm Nov 6, 2024

Choose a reason for hiding this comment

Uh oh!

sharkdp Nov 7, 2024 • 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!

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AlexWaygood Nov 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

carljm Nov 6, 2024

Choose a reason for hiding this comment

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

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!

carljm Nov 7, 2024

Choose a reason for hiding this comment

Uh oh!

carljm Nov 7, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

sharkdp commented Nov 6, 2024 •

edited

Loading

github-actions bot commented Nov 6, 2024 •

edited

Loading

`ruff-ecosystem` results

sharkdp Nov 7, 2024 •

edited

Loading