Add types to _core._run
#2733
Add types to _core._run
#2733
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2733 +/- ##
==========================================
+ Coverage 98.91% 98.92% +0.01%
==========================================
Files 113 113
Lines 16718 16708 -10
Branches 3030 3027 -3
==========================================
- Hits 16536 16528 -8
+ Misses 125 124 -1
+ Partials 57 56 -1
|
There was a problem hiding this comment.
The None cases can presumable be addressed with various assert x is not None, at least adding those (or type: ignore) makes it easier to see in the code review where it happens instead of having to cross-reference with the CI output.
I'm not sure how to handle _autojump in a way that's terribly helpful for end-users, as the mechanism by how it's enabled in the runner to start with is kinda weird. But since it requires explicitly setting runner.clock_autojump_treshold from outside the class, you're probably not hitting it unintentionally. Maybe just a try/except with a helpful error message directing them to MockClock, and a type: ignore.
|
Also: turn up the strictness in |
| def start_soon(self, async_fn, *args, name=None): | ||
| def start_soon( | ||
| self, | ||
| # TODO: TypeVarTuple |
There was a problem hiding this comment.
I thought I saw something about mypy being able to understand TypeVarTuple in one of the newer updates unless I am mistaken
There was a problem hiding this comment.
It's got experimental support - definitions work, but when Mypy tries checking function calls it tends to crash with AssertionErrors.
There was a problem hiding this comment.
There was a problem hiding this comment.
It's got experimental support - definitions work, but when Mypy tries checking function calls it tends to crash with
AssertionErrors.
Hmm, does that mean that we could type our code with TypeVarTuple - but as long as we don't enable checking the code with mypy then we're fine, and users on other type checkers can make use of them? (and we probably want to add pyright or something to our CI with only enough checks added to catch incorrect calls to TypeVarTupled functions)
There was a problem hiding this comment.
If so, we should leave it for a different PR that changes it in all files though and can figure out any issues.
There was a problem hiding this comment.
You have to pass the undocumented --enable-incomplete-feature=TypeVarTuple + =Unpack to enable the incomplete code. If not, you get a warning raised for the code, Unpack becomes Any. So maybe we should indeed use them, if we can suppress the error.
|
While typing further, found a few more issues.
|
trio/_core/_generated_run.py
Outdated
| # isort: skip_file | ||
| from __future__ import annotations | ||
|
|
||
| from typing import Any, Callable, Awaitable |
There was a problem hiding this comment.
Use from collections.abc import Awaitable, Callable instead, the typing versions are just aliases
There was a problem hiding this comment.
there's no real reason not to use them either, once we drop py39 (?) support and run pyupgrade they'll all get replaced. I'm weakly in favor of using the non-aliases, but don't think it's worth delaying PR's over it.
| # derive() returns Any for some reason. | ||
| return excgroup.derive(exceptions) # type: ignore[no-any-return] |
There was a problem hiding this comment.
Made an upstream issue! See agronholm/exceptiongroup#76
| return task | ||
|
|
||
| def task_exited(self, task, outcome): | ||
| def task_exited(self, task: Task, outcome: Outcome[Any]) -> None: |
There was a problem hiding this comment.
I .... think the Any might possibly be object-ified.
There was a problem hiding this comment.
Unfortunately no, Outcome can't be invariant because it uses them in both unwrap()'s return value and send()'s parameter.
|
Thanks! I have no further comments but will leave final approval to @A5rocks since they've been more involved in the process thus far. |
|
FTR: I cloned this PR and tried messing around with imports to see if I could resolve https://mypy.readthedocs.io/en/stable/error_code_list.html#check-that-type-of-target-is-known-has-type for |
_io_windows is listed as a TODO in #2734, and not a focus of this PR |
…pl uses TypeVarTuple, 3. we run mypy without -m
This adds types to the
_runmodule (as much as can be done withoutTypeVarTuple). In particular I madeTaskStatusa protocol, not a concrete class - since it's just documented as being a non-specific object with the method. Users shouldn't be depending on a specific type. Though the positional arguments can't be checked, this does makenursery.start(...)produce the right result, check you useTaskStatuscorrectly etc.There's a number of interesting type errors -
Clock._autojump()is called despite that not being defined on the ABC, and a bunch of places where_cancel_statusis accessed without checking if it's None.