Skip to content

Commit

Permalink
Auto merge of #4568 - mikerite:fix-4548, r=flip1995
Browse files Browse the repository at this point in the history 
Fix `nonminimal-bool` false positive

Closes #4548
Closes #3847

changelog: Fix `nonminimal-bool` false positive
  • Loading branch information
@bors
bors committed Sep 25, 2019
2 parents d5ec41c + 89cdd26 commit b993b80
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 55 deletions.
125 changes: 76 additions & 49 deletions clippy_lints/src/booleans.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use crate::utils::{
get_trait_def_id, implements_trait, in_macro, match_type, paths, snippet_opt, span_lint_and_then, SpanlessEq,
get_trait_def_id, implements_trait, in_macro, match_type, paths, snippet_opt, span_lint_and_sugg,
span_lint_and_then, SpanlessEq,
};
use if_chain::if_chain;
use rustc::hir::intravisit::*;
Expand Down Expand Up @@ -159,46 +160,6 @@ struct SuggestContext<'a, 'tcx, 'v> {
}

impl<'a, 'tcx, 'v> SuggestContext<'a, 'tcx, 'v> {
fn snip(&self, e: &Expr) -> Option<String> {
snippet_opt(self.cx, e.span)
}

fn simplify_not(&self, expr: &Expr) -> Option<String> {
match &expr.node {
ExprKind::Binary(binop, lhs, rhs) => {
if !implements_ord(self.cx, lhs) {
return None;
}

match binop.node {
BinOpKind::Eq => Some(" != "),
BinOpKind::Ne => Some(" == "),
BinOpKind::Lt => Some(" >= "),
BinOpKind::Gt => Some(" <= "),
BinOpKind::Le => Some(" > "),
BinOpKind::Ge => Some(" < "),
_ => None,
}
.and_then(|op| Some(format!("{}{}{}", self.snip(lhs)?, op, self.snip(rhs)?)))
},
ExprKind::MethodCall(path, _, args) if args.len() == 1 => {
let type_of_receiver = self.cx.tables.expr_ty(&args[0]);
if !match_type(self.cx, type_of_receiver, &paths::OPTION)
&& !match_type(self.cx, type_of_receiver, &paths::RESULT)
{
return None;
}
METHODS_WITH_NEGATION
.iter()
.cloned()
.flat_map(|(a, b)| vec![(a, b), (b, a)])
.find(|&(a, _)| a == path.ident.name.as_str())
.and_then(|(_, neg_method)| Some(format!("{}.{}()", self.snip(&args[0])?, neg_method)))
},
_ => None,
}
}

fn recurse(&mut self, suggestion: &Bool) -> Option<()> {
use quine_mc_cluskey::Bool::*;
match suggestion {
Expand All @@ -217,12 +178,12 @@ impl<'a, 'tcx, 'v> SuggestContext<'a, 'tcx, 'v> {
},
Term(n) => {
let terminal = self.terminals[n as usize];
if let Some(str) = self.simplify_not(terminal) {
if let Some(str) = simplify_not(self.cx, terminal) {
self.simplified = true;
self.output.push_str(&str)
} else {
self.output.push('!');
let snip = self.snip(terminal)?;
let snip = snippet_opt(self.cx, terminal.span)?;
self.output.push_str(&snip);
}
},
Expand Down Expand Up @@ -254,14 +215,55 @@ impl<'a, 'tcx, 'v> SuggestContext<'a, 'tcx, 'v> {
}
},
&Term(n) => {
let snip = self.snip(self.terminals[n as usize])?;
let snip = snippet_opt(self.cx, self.terminals[n as usize].span)?;
self.output.push_str(&snip);
},
}
Some(())
}
}

fn simplify_not(cx: &LateContext<'_, '_>, expr: &Expr) -> Option<String> {
match &expr.node {
ExprKind::Binary(binop, lhs, rhs) => {
if !implements_ord(cx, lhs) {
return None;
}

match binop.node {
BinOpKind::Eq => Some(" != "),
BinOpKind::Ne => Some(" == "),
BinOpKind::Lt => Some(" >= "),
BinOpKind::Gt => Some(" <= "),
BinOpKind::Le => Some(" > "),
BinOpKind::Ge => Some(" < "),
_ => None,
}
.and_then(|op| {
Some(format!(
"{}{}{}",
snippet_opt(cx, lhs.span)?,
op,
snippet_opt(cx, rhs.span)?
))
})
},
ExprKind::MethodCall(path, _, args) if args.len() == 1 => {
let type_of_receiver = cx.tables.expr_ty(&args[0]);
if !match_type(cx, type_of_receiver, &paths::OPTION) && !match_type(cx, type_of_receiver, &paths::RESULT) {
return None;
}
METHODS_WITH_NEGATION
.iter()
.cloned()
.flat_map(|(a, b)| vec![(a, b), (b, a)])
.find(|&(a, _)| a == path.ident.name.as_str())
.and_then(|(_, neg_method)| Some(format!("{}.{}()", snippet_opt(cx, args[0].span)?, neg_method)))
},
_ => None,
}
}

// The boolean part of the return indicates whether some simplifications have been applied.
fn suggest(cx: &LateContext<'_, '_>, suggestion: &Bool, terminals: &[&Expr]) -> (String, bool) {
let mut suggest_context = SuggestContext {
Expand Down Expand Up @@ -330,7 +332,7 @@ fn terminal_stats(b: &Bool) -> Stats {
}

impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> {
fn bool_expr(&self, e: &Expr) {
fn bool_expr(&self, e: &'tcx Expr) {
let mut h2q = Hir2Qmm {
terminals: Vec::new(),
cx: self.cx,
Expand Down Expand Up @@ -420,10 +422,8 @@ impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> {
);
};
if improvements.is_empty() {
let suggest = suggest(self.cx, &expr, &h2q.terminals);
if suggest.1 {
nonminimal_bool_lint(vec![suggest.0])
}
let mut visitor = NotSimplificationVisitor { cx: self.cx };
visitor.visit_expr(e);
} else {
nonminimal_bool_lint(
improvements
Expand Down Expand Up @@ -464,3 +464,30 @@ fn implements_ord<'a, 'tcx>(cx: &'a LateContext<'a, 'tcx>, expr: &Expr) -> bool
let ty = cx.tables.expr_ty(expr);
get_trait_def_id(cx, &paths::ORD).map_or(false, |id| implements_trait(cx, ty, id, &[]))
}

struct NotSimplificationVisitor<'a, 'tcx> {
cx: &'a LateContext<'a, 'tcx>,
}

impl<'a, 'tcx> Visitor<'tcx> for NotSimplificationVisitor<'a, 'tcx> {
fn visit_expr(&mut self, expr: &'tcx Expr) {
if let ExprKind::Unary(UnNot, inner) = &expr.node {
if let Some(suggestion) = simplify_not(self.cx, inner) {
span_lint_and_sugg(
self.cx,
NONMINIMAL_BOOL,
expr.span,
"this boolean expression can be simplified",
"try",
suggestion,
Applicability::MachineApplicable,
);
}
}

walk_expr(self, expr);
}
fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
NestedVisitorMap::None
}
}
20 changes: 20 additions & 0 deletions tests/ui/booleans.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,3 +142,23 @@ fn dont_warn_for_negated_partial_ord_comparison() {
let _ = !(a > b);
let _ = !(a >= b);
}

fn issue3847(a: u32, b: u32) -> bool {
const THRESHOLD: u32 = 1_000;

if a < THRESHOLD && b >= THRESHOLD || a >= THRESHOLD && b < THRESHOLD {
return false;
}
true
}

fn issue4548() {
fn f(_i: u32, _j: u32) -> u32 {
unimplemented!();
}

let i = 0;
let j = 0;

if i != j && f(i, j) != 0 || i == j && f(i, j) != 1 {}
}
12 changes: 6 additions & 6 deletions tests/ui/booleans.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,22 +158,22 @@ LL | let _ = !(a.is_some() && !c);
| ^^^^^^^^^^^^^^^^^^^^ help: try: `c || a.is_none()`

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

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

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

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

0 comments on commit b993b80

Please sign in to comment.