Enforce mypy-cleanliness on trio._core #1610

pquentin · 2020-06-12T07:16:13Z

This is a less ambitious version of #1254, the idea being that we can introduce mypy cleanliness gradually. I did copy relevant code from #1254, and fixed other issues too.

The relevant message in CI can be found in the check formatting check:

++ mypy -p trio._core --platform linux
Success: no issues found in 45 source files

pquentin · 2020-06-12T07:21:49Z

trio/_core/_io_kqueue.py

@@ -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


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.

trio/_core/_run.py

It depends on typed-ast.

codecov · 2020-06-12T08:16:23Z

Codecov Report

Merging #1610 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1610   +/-   ##
=======================================
  Coverage   99.69%   99.69%           
=======================================
  Files         110      110           
  Lines       13955    13957    +2     
  Branches     1074     1076    +2     
=======================================
+ Hits        13913    13915    +2     
  Misses         27       27           
  Partials       15       15

Impacted Files	Coverage Δ
trio/_core/__init__.py	`100.00% <100.00%> (ø)`
trio/_core/_io_epoll.py	`100.00% <100.00%> (ø)`
trio/_core/_io_kqueue.py	`85.12% <100.00%> (ø)`
trio/_core/_multierror.py	`100.00% <100.00%> (ø)`
trio/_core/_run.py	`99.77% <100.00%> (+<0.01%)`	⬆️
trio/_core/tests/test_multierror.py	`100.00% <100.00%> (ø)`

"if not TYPE_CHECKING" is always True when running the code.

pquentin · 2020-06-12T10:44:11Z

Closing/reopening because the FreeBSD build failed with #1604

pquentin · 2020-06-12T10:59:30Z

Coverage is OK, next up is understanding the unrolled_run_next_send issue.

mypy thought the lambda was returning "object".

pquentin · 2020-06-22T06:17:03Z

unrolled_run_next_send fixed, please take another look.

njsmith · 2020-06-22T07:45:14Z

trio/_core/_run.py

@@ -1151,7 +1151,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), type=object)
+    _value_factory: Callable[[], Value] = lambda: Value(None)
+    unrolled_run_next_send = attr.ib(factory=_value_factory, type=Value)


I think the type here should be outcome.Outcome, not outcome.Value? E.g. if get_events raises an exception, then unrolled_run_next_send will be an outcome.Error.

Thanks! Fixed.

The value can be an Error too, so we should use Outcome, not Value.

njsmith

@oremanj You definitely know this typing stuff better than I do, so if you have a chance to give feedback here it'd be great :-). If you're busy though no worries, we can still move ahead and fix things later.

njsmith · 2020-06-22T07:47:08Z

trio/_core/__init__.py

@@ -4,6 +4,9 @@
 are publicly available in either trio, trio.lowlevel, or trio.testing.
 """

+import typing as _t
+import sys


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

Unfortunately, importing sys as _sys breaks mypy...

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...

Thanks, added del sys

njsmith · 2020-06-22T07:49:53Z

trio/_core/_multierror.py

@@ -415,7 +415,7 @@ def traceback_exception_init(
        self.embedded = []


-traceback.TracebackException.__init__ = traceback_exception_init
+traceback.TracebackException.__init__ = traceback_exception_init  # type: ignore


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

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.

njsmith · 2020-06-22T07:55:07Z

trio/_core/_run.py

+    sys.platform == "darwin"
+    or sys.platform.startswith("freebsd")
+    or (not TYPE_CHECKING and hasattr(select, "epoll"))
+):


Well... this check is gross and we should probably complain about it to typing-sig.

But having written it, I guess we might as well copy-paste it into trio/_core/__init__.py too?

Oh heh and there's a copy/paste error here – you accidentally replaced hasattr(select, "kqueue") with hasattr(select, "epoll").

Well... this check is gross and we should probably complain about it to typing-sig.

Do you mean that mypy should be able to tell wich platforms support epoll/kqueue?

But having written it, I guess we might as well copy-paste it into trio/_core/__init__.py too?

Do you mean that this hack is better than the current try/except ImportError way in trio/_core/__init__.py?

Oh heh and there's a copy/paste error here – you accidentally replaced hasattr(select, "kqueue") with hasattr(select, "epoll").

Thanks, fixed.

Do you mean that mypy should be able to tell wich platforms support epoll/kqueue?

Maybe? I mean that having to copy-paste this complicated and not-even-terribly-correct code into multiple places is suboptimal, and maybe it could be avoided somehow. But yeah, teaching mypy to evaluate hasattr statements seems like one good way to do that... it already has a model of which attributes the select module has!

And heh, on further investigation, it looks like the way mypy's understanding of select works is:

if sys.platform != "linux" and sys.platform != "win32":

https://github.com/python/typeshed/blob/66a9a4b5ce72aaf022b24706bf58254602ef02f8/stdlib/2and3/select.pyi#L36-L38

That is probably nicer than trying to enumerate all the BSD variations... so I guess maybe for now we should do:

if sys.platform == "win32": ... elif sys.platform == "linux" or hasattr(select, "epoll"): ... elif TYPE_CHECKING or hasattr(select, "kqueue"): ... else: raise NotImplementedError

Do you mean that this hack is better than the current try/except ImportError way in trio/_core/init.py?

Yeah, b/c the way that's written right now, if you try to run mypy on macOS then it'll complain that current_kqueue and friends are undefined.

Thanks, done. (Btw, note that you can use "Quote reply" to preserve the markdown formatting of the text you're quoting.)

Any arm of the if that might be true (according to mypy's very limited static analysis) must type-check. That's the reason for all the not TYPE_CHECKING and in the original -- it basically is saying "I know you can't tell this hasattr() result when type-checking, so just ignore it". With @njsmith's suggestion, you're requiring the linux arm to type-check on kqueue platforms too (since as far as mypy knows they might have select.epoll), which is probably ultimately responsible for the need to invoke mypy with --platform linux in this diff.

So I guess that means we should do something like this?

if sys.platform == "win32": ... elif sys.platform == "linux" or (not TYPE_CHECKING and hasattr(select, "epoll")): ... elif TYPE_CHECKING or (not TYPE_CHECKING and hasattr(select, "kqueue")): ... else: raise NotImplementedError

?

Also raised this upstream here: python/mypy#9042

I don't think you need the not TYPE_CHECKING guard in the kqueue arm, because mypy can tell that TYPE_CHECKING or ... is true regardless of the ....

Thanks, switched to this. Will probably help with multiplatform support even if right now I get the exact same errors on macOS

An added benefit is that we currently don't need to look at TYPE_CHECKING or add ugly pragmas. This might change when we run mypy on more platforms.

oremanj

Sorry this took me so long to review. Looks good overall!

oremanj · 2020-06-24T07:36:38Z

check.sh

@@ -23,6 +23,12 @@ 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


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)

Thanks, done

oremanj · 2020-06-24T07:38:29Z

trio/_core/__init__.py

@@ -4,6 +4,9 @@
 are publicly available in either trio, trio.lowlevel, or trio.testing.
 """

+import typing as _t
+import sys


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...

oremanj · 2020-06-24T07:39:14Z

trio/_core/_io_epoll.py

@@ -1,6 +1,7 @@
 import select
 import attr
 from collections import defaultdict
+from typing import DefaultDict


I generally just use Dict for defaultdicts, since you rarely wind up accessing the .factory member

Switched to Dict

oremanj · 2020-06-24T07:39:45Z

trio/_core/_io_epoll.py

+    _registered = attr.ib(
+        factory=lambda: defaultdict(EpollWaiters), type=DefaultDict[int, EpollWaiters]
+    )
+    _force_wakeup = attr.ib(factory=WakeupSocketpair, type=WakeupSocketpair)


Do you need the type= here? I would expect mypy to be able to infer it from the factory return type.

You're right, that was not needed! Thanks, fixed.

oremanj · 2020-06-24T07:48:40Z

trio/_core/_run.py

@@ -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)


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.

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?

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.

mypy can infer the type from the factory.

pquentin · 2020-06-25T05:58:27Z

Thanks for the reviews! I adressed all comments, please take another look.

Enforce mypy-cleanliness on trio._core

b0ed1e4

pquentin commented Jun 12, 2020

View reviewed changes

Do not try installing mypy on PyPy

048ca9b

It depends on typed-ast.

pquentin force-pushed the mypy-trio-core branch from c9bc3cf to 048ca9b Compare June 12, 2020 08:02

Fix missing branch coverage

1c7cdc2

"if not TYPE_CHECKING" is always True when running the code.

pquentin closed this Jun 12, 2020

pquentin reopened this Jun 12, 2020

pquentin added 2 commits June 22, 2020 10:05

Fix unrolled_run_next_send mypy error

5ce4a1c

mypy thought the lambda was returning "object".

Merge remote-tracking branch 'python-trio/master' into mypy-trio-core

659c4f2

pquentin requested review from oremanj and njsmith June 22, 2020 06:17

njsmith reviewed Jun 22, 2020

View reviewed changes

Fix type annotation in _core._run

2cc70a1

The value can be an Error too, so we should use Outcome, not Value.

njsmith reviewed Jun 22, 2020

View reviewed changes

pquentin added 2 commits June 22, 2020 13:55

Fix typo (epoll -> kqueue)

0efb647

Reuse mypy checks for kqueue/select detection

81f83f5

An added benefit is that we currently don't need to look at TYPE_CHECKING or add ugly pragmas. This might change when we run mypy on more platforms.

oremanj reviewed Jun 24, 2020

View reviewed changes

njsmith mentioned this pull request Jun 24, 2020

request: Statically evaluating module-level hasattr checks python/mypy#9042

Open

pquentin added 7 commits June 25, 2020 09:28

Remove _sys from trio._core exports

affba27

Tell mypy to ignore hasattr checks

205a710

Explain future mypy plans in check.sh

5d2470f

Just use typing.Dict instead of DefaultDict

316b640

Remove unneeded attr.ib type kwarg

296490f

mypy can infer the type from the factory.

Merge remote-tracking branch 'python-trio/master' into mypy-trio-core

42019f1

Fix formatting

3a21fb3

oremanj merged commit 5e81f47 into python-trio:master Jun 25, 2020

pquentin deleted the mypy-trio-core branch June 25, 2020 06:44

Enforce mypy-cleanliness on trio._core #1610

Enforce mypy-cleanliness on trio._core #1610

Conversation

pquentin commented Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jun 12, 2020 • edited Loading

Codecov Report

pquentin commented Jun 12, 2020

pquentin commented Jun 12, 2020

pquentin commented Jun 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oremanj Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pquentin Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

oremanj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pquentin commented Jun 25, 2020 • edited Loading

pquentin commented Jun 12, 2020 •

edited

Loading

codecov bot commented Jun 12, 2020 •

edited

Loading

oremanj Jun 24, 2020 •

edited

Loading

pquentin Jun 25, 2020 •

edited

Loading

pquentin commented Jun 25, 2020 •

edited

Loading