]>
Commit | Line | Data |
---|---|---|
f20569fa XL |
1 | //! Checks for needless boolean results of if-else expressions |
2 | //! | |
3 | //! This lint is **warn** by default | |
4 | ||
cdc7bbd5 XL |
5 | use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg}; |
6 | use clippy_utils::source::snippet_with_applicability; | |
7 | use clippy_utils::sugg::Sugg; | |
49aad941 | 8 | use clippy_utils::{ |
add651ee FG |
9 | get_parent_node, higher, is_else_clause, is_expn_of, peel_blocks, peel_blocks_with_stmt, span_extract_comment, |
10 | SpanlessEq, | |
49aad941 | 11 | }; |
f20569fa XL |
12 | use rustc_ast::ast::LitKind; |
13 | use rustc_errors::Applicability; | |
a2a8927a | 14 | use rustc_hir::{BinOpKind, Block, Expr, ExprKind, HirId, Node, UnOp}; |
f20569fa XL |
15 | use rustc_lint::{LateContext, LateLintPass}; |
16 | use rustc_session::{declare_lint_pass, declare_tool_lint}; | |
17 | use rustc_span::source_map::Spanned; | |
18 | use rustc_span::Span; | |
19 | ||
20 | declare_clippy_lint! { | |
94222f64 XL |
21 | /// ### What it does |
22 | /// Checks for expressions of the form `if c { true } else { | |
f20569fa XL |
23 | /// false }` (or vice versa) and suggests using the condition directly. |
24 | /// | |
94222f64 XL |
25 | /// ### Why is this bad? |
26 | /// Redundant code. | |
f20569fa | 27 | /// |
94222f64 XL |
28 | /// ### Known problems |
29 | /// Maybe false positives: Sometimes, the two branches are | |
f20569fa XL |
30 | /// painstakingly documented (which we, of course, do not detect), so they *may* |
31 | /// have some value. Even then, the documentation can be rewritten to match the | |
32 | /// shorter code. | |
33 | /// | |
94222f64 | 34 | /// ### Example |
923072b8 FG |
35 | /// ```rust |
36 | /// # let x = true; | |
f20569fa XL |
37 | /// if x { |
38 | /// false | |
39 | /// } else { | |
40 | /// true | |
41 | /// } | |
923072b8 | 42 | /// # ; |
f20569fa | 43 | /// ``` |
923072b8 FG |
44 | /// |
45 | /// Use instead: | |
46 | /// ```rust | |
47 | /// # let x = true; | |
f20569fa | 48 | /// !x |
923072b8 | 49 | /// # ; |
f20569fa | 50 | /// ``` |
a2a8927a | 51 | #[clippy::version = "pre 1.29.0"] |
f20569fa XL |
52 | pub NEEDLESS_BOOL, |
53 | complexity, | |
54 | "if-statements with plain booleans in the then- and else-clause, e.g., `if p { true } else { false }`" | |
55 | } | |
56 | ||
57 | declare_clippy_lint! { | |
94222f64 XL |
58 | /// ### What it does |
59 | /// Checks for expressions of the form `x == true`, | |
f20569fa XL |
60 | /// `x != true` and order comparisons such as `x < true` (or vice versa) and |
61 | /// suggest using the variable directly. | |
62 | /// | |
94222f64 XL |
63 | /// ### Why is this bad? |
64 | /// Unnecessary code. | |
f20569fa | 65 | /// |
94222f64 | 66 | /// ### Example |
f20569fa XL |
67 | /// ```rust,ignore |
68 | /// if x == true {} | |
69 | /// if y == false {} | |
70 | /// ``` | |
71 | /// use `x` directly: | |
72 | /// ```rust,ignore | |
73 | /// if x {} | |
74 | /// if !y {} | |
75 | /// ``` | |
a2a8927a | 76 | #[clippy::version = "pre 1.29.0"] |
f20569fa XL |
77 | pub BOOL_COMPARISON, |
78 | complexity, | |
79 | "comparing a variable to a boolean, e.g., `if x == true` or `if x != true`" | |
80 | } | |
81 | ||
49aad941 FG |
82 | declare_clippy_lint! { |
83 | /// ### What it does | |
84 | /// Checks for expressions of the form `if c { x = true } else { x = false }` | |
85 | /// (or vice versa) and suggest assigning the variable directly from the | |
86 | /// condition. | |
87 | /// | |
88 | /// ### Why is this bad? | |
89 | /// Redundant code. | |
90 | /// | |
91 | /// ### Example | |
92 | /// ```rust,ignore | |
93 | /// # fn must_keep(x: i32, y: i32) -> bool { x == y } | |
94 | /// # let x = 32; let y = 10; | |
95 | /// # let mut skip: bool; | |
96 | /// if must_keep(x, y) { | |
97 | /// skip = false; | |
98 | /// } else { | |
99 | /// skip = true; | |
100 | /// } | |
101 | /// ``` | |
102 | /// Use instead: | |
103 | /// ```rust,ignore | |
104 | /// # fn must_keep(x: i32, y: i32) -> bool { x == y } | |
105 | /// # let x = 32; let y = 10; | |
106 | /// # let mut skip: bool; | |
107 | /// skip = !must_keep(x, y); | |
108 | /// ``` | |
add651ee | 109 | #[clippy::version = "1.71.0"] |
49aad941 FG |
110 | pub NEEDLESS_BOOL_ASSIGN, |
111 | complexity, | |
112 | "setting the same boolean variable in both branches of an if-statement" | |
113 | } | |
114 | declare_lint_pass!(NeedlessBool => [NEEDLESS_BOOL, NEEDLESS_BOOL_ASSIGN]); | |
f20569fa | 115 | |
a2a8927a XL |
116 | fn condition_needs_parentheses(e: &Expr<'_>) -> bool { |
117 | let mut inner = e; | |
118 | while let ExprKind::Binary(_, i, _) | |
119 | | ExprKind::Call(i, _) | |
120 | | ExprKind::Cast(i, _) | |
121 | | ExprKind::Type(i, _) | |
add651ee | 122 | | ExprKind::Index(i, _, _) = inner.kind |
a2a8927a XL |
123 | { |
124 | if matches!( | |
125 | i.kind, | |
126 | ExprKind::Block(..) | |
127 | | ExprKind::ConstBlock(..) | |
128 | | ExprKind::If(..) | |
129 | | ExprKind::Loop(..) | |
130 | | ExprKind::Match(..) | |
131 | ) { | |
132 | return true; | |
133 | } | |
134 | inner = i; | |
135 | } | |
136 | false | |
137 | } | |
138 | ||
139 | fn is_parent_stmt(cx: &LateContext<'_>, id: HirId) -> bool { | |
140 | matches!( | |
141 | get_parent_node(cx.tcx, id), | |
142 | Some(Node::Stmt(..) | Node::Block(Block { stmts: &[], .. })) | |
143 | ) | |
144 | } | |
145 | ||
f20569fa XL |
146 | impl<'tcx> LateLintPass<'tcx> for NeedlessBool { |
147 | fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { | |
148 | use self::Expression::{Bool, RetBool}; | |
49aad941 | 149 | if e.span.from_expansion() || !span_extract_comment(cx.tcx.sess.source_map(), e.span).is_empty() { |
136023e0 XL |
150 | return; |
151 | } | |
94222f64 XL |
152 | if let Some(higher::If { |
153 | cond, | |
154 | then, | |
155 | r#else: Some(r#else), | |
156 | }) = higher::If::hir(e) | |
157 | { | |
f20569fa XL |
158 | let reduce = |ret, not| { |
159 | let mut applicability = Applicability::MachineApplicable; | |
94222f64 | 160 | let snip = Sugg::hir_with_applicability(cx, cond, "<predicate>", &mut applicability); |
f20569fa XL |
161 | let mut snip = if not { !snip } else { snip }; |
162 | ||
163 | if ret { | |
164 | snip = snip.make_return(); | |
165 | } | |
166 | ||
cdc7bbd5 | 167 | if is_else_clause(cx.tcx, e) { |
17df50a5 | 168 | snip = snip.blockify(); |
f20569fa XL |
169 | } |
170 | ||
a2a8927a XL |
171 | if condition_needs_parentheses(cond) && is_parent_stmt(cx, e.hir_id) { |
172 | snip = snip.maybe_par(); | |
173 | } | |
174 | ||
f20569fa XL |
175 | span_lint_and_sugg( |
176 | cx, | |
177 | NEEDLESS_BOOL, | |
178 | e.span, | |
179 | "this if-then-else expression returns a bool literal", | |
180 | "you can reduce it to", | |
181 | snip.to_string(), | |
182 | applicability, | |
183 | ); | |
184 | }; | |
a2a8927a XL |
185 | if let Some((a, b)) = fetch_bool_block(then).and_then(|a| Some((a, fetch_bool_block(r#else)?))) { |
186 | match (a, b) { | |
f20569fa XL |
187 | (RetBool(true), RetBool(true)) | (Bool(true), Bool(true)) => { |
188 | span_lint( | |
189 | cx, | |
190 | NEEDLESS_BOOL, | |
191 | e.span, | |
192 | "this if-then-else expression will always return true", | |
193 | ); | |
194 | }, | |
195 | (RetBool(false), RetBool(false)) | (Bool(false), Bool(false)) => { | |
196 | span_lint( | |
197 | cx, | |
198 | NEEDLESS_BOOL, | |
199 | e.span, | |
200 | "this if-then-else expression will always return false", | |
201 | ); | |
202 | }, | |
203 | (RetBool(true), RetBool(false)) => reduce(true, false), | |
204 | (Bool(true), Bool(false)) => reduce(false, false), | |
205 | (RetBool(false), RetBool(true)) => reduce(true, true), | |
206 | (Bool(false), Bool(true)) => reduce(false, true), | |
207 | _ => (), | |
208 | } | |
f20569fa | 209 | } |
49aad941 FG |
210 | if let Some((lhs_a, a)) = fetch_assign(then) && |
211 | let Some((lhs_b, b)) = fetch_assign(r#else) && | |
212 | SpanlessEq::new(cx).eq_expr(lhs_a, lhs_b) | |
213 | { | |
214 | let mut applicability = Applicability::MachineApplicable; | |
215 | let cond = Sugg::hir_with_applicability(cx, cond, "..", &mut applicability); | |
216 | let lhs = snippet_with_applicability(cx, lhs_a.span, "..", &mut applicability); | |
217 | let sugg = if a == b { | |
218 | format!("{cond}; {lhs} = {a:?};") | |
219 | } else { | |
220 | format!("{lhs} = {};", if a { cond } else { !cond }) | |
221 | }; | |
222 | span_lint_and_sugg( | |
223 | cx, | |
224 | NEEDLESS_BOOL_ASSIGN, | |
225 | e.span, | |
226 | "this if-then-else expression assigns a bool literal", | |
227 | "you can reduce it to", | |
228 | sugg, | |
229 | applicability | |
230 | ); | |
231 | } | |
f20569fa XL |
232 | } |
233 | } | |
234 | } | |
235 | ||
236 | declare_lint_pass!(BoolComparison => [BOOL_COMPARISON]); | |
237 | ||
238 | impl<'tcx> LateLintPass<'tcx> for BoolComparison { | |
239 | fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { | |
240 | if e.span.from_expansion() { | |
241 | return; | |
242 | } | |
243 | ||
244 | if let ExprKind::Binary(Spanned { node, .. }, ..) = e.kind { | |
245 | let ignore_case = None::<(fn(_) -> _, &str)>; | |
246 | let ignore_no_literal = None::<(fn(_, _) -> _, &str)>; | |
247 | match node { | |
248 | BinOpKind::Eq => { | |
249 | let true_case = Some((|h| h, "equality checks against true are unnecessary")); | |
250 | let false_case = Some(( | |
a2a8927a | 251 | |h: Sugg<'tcx>| !h, |
f20569fa XL |
252 | "equality checks against false can be replaced by a negation", |
253 | )); | |
17df50a5 | 254 | check_comparison(cx, e, true_case, false_case, true_case, false_case, ignore_no_literal); |
f20569fa XL |
255 | }, |
256 | BinOpKind::Ne => { | |
257 | let true_case = Some(( | |
a2a8927a | 258 | |h: Sugg<'tcx>| !h, |
f20569fa XL |
259 | "inequality checks against true can be replaced by a negation", |
260 | )); | |
261 | let false_case = Some((|h| h, "inequality checks against false are unnecessary")); | |
17df50a5 | 262 | check_comparison(cx, e, true_case, false_case, true_case, false_case, ignore_no_literal); |
f20569fa XL |
263 | }, |
264 | BinOpKind::Lt => check_comparison( | |
265 | cx, | |
266 | e, | |
267 | ignore_case, | |
268 | Some((|h| h, "greater than checks against false are unnecessary")), | |
269 | Some(( | |
a2a8927a | 270 | |h: Sugg<'tcx>| !h, |
f20569fa XL |
271 | "less than comparison against true can be replaced by a negation", |
272 | )), | |
273 | ignore_case, | |
274 | Some(( | |
a2a8927a | 275 | |l: Sugg<'tcx>, r: Sugg<'tcx>| (!l).bit_and(&r), |
f20569fa XL |
276 | "order comparisons between booleans can be simplified", |
277 | )), | |
278 | ), | |
279 | BinOpKind::Gt => check_comparison( | |
280 | cx, | |
281 | e, | |
282 | Some(( | |
a2a8927a | 283 | |h: Sugg<'tcx>| !h, |
f20569fa XL |
284 | "less than comparison against true can be replaced by a negation", |
285 | )), | |
286 | ignore_case, | |
287 | ignore_case, | |
288 | Some((|h| h, "greater than checks against false are unnecessary")), | |
289 | Some(( | |
a2a8927a | 290 | |l: Sugg<'tcx>, r: Sugg<'tcx>| l.bit_and(&(!r)), |
f20569fa XL |
291 | "order comparisons between booleans can be simplified", |
292 | )), | |
293 | ), | |
294 | _ => (), | |
295 | } | |
296 | } | |
297 | } | |
298 | } | |
299 | ||
300 | struct ExpressionInfoWithSpan { | |
301 | one_side_is_unary_not: bool, | |
302 | left_span: Span, | |
303 | right_span: Span, | |
304 | } | |
305 | ||
306 | fn is_unary_not(e: &Expr<'_>) -> (bool, Span) { | |
307 | if let ExprKind::Unary(UnOp::Not, operand) = e.kind { | |
308 | return (true, operand.span); | |
309 | } | |
310 | (false, e.span) | |
311 | } | |
312 | ||
313 | fn one_side_is_unary_not<'tcx>(left_side: &'tcx Expr<'_>, right_side: &'tcx Expr<'_>) -> ExpressionInfoWithSpan { | |
314 | let left = is_unary_not(left_side); | |
315 | let right = is_unary_not(right_side); | |
316 | ||
317 | ExpressionInfoWithSpan { | |
318 | one_side_is_unary_not: left.0 != right.0, | |
319 | left_span: left.1, | |
320 | right_span: right.1, | |
321 | } | |
322 | } | |
323 | ||
324 | fn check_comparison<'a, 'tcx>( | |
325 | cx: &LateContext<'tcx>, | |
326 | e: &'tcx Expr<'_>, | |
327 | left_true: Option<(impl FnOnce(Sugg<'a>) -> Sugg<'a>, &str)>, | |
328 | left_false: Option<(impl FnOnce(Sugg<'a>) -> Sugg<'a>, &str)>, | |
329 | right_true: Option<(impl FnOnce(Sugg<'a>) -> Sugg<'a>, &str)>, | |
330 | right_false: Option<(impl FnOnce(Sugg<'a>) -> Sugg<'a>, &str)>, | |
331 | no_literal: Option<(impl FnOnce(Sugg<'a>, Sugg<'a>) -> Sugg<'a>, &str)>, | |
332 | ) { | |
cdc7bbd5 | 333 | if let ExprKind::Binary(op, left_side, right_side) = e.kind { |
f20569fa XL |
334 | let (l_ty, r_ty) = ( |
335 | cx.typeck_results().expr_ty(left_side), | |
336 | cx.typeck_results().expr_ty(right_side), | |
337 | ); | |
338 | if is_expn_of(left_side.span, "cfg").is_some() || is_expn_of(right_side.span, "cfg").is_some() { | |
339 | return; | |
340 | } | |
341 | if l_ty.is_bool() && r_ty.is_bool() { | |
342 | let mut applicability = Applicability::MachineApplicable; | |
343 | ||
c295e0f8 | 344 | if op.node == BinOpKind::Eq { |
cdc7bbd5 | 345 | let expression_info = one_side_is_unary_not(left_side, right_side); |
f20569fa XL |
346 | if expression_info.one_side_is_unary_not { |
347 | span_lint_and_sugg( | |
348 | cx, | |
349 | BOOL_COMPARISON, | |
350 | e.span, | |
351 | "this comparison might be written more concisely", | |
352 | "try simplifying it as shown", | |
353 | format!( | |
354 | "{} != {}", | |
355 | snippet_with_applicability(cx, expression_info.left_span, "..", &mut applicability), | |
356 | snippet_with_applicability(cx, expression_info.right_span, "..", &mut applicability) | |
357 | ), | |
358 | applicability, | |
17df50a5 | 359 | ); |
f20569fa XL |
360 | } |
361 | } | |
362 | ||
363 | match (fetch_bool_expr(left_side), fetch_bool_expr(right_side)) { | |
a2a8927a | 364 | (Some(true), None) => left_true.map_or((), |(h, m)| { |
17df50a5 | 365 | suggest_bool_comparison(cx, e, right_side, applicability, m, h); |
f20569fa | 366 | }), |
a2a8927a | 367 | (None, Some(true)) => right_true.map_or((), |(h, m)| { |
17df50a5 | 368 | suggest_bool_comparison(cx, e, left_side, applicability, m, h); |
f20569fa | 369 | }), |
a2a8927a | 370 | (Some(false), None) => left_false.map_or((), |(h, m)| { |
17df50a5 | 371 | suggest_bool_comparison(cx, e, right_side, applicability, m, h); |
f20569fa | 372 | }), |
a2a8927a | 373 | (None, Some(false)) => right_false.map_or((), |(h, m)| { |
17df50a5 | 374 | suggest_bool_comparison(cx, e, left_side, applicability, m, h); |
f20569fa | 375 | }), |
a2a8927a | 376 | (None, None) => no_literal.map_or((), |(h, m)| { |
f20569fa XL |
377 | let left_side = Sugg::hir_with_applicability(cx, left_side, "..", &mut applicability); |
378 | let right_side = Sugg::hir_with_applicability(cx, right_side, "..", &mut applicability); | |
379 | span_lint_and_sugg( | |
380 | cx, | |
381 | BOOL_COMPARISON, | |
382 | e.span, | |
383 | m, | |
384 | "try simplifying it as shown", | |
385 | h(left_side, right_side).to_string(), | |
386 | applicability, | |
17df50a5 | 387 | ); |
f20569fa XL |
388 | }), |
389 | _ => (), | |
390 | } | |
391 | } | |
392 | } | |
393 | } | |
394 | ||
395 | fn suggest_bool_comparison<'a, 'tcx>( | |
396 | cx: &LateContext<'tcx>, | |
397 | e: &'tcx Expr<'_>, | |
398 | expr: &Expr<'_>, | |
353b0b11 | 399 | mut app: Applicability, |
f20569fa XL |
400 | message: &str, |
401 | conv_hint: impl FnOnce(Sugg<'a>) -> Sugg<'a>, | |
402 | ) { | |
353b0b11 | 403 | let hint = Sugg::hir_with_context(cx, expr, e.span.ctxt(), "..", &mut app); |
f20569fa XL |
404 | span_lint_and_sugg( |
405 | cx, | |
406 | BOOL_COMPARISON, | |
407 | e.span, | |
408 | message, | |
409 | "try simplifying it as shown", | |
410 | conv_hint(hint).to_string(), | |
353b0b11 | 411 | app, |
f20569fa XL |
412 | ); |
413 | } | |
414 | ||
415 | enum Expression { | |
416 | Bool(bool), | |
417 | RetBool(bool), | |
f20569fa XL |
418 | } |
419 | ||
a2a8927a XL |
420 | fn fetch_bool_block(expr: &Expr<'_>) -> Option<Expression> { |
421 | match peel_blocks_with_stmt(expr).kind { | |
422 | ExprKind::Ret(Some(ret)) => Some(Expression::RetBool(fetch_bool_expr(ret)?)), | |
423 | _ => Some(Expression::Bool(fetch_bool_expr(expr)?)), | |
f20569fa XL |
424 | } |
425 | } | |
426 | ||
a2a8927a | 427 | fn fetch_bool_expr(expr: &Expr<'_>) -> Option<bool> { |
49aad941 | 428 | if let ExprKind::Lit(lit_ptr) = peel_blocks(expr).kind { |
a2a8927a XL |
429 | if let LitKind::Bool(value) = lit_ptr.node { |
430 | return Some(value); | |
431 | } | |
f20569fa | 432 | } |
a2a8927a | 433 | None |
f20569fa | 434 | } |
49aad941 FG |
435 | |
436 | fn fetch_assign<'tcx>(expr: &'tcx Expr<'tcx>) -> Option<(&'tcx Expr<'tcx>, bool)> { | |
437 | if let ExprKind::Assign(lhs, rhs, _) = peel_blocks_with_stmt(expr).kind { | |
438 | fetch_bool_expr(rhs).map(|b| (lhs, b)) | |
439 | } else { | |
440 | None | |
441 | } | |
442 | } |