Move fixable checks into patch blocks (#4721)

astral-sh · May 30, 2023 · e323bb0 · e323bb0
1 parent 80fa3f2
commit e323bb0
Show file tree

Hide file tree

Showing 11 changed files with 326 additions and 311 deletions.
diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs
@@ -184,25 +184,27 @@ pub(crate) fn unittest_assertion(
     match func {
         Expr::Attribute(ast::ExprAttribute { attr, .. }) => {
             if let Ok(unittest_assert) = UnittestAssert::try_from(attr.as_str()) {
-                // We're converting an expression to a statement, so avoid applying the fix if
-                // the assertion is part of a larger expression.
-                let fixable = checker.semantic_model().stmt().is_expr_stmt()
-                    && checker.semantic_model().expr_parent().is_none()
-                    && !checker.semantic_model().scope().kind.is_lambda()
-                    && !has_comments_in(expr.range(), checker.locator);
                 let mut diagnostic = Diagnostic::new(
                     PytestUnittestAssertion {
                         assertion: unittest_assert.to_string(),
                     },
                     func.range(),
                 );
-                if fixable && checker.patch(diagnostic.kind.rule()) {
-                    if let Ok(stmt) = unittest_assert.generate_assert(args, keywords) {
-                        #[allow(deprecated)]
-                        diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
-                            checker.generator().stmt(&stmt),
-                            expr.range(),
-                        )));
+                if checker.patch(diagnostic.kind.rule()) {
+                    // We're converting an expression to a statement, so avoid applying the fix if
+                    // the assertion is part of a larger expression.
+                    if checker.semantic_model().stmt().is_expr_stmt()
+                        && checker.semantic_model().expr_parent().is_none()
+                        && !checker.semantic_model().scope().kind.is_lambda()
+                        && !has_comments_in(expr.range(), checker.locator)
+                    {
+                        if let Ok(stmt) = unittest_assert.generate_assert(args, keywords) {
+                            #[allow(deprecated)]
+                            diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
+                                checker.generator().stmt(&stmt),
+                                expr.range(),
+                            )));
+                        }
                     }
                 }
                 Some(diagnostic)
@@ -440,15 +442,17 @@ pub(crate) fn composite_condition(
 ) {
     let composite = is_composite_condition(test);
     if matches!(composite, CompositionKind::Simple | CompositionKind::Mixed) {
-        let fixable = matches!(composite, CompositionKind::Simple)
-            && msg.is_none()
-            && !has_comments_in(stmt.range(), checker.locator);
         let mut diagnostic = Diagnostic::new(PytestCompositeAssertion, stmt.range());
-        if fixable && checker.patch(diagnostic.kind.rule()) {
-            #[allow(deprecated)]
-            diagnostic.try_set_fix_from_edit(|| {
-                fix_composite_condition(stmt, checker.locator, checker.stylist)
-            });
+        if checker.patch(diagnostic.kind.rule()) {
+            if matches!(composite, CompositionKind::Simple)
+                && msg.is_none()
+                && !has_comments_in(stmt.range(), checker.locator)
+            {
+                #[allow(deprecated)]
+                diagnostic.try_set_fix_from_edit(|| {
+                    fix_composite_condition(stmt, checker.locator, checker.stylist)
+                });
+            }
         }
         checker.diagnostics.push(diagnostic);
     }

diff --git a/crates/ruff/src/rules/flake8_simplify/rules/ast_bool_op.rs b/crates/ruff/src/rules/flake8_simplify/rules/ast_bool_op.rs
@@ -293,7 +293,6 @@ pub(crate) fn duplicate_isinstance_call(checker: &mut Checker, expr: &Expr) {
             } else {
                 unreachable!("Indices should only contain `isinstance` calls")
             };
-            let fixable = !contains_effect(target, |id| checker.semantic_model().is_builtin(id));
             let mut diagnostic = Diagnostic::new(
                 DuplicateIsinstanceCall {
                     name: if let Expr::Name(ast::ExprName { id, .. }) = target {
@@ -304,73 +303,75 @@ pub(crate) fn duplicate_isinstance_call(checker: &mut Checker, expr: &Expr) {
                 },
                 expr.range(),
             );
-            if fixable && checker.patch(diagnostic.kind.rule()) {
-                // Grab the types used in each duplicate `isinstance` call (e.g., `int` and `str`
-                // in `isinstance(obj, int) or isinstance(obj, str)`).
-                let types: Vec<&Expr> = indices
-                    .iter()
-                    .map(|index| &values[*index])
-                    .map(|expr| {
-                        let Expr::Call(ast::ExprCall { args, ..}) = expr else {
-                            unreachable!("Indices should only contain `isinstance` calls")
-                        };
-                        args.get(1).expect("`isinstance` should have two arguments")
-                    })
-                    .collect();
-
-                // Generate a single `isinstance` call.
-                let node = ast::ExprTuple {
-                    // Flatten all the types used across the `isinstance` calls.
-                    elts: types
+            if checker.patch(diagnostic.kind.rule()) {
+                if !contains_effect(target, |id| checker.semantic_model().is_builtin(id)) {
+                    // Grab the types used in each duplicate `isinstance` call (e.g., `int` and `str`
+                    // in `isinstance(obj, int) or isinstance(obj, str)`).
+                    let types: Vec<&Expr> = indices
                         .iter()
-                        .flat_map(|value| {
-                            if let Expr::Tuple(ast::ExprTuple { elts, .. }) = value {
-                                Left(elts.iter())
-                            } else {
-                                Right(iter::once(*value))
-                            }
+                        .map(|index| &values[*index])
+                        .map(|expr| {
+                            let Expr::Call(ast::ExprCall { args, .. }) = expr else {
+                                unreachable!("Indices should only contain `isinstance` calls")
+                            };
+                            args.get(1).expect("`isinstance` should have two arguments")
                         })
-                        .map(Clone::clone)
-                        .collect(),
-                    ctx: ExprContext::Load,
-                    range: TextRange::default(),
-                };
-                let node1 = ast::ExprName {
-                    id: "isinstance".into(),
-                    ctx: ExprContext::Load,
-                    range: TextRange::default(),
-                };
-                let node2 = ast::ExprCall {
-                    func: Box::new(node1.into()),
-                    args: vec![target.clone(), node.into()],
-                    keywords: vec![],
-                    range: TextRange::default(),
-                };
-                let call = node2.into();
-
-                // Generate the combined `BoolOp`.
-                let node = ast::ExprBoolOp {
-                    op: Boolop::Or,
-                    values: iter::once(call)
-                        .chain(
-                            values
-                                .iter()
-                                .enumerate()
-                                .filter(|(index, _)| !indices.contains(index))
-                                .map(|(_, elt)| elt.clone()),
-                        )
-                        .collect(),
-                    range: TextRange::default(),
-                };
-                let bool_op = node.into();
-
-                // Populate the `Fix`. Replace the _entire_ `BoolOp`. Note that if we have
-                // multiple duplicates, the fixes will conflict.
-                #[allow(deprecated)]
-                diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
-                    checker.generator().expr(&bool_op),
-                    expr.range(),
-                )));
+                        .collect();
+
+                    // Generate a single `isinstance` call.
+                    let node = ast::ExprTuple {
+                        // Flatten all the types used across the `isinstance` calls.
+                        elts: types
+                            .iter()
+                            .flat_map(|value| {
+                                if let Expr::Tuple(ast::ExprTuple { elts, .. }) = value {
+                                    Left(elts.iter())
+                                } else {
+                                    Right(iter::once(*value))
+                                }
+                            })
+                            .map(Clone::clone)
+                            .collect(),
+                        ctx: ExprContext::Load,
+                        range: TextRange::default(),
+                    };
+                    let node1 = ast::ExprName {
+                        id: "isinstance".into(),
+                        ctx: ExprContext::Load,
+                        range: TextRange::default(),
+                    };
+                    let node2 = ast::ExprCall {
+                        func: Box::new(node1.into()),
+                        args: vec![target.clone(), node.into()],
+                        keywords: vec![],
+                        range: TextRange::default(),
+                    };
+                    let call = node2.into();
+
+                    // Generate the combined `BoolOp`.
+                    let node = ast::ExprBoolOp {
+                        op: Boolop::Or,
+                        values: iter::once(call)
+                            .chain(
+                                values
+                                    .iter()
+                                    .enumerate()
+                                    .filter(|(index, _)| !indices.contains(index))
+                                    .map(|(_, elt)| elt.clone()),
+                            )
+                            .collect(),
+                        range: TextRange::default(),
+                    };
+                    let bool_op = node.into();
+
+                    // Populate the `Fix`. Replace the _entire_ `BoolOp`. Note that if we have
+                    // multiple duplicates, the fixes will conflict.
+                    #[allow(deprecated)]
+                    diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
+                        checker.generator().expr(&bool_op),
+                        expr.range(),
+                    )));
+                }
             }
             checker.diagnostics.push(diagnostic);
         }