Skip to content

Commit f975fb5

Browse files
oli-obkoli-obk
authored
Merge pull request #2216 from LaurentMazare/master
Handle methods with an obvious negation in the non-minimal bool lint
2 parents 1f6e2a6 + 25783fa commit f975fb5

File tree

3 files changed

+129
-42
lines changed

3 files changed

+129
-42
lines changed

‎clippy_lints/src/booleans.rs

Lines changed: 83 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,12 @@ declare_lint! {
4444
"boolean expressions that contain terminals which can be eliminated"
4545
}
4646

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+
4753
#[derive(Copy, Clone)]
4854
pub struct NonminimalBool;
4955

@@ -153,8 +159,16 @@ impl<'a, 'tcx, 'v> Hir2Qmm<'a, 'tcx, 'v> {
153159
}
154160
}
155161

156-
fn suggest(cx: &LateContext, suggestion: &Bool, terminals: &[&Expr]) -> String {
157-
fn recurse(brackets: bool, cx: &LateContext, suggestion: &Bool, terminals: &[&Expr], mut s: String) -> String {
162+
// The boolean part of the return indicates whether some simplifications have been applied.
163+
fn suggest(cx: &LateContext, suggestion: &Bool, terminals: &[&Expr]) -> (String, bool) {
164+
fn recurse(
165+
brackets: bool,
166+
cx: &LateContext,
167+
suggestion: &Bool,
168+
terminals: &[&Expr],
169+
mut s: String,
170+
simplified: &mut bool,
171+
) -> String {
158172
use quine_mc_cluskey::Bool::*;
159173
let snip = |e: &Expr| snippet_opt(cx, e.span).expect("don't try to improve booleans created by macros");
160174
match *suggestion {
@@ -169,49 +183,70 @@ fn suggest(cx: &LateContext, suggestion: &Bool, terminals: &[&Expr]) -> String {
169183
Not(ref inner) => match **inner {
170184
And(_) | Or(_) => {
171185
s.push('!');
172-
recurse(true, cx, inner, terminals, s)
186+
recurse(true, cx, inner, terminals, s, simplified)
173187
},
174-
Term(n) => if let ExprBinary(binop, ref lhs, ref rhs) = terminals[n as usize].node {
175-
let op = match binop.node {
176-
BiEq => " != ",
177-
BiNe => " == ",
178-
BiLt => " >= ",
179-
BiGt => " <= ",
180-
BiLe => " > ",
181-
BiGe => " < ",
182-
_ => {
188+
Term(n) => match terminals[n as usize].node {
189+
ExprBinary(binop, ref lhs, ref rhs) => {
190+
let op = match binop.node {
191+
BiEq => " != ",
192+
BiNe => " == ",
193+
BiLt => " >= ",
194+
BiGt => " <= ",
195+
BiLe => " > ",
196+
BiGe => " < ",
197+
_ => {
198+
s.push('!');
199+
return recurse(true, cx, inner, terminals, s, simplified);
200+
},
201+
};
202+
*simplified = true;
203+
s.push_str(&snip(lhs));
204+
s.push_str(op);
205+
s.push_str(&snip(rhs));
206+
s
207+
},
208+
ExprMethodCall(ref path, _, ref args) if args.len() == 1 => {
209+
let negation = METHODS_WITH_NEGATION
210+
.iter().cloned()
211+
.flat_map(|(a, b)| vec![(a, b), (b, a)])
212+
.find(|&(a, _)| a == path.name.as_str());
213+
if let Some((_, negation_method)) = negation {
214+
*simplified = true;
215+
s.push_str(&snip(&args[0]));
216+
s.push('.');
217+
s.push_str(negation_method);
218+
s.push_str("()");
219+
s
220+
} else {
183221
s.push('!');
184-
return recurse(true, cx, inner, terminals, s);
185-
},
186-
};
187-
s.push_str(&snip(lhs));
188-
s.push_str(op);
189-
s.push_str(&snip(rhs));
190-
s
191-
} else {
192-
s.push('!');
193-
recurse(false, cx, inner, terminals, s)
222+
recurse(false, cx, inner, terminals, s, simplified)
223+
}
224+
},
225+
_ => {
226+
s.push('!');
227+
recurse(false, cx, inner, terminals, s, simplified)
228+
},
194229
},
195230
_ => {
196231
s.push('!');
197-
recurse(false, cx, inner, terminals, s)
232+
recurse(false, cx, inner, terminals, s, simplified)
198233
},
199234
},
200235
And(ref v) => {
201236
if brackets {
202237
s.push('(');
203238
}
204239
if let Or(_) = v[0] {
205-
s = recurse(true, cx, &v[0], terminals, s);
240+
s = recurse(true, cx, &v[0], terminals, s, simplified);
206241
} else {
207-
s = recurse(false, cx, &v[0], terminals, s);
242+
s = recurse(false, cx, &v[0], terminals, s, simplified);
208243
}
209244
for inner in &v[1..] {
210245
s.push_str(" && ");
211246
if let Or(_) = *inner {
212-
s = recurse(true, cx, inner, terminals, s);
247+
s = recurse(true, cx, inner, terminals, s, simplified);
213248
} else {
214-
s = recurse(false, cx, inner, terminals, s);
249+
s = recurse(false, cx, inner, terminals, s, simplified);
215250
}
216251
}
217252
if brackets {
@@ -223,10 +258,10 @@ fn suggest(cx: &LateContext, suggestion: &Bool, terminals: &[&Expr]) -> String {
223258
if brackets {
224259
s.push('(');
225260
}
226-
s = recurse(false, cx, &v[0], terminals, s);
261+
s = recurse(false, cx, &v[0], terminals, s, simplified);
227262
for inner in &v[1..] {
228263
s.push_str(" || ");
229-
s = recurse(false, cx, inner, terminals, s);
264+
s = recurse(false, cx, inner, terminals, s, simplified);
230265
}
231266
if brackets {
232267
s.push(')');
@@ -249,7 +284,9 @@ fn suggest(cx: &LateContext, suggestion: &Bool, terminals: &[&Expr]) -> String {
249284
},
250285
}
251286
}
252-
recurse(false, cx, suggestion, terminals, String::new())
287+
let mut simplified = false;
288+
let s = recurse(false, cx, suggestion, terminals, String::new(), &mut simplified);
289+
(s, simplified)
253290
}
254291

255292
fn simple_negate(b: Bool) -> Bool {
@@ -359,7 +396,7 @@ impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> {
359396
db.span_suggestion(
360397
e.span,
361398
"it would look like the following",
362-
suggest(self.cx, suggestion, &h2q.terminals),
399+
suggest(self.cx, suggestion, &h2q.terminals).0,
363400
);
364401
},
365402
);
@@ -376,22 +413,26 @@ impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> {
376413
improvements.push(suggestion);
377414
}
378415
}
379-
if !improvements.is_empty() {
416+
let nonminimal_bool_lint = |suggestions| {
380417
span_lint_and_then(
381418
self.cx,
382419
NONMINIMAL_BOOL,
383420
e.span,
384421
"this boolean expression can be simplified",
385-
|db| {
386-
db.span_suggestions(
387-
e.span,
388-
"try",
389-
improvements
390-
.into_iter()
391-
.map(|suggestion| suggest(self.cx, suggestion, &h2q.terminals))
392-
.collect(),
393-
);
394-
},
422+
|db| { db.span_suggestions(e.span, "try", suggestions); },
423+
);
424+
};
425+
if improvements.is_empty() {
426+
let suggest = suggest(self.cx, &expr, &h2q.terminals);
427+
if suggest.1 {
428+
nonminimal_bool_lint(vec![suggest.0])
429+
}
430+
} else {
431+
nonminimal_bool_lint(
432+
improvements
433+
.into_iter()
434+
.map(|suggestion| suggest(self.cx, suggestion, &h2q.terminals).0)
435+
.collect()
395436
);
396437
}
397438
}

‎tests/ui/booleans.rs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,3 +38,19 @@ fn equality_stuff() {
3838
let _ = a > b && a == b;
3939
let _ = a != b || !(a != b || c == d);
4040
}
41+
42+
#[allow(unused, many_single_char_names)]
43+
fn methods_with_negation() {
44+
let a: Option<i32> = unimplemented!();
45+
let b: Result<i32, i32> = unimplemented!();
46+
let _ = a.is_some();
47+
let _ = !a.is_some();
48+
let _ = a.is_none();
49+
let _ = !a.is_none();
50+
let _ = b.is_err();
51+
let _ = !b.is_err();
52+
let _ = b.is_ok();
53+
let _ = !b.is_ok();
54+
let c = false;
55+
let _ = !(a.is_some() && !c);
56+
}

‎tests/ui/booleans.stderr

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,3 +130,33 @@ help: try
130130
39 | let _ = !(a == b && c == d);
131131
| ^^^^^^^^^^^^^^^^^^^
132132

133+
error: this boolean expression can be simplified
134+
--> $DIR/booleans.rs:47:13
135+
|
136+
47 | let _ = !a.is_some();
137+
| ^^^^^^^^^^^^ help: try: `a.is_none()`
138+
139+
error: this boolean expression can be simplified
140+
--> $DIR/booleans.rs:49:13
141+
|
142+
49 | let _ = !a.is_none();
143+
| ^^^^^^^^^^^^ help: try: `a.is_some()`
144+
145+
error: this boolean expression can be simplified
146+
--> $DIR/booleans.rs:51:13
147+
|
148+
51 | let _ = !b.is_err();
149+
| ^^^^^^^^^^^ help: try: `b.is_ok()`
150+
151+
error: this boolean expression can be simplified
152+
--> $DIR/booleans.rs:53:13
153+
|
154+
53 | let _ = !b.is_ok();
155+
| ^^^^^^^^^^ help: try: `b.is_err()`
156+
157+
error: this boolean expression can be simplified
158+
--> $DIR/booleans.rs:55:13
159+
|
160+
55 | let _ = !(a.is_some() && !c);
161+
| ^^^^^^^^^^^^^^^^^^^^ help: try: `c || a.is_none()`
162+

0 commit comments

Comments
 (0)