-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Exclude the same special attributes from Protocol as CPython #15490
Conversation
Reduce the items to just `typing._SPECIAL_NAMES`
This comment has been minimized.
This comment has been minimized.
mypy/nodes.py
Outdated
@@ -3017,6 +3017,24 @@ class is generic then it will be a type constructor of higher kind. | |||
"is_intersection", | |||
] | |||
|
|||
# Special attributes not collected as protocol members by Python 3.12 | |||
# See typing._SPECIAL_NAMES | |||
EXCLUDED_ATTRIBUTES: Final = frozenset( |
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 that module level constant is better :)
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 moved it back above the class. Unless it should go somewhere else.
mypy/nodes.py
Outdated
"__annotations__", | ||
"__dict__", | ||
"__doc__", | ||
"__init__", |
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.
CC @AlexWaygood about that
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 excluding __init__
and __new__
makes sense. As @HexDecimal says, it's what CPython does: https://github.com/python/cpython/blob/4328dc646517f9251bdf6c827014f01c5229e8d9/Lib/typing.py#L1678-L1682
In general, constructors and initialisers aren't thought to be relevant to whether or not a class abides by the Liskov Substitution Principle. In general, there's no guarantee that just because class X
is a subtype of class Y
, class X
's initialiser will therefore be compatible with class Y
's initialiser.
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.
Python had a change in 3.11 so that __init__
methods on protocol base classes are preserved in concrete subclasses of that protocol, but it still doesn't an consider __init__
method a protocol member for the purposes of isinstance()
or issubclass()
checks, I don't think
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 found an old discussion of this topic at #3823.
mypy/nodes.py
Outdated
"__doc__", | ||
"__init__", | ||
"__module__", | ||
"__new__", |
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 am also not sure about __new__
. I think that there might be valid use-case for it.
This comment has been minimized.
This comment has been minimized.
Confirms and fixes python#11013
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
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.
Thanks for picking this up! This looks good to me, are people concerned about this?
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 good to me!
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 good!
The __class_getitem__
is technically a lie for 3.8, but its a reserved name so I don't see any way it could break things to just add it unconditionally.
Fixes #11884
Fixes #11886
Tests have been added for these cases.
PR is based on #11885
@sobolevn
CPython holds a collection of attributes which it excludes from protocols. It can be viewed here. This list isn't a public part of the API so I copied it into Mypy. Using this collection directly from Python would cause version-based problems anyways.
I'm not sure about where
EXCLUDED_ATTRIBUTES
should go in Mypy. Different versions of CPython also exclude different things and I'm not sure how to handle that in Mypy's source.__class_getitem__
was added to this collection in Python 3.9 for example. This is using the latest version of the collection which is likely to change again in the future, so maybe a new test could check for that happening.I wanted to add the PathLike example from #11886 but couldn't figure out how to include
from types import GenericAlias
in a test.#11886 had a debate over what should be included or excluded. This PR chooses to refer to CPython instead of arguing over each item. A debase can still be had, but I think this collection is a better starting point. I chose to use the items only from
typing._SPECIAL_NAMES
as the others looked like CPython internals.Also fixes #11013