Lint Option.map(f) where f returns nil

CHANGELOG.md

@@ -705,6 +705,7 @@ All notable changes to this project will be documented in this file.
 [`nonsensical_open_options`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#nonsensical_open_options
 [`not_unsafe_ptr_arg_deref`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#not_unsafe_ptr_arg_deref
 [`ok_expect`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#ok_expect
+[`option_map_nil_fn`]: https://github.com/Manishearth/rust-clippy/wiki#option_map_nil_fn
 [`op_ref`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#op_ref
 [`option_map_or_none`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#option_map_or_none
 [`option_map_unwrap_or`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#option_map_unwrap_or

clippy_lints/src/lib.rs

@@ -145,6 +145,7 @@ pub mod lifetimes;
 pub mod literal_representation;
 pub mod loops;
 pub mod map_clone;
+pub mod map_nil_fn;
 pub mod matches;
 pub mod mem_forget;
 pub mod methods;
@@ -405,6 +406,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
     reg.register_late_lint_pass(box question_mark::QuestionMarkPass);
     reg.register_late_lint_pass(box suspicious_trait_impl::SuspiciousImpl);
     reg.register_late_lint_pass(box redundant_field_names::RedundantFieldNames);
+    reg.register_late_lint_pass(box map_nil_fn::Pass);
 
 
     reg.register_lint_group("clippy_restriction", vec![
@@ -441,6 +443,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         if_not_else::IF_NOT_ELSE,
         infinite_iter::MAYBE_INFINITE_ITER,
         items_after_statements::ITEMS_AFTER_STATEMENTS,
+        map_nil_fn::OPTION_MAP_NIL_FN,
         matches::SINGLE_MATCH_ELSE,
         methods::FILTER_MAP,
         methods::OPTION_MAP_UNWRAP_OR,

clippy_lints/src/map_nil_fn.rs

@@ -0,0 +1,91 @@
+use rustc::hir;
+use rustc::lint::*;
+use rustc::ty;
+use utils::{in_macro, match_type, method_chain_args, snippet, span_lint_and_then};
+use utils::paths;
+
+#[derive(Clone)]
+pub struct Pass;
+
+/// **What it does:** Checks for usage of `Option.map(f)` where f is a nil
+/// function
+///
+/// **Why is this bad?** Readability, this can be written more clearly with
+/// an if statement
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// let x : Option<&str> = do_stuff();
+/// x.map(log_err_msg);
+/// ```
+/// The correct use would be:
+/// ```rust
+/// let x : Option<&str> = do_stuff();
+/// if let Some(msg) = x {
+///     log_err_msg(msg)
+/// }
+/// ```
+declare_lint! {
+    pub OPTION_MAP_NIL_FN,
+    Allow,
+    "using `Option.map(f)`, where f is a nil function"
+}
+
+
+impl LintPass for Pass {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(OPTION_MAP_NIL_FN)
+    }
+}
+
+fn is_nil_function(cx: &LateContext, expr: &hir::Expr) -> bool {
+    let ty = cx.tables.expr_ty(expr);
+
+    if let ty::TyFnDef(_, _, bare) = ty.sty {
+        if let Some(fn_type) = cx.tcx.no_late_bound_regions(&bare.sig) {
+            return fn_type.output().is_nil();
+        }
+    }
+    false
+}
+
+fn lint_map_nil_fn(cx: &LateContext, stmt: &hir::Stmt, expr: &hir::Expr, map_args: &[hir::Expr]) {
+    let var_arg = &map_args[0];
+    let fn_arg = &map_args[1];
+
+    if !match_type(cx, cx.tables.expr_ty(var_arg), &paths::OPTION) {
+        return;
+    }
+
+    let suggestion = if is_nil_function(cx, fn_arg) {
+        format!("if let Some(...) = {0} {{ {1}(...) }}",
+                snippet(cx, var_arg.span, "_"),
+                snippet(cx, fn_arg.span, "_"))
+    } else {
+        return;
+    };
+
+    span_lint_and_then(cx,
+                       OPTION_MAP_NIL_FN,
+                       expr.span,
+                       "called `map(f)` on an Option value where `f` is a nil function",
+                       |db| { db.span_suggestion(stmt.span, "try this", suggestion); });
+}
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
+    fn check_stmt(&mut self, cx: &LateContext, stmt: &hir::Stmt) {
+        if in_macro(cx, stmt.span) {
+            return;
+        }
+
+        if let hir::StmtSemi(ref expr, _) = stmt.node {
+            if let hir::ExprMethodCall(_, _, _) = expr.node {
+                if let Some(arglists) = method_chain_args(expr, &["map"]) {
+                    lint_map_nil_fn(cx, stmt, expr, arglists[0]);
+                }
+            }
+        }
+    }
+}

tests/compile-fail/map_nil_fn.rs

@@ -0,0 +1,42 @@
+#![feature(plugin)]
+#![feature(const_fn)]
+#![plugin(clippy)]
+
+#![deny(clippy_pedantic)]
+#![allow(unused, missing_docs_in_private_items)]
+
+fn do_nothing<T>(_: T) {}
+
+fn plus_one(value: usize) -> usize {
+    value + 1
+}
+
+struct HasOption {
+    field: Option<usize>,
+}
+
+impl HasOption {
+    fn do_option_nothing(self: &HasOption, value: usize) {}
+
+    fn do_option_plus_one(self: &HasOption, value: usize) -> usize {
+        value + 1
+    }
+}
+
+#[cfg_attr(rustfmt, rustfmt_skip)]
+fn main() {
+    let x = HasOption { field: Some(10) };
+
+    x.field.map(plus_one);
+    let _ : Option<()> = x.field.map(do_nothing);
+
+    x.field.map(do_nothing);
+    //~^ ERROR called `map(f)` on an Option value where `f` is a nil function
+    //~| HELP try this
+    //~| SUGGESTION if let Some(...) = x.field { do_nothing(...) }
+
+    x.field.map(do_nothing);
+    //~^ ERROR called `map(f)` on an Option value where `f` is a nil function
+    //~| HELP try this
+    //~| SUGGESTION if let Some(...) = x.field { do_nothing(...) }
+}