Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork3.1k
Exclude the same special attributes from Protocol as CPython#15490
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This comment has been minimized.
This comment has been minimized.
mypy/nodes.py Outdated
| # 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.
Ithink 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 classX is a subtype of classY, classX's initialiser will therefore be compatible with classY'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 ofisinstance() orissubclass() 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 andfixespython#11013
According tomypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
hauntsaninja left a comment
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?
AlexWaygood left a comment
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!
emmatyping left a comment
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.
Uh oh!
There was an error while loading.Please reload this page.
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_ATTRIBUTESshould 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 GenericAliasin 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 debate can still be had, but I think this collection is a better starting point. I chose to use the items only from
typing._SPECIAL_NAMESas the others looked like CPython internals.Alsofixes#11013