From 9fb4df12681c6043b7cf690d60a50f439d7d3579 Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Fri, 10 Feb 2023 20:04:45 +0100 Subject: [PATCH] autofix for composite assertion --- README.md | 2 +- .../fixtures/flake8_pytest_style/PT018.py | 13 +- src/checkers/ast.rs | 2 +- .../flake8_pytest_style/rules/assertion.rs | 145 ++++++++++++--- ...es__flake8_pytest_style__tests__PT018.snap | 171 ++++++++++++++++-- src/violations.rs | 6 +- 6 files changed, 295 insertions(+), 44 deletions(-) diff --git a/README.md b/README.md index 26a689e5214422..0e35540acfd466 100644 --- a/README.md +++ b/README.md @@ -1100,7 +1100,7 @@ For more, see [flake8-pytest-style](https://pypi.org/project/flake8-pytest-style | PT015 | assert-always-false | Assertion always fails, replace with `pytest.fail()` | | | PT016 | fail-without-message | No message passed to `pytest.fail()` | | | PT017 | assert-in-except | Found assertion on exception `{name}` in except block, use `pytest.raises()` instead | | -| PT018 | composite-assertion | Assertion should be broken down into multiple parts | | +| PT018 | composite-assertion | Assertion should be broken down into multiple parts | 🛠 | | PT019 | fixture-param-without-value | Fixture `{name}` without value is injected as parameter, use `@pytest.mark.usefixtures` instead | | | PT020 | deprecated-yield-fixture | `@pytest.yield_fixture` is deprecated, use `@pytest.fixture` | | | PT021 | fixture-finalizer-callback | Use `yield` instead of `request.addfinalizer` | | diff --git a/resources/test/fixtures/flake8_pytest_style/PT018.py b/resources/test/fixtures/flake8_pytest_style/PT018.py index 4fc893abcb214f..e94c5d1ec79be7 100644 --- a/resources/test/fixtures/flake8_pytest_style/PT018.py +++ b/resources/test/fixtures/flake8_pytest_style/PT018.py @@ -6,13 +6,24 @@ def test_ok(): assert something or something_else assert something or something_else and something_third assert not (something and something_else) - + assert something, "something message" + assert something or something_else and something_third, "another message" def test_error(): assert something and something_else assert something and something_else and something_third assert something and not something_else assert something and (something_else or something_third) + assert not something and something_else assert not (something or something_else) assert not (something or something_else or something_third) + + # recursive case + assert not (a or not (b or c)) + assert not (a or not (b and c)) # note that we only reduce once here + + # detected, but no autofix for messages + assert something and something_else, "error message" + assert not (something or something_else and something_third), "with message" + # detected, but no autofix for mixed conditions (e.g. `a or b and c`) assert not (something or something_else and something_third) diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index 6a25cfe2c02d32..b8237229c064be 100644 --- a/src/checkers/ast.rs +++ b/src/checkers/ast.rs @@ -1495,7 +1495,7 @@ where } if self.settings.rules.enabled(&Rule::CompositeAssertion) { if let Some(diagnostic) = - flake8_pytest_style::rules::composite_condition(stmt, test) + flake8_pytest_style::rules::composite_condition(self, stmt, test) { self.diagnostics.push(diagnostic); } diff --git a/src/rules/flake8_pytest_style/rules/assertion.rs b/src/rules/flake8_pytest_style/rules/assertion.rs index 049b678c8e1bb6..52e560780c2bd2 100644 --- a/src/rules/flake8_pytest_style/rules/assertion.rs +++ b/src/rules/flake8_pytest_style/rules/assertion.rs @@ -1,16 +1,18 @@ use rustpython_ast::{ - Boolop, Excepthandler, ExcepthandlerKind, Expr, ExprKind, Keyword, Stmt, StmtKind, Unaryop, + Boolop, Excepthandler, ExcepthandlerKind, Expr, ExprKind, Keyword, Located, Stmt, StmtKind, + Unaryop, }; use super::helpers::is_falsy_constant; use super::unittest_assert::UnittestAssert; -use crate::ast::helpers::unparse_stmt; +use crate::ast::helpers::{create_expr, create_stmt, unparse_stmt}; use crate::ast::types::Range; use crate::ast::visitor; use crate::ast::visitor::Visitor; use crate::checkers::ast::Checker; use crate::fix::Fix; use crate::registry::Diagnostic; +use crate::source_code::Stylist; use crate::violations; /// Visitor that tracks assert statements and checks if they reference @@ -65,22 +67,6 @@ where } } -/// Check if the test expression is a composite condition. -/// For example, `a and b` or `not (a or b)`. The latter is equivalent -/// to `not a and not b` by De Morgan's laws. -fn is_composite_condition(test: &Expr) -> bool { - match &test.node { - ExprKind::BoolOp { - op: Boolop::And, .. - } => true, - ExprKind::UnaryOp { - op: Unaryop::Not, - operand, - } => matches!(&operand.node, ExprKind::BoolOp { op: Boolop::Or, .. }), - _ => false, - } -} - fn check_assert_in_except(name: &str, body: &[Stmt]) -> Vec { // Walk body to find assert statements that reference the exception name let mut visitor = ExceptionHandlerVisitor::new(name); @@ -153,13 +139,130 @@ pub fn assert_in_exception_handler(handlers: &[Excepthandler]) -> Vec bool { + match &test.node { + ExprKind::BoolOp { + op: Boolop::And, .. + } => true, + ExprKind::UnaryOp { + op: Unaryop::Not, + operand, + } => matches!(&operand.node, ExprKind::BoolOp { op: Boolop::Or, .. }), + _ => false, + } +} + +/// Negate condition, i.e. `a` => `not a` and `not a` => `a` +pub fn negate(f: Located) -> Located { + match f.node { + ExprKind::UnaryOp { + op: Unaryop::Not, + operand, + } => *operand, + _ => create_expr(ExprKind::UnaryOp { + op: Unaryop::Not, + operand: Box::new(f), + }), + } +} + +/// Replace composite condition `assert a == "hello" and b == "world"` with two statements +/// `assert a == "hello"` and `assert b == "world"`. +pub fn fix_composite_condition(stylist: &Stylist, assert: &Stmt, test: &Expr) -> Option { + // We do not split compounds if there is a message + if let StmtKind::Assert { msg: Some(_), .. } = &assert.node { + return None; + } + + let mut conditions: Vec> = vec![]; + match &test.node { + ExprKind::BoolOp { + op: Boolop::And, + values, + } => { + // Compound, so split (Split) + conditions.extend(values.clone()); + } + ExprKind::UnaryOp { + op: Unaryop::Not, + operand, + } => { + match &operand.node { + ExprKind::BoolOp { + op: Boolop::Or, + values, + } => { + // Only take cases without mixed `and` and `or` + if !values.iter().fold(true, |acc, mk| { + acc && match mk.node { + ExprKind::BoolOp { + op: Boolop::And, .. + } => false, + _ => true, + } + }) { + return None; + } + + // `not (a or b)` equals `not a and not b` + let vals = values + .iter() + .map(|f| negate(f.clone())) + .collect::>>(); + + // Compound, so split (Split) + conditions.extend(vals); + } + _ => { + // Do not split + conditions.push(*operand.clone()); + } + } + } + _ => {} + }; + + // for each condition create `assert condition` + let mut content: Vec = Vec::with_capacity(conditions.len()); + for condition in conditions { + content.push(unparse_stmt( + &create_stmt(StmtKind::Assert { + test: Box::new(condition.clone()), + msg: None, + }), + stylist, + )); + } + + let content = content.join(stylist.line_ending().as_str()); + + Some(Fix::replacement( + content, + assert.location, + assert.end_location.unwrap(), + )) +} + /// PT018 -pub fn composite_condition(assert_stmt: &Stmt, test_expr: &Expr) -> Option { +pub fn composite_condition( + checker: &mut Checker, + assert_stmt: &Stmt, + test_expr: &Expr, +) -> Option { if is_composite_condition(test_expr) { - Some(Diagnostic::new( + let mut diagnostic = Diagnostic::new( violations::CompositeAssertion, Range::from_located(assert_stmt), - )) + ); + if checker.patch(diagnostic.kind.rule()) { + if let Some(fix) = fix_composite_condition(checker.stylist, assert_stmt, test_expr) { + diagnostic.amend(fix); + } + } + Some(diagnostic) } else { None } diff --git a/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT018.snap b/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT018.snap index fa5ba420680ad5..e670fb1d6612d9 100644 --- a/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT018.snap +++ b/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT018.snap @@ -5,70 +5,203 @@ expression: diagnostics - kind: CompositeAssertion: ~ location: - row: 12 + row: 13 column: 4 end_location: - row: 12 + row: 13 column: 39 - fix: ~ + fix: + content: + - assert something + - assert something_else + location: + row: 13 + column: 4 + end_location: + row: 13 + column: 39 parent: ~ - kind: CompositeAssertion: ~ location: - row: 13 + row: 14 column: 4 end_location: - row: 13 + row: 14 column: 59 - fix: ~ + fix: + content: + - assert something + - assert something_else + - assert something_third + location: + row: 14 + column: 4 + end_location: + row: 14 + column: 59 parent: ~ - kind: CompositeAssertion: ~ location: - row: 14 + row: 15 column: 4 end_location: - row: 14 + row: 15 column: 43 - fix: ~ + fix: + content: + - assert something + - assert not something_else + location: + row: 15 + column: 4 + end_location: + row: 15 + column: 43 parent: ~ - kind: CompositeAssertion: ~ location: - row: 15 + row: 16 column: 4 end_location: - row: 15 + row: 16 column: 60 - fix: ~ + fix: + content: + - assert something + - assert something_else or something_third + location: + row: 16 + column: 4 + end_location: + row: 16 + column: 60 parent: ~ - kind: CompositeAssertion: ~ location: - row: 16 + row: 17 column: 4 end_location: - row: 16 + row: 17 + column: 43 + fix: + content: + - assert not something + - assert something_else + location: + row: 17 + column: 4 + end_location: + row: 17 + column: 43 + parent: ~ +- kind: + CompositeAssertion: ~ + location: + row: 18 + column: 4 + end_location: + row: 18 column: 44 - fix: ~ + fix: + content: + - assert not something + - assert not something_else + location: + row: 18 + column: 4 + end_location: + row: 18 + column: 44 parent: ~ - kind: CompositeAssertion: ~ location: - row: 17 + row: 19 column: 4 end_location: - row: 17 + row: 19 column: 63 + fix: + content: + - assert not something + - assert not something_else + - assert not something_third + location: + row: 19 + column: 4 + end_location: + row: 19 + column: 63 + parent: ~ +- kind: + CompositeAssertion: ~ + location: + row: 22 + column: 4 + end_location: + row: 22 + column: 34 + fix: + content: + - assert not a + - assert b or c + location: + row: 22 + column: 4 + end_location: + row: 22 + column: 34 + parent: ~ +- kind: + CompositeAssertion: ~ + location: + row: 23 + column: 4 + end_location: + row: 23 + column: 35 + fix: + content: + - assert not a + - assert b and c + location: + row: 23 + column: 4 + end_location: + row: 23 + column: 35 + parent: ~ +- kind: + CompositeAssertion: ~ + location: + row: 26 + column: 4 + end_location: + row: 26 + column: 56 fix: ~ parent: ~ - kind: CompositeAssertion: ~ location: - row: 18 + row: 27 column: 4 end_location: - row: 18 + row: 27 + column: 80 + fix: ~ + parent: ~ +- kind: + CompositeAssertion: ~ + location: + row: 29 + column: 4 + end_location: + row: 29 column: 64 fix: ~ parent: ~ diff --git a/src/violations.rs b/src/violations.rs index b6bb49ea4b9338..47de7f66b15bd6 100644 --- a/src/violations.rs +++ b/src/violations.rs @@ -4730,11 +4730,15 @@ impl Violation for AssertInExcept { define_violation!( pub struct CompositeAssertion; ); -impl Violation for CompositeAssertion { +impl AlwaysAutofixableViolation for CompositeAssertion { #[derive_message_formats] fn message(&self) -> String { format!("Assertion should be broken down into multiple parts") } + + fn autofix_title(&self) -> String { + "Break down assertion into multiple parts".to_string() + } } define_violation!(