-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Detect unneeded
async
keywords on functions (#9966)
## 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`.
- Loading branch information
Showing
8 changed files
with
308 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
import time | ||
import asyncio | ||
|
||
|
||
async def pass_1a(): # OK: awaits a coroutine | ||
await asyncio.sleep(1) | ||
|
||
|
||
async def pass_1b(): # OK: awaits a coroutine | ||
def foo(optional_arg=await bar()): | ||
pass | ||
|
||
|
||
async def pass_2(): # OK: uses an async context manager | ||
async with None as i: | ||
pass | ||
|
||
|
||
async def pass_3(): # OK: uses an async loop | ||
async for i in []: | ||
pass | ||
|
||
|
||
class Foo: | ||
async def pass_4(): # OK: method of a class | ||
pass | ||
|
||
|
||
def foo(): | ||
async def pass_5(): # OK: uses an await | ||
await bla | ||
|
||
|
||
async def fail_1a(): # RUF029 | ||
time.sleep(1) | ||
|
||
|
||
async def fail_1b(): # RUF029: yield does not require async | ||
yield "hello" | ||
|
||
|
||
async def fail_2(): # RUF029 | ||
with None as i: | ||
pass | ||
|
||
|
||
async def fail_3(): # RUF029 | ||
for i in []: | ||
pass | ||
|
||
return foo | ||
|
||
|
||
async def fail_4a(): # RUF029: the /outer/ function does not await | ||
async def foo(): | ||
await bla | ||
|
||
|
||
async def fail_4b(): # RUF029: the /outer/ function does not await | ||
class Foo: | ||
async def foo(): | ||
await bla | ||
|
||
|
||
def foo(): | ||
async def fail_4c(): # RUF029: the /inner/ function does not await | ||
pass |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
177 changes: 177 additions & 0 deletions
177
crates/ruff_linter/src/rules/ruff/rules/unused_async.rs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,177 @@ | ||
use ruff_diagnostics::{Diagnostic, Violation}; | ||
use ruff_macros::{derive_message_formats, violation}; | ||
use ruff_python_ast::identifier::Identifier; | ||
use ruff_python_ast::visitor::preorder; | ||
use ruff_python_ast::{self as ast, AnyNodeRef, Expr, Stmt}; | ||
|
||
use crate::checkers::ast::Checker; | ||
|
||
/// ## What it does | ||
/// Checks for functions declared `async` that do not await or otherwise use features requiring the | ||
/// function to be declared `async`. | ||
/// | ||
/// ## Why is this bad? | ||
/// Declaring a function `async` when it's not is usually a mistake, and will artificially limit the | ||
/// contexts where that function may be called. In some cases, labeling a function `async` is | ||
/// semantically meaningful (e.g. with the trio library). | ||
/// | ||
/// ## Examples | ||
/// ```python | ||
/// async def foo(): | ||
/// bar() | ||
/// ``` | ||
/// | ||
/// Use instead: | ||
/// ```python | ||
/// def foo(): | ||
/// bar() | ||
/// ``` | ||
#[violation] | ||
pub struct UnusedAsync { | ||
name: String, | ||
} | ||
|
||
impl Violation for UnusedAsync { | ||
#[derive_message_formats] | ||
fn message(&self) -> String { | ||
let UnusedAsync { name } = self; | ||
format!( | ||
"Function `{name}` is declared `async`, but doesn't `await` or use `async` features." | ||
) | ||
} | ||
} | ||
|
||
#[derive(Default)] | ||
struct AsyncExprVisitor { | ||
found_await_or_async: bool, | ||
} | ||
|
||
/// Traverse a function's body to find whether it contains an await-expr, an async-with, or an | ||
/// async-for. Stop traversing after one is found. The bodies of inner-functions and inner-classes | ||
/// aren't traversed. | ||
impl<'a> preorder::PreorderVisitor<'a> for AsyncExprVisitor { | ||
fn enter_node(&mut self, _node: AnyNodeRef<'a>) -> preorder::TraversalSignal { | ||
if self.found_await_or_async { | ||
preorder::TraversalSignal::Skip | ||
} else { | ||
preorder::TraversalSignal::Traverse | ||
} | ||
} | ||
fn visit_expr(&mut self, expr: &'a Expr) { | ||
match expr { | ||
Expr::Await(_) => { | ||
self.found_await_or_async = true; | ||
} | ||
_ => preorder::walk_expr(self, expr), | ||
} | ||
} | ||
fn visit_stmt(&mut self, stmt: &'a Stmt) { | ||
match stmt { | ||
Stmt::With(ast::StmtWith { is_async: true, .. }) => { | ||
self.found_await_or_async = true; | ||
} | ||
Stmt::For(ast::StmtFor { is_async: true, .. }) => { | ||
self.found_await_or_async = true; | ||
} | ||
// avoid counting inner classes' or functions' bodies toward the search | ||
Stmt::FunctionDef(function_def) => { | ||
function_def_visit_preorder_except_body(function_def, self); | ||
} | ||
Stmt::ClassDef(class_def) => { | ||
class_def_visit_preorder_except_body(class_def, self); | ||
} | ||
_ => preorder::walk_stmt(self, stmt), | ||
} | ||
} | ||
} | ||
|
||
/// Very nearly `crate::node::StmtFunctionDef.visit_preorder`, except it is specialized and, | ||
/// crucially, doesn't traverse the body. | ||
fn function_def_visit_preorder_except_body<'a, V>( | ||
function_def: &'a ast::StmtFunctionDef, | ||
visitor: &mut V, | ||
) where | ||
V: preorder::PreorderVisitor<'a>, | ||
{ | ||
let ast::StmtFunctionDef { | ||
parameters, | ||
decorator_list, | ||
returns, | ||
type_params, | ||
.. | ||
} = function_def; | ||
|
||
for decorator in decorator_list { | ||
visitor.visit_decorator(decorator); | ||
} | ||
|
||
if let Some(type_params) = type_params { | ||
visitor.visit_type_params(type_params); | ||
} | ||
|
||
visitor.visit_parameters(parameters); | ||
|
||
for expr in returns { | ||
visitor.visit_annotation(expr); | ||
} | ||
} | ||
|
||
/// Very nearly `crate::node::StmtClassDef.visit_preorder`, except it is specialized and, | ||
/// crucially, doesn't traverse the body. | ||
fn class_def_visit_preorder_except_body<'a, V>(class_def: &'a ast::StmtClassDef, visitor: &mut V) | ||
where | ||
V: preorder::PreorderVisitor<'a>, | ||
{ | ||
let ast::StmtClassDef { | ||
arguments, | ||
decorator_list, | ||
type_params, | ||
.. | ||
} = class_def; | ||
|
||
for decorator in decorator_list { | ||
visitor.visit_decorator(decorator); | ||
} | ||
|
||
if let Some(type_params) = type_params { | ||
visitor.visit_type_params(type_params); | ||
} | ||
|
||
if let Some(arguments) = arguments { | ||
visitor.visit_arguments(arguments); | ||
} | ||
} | ||
|
||
/// RUF029 | ||
pub(crate) fn unused_async( | ||
checker: &mut Checker, | ||
function_def @ ast::StmtFunctionDef { | ||
is_async, | ||
name, | ||
body, | ||
.. | ||
}: &ast::StmtFunctionDef, | ||
) { | ||
if !is_async { | ||
return; | ||
} | ||
|
||
if checker.semantic().current_scope().kind.is_class() { | ||
return; | ||
} | ||
|
||
let found_await_or_async = { | ||
let mut visitor = AsyncExprVisitor::default(); | ||
preorder::walk_body(&mut visitor, body); | ||
visitor.found_await_or_async | ||
}; | ||
|
||
if !found_await_or_async { | ||
checker.diagnostics.push(Diagnostic::new( | ||
UnusedAsync { | ||
name: name.to_string(), | ||
}, | ||
function_def.identifier(), | ||
)); | ||
} | ||
} |
56 changes: 56 additions & 0 deletions
56
...ff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF029_RUF029.py.snap
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
--- | ||
source: crates/ruff_linter/src/rules/ruff/mod.rs | ||
--- | ||
RUF029.py:34:11: RUF029 Function `fail_1a` is declared `async`, but doesn't `await` or use `async` features. | ||
| | ||
34 | async def fail_1a(): # RUF029 | ||
| ^^^^^^^ RUF029 | ||
35 | time.sleep(1) | ||
| | ||
|
||
RUF029.py:38:11: RUF029 Function `fail_1b` is declared `async`, but doesn't `await` or use `async` features. | ||
| | ||
38 | async def fail_1b(): # RUF029: yield does not require async | ||
| ^^^^^^^ RUF029 | ||
39 | yield "hello" | ||
| | ||
|
||
RUF029.py:42:11: RUF029 Function `fail_2` is declared `async`, but doesn't `await` or use `async` features. | ||
| | ||
42 | async def fail_2(): # RUF029 | ||
| ^^^^^^ RUF029 | ||
43 | with None as i: | ||
44 | pass | ||
| | ||
|
||
RUF029.py:47:11: RUF029 Function `fail_3` is declared `async`, but doesn't `await` or use `async` features. | ||
| | ||
47 | async def fail_3(): # RUF029 | ||
| ^^^^^^ RUF029 | ||
48 | for i in []: | ||
49 | pass | ||
| | ||
|
||
RUF029.py:54:11: RUF029 Function `fail_4a` is declared `async`, but doesn't `await` or use `async` features. | ||
| | ||
54 | async def fail_4a(): # RUF029: the /outer/ function does not await | ||
| ^^^^^^^ RUF029 | ||
55 | async def foo(): | ||
56 | await bla | ||
| | ||
|
||
RUF029.py:59:11: RUF029 Function `fail_4b` is declared `async`, but doesn't `await` or use `async` features. | ||
| | ||
59 | async def fail_4b(): # RUF029: the /outer/ function does not await | ||
| ^^^^^^^ RUF029 | ||
60 | class Foo: | ||
61 | async def foo(): | ||
| | ||
|
||
RUF029.py:66:15: RUF029 Function `fail_4c` is declared `async`, but doesn't `await` or use `async` features. | ||
| | ||
65 | def foo(): | ||
66 | async def fail_4c(): # RUF029: the /inner/ function does not await | ||
| ^^^^^^^ RUF029 | ||
67 | pass | ||
| |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.