diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/needless_bool.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/needless_bool.rs index 1d64db5fc8..38f30b1587 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/needless_bool.rs +++ b/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 -/// return a proper Boolean. While the fix will try to wrap non-boolean values in a call to bool, -/// custom implementations of comparison functions like `__eq__` can avoid the bool call and still -/// lead to altered behavior. +/// This rule provides safe fixes when the replacement expression is guaranteed to evaluate +/// to a real boolean value – for example, a logical negation (`not`), an identity or +/// membership comparison (`is`, `is not`, `in`, `not in`), or a call to the builtin +/// `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) @@ -216,63 +222,184 @@ pub(crate) fn needless_bool(checker: &Checker, stmt: &Stmt) { return; } - // Generate the replacement condition. - let condition = if checker + // Build replacement condition and decide safety together. + 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`. - if inverted { - match if_test { - Expr::UnaryOp(ast::ExprUnaryOp { - op: ast::UnaryOp::Not, - operand, - .. - }) => Some((**operand).clone()), + build_replacement_and_safety( + if_test, + inverted, + checker.semantic().has_builtin_binding("bool"), + ) + }; - Expr::Compare(ast::ExprCompare { - ops, - left, - comparators, - .. - }) if matches!( - 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"); - }; + // 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, + }) + }); - Some(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, - })) - } + // Generate source code. + let replacement = replacement + .as_ref() + .map(|stmt| checker.generator().stmt(stmt)); + let condition_code = condition + .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 `), +/// - an identity/membership comparison (`is`, `is not`, `in`, `not in`), or +/// - a call to the builtin `bool()` (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, 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 ` 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. - Some(if_test.clone()) - } else if checker.semantic().has_builtin_binding("bool") { - // Otherwise, we need to wrap the condition in a call to `bool`. + }, + + // Replacement becomes `not ` which is always a bool. + (true, _) => ( + Some(Expr::UnaryOp(ast::ExprUnaryOp { + 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, @@ -290,50 +417,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)) - } else { - None + (Some(Expr::Call(call_node)), Applicability::Safe) } - }; - - // 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, - ))); + (false, _) => (None, Applicability::Unsafe), } } -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum Bool { - True, - False, -} - impl From for Bool { fn from(value: bool) -> Self { if value { Bool::True } else { Bool::False } diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM103_SIM103.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM103_SIM103.py.snap index 51cd372286..02e110053b 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM103_SIM103.py.snap +++ b/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