]>
Commit | Line | Data |
---|---|---|
abe05a73 | 1 | use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; |
ea8adc8c XL |
2 | use rustc::hir::*; |
3 | use rustc::hir::intravisit::*; | |
abe05a73 XL |
4 | use syntax::ast::{LitKind, NodeId, DUMMY_NODE_ID}; |
5 | use syntax::codemap::{dummy_spanned, Span, DUMMY_SP}; | |
ea8adc8c | 6 | use syntax::util::ThinVec; |
83c7162d | 7 | use utils::{in_macro, paths, match_type, snippet_opt, span_lint_and_then, SpanlessEq}; |
ea8adc8c XL |
8 | |
9 | /// **What it does:** Checks for boolean expressions that can be written more | |
10 | /// concisely. | |
11 | /// | |
12 | /// **Why is this bad?** Readability of boolean expressions suffers from | |
13 | /// unnecessary duplication. | |
14 | /// | |
15 | /// **Known problems:** Ignores short circuiting behavior of `||` and | |
16 | /// `&&`. Ignores `|`, `&` and `^`. | |
17 | /// | |
18 | /// **Example:** | |
19 | /// ```rust | |
20 | /// if a && true // should be: if a | |
21 | /// if !(a == b) // should be: if a != b | |
22 | /// ``` | |
0531ce1d | 23 | declare_clippy_lint! { |
ea8adc8c | 24 | pub NONMINIMAL_BOOL, |
0531ce1d | 25 | complexity, |
ea8adc8c XL |
26 | "boolean expressions that can be written more concisely" |
27 | } | |
28 | ||
29 | /// **What it does:** Checks for boolean expressions that contain terminals that | |
30 | /// can be eliminated. | |
31 | /// | |
32 | /// **Why is this bad?** This is most likely a logic bug. | |
33 | /// | |
34 | /// **Known problems:** Ignores short circuiting behavior. | |
35 | /// | |
36 | /// **Example:** | |
37 | /// ```rust | |
38 | /// if a && b || a { ... } | |
39 | /// ``` | |
40 | /// The `b` is unnecessary, the expression is equivalent to `if a`. | |
0531ce1d | 41 | declare_clippy_lint! { |
ea8adc8c | 42 | pub LOGIC_BUG, |
0531ce1d | 43 | correctness, |
ea8adc8c XL |
44 | "boolean expressions that contain terminals which can be eliminated" |
45 | } | |
46 | ||
abe05a73 XL |
47 | // For each pairs, both orders are considered. |
48 | const METHODS_WITH_NEGATION: [(&str, &str); 2] = [ | |
49 | ("is_some", "is_none"), | |
50 | ("is_err", "is_ok"), | |
51 | ]; | |
52 | ||
ea8adc8c XL |
53 | #[derive(Copy, Clone)] |
54 | pub struct NonminimalBool; | |
55 | ||
56 | impl LintPass for NonminimalBool { | |
57 | fn get_lints(&self) -> LintArray { | |
58 | lint_array!(NONMINIMAL_BOOL, LOGIC_BUG) | |
59 | } | |
60 | } | |
61 | ||
62 | impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NonminimalBool { | |
63 | fn check_fn( | |
64 | &mut self, | |
65 | cx: &LateContext<'a, 'tcx>, | |
66 | _: intravisit::FnKind<'tcx>, | |
67 | _: &'tcx FnDecl, | |
68 | body: &'tcx Body, | |
69 | _: Span, | |
70 | _: NodeId, | |
71 | ) { | |
0531ce1d | 72 | NonminimalBoolVisitor { cx }.visit_body(body) |
ea8adc8c XL |
73 | } |
74 | } | |
75 | ||
76 | struct NonminimalBoolVisitor<'a, 'tcx: 'a> { | |
77 | cx: &'a LateContext<'a, 'tcx>, | |
78 | } | |
79 | ||
80 | use quine_mc_cluskey::Bool; | |
81 | struct Hir2Qmm<'a, 'tcx: 'a, 'v> { | |
82 | terminals: Vec<&'v Expr>, | |
83 | cx: &'a LateContext<'a, 'tcx>, | |
84 | } | |
85 | ||
86 | impl<'a, 'tcx, 'v> Hir2Qmm<'a, 'tcx, 'v> { | |
87 | fn extract(&mut self, op: BinOp_, a: &[&'v Expr], mut v: Vec<Bool>) -> Result<Vec<Bool>, String> { | |
88 | for a in a { | |
89 | if let ExprBinary(binop, ref lhs, ref rhs) = a.node { | |
90 | if binop.node == op { | |
91 | v = self.extract(op, &[lhs, rhs], v)?; | |
92 | continue; | |
93 | } | |
94 | } | |
95 | v.push(self.run(a)?); | |
96 | } | |
97 | Ok(v) | |
98 | } | |
99 | ||
100 | fn run(&mut self, e: &'v Expr) -> Result<Bool, String> { | |
101 | // prevent folding of `cfg!` macros and the like | |
102 | if !in_macro(e.span) { | |
103 | match e.node { | |
104 | ExprUnary(UnNot, ref inner) => return Ok(Bool::Not(box self.run(inner)?)), | |
abe05a73 XL |
105 | ExprBinary(binop, ref lhs, ref rhs) => match binop.node { |
106 | BiOr => return Ok(Bool::Or(self.extract(BiOr, &[lhs, rhs], Vec::new())?)), | |
107 | BiAnd => return Ok(Bool::And(self.extract(BiAnd, &[lhs, rhs], Vec::new())?)), | |
108 | _ => (), | |
ea8adc8c | 109 | }, |
abe05a73 XL |
110 | ExprLit(ref lit) => match lit.node { |
111 | LitKind::Bool(true) => return Ok(Bool::True), | |
112 | LitKind::Bool(false) => return Ok(Bool::False), | |
113 | _ => (), | |
ea8adc8c XL |
114 | }, |
115 | _ => (), | |
116 | } | |
117 | } | |
118 | for (n, expr) in self.terminals.iter().enumerate() { | |
119 | if SpanlessEq::new(self.cx).ignore_fn().eq_expr(e, expr) { | |
abe05a73 XL |
120 | #[allow(cast_possible_truncation)] |
121 | return Ok(Bool::Term(n as u8)); | |
ea8adc8c XL |
122 | } |
123 | let negated = match e.node { | |
124 | ExprBinary(binop, ref lhs, ref rhs) => { | |
125 | let mk_expr = |op| { | |
126 | Expr { | |
127 | id: DUMMY_NODE_ID, | |
128 | hir_id: DUMMY_HIR_ID, | |
129 | span: DUMMY_SP, | |
130 | attrs: ThinVec::new(), | |
131 | node: ExprBinary(dummy_spanned(op), lhs.clone(), rhs.clone()), | |
132 | } | |
133 | }; | |
134 | match binop.node { | |
135 | BiEq => mk_expr(BiNe), | |
136 | BiNe => mk_expr(BiEq), | |
137 | BiGt => mk_expr(BiLe), | |
138 | BiGe => mk_expr(BiLt), | |
139 | BiLt => mk_expr(BiGe), | |
140 | BiLe => mk_expr(BiGt), | |
141 | _ => continue, | |
142 | } | |
143 | }, | |
144 | _ => continue, | |
145 | }; | |
146 | if SpanlessEq::new(self.cx).ignore_fn().eq_expr(&negated, expr) { | |
abe05a73 XL |
147 | #[allow(cast_possible_truncation)] |
148 | return Ok(Bool::Not(Box::new(Bool::Term(n as u8)))); | |
ea8adc8c XL |
149 | } |
150 | } | |
151 | let n = self.terminals.len(); | |
152 | self.terminals.push(e); | |
153 | if n < 32 { | |
abe05a73 XL |
154 | #[allow(cast_possible_truncation)] |
155 | Ok(Bool::Term(n as u8)) | |
ea8adc8c XL |
156 | } else { |
157 | Err("too many literals".to_owned()) | |
158 | } | |
159 | } | |
160 | } | |
161 | ||
ff7c6d11 XL |
162 | struct SuggestContext<'a, 'tcx: 'a, 'v> { |
163 | terminals: &'v [&'v Expr], | |
164 | cx: &'a LateContext<'a, 'tcx>, | |
165 | output: String, | |
166 | simplified: bool, | |
167 | } | |
168 | ||
169 | impl<'a, 'tcx, 'v> SuggestContext<'a, 'tcx, 'v> { | |
170 | fn snip(&self, e: &Expr) -> Option<String> { | |
171 | snippet_opt(self.cx, e.span) | |
172 | } | |
173 | ||
174 | fn simplify_not(&self, expr: &Expr) -> Option<String> { | |
175 | match expr.node { | |
176 | ExprBinary(binop, ref lhs, ref rhs) => { | |
177 | match binop.node { | |
178 | BiEq => Some(" != "), | |
179 | BiNe => Some(" == "), | |
180 | BiLt => Some(" >= "), | |
181 | BiGt => Some(" <= "), | |
182 | BiLe => Some(" > "), | |
183 | BiGe => Some(" < "), | |
184 | _ => None, | |
185 | }.and_then(|op| Some(format!("{}{}{}", self.snip(lhs)?, op, self.snip(rhs)?))) | |
186 | }, | |
187 | ExprMethodCall(ref path, _, ref args) if args.len() == 1 => { | |
83c7162d XL |
188 | let type_of_receiver = self.cx.tables.expr_ty(&args[0]); |
189 | if !match_type(self.cx, type_of_receiver, &paths::OPTION) && | |
190 | !match_type(self.cx, type_of_receiver, &paths::RESULT) { | |
191 | return None; | |
192 | } | |
ff7c6d11 XL |
193 | METHODS_WITH_NEGATION |
194 | .iter().cloned() | |
195 | .flat_map(|(a, b)| vec![(a, b), (b, a)]) | |
196 | .find(|&(a, _)| a == path.name.as_str()) | |
197 | .and_then(|(_, neg_method)| Some(format!("{}.{}()", self.snip(&args[0])?, neg_method))) | |
198 | }, | |
199 | _ => None, | |
200 | } | |
201 | } | |
202 | ||
203 | fn recurse(&mut self, suggestion: &Bool) -> Option<()> { | |
ea8adc8c | 204 | use quine_mc_cluskey::Bool::*; |
ea8adc8c XL |
205 | match *suggestion { |
206 | True => { | |
ff7c6d11 | 207 | self.output.push_str("true"); |
ea8adc8c XL |
208 | }, |
209 | False => { | |
ff7c6d11 | 210 | self.output.push_str("false"); |
ea8adc8c | 211 | }, |
abe05a73 XL |
212 | Not(ref inner) => match **inner { |
213 | And(_) | Or(_) => { | |
ff7c6d11 XL |
214 | self.output.push('!'); |
215 | self.output.push('('); | |
216 | self.recurse(inner); | |
217 | self.output.push(')'); | |
abe05a73 | 218 | }, |
ff7c6d11 XL |
219 | Term(n) => { |
220 | let terminal = self.terminals[n as usize]; | |
221 | if let Some(str) = self.simplify_not(terminal) { | |
222 | self.simplified = true; | |
223 | self.output.push_str(&str) | |
224 | } else { | |
225 | self.output.push('!'); | |
226 | let snip = self.snip(terminal)?; | |
227 | self.output.push_str(&snip); | |
228 | } | |
abe05a73 | 229 | }, |
ff7c6d11 XL |
230 | True | False | Not(_) => { |
231 | self.output.push('!'); | |
232 | self.recurse(inner)?; | |
abe05a73 | 233 | }, |
ea8adc8c XL |
234 | }, |
235 | And(ref v) => { | |
ff7c6d11 XL |
236 | for (index, inner) in v.iter().enumerate() { |
237 | if index > 0 { | |
238 | self.output.push_str(" && "); | |
239 | } | |
ea8adc8c | 240 | if let Or(_) = *inner { |
ff7c6d11 XL |
241 | self.output.push('('); |
242 | self.recurse(inner); | |
243 | self.output.push(')'); | |
ea8adc8c | 244 | } else { |
ff7c6d11 | 245 | self.recurse(inner); |
ea8adc8c XL |
246 | } |
247 | } | |
ea8adc8c XL |
248 | }, |
249 | Or(ref v) => { | |
ff7c6d11 XL |
250 | for (index, inner) in v.iter().enumerate() { |
251 | if index > 0 { | |
252 | self.output.push_str(" || "); | |
253 | } | |
254 | self.recurse(inner); | |
ea8adc8c | 255 | } |
ea8adc8c XL |
256 | }, |
257 | Term(n) => { | |
ff7c6d11 XL |
258 | let snip = self.snip(self.terminals[n as usize])?; |
259 | self.output.push_str(&snip); | |
ea8adc8c XL |
260 | }, |
261 | } | |
ff7c6d11 | 262 | Some(()) |
ea8adc8c | 263 | } |
ff7c6d11 XL |
264 | } |
265 | ||
266 | // The boolean part of the return indicates whether some simplifications have been applied. | |
267 | fn suggest(cx: &LateContext, suggestion: &Bool, terminals: &[&Expr]) -> (String, bool) { | |
268 | let mut suggest_context = SuggestContext { | |
0531ce1d XL |
269 | terminals, |
270 | cx, | |
ff7c6d11 XL |
271 | output: String::new(), |
272 | simplified: false, | |
273 | }; | |
274 | suggest_context.recurse(suggestion); | |
275 | (suggest_context.output, suggest_context.simplified) | |
ea8adc8c XL |
276 | } |
277 | ||
278 | fn simple_negate(b: Bool) -> Bool { | |
279 | use quine_mc_cluskey::Bool::*; | |
280 | match b { | |
281 | True => False, | |
282 | False => True, | |
283 | t @ Term(_) => Not(Box::new(t)), | |
284 | And(mut v) => { | |
285 | for el in &mut v { | |
286 | *el = simple_negate(::std::mem::replace(el, True)); | |
287 | } | |
288 | Or(v) | |
289 | }, | |
290 | Or(mut v) => { | |
291 | for el in &mut v { | |
292 | *el = simple_negate(::std::mem::replace(el, True)); | |
293 | } | |
294 | And(v) | |
295 | }, | |
296 | Not(inner) => *inner, | |
297 | } | |
298 | } | |
299 | ||
300 | #[derive(Default)] | |
301 | struct Stats { | |
302 | terminals: [usize; 32], | |
303 | negations: usize, | |
304 | ops: usize, | |
305 | } | |
306 | ||
307 | fn terminal_stats(b: &Bool) -> Stats { | |
308 | fn recurse(b: &Bool, stats: &mut Stats) { | |
309 | match *b { | |
310 | True | False => stats.ops += 1, | |
311 | Not(ref inner) => { | |
312 | match **inner { | |
313 | And(_) | Or(_) => stats.ops += 1, // brackets are also operations | |
314 | _ => stats.negations += 1, | |
315 | } | |
316 | recurse(inner, stats); | |
317 | }, | |
318 | And(ref v) | Or(ref v) => { | |
319 | stats.ops += v.len() - 1; | |
320 | for inner in v { | |
321 | recurse(inner, stats); | |
322 | } | |
323 | }, | |
324 | Term(n) => stats.terminals[n as usize] += 1, | |
325 | } | |
326 | } | |
327 | use quine_mc_cluskey::Bool::*; | |
328 | let mut stats = Stats::default(); | |
329 | recurse(b, &mut stats); | |
330 | stats | |
331 | } | |
332 | ||
333 | impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> { | |
334 | fn bool_expr(&self, e: &Expr) { | |
335 | let mut h2q = Hir2Qmm { | |
336 | terminals: Vec::new(), | |
337 | cx: self.cx, | |
338 | }; | |
339 | if let Ok(expr) = h2q.run(e) { | |
ea8adc8c XL |
340 | if h2q.terminals.len() > 8 { |
341 | // QMC has exponentially slow behavior as the number of terminals increases | |
342 | // 8 is reasonable, it takes approximately 0.2 seconds. | |
343 | // See #825 | |
344 | return; | |
345 | } | |
346 | ||
347 | let stats = terminal_stats(&expr); | |
348 | let mut simplified = expr.simplify(); | |
349 | for simple in Bool::Not(Box::new(expr.clone())).simplify() { | |
350 | match simple { | |
351 | Bool::Not(_) | Bool::True | Bool::False => {}, | |
352 | _ => simplified.push(Bool::Not(Box::new(simple.clone()))), | |
353 | } | |
354 | let simple_negated = simple_negate(simple); | |
355 | if simplified.iter().any(|s| *s == simple_negated) { | |
356 | continue; | |
357 | } | |
358 | simplified.push(simple_negated); | |
359 | } | |
360 | let mut improvements = Vec::new(); | |
361 | 'simplified: for suggestion in &simplified { | |
362 | let simplified_stats = terminal_stats(suggestion); | |
363 | let mut improvement = false; | |
364 | for i in 0..32 { | |
365 | // ignore any "simplifications" that end up requiring a terminal more often | |
366 | // than in the original expression | |
367 | if stats.terminals[i] < simplified_stats.terminals[i] { | |
368 | continue 'simplified; | |
369 | } | |
370 | if stats.terminals[i] != 0 && simplified_stats.terminals[i] == 0 { | |
371 | span_lint_and_then( | |
372 | self.cx, | |
373 | LOGIC_BUG, | |
374 | e.span, | |
375 | "this boolean expression contains a logic bug", | |
376 | |db| { | |
377 | db.span_help( | |
378 | h2q.terminals[i].span, | |
379 | "this expression can be optimized out by applying boolean operations to the \ | |
abe05a73 | 380 | outer expression", |
ea8adc8c XL |
381 | ); |
382 | db.span_suggestion( | |
383 | e.span, | |
384 | "it would look like the following", | |
abe05a73 | 385 | suggest(self.cx, suggestion, &h2q.terminals).0, |
ea8adc8c XL |
386 | ); |
387 | }, | |
388 | ); | |
389 | // don't also lint `NONMINIMAL_BOOL` | |
390 | return; | |
391 | } | |
392 | // if the number of occurrences of a terminal decreases or any of the stats | |
393 | // decreases while none increases | |
abe05a73 XL |
394 | improvement |= (stats.terminals[i] > simplified_stats.terminals[i]) |
395 | || (stats.negations > simplified_stats.negations && stats.ops == simplified_stats.ops) | |
396 | || (stats.ops > simplified_stats.ops && stats.negations == simplified_stats.negations); | |
ea8adc8c XL |
397 | } |
398 | if improvement { | |
399 | improvements.push(suggestion); | |
400 | } | |
401 | } | |
abe05a73 | 402 | let nonminimal_bool_lint = |suggestions| { |
ea8adc8c XL |
403 | span_lint_and_then( |
404 | self.cx, | |
405 | NONMINIMAL_BOOL, | |
406 | e.span, | |
407 | "this boolean expression can be simplified", | |
abe05a73 XL |
408 | |db| { db.span_suggestions(e.span, "try", suggestions); }, |
409 | ); | |
410 | }; | |
411 | if improvements.is_empty() { | |
412 | let suggest = suggest(self.cx, &expr, &h2q.terminals); | |
413 | if suggest.1 { | |
414 | nonminimal_bool_lint(vec![suggest.0]) | |
415 | } | |
416 | } else { | |
417 | nonminimal_bool_lint( | |
418 | improvements | |
419 | .into_iter() | |
420 | .map(|suggestion| suggest(self.cx, suggestion, &h2q.terminals).0) | |
421 | .collect() | |
ea8adc8c XL |
422 | ); |
423 | } | |
424 | } | |
425 | } | |
426 | } | |
427 | ||
428 | impl<'a, 'tcx> Visitor<'tcx> for NonminimalBoolVisitor<'a, 'tcx> { | |
429 | fn visit_expr(&mut self, e: &'tcx Expr) { | |
430 | if in_macro(e.span) { | |
431 | return; | |
432 | } | |
433 | match e.node { | |
434 | ExprBinary(binop, _, _) if binop.node == BiOr || binop.node == BiAnd => self.bool_expr(e), | |
abe05a73 XL |
435 | ExprUnary(UnNot, ref inner) => if self.cx.tables.node_types()[inner.hir_id].is_bool() { |
436 | self.bool_expr(e); | |
437 | } else { | |
438 | walk_expr(self, e); | |
ea8adc8c XL |
439 | }, |
440 | _ => walk_expr(self, e), | |
441 | } | |
442 | } | |
443 | fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { | |
444 | NestedVisitorMap::None | |
445 | } | |
446 | } |