Add new ASYNC912 rule to warn on missing checkpoint-or-exceptions

[Zac-HD]

import contextlib, trio

@trio.run
async def livelocks():
    with trio.move_on_after(0.1):  # can't cancel, because sleep() raises before checkpointing
        while True:
            try:
                await trio.sleep("1")
            except TypeError:
                pass
            # or
            with contextlib.suppress(TypeError):
                await trio.sleep("1")

Per https://trio.readthedocs.io/en/stable/reference-core.html#checkpoints, Trio's own functions unconditionally checkpoint unless an exception is raised, in which case they may or may not checkpoint. Glossing the design goals of flake8-async as (1) enforce these conditions in your own code and (2) miscellaneous, to date we've just ignored the possible lack of checkpoints when an exception is raised. In fairness this is a pretty rare pattern; I think it's only bitten me twice at work - though once very badly.

I think the ideal situation here is to add a new ASYNC912 rule, with a corresponding async912-context-managers= config option to expand the denylist of context managers presumed to catch exceptions without re-raising them. I think assuming that any context manager might discard an exception is just too noisy for realistic use, although you could set =* to emulate that if you really wanted to.

How to solve this: add await trio.lowlevel.checkpoint() to the loop body, outside the try/except or the with block. An autofix would be nice eventually, but this is rare enough that I wouldn't bother unless it's really easy.

[jakkdl]

I think the best way of implementing this is to further extend Visitor91X to handle checkpoints in contexts-with-suppressed-exceptions in some special way. Easiest would be to fully ignore them (if some option is set) and then raise 910/911 as normal, but it should be doable to set some special marker such that 912 is raised in those cases instead.

Given that we're piggybacking on 910/911, it should be easy to piggyback on their autofixing as well.

[jakkdl]

Oh wait no, the try/except version is already supported by 910/911:

async def no_checkpoint():
    try:
        await trio.sleep("1")
    except ValueError:
        ...

gives an ASYNC910 warning. So there's two separate feature requests:

1. Add an option for async910/911 for exception-eating context managers. Possibly raising a different code when the only checkpoint is one whose checkpoint is not guaranteed due to user-supplied context manager.
2. Integrate ASYNC100 into Visitor91x, to also make use of e.g.

async def does_not_currently_raise_async100():
    async with trio.move_on_after(0.1):
        while True:
            if ...:
                await trio.lowlevel.checkpoint()

That will likely make ASYNC100 way noisier, so we might make that a separate rule or make the behaviour togglable with an option.

[jakkdl]

How to solve this: add await trio.lowlevel.checkpoint() to the loop body, outside the try/except or the with block. An autofix would be nice eventually, but this is rare enough that I wouldn't bother unless it's really easy.

Current ASYNC910 autofix logic would place the checkpoint outside and after the loop:

# input
async def no_checkpoint():  # ASYNC910: 0, "exit", Statement("function definition", lineno)
    while bar():
        try:
            await trio.sleep("1")
        except ValueError:
            ...
# output
async def no_checkpoint():  # ASYNC910: 0, "exit", Statement("function definition", lineno)
    while bar():
        try:
            await trio.sleep("1")
        except ValueError:
            ...
    await trio.lowlevel.checkpoint()

Which doesn't actually solve the problem 🙃 And there's no check currently that enforces checkpoints-in-loop-bodies (probably not too bad to add?). So while adding an autofix for async100 might be fairly simple, adding a good autofix is less so.

[Zac-HD]

I'm generally cautious about increasing noise levels - false-alarms are the traditional killer of static analysis tools.

Similarly, better IMO to have no autofix than a bad autofix. If in doubt, let's leave this for later.

[jakkdl]

I'm generally cautious about increasing noise levels - false-alarms are the traditional killer of static analysis tools.

It'll be the same logic as other async91x rules, so the same level of false alarms ("way" noisier was perhaps an overstatement). But I can add it as a separate optional rule for now and you can see how noisy it is.

Similarly, better IMO to have no autofix than a bad autofix. If in doubt, let's leave this for later.

👍

[Zac-HD]

yeah, I'm looking forward to (and/or dreading) seeing the results on our code at work...

[jakkdl]

Tasks:

• integrate ASYNC100 into Visitor91x
• Add ASYNC912 - cancelscope with only conditional checkpoints
• Add async912-context-managers for context managers that might suppress exceptions. Though it'll be used by 910, 911 and 100 as well so gonna need a different name.
  - Include contextlib.suppress by default.
  - Support wildcards.
  - A fully global --context-managers=* would probably need additional support to see any real use. Either a decent way of setting per-file settings, or an additional setting to whitelist. But the basic use case is going to be easy to support and can have the extra support as followup in case anybody expresses interest.