Re-code flake8-trio and flake8-async rules to match upstream

[augustelalande]

Summary

Re-code flake8-trio and flake8-async rules to match upstream. Resolves #10303.

Test Plan

[augustelalande]

This is mostly done, except for the previous ASYNC101 and ASYNC102 which don't exactly match any the of the new ASYNC rules. The closest matches are ASYNC220 and ASYNC222, but the behaviors don't match exactly. Part of the problem is that the previous rules should be split into multiple rules (this is easy). The other problem which I'm not sure how to resolve is that it doesn't seem like any of the new ASYNC rules check for time.sleep or ~open~ (nevermind ASYNC230 covers open), but this is a useful check that we should keep, so the question is what should I do?

[github-actions[bot]]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

[augustelalande]

Ok so I modified the implementation of ASYNC220 and ASYNC222, as well as added ASYNC221 so that the three rules would match the behavior of flake8-async. For now I added a check for time.sleep as part of ASYNC222.

[jakkdl]

Oh, I'd missed that we don't check for time.sleep anymore. It's a bad match for ASYNC222, as the recommended replacement is using [trio/anyio/asyncio].sleep(), and not to wrap it in to_thread_run_sync or run_in_executor. I'll implement it quickly as ASYNC251

[augustelalande]

Ok I moved the time.sleep check to a newly created ASYNC251

[Zac-HD]

We've just released ASYNC251 upstream, so this is back to matching the merged flake8-async plugin.

[zanieb]

Note that we'll need to add "redirects" for all of these rules, see rule_redirects.rs. See https://github.com/astral-sh/ruff/pull/9756 for example.

We may be able to make use of https://github.com/astral-sh/ruff/pull/9691 as well, not sure yet.

[augustelalande]

Ok I added redirects for the old TRIO rules. But as mentioned the old ASYNC rules don't directly correspond to new ASYNC rules.

[jakkdl]

I've got some comments on the human-friendly rule names you have, and it might be good if we synced them to ease transition between flake8-async and ruff. See https://github.com/python-trio/flake8-async/pull/248#issuecomment-2117984198

(I'm sorry we're moving so quickly as of late :grin:)

[JelleZijlstra]

This PR has some merge conflicts now, anything I can do to help?

I'm motivated to add (the new) ASYNC101 to Ruff because of the problems mentioned in the draft PEP 789.

[charliermarsh]

The main issue is that it's going to be a breaking change for users (e.g., ASYNC100 continues to exist but is now a different rule). We may be able to include it in v0.5.0 though which will be ~soon.

[charliermarsh]

@zanieb - let's include this in v0.5.0 since it's starting to cause problems.

[charliermarsh]

@augustelalande -- I'm happy to take responsibility for resolving any conflicts.

[augustelalande]

@charliermarsh I'll do it now. I'm also gonna update the rules with the new names from https://github.com/python-trio/flake8-async/pull/248#issuecomment-2117984198

[augustelalande]

Nevermind the last part. Updating all the rule names is a significant effort so should be a separate PR.

[MichaReiser]

Okay, I'm done with the rebase and I very much hope I didn't mess anything up. I now need to get a better understanding of the change itself to be able to review it :)

[MichaReiser]

I'm trying to get my head around over all the changes and are having a hard time. What's unclear to me is:

• What's the exact mapping from old rules to new rules
• What I understand is that our rules don't precisely match flake8-async. Which rules are different and to what extend?

[MichaReiser]

From the chanegelog: Added asyncio support for several error codes. I think that' the case for most of the TRIO rules. This isn't something we have to change as part of this PR but important for the changelog

[MichaReiser]

Okay, I think I now have a good overview of what has changed. I plan on reviewing this PR tomorrow (and hopefully merge).

I don't think there's anything we can do about mapping the old ASYNC rules.

Trio to ASYNC

• TRIO100 -> ASYNC100 :
  • No support for anyio or asyncio
  • Ruff: TrioTimeoutWithoutAwait
  • Flake8: cancel-scope-no-checkpoint
• TRIO105 -> ASYNC105
  • Ruff: TrioSyncCall
  • Flake8: missing-await
  • Both only support trio
• TRIO109 -> ASYNC109
  • I think the flake8 rule also supports anyio and asyncio.
  • Ruff: TrioAsyncFunctionWithTimeout
  • Flake8: async-function-with-timeout
• TRIO110 -> ASYNC110
  • Ruff: TrioUnneededSleep
  • Flake8: async-busy-wait
  • Missing anyio support
• TRIO115 -> ASYNC115
  • Ruff: TrioZeroSleepCall
  • Flake8: async-zero-sleep
  • Missing anyio support
• TRIO116 -> ASYNC116
  • Ruff: SleepForeverCall
  • Flake8: long-sleep-not-forever
  • Missing anyio support

ASYNC rule changes

• ASYNC100 -> ASYNC210:
  • Ruff: BlockingHttpCallInAsyncFunction
  • Flake8: blocking-http-call
  • Looks about right
• ASYNC101 ->
  • Used to test for
    • open -> ASYNC230
    • time.sleep -> ASYNC251
    • subprocess:
      • subprocess.run, -> ASYNC221
      • subprocess.Popen -> ASYNC220
      • subprocess.call -> ASYNC221
      • subprocess.check_call -> ASYNC221
      • subprocess.check_output -> ASYNC221
      • subprocess.getoutput, -> ASYNC221
      • subprocess.getstatusoutput -> ASYNC221
    • os
      • wait -> ASYNC222
      • wait3 -> ASYNC222
      • wait4 -> ASYNC222
      • waitid -> ASYNC222
      • waitpid -> ASYNC222
  • ASYNC220:
    • Also tests for os.popen, os.spawn (if not p_wait)
    • flake8: blocking-create-subprocess
    • Ruff: CreateSubprocessInAsyncFunction
  • ASYNC221:
    • Also tests for os.system, os.posix_spawn, os.posix_spawnp
    • flake8: blocking-run-process
    • Ruff: RunProcessInAsyncFunction
    • Sometimes os.spawn (when not ASYNC220)
  • ASYNC222:
    • Ruff: WaitForProcessInAsyncFunction
    • Flake8: blocking-process-wait
  • ASYNC230:
    • Ruff: BlockingOpenCallInAsyncFunction
    • flake8: blocking-open-call
  • ASYNC251:
    • Ruff: BlockingSleepInAsyncFunction
    • flake8: blocking-sleep

[jakkdl]

I'm not sure with "no support for x" you refer to upstream, or this PR. But in case there's any confusion about upstream support:

• ASYNC100 supports all of trio/anyio/asyncio
• ASYNC105 is trio-only. Since type checkers started catching this the rule is mostly redundant, which is the only reason we haven't cared to extend it for anyio/asyncio.
• ASYNC109 supports all of trio/anyio/asyncio
• ASYNC110 has trio&anyio support. asyncio support is TODO, but it's a trivial fix so you could probably go ahead and support it regardless of if/when I get around to it upstream
• ASYNC115&ASYNC116 has trio&anyio support. asyncio does not have equivalent constructs

[MichaReiser]

@jakkdl Thanks for detailing the upstream features. I mainly tried to document with no support what features Ruff lacks compared to the upstream rules. I don't think we should or have to address these shortcomings in this PR but I want to be aware of it when reviewing (and we may even want to document it)

[augustelalande]

Good summary from @MichaReiser and @jakkdl

There is still a significant amount of work to bring feature parity between ruff and flake8-async, but IMO the first step has to be resolving the merge between flake8-async and flake8-trio (i.e. this PR), everything else should follow in future PRs.

[augustelalande]

@jakkdl Thanks for detailing the upstream features. I mainly tried to document with no support what features Ruff lacks compared to the upstream rules. I don't think we should or have to address these shortcomings in this PR but I want to be aware of it when reviewing (and we may even want to document it)

Not sure if it needs to be documented in the code base, or if simply opening an issue would be sufficient.

FYI flake8-trio is already being tracked in #8451

[MichaReiser]

I manually tested the redirection rules and it all looks good. Let's merge this.

[MichaReiser]

I think one nice follow up after the release would be to rename the rules to match the upstream names.