Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove, move or # noqa more TypeAlias declarations #8450

Merged
merged 3 commits into from
Aug 5, 2022
Merged

Remove, move or # noqa more TypeAlias declarations #8450

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
571e375
Remove, move or `# noqa` more `TypeAlias` declarations
AlexWaygood Jul 31, 2022
1424f10
Improve comment
AlexWaygood Aug 5, 2022
afbf143
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions stdlib/email/__init__.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ from typing import IO, Union
from typing_extensions import TypeAlias

# Definitions imported by multiple submodules in typeshed
_ParamType: TypeAlias = Union[str, tuple[str | None, str | None, str]]
_ParamsType: TypeAlias = Union[str, None, tuple[str, str | None, str]]
_ParamType: TypeAlias = Union[str, tuple[str | None, str | None, str]] # noqa: Y047
_ParamsType: TypeAlias = Union[str, None, tuple[str, str | None, str]] # noqa: Y047

def message_from_string(s: str, _class: Callable[[], Message] = ..., *, policy: Policy = ...) -> Message: ...
def message_from_bytes(s: bytes, _class: Callable[[], Message] = ..., *, policy: Policy = ...) -> Message: ...
Expand Down
2 changes: 1 addition & 1 deletion stdlib/lib2to3/pgen2/__init__.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@ from typing import Any
from typing_extensions import TypeAlias

# This is imported in several lib2to3/pgen2 submodules
_Convert: TypeAlias = Callable[[Grammar, _RawNode], Any]
_Convert: TypeAlias = Callable[[Grammar, _RawNode], Any] # noqa: Y047
29 changes: 3 additions & 26 deletions stdlib/multiprocessing/__init__.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import sys
from multiprocessing import context, reduction as reducer, synchronize
from multiprocessing import context, reduction as reducer
from multiprocessing.context import (
AuthenticationError as AuthenticationError,
BufferTooShort as BufferTooShort,
Expand All @@ -10,12 +10,10 @@ from multiprocessing.context import (
from multiprocessing.process import active_children as active_children, current_process as current_process

# These are technically functions that return instances of these Queue classes.
# Using them as annotations is deprecated. Either use imports from
# multiprocessing.queues or the aliases defined below. See #4266 for discussion.
# Using them as annotations is deprecated. Use imports from multiprocessing.queues instead.
# See #4266 for discussion.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment is just outdated? As far as I can see, all these are classes in the implementation, at least in Python 3.7+. Was this maybe different in the past? In that case, I don't think we need the type aliases below.

Copy link
Collaborator

@Akuli Akuli Jul 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They became functions in the stub at some point, but it was reverted.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow I missed that you asked about the implementation, not about the stub. These are functions (actually methods) in the implementation:

>>> from multiprocessing import Queue
>>> Queue
<bound method BaseContext.Queue of <multiprocessing.context.DefaultContext object at 0x7f66f26d73d0>>

There is also an undocumented class that is different from the public and documented Queue:

>>> from multiprocessing.queues import Queue
>>> Queue
<class 'multiprocessing.queues.Queue'>

There's similarly e.g. a method multiprocessing.Event and a class multiprocessing.synchronize.Event.

Copy link
Member Author

@AlexWaygood AlexWaygood Jul 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what happens at runtime:

>>> import multiprocessing
>>> multiprocessing.Queue
<bound method BaseContext.Queue of <multiprocessing.context.DefaultContext object at 0x000002F32AF7E5C0>>
>>> multiprocessing.Queue[int]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'method' object is not subscriptable

So the stubs are a bit of a lie at the moment. As @Akuli says, the stubs were changed in #4289 so that they could match the runtime more closely, but then that was reverted in #4314 after Jukka complained in #4313.

The thing is, either way, I don't think having these aliases helps the situation much. To work around these issues, a user can probably just put multiprocessing.Queue in quotes in their annotations, or use from __future__ import annotations.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should try to make them into functions again in the stubs and see the fallout, now that we have mypy_primer. Annotating something with muliprocessing.Queue, which is a function, is not something we should support.

This comment was marked as off-topic.

Sign in to view

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I marked my previous comment as off-topic, as it doesn't belong at all to this PR. I somehow didn't see other people's comments that were posted before I wrote mine.

Copy link
Member Author

@AlexWaygood AlexWaygood Jul 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the type aliases are connected to this, we should keep those that are alternatives to a function.

I don't really see how the aliases help, even the ones which are alternatives to a function.

Take multiprocessing.Lock -- multiprocessing.Lock is a method, but multiprocessing.synchronize.Lock is a class. If it's a choice between doing this:

from multiprocessing.synchronize import Lock
foo: Lock

or this:

from typing import TYPE_CHECKING
if TYPE_CHECKING:
    from multiprocessing import _LockType
foo: "_LockType"

Then, I find the first way much cleaner and more ergonomic. And, as @Akuli says in his minimized comment, we know none of the projects in mypy_primer are using these aliases.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being unclear. Yes, if there is a valid type somewhere, like multiprocessing.synchronize.Lock, we don't need the alias. It's possible that this is true for all aliases, I didn't check.

Copy link
Member Author

@AlexWaygood AlexWaygood Jul 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_BarrierType, _BoundedSemaphoreType, _ConditionType, _EventType, _LockType, _RLockType and _SemaphoreType are all aliases to valid types in multiprocessing.synchronize.

_QueueType, _SimpleQueueType and _JoinableQueueType are all aliases to valid types in multiprocessing.queues.

So yeah, it's true for all the aliases :)

AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
from multiprocessing.queues import JoinableQueue as JoinableQueue, Queue as Queue, SimpleQueue as SimpleQueue
from multiprocessing.spawn import freeze_support as freeze_support
from typing import TypeVar
from typing_extensions import TypeAlias

if sys.version_info >= (3, 8):
from multiprocessing.process import parent_process as parent_process
Expand Down Expand Up @@ -62,27 +60,6 @@ __all__ = [
if sys.version_info >= (3, 8):
__all__ += ["parent_process"]

# The following type aliases can be used to annotate the return values of
# the corresponding functions. They are not defined at runtime.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were a bit controversial when @srittau added them, and he probably doesn't want them removed: #5346

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for the reference.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other options are:

  • Special-case __init__.pyi files in the flake8-pyi check (what I did originally, but @JelleZijlstra doesn't like that).
  • Just # noqa all the ones in multiprocessing.
  • Abandon the flake8-pyi check entirely.
  • Something else?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is somewhat of a "true positive". These type aliases are just weird: named as if they were private, intended to be public, not documented, not discoverable. But again, @srittau will probably disagree. I think we should wait for his input.

Copy link
Member Author

@AlexWaygood AlexWaygood Jul 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But again, @srittau will probably disagree. I think we should wait for his input.

Yeah of course, not going to merge without it :)

#
# from multiprocessing import Lock
# from typing import TYPE_CHECKING
# if TYPE_CHECKING:
# from multiprocessing import _LockType
# lock: _LockType = Lock()

_T = TypeVar("_T")
_QueueType: TypeAlias = Queue[_T]
_SimpleQueueType: TypeAlias = SimpleQueue[_T]
_JoinableQueueType: TypeAlias = JoinableQueue[_T]
_BarrierType: TypeAlias = synchronize.Barrier
_BoundedSemaphoreType: TypeAlias = synchronize.BoundedSemaphore
_ConditionType: TypeAlias = synchronize.Condition
_EventType: TypeAlias = synchronize.Event
_LockType: TypeAlias = synchronize.Lock
_RLockType: TypeAlias = synchronize.RLock
_SemaphoreType: TypeAlias = synchronize.Semaphore

# These functions (really bound methods)
# are all autogenerated at runtime here: https://github.com/python/cpython/blob/600c65c094b0b48704d8ec2416930648052ba715/Lib/multiprocessing/__init__.py#L23
RawValue = context._default_context.RawValue
Expand Down
7 changes: 0 additions & 7 deletions stdlib/tkinter/__init__.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,13 +186,6 @@ _EntryValidateCommand: TypeAlias = (
) # example when it's sequence: entry['invalidcommand'] = [entry.register(print), '%P']
_GridIndex: TypeAlias = int | str | Literal["all"]
_ImageSpec: TypeAlias = _Image | str # str can be from e.g. tkinter.image_names()
_Padding: TypeAlias = Union[
_ScreenUnits,
tuple[_ScreenUnits],
tuple[_ScreenUnits, _ScreenUnits],
tuple[_ScreenUnits, _ScreenUnits, _ScreenUnits],
tuple[_ScreenUnits, _ScreenUnits, _ScreenUnits, _ScreenUnits],
]
_Relief: TypeAlias = Literal["raised", "sunken", "flat", "ridge", "solid", "groove"] # manual page: Tk_GetRelief
_ScreenUnits: TypeAlias = str | float # Often the right type instead of int. Manual page: Tk_GetPixels
_XYScrollCommand: TypeAlias = str | Callable[[float, float], Any] # -xscrollcommand and -yscrollcommand in 'options' manual page
Expand Down
32 changes: 20 additions & 12 deletions stdlib/tkinter/ttk.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import sys
import tkinter
from collections.abc import Callable
from tkinter.font import _FontDescription
from typing import Any, overload
from typing import Any, Union, overload
from typing_extensions import Literal, TypeAlias, TypedDict

__all__ = [
Expand Down Expand Up @@ -37,6 +37,14 @@ __all__ = [
def tclobjs_to_py(adict: dict[Any, Any]) -> dict[Any, Any]: ...
def setup_master(master: Any | None = ...): ...

_Padding: TypeAlias = Union[
tkinter._ScreenUnits,
tuple[tkinter._ScreenUnits],
tuple[tkinter._ScreenUnits, tkinter._ScreenUnits],
tuple[tkinter._ScreenUnits, tkinter._ScreenUnits, tkinter._ScreenUnits],
tuple[tkinter._ScreenUnits, tkinter._ScreenUnits, tkinter._ScreenUnits, tkinter._ScreenUnits],
]

# from ttk_widget (aka ttk::widget) manual page, differs from tkinter._Compound
_TtkCompound: TypeAlias = Literal["text", "image", tkinter._Compound]

Expand Down Expand Up @@ -336,7 +344,7 @@ class Frame(Widget):
cursor: tkinter._Cursor = ...,
height: tkinter._ScreenUnits = ...,
name: str = ...,
padding: tkinter._Padding = ...,
padding: _Padding = ...,
relief: tkinter._Relief = ...,
style: str = ...,
takefocus: tkinter._TakeFocusValue = ...,
Expand All @@ -351,7 +359,7 @@ class Frame(Widget):
borderwidth: tkinter._ScreenUnits = ...,
cursor: tkinter._Cursor = ...,
height: tkinter._ScreenUnits = ...,
padding: tkinter._Padding = ...,
padding: _Padding = ...,
relief: tkinter._Relief = ...,
style: str = ...,
takefocus: tkinter._TakeFocusValue = ...,
Expand All @@ -378,7 +386,7 @@ class Label(Widget):
image: tkinter._ImageSpec = ...,
justify: Literal["left", "center", "right"] = ...,
name: str = ...,
padding: tkinter._Padding = ...,
padding: _Padding = ...,
relief: tkinter._Relief = ...,
state: str = ...,
style: str = ...,
Expand All @@ -404,7 +412,7 @@ class Label(Widget):
foreground: tkinter._Color = ...,
image: tkinter._ImageSpec = ...,
justify: Literal["left", "center", "right"] = ...,
padding: tkinter._Padding = ...,
padding: _Padding = ...,
relief: tkinter._Relief = ...,
state: str = ...,
style: str = ...,
Expand Down Expand Up @@ -432,7 +440,7 @@ class Labelframe(Widget):
labelanchor: Literal["nw", "n", "ne", "en", "e", "es", "se", "s", "sw", "ws", "w", "wn"] = ...,
labelwidget: tkinter.Misc = ...,
name: str = ...,
padding: tkinter._Padding = ...,
padding: _Padding = ...,
relief: tkinter._Relief = ..., # undocumented
style: str = ...,
takefocus: tkinter._TakeFocusValue = ...,
Expand All @@ -451,7 +459,7 @@ class Labelframe(Widget):
height: tkinter._ScreenUnits = ...,
labelanchor: Literal["nw", "n", "ne", "en", "e", "es", "se", "s", "sw", "ws", "w", "wn"] = ...,
labelwidget: tkinter.Misc = ...,
padding: tkinter._Padding = ...,
padding: _Padding = ...,
relief: tkinter._Relief = ...,
style: str = ...,
takefocus: tkinter._TakeFocusValue = ...,
Expand Down Expand Up @@ -518,7 +526,7 @@ class Notebook(Widget):
cursor: tkinter._Cursor = ...,
height: int = ...,
name: str = ...,
padding: tkinter._Padding = ...,
padding: _Padding = ...,
style: str = ...,
takefocus: tkinter._TakeFocusValue = ...,
width: int = ...,
Expand All @@ -530,7 +538,7 @@ class Notebook(Widget):
*,
cursor: tkinter._Cursor = ...,
height: int = ...,
padding: tkinter._Padding = ...,
padding: _Padding = ...,
style: str = ...,
takefocus: tkinter._TakeFocusValue = ...,
width: int = ...,
Expand All @@ -544,7 +552,7 @@ class Notebook(Widget):
*,
state: Literal["normal", "disabled", "hidden"] = ...,
sticky: str = ..., # consists of letters 'n', 's', 'w', 'e', no repeats, may be empty
padding: tkinter._Padding = ...,
padding: _Padding = ...,
text: str = ...,
image: Any = ..., # Sequence of an image name, followed by zero or more (sequences of one or more state names followed by an image name)
compound: tkinter._Compound = ...,
Expand Down Expand Up @@ -957,7 +965,7 @@ class Treeview(Widget, tkinter.XView, tkinter.YView):
displaycolumns: str | list[str] | tuple[str, ...] | list[int] | tuple[int, ...] | Literal["#all"] = ...,
height: int = ...,
name: str = ...,
padding: tkinter._Padding = ...,
padding: _Padding = ...,
selectmode: Literal["extended", "browse", "none"] = ...,
# list/tuple of Literal don't actually work in mypy
#
Expand All @@ -978,7 +986,7 @@ class Treeview(Widget, tkinter.XView, tkinter.YView):
cursor: tkinter._Cursor = ...,
displaycolumns: str | list[str] | tuple[str, ...] | list[int] | tuple[int, ...] | Literal["#all"] = ...,
height: int = ...,
padding: tkinter._Padding = ...,
padding: _Padding = ...,
selectmode: Literal["extended", "browse", "none"] = ...,
show: Literal["tree", "headings", "tree headings", ""] | list[str] | tuple[str, ...] = ...,
style: str = ...,
Expand Down