Skip to content

Commit 72c9f7e

Browse files
authored
Include actual conditions in E712 diagnostics (#10254)
## Summary Changes the generic recommendation to replace ```python if foo == True: ... ``` with `if cond:` to `if foo:`. Still uses a generic message for compound comparisons as a specific message starts to become confusing. For example, ```python if foo == True != False: ... ``` produces two recommendations, one of which would recommend `if True:`, which is confusing. Resolves [recommendation in a previous PR](https://github.com/astral-sh/ruff/pull/8613/files#r1514915070). ## Test Plan `cargo nextest run`
1 parent 57be3fc commit 72c9f7e

File tree

3 files changed

+120
-66
lines changed

3 files changed

+120
-66
lines changed

crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs

+77-19
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ use ruff_text_size::Ranged;
99

1010
use crate::checkers::ast::Checker;
1111
use crate::codes::Rule;
12+
use crate::fix::snippet::SourceCodeSnippet;
1213

1314
#[derive(Debug, PartialEq, Eq, Copy, Clone)]
1415
enum EqCmpOp {
@@ -102,39 +103,52 @@ impl AlwaysFixableViolation for NoneComparison {
102103
///
103104
/// [PEP 8]: https://peps.python.org/pep-0008/#programming-recommendations
104105
#[violation]
105-
pub struct TrueFalseComparison(bool, EqCmpOp);
106+
pub struct TrueFalseComparison {
107+
value: bool,
108+
op: EqCmpOp,
109+
cond: Option<SourceCodeSnippet>,
110+
}
106111

107112
impl AlwaysFixableViolation for TrueFalseComparison {
108113
#[derive_message_formats]
109114
fn message(&self) -> String {
110-
let TrueFalseComparison(value, op) = self;
115+
let TrueFalseComparison { value, op, cond } = self;
116+
let Some(cond) = cond else {
117+
return "Avoid equality comparisons to `True` or `False`".to_string();
118+
};
119+
let cond = cond.truncated_display();
111120
match (value, op) {
112121
(true, EqCmpOp::Eq) => {
113-
format!("Avoid equality comparisons to `True`; use `if cond:` for truth checks")
122+
format!("Avoid equality comparisons to `True`; use `if {cond}:` for truth checks")
114123
}
115124
(true, EqCmpOp::NotEq) => {
116125
format!(
117-
"Avoid inequality comparisons to `True`; use `if not cond:` for false checks"
126+
"Avoid inequality comparisons to `True`; use `if not {cond}:` for false checks"
118127
)
119128
}
120129
(false, EqCmpOp::Eq) => {
121130
format!(
122-
"Avoid equality comparisons to `False`; use `if not cond:` for false checks"
131+
"Avoid equality comparisons to `False`; use `if not {cond}:` for false checks"
123132
)
124133
}
125134
(false, EqCmpOp::NotEq) => {
126-
format!("Avoid inequality comparisons to `False`; use `if cond:` for truth checks")
135+
format!(
136+
"Avoid inequality comparisons to `False`; use `if {cond}:` for truth checks"
137+
)
127138
}
128139
}
129140
}
130141

131142
fn fix_title(&self) -> String {
132-
let TrueFalseComparison(value, op) = self;
143+
let TrueFalseComparison { value, op, cond } = self;
144+
let Some(cond) = cond.as_ref().and_then(|cond| cond.full_display()) else {
145+
return "Replace comparison".to_string();
146+
};
133147
match (value, op) {
134-
(true, EqCmpOp::Eq) => "Replace with `cond`".to_string(),
135-
(true, EqCmpOp::NotEq) => "Replace with `not cond`".to_string(),
136-
(false, EqCmpOp::Eq) => "Replace with `not cond`".to_string(),
137-
(false, EqCmpOp::NotEq) => "Replace with `cond`".to_string(),
148+
(true, EqCmpOp::Eq) => format!("Replace with `{cond}`"),
149+
(true, EqCmpOp::NotEq) => format!("Replace with `not {cond}`"),
150+
(false, EqCmpOp::Eq) => format!("Replace with `not {cond}`"),
151+
(false, EqCmpOp::NotEq) => format!("Replace with `{cond}`"),
138152
}
139153
}
140154
}
@@ -178,17 +192,35 @@ pub(crate) fn literal_comparisons(checker: &mut Checker, compare: &ast::ExprComp
178192
if let Expr::BooleanLiteral(ast::ExprBooleanLiteral { value, .. }) = comparator {
179193
match op {
180194
EqCmpOp::Eq => {
195+
let cond = if compare.ops.len() == 1 {
196+
Some(SourceCodeSnippet::from_str(checker.locator().slice(next)))
197+
} else {
198+
None
199+
};
181200
let diagnostic = Diagnostic::new(
182-
TrueFalseComparison(*value, op),
183-
comparator.range(),
201+
TrueFalseComparison {
202+
value: *value,
203+
op,
204+
cond,
205+
},
206+
compare.range(),
184207
);
185208
bad_ops.insert(0, CmpOp::Is);
186209
diagnostics.push(diagnostic);
187210
}
188211
EqCmpOp::NotEq => {
212+
let cond = if compare.ops.len() == 1 {
213+
Some(SourceCodeSnippet::from_str(checker.locator().slice(next)))
214+
} else {
215+
None
216+
};
189217
let diagnostic = Diagnostic::new(
190-
TrueFalseComparison(*value, op),
191-
comparator.range(),
218+
TrueFalseComparison {
219+
value: *value,
220+
op,
221+
cond,
222+
},
223+
compare.range(),
192224
);
193225
bad_ops.insert(0, CmpOp::IsNot);
194226
diagnostics.push(diagnostic);
@@ -231,14 +263,40 @@ pub(crate) fn literal_comparisons(checker: &mut Checker, compare: &ast::ExprComp
231263
if let Expr::BooleanLiteral(ast::ExprBooleanLiteral { value, .. }) = next {
232264
match op {
233265
EqCmpOp::Eq => {
234-
let diagnostic =
235-
Diagnostic::new(TrueFalseComparison(*value, op), next.range());
266+
let cond = if compare.ops.len() == 1 {
267+
Some(SourceCodeSnippet::from_str(
268+
checker.locator().slice(comparator),
269+
))
270+
} else {
271+
None
272+
};
273+
let diagnostic = Diagnostic::new(
274+
TrueFalseComparison {
275+
value: *value,
276+
op,
277+
cond,
278+
},
279+
compare.range(),
280+
);
236281
bad_ops.insert(index, CmpOp::Is);
237282
diagnostics.push(diagnostic);
238283
}
239284
EqCmpOp::NotEq => {
240-
let diagnostic =
241-
Diagnostic::new(TrueFalseComparison(*value, op), next.range());
285+
let cond = if compare.ops.len() == 1 {
286+
Some(SourceCodeSnippet::from_str(
287+
checker.locator().slice(comparator),
288+
))
289+
} else {
290+
None
291+
};
292+
let diagnostic = Diagnostic::new(
293+
TrueFalseComparison {
294+
value: *value,
295+
op,
296+
cond,
297+
},
298+
compare.range(),
299+
);
242300
bad_ops.insert(index, CmpOp::IsNot);
243301
diagnostics.push(diagnostic);
244302
}

0 commit comments

Comments
 (0)