Skip to content

Commit adc1df1

Browse files
borsbors
committed
Auto merge of #4568 - mikerite:fix-4548, r=flip1995
Fix `nonminimal-bool` false positive Closes #4548 Closes #3847 changelog: Fix `nonminimal-bool` false positive
2 parents 4d30b08 + 89cdd26 commit adc1df1

File tree

3 files changed

+102
-55
lines changed

3 files changed

+102
-55
lines changed

‎clippy_lints/src/booleans.rs

+76-49
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use crate::utils::{
2-
get_trait_def_id, implements_trait, in_macro, match_type, paths, snippet_opt, span_lint_and_then, SpanlessEq,
2+
get_trait_def_id, implements_trait, in_macro, match_type, paths, snippet_opt, span_lint_and_sugg,
3+
span_lint_and_then, SpanlessEq,
34
};
45
use if_chain::if_chain;
56
use rustc::hir::intravisit::*;
@@ -159,46 +160,6 @@ struct SuggestContext<'a, 'tcx, 'v> {
159160
}
160161

161162
impl<'a, 'tcx, 'v> SuggestContext<'a, 'tcx, 'v> {
162-
fn snip(&self, e: &Expr) -> Option<String> {
163-
snippet_opt(self.cx, e.span)
164-
}
165-
166-
fn simplify_not(&self, expr: &Expr) -> Option<String> {
167-
match &expr.node {
168-
ExprKind::Binary(binop, lhs, rhs) => {
169-
if !implements_ord(self.cx, lhs) {
170-
return None;
171-
}
172-
173-
match binop.node {
174-
BinOpKind::Eq => Some(" != "),
175-
BinOpKind::Ne => Some(" == "),
176-
BinOpKind::Lt => Some(" >= "),
177-
BinOpKind::Gt => Some(" <= "),
178-
BinOpKind::Le => Some(" > "),
179-
BinOpKind::Ge => Some(" < "),
180-
_ => None,
181-
}
182-
.and_then(|op| Some(format!("{}{}{}", self.snip(lhs)?, op, self.snip(rhs)?)))
183-
},
184-
ExprKind::MethodCall(path, _, args) if args.len() == 1 => {
185-
let type_of_receiver = self.cx.tables.expr_ty(&args[0]);
186-
if !match_type(self.cx, type_of_receiver, &paths::OPTION)
187-
&& !match_type(self.cx, type_of_receiver, &paths::RESULT)
188-
{
189-
return None;
190-
}
191-
METHODS_WITH_NEGATION
192-
.iter()
193-
.cloned()
194-
.flat_map(|(a, b)| vec![(a, b), (b, a)])
195-
.find(|&(a, _)| a == path.ident.name.as_str())
196-
.and_then(|(_, neg_method)| Some(format!("{}.{}()", self.snip(&args[0])?, neg_method)))
197-
},
198-
_ => None,
199-
}
200-
}
201-
202163
fn recurse(&mut self, suggestion: &Bool) -> Option<()> {
203164
use quine_mc_cluskey::Bool::*;
204165
match suggestion {
@@ -217,12 +178,12 @@ impl<'a, 'tcx, 'v> SuggestContext<'a, 'tcx, 'v> {
217178
},
218179
Term(n) => {
219180
let terminal = self.terminals[n as usize];
220-
if let Some(str) = self.simplify_not(terminal) {
181+
if let Some(str) = simplify_not(self.cx, terminal) {
221182
self.simplified = true;
222183
self.output.push_str(&str)
223184
} else {
224185
self.output.push('!');
225-
let snip = self.snip(terminal)?;
186+
let snip = snippet_opt(self.cx, terminal.span)?;
226187
self.output.push_str(&snip);
227188
}
228189
},
@@ -254,14 +215,55 @@ impl<'a, 'tcx, 'v> SuggestContext<'a, 'tcx, 'v> {
254215
}
255216
},
256217
&Term(n) => {
257-
let snip = self.snip(self.terminals[n as usize])?;
218+
let snip = snippet_opt(self.cx, self.terminals[n as usize].span)?;
258219
self.output.push_str(&snip);
259220
},
260221
}
261222
Some(())
262223
}
263224
}
264225

226+
fn simplify_not(cx: &LateContext<'_, '_>, expr: &Expr) -> Option<String> {
227+
match &expr.node {
228+
ExprKind::Binary(binop, lhs, rhs) => {
229+
if !implements_ord(cx, lhs) {
230+
return None;
231+
}
232+
233+
match binop.node {
234+
BinOpKind::Eq => Some(" != "),
235+
BinOpKind::Ne => Some(" == "),
236+
BinOpKind::Lt => Some(" >= "),
237+
BinOpKind::Gt => Some(" <= "),
238+
BinOpKind::Le => Some(" > "),
239+
BinOpKind::Ge => Some(" < "),
240+
_ => None,
241+
}
242+
.and_then(|op| {
243+
Some(format!(
244+
"{}{}{}",
245+
snippet_opt(cx, lhs.span)?,
246+
op,
247+
snippet_opt(cx, rhs.span)?
248+
))
249+
})
250+
},
251+
ExprKind::MethodCall(path, _, args) if args.len() == 1 => {
252+
let type_of_receiver = cx.tables.expr_ty(&args[0]);
253+
if !match_type(cx, type_of_receiver, &paths::OPTION) && !match_type(cx, type_of_receiver, &paths::RESULT) {
254+
return None;
255+
}
256+
METHODS_WITH_NEGATION
257+
.iter()
258+
.cloned()
259+
.flat_map(|(a, b)| vec![(a, b), (b, a)])
260+
.find(|&(a, _)| a == path.ident.name.as_str())
261+
.and_then(|(_, neg_method)| Some(format!("{}.{}()", snippet_opt(cx, args[0].span)?, neg_method)))
262+
},
263+
_ => None,
264+
}
265+
}
266+
265267
// The boolean part of the return indicates whether some simplifications have been applied.
266268
fn suggest(cx: &LateContext<'_, '_>, suggestion: &Bool, terminals: &[&Expr]) -> (String, bool) {
267269
let mut suggest_context = SuggestContext {
@@ -330,7 +332,7 @@ fn terminal_stats(b: &Bool) -> Stats {
330332
}
331333

332334
impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> {
333-
fn bool_expr(&self, e: &Expr) {
335+
fn bool_expr(&self, e: &'tcx Expr) {
334336
let mut h2q = Hir2Qmm {
335337
terminals: Vec::new(),
336338
cx: self.cx,
@@ -420,10 +422,8 @@ impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> {
420422
);
421423
};
422424
if improvements.is_empty() {
423-
let suggest = suggest(self.cx, &expr, &h2q.terminals);
424-
if suggest.1 {
425-
nonminimal_bool_lint(vec![suggest.0])
426-
}
425+
let mut visitor = NotSimplificationVisitor { cx: self.cx };
426+
visitor.visit_expr(e);
427427
} else {
428428
nonminimal_bool_lint(
429429
improvements
@@ -464,3 +464,30 @@ fn implements_ord<'a, 'tcx>(cx: &'a LateContext<'a, 'tcx>, expr: &Expr) -> bool
464464
let ty = cx.tables.expr_ty(expr);
465465
get_trait_def_id(cx, &paths::ORD).map_or(false, |id| implements_trait(cx, ty, id, &[]))
466466
}
467+
468+
struct NotSimplificationVisitor<'a, 'tcx> {
469+
cx: &'a LateContext<'a, 'tcx>,
470+
}
471+
472+
impl<'a, 'tcx> Visitor<'tcx> for NotSimplificationVisitor<'a, 'tcx> {
473+
fn visit_expr(&mut self, expr: &'tcx Expr) {
474+
if let ExprKind::Unary(UnNot, inner) = &expr.node {
475+
if let Some(suggestion) = simplify_not(self.cx, inner) {
476+
span_lint_and_sugg(
477+
self.cx,
478+
NONMINIMAL_BOOL,
479+
expr.span,
480+
"this boolean expression can be simplified",
481+
"try",
482+
suggestion,
483+
Applicability::MachineApplicable,
484+
);
485+
}
486+
}
487+
488+
walk_expr(self, expr);
489+
}
490+
fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
491+
NestedVisitorMap::None
492+
}
493+
}

‎tests/ui/booleans.rs

+20
Original file line numberDiff line numberDiff line change
@@ -142,3 +142,23 @@ fn dont_warn_for_negated_partial_ord_comparison() {
142142
let _ = !(a > b);
143143
let _ = !(a >= b);
144144
}
145+
146+
fn issue3847(a: u32, b: u32) -> bool {
147+
const THRESHOLD: u32 = 1_000;
148+
149+
if a < THRESHOLD && b >= THRESHOLD || a >= THRESHOLD && b < THRESHOLD {
150+
return false;
151+
}
152+
true
153+
}
154+
155+
fn issue4548() {
156+
fn f(_i: u32, _j: u32) -> u32 {
157+
unimplemented!();
158+
}
159+
160+
let i = 0;
161+
let j = 0;
162+
163+
if i != j && f(i, j) != 0 || i == j && f(i, j) != 1 {}
164+
}

‎tests/ui/booleans.stderr

+6-6
Original file line numberDiff line numberDiff line change
@@ -158,22 +158,22 @@ LL | let _ = !(a.is_some() && !c);
158158
| ^^^^^^^^^^^^^^^^^^^^ help: try: `c || a.is_none()`
159159

160160
error: this boolean expression can be simplified
161-
--> $DIR/booleans.rs:54:13
161+
--> $DIR/booleans.rs:54:26
162162
|
163163
LL | let _ = !(!c ^ c) || !a.is_some();
164-
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `!(!c ^ c) || a.is_none()`
164+
| ^^^^^^^^^^^^ help: try: `a.is_none()`
165165

166166
error: this boolean expression can be simplified
167-
--> $DIR/booleans.rs:55:13
167+
--> $DIR/booleans.rs:55:25
168168
|
169169
LL | let _ = (!c ^ c) || !a.is_some();
170-
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `(!c ^ c) || a.is_none()`
170+
| ^^^^^^^^^^^^ help: try: `a.is_none()`
171171

172172
error: this boolean expression can be simplified
173-
--> $DIR/booleans.rs:56:13
173+
--> $DIR/booleans.rs:56:23
174174
|
175175
LL | let _ = !c ^ c || !a.is_some();
176-
| ^^^^^^^^^^^^^^^^^^^^^^ help: try: `!c ^ c || a.is_none()`
176+
| ^^^^^^^^^^^^ help: try: `a.is_none()`
177177

178178
error: this boolean expression can be simplified
179179
--> $DIR/booleans.rs:128:8

0 commit comments

Comments
 (0)