Skip to content

Commit dbf8d0c

Browse files
charliermarshcharliermarsh
authored
Show negated condition in needless-bool diagnostics (#10854)
## Summary Closes #10843.
1 parent 02e88fd commit dbf8d0c

File tree

4 files changed

+146
-81
lines changed

4 files changed

+146
-81
lines changed

‎crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM103.py

+10-2
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ def f():
5353

5454

5555
def f():
56-
# SIM103 (but not fixable)
56+
# SIM103
5757
if a:
5858
return False
5959
else:
@@ -77,7 +77,7 @@ def f():
7777

7878

7979
def f():
80-
# OK
80+
# SIM103 (but not fixable)
8181
def bool():
8282
return False
8383
if a:
@@ -86,6 +86,14 @@ def bool():
8686
return False
8787

8888

89+
def f():
90+
# SIM103
91+
if keys is not None and notice.key not in keys:
92+
return False
93+
else:
94+
return True
95+
96+
8997
###
9098
# Positive cases (preview)
9199
###

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

+38-33
Original file line numberDiff line numberDiff line change
@@ -41,30 +41,31 @@ use crate::fix::snippet::SourceCodeSnippet;
4141
/// [preview]: https://docs.astral.sh/ruff/preview/
4242
#[violation]
4343
pub struct NeedlessBool {
44-
condition: SourceCodeSnippet,
45-
replacement: Option<SourceCodeSnippet>,
44+
condition: Option<SourceCodeSnippet>,
45+
negate: bool,
4646
}
4747

4848
impl Violation for NeedlessBool {
4949
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
5050

5151
#[derive_message_formats]
5252
fn message(&self) -> String {
53-
let NeedlessBool { condition, .. } = self;
54-
if let Some(condition) = condition.full_display() {
53+
let NeedlessBool { condition, negate } = self;
54+
55+
if let Some(condition) = condition.as_ref().and_then(SourceCodeSnippet::full_display) {
5556
format!("Return the condition `{condition}` directly")
57+
} else if *negate {
58+
format!("Return the negated condition directly")
5659
} else {
5760
format!("Return the condition directly")
5861
}
5962
}
6063

6164
fn fix_title(&self) -> Option<String> {
62-
let NeedlessBool { replacement, .. } = self;
63-
if let Some(replacement) = replacement
64-
.as_ref()
65-
.and_then(SourceCodeSnippet::full_display)
66-
{
67-
Some(format!("Replace with `{replacement}`"))
65+
let NeedlessBool { condition, .. } = self;
66+
67+
if let Some(condition) = condition.as_ref().and_then(SourceCodeSnippet::full_display) {
68+
Some(format!("Replace with `return {condition}`"))
6869
} else {
6970
Some(format!("Inline condition"))
7071
}
@@ -191,37 +192,29 @@ pub(crate) fn needless_bool(checker: &mut Checker, stmt: &Stmt) {
191192
return;
192193
}
193194

194-
let condition = checker.locator().slice(if_test);
195-
let replacement = if checker.indexer().has_comments(&range, checker.locator()) {
195+
// Generate the replacement condition.
196+
let condition = if checker.indexer().has_comments(&range, checker.locator()) {
196197
None
197198
} else {
198199
// If the return values are inverted, wrap the condition in a `not`.
199200
if inverted {
200-
let node = ast::StmtReturn {
201-
value: Some(Box::new(Expr::UnaryOp(ast::ExprUnaryOp {
202-
op: ast::UnaryOp::Not,
203-
operand: Box::new(if_test.clone()),
204-
range: TextRange::default(),
205-
}))),
201+
Some(Expr::UnaryOp(ast::ExprUnaryOp {
202+
op: ast::UnaryOp::Not,
203+
operand: Box::new(if_test.clone()),
206204
range: TextRange::default(),
207-
};
208-
Some(checker.generator().stmt(&node.into()))
205+
}))
209206
} else if if_test.is_compare_expr() {
210207
// If the condition is a comparison, we can replace it with the condition, since we
211208
// know it's a boolean.
212-
let node = ast::StmtReturn {
213-
value: Some(Box::new(if_test.clone())),
214-
range: TextRange::default(),
215-
};
216-
Some(checker.generator().stmt(&node.into()))
209+
Some(if_test.clone())
217210
} else if checker.semantic().is_builtin("bool") {
218211
// Otherwise, we need to wrap the condition in a call to `bool`.
219212
let func_node = ast::ExprName {
220213
id: "bool".into(),
221214
ctx: ExprContext::Load,
222215
range: TextRange::default(),
223216
};
224-
let value_node = ast::ExprCall {
217+
let call_node = ast::ExprCall {
225218
func: Box::new(func_node.into()),
226219
arguments: Arguments {
227220
args: Box::from([if_test.clone()]),
@@ -230,20 +223,32 @@ pub(crate) fn needless_bool(checker: &mut Checker, stmt: &Stmt) {
230223
},
231224
range: TextRange::default(),
232225
};
233-
let return_node = ast::StmtReturn {
234-
value: Some(Box::new(value_node.into())),
235-
range: TextRange::default(),
236-
};
237-
Some(checker.generator().stmt(&return_node.into()))
226+
Some(Expr::Call(call_node))
238227
} else {
239228
None
240229
}
241230
};
242231

232+
// Generate the replacement `return` statement.
233+
let replacement = condition.as_ref().map(|expr| {
234+
Stmt::Return(ast::StmtReturn {
235+
value: Some(Box::new(expr.clone())),
236+
range: TextRange::default(),
237+
})
238+
});
239+
240+
// Generate source code.
241+
let replacement = replacement
242+
.as_ref()
243+
.map(|stmt| checker.generator().stmt(stmt));
244+
let condition = condition
245+
.as_ref()
246+
.map(|expr| checker.generator().expr(expr));
247+
243248
let mut diagnostic = Diagnostic::new(
244249
NeedlessBool {
245-
condition: SourceCodeSnippet::from_str(condition),
246-
replacement: replacement.clone().map(SourceCodeSnippet::new),
250+
condition: condition.map(SourceCodeSnippet::new),
251+
negate: inverted,
247252
},
248253
range,
249254
);

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

+33-7
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
---
22
source: crates/ruff_linter/src/rules/flake8_simplify/mod.rs
33
---
4-
SIM103.py:3:5: SIM103 [*] Return the condition `a` directly
4+
SIM103.py:3:5: SIM103 [*] Return the condition `bool(a)` directly
55
|
66
1 | def f():
77
2 | # SIM103
@@ -52,7 +52,7 @@ SIM103.py:11:5: SIM103 [*] Return the condition `a == b` directly
5252
16 13 |
5353
17 14 | def f():
5454

55-
SIM103.py:21:5: SIM103 [*] Return the condition `b` directly
55+
SIM103.py:21:5: SIM103 [*] Return the condition `bool(b)` directly
5656
|
5757
19 | if a:
5858
20 | return 1
@@ -78,7 +78,7 @@ SIM103.py:21:5: SIM103 [*] Return the condition `b` directly
7878
26 23 |
7979
27 24 | def f():
8080

81-
SIM103.py:32:9: SIM103 [*] Return the condition `b` directly
81+
SIM103.py:32:9: SIM103 [*] Return the condition `bool(b)` directly
8282
|
8383
30 | return 1
8484
31 | else:
@@ -104,10 +104,10 @@ SIM103.py:32:9: SIM103 [*] Return the condition `b` directly
104104
37 34 |
105105
38 35 | def f():
106106

107-
SIM103.py:57:5: SIM103 [*] Return the condition `a` directly
107+
SIM103.py:57:5: SIM103 [*] Return the condition `not a` directly
108108
|
109109
55 | def f():
110-
56 | # SIM103 (but not fixable)
110+
56 | # SIM103
111111
57 | if a:
112112
| _____^
113113
58 | | return False
@@ -120,7 +120,7 @@ SIM103.py:57:5: SIM103 [*] Return the condition `a` directly
120120
ℹ Unsafe fix
121121
54 54 |
122122
55 55 | def f():
123-
56 56 | # SIM103 (but not fixable)
123+
56 56 | # SIM103
124124
57 |- if a:
125125
58 |- return False
126126
59 |- else:
@@ -130,7 +130,7 @@ SIM103.py:57:5: SIM103 [*] Return the condition `a` directly
130130
62 59 |
131131
63 60 | def f():
132132

133-
SIM103.py:83:5: SIM103 Return the condition `a` directly
133+
SIM103.py:83:5: SIM103 Return the condition directly
134134
|
135135
81 | def bool():
136136
82 | return False
@@ -142,3 +142,29 @@ SIM103.py:83:5: SIM103 Return the condition `a` directly
142142
| |____________________^ SIM103
143143
|
144144
= help: Inline condition
145+
146+
SIM103.py:91:5: SIM103 [*] Return the condition `not (keys is not None and notice.key not in keys)` directly
147+
|
148+
89 | def f():
149+
90 | # SIM103
150+
91 | if keys is not None and notice.key not in keys:
151+
| _____^
152+
92 | | return False
153+
93 | | else:
154+
94 | | return True
155+
| |___________________^ SIM103
156+
|
157+
= help: Replace with `return not (keys is not None and notice.key not in keys)`
158+
159+
ℹ Unsafe fix
160+
88 88 |
161+
89 89 | def f():
162+
90 90 | # SIM103
163+
91 |- if keys is not None and notice.key not in keys:
164+
92 |- return False
165+
93 |- else:
166+
94 |- return True
167+
91 |+ return not (keys is not None and notice.key not in keys)
168+
95 92 |
169+
96 93 |
170+
97 94 | ###

0 commit comments

Comments
 (0)