-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
stdlib: add __slots__ #14611
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
stdlib: add __slots__ #14611
Conversation
| def variance(data: Iterable[_NumberT], xbar: _NumberT | None = None) -> _NumberT: ... | ||
|
|
||
| class NormalDist: | ||
| __slots__ = {"_mu": "Arithmetic mean of a normal distribution", "_sigma": "Standard deviation of a normal distribution"} |
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 a little weird; the values are used as documentation for the slots. My tool just copied over the slots as is, but maybe we don't want the values in typeshed, but then how do we preserve the type of __slots__? I don't have a strong opinion.
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.
There's also a case like this in tarfile but I backed it out of this PR because the linter was complaining that the strings were too long. I can add it in a separate PR later.
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 is okay, especially if stubtest will verify that the values are consistent with the values at runtime when __slots__ is a dictionary at runtime. This is a feature supported by the language (the values of the dictionary will show up in the output of help() as per-attribute documentation when you call help(NormalDist) in the REPL). It's conceivable that we might even start displaying them in tooltips in ty's language server when you hover over a class or attribute.
|
Mypy complains I tried to reproduce this locally but it seems quite fragile; it goes away if I make As a workaround, adding an explicit annotation |
This comment has been minimized.
This comment has been minimized.
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 great!
|
|
||
| class Protocol(BaseProtocol): | ||
| # Need annotation or mypy will complain about 'Cannot determine type of "__slots__" in base class' | ||
| __slots__: tuple[()] = () |
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.
should this be
| __slots__: tuple[()] = () | |
| __slots__: ClassVar[tuple[()]] = () |
?
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.
Or maybe Final.
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.
Or maybe Final.
That would imply that you can't override it in subclasses, which seems questionable?
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.
Oh right. Well, if type checkers used standard inheritance rules you couldn't override this anyway since no other type is a subtype of tuple[()].
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.
Actually this makes mypy unhappy, it now also wants you to use a ClassVar annotation in all child classes. I'm inclined to go back to the bare tuple[()] annotation unless it causes concrete problems with some type checker.
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.
yeah that's fine
| if sys.version_info >= (3, 13): | ||
| __slots__ = ( | ||
| "_raw_paths", | ||
| "_drv", | ||
| "_root", | ||
| "_tail_cached", | ||
| "_str", | ||
| "_str_normcase_cached", | ||
| "_parts_normcase_cached", | ||
| "_hash", | ||
| ) | ||
| elif sys.version_info >= (3, 12): | ||
| __slots__ = ( | ||
| "_raw_paths", | ||
| "_drv", | ||
| "_root", | ||
| "_tail_cached", | ||
| "_str", | ||
| "_str_normcase_cached", | ||
| "_parts_normcase_cached", | ||
| "_lines_cached", | ||
| "_hash", | ||
| ) | ||
| else: | ||
| __slots__ = ("_drv", "_root", "_parts", "_str", "_hash", "_pparts", "_cached_cparts") |
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 guess this indicates that we'll be taking on a bit more of a maintenance burden by doing this: these are intended to be implementation details at runtime (all the user should care about is that you can't set any public-API attributes -- they're all read-only properties). I still think the benefits are worth the costs here, though.
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.
Yes, there are a couple of classes that have had their slots changed over time (you can see in the later commits of this PR). But really not that many.
| def variance(data: Iterable[_NumberT], xbar: _NumberT | None = None) -> _NumberT: ... | ||
|
|
||
| class NormalDist: | ||
| __slots__ = {"_mu": "Arithmetic mean of a normal distribution", "_sigma": "Standard deviation of a normal distribution"} |
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 is okay, especially if stubtest will verify that the values are consistent with the values at runtime when __slots__ is a dictionary at runtime. This is a feature supported by the language (the values of the dictionary will show up in the output of help() as per-attribute documentation when you call help(NormalDist) in the REPL). It's conceivable that we might even start displaying them in tooltips in ty's language server when you hover over a class or attribute.
|
Diff from mypy_primer, showing the effect of this PR on open source code: psycopg (https://github.com/psycopg/psycopg)
+ tests/types/test_uuid.py:62: error: Unused "type: ignore" comment [unused-ignore]
discord.py (https://github.com/Rapptz/discord.py)
- ...typeshed_to_test/stdlib/typing.pyi:1016: note: "update" of "TypedDict" defined here
+ ...typeshed_to_test/stdlib/typing.pyi:1029: note: "update" of "TypedDict" defined here
- discord/ext/commands/hybrid.py:508: error: Overlap between argument names and ** TypedDict items: "description", "name" [misc]
+ discord/ext/commands/hybrid.py:508: error: Overlap between argument names and ** TypedDict items: "name", "description" [misc]
- discord/ext/commands/hybrid.py:629: error: Overlap between argument names and ** TypedDict items: "description", "name" [misc]
+ discord/ext/commands/hybrid.py:629: error: Overlap between argument names and ** TypedDict items: "name", "description" [misc]
- discord/ext/commands/hybrid.py:834: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
+ discord/ext/commands/hybrid.py:834: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
- discord/ext/commands/hybrid.py:858: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
+ discord/ext/commands/hybrid.py:858: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
- discord/ext/commands/hybrid.py:883: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
+ discord/ext/commands/hybrid.py:883: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
- discord/ext/commands/hybrid.py:935: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
+ discord/ext/commands/hybrid.py:935: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
- discord/ext/commands/bot.py:290: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
+ discord/ext/commands/bot.py:290: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
- discord/ext/commands/bot.py:314: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
+ discord/ext/commands/bot.py:314: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
|
Done using JelleZijlstra/stubdefaulter#103
Part of #8832