Merge pull request #1257 from KitFreddura/master

If let some lint

CHANGELOG.md

@@ -250,6 +250,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_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
 [`ifs_same_cond`]: https://github.com/Manishearth/rust-clippy/wiki#ifs_same_cond

README.md

@@ -174,7 +174,7 @@ You can check out this great service at [clippy.bashy.io](https://clippy.bashy.i
 
 ## Lints
 
-There are 172 lints included in this crate:
+There are 173 lints included in this crate:
 
 name                                                                                                                 | default | triggers on
 ---------------------------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------
@@ -231,6 +231,7 @@ name
 [for_loop_over_option](https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_option)                         | warn    | for-looping over an `Option`, which is more clearly expressed as an `if let`
 [for_loop_over_result](https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_result)                         | warn    | for-looping over a `Result`, which is more clearly expressed as an `if let`
 [identity_op](https://github.com/Manishearth/rust-clippy/wiki#identity_op)                                           | warn    | using identity operations, e.g. `x + 0` or `y / 1`
+[if_let_some_result](https://github.com/Manishearth/rust-clippy/wiki#if_let_some_result)                             | warn    | usage of `ok()` in `if let Some(pat)` statements is unnecessary, match on `Ok(pat)` instead
 [if_not_else](https://github.com/Manishearth/rust-clippy/wiki#if_not_else)                                           | allow   | `if` branches that could be swapped so no negation operation is necessary on the condition
 [if_same_then_else](https://github.com/Manishearth/rust-clippy/wiki#if_same_then_else)                               | warn    | if with the same *then* and *else* blocks
 [ifs_same_cond](https://github.com/Manishearth/rust-clippy/wiki#ifs_same_cond)                                       | warn    | consecutive `ifs` with the same condition

clippy_lints/src/lib.rs

@@ -106,6 +106,7 @@ pub mod neg_multiply;
 pub mod new_without_default;
 pub mod no_effect;
 pub mod non_expressive_names;
+pub mod ok_if_let;
 pub mod open_options;
 pub mod overflow_check_conditional;
 pub mod panic;
@@ -254,6 +255,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
     reg.register_late_lint_pass(box let_if_seq::LetIfSeq);
     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::OkIfLetPass);
 
     reg.register_lint_group("clippy_restrictions", vec![
         arithmetic::FLOAT_ARITHMETIC,
@@ -404,6 +406,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         no_effect::NO_EFFECT,
         no_effect::UNNECESSARY_OPERATION,
         non_expressive_names::MANY_SINGLE_CHAR_NAMES,
+        ok_if_let::IF_LET_SOME_RESULT,
         open_options::NONSENSICAL_OPEN_OPTIONS,
         overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL,
         panic::PANIC_PARAMS,

clippy_lints/src/ok_if_let.rs

@@ -0,0 +1,53 @@
+use rustc::lint::*;
+use rustc::hir::*;
+use utils::{paths, method_chain_args, span_help_and_lint, match_type, snippet};
+
+/// **What it does:*** Checks for unnecessary `ok()` in if let.
+///
+/// **Why is this bad?** Calling `ok()` in if let is unnecessary, instead match on `Ok(pat)`
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rustc
+/// for result in iter {
+///     if let Some(bench) = try!(result).parse().ok() {
+///         vec.push(bench)
+///     }
+/// }
+/// ```
+declare_lint! {
+    pub IF_LET_SOME_RESULT,
+    Warn,
+    "usage of `ok()` in `if let Some(pat)` statements is unnecessary, match on `Ok(pat)` instead"
+}
+
+#[derive(Copy, Clone)]
+pub struct OkIfLetPass;
+
+impl LintPass for OkIfLetPass {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(IF_LET_SOME_RESULT)
+    }
+}
+
+impl LateLintPass for OkIfLetPass {
+    fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
+        if_let_chain! {[ //begin checking variables
+            let ExprMatch(ref op, ref body, ref source) = expr.node, //test if expr is a match
+            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;
+
+        ], {
+            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)); 
+            }
+        }}
+    }
+}

clippy_lints/src/zero_div_zero.rs

@@ -38,8 +38,8 @@ impl LateLintPass for Pass {
             // do something like 0.0/(2.0 - 2.0), but it would be nice to warn on that case too.
             let Some(Constant::Float(ref lhs_value, lhs_width)) = constant_simple(left),
             let Some(Constant::Float(ref rhs_value, rhs_width)) = constant_simple(right),
-            let Some(0.0) = lhs_value.parse().ok(),
-            let Some(0.0) = rhs_value.parse().ok()
+            let Ok(0.0) = lhs_value.parse(),
+            let Ok(0.0) = rhs_value.parse()
         ], {
             // since we're about to suggest a use of std::f32::NaN or std::f64::NaN,
             // match the precision of the literals that are given.

tests/compile-fail/ok_if_let.rs

@@ -0,0 +1,27 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#![deny(if_let_some_result)]
+
+fn str_to_int(x: &str) -> i32 {
+    if let Some(y) = x.parse().ok() { 
+    //~^ERROR Matching on `Some` with `ok()` is redundant
+        y
+    } else {
+        0
+    }
+}
+
+fn str_to_int_ok(x: &str) -> i32 {
+    if let Ok(y) = x.parse() {
+        y
+    } else {
+        0
+    }
+}
+
+fn main() {
+    let y = str_to_int("1");
+    let z = str_to_int_ok("2");
+    println!("{}{}", y, z);
+}