Skip to content

Commit 0cc48ad

Browse files
author
Michael Wright
committed
Fix nonminimal-bool false positive
Closes #4548 Closes #3847
1 parent ca6d36b commit 0cc48ad

File tree

3 files changed

+99
-55
lines changed

3 files changed

+99
-55
lines changed

‎clippy_lints/src/booleans.rs

+73-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 = snip(self.cx, terminal)?;
226187
self.output.push_str(&snip);
227188
}
228189
},
@@ -254,14 +215,52 @@ 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 = snip(self.cx, self.terminals[n as usize])?;
258219
self.output.push_str(&snip);
259220
},
260221
}
261222
Some(())
262223
}
263224
}
264225

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

332331
impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> {
333-
fn bool_expr(&self, e: &Expr) {
332+
fn bool_expr(&self, e: &'tcx Expr) {
334333
let mut h2q = Hir2Qmm {
335334
terminals: Vec::new(),
336335
cx: self.cx,
@@ -420,10 +419,8 @@ impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> {
420419
);
421420
};
422421
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-
}
422+
let mut visitor = NotSimplificationVisitor { cx: self.cx };
423+
visitor.visit_expr(e);
427424
} else {
428425
nonminimal_bool_lint(
429426
improvements
@@ -464,3 +461,30 @@ fn implements_ord<'a, 'tcx>(cx: &'a LateContext<'a, 'tcx>, expr: &Expr) -> bool
464461
let ty = cx.tables.expr_ty(expr);
465462
get_trait_def_id(cx, &paths::ORD).map_or(false, |id| implements_trait(cx, ty, id, &[]))
466463
}
464+
465+
struct NotSimplificationVisitor<'a, 'tcx> {
466+
cx: &'a LateContext<'a, 'tcx>,
467+
}
468+
469+
impl<'a, 'tcx> Visitor<'tcx> for NotSimplificationVisitor<'a, 'tcx> {
470+
fn visit_expr(&mut self, expr: &'tcx Expr) {
471+
if let ExprKind::Unary(UnNot, inner) = &expr.node {
472+
if let Some(suggestion) = simplify_not(self.cx, inner) {
473+
span_lint_and_sugg(
474+
self.cx,
475+
NONMINIMAL_BOOL,
476+
expr.span,
477+
"this boolean expression can be simplified",
478+
"try",
479+
suggestion,
480+
Applicability::MachineApplicable,
481+
);
482+
}
483+
}
484+
485+
walk_expr(self, expr);
486+
}
487+
fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
488+
NestedVisitorMap::None
489+
}
490+
}

‎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)