Split out from #121

New error: use of deprecated [function/method/etc], or can reuse TRIO117 now that there's a decent way of having multiple messages for the same error.

# deprecated anyio funcs
anyio_deprecated_funcs = (
    "anyio.abc.BlockingPortal.spawn_task",
    "anyio.abc.TaskGroup.spawn",
    "anyio.create_capacity_limiter",
    "anyio.create_condition",
    "anyio.create_event",
    "anyio.create_lock",
    "anyio.create_semaphore",
    "anyio.from_thread.create_blocking_bortal",
    "anyio.open_cancel_scope",
)

Are there any other trio deprecated funcs than multierror?
non-explicit strict_exception_groups=True? (Or maybe that's a separate check for excepts without groups)

I just had a fun instance where I'd accidentally left a breakpoint() in my code when running tox -e check ... which led to flake8 crashing when trying to lint my code - since it's apparently including this plugin in it's checks. I looked at flake8 documentation if there was a way to disable a registered plugin, but somehow didn't see one, and the act of installing the plugin when creating the tox environment registers it in flake8.
--disable=TRIO looks like it just suppresses printing the errors, not disable it from running.

Feels like there should be an easy way of fixing this. But if not I think we can pass a flag parsed in setup.py by setting install_command in tox.ini, or use setenv to set an environment variable that's similarly parsed in setup.py.

anyio is a compatibility layer on top of asyncio or Trio, allowing you to support trio-like structured concurrency using either. In practice, this means that most Trio-supporting libraries have a free way to support asyncio too, and I expect it'll be more generally helpful for the asyncio ecosystem as TaskGroups become more popular over the coming years.

Since almost all of flake8-trio's checks are actually about structured concurrency rather than being truly library-specific, and anyio matches the trio API quite closely, it would be relatively easy to add support for anyio. There are a couple of small divergences, but I think it would mostly be a matter of checking twice as many names.

Background

This is going to have a fair bit of overlap with Zac-HD/flake8-async-archive#6 and Zac-HD/flake8-async-archive#9, precisely because I hoped for a framework-agnostic approach. However, I've ultimately changed my mind because Trio-specific checks can be more specific, and apply to cases where there are no generic warnings or recommendations. Using trio.wrap_file(os.fdopen(..)) is an example of both!

Specific recommendations

I think each cluster of these should probably use a separate TRIO2xx code, so they can be individually suppressed - it's useful for incremental adoption at least. Let's reserve TRIO200 for user-supplied recommendations (see below).

• HTTP requests: #94

  • requests and sync-httpx - as for flake8-async
  • urllib3.request(), but not bare request() (too generic) - use equivalent method with an httpx.AsyncClient
  • urllib.request.urlopen() (stdlib) - likewise, including request.urlopen() and urlopen()
  • httpx.Client() #118

• Process invocations: #94

  • subprocess.Popen and os.popen - await nursery.start(trio.run_process, ...)
  • other subprocess functions, os.system - await trio.run_process(...)
  • os.spawn[vl]p?e? family - usually as for system; if mode= is given and not [os.]P_WAIT then as for popen.

File IO:

• Use trio.open_file() instead of open(), io.open, io.open_code, or os.fdopen in async functions #101
• Use of blocking methods on file objects - wrap in trio.wrap_file() to get an async file object (see below)
• Use of blocking methods on pathlib.Path objects - wrap in trio.Path() to get an async path object (again, see below) #104 #111

Path manipulation:

• Any use of os.path module - prefer using trio.Path objects (it's a copout but good advice IMO) #100
• TBD: os.walk, os.pidfd_open - don't have exact equivalents on trio.Path? 🤔

OS functionality:

• os.wait.*() (except waitstatus_to_exitcode) - must be wrapped in trio.to_thread.run_sync()
• os.pidfd_open - must be wrapped in trio.wrap_file() immediately
• os.remove, os.unlink - prefer trio.Path methods or wrap in trio.to_thread.run_sync()
• ...you know what there's too much to list in the os module and it's rare in async code, let's revisit it later.

I haven't observed antipatterns for use of the socket library in sync code and don't want to speculate, but that's one of several potential future categories here, along with various websocket libraries. Note also that this is large enough that it should probably be split into multiple PRs, and leave anything nontrivial for after we get the basics in.

The problem with Path (and file) objects

• ...

is that they're often passed in as arguments to a function, so all the linter can see is varname.read() - and we can't warn on that without an unacceptable level of false alarms. Of course warning on Path("log.txt").write_text(data) is reasonable, but I think we can do better by checking for arguments which are explicitly annotated as (non-Trio!) Paths and warning on those. This should be left to a future PR, I'm just writing it up here so I can see all the design considerations in one place.

Config option to add simple recommendations

• #80
  It'd be nice if end users could check for blocking uses of nonpublic code, and also nice to try out new warnings without needing to ship an updated version. To that end, let's make each collection of things-to-check extensible where that's easy-ish to do; but to ease maintainence we'll only support configuring the simple patterns for now:

# Warn if we see the thing before ` -> `, and recommend the thing after as a literal string.
# Error out on any entry which doesn't have that delimiter.

trio200-blocking-calls =
    # If you want to support with-or-without library name, list each case.
    # It's verbose but a simple implementation is worth a lot!
    bar -> qux.bar,
    foo.bar -> qux.bar,
    
	# Later: syntax for 'method call' warnings.   (literal `(...)` - empty parens will be an error)
    CustomPath(...).read_text -> AsyncCustomPath(...).read_text

• The (...) method-call syntax should also be used from type annotations once we get that working. Complicated logic like "trio.wrap_file(os.fdopen(...)) is OK" just isn't user-configurable.

async def f():
    r = {}
    r.get("not a sync http client")  # False alarm: raises TRIO210

I've seen similar issues on Queue().put(...), SomeCls.delete(), etc.

Our TRIO113 check is lovely, but as soon as you go beyond Trio's built-in .start()-able functions it gets a lot less magical, because you have to manually configure the functions or methods which take a task_status= argument, and remember to update that every time you add a new one.

So... why not make the linter help with that too? If we see an async function definition fn with an argument named task_status and a default value of trio.TASK_STATUS_IGNORED, and none of fn, *.fn, or self.fn are in the startable_in_context_manager list, emit a warning!

I considered adding this to TRIO113 or making it a new warning. Adding would mean they get atomically disabled together, but conflates semantically-kinda-distinct issues; a separate warning means we're using the same config in two places and might only check one or the other. On net I think expanding 113 is better because it never makes sense to check only one or the other.

(On a meta-level, this is basically a hacky way to get the benefits of inter-module analysis by paying write-time rather than run-time costs; I actually kinda like it as a design pattern. There's a continuum of designs with increasingly-automated caching here...)

Many async web frameworks provide something like Flask's @app.route(...) decorator, where functions have to be async. However this is also a case where it's fine not to have any checkpoints, if you have something like:

@app.route("/heartbeat")
async def heartbeat():
    return "ok"

So I'd like to support a no_checkpoint_warning_decorators option listing the names or dotted-names of decorators which should disable TRIO107 and 108 checks inside those functions, as we currently do for asynccontextmanager.

try:
    ...
except trio.Cancelled:
    raise
except BaseException:
    pass  # this should be OK, because the error _can't_ be a Cancelled.

We should also make the README and the message for BaseException suggest this pattern.

from abc import ABC, abstractmethod


class Handle(ABC):
    @abstractmethod
    async def ajoin(self):
        ...

$ flake8 example.py
example.py:6:5: TRIO107: exit from async function with no guaranteed checkpoint or exception since function definition on line 6

Note that we'll want to handle abstractclassmethod and abstractproperty too. In addition to the explicit decorator-based handling, I'd skip 107 for functions where ... is the complete body.

from contextlib import asynccontextmanager
import trio


@asynccontextmanager
async def in_background(*background_tasks):
    async with trio.open_nursery() as nursery:
        for bgt in background_tasks:
            nursery.start_soon(bgt)
        yield

doesn't emit TRIO101 because we know this is (probably) a context manager, but we don't have the same filter for 108:

$ flake8 example.py 
example.py:6:1: TRIO108: exit from async iterable with no guaranteed checkpoint since yield on line 10
example.py:10:9: TRIO108: yield from async iterable with no guaranteed checkpoint since function definition on line 6

@Zac-HD With #88 merged you need to add https://pre-commit.ci/ to the repository for the checks to actually run. High priority, although I have the hook installed locally so won't push code that doesn't pass the checks.

import trio

async def foo():
    for _ in [1, 2, 3]:
        await trio.sleep(0)

$ flake8 example.py
example.py:3:1: TRIO107: exit from async function with no guaranteed checkpoint or exception since function definition on line 3

We can do better because static lists and tuples (and maybe some other collections?) have an .elts attribute in the AST which allows us to check whether they're nonempty.

Turns out that this is what I actually want in practice, and it would be simpler and more robust to configuration errors.

I'm also pretty comfortable recommending that if you have multiple functions or methods with the same name, where some are startable and others aren't, you should probably rename some of them! [aside: yes, I've recently renamed some functions 😅]

Per #69 (comment) it'd also be good to have the readme suggest using git grep -E 'task_status.*=trio.TASK_STATUS_IGNORED' to fill out the list initially, and suggest that inspecting call sites for await trio.sleep() can be useful in identifying functions which aren't startable but perhaps could be.

One thing I just tried was configuring websocket.* as a known-sync call, in order to suggest https://pypi.org/project/trio-websocket/ over https://pypi.org/project/websocket/

But then it turns out that quite naturally we've used websocket as a variable name for a... websocket... and then we get a lint error on e.g. message = await websocket.get_message(). One option is to go through and rename each such instance to e.g. ws, but another would be to avoid emitting TRIO200 if the call is immediately awaited, because in these cases it's clearly not a blocking sync call after all.

I've noticed a few places where await trio.sleep(0) is added in places that don't actually need a checkpoint, and so I've thought up a two-part plan to mitigate cargo-cult copy-paste coding.

• Instead of await trio.sleep(0), suggest await trio.lowlevel.checkpoint(). It's an identical runtime effect, but the name is much more suggestive of what's actually going on: "we need a checkpoint here for some low-level reason" ✅
• Refactoring the increasingly-large plugin file as suggested below.
• In each async function, for each await trio.lowlevel.checkpoint(), check if there would be no 107/108 warnings emitted if that checkpoint was removed (in addition to any already-suggested-removals, of course).

I initially marked this as an "idea" issue rather than for implementation because while I'm fairly confident (1) is a good plan, (2) seems a little harder to implement. It's also triggering my "this feels pretty tedious" detector, but building a libCST-based autofixer also seems like a little too much duplication of effort, right? Even if we could autofix 105, 107, 108, 112, and 113; and hook it in to shed --refactor... tempting but probably not worth it 😕

Thoughts?

Per the Trio docs, async for and async with are equivalent to await in the sense that they're also checkpoints. The following code should therefore not emit a warning:

with trio.move_on_after(10):
    async for x in ...:
        pass

with trio.move_on_after(10):
    async with ...:
        pass

sync_errors in test_flake8_trio has not been updated in a while, I think that should probably be a list of errors to ignore on sync code rather than the other way so you don't forget to expand the list when adding new errors.

Check that all error messages (TRIO) are mentioned in all four of README.md CHANGELOG.md flake8_trio.py and tests/test_flake8_trio.py.

Check that the line numbers of the set of lines in tests/trio*.py with an # error comment are the same as the line numbers in lines matching make_error[...] <lineno> [...] in tests/test_flake8_trio.py. As lines are moved around and specs change I get increasingly worried that these don't match.

• Get good tests, following the example of flake8-bugbear
• Add typechecking with pyright in strict mode (running via tox)
• [Zac] set up PyPI token and auto-release system

$ tox -av
using tox.ini: /tmp/flake8-trio/tox.ini (pid 252136)
using tox-3.25.0 from /home/foobar/.local/lib/python3.10/site-packages/tox/__init__.py (pid 252136)
default environments:
py39-flake8_5  -> Runs pytest, optionally with posargs
py39-flake8_6  -> Runs pytest, optionally with posargs
py310-flake8_5 -> Runs pytest, optionally with posargs
py310-flake8_6 -> Runs pytest, optionally with posargs
py311-flake8_5 -> Runs pytest, optionally with posargs
py311-flake8_6 -> Runs pytest, optionally with posargs

In https://github.com/Zac-HD/flake8-trio/blob/main/CONTRIBUTING.md

tox -e check
tox -e py311

Having written all the visitors, is the plugin being slow enough on large files it's worth trying to optimize it? Merging all the visitors might get somewhat messy, but could lead to a large speedup. Otherwise I'm not aware of any big things I could do to speed it up, but there's probably a few spots. (the recently added lists/stacks that get copied for resetting at exit can likely instead just pop the element they pushed (if they pushed one)

Now that we have added optional checks, I was struck by the thought that maybe 107&108 should be made optional as well? I remember it being singled out when you'd previously discussed it with other Trio folks, and as far as I understand they're not really errors - and it's highly viable to write a codebase where you only take as assumption that calls to the trio library guarantees checkpoints - but not when calling your own async functions.
When transitioning a codebase to use this plugin I also imagine that they're going to be among the noisiest checks, and you might only want to enable them after fixing all the other checks.

Copied out from #81

• Get rid of visitors manually visiting subnodes irrelevant with libcst

  • some possible solutions, either these two
    • exit_Xxx which is called for a node after it's subnodes have been visited (would clean up all cases of self.generic_visit(node) being used)
    • visit functions for node parts that in the AST don't have dedicated nodes, e.g. visit_Finally or visit_WhileElse. This would split up several long functions and solve the last cases of revisits - only trouble child would be 107_108.visit_loop, which explicitly wants to visit the body twice atm, but I could probably rewrite that
  • Or visitor classes implement a visit_Xxx as a pseudo-contextmanager, with enter and exit as magic names, and other names called as they match up with the _fields of the visited node. And the external runner handles a visit different depending on if it's isinstance function or class

• #103 I'm also starting to want to split up flake8_trio.py into different files, 1500 lines is starting to be quite inconvenient to navigate, and get any overview on - I also know you are hesitant to split given installation complications so would have to take care when doing that.

• #91

• don't hard code unnecessary variables in Flake8TrioVisitor.get_state - it would most likely just mean renaming a lot of variables. But is bound to introduce bugs in it's current state so plan to fix it soon after merge.

• #102 Move all trioxxx.py files into a subdirectory of tests/ so it's clear what files contains tests and which ones contain test data. Can then also get rid of regex and naming restrictions on files.

Quickly ran the plugin against python-trio: (although this includes tests and stuff, so a bunch may come from there)

$ for i in {100..112}; do echo -n $i": "; flake8 --select=TRIO$i | wc -l; done
100: 0
101: 0
102: 18
103: 11
104: 5
105: 0
106: 6
107: 276
108: 42
109: 1
110: 1
111: 0
112: 0

A huge number of 107, I suspect a lot of them are false positives that could mayhaps get filtered out. E.g. currently 107 doesn't check if an async function is used in a trio context at all - could maybe at least check if trio is imported in the file. Or a more sophisticated check would be if the function is referenced in a function that's in a trio context ... building up a huge tree up to trio.run(). Feels possible complicated or error-prone, but maybe worth it?

Might very well be redundant once the code is restructured, but otherwise this might make a lot of sense. Might plausibly also be something for flake8_bugbear or other linter plugins based on ast. (or straight up added as an [optional] check to flake8_bugbear maybe)

Realized I'd forgotten it in current PR I'm writing, and checking #69 it had been completely forgotten there too. There's some cases where it doesn't matter (node types that can't have child nodes) - but that's certainly not the case with AsyncFunctionDef as per TRIO114

def sync():
    while ...:
        if ...:
            return
        yield

Running flake8-trio on this example crashed with AttributeError: 'Visitor107_108' object has no attribute 'safe_decorator'. I fixed that in d484205, but there are still missing-checkpoint warnings - which is a false alarm for a sync function!

As well as fixing this let's add a test that for each test file, if you remove each "async " and "await " keyword and then run the linter, there are no TRIO warnings (except TRIO105 missing-await warnings).

After working more on TRIO232 I kinda wanna look into hooking in[to] a type parser, that would not only make TRIO2XX object handling much simpler and more powerful but also improve a bunch of other checks: detection of nurseries (TRIO101, TRIO112, TRIO113), handling of cancel scopes (TRIO102), better support for imports, etc.

Looking around I'm not seeing a lot of good support for it though, mypy has a plugin system - but that's primarily intended to improve/extend the type inference, although pydantic has a mypy plugin https://docs.pydantic.dev/mypy_plugin/ that adds errors on static analysis - so could maybe do something similar to them and add checks onto a bunch of hooks, moving them out of logic in the flake8 plugin. (did not find any mentions of plugins for pyright, pyre, or pytype)

Another alternative is to run a type checker on startup of flake8-trio, and then presumably the type data is saved somewhere, and it can be inspected, so when traversing the AST we can delve into that data structure and get out the typing data we care for.

This is kind of a meaty addition though, but I kinda feel like I'm reinventing the wheel with subpar tools when I'm writing logic to handle assignments, annotations, and everything. So it might be worth spending some time experimenting with possible alternatives and see if it's not too crazy.

(I also started reinventing libCST's match functionality the other day when I got frustrated with the endless chains of isinstance...)

I don't think we're actually gaining all that much from being a plugin to flake8, as opposed to a standalone checker, so I don't think the limitations from that should weigh too heavily.

I think we can just assume that almost all nurseries are named nursery and also that ~everything named nursery is in fact a nursery, at which point this is a pretty simple addition change:

https://github.com/Zac-HD/flake8-trio/blob/a900cffc2e47c36a9d0576cd384eb20ca33289b4/flake8_trio.py#L820-L821

and (
    (node.func.value.id == "trio" and node.func.attr in trio_async_functions)
    or (node.func.value.id == "nursery" and node.func.attr == "start")
)

plus tests etc.

As a "list of ideas" issue, any of the ideas below might turn out to be too difficult to implement, or in fact counterproductive if implemented. Nonetheless, we need somewhere to dump ideas and this issue is that place. Items are in rough priority order by a combination of estimated importance and tractability.

• Never yield inside a nursery or cancel scope
  #4
• Anything that awaits inside a finally: clause needs shielding and a timeout, e.g. with trio.fail_after(1, shield=True): (source) (also applies to async for or async with)
  #5
• Never have an except Cancelled or except BaseException block with a code path that doesn't re-raise the error.
  #8
• Calling a Trio async function without immediately awaiting it. Per the discussion here typecheckers have some support for the (difficult!) general case, but it's easy and useful to make a list of all Trio functions and check for e.g. trio.sleep(0) without an await. We don't need to check async for or async with because using the sync syntax is a noisy error at runtime, unlike the checkpoint warnings.
  #10
• Importing things from the main trio namespace in a way that isn't just import trio (this is really just to make the linter easier to implement, but it's consistent with ~99% of existing usage anyway)
  #11
• Async functions must have at least one checkpoint (i.e. await, async for, or async with) on every code path, unless an exception is raised (the same rule that Trio internals follow)
  #16
• Async iterables (i.e. with a yield) should have at least one checkpoint in each loop iteration, and an additional checkpoint after the last iteration (unless an exception is raised, again like the Trio internals)
  #17
• Expanding TRIO102 - you need a shielded cancel scope to await inside an except Cancelled/BaseException block, not just finally:
  #18
• Async function definitions should not have a timeout parameter - encourage use of trio.fail_after() or trio.move_on_after() instead. (httpx does OK here, but most uses are IMO a design mistake)
  #20
• Prefer waiting on a trio.Event rather than sleeping in a loop. Concretely I think matching while not <expr>: await trio.sleep(...) might work well here?
  #21
• When nesting async context managers, nurseries should be outermost innermost. (actually pretty complicated?)
  #22
• Warn if we see a nursery body that contains exactly one statement, and that statement is a call to nursery.start[_soon] ("consider using a regular function call instead"), unless nursery is passed as an argument to the start-soon'd function.
  #34

Trio's MultiError is deprecated in favor of the builtin (or backport of) ExceptionGroup, so downstream code should be written or ported to the latter. That includes catching, raising, and use of methods - so I think just linting for any reference to trio.MultiError would be sufficient and easy to implement.

This lint is important because MultiError is still raised from Trio, in order to give downstream time to migrate, and so users quite naturally use the name mentioned in their tracebacks.

Matters w/r/t TRIO102, 103 and 104.

flake8-trio currently always just looks for trio.Cancelled, but
#120 (comment)

Unfortunately anyio has a slightly different API here; see anyio.get_cancelled_exc_class(). That's important because you can have both trio and asyncio in the same program (ugh, but sometimes the least-bad option!).

Also add another error code for assigning the result of anyio.get_cancelled_exc_class() to a variable. That can be caught if within the same file with our rudimentary type tracking, but not if done across files and imported. So the plugin should likely track assignments for cases where single-backend is used and the error code gets disabled.

We probably also want to lint for "no assigning that result because it breaks our linter, as well as multi-backend programs".

Quick GitHub search gives 850 results for from anyio import ... and 230 results for trio. So seems relatively prevalent?

With the addition of utility visitors / type tracking, it's not technically difficult to add support for different imports and make TRIO106 a relic of the past. Just gotta add a visit function for Import / ImportFrom, save all aliases in a dictionary, and then e.g. trio105 can make a lookup in the dict to see if the thing it's analyzing has been aliased.

It's going to require rewriting a bunch of checks to use it though, who currently have hard-coded values tested against ast.unparse or a stack of isinstance checks. So if implemented before libCST it would probably be a bunch of duplicated effort - so if you think it's a good idea I would vote to delay it for now.

As discussed here, async generators make it very difficult to write cancellation-safe code, which is essential when you want to use timeouts (always) or directly cancel some tasks (not that rare either). I therefore think it's worth having a disabled-by-default check, TRIO900, which bans all async generators unless they have an @asynccontextmanager decorator.

There's a bunch of different ways of handling spaces in comma_separated_list to fix --trio200-blocking-calls. Pending on what happens with PyCQA/flake8#1771

• handle ['a', '->', 'b'] as well as ['a->b'] with fancy logic
• don't use comma_separated_list and write a custom action (more attractive if PyCQA/flake8#1770 is fixed)
• Require users to wrap values in quotes or something. Though some quick testing looks like this doesn't help.
• Wait for it to be fixed upstream

One pattern I've seen a few times lately is a context manager which ensures some external service, e.g. an external process to interact with, is available. The code might look something like this:

@asynccontextmanager
def run_sampling_profiler():
    async with trio.open_nursery() as nursery:
        nursery.start_soon(trio.run_process, ["start-sampling-profiler", str(os.getpid())])
        yield

The only problem is that the code in the calling with-block can start running before our external process is actually running! To fix this, we should have used await nursery.start(trio.run_process, ...).

So to lint for this, we should check nursery.start_soon() calls either inside async def __aenter__(self): or before the yield statement in a function like the above, and raise a warning if the target callable could instead be used with nursery.start(). But how do we distinguish those startable targets?

• They accept a task_status= keyword argument... but doing cross-module introspection would be a pretty nasty latency hit
• We can list out known cases - search task_status= on this page, and I'll be happy to expand the list if there are other suggestions
• We can support a configuration option to add more on a project-specific basis

async def f():
    while True:
        if ...:
            await trio.sleep(0)
            return
        # If you delete this loop, no warning.
        while foo():
            await bar()

$ flake8 example.py
example.py:1:1: TRIO107: exit from async function with no guaranteed checkpoint
                         or exception since function definition on line 1

but plainly deleting the inner loop doesn't change whether the function checkpoints before exiting.

# These examples are probably not meant to ever wake up:
await trio.sleep(100000)
await trio.sleep(10**10)

# and these ones _definitely_ never wake up:
await trio.sleep(float("inf"))
await trio.sleep(math.inf)
await trio.sleep(1e999)  # 'inf literal' overflow trick

This is a pretty simple check, but using await trio.sleep_forever() instead does have a certain elegance - and the first example (real code) might actually wake up if you leave the process running for ~27 hours!

Turns out that many existing cases in our codebase are on some RPC method, test function, or similar, where a magic decorator causes code to run... elsewhere. So just skipping this check for decorated functions would avoid a lot of false alarms 🙂

I get an error on line 1353 because it hasn't been parsed from list-of-strings into a dict:

https://github.com/Zac-HD/flake8-trio/blob/7cdd9180e3aefca89d1a58d197022f266b45868c/flake8_trio.py#L1350-L1356

I also started on a quick regression test, but don't have time to finish it right now: https://github.com/Zac-HD/flake8-trio/compare/config-error

def foo():
  try:
    raise trio.Cancelled()
  finally:
    return

will silently eat the error without re-raising. I'm not sure how often people do this, but it seems crazy bad to ever return/yield in finally. Should be a pretty simple addition.

edit: or if another exception is raised inside the finally

Reproducing example:

from contextlib import asynccontextmanager
import trio

def cleanup(): pass
async def acleanup(): pass

@asynccontextmanager  # Comment out this line to get the expected error!
async def sample_with_proxy():
    try:
        await trio.sleep(0)
        yield
    finally:
        # Uh oh!  This should trigger the lint warning TRIO-102,
        # "await inside try/finally must have shielded cancel scope with a timeout"
        await acleanup()
        cleanup()

I this might actually be a more general issue, where the concept of a _safe_decorator should only apply to the checkpoint warnings (107 and 108), but when refactoring it's been preserved for other things as well. CC @jakkdl for this one?

in 490a775 you converted all files to CRLF line endings. Was this intentional?

It's not a huge problem for me, but it makes diffs on my local machine have ugly ^M's and apparently the remotes are stored with LF endings so whenever I try to diff or rebase against a remote branch it's a mess. So unless you don't mind I'd like to convert all files back to LF and add a .gitattributes file to hopefully stop it from happening in the future. But I'll wait with it until there's no open PR's to avoid merging nightmares.
If it was intentional I'll add a .gitattributes file indicating CRLF is preferred, and see if I can get git to play nicely with that on my local machine.

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.

async def to_queue(iter_func, queue):
    async with iter_func() as it:
        async for x in it:
            queue.put(x)


async def to_queue2(iter_func, queue):
    try:
        async with iter_func() as it:
            async for x in it:
                queue.put(x)
    finally:
        queue.put(None)

$ flake8 example.py
example.py:7:1: TRIO107: exit from async function with no guaranteed checkpoint
                         or exception since function definition on line 7

but if the first case is fine, the second should be fine too.

Moving from #70

what does the UX look like if we're not leaning on flake8 for that? I presume similar, so how much work is that to add?

Quickly looked at whether the flake8 interface could still work - and it's possible to still use flake8 without much problem. You can indicate that you want the lines of a file and then that can be dumped into libcst.parse_module(). Flake8 does require plugins to accept one of tree, logical_line or physical_line as parameter in __init__ for it to classify the plugin and run it at the correct time, so we'd have to still accept the tree parameter - but we can just throw it out and construct our own tree from the lines sent at the same time.

Somewhat hackish, but means that the consideration of ditching flake8 is pretty much fully independent.

This even means that the transition can even be gradual - where in the intermediate phase you have both an AST and a libCST tree, and run separate runners for them.

TODO

Standalone functionality

1. Color support - I was most of the way to replicating flake8's color printing, but once it got into testing/coverage and windows support I abandoned it and threw it out. For a later PR, if ever.

• Disable visitors with other ways than --enable-visitor-codes-regex #163
• pre-commit support #161
• #189
• Add parallell processing of files #124 (comment)
• test that autofixed files don't generate errors #159
• #193
  • test
• #192
• #191
• Write documentation

Converted to libCST:

• TRIO100 #142
• TRIO101 #142
• TRIO102
• TRIO103
• TRIO104
• TRIO105
• TRIO106
• TRIO109
• TRIO110
• TRIO111
• TRIO112
• TRIO113
• TRIO114
• TRIO115
• TRIO116
• TRIO117
• TRIO118
• TRIO200
• TRIO210
• TRIO211
• TRIO212
• TRIO220
• TRIO221
• TRIO222
• TRIO230
• TRIO231
• TRIO232
• TRIO240
• TRIO900
• TRIO910 #143
• TRIO911 #143

Autofixing

• TRIO100: #149
  • don't autofix if there's other warnings about the content of the context. (?)
• TRIO102: it's unsafe to await inside finally: or except BaseException/trio.Cancelled unless you use a shielded
  cancel scope with a timeout.
  • I think a context manager can be added around it?
• TRIO105: Calling a trio async function without immediately awaiting it.
• TRIO112: nursery body with only a call to nursery.start[_soon] and not passing itself as a parameter can be replaced with a regular async function call.
• TRIO113: using nursery.start_soon in __aenter__ doesn't wait for the task to begin. Consider replacing with nursery.start.
• TRIO115: Replace trio.sleep(0) with the more suggestive trio.lowlevel.checkpoint().
• TRIO117: Don't raise or catch trio.[NonBase]MultiError, prefer [exceptiongroup.][Base]ExceptionGroup. Even if Trio still raises MultiError for legacy code, it can be caught with BaseExceptionGroup so it's fully redundant.

user-configured

• TRIO200: User-configured error for blocking sync calls in async functions. Does nothing by default, see trio200-blocking-calls for how to configure it.
  • Could probably add some fancy config flag that will replace stuff. Although the possibly ways of morphing the call might be too complicated to support more than the most basic stuff, will find that out as other 2xx autofixers are written / researched.

I think many of these can be fixed, but needs a long list of what to replace with

• TRIO210: Sync HTTP call in async function, use httpx.AsyncClient.
• TRIO211: Likely sync HTTP call in async function, use httpx.AsyncClient. Looks for urllib3 method calls on pool objects, but only matching on the method signature and not the object.
• TRIO212: Blocking sync HTTP call on httpx object, use httpx.AsyncClient.
• TRIO220: Sync process call in async function, use await nursery.start(trio.run_process, ...).
• TRIO221: Sync process call in async function, use await trio.run_process(...).
• TRIO222: Sync os.* call in async function, wrap in await trio.to_thread.run_sync().
• TRIO230: Sync IO call in async function, use trio.open_file(...).
• TRIO231: Sync IO call in async function, use trio.wrap_file(...).
• TRIO232: Blocking sync call on file object, wrap the file object in trio.wrap_file() to get an async file object.
• TRIO240: Avoid using os.path in async functions, prefer using trio.Path objects.

Requires a bunch of logic

I think I can chuck in a bunch of trio.lowlevel.checkpoint() just before the line that would raise these - but there's going to be a bunch of edge cases that are tricky.
higher priority

• TRIO910: #158
• TRIO911: #158

def __aenter__(self):
    self._nursery.start_soon(trio.run_process, cmd)                       # already emits TRIO113
    self._nursery.start_soon(partial(trio.run_process, cmd, shell=True))  # we want this to emit TRIO113

Looking through the code, we actually already handle the partial() idiom everywhere else. It'd be good to add a regression test for TRIO112 with partial() though to make sure that keeps working. (first proposed in #60 (comment))

Reading the readme, it says

TRIO103: except BaseException and except trio.Cancelled with a code path that doesn't re-raise. Note that any raise statements in loops are ignored since it's tricky to parse loop flow with break, continue and/or the zero-iteration case.

But having implemented that logic with 107/108 I could extend that to 103 as well, likely without too much trouble.

Should we update setup.py to require flake8>=6.0, or modify tests so they can run on both <6.0 and >=6.0 - so we can make sure that code doesn't break on <6.0? Should be unlikely, but given that this one completely snuck up on me it's probably worth taking into account.