Skip to content

Commit

Permalink
Detect unneeded async keywords on functions (#9966)
Browse files Browse the repository at this point in the history
## 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
plredmond committed Apr 16, 2024
1 parent 45db695 commit 65edbfe
Show file tree
Hide file tree
Showing 8 changed files with 308 additions and 0 deletions.
67 changes: 67 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF029.py
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
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::SslWithBadDefaults) {
flake8_bandit::rules::ssl_with_bad_defaults(checker, function_def);
}
if checker.enabled(Rule::UnusedAsync) {
ruff::rules::unused_async(checker, function_def);
}
}
Stmt::Return(_) => {
if checker.enabled(Rule::ReturnOutsideFunction) {
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -963,6 +963,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Ruff, "026") => (RuleGroup::Preview, rules::ruff::rules::DefaultFactoryKwarg),
(Ruff, "027") => (RuleGroup::Preview, rules::ruff::rules::MissingFStringSyntax),
(Ruff, "028") => (RuleGroup::Preview, rules::ruff::rules::InvalidFormatterSuppressionComment),
(Ruff, "029") => (RuleGroup::Preview, rules::ruff::rules::UnusedAsync),
(Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA),
(Ruff, "200") => (RuleGroup::Stable, rules::ruff::rules::InvalidPyprojectToml),
#[cfg(feature = "test-rules")]
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ mod tests {
#[test_case(Rule::MissingFStringSyntax, Path::new("RUF027_1.py"))]
#[test_case(Rule::MissingFStringSyntax, Path::new("RUF027_2.py"))]
#[test_case(Rule::InvalidFormatterSuppressionComment, Path::new("RUF028.py"))]
#[test_case(Rule::UnusedAsync, Path::new("RUF029.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ pub(crate) use test_rules::*;
pub(crate) use unnecessary_dict_comprehension_for_iterable::*;
pub(crate) use unnecessary_iterable_allocation_for_first_element::*;
pub(crate) use unnecessary_key_check::*;
pub(crate) use unused_async::*;
pub(crate) use unused_noqa::*;

mod ambiguous_unicode_character;
Expand Down Expand Up @@ -58,6 +59,7 @@ pub(crate) mod test_rules;
mod unnecessary_dict_comprehension_for_iterable;
mod unnecessary_iterable_allocation_for_first_element;
mod unnecessary_key_check;
mod unused_async;
mod unused_noqa;

#[derive(Clone, Copy)]
Expand Down
177 changes: 177 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/unused_async.rs
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(),
));
}
}
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
|
1 change: 1 addition & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 65edbfe

Please sign in to comment.