mirror of https://github.com/astral-sh/ruff
flake8-simplify(SIM103): mark guaranteed-bool fixes as safe
This commit is contained in:
Takayuki Maeda
parent
2467c4352e
commit
1c186004e5
|
|
@ -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 <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.
|
||||
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 <expr>` 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<bool> for Bool {
|
||||
fn from(value: bool) -> Self {
|
||||
if value { Bool::True } else { Bool::False }
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in New Issue