From bcb447c5245becd8750841c01f1903020dd0eed4 Mon Sep 17 00:00:00 2001 From: Malte Vesper Date: Tue, 21 Nov 2023 18:58:33 +1300 Subject: [PATCH 1/2] Omit select context managers from SIM117 (#8606) Semantically it makes sense to put certain contextmanagers into separate with statements. Currently asyncio.timeout and its relatives in anyio and trio are exempt from SIM117. Closes https://github.com/astral-sh/ruff/issues/8606 --- .../test/fixtures/flake8_simplify/SIM117.py | 29 ++++++++++++++ .../rules/flake8_simplify/rules/ast_with.rs | 40 +++++++++++++++++++ ...ke8_simplify__tests__SIM117_SIM117.py.snap | 23 +++++++++++ 3 files changed, 92 insertions(+) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM117.py b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM117.py index c3c64044a2aac..ee0234709f8e0 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM117.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM117.py @@ -134,3 +134,32 @@ def method1(self) -> T: f" something { my_dict["key"] } something else " f"foo {f"bar {x}"} baz" + +# Allow cascading for some statements +import anyio +import asyncio +import trio + +async with asyncio.timeout(1): + async with A() as a: + pass + +async with A(): + async with asyncio.timeout(1): + pass + +async with asyncio.timeout(1): + async with asyncio.timeout_at(1): + async with anyio.CancelScope(): + async with anyio.fail_after(1): + async with anyio.move_on_after(1): + async with trio.fail_after(1): + async with trio.fail_at(1): + async with trio.move_on_after(1): + async with trio.move_on_at(1): + pass + +# Do not surpress combination, if with_item is alreayd combined with another item +async with asyncio.timeout(1), A(): + async with B(): + pass \ No newline at end of file diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs index e3dedd26a477c..7a060e1f95df6 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs @@ -1,3 +1,4 @@ +use ast::Expr; use log::error; use ruff_diagnostics::{Diagnostic, Fix}; @@ -16,6 +17,12 @@ use super::fix_with; /// Checks for the unnecessary nesting of multiple consecutive context /// managers. /// +/// The following context managers are exempt if used standalone: +/// +/// - `anyio`.{`CancelScope`, `fail_after`, `move_on_after`} +/// - `asyncio`.{`timeout`, `timeout_at`} +/// - `trio`.{`fail_after`, `fail_at`, `move_on_after`, `move_on_at`} +/// /// ## Why is this bad? /// In Python 3, a single `with` block can include multiple context /// managers. @@ -73,6 +80,35 @@ fn next_with(body: &[Stmt]) -> Option<(bool, &[WithItem], &[Stmt])> { Some((*is_async, items, body)) } +/// Check if `with_items` contains a single item which should not necessarily be +/// grouped with other items +/// +/// async with asyncio.timeout(1): # timeout should stand out +/// with resource1(), resource2(): +/// ... +fn explicit_with_items(checker: &mut Checker, with_items: &[WithItem]) -> bool { + match with_items { + [with_item] => match &with_item.context_expr { + Expr::Call(expr_call) => checker + .semantic() + .resolve_call_path(&expr_call.func) + .is_some_and(|call_path| { + matches!( + call_path.as_slice(), + ["asyncio", "timeout" | "timeout_at"] + | ["anyio", "CancelScope" | "fail_after" | "move_on_after"] + | [ + "trio", + "fail_after" | "fail_at" | "move_on_after" | "move_on_at" + ] + ) + }), + _ => false, + }, + _ => false, + } +} + /// SIM117 pub(crate) fn multiple_with_statements( checker: &mut Checker, @@ -111,6 +147,10 @@ pub(crate) fn multiple_with_statements( return; } + if explicit_with_items(checker, &with_stmt.items) || explicit_with_items(checker, items) { + return; + } + let Some(colon) = items.last().and_then(|item| { SimpleTokenizer::starts_at(item.end(), checker.locator().contents()) .skip_trivia() diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM117_SIM117.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM117_SIM117.py.snap index 5b71ad0883a2a..c08ee4d949e55 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM117_SIM117.py.snap +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM117_SIM117.py.snap @@ -316,5 +316,28 @@ SIM117.py:126:1: SIM117 [*] Use a single `with` statement with multiple contexts 135 134 | 136 |- f"foo {f"bar {x}"} baz" 135 |+ f"foo {f"bar {x}"} baz" +137 136 | +138 137 | # Allow cascading for some statements +139 138 | import anyio + +SIM117.py:163:1: SIM117 [*] Use a single `with` statement with multiple contexts instead of nested `with` statements + | +162 | # Do not surpress combination, if with_item is alreayd combined with another item +163 | / async with asyncio.timeout(1), A(): +164 | | async with B(): + | |___________________^ SIM117 +165 | pass + | + = help: Combine `with` statements + +ℹ Unsafe fix +160 160 | pass +161 161 | +162 162 | # Do not surpress combination, if with_item is alreayd combined with another item +163 |-async with asyncio.timeout(1), A(): +164 |- async with B(): +165 |- pass + 163 |+async with asyncio.timeout(1), A(), B(): + 164 |+ pass From d2aec512fad26dc9de7cee88e770b2aee9349025 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 21 Nov 2023 11:49:37 +0000 Subject: [PATCH 2/2] Use let-else --- .../test/fixtures/flake8_simplify/SIM117.py | 6 +- .../rules/flake8_simplify/rules/ast_with.rs | 60 ++++++++++--------- ...ke8_simplify__tests__SIM117_SIM117.py.snap | 6 +- 3 files changed, 38 insertions(+), 34 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM117.py b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM117.py index ee0234709f8e0..43c8c8a12b4ab 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM117.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM117.py @@ -135,7 +135,7 @@ def method1(self) -> T: f"foo {f"bar {x}"} baz" -# Allow cascading for some statements +# Allow cascading for some statements. import anyio import asyncio import trio @@ -159,7 +159,7 @@ def method1(self) -> T: async with trio.move_on_at(1): pass -# Do not surpress combination, if with_item is alreayd combined with another item +# Do not supress combination, if a context manager is already combined with another. async with asyncio.timeout(1), A(): async with B(): - pass \ No newline at end of file + pass diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs index 7a060e1f95df6..fd1afd9d468db 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs @@ -17,12 +17,6 @@ use super::fix_with; /// Checks for the unnecessary nesting of multiple consecutive context /// managers. /// -/// The following context managers are exempt if used standalone: -/// -/// - `anyio`.{`CancelScope`, `fail_after`, `move_on_after`} -/// - `asyncio`.{`timeout`, `timeout_at`} -/// - `trio`.{`fail_after`, `fail_at`, `move_on_after`, `move_on_at`} -/// /// ## Why is this bad? /// In Python 3, a single `with` block can include multiple context /// managers. @@ -31,6 +25,13 @@ use super::fix_with; /// will minimize the indentation depth of the code, making it more /// readable. /// +/// The following context managers are exempt when used as standalone +/// statements: +/// +/// - `anyio`.{`CancelScope`, `fail_after`, `move_on_after`} +/// - `asyncio`.{`timeout`, `timeout_at`} +/// - `trio`.{`fail_after`, `fail_at`, `move_on_after`, `move_on_at`} +/// /// ## Example /// ```python /// with A() as a: @@ -81,32 +82,35 @@ fn next_with(body: &[Stmt]) -> Option<(bool, &[WithItem], &[Stmt])> { } /// Check if `with_items` contains a single item which should not necessarily be -/// grouped with other items +/// grouped with other items. /// -/// async with asyncio.timeout(1): # timeout should stand out +/// For example: +/// ```python +/// async with asyncio.timeout(1): /// with resource1(), resource2(): /// ... +/// ``` fn explicit_with_items(checker: &mut Checker, with_items: &[WithItem]) -> bool { - match with_items { - [with_item] => match &with_item.context_expr { - Expr::Call(expr_call) => checker - .semantic() - .resolve_call_path(&expr_call.func) - .is_some_and(|call_path| { - matches!( - call_path.as_slice(), - ["asyncio", "timeout" | "timeout_at"] - | ["anyio", "CancelScope" | "fail_after" | "move_on_after"] - | [ - "trio", - "fail_after" | "fail_at" | "move_on_after" | "move_on_at" - ] - ) - }), - _ => false, - }, - _ => false, - } + let [with_item] = with_items else { + return false; + }; + let Expr::Call(expr_call) = &with_item.context_expr else { + return false; + }; + checker + .semantic() + .resolve_call_path(&expr_call.func) + .is_some_and(|call_path| { + matches!( + call_path.as_slice(), + ["asyncio", "timeout" | "timeout_at"] + | ["anyio", "CancelScope" | "fail_after" | "move_on_after"] + | [ + "trio", + "fail_after" | "fail_at" | "move_on_after" | "move_on_at" + ] + ) + }) } /// SIM117 diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM117_SIM117.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM117_SIM117.py.snap index c08ee4d949e55..21af88e707913 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM117_SIM117.py.snap +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM117_SIM117.py.snap @@ -317,12 +317,12 @@ SIM117.py:126:1: SIM117 [*] Use a single `with` statement with multiple contexts 136 |- f"foo {f"bar {x}"} baz" 135 |+ f"foo {f"bar {x}"} baz" 137 136 | -138 137 | # Allow cascading for some statements +138 137 | # Allow cascading for some statements. 139 138 | import anyio SIM117.py:163:1: SIM117 [*] Use a single `with` statement with multiple contexts instead of nested `with` statements | -162 | # Do not surpress combination, if with_item is alreayd combined with another item +162 | # Do not supress combination, if a context manager is already combined with another. 163 | / async with asyncio.timeout(1), A(): 164 | | async with B(): | |___________________^ SIM117 @@ -333,7 +333,7 @@ SIM117.py:163:1: SIM117 [*] Use a single `with` statement with multiple contexts ℹ Unsafe fix 160 160 | pass 161 161 | -162 162 | # Do not surpress combination, if with_item is alreayd combined with another item +162 162 | # Do not supress combination, if a context manager is already combined with another. 163 |-async with asyncio.timeout(1), A(): 164 |- async with B(): 165 |- pass