Skip to content

Commit

Permalink
[TRIO] Add TRIO105: SyncTrioCall (#8490)
Browse files Browse the repository at this point in the history
## Summary

Adds `TRIO105` from the [flake8-trio
plugin](https://github.com/Zac-HD/flake8-trio). The `MethodName` logic
mirrors that of `TRIO100` to stay consistent within the plugin.

It is at 95% parity with the exception of upstream also checking for a
slightly more complex scenario where a call to `start()` on a
`trio.Nursery` context should also be immediately awaited. Upstream
plugin appears to just check for anything named `nursery` judging from
[the relevant issue](python-trio/flake8-async#56).

Unsure if we want to do so something similar or, alternatively, if there
is some capability in ruff to check for calls made on this context some
other way

## Test Plan

Added a new fixture, based on [the one from upstream
plugin](https://github.com/Zac-HD/flake8-trio/blob/main/tests/eval_files/trio105.py)

## Issue link

Refers: #8451
  • Loading branch information
qdegraaf authored Nov 5, 2023
1 parent 72ebde8 commit 4170ef0
Show file tree
Hide file tree
Showing 11 changed files with 872 additions and 61 deletions.
64 changes: 64 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_trio/TRIO105.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import trio


async def func() -> None:
trio.run(foo) # OK, not async

# OK
await trio.aclose_forcefully(foo)
await trio.open_file(foo)
await trio.open_ssl_over_tcp_listeners(foo, foo)
await trio.open_ssl_over_tcp_stream(foo, foo)
await trio.open_tcp_listeners(foo)
await trio.open_tcp_stream(foo, foo)
await trio.open_unix_socket(foo)
await trio.run_process(foo)
await trio.sleep(5)
await trio.sleep_until(5)
await trio.lowlevel.cancel_shielded_checkpoint()
await trio.lowlevel.checkpoint()
await trio.lowlevel.checkpoint_if_cancelled()
await trio.lowlevel.open_process(foo)
await trio.lowlevel.permanently_detach_coroutine_object(foo)
await trio.lowlevel.reattach_detached_coroutine_object(foo, foo)
await trio.lowlevel.temporarily_detach_coroutine_object(foo)
await trio.lowlevel.wait_readable(foo)
await trio.lowlevel.wait_task_rescheduled(foo)
await trio.lowlevel.wait_writable(foo)

# TRIO105
trio.aclose_forcefully(foo)
trio.open_file(foo)
trio.open_ssl_over_tcp_listeners(foo, foo)
trio.open_ssl_over_tcp_stream(foo, foo)
trio.open_tcp_listeners(foo)
trio.open_tcp_stream(foo, foo)
trio.open_unix_socket(foo)
trio.run_process(foo)
trio.serve_listeners(foo, foo)
trio.serve_ssl_over_tcp(foo, foo, foo)
trio.serve_tcp(foo, foo)
trio.sleep(foo)
trio.sleep_forever()
trio.sleep_until(foo)
trio.lowlevel.cancel_shielded_checkpoint()
trio.lowlevel.checkpoint()
trio.lowlevel.checkpoint_if_cancelled()
trio.lowlevel.open_process()
trio.lowlevel.permanently_detach_coroutine_object(foo)
trio.lowlevel.reattach_detached_coroutine_object(foo, foo)
trio.lowlevel.temporarily_detach_coroutine_object(foo)
trio.lowlevel.wait_readable(foo)
trio.lowlevel.wait_task_rescheduled(foo)
trio.lowlevel.wait_writable(foo)

async with await trio.open_file(foo): # Ok
pass

async with trio.open_file(foo): # TRIO105
pass


def func() -> None:
# TRIO105 (without fix)
trio.open_file(foo)
7 changes: 5 additions & 2 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ use crate::rules::{
flake8_comprehensions, flake8_datetimez, flake8_debugger, flake8_django,
flake8_future_annotations, flake8_gettext, flake8_implicit_str_concat, flake8_logging,
flake8_logging_format, flake8_pie, flake8_print, flake8_pyi, flake8_pytest_style, flake8_self,
flake8_simplify, flake8_tidy_imports, flake8_use_pathlib, flynt, numpy, pandas_vet,
pep8_naming, pycodestyle, pyflakes, pygrep_hooks, pylint, pyupgrade, refurb, ruff,
flake8_simplify, flake8_tidy_imports, flake8_trio, flake8_use_pathlib, flynt, numpy,
pandas_vet, pep8_naming, pycodestyle, pyflakes, pygrep_hooks, pylint, pyupgrade, refurb, ruff,
};
use crate::settings::types::PythonVersion;

Expand Down Expand Up @@ -926,6 +926,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::ImplicitCwd) {
refurb::rules::no_implicit_cwd(checker, call);
}
if checker.enabled(Rule::TrioSyncCall) {
flake8_trio::rules::sync_call(checker, call);
}
}
Expr::Dict(
dict @ ast::ExprDict {
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 @@ -292,6 +292,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {

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

// flake8-builtins
(Flake8Builtins, "001") => (RuleGroup::Stable, rules::flake8_builtins::rules::BuiltinVariableShadowing),
Expand Down
157 changes: 157 additions & 0 deletions crates/ruff_linter/src/rules/flake8_trio/method_name.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
use ruff_python_ast::call_path::CallPath;

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub(super) enum MethodName {
AcloseForcefully,
CancelScope,
CancelShieldedCheckpoint,
Checkpoint,
CheckpointIfCancelled,
FailAfter,
FailAt,
MoveOnAfter,
MoveOnAt,
OpenFile,
OpenProcess,
OpenSslOverTcpListeners,
OpenSslOverTcpStream,
OpenTcpListeners,
OpenTcpStream,
OpenUnixSocket,
PermanentlyDetachCoroutineObject,
ReattachDetachedCoroutineObject,
RunProcess,
ServeListeners,
ServeSslOverTcp,
ServeTcp,
Sleep,
SleepForever,
TemporarilyDetachCoroutineObject,
WaitReadable,
WaitTaskRescheduled,
WaitWritable,
}

impl MethodName {
/// Returns `true` if the method is async, `false` if it is sync.
pub(super) fn is_async(self) -> bool {
match self {
MethodName::AcloseForcefully
| MethodName::CancelShieldedCheckpoint
| MethodName::Checkpoint
| MethodName::CheckpointIfCancelled
| MethodName::OpenFile
| MethodName::OpenProcess
| MethodName::OpenSslOverTcpListeners
| MethodName::OpenSslOverTcpStream
| MethodName::OpenTcpListeners
| MethodName::OpenTcpStream
| MethodName::OpenUnixSocket
| MethodName::PermanentlyDetachCoroutineObject
| MethodName::ReattachDetachedCoroutineObject
| MethodName::RunProcess
| MethodName::ServeListeners
| MethodName::ServeSslOverTcp
| MethodName::ServeTcp
| MethodName::Sleep
| MethodName::SleepForever
| MethodName::TemporarilyDetachCoroutineObject
| MethodName::WaitReadable
| MethodName::WaitTaskRescheduled
| MethodName::WaitWritable => true,

MethodName::MoveOnAfter
| MethodName::MoveOnAt
| MethodName::FailAfter
| MethodName::FailAt
| MethodName::CancelScope => false,
}
}
}

impl MethodName {
pub(super) fn try_from(call_path: &CallPath<'_>) -> Option<Self> {
match call_path.as_slice() {
["trio", "CancelScope"] => Some(Self::CancelScope),
["trio", "aclose_forcefully"] => Some(Self::AcloseForcefully),
["trio", "fail_after"] => Some(Self::FailAfter),
["trio", "fail_at"] => Some(Self::FailAt),
["trio", "lowlevel", "cancel_shielded_checkpoint"] => {
Some(Self::CancelShieldedCheckpoint)
}
["trio", "lowlevel", "checkpoint"] => Some(Self::Checkpoint),
["trio", "lowlevel", "checkpoint_if_cancelled"] => Some(Self::CheckpointIfCancelled),
["trio", "lowlevel", "open_process"] => Some(Self::OpenProcess),
["trio", "lowlevel", "permanently_detach_coroutine_object"] => {
Some(Self::PermanentlyDetachCoroutineObject)
}
["trio", "lowlevel", "reattach_detached_coroutine_object"] => {
Some(Self::ReattachDetachedCoroutineObject)
}
["trio", "lowlevel", "temporarily_detach_coroutine_object"] => {
Some(Self::TemporarilyDetachCoroutineObject)
}
["trio", "lowlevel", "wait_readable"] => Some(Self::WaitReadable),
["trio", "lowlevel", "wait_task_rescheduled"] => Some(Self::WaitTaskRescheduled),
["trio", "lowlevel", "wait_writable"] => Some(Self::WaitWritable),
["trio", "move_on_after"] => Some(Self::MoveOnAfter),
["trio", "move_on_at"] => Some(Self::MoveOnAt),
["trio", "open_file"] => Some(Self::OpenFile),
["trio", "open_ssl_over_tcp_listeners"] => Some(Self::OpenSslOverTcpListeners),
["trio", "open_ssl_over_tcp_stream"] => Some(Self::OpenSslOverTcpStream),
["trio", "open_tcp_listeners"] => Some(Self::OpenTcpListeners),
["trio", "open_tcp_stream"] => Some(Self::OpenTcpStream),
["trio", "open_unix_socket"] => Some(Self::OpenUnixSocket),
["trio", "run_process"] => Some(Self::RunProcess),
["trio", "serve_listeners"] => Some(Self::ServeListeners),
["trio", "serve_ssl_over_tcp"] => Some(Self::ServeSslOverTcp),
["trio", "serve_tcp"] => Some(Self::ServeTcp),
["trio", "sleep"] => Some(Self::Sleep),
["trio", "sleep_forever"] => Some(Self::SleepForever),
_ => None,
}
}
}

impl std::fmt::Display for MethodName {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
MethodName::AcloseForcefully => write!(f, "trio.aclose_forcefully"),
MethodName::CancelScope => write!(f, "trio.CancelScope"),
MethodName::CancelShieldedCheckpoint => {
write!(f, "trio.lowlevel.cancel_shielded_checkpoint")
}
MethodName::Checkpoint => write!(f, "trio.lowlevel.checkpoint"),
MethodName::CheckpointIfCancelled => write!(f, "trio.lowlevel.checkpoint_if_cancelled"),
MethodName::FailAfter => write!(f, "trio.fail_after"),
MethodName::FailAt => write!(f, "trio.fail_at"),
MethodName::MoveOnAfter => write!(f, "trio.move_on_after"),
MethodName::MoveOnAt => write!(f, "trio.move_on_at"),
MethodName::OpenFile => write!(f, "trio.open_file"),
MethodName::OpenProcess => write!(f, "trio.lowlevel.open_process"),
MethodName::OpenSslOverTcpListeners => write!(f, "trio.open_ssl_over_tcp_listeners"),
MethodName::OpenSslOverTcpStream => write!(f, "trio.open_ssl_over_tcp_stream"),
MethodName::OpenTcpListeners => write!(f, "trio.open_tcp_listeners"),
MethodName::OpenTcpStream => write!(f, "trio.open_tcp_stream"),
MethodName::OpenUnixSocket => write!(f, "trio.open_unix_socket"),
MethodName::PermanentlyDetachCoroutineObject => {
write!(f, "trio.lowlevel.permanently_detach_coroutine_object")
}
MethodName::ReattachDetachedCoroutineObject => {
write!(f, "trio.lowlevel.reattach_detached_coroutine_object")
}
MethodName::RunProcess => write!(f, "trio.run_process"),
MethodName::ServeListeners => write!(f, "trio.serve_listeners"),
MethodName::ServeSslOverTcp => write!(f, "trio.serve_ssl_over_tcp"),
MethodName::ServeTcp => write!(f, "trio.serve_tcp"),
MethodName::Sleep => write!(f, "trio.sleep"),
MethodName::SleepForever => write!(f, "trio.sleep_forever"),
MethodName::TemporarilyDetachCoroutineObject => {
write!(f, "trio.lowlevel.temporarily_detach_coroutine_object")
}
MethodName::WaitReadable => write!(f, "trio.lowlevel.wait_readable"),
MethodName::WaitTaskRescheduled => write!(f, "trio.lowlevel.wait_task_rescheduled"),
MethodName::WaitWritable => write!(f, "trio.lowlevel.wait_writable"),
}
}
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/flake8_trio/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
//! Rules from [flake8-trio](https://pypi.org/project/flake8-trio/).
pub(super) mod method_name;
pub(crate) mod rules;

#[cfg(test)]
Expand All @@ -14,6 +15,7 @@ mod tests {
use crate::test::test_path;

#[test_case(Rule::TrioTimeoutWithoutAwait, Path::new("TRIO100.py"))]
#[test_case(Rule::TrioSyncCall, Path::new("TRIO105.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/flake8_trio/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
pub(crate) use sync_call::*;
pub(crate) use timeout_without_await::*;

mod sync_call;
mod timeout_without_await;
87 changes: 87 additions & 0 deletions crates/ruff_linter/src/rules/flake8_trio/rules/sync_call.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{Expr, ExprCall};
use ruff_text_size::{Ranged, TextRange};

use crate::checkers::ast::Checker;
use crate::fix::edits::pad;
use crate::rules::flake8_trio::method_name::MethodName;

/// ## What it does
/// Checks for calls to trio functions that are not immediately awaited.
///
/// ## Why is this bad?
/// Many of the functions exposed by trio are asynchronous, and must be awaited
/// to take effect. Calling a trio function without an `await` can lead to
/// `RuntimeWarning` diagnostics and unexpected behaviour.
///
/// ## Fix safety
/// This rule's fix is marked as unsafe, as adding an `await` to a function
/// call changes its semantics and runtime behavior.
///
/// ## Example
/// ```python
/// async def double_sleep(x):
/// trio.sleep(2 * x)
/// ```
///
/// Use instead:
/// ```python
/// async def double_sleep(x):
/// await trio.sleep(2 * x)
/// ```
#[violation]
pub struct TrioSyncCall {
method_name: MethodName,
}

impl Violation for TrioSyncCall {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
let Self { method_name } = self;
format!("Call to `{method_name}` is not immediately awaited")
}

fn fix_title(&self) -> Option<String> {
Some(format!("Add `await`"))
}
}

/// TRIO105
pub(crate) fn sync_call(checker: &mut Checker, call: &ExprCall) {
let Some(method_name) = ({
let Some(call_path) = checker.semantic().resolve_call_path(call.func.as_ref()) else {
return;
};
MethodName::try_from(&call_path)
}) else {
return;
};

if !method_name.is_async() {
return;
}

if checker
.semantic()
.current_expression_parent()
.is_some_and(Expr::is_await_expr)
{
return;
};

let mut diagnostic = Diagnostic::new(TrioSyncCall { method_name }, call.range);
if checker.semantic().in_async_context() {
diagnostic.set_fix(Fix::unsafe_edit(Edit::insertion(
pad(
"await".to_string(),
TextRange::new(call.func.start(), call.func.start()),
checker.locator(),
),
call.func.start(),
)));
}
checker.diagnostics.push(diagnostic);
}
Loading

0 comments on commit 4170ef0

Please sign in to comment.