From c75524a787254237a99c2ab169999d05d99dd463 Mon Sep 17 00:00:00 2001 From: harupy Date: Wed, 26 Jul 2023 10:09:42 +0900 Subject: [PATCH 1/4] Fix PT012 --- .../fixtures/flake8_pytest_style/PT012.py | 11 +-- .../rules/flake8_pytest_style/rules/raises.rs | 45 ++++++---- ...es__flake8_pytest_style__tests__PT012.snap | 88 ++++++++----------- 3 files changed, 70 insertions(+), 74 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT012.py b/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT012.py index 21536cf6f611b..050ad053ec2ac 100644 --- a/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT012.py +++ b/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT012.py @@ -11,6 +11,10 @@ async def test_ok_trivial_with(): with context_manager_under_test(): pass + with pytest.raises(ValueError): + with context_manager_under_test(): + raise ValueError + with pytest.raises(AttributeError): async with context_manager_under_test(): pass @@ -47,13 +51,10 @@ async def test_error_complex_statement(): while True: [].size - with pytest.raises(AttributeError): - with context_manager_under_test(): - [].size - with pytest.raises(AttributeError): async with context_manager_under_test(): - [].size + if True: + raise Exception def test_error_try(): diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/raises.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/raises.rs index c4d45e2bd2865..0c55e8f7e6083 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/raises.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/raises.rs @@ -83,11 +83,29 @@ fn is_pytest_raises(func: &Expr, semantic: &SemanticModel) -> bool { }) } +const fn is_compound_statement(stmt: &Stmt) -> bool { + matches!( + stmt, + Stmt::FunctionDef(_) + | Stmt::AsyncFunctionDef(_) + | Stmt::ClassDef(_) + | Stmt::While(_) + | Stmt::For(_) + | Stmt::AsyncFor(_) + | Stmt::Match(_) + | Stmt::With(_) + | Stmt::AsyncWith(_) + | Stmt::If(_) + | Stmt::Try(_) + | Stmt::TryStar(_) + ) +} + const fn is_non_trivial_with_body(body: &[Stmt]) -> bool { if body.len() > 1 { true } else if let Some(first_body_stmt) = body.first() { - !first_body_stmt.is_pass_stmt() + is_compound_statement(first_body_stmt) } else { false } @@ -124,8 +142,6 @@ pub(crate) fn complex_raises( items: &[WithItem], body: &[Stmt], ) { - let mut is_too_complex = false; - let raises_called = items.iter().any(|item| match &item.context_expr { Expr::Call(ast::ExprCall { func, .. }) => is_pytest_raises(func, checker.semantic()), _ => false, @@ -133,28 +149,19 @@ pub(crate) fn complex_raises( // Check body for `pytest.raises` context manager if raises_called { - if body.len() > 1 { - is_too_complex = true; + let is_too_complex = if body.len() > 1 { + true } else if let Some(first_stmt) = body.first() { match first_stmt { Stmt::With(ast::StmtWith { body, .. }) | Stmt::AsyncWith(ast::StmtAsyncWith { body, .. }) => { - if is_non_trivial_with_body(body) { - is_too_complex = true; - } - } - Stmt::If(_) - | Stmt::For(_) - | Stmt::Match(_) - | Stmt::AsyncFor(_) - | Stmt::While(_) - | Stmt::Try(_) - | Stmt::TryStar(_) => { - is_too_complex = true; + is_non_trivial_with_body(body) } - _ => {} + stmt => is_compound_statement(stmt), } - } + } else { + false + }; if is_too_complex { checker.diagnostics.push(Diagnostic::new( diff --git a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT012.snap b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT012.snap index b65c8097db854..006aa5668a267 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT012.snap +++ b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT012.snap @@ -11,51 +11,12 @@ PT012.py:28:5: PT012 `pytest.raises()` block should contain a single simple stat | |_______________^ PT012 | -PT012.py:34:5: PT012 `pytest.raises()` block should contain a single simple statement - | -33 | async def test_error_complex_statement(): -34 | with pytest.raises(AttributeError): - | _____^ -35 | | if True: -36 | | [].size - | |___________________^ PT012 -37 | -38 | with pytest.raises(AttributeError): - | - -PT012.py:38:5: PT012 `pytest.raises()` block should contain a single simple statement - | -36 | [].size -37 | -38 | with pytest.raises(AttributeError): - | _____^ -39 | | for i in []: -40 | | [].size - | |___________________^ PT012 -41 | -42 | with pytest.raises(AttributeError): - | - -PT012.py:42:5: PT012 `pytest.raises()` block should contain a single simple statement - | -40 | [].size -41 | -42 | with pytest.raises(AttributeError): - | _____^ -43 | | async for i in []: -44 | | [].size - | |___________________^ PT012 -45 | -46 | with pytest.raises(AttributeError): - | - PT012.py:46:5: PT012 `pytest.raises()` block should contain a single simple statement | -44 | [].size -45 | +45 | async def test_error_complex_statement(): 46 | with pytest.raises(AttributeError): | _____^ -47 | | while True: +47 | | if True: 48 | | [].size | |___________________^ PT012 49 | @@ -68,7 +29,7 @@ PT012.py:50:5: PT012 `pytest.raises()` block should contain a single simple stat 49 | 50 | with pytest.raises(AttributeError): | _____^ -51 | | with context_manager_under_test(): +51 | | for i in []: 52 | | [].size | |___________________^ PT012 53 | @@ -81,20 +42,47 @@ PT012.py:54:5: PT012 `pytest.raises()` block should contain a single simple stat 53 | 54 | with pytest.raises(AttributeError): | _____^ -55 | | async with context_manager_under_test(): +55 | | async for i in []: 56 | | [].size | |___________________^ PT012 +57 | +58 | with pytest.raises(AttributeError): + | + +PT012.py:58:5: PT012 `pytest.raises()` block should contain a single simple statement + | +56 | [].size +57 | +58 | with pytest.raises(AttributeError): + | _____^ +59 | | while True: +60 | | [].size + | |___________________^ PT012 +61 | +62 | with pytest.raises(AttributeError): + | + +PT012.py:62:5: PT012 `pytest.raises()` block should contain a single simple statement + | +60 | [].size +61 | +62 | with pytest.raises(AttributeError): + | _____^ +63 | | async with context_manager_under_test(): +64 | | if True: +65 | | raise Exception("foo") + | |______________________________________^ PT012 | -PT012.py:60:5: PT012 `pytest.raises()` block should contain a single simple statement +PT012.py:69:5: PT012 `pytest.raises()` block should contain a single simple statement | -59 | def test_error_try(): -60 | with pytest.raises(AttributeError): +68 | def test_error_try(): +69 | with pytest.raises(AttributeError): | _____^ -61 | | try: -62 | | [].size -63 | | except: -64 | | raise +70 | | try: +71 | | [].size +72 | | except: +73 | | raise | |_________________^ PT012 | From b3b7030046760ad00182ce3b704f06a3a95edc90 Mon Sep 17 00:00:00 2001 From: harupy Date: Wed, 26 Jul 2023 10:17:48 +0900 Subject: [PATCH 2/4] update snapshot --- ...es__flake8_pytest_style__tests__PT012.snap | 90 +++++++++---------- 1 file changed, 45 insertions(+), 45 deletions(-) diff --git a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT012.snap b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT012.snap index 006aa5668a267..7d7bcc3881fdd 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT012.snap +++ b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT012.snap @@ -1,22 +1,48 @@ --- source: crates/ruff/src/rules/flake8_pytest_style/mod.rs --- -PT012.py:28:5: PT012 `pytest.raises()` block should contain a single simple statement +PT012.py:32:5: PT012 `pytest.raises()` block should contain a single simple statement | -27 | def test_error_multiple_statements(): -28 | with pytest.raises(AttributeError): +31 | def test_error_multiple_statements(): +32 | with pytest.raises(AttributeError): | _____^ -29 | | len([]) -30 | | [].size +33 | | len([]) +34 | | [].size | |_______________^ PT012 | +PT012.py:38:5: PT012 `pytest.raises()` block should contain a single simple statement + | +37 | async def test_error_complex_statement(): +38 | with pytest.raises(AttributeError): + | _____^ +39 | | if True: +40 | | [].size + | |___________________^ PT012 +41 | +42 | with pytest.raises(AttributeError): + | + +PT012.py:42:5: PT012 `pytest.raises()` block should contain a single simple statement + | +40 | [].size +41 | +42 | with pytest.raises(AttributeError): + | _____^ +43 | | for i in []: +44 | | [].size + | |___________________^ PT012 +45 | +46 | with pytest.raises(AttributeError): + | + PT012.py:46:5: PT012 `pytest.raises()` block should contain a single simple statement | -45 | async def test_error_complex_statement(): +44 | [].size +45 | 46 | with pytest.raises(AttributeError): | _____^ -47 | | if True: +47 | | async for i in []: 48 | | [].size | |___________________^ PT012 49 | @@ -29,7 +55,7 @@ PT012.py:50:5: PT012 `pytest.raises()` block should contain a single simple stat 49 | 50 | with pytest.raises(AttributeError): | _____^ -51 | | for i in []: +51 | | while True: 52 | | [].size | |___________________^ PT012 53 | @@ -42,47 +68,21 @@ PT012.py:54:5: PT012 `pytest.raises()` block should contain a single simple stat 53 | 54 | with pytest.raises(AttributeError): | _____^ -55 | | async for i in []: -56 | | [].size - | |___________________^ PT012 -57 | -58 | with pytest.raises(AttributeError): - | - -PT012.py:58:5: PT012 `pytest.raises()` block should contain a single simple statement - | -56 | [].size -57 | -58 | with pytest.raises(AttributeError): - | _____^ -59 | | while True: -60 | | [].size - | |___________________^ PT012 -61 | -62 | with pytest.raises(AttributeError): - | - -PT012.py:62:5: PT012 `pytest.raises()` block should contain a single simple statement - | -60 | [].size -61 | -62 | with pytest.raises(AttributeError): - | _____^ -63 | | async with context_manager_under_test(): -64 | | if True: -65 | | raise Exception("foo") - | |______________________________________^ PT012 +55 | | async with context_manager_under_test(): +56 | | if True: +57 | | raise Exception + | |_______________________________^ PT012 | -PT012.py:69:5: PT012 `pytest.raises()` block should contain a single simple statement +PT012.py:61:5: PT012 `pytest.raises()` block should contain a single simple statement | -68 | def test_error_try(): -69 | with pytest.raises(AttributeError): +60 | def test_error_try(): +61 | with pytest.raises(AttributeError): | _____^ -70 | | try: -71 | | [].size -72 | | except: -73 | | raise +62 | | try: +63 | | [].size +64 | | except: +65 | | raise | |_________________^ PT012 | From c5cccf2896506452385b070cf1e45ea069e04880 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 25 Jul 2023 21:28:11 -0400 Subject: [PATCH 3/4] Tweak conditions --- .../rules/flake8_pytest_style/rules/raises.rs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/raises.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/raises.rs index 0c55e8f7e6083..42214f11057dc 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/raises.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/raises.rs @@ -102,12 +102,10 @@ const fn is_compound_statement(stmt: &Stmt) -> bool { } const fn is_non_trivial_with_body(body: &[Stmt]) -> bool { - if body.len() > 1 { - true - } else if let Some(first_body_stmt) = body.first() { - is_compound_statement(first_body_stmt) + if let [stmt] = body { + is_compound_statement(stmt) } else { - false + true } } @@ -149,10 +147,8 @@ pub(crate) fn complex_raises( // Check body for `pytest.raises` context manager if raises_called { - let is_too_complex = if body.len() > 1 { - true - } else if let Some(first_stmt) = body.first() { - match first_stmt { + let is_too_complex = if let [stmt] = body { + match stmt { Stmt::With(ast::StmtWith { body, .. }) | Stmt::AsyncWith(ast::StmtAsyncWith { body, .. }) => { is_non_trivial_with_body(body) @@ -160,7 +156,7 @@ pub(crate) fn complex_raises( stmt => is_compound_statement(stmt), } } else { - false + true }; if is_too_complex { From 0dbc5b33f42a1121313aebd4e16e3474b0826e74 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 25 Jul 2023 21:31:35 -0400 Subject: [PATCH 4/4] Move is_compound_statement --- .../rules/flake8_pytest_style/rules/raises.rs | 20 +------------------ crates/ruff_python_ast/src/helpers.rs | 19 ++++++++++++++++++ .../src/statement/suite.rs | 19 +----------------- 3 files changed, 21 insertions(+), 37 deletions(-) diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/raises.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/raises.rs index 42214f11057dc..50766758fc9d5 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/raises.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/raises.rs @@ -1,4 +1,4 @@ -use ruff_python_ast::helpers::find_keyword; +use ruff_python_ast::helpers::{find_keyword, is_compound_statement}; use rustpython_parser::ast::{self, Expr, Keyword, Ranged, Stmt, WithItem}; use ruff_diagnostics::{Diagnostic, Violation}; @@ -83,24 +83,6 @@ fn is_pytest_raises(func: &Expr, semantic: &SemanticModel) -> bool { }) } -const fn is_compound_statement(stmt: &Stmt) -> bool { - matches!( - stmt, - Stmt::FunctionDef(_) - | Stmt::AsyncFunctionDef(_) - | Stmt::ClassDef(_) - | Stmt::While(_) - | Stmt::For(_) - | Stmt::AsyncFor(_) - | Stmt::Match(_) - | Stmt::With(_) - | Stmt::AsyncWith(_) - | Stmt::If(_) - | Stmt::Try(_) - | Stmt::TryStar(_) - ) -} - const fn is_non_trivial_with_body(body: &[Stmt]) -> bool { if let [stmt] = body { is_compound_statement(stmt) diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 176845502779a..202989d4c29a5 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -18,6 +18,25 @@ use crate::call_path::CallPath; use crate::source_code::{Indexer, Locator}; use crate::statement_visitor::{walk_body, walk_stmt, StatementVisitor}; +/// Return `true` if the `Stmt` is a compound statement (as opposed to a simple statement). +pub const fn is_compound_statement(stmt: &Stmt) -> bool { + matches!( + stmt, + Stmt::FunctionDef(_) + | Stmt::AsyncFunctionDef(_) + | Stmt::ClassDef(_) + | Stmt::While(_) + | Stmt::For(_) + | Stmt::AsyncFor(_) + | Stmt::Match(_) + | Stmt::With(_) + | Stmt::AsyncWith(_) + | Stmt::If(_) + | Stmt::Try(_) + | Stmt::TryStar(_) + ) +} + fn is_iterable_initializer(id: &str, is_builtin: F) -> bool where F: Fn(&str) -> bool, diff --git a/crates/ruff_python_formatter/src/statement/suite.rs b/crates/ruff_python_formatter/src/statement/suite.rs index 3d6ec81953190..80a61a8a69c24 100644 --- a/crates/ruff_python_formatter/src/statement/suite.rs +++ b/crates/ruff_python_formatter/src/statement/suite.rs @@ -3,6 +3,7 @@ use rustpython_parser::ast::{Ranged, Stmt, Suite}; use ruff_formatter::{ format_args, write, FormatOwnedWithRule, FormatRefWithRule, FormatRuleWithOptions, }; +use ruff_python_ast::helpers::is_compound_statement; use ruff_python_trivia::lines_before; use crate::context::NodeLevel; @@ -142,24 +143,6 @@ const fn is_class_or_function_definition(stmt: &Stmt) -> bool { ) } -const fn is_compound_statement(stmt: &Stmt) -> bool { - matches!( - stmt, - Stmt::FunctionDef(_) - | Stmt::AsyncFunctionDef(_) - | Stmt::ClassDef(_) - | Stmt::While(_) - | Stmt::For(_) - | Stmt::AsyncFor(_) - | Stmt::Match(_) - | Stmt::With(_) - | Stmt::AsyncWith(_) - | Stmt::If(_) - | Stmt::Try(_) - | Stmt::TryStar(_) - ) -} - impl FormatRuleWithOptions> for FormatSuite { type Options = SuiteLevel;