]>
Commit | Line | Data |
---|---|---|
f20569fa XL |
1 | //! Checks for needless boolean results of if-else expressions |
2 | //! | |
3 | //! This lint is **warn** by default | |
4 | ||
5 | use crate::utils::sugg::Sugg; | |
6 | use crate::utils::{is_expn_of, parent_node_is_if_expr, snippet_with_applicability, span_lint, span_lint_and_sugg}; | |
7 | use rustc_ast::ast::LitKind; | |
8 | use rustc_errors::Applicability; | |
9 | use rustc_hir::{BinOpKind, Block, Expr, ExprKind, StmtKind, UnOp}; | |
10 | use rustc_lint::{LateContext, LateLintPass}; | |
11 | use rustc_session::{declare_lint_pass, declare_tool_lint}; | |
12 | use rustc_span::source_map::Spanned; | |
13 | use rustc_span::Span; | |
14 | ||
15 | declare_clippy_lint! { | |
16 | /// **What it does:** Checks for expressions of the form `if c { true } else { | |
17 | /// false }` (or vice versa) and suggests using the condition directly. | |
18 | /// | |
19 | /// **Why is this bad?** Redundant code. | |
20 | /// | |
21 | /// **Known problems:** Maybe false positives: Sometimes, the two branches are | |
22 | /// painstakingly documented (which we, of course, do not detect), so they *may* | |
23 | /// have some value. Even then, the documentation can be rewritten to match the | |
24 | /// shorter code. | |
25 | /// | |
26 | /// **Example:** | |
27 | /// ```rust,ignore | |
28 | /// if x { | |
29 | /// false | |
30 | /// } else { | |
31 | /// true | |
32 | /// } | |
33 | /// ``` | |
34 | /// Could be written as | |
35 | /// ```rust,ignore | |
36 | /// !x | |
37 | /// ``` | |
38 | pub NEEDLESS_BOOL, | |
39 | complexity, | |
40 | "if-statements with plain booleans in the then- and else-clause, e.g., `if p { true } else { false }`" | |
41 | } | |
42 | ||
43 | declare_clippy_lint! { | |
44 | /// **What it does:** Checks for expressions of the form `x == true`, | |
45 | /// `x != true` and order comparisons such as `x < true` (or vice versa) and | |
46 | /// suggest using the variable directly. | |
47 | /// | |
48 | /// **Why is this bad?** Unnecessary code. | |
49 | /// | |
50 | /// **Known problems:** None. | |
51 | /// | |
52 | /// **Example:** | |
53 | /// ```rust,ignore | |
54 | /// if x == true {} | |
55 | /// if y == false {} | |
56 | /// ``` | |
57 | /// use `x` directly: | |
58 | /// ```rust,ignore | |
59 | /// if x {} | |
60 | /// if !y {} | |
61 | /// ``` | |
62 | pub BOOL_COMPARISON, | |
63 | complexity, | |
64 | "comparing a variable to a boolean, e.g., `if x == true` or `if x != true`" | |
65 | } | |
66 | ||
67 | declare_lint_pass!(NeedlessBool => [NEEDLESS_BOOL]); | |
68 | ||
69 | impl<'tcx> LateLintPass<'tcx> for NeedlessBool { | |
70 | fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { | |
71 | use self::Expression::{Bool, RetBool}; | |
72 | if let ExprKind::If(ref pred, ref then_block, Some(ref else_expr)) = e.kind { | |
73 | let reduce = |ret, not| { | |
74 | let mut applicability = Applicability::MachineApplicable; | |
75 | let snip = Sugg::hir_with_applicability(cx, pred, "<predicate>", &mut applicability); | |
76 | let mut snip = if not { !snip } else { snip }; | |
77 | ||
78 | if ret { | |
79 | snip = snip.make_return(); | |
80 | } | |
81 | ||
82 | if parent_node_is_if_expr(&e, &cx) { | |
83 | snip = snip.blockify() | |
84 | } | |
85 | ||
86 | span_lint_and_sugg( | |
87 | cx, | |
88 | NEEDLESS_BOOL, | |
89 | e.span, | |
90 | "this if-then-else expression returns a bool literal", | |
91 | "you can reduce it to", | |
92 | snip.to_string(), | |
93 | applicability, | |
94 | ); | |
95 | }; | |
96 | if let ExprKind::Block(ref then_block, _) = then_block.kind { | |
97 | match (fetch_bool_block(then_block), fetch_bool_expr(else_expr)) { | |
98 | (RetBool(true), RetBool(true)) | (Bool(true), Bool(true)) => { | |
99 | span_lint( | |
100 | cx, | |
101 | NEEDLESS_BOOL, | |
102 | e.span, | |
103 | "this if-then-else expression will always return true", | |
104 | ); | |
105 | }, | |
106 | (RetBool(false), RetBool(false)) | (Bool(false), Bool(false)) => { | |
107 | span_lint( | |
108 | cx, | |
109 | NEEDLESS_BOOL, | |
110 | e.span, | |
111 | "this if-then-else expression will always return false", | |
112 | ); | |
113 | }, | |
114 | (RetBool(true), RetBool(false)) => reduce(true, false), | |
115 | (Bool(true), Bool(false)) => reduce(false, false), | |
116 | (RetBool(false), RetBool(true)) => reduce(true, true), | |
117 | (Bool(false), Bool(true)) => reduce(false, true), | |
118 | _ => (), | |
119 | } | |
120 | } else { | |
121 | panic!("IfExpr `then` node is not an `ExprKind::Block`"); | |
122 | } | |
123 | } | |
124 | } | |
125 | } | |
126 | ||
127 | declare_lint_pass!(BoolComparison => [BOOL_COMPARISON]); | |
128 | ||
129 | impl<'tcx> LateLintPass<'tcx> for BoolComparison { | |
130 | fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { | |
131 | if e.span.from_expansion() { | |
132 | return; | |
133 | } | |
134 | ||
135 | if let ExprKind::Binary(Spanned { node, .. }, ..) = e.kind { | |
136 | let ignore_case = None::<(fn(_) -> _, &str)>; | |
137 | let ignore_no_literal = None::<(fn(_, _) -> _, &str)>; | |
138 | match node { | |
139 | BinOpKind::Eq => { | |
140 | let true_case = Some((|h| h, "equality checks against true are unnecessary")); | |
141 | let false_case = Some(( | |
142 | |h: Sugg<'_>| !h, | |
143 | "equality checks against false can be replaced by a negation", | |
144 | )); | |
145 | check_comparison(cx, e, true_case, false_case, true_case, false_case, ignore_no_literal) | |
146 | }, | |
147 | BinOpKind::Ne => { | |
148 | let true_case = Some(( | |
149 | |h: Sugg<'_>| !h, | |
150 | "inequality checks against true can be replaced by a negation", | |
151 | )); | |
152 | let false_case = Some((|h| h, "inequality checks against false are unnecessary")); | |
153 | check_comparison(cx, e, true_case, false_case, true_case, false_case, ignore_no_literal) | |
154 | }, | |
155 | BinOpKind::Lt => check_comparison( | |
156 | cx, | |
157 | e, | |
158 | ignore_case, | |
159 | Some((|h| h, "greater than checks against false are unnecessary")), | |
160 | Some(( | |
161 | |h: Sugg<'_>| !h, | |
162 | "less than comparison against true can be replaced by a negation", | |
163 | )), | |
164 | ignore_case, | |
165 | Some(( | |
166 | |l: Sugg<'_>, r: Sugg<'_>| (!l).bit_and(&r), | |
167 | "order comparisons between booleans can be simplified", | |
168 | )), | |
169 | ), | |
170 | BinOpKind::Gt => check_comparison( | |
171 | cx, | |
172 | e, | |
173 | Some(( | |
174 | |h: Sugg<'_>| !h, | |
175 | "less than comparison against true can be replaced by a negation", | |
176 | )), | |
177 | ignore_case, | |
178 | ignore_case, | |
179 | Some((|h| h, "greater than checks against false are unnecessary")), | |
180 | Some(( | |
181 | |l: Sugg<'_>, r: Sugg<'_>| l.bit_and(&(!r)), | |
182 | "order comparisons between booleans can be simplified", | |
183 | )), | |
184 | ), | |
185 | _ => (), | |
186 | } | |
187 | } | |
188 | } | |
189 | } | |
190 | ||
191 | struct ExpressionInfoWithSpan { | |
192 | one_side_is_unary_not: bool, | |
193 | left_span: Span, | |
194 | right_span: Span, | |
195 | } | |
196 | ||
197 | fn is_unary_not(e: &Expr<'_>) -> (bool, Span) { | |
198 | if let ExprKind::Unary(UnOp::Not, operand) = e.kind { | |
199 | return (true, operand.span); | |
200 | } | |
201 | (false, e.span) | |
202 | } | |
203 | ||
204 | fn one_side_is_unary_not<'tcx>(left_side: &'tcx Expr<'_>, right_side: &'tcx Expr<'_>) -> ExpressionInfoWithSpan { | |
205 | let left = is_unary_not(left_side); | |
206 | let right = is_unary_not(right_side); | |
207 | ||
208 | ExpressionInfoWithSpan { | |
209 | one_side_is_unary_not: left.0 != right.0, | |
210 | left_span: left.1, | |
211 | right_span: right.1, | |
212 | } | |
213 | } | |
214 | ||
215 | fn check_comparison<'a, 'tcx>( | |
216 | cx: &LateContext<'tcx>, | |
217 | e: &'tcx Expr<'_>, | |
218 | left_true: Option<(impl FnOnce(Sugg<'a>) -> Sugg<'a>, &str)>, | |
219 | left_false: Option<(impl FnOnce(Sugg<'a>) -> Sugg<'a>, &str)>, | |
220 | right_true: Option<(impl FnOnce(Sugg<'a>) -> Sugg<'a>, &str)>, | |
221 | right_false: Option<(impl FnOnce(Sugg<'a>) -> Sugg<'a>, &str)>, | |
222 | no_literal: Option<(impl FnOnce(Sugg<'a>, Sugg<'a>) -> Sugg<'a>, &str)>, | |
223 | ) { | |
224 | use self::Expression::{Bool, Other}; | |
225 | ||
226 | if let ExprKind::Binary(op, ref left_side, ref right_side) = e.kind { | |
227 | let (l_ty, r_ty) = ( | |
228 | cx.typeck_results().expr_ty(left_side), | |
229 | cx.typeck_results().expr_ty(right_side), | |
230 | ); | |
231 | if is_expn_of(left_side.span, "cfg").is_some() || is_expn_of(right_side.span, "cfg").is_some() { | |
232 | return; | |
233 | } | |
234 | if l_ty.is_bool() && r_ty.is_bool() { | |
235 | let mut applicability = Applicability::MachineApplicable; | |
236 | ||
237 | if let BinOpKind::Eq = op.node { | |
238 | let expression_info = one_side_is_unary_not(&left_side, &right_side); | |
239 | if expression_info.one_side_is_unary_not { | |
240 | span_lint_and_sugg( | |
241 | cx, | |
242 | BOOL_COMPARISON, | |
243 | e.span, | |
244 | "this comparison might be written more concisely", | |
245 | "try simplifying it as shown", | |
246 | format!( | |
247 | "{} != {}", | |
248 | snippet_with_applicability(cx, expression_info.left_span, "..", &mut applicability), | |
249 | snippet_with_applicability(cx, expression_info.right_span, "..", &mut applicability) | |
250 | ), | |
251 | applicability, | |
252 | ) | |
253 | } | |
254 | } | |
255 | ||
256 | match (fetch_bool_expr(left_side), fetch_bool_expr(right_side)) { | |
257 | (Bool(true), Other) => left_true.map_or((), |(h, m)| { | |
258 | suggest_bool_comparison(cx, e, right_side, applicability, m, h) | |
259 | }), | |
260 | (Other, Bool(true)) => right_true.map_or((), |(h, m)| { | |
261 | suggest_bool_comparison(cx, e, left_side, applicability, m, h) | |
262 | }), | |
263 | (Bool(false), Other) => left_false.map_or((), |(h, m)| { | |
264 | suggest_bool_comparison(cx, e, right_side, applicability, m, h) | |
265 | }), | |
266 | (Other, Bool(false)) => right_false.map_or((), |(h, m)| { | |
267 | suggest_bool_comparison(cx, e, left_side, applicability, m, h) | |
268 | }), | |
269 | (Other, Other) => no_literal.map_or((), |(h, m)| { | |
270 | let left_side = Sugg::hir_with_applicability(cx, left_side, "..", &mut applicability); | |
271 | let right_side = Sugg::hir_with_applicability(cx, right_side, "..", &mut applicability); | |
272 | span_lint_and_sugg( | |
273 | cx, | |
274 | BOOL_COMPARISON, | |
275 | e.span, | |
276 | m, | |
277 | "try simplifying it as shown", | |
278 | h(left_side, right_side).to_string(), | |
279 | applicability, | |
280 | ) | |
281 | }), | |
282 | _ => (), | |
283 | } | |
284 | } | |
285 | } | |
286 | } | |
287 | ||
288 | fn suggest_bool_comparison<'a, 'tcx>( | |
289 | cx: &LateContext<'tcx>, | |
290 | e: &'tcx Expr<'_>, | |
291 | expr: &Expr<'_>, | |
292 | mut applicability: Applicability, | |
293 | message: &str, | |
294 | conv_hint: impl FnOnce(Sugg<'a>) -> Sugg<'a>, | |
295 | ) { | |
296 | let hint = if expr.span.from_expansion() { | |
297 | if applicability != Applicability::Unspecified { | |
298 | applicability = Applicability::MaybeIncorrect; | |
299 | } | |
300 | Sugg::hir_with_macro_callsite(cx, expr, "..") | |
301 | } else { | |
302 | Sugg::hir_with_applicability(cx, expr, "..", &mut applicability) | |
303 | }; | |
304 | span_lint_and_sugg( | |
305 | cx, | |
306 | BOOL_COMPARISON, | |
307 | e.span, | |
308 | message, | |
309 | "try simplifying it as shown", | |
310 | conv_hint(hint).to_string(), | |
311 | applicability, | |
312 | ); | |
313 | } | |
314 | ||
315 | enum Expression { | |
316 | Bool(bool), | |
317 | RetBool(bool), | |
318 | Other, | |
319 | } | |
320 | ||
321 | fn fetch_bool_block(block: &Block<'_>) -> Expression { | |
322 | match (&*block.stmts, block.expr.as_ref()) { | |
323 | (&[], Some(e)) => fetch_bool_expr(&**e), | |
324 | (&[ref e], None) => { | |
325 | if let StmtKind::Semi(ref e) = e.kind { | |
326 | if let ExprKind::Ret(_) = e.kind { | |
327 | fetch_bool_expr(&**e) | |
328 | } else { | |
329 | Expression::Other | |
330 | } | |
331 | } else { | |
332 | Expression::Other | |
333 | } | |
334 | }, | |
335 | _ => Expression::Other, | |
336 | } | |
337 | } | |
338 | ||
339 | fn fetch_bool_expr(expr: &Expr<'_>) -> Expression { | |
340 | match expr.kind { | |
341 | ExprKind::Block(ref block, _) => fetch_bool_block(block), | |
342 | ExprKind::Lit(ref lit_ptr) => { | |
343 | if let LitKind::Bool(value) = lit_ptr.node { | |
344 | Expression::Bool(value) | |
345 | } else { | |
346 | Expression::Other | |
347 | } | |
348 | }, | |
349 | ExprKind::Ret(Some(ref expr)) => match fetch_bool_expr(expr) { | |
350 | Expression::Bool(value) => Expression::RetBool(value), | |
351 | _ => Expression::Other, | |
352 | }, | |
353 | _ => Expression::Other, | |
354 | } | |
355 | } |