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

Implement TRIO105 for anyio (library function must be awaited) #121

Closed
jakkdl opened this issue Feb 2, 2023 · 17 comments
Closed

Implement TRIO105 for anyio (library function must be awaited) #121

jakkdl opened this issue Feb 2, 2023 · 17 comments
Assignees

Comments

@jakkdl
Copy link
Member

jakkdl commented Feb 2, 2023

From #120 (comment)

Some of trio_async_funcs don't exist in anyio, and vice-versa; we should probably make a second list (well, tuple) for anyio and check them separately.

I can probably compile it without too much issue, but let's have a separate issue & PR for it.

@jakkdl
Copy link
Member Author

jakkdl commented Feb 2, 2023

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

@Zac-HD
Copy link
Member

Zac-HD commented Feb 2, 2023

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

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.

@jakkdl
Copy link
Member Author

jakkdl commented Feb 2, 2023

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.

@Zac-HD
Copy link
Member

Zac-HD commented Feb 2, 2023

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.

@jakkdl
Copy link
Member Author

jakkdl commented Feb 2, 2023

Ah yes, I was assuming I'll import over the type-tracking functionality used in 2xx, where types in parameter lists are picked up.

@Zac-HD Zac-HD assigned Zac-HD and jakkdl and unassigned Zac-HD Feb 2, 2023
@jakkdl
Copy link
Member Author

jakkdl commented Feb 3, 2023

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.

@Zac-HD
Copy link
Member

Zac-HD commented Feb 3, 2023

OK, let's just check based on types and the name "tg" then. Thanks for investigating!

@Zac-HD
Copy link
Member

Zac-HD commented Feb 4, 2023

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()
  • ...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

Note also that handle = await anyio.open_process(...) is a separate function, where Trio would use handle = await nursery.start(trio.run_process, ...)

@jakkdl
Copy link
Member Author

jakkdl commented Feb 4, 2023

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

@jakkdl
Copy link
Member Author

jakkdl commented Feb 8, 2023

Went through the anyio API:

awaitable functions

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

awaitable methods

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

deprecated functions

    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

stuff that shouldn't be awaited

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.

functions

    current_time
    current_effective_deadline
    get_current_task
    get_running_tasks
    open_signal_receiver

methods

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

context managers

    fail_after
    move_on_after
    open_cancel_scope # is deprecated

@Zac-HD
Copy link
Member

Zac-HD commented Feb 8, 2023

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.

@jakkdl
Copy link
Member Author

jakkdl commented Feb 9, 2023

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.

@jakkdl
Copy link
Member Author

jakkdl commented Feb 9, 2023

Ah, awaiting non-awaitable functions/methods is picked up by type checkers - which makes it somewhat redundant, though requires strict typing of return values.

@Zac-HD
Copy link
Member

Zac-HD commented Feb 9, 2023

Let's leave this to typecheckers; illegal-await is also an immediate error at runtime and thus easier to debug than missing-await errors.

@jakkdl
Copy link
Member Author

jakkdl commented Feb 9, 2023

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.

@Zac-HD
Copy link
Member

Zac-HD commented Feb 10, 2023

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.

@Zac-HD Zac-HD closed this as completed Feb 10, 2023
@jakkdl
Copy link
Member Author

jakkdl commented Feb 11, 2023

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 😅

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants