Merge 1c186004e5 into b0bc990cbf

crates/ruff_linter/src/rules/flake8_simplify/rules/needless_bool.rs

@@ -1,3 +1,4 @@
+use ruff_diagnostics::Applicability;
 use ruff_macros::{ViolationMetadata, derive_message_formats};
 use ruff_python_ast::name::Name;
 use ruff_python_ast::traversal;
@@ -48,10 +49,15 @@ use crate::{Edit, Fix, FixAvailability, Violation};
 ///
 /// ## Fix safety
 ///
-/// This fix is marked as unsafe because it may change the program’s behavior if the condition does not
+/// This rule provides safe fixes when the replacement expression is guaranteed to evaluate
-/// return a proper Boolean. While the fix will try to wrap non-boolean values in a call to bool,
+/// to a real boolean value – for example, a logical negation (`not`), an identity or
-/// custom implementations of comparison functions like `__eq__` can avoid the bool call and still
+/// membership comparison (`is`, `is not`, `in`, `not in`), or a call to the builtin
-/// lead to altered behavior.
+/// `bool(...)`.
+///
+/// In other cases, the fix is marked as unsafe because it can change runtime behavior. In
+/// particular, equality and inequality comparisons (`==`, `!=`) may be overloaded to return
+/// non-boolean values. When `bool` is shadowed and the expression is not guaranteed to be
+/// boolean, no fix is offered.
 ///
 /// ## References
 /// - [Python documentation: Truth Value Testing](https://docs.python.org/3/library/stdtypes.html#truth-value-testing)
@@ -217,63 +223,184 @@ pub(crate) fn needless_bool(checker: &Checker, stmt: &Stmt) {
         return;
     }
 
-    // Generate the replacement condition.
+    // Build replacement condition and decide safety together.
-    let condition = if checker
+    let (condition, applicability) = if checker
         .comment_ranges()
         .has_comments(&range, checker.source())
     {
-        None
+        (None, Applicability::Unsafe)
     } else {
-        // If the return values are inverted, wrap the condition in a `not`.
+        build_replacement_and_safety(
-        if inverted {
+            if_test,
-            match if_test {
+            inverted,
-                Expr::UnaryOp(ast::ExprUnaryOp {
+            checker.semantic().has_builtin_binding("bool"),
-                    op: ast::UnaryOp::Not,
+        )
-                    operand,
+    };
-                    ..
-                }) => Some((**operand).clone()),
 
-                Expr::Compare(ast::ExprCompare {
+    // Generate the replacement `return` statement.
-                    ops,
+    let replacement = condition.as_ref().map(|expr| {
-                    left,
+        Stmt::Return(ast::StmtReturn {
-                    comparators,
+            value: Some(Box::new(expr.clone())),
-                    ..
+            range: TextRange::default(),
-                }) if matches!(
+            node_index: ruff_python_ast::AtomicNodeIndex::NONE,
-                    ops.as_ref(),
+        })
-                    [ast::CmpOp::Eq
+    });
-                        | ast::CmpOp::NotEq
-                        | ast::CmpOp::In
-                        | ast::CmpOp::NotIn
-                        | ast::CmpOp::Is
-                        | ast::CmpOp::IsNot]
-                ) =>
-                {
-                    let ([op], [right]) = (ops.as_ref(), comparators.as_ref()) else {
-                        unreachable!("Single comparison with multiple comparators");
-                    };
 
-                    Some(Expr::Compare(ast::ExprCompare {
+    // Generate source code.
-                        ops: Box::new([op.negate()]),
+    let replacement = replacement
-                        left: left.clone(),
+        .as_ref()
-                        comparators: Box::new([right.clone()]),
+        .map(|stmt| checker.generator().stmt(stmt));
-                        range: TextRange::default(),
+    let condition_code = condition
-                        node_index: ruff_python_ast::AtomicNodeIndex::NONE,
+        .as_ref()
-                    }))
+        .map(|expr| checker.generator().expr(expr));
-                }
 
-                _ => Some(Expr::UnaryOp(ast::ExprUnaryOp {
+    let mut diagnostic = checker.report_diagnostic(
+        NeedlessBool {
+            condition: condition_code.map(SourceCodeSnippet::new),
+            negate: inverted,
+        },
+        range,
+    );
+    if let Some(replacement) = replacement {
+        diagnostic.set_fix(Fix::applicable_edit(
+            Edit::range_replacement(replacement, range),
+            applicability,
+        ));
+    }
+}
+
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+enum Bool {
+    True,
+    False,
+}
+
+/// Build the replacement expression for SIM103 and determine whether applying it is safe.
+///
+/// Safety rules:
+/// - Safe if the replacement is guaranteed boolean:
+///   - a negation (`not <expr>`),
+///   - an identity/membership comparison (`is`, `is not`, `in`, `not in`), or
+///   - a call to the builtin `bool(<expr>)` (only when `bool` is not shadowed).
+/// - Unsafe for equality comparisons (`==`, `!=`), because user types may overload them to return
+///   non-boolean values.
+/// - When `bool` is shadowed and the expression is not guaranteed boolean, no fix is provided.
+fn build_replacement_and_safety(
+    if_test: &Expr,
+    inverted: bool,
+    has_builtin_bool: bool,
+) -> (Option<Expr>, Applicability) {
+    fn is_identity_or_membership_ops(ops: &[ast::CmpOp]) -> bool {
+        ops.iter().all(|op| {
+            matches!(
+                op,
+                ast::CmpOp::In | ast::CmpOp::NotIn | ast::CmpOp::Is | ast::CmpOp::IsNot
+            )
+        })
+    }
+
+    fn is_eq_neq_ops(ops: &[ast::CmpOp]) -> bool {
+        ops.iter()
+            .all(|op| matches!(op, ast::CmpOp::Eq | ast::CmpOp::NotEq))
+    }
+
+    match (inverted, if_test) {
+        // Replacement becomes the operand; safe only if guaranteed-boolean.
+        (
+            true,
+            Expr::UnaryOp(ast::ExprUnaryOp {
+                op: ast::UnaryOp::Not,
+                operand,
+                ..
+            }),
+        ) => match operand.as_ref() {
+            Expr::UnaryOp(ast::ExprUnaryOp {
+                op: ast::UnaryOp::Not,
+                ..
+            }) => (Some((**operand).clone()), Applicability::Safe),
+            Expr::Compare(ast::ExprCompare { ops, .. }) => {
+                let app = if is_identity_or_membership_ops(ops.as_ref()) {
+                    Applicability::Safe
+                } else {
+                    Applicability::Unsafe
+                };
+                (Some((**operand).clone()), app)
+            }
+            _ => (Some((**operand).clone()), Applicability::Unsafe),
+        },
+
+        // Replacement becomes a negated comparison: safe only for identity/membership. For
+        // other comparisons, the replacement will be `not <expr>` which is a bool, except for
+        // `==`/`!=` which can be overloaded to return non-bool.
+        (
+            true,
+            Expr::Compare(ast::ExprCompare {
+                ops,
+                left,
+                comparators,
+                ..
+            }),
+        ) => match ops.as_ref() {
+            [
+                ast::CmpOp::Eq
+                | ast::CmpOp::NotEq
+                | ast::CmpOp::In
+                | ast::CmpOp::NotIn
+                | ast::CmpOp::Is
+                | ast::CmpOp::IsNot,
+            ] => {
+                let ([op], [right]) = (ops.as_ref(), comparators.as_ref()) else {
+                    unreachable!("Single comparison with multiple comparators");
+                };
+                let replacement = Expr::Compare(ast::ExprCompare {
+                    ops: Box::new([op.negate()]),
+                    left: left.clone(),
+                    comparators: Box::new([right.clone()]),
+                    range: TextRange::default(),
+                    node_index: ruff_python_ast::AtomicNodeIndex::NONE,
+                });
+                (
+                    Some(replacement),
+                    if is_identity_or_membership_ops(ops.as_ref()) && !is_eq_neq_ops(ops.as_ref()) {
+                        Applicability::Safe
+                    } else {
+                        Applicability::Unsafe
+                    },
+                )
+            }
+            _ => {
+                let replacement = Expr::UnaryOp(ast::ExprUnaryOp {
                     op: ast::UnaryOp::Not,
                     operand: Box::new(if_test.clone()),
                     range: TextRange::default(),
                     node_index: ruff_python_ast::AtomicNodeIndex::NONE,
-                })),
+                });
+                (Some(replacement), Applicability::Safe)
             }
-        } else if if_test.is_compare_expr() {
+        },
-            // If the condition is a comparison, we can replace it with the condition, since we
+
-            // know it's a boolean.
+        // Replacement becomes `not <expr>` which is always a bool.
-            Some(if_test.clone())
+        (true, _) => (
-        } else if checker.semantic().has_builtin_binding("bool") {
+            Some(Expr::UnaryOp(ast::ExprUnaryOp {
-            // Otherwise, we need to wrap the condition in a call to `bool`.
+                op: ast::UnaryOp::Not,
+                operand: Box::new(if_test.clone()),
+                range: TextRange::default(),
+                node_index: ruff_python_ast::AtomicNodeIndex::NONE,
+            })),
+            Applicability::Safe,
+        ),
+
+        // Non-inverted: direct compare is safe only for identity/membership; otherwise
+        // we rely on wrapping with `bool(...)` if available.
+        (false, Expr::Compare(ast::ExprCompare { ops, .. })) => (
+            Some(if_test.clone()),
+            if is_identity_or_membership_ops(ops.as_ref()) {
+                Applicability::Safe
+            } else {
+                Applicability::Unsafe
+            },
+        ),
+        (false, _) if has_builtin_bool => {
             let func_node = ast::ExprName {
                 id: Name::new_static("bool"),
                 ctx: ExprContext::Load,
@@ -291,50 +418,12 @@ pub(crate) fn needless_bool(checker: &Checker, stmt: &Stmt) {
                 range: TextRange::default(),
                 node_index: ruff_python_ast::AtomicNodeIndex::NONE,
             };
-            Some(Expr::Call(call_node))
+            (Some(Expr::Call(call_node)), Applicability::Safe)
-        } else {
-            None
         }
-    };
+        (false, _) => (None, Applicability::Unsafe),
-
-    // Generate the replacement `return` statement.
-    let replacement = condition.as_ref().map(|expr| {
-        Stmt::Return(ast::StmtReturn {
-            value: Some(Box::new(expr.clone())),
-            range: TextRange::default(),
-            node_index: ruff_python_ast::AtomicNodeIndex::NONE,
-        })
-    });
-
-    // Generate source code.
-    let replacement = replacement
-        .as_ref()
-        .map(|stmt| checker.generator().stmt(stmt));
-    let condition = condition
-        .as_ref()
-        .map(|expr| checker.generator().expr(expr));
-
-    let mut diagnostic = checker.report_diagnostic(
-        NeedlessBool {
-            condition: condition.map(SourceCodeSnippet::new),
-            negate: inverted,
-        },
-        range,
-    );
-    if let Some(replacement) = replacement {
-        diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
-            replacement,
-            range,
-        )));
     }
 }
 
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
-enum Bool {
-    True,
-    False,
-}
-
 impl From<bool> for Bool {
     fn from(value: bool) -> Self {
         if value { Bool::True } else { Bool::False }

crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM103_SIM103.py.snap

@@ -23,7 +23,6 @@ help: Replace with `return bool(a)`
 4 | 
 5 | 
 6 | def f():
-note: This is an unsafe fix and may change runtime behavior
 
 SIM103 [*] Return the condition `a == b` directly
   --> SIM103.py:11:5
@@ -73,7 +72,6 @@ help: Replace with `return bool(b)`
 22 | 
 23 | 
 24 | def f():
-note: This is an unsafe fix and may change runtime behavior
 
 SIM103 [*] Return the condition `bool(b)` directly
   --> SIM103.py:32:9
@@ -98,7 +96,6 @@ help: Replace with `return bool(b)`
 33 | 
 34 | 
 35 | def f():
-note: This is an unsafe fix and may change runtime behavior
 
 SIM103 [*] Return the condition `not a` directly
   --> SIM103.py:57:5
@@ -123,7 +120,6 @@ help: Replace with `return not a`
 58 | 
 59 | 
 60 | def f():
-note: This is an unsafe fix and may change runtime behavior
 
 SIM103 Return the condition directly
   --> SIM103.py:83:5
@@ -161,7 +157,6 @@ help: Replace with `return not (keys is not None and notice.key not in keys)`
 92 | 
 93 | 
 94 | ###
-note: This is an unsafe fix and may change runtime behavior
 
 SIM103 [*] Return the condition `bool(a)` directly
    --> SIM103.py:104:5
@@ -184,7 +179,6 @@ help: Replace with `return bool(a)`
 105 | 
 106 | 
 107 | def f():
-note: This is an unsafe fix and may change runtime behavior
 
 SIM103 [*] Return the condition `not a` directly
    --> SIM103.py:111:5
@@ -207,7 +201,6 @@ help: Replace with `return not a`
 112 | 
 113 | 
 114 | def f():
-note: This is an unsafe fix and may change runtime behavior
 
 SIM103 [*] Return the condition `10 < a` directly
    --> SIM103.py:117:5
@@ -251,7 +244,6 @@ help: Replace with `return not 10 < a`
 124 | 
 125 | 
 126 | def f():
-note: This is an unsafe fix and may change runtime behavior
 
 SIM103 [*] Return the condition `10 not in a` directly
    --> SIM103.py:129:5
@@ -273,7 +265,6 @@ help: Replace with `return 10 not in a`
 130 | 
 131 | 
 132 | def f():
-note: This is an unsafe fix and may change runtime behavior
 
 SIM103 [*] Return the condition `10 in a` directly
    --> SIM103.py:135:5
@@ -295,7 +286,6 @@ help: Replace with `return 10 in a`
 136 | 
 137 | 
 138 | def f():
-note: This is an unsafe fix and may change runtime behavior
 
 SIM103 [*] Return the condition `a is not 10` directly
    --> SIM103.py:141:5
@@ -317,7 +307,6 @@ help: Replace with `return a is not 10`
 142 | 
 143 | 
 144 | def f():
-note: This is an unsafe fix and may change runtime behavior
 
 SIM103 [*] Return the condition `a is 10` directly
    --> SIM103.py:147:5
@@ -339,7 +328,6 @@ help: Replace with `return a is 10`
 148 | 
 149 | 
 150 | def f():
-note: This is an unsafe fix and may change runtime behavior
 
 SIM103 [*] Return the condition `a != 10` directly
    --> SIM103.py:153:5