Closed jakkdl closed 1 year ago
nursery calls have also been implemented in this since #56, that should probably be rewritten to use the new type-tracking functionality (Do we also want name-based tracking on top of that?), and anyio's version of that is Task Groups and https://anyio.readthedocs.io/en/stable/api.html#anyio.create_task_group
nursery calls have also been implemented in this since https://github.com/Zac-HD/flake8-trio/issues/56, that should probably be rewritten to use the new type-tracking functionality (Do we also want name-based tracking on top of that?)
Skimming through that issue, we only used name-based tracking at all because we thought type-based tracking wasn't worth the trouble. Since we have the latter now let's add that; but keep name-based checks for nursery
because there are a few patterns where you pass that around as an argument.
For TaskGroup
, it looks like the conventional names are covered by tg|task_?group
.
because there are a few patterns where you pass that around as an argument.
This only matters if they're not typing the parameter list, if they do it's covered.
While giving false positives on non-nursery objects named tg
isn't too bad since it's an awful name, it almost surely will give false positives.
Hmm, we currently only match .*nursery\.start
; I think (tg|.*(nursery|task_?group)\.start
wouldn't have too many false alarms? Important that we're looking at a method call not just the variable name though.
Ah yes, I was assuming I'll import over the type-tracking functionality used in 2xx, where types in parameter lists are picked up.
Searching github, I get >7k code results for "task_group", and 18k for "taskgroup" in python code - and skimming the first few pages I find that e.g. Apache AirFlow has something called task groups, and bunch of random people implementing whatever they call task groups. Searching for task_group.start()
wasn't very fruitful though as github doesn't care for punctuation, so I get a lot of hits for task_group_start and stuff.
So it's definitely a relatively common name for Stuff:tm:, so it ends up becoming a tradeoff between false positives vs checking untyped code where task groups are sent around as parameters or other complicated stuff.
OK, let's just check based on types and the name "tg" then. Thanks for investigating!
Related documentation things: we should have a tiny section listing deprecated anyio
stuff that we don't lint for, including
anyio.open_cancel_scope()
- let's also leave a comment on the cancel_scope_names
constant that it's deliberately omitted anyio.abc.TaskGroup.spawn()
, which is deprecated in favor or .start_soon()
Note also that handle = await anyio.open_process(...)
is a separate function, where Trio would use handle = await nursery.start(trio.run_process, ...)
...actually there are nine of these, maybe we should have a lint rule for use of deprecated functionality? ctrl-f "Deprecated since" on this page
We do have TRIO117 for the deprecated MultiError, but that's a special case so a generic error code for deprecated library functions sounds good.
Went through the anyio API:
I note that this is more comprehensive than we have for trio, where no submodule functions are checked.
aclose_forcefully
connect_tcp
connect_unix
create_connected_udp_socket
create_tcp_listener
create_udp_socket
create_unix_listener
getaddrinfo
maybe_async
open_file
open_process
run_process
run_sync_in_worker_thread
sleep
sleep_forever
sleep_until
wait_all_tasks_blocked
to_thread.run_sync
to_thread.run_sync_in_worker_thread
to_process.checkpoint_if_cancelled
to_process.open_process
to_process.run_sync
lowlevel.cancel_shielded_checkpoint
lowlevel.checkpoint
lowlevel.checkpoint_if_cancelled
Are just way too many and messy to list manually, so would either runtime generate these - or write some fancy thing to generate a hard-coded list. Or a lot of them aren't useful, and relevant ones can be hand-picked. But checking for them would be somewhat doable with the type-tracking, and not just do taskgroup.start
anyio.abc.TaskGroup.spawn
anyio.abc.BlockingPortal.sleep_until_stopped
anyio.abc.BlockingPortal.stop
anyio.AsyncFile.aclose
anyio.abc.UnreliableObjectReceiveStream.receive
anyio.abc.UnreliableObjectSendStream.send
anyio.abc.ObjectStream.send_eof
anyio.abc.ByteReceiveStream.receive
anyio.abc.ByteSendStream.send
anyio.abc.ByteStream.send_eof
anyio.abc.Listener.serve
# and many many more
abc.BlockingPortal.spawn_task
abc.TaskGroup.spawn
create_capacity_limiter
create_condition
create_event
create_lock
create_semaphore
from_thread.create_blocking_bortal
open_cancel_scope
https://github.com/agronholm/anyio/blob/5d9a476d6e3e6b6d7b1876fa18e70cd910d2e7e7/docs/migration.rst With AnyIO 3 they made a bunch of previously async stuff sync, some most of which return resources that can be awaited - but will give deprecationwarning.
The transition from AnyIO 2 to 3 seems to have been ~1.5 years ago, but shouldn't be much effort to also check these.
current_time
current_effective_deadline
get_current_task
get_running_tasks
open_signal_receiver
anyio.abc.CancelScope.cancel
CapacityLimiter.acquire_nowait
CapacityLimiter.acquire_on_behalf_of_nowait
Condition.release
Event.set
Lock.release
Semaphore.release
MemoryObjectReceiveStream.receive_nowait # <.streams.memory.MemoryObjectReceiveStream.receive_nowait>`
MemoryObjectSendStream.send_nowait # <.streams.memory.MemoryObjectSendStream.send_nowait>`
fail_after
move_on_after
open_cancel_scope # is deprecated
I'm inclined to leave out awaitable methods (except tg.start) as not high enough priority to be worth the trouble at the moment, and leave deprecated don't-await-this to the runtime system since they'll go away at some point without us.
Sounds good, I'll leave those out for the time being and can consider revisiting them later. Any code for awaitable methods would of course be reusable for Trio methods, and don't-await-this could also be extended to check all non-async functions in trio/anyio.
I'm a bit vary of matching the variable name of tg
as well - I find at least two instances of tg.start()
where tg is short for Telegram, but mostly because I think most/all instances will be caught by type tracking on anyio.create_task_group()
. And I think the plugin could also be extended to handle stuff like from anyio import create_task_group
without much effort in future PRs thanks to the shared state/variable stuff. So I think the only additional thing it will catch is a function taking a task group as a parameter, untyped, named tg
.
Ah, awaiting non-awaitable functions/methods is picked up by type checkers - which makes it somewhat redundant, though requires strict typing of return values.
Let's leave this to typecheckers; illegal-await is also an immediate error at runtime and thus easier to debug than missing-await errors.
Ooh, mypy 1.0.0 does give me
tests/eval_files/trio113.py:12: error: Value of type "Coroutine[Any, Any, object]" must be used [unused-coroutine] tests/eval_files/trio113.py:12: note: Are you missing an await?
now when I write
async with anyio.create_task_group() as bar:
bar.start(my_callable) # TRIO113: 8, anyio.TaskGroup.start
And it catches all the awaitable library functions as well ... so there's maybe a decent case for skipping TRIO105 for anyio and just tell people to use mypy? (pyright does not give the warning, did not check other ones)
I tried installing trio.typing and enabling it's plugin to see if it would catch the trio one as well - but it didn't. So should maybe spend some time figuring out what part of trio's magic breaks that check.
Yeah, let's stick with the status quo then.
Low priority, I'd like to work out why pyright
doesn't do this and get mypy
working for Trio, but that's definitely lower-priority than e.g. LibCST.
Coolies. I will push a PR with some modifications to type tracking and nurseries I implemented before I realized this, but otherwise revert most modifications.
Oh nvm, pyright does have support for it - but since eval_files are in the exclude list it didn't say anything even when explicitly passed the file :sweat_smile:
So this falls under "static typing improvements for Trio" in the TODO, I might do some more testing/looking around and then maybe open an issue / comment on some existing typing issue in trio.
From https://github.com/Zac-HD/flake8-trio/pull/120#discussion_r1094980781
I can probably compile it without too much issue, but let's have a separate issue & PR for it.