Detect unneeded `async` keywords on functions

[plredmond]

Summary

This change adds a rule to detect functions declared async but lacking any of await, async with, or async for. This resolves #9951.

Test Plan

This change was tested by following https://docs.astral.sh/ruff/contributing/#rule-testing-fixtures-and-snapshots and adding positive and negative cases for each of await vs nothing, async with vs with, and async for vs for.

Questions/todo

• [ ] I'm unsure whether the rule should also check for yield appearing in functions. My understanding is that there has been several rounds of iteration in how python async is supposed to be done, and that yield is not considered current, and so I haven't added a case for that. Therefore an async function containing only yield will cause this rule to fire.
• [ ] I was unsure how to access just the range of the function declaration (without the body), so the rule currently indicates the whole function declaration & body as the problem area.
• [ ] Needs review by @zanieb

[plredmond]

Sorry, I neglected formatting and linting of the rust code. I'll do that now.

[github-actions[bot]]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+314 -0 violations, +0 -0 fixes in 5 projects; 39 projects unchanged)

DisnakeDev/disnake (+65 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ disnake/ext/commands/core.py:1722:19: RUF029 Function `wrapper` is declared `async`, but doesn't `await` or use `async` features.
+ disnake/utils.py:522:11: RUF029 Function `_assetbytes_to_base64_data` is declared `async`, but doesn't `await` or use `async` features.
+ disnake/utils.py:527:11: RUF029 Function `_assetbytes_to_base64_data` is declared `async`, but doesn't `await` or use `async` features.
+ examples/basic_bot.py:81:11: RUF029 Function `on_ready` is declared `async`, but doesn't `await` or use `async` features.
+ examples/basic_voice.py:136:11: RUF029 Function `on_ready` is declared `async`, but doesn't `await` or use `async` features.
+ examples/converters.py:77:11: RUF029 Function `on_ready` is declared `async`, but doesn't `await` or use `async` features.
+ examples/edit_delete.py:52:11: RUF029 Function `on_ready` is declared `async`, but doesn't `await` or use `async` features.
+ examples/guessing_game.py:39:11: RUF029 Function `on_ready` is declared `async`, but doesn't `await` or use `async` features.
+ examples/interactions/autocomplete.py:25:11: RUF029 Function `autocomplete_langs` is declared `async`, but doesn't `await` or use `async` features.
+ examples/interactions/autocomplete.py:31:11: RUF029 Function `languages_1` is declared `async`, but doesn't `await` or use `async` features.
... 55 additional changes omitted for project

RasaHQ/rasa (+168 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ rasa/core/channels/botframework.py:293:19: RUF029 Function `health` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/channels/callback.py:68:19: RUF029 Function `health` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/channels/console.py:106:11: RUF029 Function `_get_user_input` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/channels/console.py:111:11: RUF029 Function `_get_user_input` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/channels/facebook.py:359:19: RUF029 Function `health` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/channels/facebook.py:363:19: RUF029 Function `token_verification` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/channels/hangouts.py:295:19: RUF029 Function `health` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/channels/mattermost.py:208:19: RUF029 Function `health` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/channels/rest.py:138:19: RUF029 Function `health` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/channels/rocketchat.py:148:19: RUF029 Function `health` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/channels/slack.py:503:19: RUF029 Function `health` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/channels/socketio.py:198:19: RUF029 Function `health` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/channels/socketio.py:202:19: RUF029 Function `connect` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/channels/socketio.py:220:19: RUF029 Function `disconnect` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/channels/telegram.py:202:19: RUF029 Function `health` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/channels/twilio.py:126:19: RUF029 Function `health` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/channels/twilio_voice.py:233:19: RUF029 Function `health` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/channels/webexteams.py:102:19: RUF029 Function `health` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/jobs.py:13:11: RUF029 Function `scheduler` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/run.py:129:15: RUF029 Function `configure_async_logging` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/run.py:292:11: RUF029 Function `create_connection_pools` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/core/training/interactive.py:1630:15: RUF029 Function `ignore_404s` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/server.py:1264:15: RUF029 Function `tracker_predict` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/server.py:1365:15: RUF029 Function `unload_model` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/server.py:1376:15: RUF029 Function `get_domain` is declared `async`, but doesn't `await` or use `async` features.
+ rasa/server.py:403:11: RUF029 Function `authenticate` is declared `async`, but doesn't `await` or use `async` features.
... 142 additional changes omitted for project

apache/airflow (+12 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ tests/providers/apache/livy/hooks/test_livy.py:508:19: RUF029 Function `mock_fun` is declared `async`, but doesn't `await` or use `async` features.
+ tests/providers/apache/livy/hooks/test_livy.py:531:19: RUF029 Function `mock_fun` is declared `async`, but doesn't `await` or use `async` features.
+ tests/providers/apache/livy/hooks/test_livy.py:556:19: RUF029 Function `mock_fun` is declared `async`, but doesn't `await` or use `async` features.
+ tests/providers/apache/livy/hooks/test_livy.py:597:19: RUF029 Function `mock_fun` is declared `async`, but doesn't `await` or use `async` features.
+ tests/providers/apache/livy/hooks/test_livy.py:616:19: RUF029 Function `mock_fun` is declared `async`, but doesn't `await` or use `async` features.
+ tests/providers/cncf/kubernetes/triggers/test_pod.py:76:15: RUF029 Function `mock_read_namespaced_pod` is declared `async`, but doesn't `await` or use `async` features.
+ tests/providers/google/cloud/hooks/test_cloud_batch.py:322:19: RUF029 Function `_get_job` is declared `async`, but doesn't `await` or use `async` features.
+ tests/providers/google/cloud/hooks/test_dataproc.py:68:11: RUF029 Function `mock_awaitable` is declared `async`, but doesn't `await` or use `async` features.
+ tests/providers/google/cloud/triggers/test_cloud_batch.py:137:19: RUF029 Function `_mock_job` is declared `async`, but doesn't `await` or use `async` features.
+ tests/providers/google/cloud/triggers/test_cloud_run.py:109:19: RUF029 Function `_mock_operation` is declared `async`, but doesn't `await` or use `async` features.
... 2 additional changes omitted for project

python/typeshed (+65 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select E,F,FA,I,PYI,RUF,UP,W

+ stdlib/asyncio/staggered.pyi:8:11: RUF029 Function `staggered_race` is declared `async`, but doesn't `await` or use `async` features.
+ stdlib/asyncio/streams.pyi:29:15: RUF029 Function `open_connection` is declared `async`, but doesn't `await` or use `async` features.
+ stdlib/asyncio/streams.pyi:37:15: RUF029 Function `start_server` is declared `async`, but doesn't `await` or use `async` features.
+ stdlib/asyncio/streams.pyi:48:15: RUF029 Function `open_connection` is declared `async`, but doesn't `await` or use `async` features.
+ stdlib/asyncio/streams.pyi:57:15: RUF029 Function `start_server` is declared `async`, but doesn't `await` or use `async` features.
+ stdlib/asyncio/streams.pyi:70:19: RUF029 Function `open_unix_connection` is declared `async`, but doesn't `await` or use `async` features.
+ stdlib/asyncio/streams.pyi:73:19: RUF029 Function `start_unix_server` is declared `async`, but doesn't `await` or use `async` features.
+ stdlib/asyncio/streams.pyi:77:19: RUF029 Function `open_unix_connection` is declared `async`, but doesn't `await` or use `async` features.
+ stdlib/asyncio/streams.pyi:80:19: RUF029 Function `start_unix_server` is declared `async`, but doesn't `await` or use `async` features.
+ stdlib/asyncio/subprocess.pyi:104:15: RUF029 Function `create_subprocess_shell` is declared `async`, but doesn't `await` or use `async` features.
... 55 additional changes omitted for project

zulip/zulip (+4 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ zerver/lib/push_notifications.py:183:15: RUF029 Function `err_func` is declared `async`, but doesn't `await` or use `async` features.
+ zerver/lib/push_notifications.py:188:15: RUF029 Function `make_apns` is declared `async`, but doesn't `await` or use `async` features.
+ zerver/tornado/event_queue.py:687:11: RUF029 Function `setup_event_queue` is declared `async`, but doesn't `await` or use `async` features.
+ zerver/tornado/views.py:42:15: RUF029 Function `wrapped` is declared `async`, but doesn't `await` or use `async` features.

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
RUF029	314	314	0	0	0

[zanieb]

The rasa ecosystem checks look like a bunch of false positives due to methods that are overriding an abstract method which must be async. This common enough that we should attempt to avoid it, but it requires multifile analysis in most cases which we do not yet support yet. There are some other issues like this, but I can't recall them — perhaps @charliermarsh knows. We could consider just not applying this to methods in classes with base classes for now?

[charliermarsh]

Ah yeah, the thing we often do there (at the very least) is check if the method has an @override decorator (can grep for is_override), since that's used to hint to static analysis tools that the method doesn't have control over its own signature. So users at least have a way to opt-out of these kinds of rules entirely for methods that override a parent method. I would be fine omitting this entirely for classes with base classes though (or even classes at all?).

[zanieb]

Here's another interesting edge-case false positive https://github.com/zulip/zulip/blob/35098f49597895718343091881fbd6198bd2022d/zerver/tornado/views.py#L35 — this one I'm less sure we can/should do anything about.

[zanieb]

Perhaps @AlexWaygood can elucidate whether or not this should trigger for functions with yield.

https://peps.python.org/pep-0525 may be helpful.

[AlexWaygood]

Perhaps @AlexWaygood can elucidate whether or not this should trigger for functions with yield.

peps.python.org/pep-0525 may be helpful.

Yeah, async functions that contain yield are async generators that can be used in async for loops by other async functions. So I don't think ruff should emit this diagnostic for async functions that have a yield in them:

>>> import asyncio
>>> async def x():
...     yield 42
...     yield 56
...
>>> x
<function x at 0x00000274CDC52200>
>>> x()
<async_generator object x at 0x00000274CC36BE20>
>>> async def y():
...     async for number in x():
...         print(number)
...
>>> asyncio.run(y())
42
56

[MichaReiser]

I think there's one issue that we might need to look into and it's that the visitor will cross function boundaries when searching for await and yield, resulting in false negatives

async def unnecessary_async():
	async def inner():
		await x

async def unnecessary_async2():
	class Inner:
		async def inner():	
			await x

I don't think the rule flags unnecessary_async or unnecessary_async2.

[plredmond]

Thanks for your review! I've looked over the comments above and can work on the changes later today. I need to devote this morning to a different task.

Ah yeah, the thing we often do there (at the very least) is check if the method has an @override decorator (can grep for is_override), since that's used to hint to static analysis tools that the method doesn't have control over its own signature. So users at least have a way to opt-out of these kinds of rules entirely for methods that override a parent method. I would be fine omitting this entirely for classes with base classes though (or even classes at all?).

@charliermarsh should I omit this check in both the cases (1) in the context of a class or (2) when the override decorator is present? Based on your description, it seems preferable to limit case (1) to classes with a base class, since they're actually likely to be overriding things, though.

[plredmond]

I don't think the rule flags unnecessary_async or unnecessary_async2.

@MichaReiser These examples make the problem a bit harder. :slightly_smiling_face: I need to check if the visitor has a callback for when an inner-function ends; I could use that to track which scope I'm in when detecting yielding-expressions to attribute them correctly.

[MichaReiser]

@plredmond I think there are only two cases where this is relevant: classes and functions. You could extend your visitor to handle visit_stmt explicitly and traverse classes and functions manually (don't call walk_stmt). This allows you to skip traversing the body (you still want to traverse decorators and the function header)

[plredmond]

Hi! Sorry to let this PR languish. I have a paper deadline just before I start my astral internship, so I have to put this on ice until then.

[MichaReiser]

IMO, unused-async is easier to understand than spurious (which I mainly associate with threading). It also fits well into other rules that catch "unused" syntax.

[AlexWaygood]

Yeah, async functions that contain yield are async generators that can be used in async for loops by other async functions. So I don't think ruff should emit this diagnostic for async functions that have a yield in them:

So, I chatted to @carljm about this offline... and he persuaded me that the rationale I gave in https://github.com/astral-sh/ruff/pull/9966#issuecomment-1941990446 for excluding async generators from this check basically makes no sense :-)

It's true that removing the async keyword from a function with a yield statement in it will mean that it will become a sync generator function rather than an async generator function. That means that it will only be usable in for loops, not async for loops, which means callsites of the function will have to be rewritten to use for loops rather than async for loops -- potentially a sweeping and disruptive refactor. However... that's not really materially different from the changes this rule recommends to non-generator functions. Removing redundant async keywords from non-generator functions that don't have any awaits in them will also necessitate callsites being refactored: you'll have to do this refactor everywhere:

- x = await function_in_question()
+ x = function_in_question()

As such, I'm now persuaded that there's no reason to exclude async generator functions from this check. (Thanks @carljm!)

[plredmond]

Picking this up today. Sorry for the delay.

[plredmond]

IMO, unused-async is easier to understand than spurious (which I mainly associate with threading). It also fits well into other rules that catch "unused" syntax.

Saving this rename for last b/c it'll disrupt the above comments on diffs.

[plredmond]

I've made a bunch of smaller/cosmetic changes per the comments above. Additionally, I've added four new test cases (two pass, and two fail). Note, that the tests do not pass right now, because i need to implement the changes. The remaining work is:

• make the new test cases pass
  • ~~change the rule to not fire when an async function contains a yield~~ (this was switched to fire, per @carljm & @AlexWaygood)
  • change the rule to fire when an async function using no async features contains an async function which does
  • change the rule to fire when an async function using no async features contains a class with an async function which does
  • skip applying this rule to class methods entirely and make an issue to track doing so in a limited way (per @zanieb)
• fix the visitor to stop traversing early when it finds a problem
• rename "spurious async" to "unused async" throughout

Could somebody take a look at the two pass-cases and fail-cases that I just added and confirm that this is the desired behavior?

[charliermarsh]

Many reviewers on here -- who wants to own the final review + approval? \cc @carljm @AlexWaygood @zanieb @MichaReiser

[plredmond]

Sorry for the excessive number of commits here. I've had a bit of churn in the test cases because I found them confusing and reorganized them. The real work is these three commits:

• switch to preorder visitor, 1cb7765
• skip the rest of traversal after finding an async/await, 41f0ac5
• don't traverse the body of inner-functions or inner-classes, b714e80

I believe this covers the test cases @MichaReiser shared above with the nested asyncs using his suggested solution (partially reimplementing traversal of function defs and class defs), as well as his suggested optimization to stop traversing once some evidence that the function is actually async has been found.

However, this doesn't cover @charliermarsh's suggestion to just omit checking methods in classes. I'm not sure how to approach that (and the test case is commented out). The rule is currently written in terms of a function definition; to omit the rule for methods, we'd need to know whether that function definition is a method. I didn't see a way to do that in analyze/statement.rs.

Except for @charliermarsh's suggestion, the remaining work is just to rename spurious->unused.

[plredmond]

Ok, I think everything is done except for this:

However, this doesn't cover @charliermarsh's suggestion to just omit checking methods in classes. I'm not sure how to approach that (and the test case is commented out). The rule is currently written in terms of a function definition; to omit the rule for methods, we'd need to know whether that function definition is a method. I didn't see a way to do that in analyze/statement.rs.

[EDIT: I'll look at the test failure in the morning. I don't see one when I test locally.]

[MichaReiser]

@plredmond I'm not entirely sure if that's what you mean but you can test if you're inside a class by using

    // Assignments in class bodies are attributes (e.g., `x = x` assigns `x` to `self.x`, and thus
    // is not a self-assignment).
    if checker.semantic().current_scope().kind.is_class() {
        return;
    }

You can also use the kind to retrieve more information about the enclosing class (e.g. if it has any base class or whatnot)

[kkom]

Came across this PR when looking at #9951 – thank you for adding this rule, very excited about it!

Regarding the latest discussion – I'm curious why not omit it for method marked with @override and continue linting methods? This is analogous to how https://docs.astral.sh/ruff/rules/no-self-use/ works – and that rule has worked really well for us in that way.

[zanieb]

@kkom we're just cutting scope from the initial rule. We could consider it in the future.

[charliermarsh]

This looks good to me, but I suggest adding the snippet that Micha posted above.

[charliermarsh]

@plredmond - Try running cargo dev generate-all -- you need to regenerate the JSON Schema since we added a new rule.

[charliermarsh]

(I believe that's the cause of your failing test.)

[plredmond]

$ cargo dev generate-all
...
$ git diff
diff --git a/ruff.schema.json b/ruff.schema.json
index 871cce40b..90582fc67 100644
--- a/ruff.schema.json
+++ b/ruff.schema.json
@@ -3951,4 +3951,4 @@
       ]
     }
   }
-}
+}
\ No newline at end of file

[charliermarsh]

Is your editor adding a newline?

[plredmond]

I also tried fetching and merging in the latest main in. Still no success. Strangely, I'm not seeing this locally.

I'll look at the CI output.

[plredmond]

Is your editor adding a newline?

It was modified by cargo dev generate-all afaik. My editor is vim and I didn't modify that file.