Merge pull request #1305 from d-dorazio/1289-lint-for-ignored-argument-in-result-option

oli-obk · web-flow · commit 8630376e71db · 2016-10-30T14:20:54.000-04:00
Add lint for redundant pattern matching in if let for Result/Option
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -257,6 +257,7 @@ All notable changes to this project will be documented in this file.
 [`for_loop_over_option`]: https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_option
 [`for_loop_over_result`]: https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_result
 [`identity_op`]: https://github.com/Manishearth/rust-clippy/wiki#identity_op
+[`if_let_redundant_pattern_matching`]: https://github.com/Manishearth/rust-clippy/wiki#if_let_redundant_pattern_matching
 [`if_let_some_result`]: https://github.com/Manishearth/rust-clippy/wiki#if_let_some_result
 [`if_not_else`]: https://github.com/Manishearth/rust-clippy/wiki#if_not_else
 [`if_same_then_else`]: https://github.com/Manishearth/rust-clippy/wiki#if_same_then_else
diff --git a/README.md b/README.md
diff --git a/clippy_lints/src/approx_const.rs b/clippy_lints/src/approx_const.rs
@@ -76,7 +76,7 @@ fn check_lit(cx: &LateContext, lit: &Lit, e: &Expr) {
 }
 
 fn check_known_consts(cx: &LateContext, e: &Expr, s: &str, module: &str) {
-    if let Ok(_) = s.parse::<f64>() {
+    if s.parse::<f64>().is_ok() {
         for &(constant, name, min_digits) in KNOWN_CONSTS {
             if is_approx_const(constant, s, min_digits) {
                 span_lint(cx,
diff --git a/clippy_lints/src/arithmetic.rs b/clippy_lints/src/arithmetic.rs
@@ -49,7 +49,7 @@ impl LintPass for Arithmetic {
 
 impl LateLintPass for Arithmetic {
     fn check_expr(&mut self, cx: &LateContext, expr: &hir::Expr) {
-        if let Some(_) = self.span {
+        if self.span.is_some() {
             return;
         }
         match expr.node {
diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs
@@ -322,7 +322,7 @@ fn check_doc(cx: &EarlyContext, valid_idents: &[String], docs: &[(&str, Span)])
 
                         if new_line {
                             let mut lookup_parser = parser.clone();
-                            if let Some(_) = lookup_parser.find(|&(_, c)| c == ']') {
+                            if lookup_parser.any(|(_, c)| c == ']') {
                                 if let Some((_, ':')) = lookup_parser.next() {
                                     lookup_parser.next_line();
                                     parser = lookup_parser;
diff --git a/clippy_lints/src/if_let_redundant_pattern_matching.rs b/clippy_lints/src/if_let_redundant_pattern_matching.rs
@@ -0,0 +1,91 @@
+use rustc::lint::*;
+use rustc::hir::*;
+use syntax::codemap::Span;
+use utils::{paths, span_lint_and_then, match_path, snippet};
+
+/// **What it does:*** Lint for redundant pattern matching over `Result` or `Option`
+///
+/// **Why is this bad?** It's more concise and clear to just use the proper utility function
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+///
+/// ```rust
+/// if let Ok(_) = Ok::<i32, i32>(42) {}
+/// if let Err(_) = Err::<i32, i32>(42) {}
+/// if let None = None::<()> {}
+/// if let Some(_) = Some(42) {}
+/// ```
+///
+/// The more idiomatic use would be:
+///
+/// ```rust
+/// if Ok::<i32, i32>(42).is_ok() {}
+/// if Err::<i32, i32>(42).is_err() {}
+/// if None::<()>.is_none() {}
+/// if Some(42).is_some() {}
+/// ```
+///
+declare_lint! {
+    pub IF_LET_REDUNDANT_PATTERN_MATCHING,
+    Warn,
+    "use the proper utility function avoiding an `if let`"
+}
+
+#[derive(Copy, Clone)]
+pub struct Pass;
+
+impl LintPass for Pass {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(IF_LET_REDUNDANT_PATTERN_MATCHING)
+    }
+}
+
+impl LateLintPass for Pass {
+    fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
+
+        if let ExprMatch(ref op, ref arms, MatchSource::IfLetDesugar{..}) = expr.node {
+
+            if arms[0].pats.len() == 1 {
+
+                let good_method = match arms[0].pats[0].node {
+                    PatKind::TupleStruct(ref path, ref pats, _) if pats.len() == 1 && pats[0].node == PatKind::Wild =>  {
+
+                        if match_path(path, &paths::RESULT_OK) {
+                            "is_ok()"
+                        } else if match_path(path, &paths::RESULT_ERR) {
+                            "is_err()"
+                        } else if match_path(path, &paths::OPTION_SOME) {
+                            "is_some()"
+                        } else {
+                            return
+                        }
+                    }
+
+                    PatKind::Path(_, ref path) if match_path(path, &paths::OPTION_NONE) => {
+                        "is_none()"
+                    }
+
+                    _ => return
+                };
+
+                span_lint_and_then(cx,
+                                   IF_LET_REDUNDANT_PATTERN_MATCHING,
+                                   arms[0].pats[0].span,
+                                   &format!("redundant pattern matching, consider using `{}`", good_method),
+                                   |db| {
+                                        let span = Span {
+                                            lo: expr.span.lo,
+                                            hi: op.span.hi,
+                                            expn_id: expr.span.expn_id,
+                                        };
+                                        db.span_suggestion(span,
+                                                           "try this",
+                                                           format!("if {}.{}", snippet(cx, op.span, "_"), good_method));
+                                    });
+            }
+
+        }
+    }
+}
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -82,6 +82,7 @@ pub mod format;
 pub mod formatting;
 pub mod functions;
 pub mod identity_op;
+pub mod if_let_redundant_pattern_matching;
 pub mod if_not_else;
 pub mod items_after_statements;
 pub mod len_zero;
@@ -257,6 +258,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
     reg.register_late_lint_pass(box eval_order_dependence::EvalOrderDependence);
     reg.register_late_lint_pass(box missing_doc::MissingDoc::new());
     reg.register_late_lint_pass(box ok_if_let::Pass);
+    reg.register_late_lint_pass(box if_let_redundant_pattern_matching::Pass);
 
     reg.register_lint_group("clippy_restrictions", vec![
         arithmetic::FLOAT_ARITHMETIC,
@@ -344,6 +346,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         functions::NOT_UNSAFE_PTR_ARG_DEREF,
         functions::TOO_MANY_ARGUMENTS,
         identity_op::IDENTITY_OP,
+        if_let_redundant_pattern_matching::IF_LET_REDUNDANT_PATTERN_MATCHING,
         len_zero::LEN_WITHOUT_IS_EMPTY,
         len_zero::LEN_ZERO,
         let_if_seq::USELESS_LET_IF_SEQ,
diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs
@@ -532,7 +532,7 @@ impl LateLintPass for Pass {
                     lint_iter_nth(cx, expr, arglists[0], false);
                 } else if let Some(arglists) = method_chain_args(expr, &["iter_mut", "nth"]) {
                     lint_iter_nth(cx, expr, arglists[0], true);
-                } else if let Some(_) = method_chain_args(expr, &["skip", "next"]) {
+                } else if method_chain_args(expr, &["skip", "next"]).is_some() {
                     lint_iter_skip_next(cx, expr);
                 }
 
@@ -796,7 +796,7 @@ fn lint_cstring_as_ptr(cx: &LateContext, expr: &hir::Expr, new: &hir::Expr, unwr
 // Type of MethodArgs is potentially a Vec
 fn lint_iter_nth(cx: &LateContext, expr: &hir::Expr, iter_args: &MethodArgs, is_mut: bool){
     let mut_str = if is_mut { "_mut" } else {""};
-    let caller_type = if let Some(_) = derefs_to_slice(cx, &iter_args[0], cx.tcx.expr_ty(&iter_args[0])) {
+    let caller_type = if derefs_to_slice(cx, &iter_args[0], cx.tcx.expr_ty(&iter_args[0])).is_some() {
         "slice"
     }
     else if match_type(cx, cx.tcx.expr_ty(&iter_args[0]), &paths::VEC) {
diff --git a/clippy_lints/src/minmax.rs b/clippy_lints/src/minmax.rs
@@ -85,7 +85,7 @@ fn fetch_const(args: &[P<Expr>], m: MinMax) -> Option<(MinMax, Constant, &Expr)>
         return None;
     }
     if let Some(c) = constant_simple(&args[0]) {
-        if let None = constant_simple(&args[1]) {
+        if constant_simple(&args[1]).is_none() {
             // otherwise ignore
             Some((m, c, &args[1]))
         } else {
diff --git a/clippy_lints/src/ok_if_let.rs b/clippy_lints/src/ok_if_let.rs
@@ -38,15 +38,15 @@ impl LateLintPass for Pass {
             let MatchSource::IfLetDesugar { .. } = *source, //test if it is an If Let
             let ExprMethodCall(_, _, ref result_types) = op.node, //check is expr.ok() has type Result<T,E>.ok()
             let PatKind::TupleStruct(ref x, ref y, _)  = body[0].pats[0].node, //get operation
-            let Some(_) = method_chain_args(op, &["ok"]) //test to see if using ok() methoduse std::marker::Sized;
+            method_chain_args(op, &["ok"]).is_some() //test to see if using ok() methoduse std::marker::Sized;
 
         ], {
             let is_result_type = match_type(cx, cx.tcx.expr_ty(&result_types[0]), &paths::RESULT);
             let some_expr_string = snippet(cx, y[0].span, "");
             if print::path_to_string(x) == "Some" && is_result_type {
                 span_help_and_lint(cx, IF_LET_SOME_RESULT, expr.span,
                 "Matching on `Some` with `ok()` is redundant",
-                &format!("Consider matching on `Ok({})` and removing the call to `ok` instead", some_expr_string)); 
+                &format!("Consider matching on `Ok({})` and removing the call to `ok` instead", some_expr_string));
             }
         }}
     }
diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs
@@ -34,6 +34,8 @@ pub const MUTEX: [&'static str; 4] = ["std", "sync", "mutex", "Mutex"];
 pub const OPEN_OPTIONS: [&'static str; 3] = ["std", "fs", "OpenOptions"];
 pub const OPS_MODULE: [&'static str; 2] = ["core", "ops"];
 pub const OPTION: [&'static str; 3] = ["core", "option", "Option"];
+pub const OPTION_NONE: [&'static str; 4] = ["core", "option", "Option", "None"];
+pub const OPTION_SOME: [&'static str; 4] = ["core", "option", "Option", "Some"];
 pub const PTR_NULL: [&'static str; 2] = ["ptr", "null"];
 pub const PTR_NULL_MUT: [&'static str; 2] = ["ptr", "null_mut"];
 pub const RANGE: [&'static str; 3] = ["core", "ops", "Range"];
@@ -59,6 +61,8 @@ pub const REGEX_BYTES_SET_NEW: [&'static str; 5] = ["regex", "re_set", "bytes",
 pub const REGEX_NEW: [&'static str; 4] = ["regex", "re_unicode", "Regex", "new"];
 pub const REGEX_SET_NEW: [&'static str; 5] = ["regex", "re_set", "unicode", "RegexSet", "new"];
 pub const RESULT: [&'static str; 3] = ["core", "result", "Result"];
+pub const RESULT_ERR: [&'static str; 4] = ["core", "result", "Result", "Err"];
+pub const RESULT_OK: [&'static str; 4] = ["core", "result", "Result", "Ok"];
 pub const SERDE_DE_VISITOR: [&'static str; 3] = ["serde", "de", "Visitor"];
 pub const STRING: [&'static str; 3] = ["collections", "string", "String"];
 pub const TRANSMUTE: [&'static str; 4] = ["core", "intrinsics", "", "transmute"];
diff --git a/tests/compile-fail/if_let_redundant_pattern_matching.rs b/tests/compile-fail/if_let_redundant_pattern_matching.rs
@@ -0,0 +1,53 @@
+#![feature(plugin)]
+
+#![plugin(clippy)]
+#![deny(clippy)]
+#![deny(if_let_redundant_pattern_matching)]
+
+
+fn main() {
+    if let Ok(_) = Ok::<i32, i32>(42) {}
+    //~^ERROR redundant pattern matching, consider using `is_ok()`
+    //~| HELP try this
+    //~| SUGGESTION if Ok::<i32, i32>(42).is_ok() {
+
+    if let Err(_) = Err::<i32, i32>(42) {
+    //~^ERROR redundant pattern matching, consider using `is_err()`
+    //~| HELP try this
+    //~| SUGGESTION if Err::<i32, i32>(42).is_err() {
+    }
+
+    if let None = None::<()> {
+    //~^ERROR redundant pattern matching, consider using `is_none()`
+    //~| HELP try this
+    //~| SUGGESTION if None::<()>.is_none() {
+    }
+
+    if let Some(_) = Some(42) {
+    //~^ERROR redundant pattern matching, consider using `is_some()`
+    //~| HELP try this
+    //~| SUGGESTION if Some(42).is_some() {
+    }
+
+    if Ok::<i32, i32>(42).is_ok() {
+
+    }
+
+    if Err::<i32, i32>(42).is_err() {
+
+    }
+
+    if None::<i32>.is_none() {
+
+    }
+
+    if Some(42).is_some() {
+
+    }
+
+    if let Ok(x) = Ok::<i32,i32>(42) {
+        println!("{}", x);
+    }
+}
+
+
diff --git a/tests/compile-fail/matches.rs b/tests/compile-fail/matches.rs
@@ -2,7 +2,7 @@
 
 #![plugin(clippy)]
 #![deny(clippy)]
-#![allow(unused)]
+#![allow(unused, if_let_redundant_pattern_matching)]
 #![deny(single_match_else)]
 
 use std::borrow::Cow;

Original file line number	Diff line number	Diff line change
`@@ -76,7 +76,7 @@ fn check_lit(cx: &LateContext, lit: &Lit, e: &Expr) {`
`76`	`76`	`}`
`77`	`77`
`78`	`78`	`fn check_known_consts(cx: &LateContext, e: &Expr, s: &str, module: &str) {`
`79`		`- if let Ok(_) = s.parse::<f64>() {`
	`79`	`+ if s.parse::<f64>().is_ok() {`
`80`	`80`	`for &(constant, name, min_digits) in KNOWN_CONSTS {`
`81`	`81`	`if is_approx_const(constant, s, min_digits) {`
`82`	`82`	`span_lint(cx,`
Original file line number	Diff line number	Diff line change
`@@ -49,7 +49,7 @@ impl LintPass for Arithmetic {`
`49`	`49`
`50`	`50`	`impl LateLintPass for Arithmetic {`
`51`	`51`	`fn check_expr(&mut self, cx: &LateContext, expr: &hir::Expr) {`
`52`		`- if let Some(_) = self.span {`
	`52`	`+ if self.span.is_some() {`
`53`	`53`	`return;`
`54`	`54`	`}`
`55`	`55`	`match expr.node {`
Original file line number	Diff line number	Diff line change
`@@ -85,7 +85,7 @@ fn fetch_const(args: &[P<Expr>], m: MinMax) -> Option<(MinMax, Constant, &Expr)>`
`85`	`85`	`return None;`
`86`	`86`	`}`
`87`	`87`	`if let Some(c) = constant_simple(&args[0]) {`
`88`		`- if let None = constant_simple(&args[1]) {`
	`88`	`+ if constant_simple(&args[1]).is_none() {`
`89`	`89`	`// otherwise ignore`
`90`	`90`	`Some((m, c, &args[1]))`
`91`	`91`	`} else {`