-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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] Add typing.Any
as a spelling for the Any type
#14742
Conversation
We already had a representation for the Any type, which we would use e.g. for expressions without type annotations. We now recognize `typing.Any` as a way to refer to this type explicitly. Like other special forms, this is tracked correctly through aliasing, and isn't confused with local definitions that happen to have the same name.
4f9c140
to
59e9670
Compare
|
This goes back to the actual definition of `typing.Any`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is excellent, thank you!!
A few suggested changes on non-obvious subtleties.
@@ -377,6 +377,7 @@ impl<'db> ClassBase<'db> { | |||
| KnownInstanceType::Union | |||
| KnownInstanceType::NoReturn | |||
| KnownInstanceType::Never | |||
| KnownInstanceType::Any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's under-specified, and perhaps unfortunate, but the typing spec does actually permit inheriting from Any
: https://typing.readthedocs.io/en/latest/spec/special-types.html#any
And this is used in typeshed (e.g. for unittest.mock.Mock
): https://github.com/python/typeshed/blob/main/stdlib/unittest/mock.pyi#L109
This is already supported by the ClassBase
enum: we should return Some(Self::Any)
instead of None
for KnownInstanceType::Any
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some tests for subclasses; see below
@@ -377,6 +377,7 @@ impl<'db> ClassBase<'db> { | |||
| KnownInstanceType::Union | |||
| KnownInstanceType::NoReturn | |||
| KnownInstanceType::Never | |||
| KnownInstanceType::Any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some tests for subclasses; see below
crates/red_knot_python_semantic/resources/mdtest/annotations/any.md
Outdated
Show resolved
Hide resolved
x: Subclass = 1 | ||
y: int = Subclass() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've updated the inference logic correctly to treat instances of Subclass
as if they were Any
instead of Instance(Subclass)
. These lines both got assignment errors before the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to make that change; Subclass
should still have the type Instance(Subclass)
, not the type Any
. Our representation of the class Subclass
will have Any
in its method resolution order (MRO), which will impact how we treat it in some cases. For example, it should be assignable to any other non-final type, since it might be a subclass of that type, as mentioned below, and accessing an unknown attribute on it should give an attribute type of Any
(the unknown base type might have that attribute, with some unknown type). But these are cases that we will implement in e.g. Type::is_assignable_to
and Type::member
, not by making the subclass of Any equivalent to Any.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite impressive for your first PR! Nice work.
Unfortunately I wasn't clear about the semantics of inheriting Any (because I wasn't actually expecting you to go ahead and implement those semantics in this PR!), so I think we'll need to revert a couple of the changes here. I would suggest leaving the semantics of inheriting Any to a separate PR.
crates/red_knot_python_semantic/resources/mdtest/annotations/any.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/annotations/any.md
Outdated
Show resolved
Hide resolved
# Since Subclass is a subclass of Any, it is assignable to and from any other type, just like Any. | ||
x: Subclass = 1 | ||
y: int = Subclass() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment, and the line x: Subclass = 1
, are not quite right; that latter assignment should be an error. Subclass
is not equivalent to Any
; it's a distinct class, which has an unknown base class. There is no "materialization" (using the term from the typing spec, to mean "replacing Any with a concrete static type") of the unknown base class that could possibly result in 1
being an instance of Subclass
.
The second test line here is correct; since Subclass
inherits from an unknown type, that unknown type could be (could materialize to) int
or a subclass of int
, therefore we should allow the assignment y: int = Subclass()
.
You can see that both pyright and mypy agree with this, via their online playgrounds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for future reference, not because it's relevant to this PR: one place where I think mypy and pyright get it wrong is that they also allow the assignment x: Fin = Sub()
in this code sample. Because Fin
is a final class, it can have no subclasses, therefore the unknown base class of Sub
can't materialize to Fin
, therefore I don't think that assignment should be allowed. But this is for future: we don't even have support for final classes in red-knot at all yet.
x: Subclass = 1 | ||
y: int = Subclass() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to make that change; Subclass
should still have the type Instance(Subclass)
, not the type Any
. Our representation of the class Subclass
will have Any
in its method resolution order (MRO), which will impact how we treat it in some cases. For example, it should be assignable to any other non-final type, since it might be a subclass of that type, as mentioned below, and accessing an unknown attribute on it should give an attribute type of Any
(the unknown base type might have that attribute, with some unknown type). But these are cases that we will implement in e.g. Type::is_assignable_to
and Type::member
, not by making the subclass of Any equivalent to Any.
* main: [`ruff`] Extend unnecessary-regular-expression to non-literal strings (`RUF055`) (#14679) Minor followups to RUF052 (#14755) [red-knot] Property tests (#14178) [red-knot] `is_subtype_of` fix for `KnownInstance` types (#14750) Improve docs for flake8-use-pathlib rules (#14741) [`ruff`] Implemented `used-dummy-variable` (`RUF052`) (#14611) [red-knot] Simplify tuples containing `Never` (#14744) Possible fix for flaky file watching test (#14543) [`flake8-import-conventions`] Improve syntax check for aliases supplied in configuration for `unconventional-import-alias (ICN001)` (#14745) [red-knot] Deeper understanding of `LiteralString` (#14649) red-knot: support narrowing for bool(E) (#14668) [`refurb`] Handle non-finite decimals in `verbose-decimal-constructor (FURB157)` (#14596) [red-knot] Re-enable linter corpus tests (#14736)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks excellent! Two very small wording nits. Feel free to merge, and congrats on landing your first red-knot PR!
crates/red_knot_python_semantic/resources/mdtest/annotations/any.md
Outdated
Show resolved
Hide resolved
* main: [red-knot] Test: Hashable/Sized => A/B (#14769) [`flake8-type-checking`] Expands TC006 docs to better explain itself (#14749) [`pycodestyle`] Handle f-strings properly for `invalid-escape-sequence (W605)` (#14748) [red-knot] Add fuzzer to catch panics for invalid syntax (#14678) Check `AIR001` from builtin or providers `operators` module (#14631) [airflow]: extend removed names (AIR302) (#14734)
We already had a representation for the Any type, which we would use e.g. for expressions without type annotations. We now recognize
typing.Any
as a way to refer to this type explicitly. Like other special forms, this is tracked correctly through aliasing, and isn't confused with local definitions that happen to have the same name.Closes #14544