From 664e645077facf9cd2c37cf2e3909d8dfafc8531 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Wed, 14 Aug 2024 14:13:11 -0500 Subject: [PATCH 1/4] update fixture and snapshot --- .../test/fixtures/flake8_async/ASYNC100.py | 13 +++++++++++++ ...__flake8_async__tests__ASYNC100_ASYNC100.py.snap | 2 ++ 2 files changed, 15 insertions(+) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC100.py b/crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC100.py index 0ccdf30a0468d..bdd490c841fef 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC100.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC100.py @@ -89,3 +89,16 @@ async def func(): async def func(): async with asyncio.timeout(delay=0.2), asyncio.timeout(delay=0.2): ... + +# Don't trigger for blocks with a yield statement +async def foo(): + with trio.fail_after(1): + yield + +# https://github.com/astral-sh/ruff/issues/12873 +@asynccontextmanager +async def good_code(): + with anyio.fail_after(10): + # There's no await keyword here, but we presume that there + # will be in the caller we yield to, so this is safe. + yield \ No newline at end of file diff --git a/crates/ruff_linter/src/rules/flake8_async/snapshots/ruff_linter__rules__flake8_async__tests__ASYNC100_ASYNC100.py.snap b/crates/ruff_linter/src/rules/flake8_async/snapshots/ruff_linter__rules__flake8_async__tests__ASYNC100_ASYNC100.py.snap index 0eca205a5b468..b3e3b067d5ff2 100644 --- a/crates/ruff_linter/src/rules/flake8_async/snapshots/ruff_linter__rules__flake8_async__tests__ASYNC100_ASYNC100.py.snap +++ b/crates/ruff_linter/src/rules/flake8_async/snapshots/ruff_linter__rules__flake8_async__tests__ASYNC100_ASYNC100.py.snap @@ -98,4 +98,6 @@ ASYNC100.py:90:5: ASYNC100 A `with asyncio.timeout(...):` context does not conta | _____^ 91 | | ... | |___________^ ASYNC100 +92 | +93 | # Don't trigger for blocks with a yield statement | From 8630f82d71677c591d7f0d76406ba52f99ae66ed Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Wed, 14 Aug 2024 14:13:16 -0500 Subject: [PATCH 2/4] update rule --- .../rules/cancel_scope_no_checkpoint.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/crates/ruff_linter/src/rules/flake8_async/rules/cancel_scope_no_checkpoint.rs b/crates/ruff_linter/src/rules/flake8_async/rules/cancel_scope_no_checkpoint.rs index 26a5297ce3911..549dbadf9b95c 100644 --- a/crates/ruff_linter/src/rules/flake8_async/rules/cancel_scope_no_checkpoint.rs +++ b/crates/ruff_linter/src/rules/flake8_async/rules/cancel_scope_no_checkpoint.rs @@ -10,6 +10,9 @@ use crate::rules::flake8_async::helpers::MethodName; /// ## What it does /// Checks for timeout context managers which do not contain a checkpoint. /// +/// For the purposes of this check, `yield` is considered a checkpoint, +/// since checkpoints may occur in the caller to which we yield. +/// /// ## Why is this bad? /// Some asynchronous context managers, such as `asyncio.timeout` and /// `trio.move_on_after`, have no effect unless they contain a checkpoint. @@ -80,6 +83,17 @@ pub(crate) fn cancel_scope_no_checkpoint( return; } + // Treat yields as checkpoints, since checkpoints can happen + // in the caller yielded to. + // https://flake8-async.readthedocs.io/en/latest/rules.html#async100 + // https://github.com/astral-sh/ruff/issues/12873 + if with_stmt.body.iter().any(|stmt| { + stmt.as_expr_stmt() + .is_some_and(|expr| expr.value.is_yield_expr()) + }) { + return; + } + // If the body contains an `await` statement, the context manager is used correctly. let mut visitor = AwaitVisitor::default(); visitor.visit_body(&with_stmt.body); From 8f424b39f702560506797e714e9b6809fc75bb0a Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Wed, 14 Aug 2024 15:24:32 -0500 Subject: [PATCH 3/4] recursively search for yield --- .../resources/test/fixtures/flake8_async/ASYNC100.py | 7 +++++++ .../flake8_async/rules/cancel_scope_no_checkpoint.rs | 9 +++------ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC100.py b/crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC100.py index bdd490c841fef..cdd271267be5d 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC100.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC100.py @@ -95,6 +95,13 @@ async def foo(): with trio.fail_after(1): yield +async def foo(): # even if only one branch contains a yield, we skip the lint + with trio.fail_after(1): + if something: + ... + else: + yield + # https://github.com/astral-sh/ruff/issues/12873 @asynccontextmanager async def good_code(): diff --git a/crates/ruff_linter/src/rules/flake8_async/rules/cancel_scope_no_checkpoint.rs b/crates/ruff_linter/src/rules/flake8_async/rules/cancel_scope_no_checkpoint.rs index 549dbadf9b95c..132f31fcab5a2 100644 --- a/crates/ruff_linter/src/rules/flake8_async/rules/cancel_scope_no_checkpoint.rs +++ b/crates/ruff_linter/src/rules/flake8_async/rules/cancel_scope_no_checkpoint.rs @@ -1,8 +1,8 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::helpers::AwaitVisitor; +use ruff_python_ast::helpers::{any_over_body, AwaitVisitor}; use ruff_python_ast::visitor::Visitor; -use ruff_python_ast::{StmtWith, WithItem}; +use ruff_python_ast::{Expr, StmtWith, WithItem}; use crate::checkers::ast::Checker; use crate::rules::flake8_async::helpers::MethodName; @@ -87,10 +87,7 @@ pub(crate) fn cancel_scope_no_checkpoint( // in the caller yielded to. // https://flake8-async.readthedocs.io/en/latest/rules.html#async100 // https://github.com/astral-sh/ruff/issues/12873 - if with_stmt.body.iter().any(|stmt| { - stmt.as_expr_stmt() - .is_some_and(|expr| expr.value.is_yield_expr()) - }) { + if any_over_body(&with_stmt.body, &Expr::is_yield_expr) { return; } From faf47ae8c5d3758a362005d7a8f24c3409252efd Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 14 Aug 2024 20:57:53 -0400 Subject: [PATCH 4/4] Minor tweaks --- .../resources/test/fixtures/flake8_async/ASYNC100.py | 7 +++++-- .../rules/flake8_async/rules/cancel_scope_no_checkpoint.rs | 4 ++-- ...__rules__flake8_async__tests__ASYNC100_ASYNC100.py.snap | 2 -- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC100.py b/crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC100.py index cdd271267be5d..8434073d22dac 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC100.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC100.py @@ -89,12 +89,14 @@ async def func(): async def func(): async with asyncio.timeout(delay=0.2), asyncio.timeout(delay=0.2): ... - + + # Don't trigger for blocks with a yield statement async def foo(): with trio.fail_after(1): yield + async def foo(): # even if only one branch contains a yield, we skip the lint with trio.fail_after(1): if something: @@ -102,10 +104,11 @@ async def foo(): # even if only one branch contains a yield, we skip the lint else: yield + # https://github.com/astral-sh/ruff/issues/12873 @asynccontextmanager async def good_code(): with anyio.fail_after(10): # There's no await keyword here, but we presume that there # will be in the caller we yield to, so this is safe. - yield \ No newline at end of file + yield diff --git a/crates/ruff_linter/src/rules/flake8_async/rules/cancel_scope_no_checkpoint.rs b/crates/ruff_linter/src/rules/flake8_async/rules/cancel_scope_no_checkpoint.rs index 132f31fcab5a2..6b0b55b014654 100644 --- a/crates/ruff_linter/src/rules/flake8_async/rules/cancel_scope_no_checkpoint.rs +++ b/crates/ruff_linter/src/rules/flake8_async/rules/cancel_scope_no_checkpoint.rs @@ -85,8 +85,8 @@ pub(crate) fn cancel_scope_no_checkpoint( // Treat yields as checkpoints, since checkpoints can happen // in the caller yielded to. - // https://flake8-async.readthedocs.io/en/latest/rules.html#async100 - // https://github.com/astral-sh/ruff/issues/12873 + // See: https://flake8-async.readthedocs.io/en/latest/rules.html#async100 + // See: https://github.com/astral-sh/ruff/issues/12873 if any_over_body(&with_stmt.body, &Expr::is_yield_expr) { return; } diff --git a/crates/ruff_linter/src/rules/flake8_async/snapshots/ruff_linter__rules__flake8_async__tests__ASYNC100_ASYNC100.py.snap b/crates/ruff_linter/src/rules/flake8_async/snapshots/ruff_linter__rules__flake8_async__tests__ASYNC100_ASYNC100.py.snap index b3e3b067d5ff2..0eca205a5b468 100644 --- a/crates/ruff_linter/src/rules/flake8_async/snapshots/ruff_linter__rules__flake8_async__tests__ASYNC100_ASYNC100.py.snap +++ b/crates/ruff_linter/src/rules/flake8_async/snapshots/ruff_linter__rules__flake8_async__tests__ASYNC100_ASYNC100.py.snap @@ -98,6 +98,4 @@ ASYNC100.py:90:5: ASYNC100 A `with asyncio.timeout(...):` context does not conta | _____^ 91 | | ... | |___________^ ASYNC100 -92 | -93 | # Don't trigger for blocks with a yield statement |