diff --git a/CHANGELOG.md b/CHANGELOG.md index fede68081bb..14438edcb98 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 3b2251b9021..755fc26bc9b 100644 --- a/clippy_lints/src/lib.rs +++ b/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, diff --git a/clippy_lints/src/map_nil_fn.rs b/clippy_lints/src/map_nil_fn.rs new file mode 100644 index 00000000000..0fd2c176a64 --- /dev/null +++ b/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]); + } + } + } + } +} diff --git a/tests/compile-fail/map_nil_fn.rs b/tests/compile-fail/map_nil_fn.rs new file mode 100644 index 00000000000..0338216c578 --- /dev/null +++ b/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) {} + +fn plus_one(value: usize) -> usize { + value + 1 +} + +struct HasOption { + field: Option, +} + +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(...) } +}