]>
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::{ |
31ef2f64 FG |
9 | higher, is_block_like, is_else_clause, is_expn_of, is_parent_stmt, peel_blocks, peel_blocks_with_stmt, |
10 | span_extract_comment, SpanlessEq, | |
49aad941 | 11 | }; |
f20569fa XL |
12 | use rustc_ast::ast::LitKind; |
13 | use rustc_errors::Applicability; | |
e8be2606 | 14 | use rustc_hir::{BinOpKind, Expr, ExprKind, UnOp}; |
f20569fa | 15 | use rustc_lint::{LateContext, LateLintPass}; |
4b012472 | 16 | use rustc_session::declare_lint_pass; |
f20569fa XL |
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 |
ed00b5ec | 35 | /// ```no_run |
923072b8 | 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: | |
ed00b5ec | 46 | /// ```no_run |
923072b8 | 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 | 123 | { |
31ef2f64 | 124 | if is_block_like(i) { |
a2a8927a XL |
125 | return true; |
126 | } | |
127 | inner = i; | |
128 | } | |
129 | false | |
130 | } | |
131 | ||
f20569fa XL |
132 | impl<'tcx> LateLintPass<'tcx> for NeedlessBool { |
133 | fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { | |
134 | use self::Expression::{Bool, RetBool}; | |
49aad941 | 135 | if e.span.from_expansion() || !span_extract_comment(cx.tcx.sess.source_map(), e.span).is_empty() { |
136023e0 XL |
136 | return; |
137 | } | |
94222f64 XL |
138 | if let Some(higher::If { |
139 | cond, | |
140 | then, | |
141 | r#else: Some(r#else), | |
142 | }) = higher::If::hir(e) | |
143 | { | |
f20569fa XL |
144 | let reduce = |ret, not| { |
145 | let mut applicability = Applicability::MachineApplicable; | |
94222f64 | 146 | let snip = Sugg::hir_with_applicability(cx, cond, "<predicate>", &mut applicability); |
f20569fa XL |
147 | let mut snip = if not { !snip } else { snip }; |
148 | ||
149 | if ret { | |
150 | snip = snip.make_return(); | |
151 | } | |
152 | ||
cdc7bbd5 | 153 | if is_else_clause(cx.tcx, e) { |
17df50a5 | 154 | snip = snip.blockify(); |
f20569fa XL |
155 | } |
156 | ||
a2a8927a XL |
157 | if condition_needs_parentheses(cond) && is_parent_stmt(cx, e.hir_id) { |
158 | snip = snip.maybe_par(); | |
159 | } | |
160 | ||
f20569fa XL |
161 | span_lint_and_sugg( |
162 | cx, | |
163 | NEEDLESS_BOOL, | |
164 | e.span, | |
165 | "this if-then-else expression returns a bool literal", | |
166 | "you can reduce it to", | |
167 | snip.to_string(), | |
168 | applicability, | |
169 | ); | |
170 | }; | |
a2a8927a XL |
171 | if let Some((a, b)) = fetch_bool_block(then).and_then(|a| Some((a, fetch_bool_block(r#else)?))) { |
172 | match (a, b) { | |
f20569fa XL |
173 | (RetBool(true), RetBool(true)) | (Bool(true), Bool(true)) => { |
174 | span_lint( | |
175 | cx, | |
176 | NEEDLESS_BOOL, | |
177 | e.span, | |
178 | "this if-then-else expression will always return true", | |
179 | ); | |
180 | }, | |
181 | (RetBool(false), RetBool(false)) | (Bool(false), Bool(false)) => { | |
182 | span_lint( | |
183 | cx, | |
184 | NEEDLESS_BOOL, | |
185 | e.span, | |
186 | "this if-then-else expression will always return false", | |
187 | ); | |
188 | }, | |
189 | (RetBool(true), RetBool(false)) => reduce(true, false), | |
190 | (Bool(true), Bool(false)) => reduce(false, false), | |
191 | (RetBool(false), RetBool(true)) => reduce(true, true), | |
192 | (Bool(false), Bool(true)) => reduce(false, true), | |
193 | _ => (), | |
194 | } | |
f20569fa | 195 | } |
ed00b5ec FG |
196 | if let Some((lhs_a, a)) = fetch_assign(then) |
197 | && let Some((lhs_b, b)) = fetch_assign(r#else) | |
198 | && SpanlessEq::new(cx).eq_expr(lhs_a, lhs_b) | |
49aad941 FG |
199 | { |
200 | let mut applicability = Applicability::MachineApplicable; | |
201 | let cond = Sugg::hir_with_applicability(cx, cond, "..", &mut applicability); | |
202 | let lhs = snippet_with_applicability(cx, lhs_a.span, "..", &mut applicability); | |
203 | let sugg = if a == b { | |
204 | format!("{cond}; {lhs} = {a:?};") | |
205 | } else { | |
206 | format!("{lhs} = {};", if a { cond } else { !cond }) | |
207 | }; | |
208 | span_lint_and_sugg( | |
209 | cx, | |
210 | NEEDLESS_BOOL_ASSIGN, | |
211 | e.span, | |
212 | "this if-then-else expression assigns a bool literal", | |
213 | "you can reduce it to", | |
214 | sugg, | |
ed00b5ec | 215 | applicability, |
49aad941 FG |
216 | ); |
217 | } | |
f20569fa XL |
218 | } |
219 | } | |
220 | } | |
221 | ||
222 | declare_lint_pass!(BoolComparison => [BOOL_COMPARISON]); | |
223 | ||
224 | impl<'tcx> LateLintPass<'tcx> for BoolComparison { | |
225 | fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { | |
226 | if e.span.from_expansion() { | |
227 | return; | |
228 | } | |
229 | ||
230 | if let ExprKind::Binary(Spanned { node, .. }, ..) = e.kind { | |
231 | let ignore_case = None::<(fn(_) -> _, &str)>; | |
232 | let ignore_no_literal = None::<(fn(_, _) -> _, &str)>; | |
233 | match node { | |
234 | BinOpKind::Eq => { | |
235 | let true_case = Some((|h| h, "equality checks against true are unnecessary")); | |
236 | let false_case = Some(( | |
a2a8927a | 237 | |h: Sugg<'tcx>| !h, |
f20569fa XL |
238 | "equality checks against false can be replaced by a negation", |
239 | )); | |
17df50a5 | 240 | check_comparison(cx, e, true_case, false_case, true_case, false_case, ignore_no_literal); |
f20569fa XL |
241 | }, |
242 | BinOpKind::Ne => { | |
243 | let true_case = Some(( | |
a2a8927a | 244 | |h: Sugg<'tcx>| !h, |
f20569fa XL |
245 | "inequality checks against true can be replaced by a negation", |
246 | )); | |
247 | let false_case = Some((|h| h, "inequality checks against false are unnecessary")); | |
17df50a5 | 248 | check_comparison(cx, e, true_case, false_case, true_case, false_case, ignore_no_literal); |
f20569fa XL |
249 | }, |
250 | BinOpKind::Lt => check_comparison( | |
251 | cx, | |
252 | e, | |
253 | ignore_case, | |
254 | Some((|h| h, "greater than checks against false are unnecessary")), | |
255 | Some(( | |
a2a8927a | 256 | |h: Sugg<'tcx>| !h, |
f20569fa XL |
257 | "less than comparison against true can be replaced by a negation", |
258 | )), | |
259 | ignore_case, | |
260 | Some(( | |
a2a8927a | 261 | |l: Sugg<'tcx>, r: Sugg<'tcx>| (!l).bit_and(&r), |
f20569fa XL |
262 | "order comparisons between booleans can be simplified", |
263 | )), | |
264 | ), | |
265 | BinOpKind::Gt => check_comparison( | |
266 | cx, | |
267 | e, | |
268 | Some(( | |
a2a8927a | 269 | |h: Sugg<'tcx>| !h, |
f20569fa XL |
270 | "less than comparison against true can be replaced by a negation", |
271 | )), | |
272 | ignore_case, | |
273 | ignore_case, | |
274 | Some((|h| h, "greater than checks against false are unnecessary")), | |
275 | Some(( | |
a2a8927a | 276 | |l: Sugg<'tcx>, r: Sugg<'tcx>| l.bit_and(&(!r)), |
f20569fa XL |
277 | "order comparisons between booleans can be simplified", |
278 | )), | |
279 | ), | |
280 | _ => (), | |
281 | } | |
282 | } | |
283 | } | |
284 | } | |
285 | ||
286 | struct ExpressionInfoWithSpan { | |
287 | one_side_is_unary_not: bool, | |
288 | left_span: Span, | |
289 | right_span: Span, | |
290 | } | |
291 | ||
292 | fn is_unary_not(e: &Expr<'_>) -> (bool, Span) { | |
293 | if let ExprKind::Unary(UnOp::Not, operand) = e.kind { | |
294 | return (true, operand.span); | |
295 | } | |
296 | (false, e.span) | |
297 | } | |
298 | ||
299 | fn one_side_is_unary_not<'tcx>(left_side: &'tcx Expr<'_>, right_side: &'tcx Expr<'_>) -> ExpressionInfoWithSpan { | |
300 | let left = is_unary_not(left_side); | |
301 | let right = is_unary_not(right_side); | |
302 | ||
303 | ExpressionInfoWithSpan { | |
304 | one_side_is_unary_not: left.0 != right.0, | |
305 | left_span: left.1, | |
306 | right_span: right.1, | |
307 | } | |
308 | } | |
309 | ||
310 | fn check_comparison<'a, 'tcx>( | |
311 | cx: &LateContext<'tcx>, | |
312 | e: &'tcx Expr<'_>, | |
e8be2606 FG |
313 | left_true: Option<(impl FnOnce(Sugg<'a>) -> Sugg<'a>, &'static str)>, |
314 | left_false: Option<(impl FnOnce(Sugg<'a>) -> Sugg<'a>, &'static str)>, | |
315 | right_true: Option<(impl FnOnce(Sugg<'a>) -> Sugg<'a>, &'static str)>, | |
316 | right_false: Option<(impl FnOnce(Sugg<'a>) -> Sugg<'a>, &'static str)>, | |
317 | no_literal: Option<(impl FnOnce(Sugg<'a>, Sugg<'a>) -> Sugg<'a>, &'static str)>, | |
f20569fa | 318 | ) { |
cdc7bbd5 | 319 | if let ExprKind::Binary(op, left_side, right_side) = e.kind { |
f20569fa XL |
320 | let (l_ty, r_ty) = ( |
321 | cx.typeck_results().expr_ty(left_side), | |
322 | cx.typeck_results().expr_ty(right_side), | |
323 | ); | |
324 | if is_expn_of(left_side.span, "cfg").is_some() || is_expn_of(right_side.span, "cfg").is_some() { | |
325 | return; | |
326 | } | |
327 | if l_ty.is_bool() && r_ty.is_bool() { | |
328 | let mut applicability = Applicability::MachineApplicable; | |
c0240ec0 FG |
329 | // Eliminate parentheses in `e` by using the lo pos of lhs and hi pos of rhs, |
330 | // calling `source_callsite` make sure macros are handled correctly, see issue #9907 | |
331 | let binop_span = left_side | |
332 | .span | |
333 | .source_callsite() | |
334 | .with_hi(right_side.span.source_callsite().hi()); | |
f20569fa | 335 | |
c295e0f8 | 336 | if op.node == BinOpKind::Eq { |
cdc7bbd5 | 337 | let expression_info = one_side_is_unary_not(left_side, right_side); |
f20569fa XL |
338 | if expression_info.one_side_is_unary_not { |
339 | span_lint_and_sugg( | |
340 | cx, | |
341 | BOOL_COMPARISON, | |
c0240ec0 | 342 | binop_span, |
f20569fa XL |
343 | "this comparison might be written more concisely", |
344 | "try simplifying it as shown", | |
345 | format!( | |
346 | "{} != {}", | |
c0240ec0 FG |
347 | snippet_with_applicability( |
348 | cx, | |
349 | expression_info.left_span.source_callsite(), | |
350 | "..", | |
351 | &mut applicability | |
352 | ), | |
353 | snippet_with_applicability( | |
354 | cx, | |
355 | expression_info.right_span.source_callsite(), | |
356 | "..", | |
357 | &mut applicability | |
358 | ) | |
f20569fa XL |
359 | ), |
360 | applicability, | |
17df50a5 | 361 | ); |
f20569fa XL |
362 | } |
363 | } | |
364 | ||
365 | match (fetch_bool_expr(left_side), fetch_bool_expr(right_side)) { | |
a2a8927a | 366 | (Some(true), None) => left_true.map_or((), |(h, m)| { |
c0240ec0 | 367 | suggest_bool_comparison(cx, binop_span, right_side, applicability, m, h); |
f20569fa | 368 | }), |
a2a8927a | 369 | (None, Some(true)) => right_true.map_or((), |(h, m)| { |
c0240ec0 | 370 | suggest_bool_comparison(cx, binop_span, left_side, applicability, m, h); |
f20569fa | 371 | }), |
a2a8927a | 372 | (Some(false), None) => left_false.map_or((), |(h, m)| { |
c0240ec0 | 373 | suggest_bool_comparison(cx, binop_span, right_side, applicability, m, h); |
f20569fa | 374 | }), |
a2a8927a | 375 | (None, Some(false)) => right_false.map_or((), |(h, m)| { |
c0240ec0 | 376 | suggest_bool_comparison(cx, binop_span, left_side, applicability, m, h); |
f20569fa | 377 | }), |
a2a8927a | 378 | (None, None) => no_literal.map_or((), |(h, m)| { |
f20569fa XL |
379 | let left_side = Sugg::hir_with_applicability(cx, left_side, "..", &mut applicability); |
380 | let right_side = Sugg::hir_with_applicability(cx, right_side, "..", &mut applicability); | |
381 | span_lint_and_sugg( | |
382 | cx, | |
383 | BOOL_COMPARISON, | |
c0240ec0 | 384 | binop_span, |
f20569fa XL |
385 | m, |
386 | "try simplifying it as shown", | |
e8be2606 | 387 | h(left_side, right_side).into_string(), |
f20569fa | 388 | applicability, |
17df50a5 | 389 | ); |
f20569fa XL |
390 | }), |
391 | _ => (), | |
392 | } | |
393 | } | |
394 | } | |
395 | } | |
396 | ||
397 | fn suggest_bool_comparison<'a, 'tcx>( | |
398 | cx: &LateContext<'tcx>, | |
c0240ec0 | 399 | span: Span, |
f20569fa | 400 | expr: &Expr<'_>, |
353b0b11 | 401 | mut app: Applicability, |
e8be2606 | 402 | message: &'static str, |
f20569fa XL |
403 | conv_hint: impl FnOnce(Sugg<'a>) -> Sugg<'a>, |
404 | ) { | |
c0240ec0 | 405 | let hint = Sugg::hir_with_context(cx, expr, span.ctxt(), "..", &mut app); |
f20569fa XL |
406 | span_lint_and_sugg( |
407 | cx, | |
408 | BOOL_COMPARISON, | |
c0240ec0 | 409 | span, |
f20569fa XL |
410 | message, |
411 | "try simplifying it as shown", | |
e8be2606 | 412 | conv_hint(hint).into_string(), |
353b0b11 | 413 | app, |
f20569fa XL |
414 | ); |
415 | } | |
416 | ||
417 | enum Expression { | |
418 | Bool(bool), | |
419 | RetBool(bool), | |
f20569fa XL |
420 | } |
421 | ||
a2a8927a XL |
422 | fn fetch_bool_block(expr: &Expr<'_>) -> Option<Expression> { |
423 | match peel_blocks_with_stmt(expr).kind { | |
424 | ExprKind::Ret(Some(ret)) => Some(Expression::RetBool(fetch_bool_expr(ret)?)), | |
425 | _ => Some(Expression::Bool(fetch_bool_expr(expr)?)), | |
f20569fa XL |
426 | } |
427 | } | |
428 | ||
a2a8927a | 429 | fn fetch_bool_expr(expr: &Expr<'_>) -> Option<bool> { |
49aad941 | 430 | if let ExprKind::Lit(lit_ptr) = peel_blocks(expr).kind { |
a2a8927a XL |
431 | if let LitKind::Bool(value) = lit_ptr.node { |
432 | return Some(value); | |
433 | } | |
f20569fa | 434 | } |
a2a8927a | 435 | None |
f20569fa | 436 | } |
49aad941 FG |
437 | |
438 | fn fetch_assign<'tcx>(expr: &'tcx Expr<'tcx>) -> Option<(&'tcx Expr<'tcx>, bool)> { | |
439 | if let ExprKind::Assign(lhs, rhs, _) = peel_blocks_with_stmt(expr).kind { | |
440 | fetch_bool_expr(rhs).map(|b| (lhs, b)) | |
441 | } else { | |
442 | None | |
443 | } | |
444 | } |