Skip to content

Commit

Permalink
Add initial flake8-trio rule (#8439)
Browse files Browse the repository at this point in the history
## Summary

This pull request adds
[flake8-trio](https://github.com/Zac-HD/flake8-trio) support to ruff,
which is a very useful plugin for trio users to avoid very common
mistakes.

Part of #8451.

## Test Plan

Traditional rule testing, as [described in the
documentation](https://docs.astral.sh/ruff/contributing/#rule-testing-fixtures-and-snapshots).
  • Loading branch information
karpetrosyan authored Nov 3, 2023
1 parent 7fa6ac9 commit 2ff1afb
Show file tree
Hide file tree
Showing 13 changed files with 242 additions and 2 deletions.
25 changes: 25 additions & 0 deletions LICENSE
Original file line number Diff line number Diff line change
Expand Up @@ -1269,6 +1269,31 @@ are:
SOFTWARE.
"""

- flake8-trio, licensed as follows:
"""
MIT License

Copyright (c) 2022 Zac Hatfield-Dodds

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.
"""

- Pyright, licensed as follows:
"""
MIT License
Expand Down
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,7 @@ quality tools, including:
- [flake8-super](https://pypi.org/project/flake8-super/)
- [flake8-tidy-imports](https://pypi.org/project/flake8-tidy-imports/)
- [flake8-todos](https://pypi.org/project/flake8-todos/)
- [flake8-trio](https://pypi.org/project/flake8-trio/)
- [flake8-type-checking](https://pypi.org/project/flake8-type-checking/)
- [flake8-use-pathlib](https://pypi.org/project/flake8-use-pathlib/)
- [flynt](https://pypi.org/project/flynt/) ([#2102](https://github.com/astral-sh/ruff/issues/2102))
Expand Down
18 changes: 18 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_trio/TRIO100.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import trio


async def foo():
with trio.fail_after():
...

async def foo():
with trio.fail_at():
await ...

async def foo():
with trio.move_on_after():
...

async def foo():
with trio.move_at():
await ...
7 changes: 5 additions & 2 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ use crate::rules::{
airflow, flake8_bandit, flake8_boolean_trap, flake8_bugbear, flake8_builtins, flake8_debugger,
flake8_django, flake8_errmsg, flake8_import_conventions, flake8_pie, flake8_pyi,
flake8_pytest_style, flake8_raise, flake8_return, flake8_simplify, flake8_slots,
flake8_tidy_imports, flake8_type_checking, mccabe, pandas_vet, pep8_naming, perflint,
pycodestyle, pyflakes, pygrep_hooks, pylint, pyupgrade, refurb, ruff, tryceratops,
flake8_tidy_imports, flake8_trio, flake8_type_checking, mccabe, pandas_vet, pep8_naming,
perflint, pycodestyle, pyflakes, pygrep_hooks, pylint, pyupgrade, refurb, ruff, tryceratops,
};
use crate::settings::types::PythonVersion;

Expand Down Expand Up @@ -1195,6 +1195,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::UselessWithLock) {
pylint::rules::useless_with_lock(checker, with_stmt);
}
if checker.enabled(Rule::TrioTimeoutWithoutAwait) {
flake8_trio::rules::timeout_without_await(checker, with_stmt, items);
}
}
Stmt::While(ast::StmtWhile { body, orelse, .. }) => {
if checker.enabled(Rule::FunctionUsesLoopVariable) {
Expand Down
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,9 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Async, "101") => (RuleGroup::Stable, rules::flake8_async::rules::OpenSleepOrSubprocessInAsyncFunction),
(Flake8Async, "102") => (RuleGroup::Stable, rules::flake8_async::rules::BlockingOsCallInAsyncFunction),

// flake8-trio
(Flake8Trio, "100") => (RuleGroup::Preview, rules::flake8_trio::rules::TrioTimeoutWithoutAwait),

// flake8-builtins
(Flake8Builtins, "001") => (RuleGroup::Stable, rules::flake8_builtins::rules::BuiltinVariableShadowing),
(Flake8Builtins, "002") => (RuleGroup::Stable, rules::flake8_builtins::rules::BuiltinArgumentShadowing),
Expand Down
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ pub enum Linter {
/// [flake8-async](https://pypi.org/project/flake8-async/)
#[prefix = "ASYNC"]
Flake8Async,
/// [flake8-trio](https://pypi.org/project/flake8-trio/)
#[prefix = "TRIO"]
Flake8Trio,
/// [flake8-bandit](https://pypi.org/project/flake8-bandit/)
#[prefix = "S"]
Flake8Bandit,
Expand Down
26 changes: 26 additions & 0 deletions crates/ruff_linter/src/rules/flake8_trio/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
//! Rules from [flake8-trio](https://pypi.org/project/flake8-trio/).
pub(crate) mod rules;

#[cfg(test)]
mod tests {
use std::path::Path;

use anyhow::Result;
use test_case::test_case;

use crate::assert_messages;
use crate::registry::Rule;
use crate::settings::LinterSettings;
use crate::test::test_path;

#[test_case(Rule::TrioTimeoutWithoutAwait, Path::new("TRIO100.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Path::new("flake8_trio").join(path).as_path(),
&LinterSettings::for_rule(rule_code),
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}
}
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/rules/flake8_trio/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
pub(crate) use timeout_without_await::*;

mod timeout_without_await;
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::call_path::CallPath;
use ruff_python_ast::visitor::{walk_expr, walk_stmt, Visitor};
use ruff_python_ast::{Expr, ExprAwait, Stmt, StmtWith, WithItem};

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for trio functions that should contain await but don't.
///
/// ## Why is this bad?
/// Some trio context managers, such as `trio.fail_after` and
/// `trio.move_on_after`, have no effect unless they contain an `await`
/// statement. The use of such functions without an `await` statement is
/// likely a mistake.
///
/// ## Example
/// ```python
/// async def func():
/// with trio.move_on_after(2):
/// do_something()
/// ```
///
/// Use instead:
/// ```python
/// async def func():
/// with trio.move_on_after(2):
/// do_something()
/// await awaitable()
/// ```
#[violation]
pub struct TrioTimeoutWithoutAwait {
method_name: MethodName,
}

impl Violation for TrioTimeoutWithoutAwait {
#[derive_message_formats]
fn message(&self) -> String {
let Self { method_name } = self;
format!("A `with {method_name}(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint.")
}
}

/// TRIO100
pub(crate) fn timeout_without_await(
checker: &mut Checker,
with_stmt: &StmtWith,
with_items: &[WithItem],
) {
let Some(method_name) = with_items.iter().find_map(|item| {
let call = item.context_expr.as_call_expr()?;
let call_path = checker.semantic().resolve_call_path(call.func.as_ref())?;
MethodName::try_from(&call_path)
}) else {
return;
};

let mut visitor = AwaitVisitor::default();
visitor.visit_body(&with_stmt.body);
if visitor.seen_await {
return;
}

checker.diagnostics.push(Diagnostic::new(
TrioTimeoutWithoutAwait { method_name },
with_stmt.range,
));
}

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
enum MethodName {
MoveOnAfter,
MoveOnAt,
FailAfter,
FailAt,
CancelScope,
}

impl MethodName {
fn try_from(call_path: &CallPath<'_>) -> Option<Self> {
match call_path.as_slice() {
["trio", "move_on_after"] => Some(Self::MoveOnAfter),
["trio", "move_on_at"] => Some(Self::MoveOnAt),
["trio", "fail_after"] => Some(Self::FailAfter),
["trio", "fail_at"] => Some(Self::FailAt),
["trio", "CancelScope"] => Some(Self::CancelScope),
_ => None,
}
}
}

impl std::fmt::Display for MethodName {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
MethodName::MoveOnAfter => write!(f, "trio.move_on_after"),
MethodName::MoveOnAt => write!(f, "trio.move_on_at"),
MethodName::FailAfter => write!(f, "trio.fail_after"),
MethodName::FailAt => write!(f, "trio.fail_at"),
MethodName::CancelScope => write!(f, "trio.CancelScope"),
}
}
}

#[derive(Debug, Default)]
struct AwaitVisitor {
seen_await: bool,
}

impl Visitor<'_> for AwaitVisitor {
fn visit_stmt(&mut self, stmt: &Stmt) {
match stmt {
Stmt::FunctionDef(_) | Stmt::ClassDef(_) => (),
_ => walk_stmt(self, stmt),
}
}

fn visit_expr(&mut self, expr: &Expr) {
if let Expr::Await(ExprAwait { .. }) = expr {
self.seen_await = true;
} else {
walk_expr(self, expr);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
---
source: crates/ruff_linter/src/rules/flake8_trio/mod.rs
---
TRIO100.py:5:5: TRIO100 A `with trio.fail_after(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint.
|
4 | async def foo():
5 | with trio.fail_after():
| _____^
6 | | ...
| |___________^ TRIO100
7 |
8 | async def foo():
|

TRIO100.py:13:5: TRIO100 A `with trio.move_on_after(...):` context does not contain any `await` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint.
|
12 | async def foo():
13 | with trio.move_on_after():
| _____^
14 | | ...
| |___________^ TRIO100
15 |
16 | async def foo():
|


1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ pub mod flake8_simplify;
pub mod flake8_slots;
pub mod flake8_tidy_imports;
pub mod flake8_todos;
pub mod flake8_trio;
pub mod flake8_type_checking;
pub mod flake8_unused_arguments;
pub mod flake8_use_pathlib;
Expand Down
2 changes: 2 additions & 0 deletions docs/faq.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ natively, including:
- [flake8-super](https://pypi.org/project/flake8-super/)
- [flake8-tidy-imports](https://pypi.org/project/flake8-tidy-imports/)
- [flake8-todos](https://pypi.org/project/flake8-todos/)
- [flake8-trio](https://pypi.org/project/flake8-trio/) ([#8451](https://github.com/astral-sh/ruff/issues/8451))
- [flake8-type-checking](https://pypi.org/project/flake8-type-checking/)
- [flake8-use-pathlib](https://pypi.org/project/flake8-use-pathlib/)
- [flynt](https://pypi.org/project/flynt/) ([#2102](https://github.com/astral-sh/ruff/issues/2102))
Expand Down Expand Up @@ -185,6 +186,7 @@ Today, Ruff can be used to replace Flake8 when used with any of the following pl
- [flake8-super](https://pypi.org/project/flake8-super/)
- [flake8-tidy-imports](https://pypi.org/project/flake8-tidy-imports/)
- [flake8-todos](https://pypi.org/project/flake8-todos/)
- [flake8-trio](https://pypi.org/project/flake8-trio/) ([#8451](https://github.com/astral-sh/ruff/issues/8451))
- [flake8-type-checking](https://pypi.org/project/flake8-type-checking/)
- [flake8-use-pathlib](https://pypi.org/project/flake8-use-pathlib/)
- [flynt](https://pypi.org/project/flynt/) ([#2102](https://github.com/astral-sh/ruff/issues/2102))
Expand Down
4 changes: 4 additions & 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 2ff1afb

Please sign in to comment.