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.

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

Enforce mypy-cleanliness on trio._core #1610

Merged
merged 15 commits into from
Jun 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions check.sh
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,14 @@ flake8 trio/ \
--ignore=D,E,W,F401,F403,F405,F821,F822\
|| EXIT_STATUS=$?

# Run mypy
# We specify Linux so that we get the same results on all platforms. Without
Copy link
Member

Choose a reason for hiding this comment

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

Can you edit this comment to mention your future plans listed below? (run mypy on all platforms, use assert guards to avoid typechecking the wrong IOManagers)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, done

# that, mypy would complain on macOS that epoll does not exist, and we can't
# ignore it as it would cause a mypy error on Linux
# In the future, we will run mypy on all platforms and use platform assert
# guards to avoid typechecking the wrong IOManagers
mypy -p trio._core --platform linux || EXIT_STATUS=$?

# Finally, leave a really clear warning of any issues and exit
if [ $EXIT_STATUS -ne 0 ]; then
cat <<EOF
Expand Down
3 changes: 3 additions & 0 deletions test-requirements.in
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,14 @@ jedi # for jedi code completion tests

# Tools
black; implementation_name == "cpython"
mypy; implementation_name == "cpython"
flake8
astor # code generation

# https://github.com/python-trio/trio/pull/654#issuecomment-420518745
typed_ast; implementation_name == "cpython"
mypy-extensions; implementation_name == "cpython"
typing-extensions; implementation_name == "cpython"

# Trio's own dependencies
cffi; os_name == "nt"
Expand Down
3 changes: 3 additions & 0 deletions test-requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ jedi==0.17.1 # via -r test-requirements.in, ipython
lazy-object-proxy==1.4.3 # via astroid
mccabe==0.6.1 # via flake8, pylint
more-itertools==8.4.0 # via pytest
mypy-extensions==0.4.3 ; implementation_name == "cpython" # via mypy
mypy==0.780 ; implementation_name == "cpython" # via -r test-requirements.in
outcome==1.0.1 # via -r test-requirements.in
packaging==20.4 # via pytest
parso==0.7.0 # via jedi
Expand Down Expand Up @@ -53,5 +55,6 @@ toml==0.10.1 # via black, pylint
traitlets==4.3.3 # via ipython
trustme==0.6.0 # via -r test-requirements.in
typed-ast==1.4.1 ; implementation_name == "cpython" # via -r test-requirements.in, black
typing-extensions==3.7.4.2; implementation_name == "cpython" # via mypy
wcwidth==0.2.5 # via prompt-toolkit, pytest
wrapt==1.12.1 # via astroid
17 changes: 8 additions & 9 deletions trio/_core/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
are publicly available in either trio, trio.lowlevel, or trio.testing.
"""

import sys
Copy link
Member

Choose a reason for hiding this comment

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

I guess technically this should be import sys as _sys, though in practice it doesn't do much harm here...

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, importing sys as _sys breaks mypy...

Copy link
Member

Choose a reason for hiding this comment

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

del sys at the end will clean up sufficiently.

Yeah, mypy's platform guards are rather brittle. It's strange since, for example, the magic from typing import TYPE_CHECKING seems to work under any name...

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, added del sys


from ._exceptions import (
TrioInternalError,
RunFinishedError,
Expand Down Expand Up @@ -73,14 +75,8 @@

from ._mock_clock import MockClock

# Kqueue imports
try:
from ._run import current_kqueue, monitor_kevent, wait_kevent
except ImportError:
pass

# Windows imports
try:
if sys.platform == "win32":
from ._run import (
monitor_completion_key,
current_iocp,
Expand All @@ -89,5 +85,8 @@
write_overlapped,
readinto_overlapped,
)
except ImportError:
pass
# Kqueue imports
elif sys.platform != "linux" and sys.platform != "win32":
from ._run import current_kqueue, monitor_kevent, wait_kevent

del sys # It would be better to import sys as _sys, but mypy does not understand it
5 changes: 4 additions & 1 deletion trio/_core/_io_epoll.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import select
import attr
from collections import defaultdict
from typing import Dict

from .. import _core
from ._run import _public
Expand Down Expand Up @@ -184,7 +185,9 @@ class EpollWaiters:
class EpollIOManager:
_epoll = attr.ib(factory=select.epoll)
# {fd: EpollWaiters}
_registered = attr.ib(factory=lambda: defaultdict(EpollWaiters))
_registered = attr.ib(
factory=lambda: defaultdict(EpollWaiters), type=Dict[int, EpollWaiters]
)
_force_wakeup = attr.ib(factory=WakeupSocketpair)
_force_wakeup_fd = attr.ib(default=None)

Expand Down
2 changes: 1 addition & 1 deletion trio/_core/_io_kqueue.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class _KqueueStatistics:

@attr.s(slots=True, eq=False)
class KqueueIOManager:
_kqueue = attr.ib(factory=select.kqueue)
_kqueue = attr.ib(factory=select.kqueue) # type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

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

This works because we require Linux when running mypy. But if I don't specify the platform, I get the following errors on macOS:

trio/_core/_io_kqueue.py:22: error: unused 'type: ignore' comment
trio/_core/_io_epoll.py:186: error: Module has no attribute "epoll"; maybe "poll"?

I plan to fix that in a further pull request by adding assert sys.platform.startswith("freebsd") or sys.platform.startswith("darwin") at the top of the file and running mypy on the various platforms we support.

# {(ident, filter): Task or UnboundedQueue}
_registered = attr.ib(factory=dict)
_force_wakeup = attr.ib(factory=WakeupSocketpair)
Expand Down
6 changes: 3 additions & 3 deletions trio/_core/_multierror.py
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ def traceback_exception_init(
self.embedded = []


traceback.TracebackException.__init__ = traceback_exception_init
traceback.TracebackException.__init__ = traceback_exception_init # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

I'm not too worried about # type: ignore comments inside this horrible blob of monkeypatching, but I am curious why you need them.

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 error is trio/_core/_multierror.py:418: error: Cannot assign to a method. It appears to be a known mypy bug: python/mypy#2427. It's unclear to me if setattr is a better workaround or not.

traceback_exception_original_format = traceback.TracebackException.format


Expand All @@ -427,7 +427,7 @@ def traceback_exception_format(self, *, chain=True):
yield from (textwrap.indent(line, " " * 2) for line in exc.format(chain=chain))


traceback.TracebackException.format = traceback_exception_format
traceback.TracebackException.format = traceback_exception_format # type: ignore


def trio_excepthook(etype, value, tb):
Expand Down Expand Up @@ -489,7 +489,7 @@ class TrioFakeSysModuleForApport:

fake_sys = TrioFakeSysModuleForApport()
fake_sys.__dict__.update(sys.__dict__)
fake_sys.__excepthook__ = trio_excepthook
fake_sys.__excepthook__ = trio_excepthook # type: ignore
apport_python_hook.sys = fake_sys

monkeypatched_or_warned = True
Expand Down
14 changes: 8 additions & 6 deletions trio/_core/_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,14 @@
from contextvars import copy_context
from math import inf
from time import perf_counter
from typing import Callable, TYPE_CHECKING

from sniffio import current_async_library_cvar

import attr
from heapq import heapify, heappop, heappush
from sortedcontainers import SortedDict
from outcome import Error, Value, capture
from outcome import Error, Outcome, Value, capture

from ._entry_queue import EntryQueue, TrioToken
from ._exceptions import TrioInternalError, RunFinishedError, Cancelled
Expand Down Expand Up @@ -655,7 +656,7 @@ def shield(self):
"""
return self._shield

@shield.setter
@shield.setter # type: ignore # "decorated property not supported"
@enable_ki_protection
def shield(self, new_value):
if not isinstance(new_value, bool):
Expand Down Expand Up @@ -1223,7 +1224,8 @@ class GuestState:
run_sync_soon_not_threadsafe = attr.ib()
done_callback = attr.ib()
unrolled_run_gen = attr.ib()
unrolled_run_next_send = attr.ib(factory=lambda: Value(None))
_value_factory: Callable[[], Value] = lambda: Value(None)
unrolled_run_next_send = attr.ib(factory=_value_factory, type=Outcome)
Copy link
Member

Choose a reason for hiding this comment

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

You can use cast() to write this without introducing an additional class-level name. (cast(Callable[[], Value], lambda: Value(None))). I'm also curious what breaks if you write it on one line without the cast.

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 error message without the lambda type hint is Returning Any from function declared to return "object". Indeed according to reveal_type the type of lambda: Value(None) is 'def () -> Any'.

So far so good, but if I switch to your inline cast which specifies the Value return type, I get trio/_core/_run.py:1227: error: Redundant cast to "Callable[[], Any]"! It's maybe a variance issue?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, this is probably because outcome is not typed, so mypy is replacing everything imported from it with Any.

We should add type hints to outcome. In the meantime, your workaround here seems fine, just note that we're not actually getting anything from declaring the type as Outcome as opposed to Any.


def guest_tick(self):
try:
Expand Down Expand Up @@ -2374,13 +2376,13 @@ async def checkpoint_if_cancelled():
task._cancel_points += 1


if os.name == "nt":
if sys.platform == "win32":
from ._io_windows import WindowsIOManager as TheIOManager
from ._generated_io_windows import *
elif hasattr(select, "epoll"):
elif sys.platform == "linux" or (not TYPE_CHECKING and hasattr(select, "epoll")):
from ._io_epoll import EpollIOManager as TheIOManager
from ._generated_io_epoll import *
elif hasattr(select, "kqueue"):
elif TYPE_CHECKING or hasattr(select, "kqueue"):
from ._io_kqueue import KqueueIOManager as TheIOManager
from ._generated_io_kqueue import *
else: # pragma: no cover
Expand Down
2 changes: 1 addition & 1 deletion trio/_core/tests/test_multierror.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
extract_tb,
print_exception,
format_exception,
_cause_message,
)
from traceback import _cause_message # type: ignore
import sys
import os
import re
Expand Down