[flake8-pytest-style] autofix for composite-assertion (PT018)

README.md

@@ -1174,7 +1174,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](https://github.com/charliermarsh/ruff/blob/main/docs/rules/composite-assertion.md) | 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` |  |

crates/ruff/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)

crates/ruff/src/checkers/ast.rs

@@ -1547,7 +1547,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);
                     }

crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs

@@ -1,27 +1,60 @@
 use ruff_macros::{define_violation, derive_message_formats};
 use rustpython_parser::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::violation::{AlwaysAutofixableViolation, Violation};
+use crate::source_code::Stylist;
+use crate::violation::{AlwaysAutofixableViolation, AutofixKind, Availability, Violation};
 
 define_violation!(
+    /// ## What it does
+    /// This violation is reported when the plugin encounter an assertion on multiple conditions.
+    ///
+    /// ## Why is this bad?
+    /// Composite assertion statements are harder to understand and to debug when failures occur.
+    ///
+    /// ## Example
+    /// ```python
+    /// def test_foo():
+    ///     assert something and something_else
+    ///
+    /// def test_bar():
+    ///     assert not (something or something_else)
+    /// ```
+    ///
+    /// Use instead:
+    /// ```python
+    /// def test_foo():
+    ///     assert something
+    ///     assert something_else
+    ///
+    /// def test_bar():
+    ///     assert not something
+    ///     assert not something_else
+    /// ```
     pub struct CompositeAssertion;
 );
 impl Violation for CompositeAssertion {
+    const AUTOFIX: Option<AutofixKind> = Some(AutofixKind::new(Availability::Sometimes));
+
     #[derive_message_formats]
     fn message(&self) -> String {
         format!("Assertion should be broken down into multiple parts")
     }
+
+    fn autofix_title_formatter(&self) -> Option<fn(&Self) -> String> {
+        Some(|CompositeAssertion| format!("Break down assertion into multiple parts"))
+    }
 }
 
 define_violation!(
@@ -119,22 +152,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.
-const 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<Diagnostic> {
     // Walk body to find assert statements that reference the exception name
     let mut visitor = ExceptionHandlerVisitor::new(name);
@@ -207,13 +224,128 @@ pub fn assert_in_exception_handler(handlers: &[Excepthandler]) -> Vec<Diagnostic
         .collect()
 }
 
+/// 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.
+const 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,
+    }
+}
+
+/// Negate condition, i.e. `a` => `not a` and `not a` => `a`
+pub fn negate(f: Located<ExprKind>) -> Located<ExprKind> {
+    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<Fix> {
+    // We do not split compounds if there is a message
+    if let StmtKind::Assert { msg: Some(_), .. } = &assert.node {
+        return None;
+    }
+
+    let mut conditions: Vec<Located<ExprKind>> = 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().all(|mk| {
+                        !matches!(
+                            mk.node,
+                            ExprKind::BoolOp {
+                                op: Boolop::And,
+                                ..
+                            }
+                        )
+                    }) {
+                        return None;
+                    }
+
+                    // `not (a or b)` equals `not a and not b`
+                    let vals = values
+                        .iter()
+                        .map(|f| negate(f.clone()))
+                        .collect::<Vec<Located<ExprKind>>>();
+
+                    // Compound, so split (Split)
+                    conditions.extend(vals);
+                }
+                _ => {
+                    // Do not split
+                    conditions.push(*operand.clone());
+                }
+            }
+        }
+        _ => {}
+    };
+
+    // for each condition create `assert condition`
+    let mut content: Vec<String> = 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<Diagnostic> {
+pub fn composite_condition(
+    checker: &mut Checker,
+    assert_stmt: &Stmt,
+    test_expr: &Expr,
+) -> Option<Diagnostic> {
     if is_composite_condition(test_expr) {
-        Some(Diagnostic::new(
-            CompositeAssertion,
-            Range::from_located(assert_stmt),
-        ))
+        let mut diagnostic = Diagnostic::new(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
     }