From e5ecbb55ee68370c629215e077ced565c0925d22 Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Mon, 16 Jan 2017 15:40:50 -0500 Subject: [PATCH 01/17] Lint `Option.map(f)` where f returns nil --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 3 ++ clippy_lints/src/map_nil_fn.rs | 91 ++++++++++++++++++++++++++++++++ tests/compile-fail/map_nil_fn.rs | 42 +++++++++++++++ 4 files changed, 137 insertions(+) create mode 100644 clippy_lints/src/map_nil_fn.rs create mode 100644 tests/compile-fail/map_nil_fn.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index fede68081bb4..14438edcb98c 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 3b2251b9021e..755fc26bc9b8 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 000000000000..0fd2c176a643 --- /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 000000000000..0338216c5782 --- /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(...) } +} From 302f5d05f57b4debbe2890f7ebabbedb2059737f Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Sun, 22 Jan 2017 12:45:45 -0500 Subject: [PATCH 02/17] Lint `Option.map(f)` where f never returns --- clippy_lints/src/map_nil_fn.rs | 2 +- tests/compile-fail/map_nil_fn.rs | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/map_nil_fn.rs b/clippy_lints/src/map_nil_fn.rs index 0fd2c176a643..9502c5d05632 100644 --- a/clippy_lints/src/map_nil_fn.rs +++ b/clippy_lints/src/map_nil_fn.rs @@ -45,7 +45,7 @@ fn is_nil_function(cx: &LateContext, expr: &hir::Expr) -> bool { 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(); + return fn_type.output().is_nil() || fn_type.output().is_never(); } } false diff --git a/tests/compile-fail/map_nil_fn.rs b/tests/compile-fail/map_nil_fn.rs index 0338216c5782..03f77c500348 100644 --- a/tests/compile-fail/map_nil_fn.rs +++ b/tests/compile-fail/map_nil_fn.rs @@ -7,6 +7,10 @@ fn do_nothing(_: T) {} +fn diverge(_: T) -> ! { + panic!() +} + fn plus_one(value: usize) -> usize { value + 1 } @@ -39,4 +43,9 @@ fn main() { //~^ 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(diverge); + //~^ ERROR called `map(f)` on an Option value where `f` is a nil function + //~| HELP try this + //~| SUGGESTION if let Some(...) = x.field { diverge(...) } } From 30f2480fd879359f8773d5c29807130c3a489785 Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Sun, 22 Jan 2017 13:36:50 -0500 Subject: [PATCH 03/17] Lint closures that return nil --- clippy_lints/src/map_nil_fn.rs | 127 ++++++++++++++++++++++++++----- tests/compile-fail/map_nil_fn.rs | 84 ++++++++++++++++++++ 2 files changed, 194 insertions(+), 17 deletions(-) diff --git a/clippy_lints/src/map_nil_fn.rs b/clippy_lints/src/map_nil_fn.rs index 9502c5d05632..4423aeece6fd 100644 --- a/clippy_lints/src/map_nil_fn.rs +++ b/clippy_lints/src/map_nil_fn.rs @@ -1,24 +1,26 @@ use rustc::hir; use rustc::lint::*; use rustc::ty; -use utils::{in_macro, match_type, method_chain_args, snippet, span_lint_and_then}; +use std::borrow::Cow; +use utils::{in_macro, iter_input_pats, 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 +/// function or closure /// /// **Why is this bad?** Readability, this can be written more clearly with /// an if statement /// -/// **Known problems:** None. +/// **Known problems:** Closures with multiple statements are not handled /// /// **Example:** /// ```rust /// let x : Option<&str> = do_stuff(); /// x.map(log_err_msg); +/// x.map(|msg| log_err_msg(format_msg(msg))) /// ``` /// The correct use would be: /// ```rust @@ -26,11 +28,14 @@ pub struct Pass; /// if let Some(msg) = x { /// log_err_msg(msg) /// } +/// if let Some(msg) = x { +/// log_err_msg(format_msg(msg)) +/// } /// ``` declare_lint! { pub OPTION_MAP_NIL_FN, Allow, - "using `Option.map(f)`, where f is a nil function" + "using `Option.map(f)`, where f is a nil function or closure" } @@ -40,17 +45,94 @@ impl LintPass for Pass { } } +fn is_nil_type(ty: ty::Ty) -> bool { + match ty.sty { + ty::TyTuple(slice) => slice.is_empty(), + ty::TyNever => true, + _ => false, + } +} + 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() || fn_type.output().is_never(); + return is_nil_type(fn_type.output()); } } false } +fn is_nil_expression(cx: &LateContext, expr: &hir::Expr) -> bool { + is_nil_type(cx.tables.expr_ty(expr)) +} + +// The expression inside a closure may or may not have surrounding braces and +// semicolons, which causes problems when generating a suggestion. Given an +// expression that evaluates to '()' or '!', recursively remove useless braces +// and semi-colons until is suitable for including in the suggestion template +fn reduce_nil_expression<'a>(cx: &LateContext, expr: &'a hir::Expr) -> Option> { + if !is_nil_expression(cx, expr) { + return None; + } + + match expr.node { + hir::ExprCall(_, _) | + hir::ExprMethodCall(_, _, _) => { + // Calls can't be reduced any more + Some(snippet(cx, expr.span, "_")) + }, + hir::ExprBlock(ref block) => { + match (&block.stmts[..], block.expr.as_ref()) { + (&[], Some(inner_expr)) => { + // Reduce `{ X }` to `X` + reduce_nil_expression(cx, inner_expr) + }, + (&[ref inner_stmt], None) => { + // Reduce `{ X; }` to `X` or `X;` + match inner_stmt.node { + hir::StmtDecl(ref d, _) => Some(snippet(cx, d.span, "_")), + hir::StmtExpr(ref e, _) => Some(snippet(cx, e.span, "_")), + hir::StmtSemi(ref e, _) => { + if is_nil_expression(cx, e) { + // `X` returns nil so we can strip the + // semicolon and reduce further + reduce_nil_expression(cx, e) + } else { + // `X` doesn't return nil so it needs a + // trailing semicolon + Some(snippet(cx, inner_stmt.span, "_")) + } + }, + } + }, + _ => None, + } + }, + _ => None, + } +} + +fn reduce_nil_closure<'a, 'tcx>( + cx: &LateContext<'a, 'tcx>, + expr: &'a hir::Expr +) -> Option<(Cow<'a, str>, Cow<'a, str>)> { + if let hir::ExprClosure(_, ref decl, inner_expr_id, _) = expr.node { + let body = cx.tcx.map.body(inner_expr_id); + + if_let_chain! {[ + decl.inputs.len() == 1, + let Some(binding) = iter_input_pats(&decl, body).next(), + let Some(expr_snippet) = reduce_nil_expression(cx, &body.value), + ], { + let binding_snippet = snippet(cx, binding.pat.span, "_"); + return Some((binding_snippet, expr_snippet)); + }} + } + None +} + 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]; @@ -59,19 +141,30 @@ fn lint_map_nil_fn(cx: &LateContext, stmt: &hir::Stmt, expr: &hir::Expr, map_arg 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; - }; + if is_nil_function(cx, fn_arg) { + let msg = "called `map(f)` on an Option value where `f` is a nil function"; + let suggestion = format!("if let Some(...) = {0} {{ {1}(...) }}", + snippet(cx, var_arg.span, "_"), + snippet(cx, fn_arg.span, "_")); - 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); }); + span_lint_and_then(cx, + OPTION_MAP_NIL_FN, + expr.span, + msg, + |db| { db.span_suggestion(stmt.span, "try this", suggestion); }); + } else if let Some((binding_snippet, expr_snippet)) = reduce_nil_closure(cx, fn_arg) { + let msg = "called `map(f)` on an Option value where `f` is a nil closure"; + let suggestion = format!("if let Some({0}) = {1} {{ {2} }}", + binding_snippet, + snippet(cx, var_arg.span, "_"), + expr_snippet); + + span_lint_and_then(cx, + OPTION_MAP_NIL_FN, + expr.span, + msg, + |db| { db.span_suggestion(stmt.span, "try this", suggestion); }); + } } impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { diff --git a/tests/compile-fail/map_nil_fn.rs b/tests/compile-fail/map_nil_fn.rs index 03f77c500348..5ac3913466a5 100644 --- a/tests/compile-fail/map_nil_fn.rs +++ b/tests/compile-fail/map_nil_fn.rs @@ -48,4 +48,88 @@ fn main() { //~^ ERROR called `map(f)` on an Option value where `f` is a nil function //~| HELP try this //~| SUGGESTION if let Some(...) = x.field { diverge(...) } + + let captured = 10; + if let Some(value) = x.field { do_nothing(value + captured) }; + let _ : Option<()> = x.field.map(|value| do_nothing(value + captured)); + + x.field.map(|value| x.do_option_nothing(value + captured)); + //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure + //~| HELP try this + //~| SUGGESTION if let Some(value) = x.field { x.do_option_nothing(value + captured) } + + x.field.map(|value| { x.do_option_plus_one(value + captured); }); + //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure + //~| HELP try this + //~| SUGGESTION if let Some(value) = x.field { x.do_option_plus_one(value + captured); } + + + x.field.map(|value| do_nothing(value + captured)); + //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure + //~| HELP try this + //~| SUGGESTION if let Some(value) = x.field { do_nothing(value + captured) } + + x.field.map(|value| { do_nothing(value + captured) }); + //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure + //~| HELP try this + //~| SUGGESTION if let Some(value) = x.field { do_nothing(value + captured) } + + x.field.map(|value| { do_nothing(value + captured); }); + //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure + //~| HELP try this + //~| SUGGESTION if let Some(value) = x.field { do_nothing(value + captured) } + + x.field.map(|value| { { do_nothing(value + captured); } }); + //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure + //~| HELP try this + //~| SUGGESTION if let Some(value) = x.field { do_nothing(value + captured) } + + + x.field.map(|value| diverge(value + captured)); + //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure + //~| HELP try this + //~| SUGGESTION if let Some(value) = x.field { diverge(value + captured) } + + x.field.map(|value| { diverge(value + captured) }); + //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure + //~| HELP try this + //~| SUGGESTION if let Some(value) = x.field { diverge(value + captured) } + + x.field.map(|value| { diverge(value + captured); }); + //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure + //~| HELP try this + //~| SUGGESTION if let Some(value) = x.field { diverge(value + captured) } + + x.field.map(|value| { { diverge(value + captured); } }); + //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure + //~| HELP try this + //~| SUGGESTION if let Some(value) = x.field { diverge(value + captured) } + + + x.field.map(|value| plus_one(value + captured)); + x.field.map(|value| { plus_one(value + captured) }); + x.field.map(|value| { let y = plus_one(value + captured); }); + //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure + //~| HELP try this + //~| SUGGESTION if let Some(value) = x.field { let y = plus_one(value + captured); } + + x.field.map(|value| { plus_one(value + captured); }); + //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure + //~| HELP try this + //~| SUGGESTION if let Some(value) = x.field { plus_one(value + captured); } + + x.field.map(|value| { { plus_one(value + captured); } }); + //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure + //~| HELP try this + //~| SUGGESTION if let Some(value) = x.field { plus_one(value + captured); } + + + x.field.map(|ref value| { do_nothing(value + captured) }); + //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure + //~| HELP try this + //~| SUGGESTION if let Some(ref value) = x.field { do_nothing(value + captured) } + + + // closures with multiple statements are not linted: + x.field.map(|value| { do_nothing(value); do_nothing(value) }); } From 2f52d1d568913f32a6bd24affeeb5ec594061a3c Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Sun, 22 Jan 2017 16:42:57 -0500 Subject: [PATCH 04/17] Return Spans instead of Cow<&str>'s --- clippy_lints/src/map_nil_fn.rs | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/clippy_lints/src/map_nil_fn.rs b/clippy_lints/src/map_nil_fn.rs index 4423aeece6fd..4c85b3d8e7f3 100644 --- a/clippy_lints/src/map_nil_fn.rs +++ b/clippy_lints/src/map_nil_fn.rs @@ -1,7 +1,7 @@ use rustc::hir; use rustc::lint::*; use rustc::ty; -use std::borrow::Cow; +use syntax::codemap::Span; use utils::{in_macro, iter_input_pats, match_type, method_chain_args, snippet, span_lint_and_then}; use utils::paths; @@ -72,7 +72,7 @@ fn is_nil_expression(cx: &LateContext, expr: &hir::Expr) -> bool { // semicolons, which causes problems when generating a suggestion. Given an // expression that evaluates to '()' or '!', recursively remove useless braces // and semi-colons until is suitable for including in the suggestion template -fn reduce_nil_expression<'a>(cx: &LateContext, expr: &'a hir::Expr) -> Option> { +fn reduce_nil_expression<'a>(cx: &LateContext, expr: &'a hir::Expr) -> Option { if !is_nil_expression(cx, expr) { return None; } @@ -81,7 +81,7 @@ fn reduce_nil_expression<'a>(cx: &LateContext, expr: &'a hir::Expr) -> Option { // Calls can't be reduced any more - Some(snippet(cx, expr.span, "_")) + Some(expr.span) }, hir::ExprBlock(ref block) => { match (&block.stmts[..], block.expr.as_ref()) { @@ -92,8 +92,8 @@ fn reduce_nil_expression<'a>(cx: &LateContext, expr: &'a hir::Expr) -> Option { // Reduce `{ X; }` to `X` or `X;` match inner_stmt.node { - hir::StmtDecl(ref d, _) => Some(snippet(cx, d.span, "_")), - hir::StmtExpr(ref e, _) => Some(snippet(cx, e.span, "_")), + hir::StmtDecl(ref d, _) => Some(d.span), + hir::StmtExpr(ref e, _) => Some(e.span), hir::StmtSemi(ref e, _) => { if is_nil_expression(cx, e) { // `X` returns nil so we can strip the @@ -102,7 +102,7 @@ fn reduce_nil_expression<'a>(cx: &LateContext, expr: &'a hir::Expr) -> Option(cx: &LateContext, expr: &'a hir::Expr) -> Option( - cx: &LateContext<'a, 'tcx>, - expr: &'a hir::Expr -) -> Option<(Cow<'a, str>, Cow<'a, str>)> { +fn reduce_nil_closure<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'a hir::Expr) -> Option<(Span, Span)> { if let hir::ExprClosure(_, ref decl, inner_expr_id, _) = expr.node { let body = cx.tcx.map.body(inner_expr_id); if_let_chain! {[ decl.inputs.len() == 1, let Some(binding) = iter_input_pats(&decl, body).next(), - let Some(expr_snippet) = reduce_nil_expression(cx, &body.value), + let Some(expr_span) = reduce_nil_expression(cx, &body.value), ], { - let binding_snippet = snippet(cx, binding.pat.span, "_"); - return Some((binding_snippet, expr_snippet)); + return Some((binding.pat.span, expr_span)) }} } None @@ -152,12 +148,12 @@ fn lint_map_nil_fn(cx: &LateContext, stmt: &hir::Stmt, expr: &hir::Expr, map_arg expr.span, msg, |db| { db.span_suggestion(stmt.span, "try this", suggestion); }); - } else if let Some((binding_snippet, expr_snippet)) = reduce_nil_closure(cx, fn_arg) { + } else if let Some((binding_span, expr_span)) = reduce_nil_closure(cx, fn_arg) { let msg = "called `map(f)` on an Option value where `f` is a nil closure"; let suggestion = format!("if let Some({0}) = {1} {{ {2} }}", - binding_snippet, + snippet(cx, binding_span, "_"), snippet(cx, var_arg.span, "_"), - expr_snippet); + snippet(cx, expr_span, "_")); span_lint_and_then(cx, OPTION_MAP_NIL_FN, From d0bdfe5ce303ded6b499e60bf4e4d4756b85939f Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Sun, 22 Jan 2017 20:57:17 -0500 Subject: [PATCH 05/17] Handle non-trivial nil closures `reduce_nil_closure` mixed together a) 'is this a nil closure?' and b) 'can it be reduced to a simple expression?'. Split the logic into two functions so we can still generate a basic warning when the closure can't be simplified. --- clippy_lints/src/map_nil_fn.rs | 25 ++++++++++++++++--------- clippy_lints/src/misc.rs | 22 ++++++++++++---------- tests/compile-fail/map_nil_fn.rs | 9 ++++++++- 3 files changed, 36 insertions(+), 20 deletions(-) diff --git a/clippy_lints/src/map_nil_fn.rs b/clippy_lints/src/map_nil_fn.rs index 4c85b3d8e7f3..cc998ad00eed 100644 --- a/clippy_lints/src/map_nil_fn.rs +++ b/clippy_lints/src/map_nil_fn.rs @@ -14,7 +14,7 @@ pub struct Pass; /// **Why is this bad?** Readability, this can be written more clearly with /// an if statement /// -/// **Known problems:** Closures with multiple statements are not handled +/// **Known problems:** None. /// /// **Example:** /// ```rust @@ -114,16 +114,17 @@ fn reduce_nil_expression<'a>(cx: &LateContext, expr: &'a hir::Expr) -> Option(cx: &LateContext<'a, 'tcx>, expr: &'a hir::Expr) -> Option<(Span, Span)> { +fn nil_closure<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'a hir::Expr) -> Option<(&'tcx hir::Arg, &'a hir::Expr)> { if let hir::ExprClosure(_, ref decl, inner_expr_id, _) = expr.node { let body = cx.tcx.map.body(inner_expr_id); + let body_expr = &body.value; if_let_chain! {[ decl.inputs.len() == 1, + is_nil_expression(cx, body_expr), let Some(binding) = iter_input_pats(&decl, body).next(), - let Some(expr_span) = reduce_nil_expression(cx, &body.value), ], { - return Some((binding.pat.span, expr_span)) + return Some((binding, body_expr)) }} } None @@ -148,12 +149,18 @@ fn lint_map_nil_fn(cx: &LateContext, stmt: &hir::Stmt, expr: &hir::Expr, map_arg expr.span, msg, |db| { db.span_suggestion(stmt.span, "try this", suggestion); }); - } else if let Some((binding_span, expr_span)) = reduce_nil_closure(cx, fn_arg) { + } else if let Some((binding, closure_expr)) = nil_closure(cx, fn_arg) { let msg = "called `map(f)` on an Option value where `f` is a nil closure"; - let suggestion = format!("if let Some({0}) = {1} {{ {2} }}", - snippet(cx, binding_span, "_"), - snippet(cx, var_arg.span, "_"), - snippet(cx, expr_span, "_")); + let suggestion = if let Some(expr_span) = reduce_nil_expression(cx, closure_expr) { + format!("if let Some({0}) = {1} {{ {2} }}", + snippet(cx, binding.pat.span, "_"), + snippet(cx, var_arg.span, "_"), + snippet(cx, expr_span, "_")) + } else { + format!("if let Some({0}) = {1} {{ ... }}", + snippet(cx, binding.pat.span, "_"), + snippet(cx, var_arg.span, "_")) + }; span_lint_and_then(cx, OPTION_MAP_NIL_FN, diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index e9b6865f0c99..ba2c20db087a 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -432,16 +432,18 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { fn check_nan(cx: &LateContext, path: &Path, expr: &Expr) { if !in_constant(cx, expr.id) { - path.segments.last().map(|seg| { - if seg.name == "NAN" { - span_lint( - cx, - CMP_NAN, - expr.span, - "doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead", - ); - } - }); + if let Some(seg) = path.segments.last() { + path.segments.last().map(|seg| { + if seg.name == "NAN" { + span_lint( + cx, + CMP_NAN, + expr.span, + "doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead", + ); + } + }); + } } } diff --git a/tests/compile-fail/map_nil_fn.rs b/tests/compile-fail/map_nil_fn.rs index 5ac3913466a5..b580e53c9d86 100644 --- a/tests/compile-fail/map_nil_fn.rs +++ b/tests/compile-fail/map_nil_fn.rs @@ -130,6 +130,13 @@ fn main() { //~| SUGGESTION if let Some(ref value) = x.field { do_nothing(value + captured) } - // closures with multiple statements are not linted: x.field.map(|value| { do_nothing(value); do_nothing(value) }); + //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure + //~| HELP try this + //~| SUGGESTION if let Some(value) = x.field { ... } + + x.field.map(|value| if value > 0 { do_nothing(value); do_nothing(value) }); + //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure + //~| HELP try this + //~| SUGGESTION if let Some(value) = x.field { ... } } From 991a30237a5cb8d711e8545a8d6ea93ccae9ad5c Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sun, 8 Apr 2018 09:48:49 +0200 Subject: [PATCH 06/17] Make it compile again --- clippy_lints/src/map_nil_fn.rs | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/map_nil_fn.rs b/clippy_lints/src/map_nil_fn.rs index cc998ad00eed..13ac1fe22946 100644 --- a/clippy_lints/src/map_nil_fn.rs +++ b/clippy_lints/src/map_nil_fn.rs @@ -56,8 +56,8 @@ fn is_nil_type(ty: ty::Ty) -> bool { 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) { + if let ty::TyFnDef(id, _) = ty.sty { + if let Some(fn_type) = cx.tcx.fn_sig(id).no_late_bound_regions() { return is_nil_type(fn_type.output()); } } @@ -115,17 +115,18 @@ fn reduce_nil_expression<'a>(cx: &LateContext, expr: &'a hir::Expr) -> Option(cx: &LateContext<'a, 'tcx>, expr: &'a hir::Expr) -> Option<(&'tcx hir::Arg, &'a hir::Expr)> { - if let hir::ExprClosure(_, ref decl, inner_expr_id, _) = expr.node { - let body = cx.tcx.map.body(inner_expr_id); + if let hir::ExprClosure(_, ref decl, inner_expr_id, _, _) = expr.node { + let body = cx.tcx.hir.body(inner_expr_id); let body_expr = &body.value; - if_let_chain! {[ - decl.inputs.len() == 1, - is_nil_expression(cx, body_expr), - let Some(binding) = iter_input_pats(&decl, body).next(), - ], { - return Some((binding, body_expr)) - }} + if_chain! { + if decl.inputs.len() == 1; + if is_nil_expression(cx, body_expr); + if let Some(binding) = iter_input_pats(&decl, body).next(); + then { + return Some((binding, body_expr)); + } + } } None } @@ -172,7 +173,7 @@ fn lint_map_nil_fn(cx: &LateContext, stmt: &hir::Stmt, expr: &hir::Expr, map_arg impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { fn check_stmt(&mut self, cx: &LateContext, stmt: &hir::Stmt) { - if in_macro(cx, stmt.span) { + if in_macro(stmt.span) { return; } From ca60e8a2a0c59b53c12efa9b1011cac99185aa59 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sun, 8 Apr 2018 10:41:51 +0200 Subject: [PATCH 07/17] Cleanup misc::check_nan This was a bit messed up after a bigger rebase. --- clippy_lints/src/misc.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index ba2c20db087a..797a871d72b8 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -433,16 +433,14 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { fn check_nan(cx: &LateContext, path: &Path, expr: &Expr) { if !in_constant(cx, expr.id) { if let Some(seg) = path.segments.last() { - path.segments.last().map(|seg| { - if seg.name == "NAN" { - span_lint( - cx, - CMP_NAN, - expr.span, - "doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead", + if seg.name == "NAN" { + span_lint( + cx, + CMP_NAN, + expr.span, + "doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead", ); - } - }); + } } } } From fbd71f901fc8ea324fdf3cf75f35e4ce587425ec Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Mon, 9 Apr 2018 07:54:08 +0200 Subject: [PATCH 08/17] Use declare_clippy_lint and 'complexity' category --- clippy_lints/src/map_nil_fn.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/map_nil_fn.rs b/clippy_lints/src/map_nil_fn.rs index 13ac1fe22946..b72c0a01eda1 100644 --- a/clippy_lints/src/map_nil_fn.rs +++ b/clippy_lints/src/map_nil_fn.rs @@ -32,9 +32,9 @@ pub struct Pass; /// log_err_msg(format_msg(msg)) /// } /// ``` -declare_lint! { +declare_clippy_lint! { pub OPTION_MAP_NIL_FN, - Allow, + complexity, "using `Option.map(f)`, where f is a nil function or closure" } From a3ff21f4d6a957e337e46d43da4e7f45d80e1504 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Mon, 9 Apr 2018 08:19:40 +0200 Subject: [PATCH 09/17] Rename lint to option_map_unit_fn Rust does not have nil. --- CHANGELOG.md | 2 +- clippy_lints/src/lib.rs | 6 +- .../{map_nil_fn.rs => option_map_unit_fn.rs} | 67 ++++++++++--------- 3 files changed, 39 insertions(+), 36 deletions(-) rename clippy_lints/src/{map_nil_fn.rs => option_map_unit_fn.rs} (69%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 14438edcb98c..0065ae1a0af2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -705,7 +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 +[`option_map_unit_fn`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#option_map_unit_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 755fc26bc9b8..87781e4e5f7a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -145,7 +145,7 @@ pub mod lifetimes; pub mod literal_representation; pub mod loops; pub mod map_clone; -pub mod map_nil_fn; +pub mod option_map_unit_fn; pub mod matches; pub mod mem_forget; pub mod methods; @@ -406,7 +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_late_lint_pass(box option_map_unit_fn::Pass); reg.register_lint_group("clippy_restriction", vec![ @@ -443,7 +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, + option_map_unit_fn::OPTION_MAP_UNIT_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/option_map_unit_fn.rs similarity index 69% rename from clippy_lints/src/map_nil_fn.rs rename to clippy_lints/src/option_map_unit_fn.rs index b72c0a01eda1..df3c78a344b7 100644 --- a/clippy_lints/src/map_nil_fn.rs +++ b/clippy_lints/src/option_map_unit_fn.rs @@ -8,8 +8,8 @@ use utils::paths; #[derive(Clone)] pub struct Pass; -/// **What it does:** Checks for usage of `Option.map(f)` where f is a nil -/// function or closure +/// **What it does:** Checks for usage of `Option.map(f)` where f is a function +/// or closure that returns the unit type. /// /// **Why is this bad?** Readability, this can be written more clearly with /// an if statement @@ -17,12 +17,15 @@ pub struct Pass; /// **Known problems:** None. /// /// **Example:** +/// /// ```rust /// let x : Option<&str> = do_stuff(); /// x.map(log_err_msg); /// x.map(|msg| log_err_msg(format_msg(msg))) /// ``` +/// /// The correct use would be: +/// /// ```rust /// let x : Option<&str> = do_stuff(); /// if let Some(msg) = x { @@ -33,19 +36,19 @@ pub struct Pass; /// } /// ``` declare_clippy_lint! { - pub OPTION_MAP_NIL_FN, + pub OPTION_MAP_UNIT_FN, complexity, - "using `Option.map(f)`, where f is a nil function or closure" + "using `Option.map(f)`, where f is a function or closure that returns ()" } impl LintPass for Pass { fn get_lints(&self) -> LintArray { - lint_array!(OPTION_MAP_NIL_FN) + lint_array!(OPTION_MAP_UNIT_FN) } } -fn is_nil_type(ty: ty::Ty) -> bool { +fn is_unit_type(ty: ty::Ty) -> bool { match ty.sty { ty::TyTuple(slice) => slice.is_empty(), ty::TyNever => true, @@ -53,27 +56,27 @@ fn is_nil_type(ty: ty::Ty) -> bool { } } -fn is_nil_function(cx: &LateContext, expr: &hir::Expr) -> bool { +fn is_unit_function(cx: &LateContext, expr: &hir::Expr) -> bool { let ty = cx.tables.expr_ty(expr); if let ty::TyFnDef(id, _) = ty.sty { if let Some(fn_type) = cx.tcx.fn_sig(id).no_late_bound_regions() { - return is_nil_type(fn_type.output()); + return is_unit_type(fn_type.output()); } } false } -fn is_nil_expression(cx: &LateContext, expr: &hir::Expr) -> bool { - is_nil_type(cx.tables.expr_ty(expr)) +fn is_unit_expression(cx: &LateContext, expr: &hir::Expr) -> bool { + is_unit_type(cx.tables.expr_ty(expr)) } -// The expression inside a closure may or may not have surrounding braces and -// semicolons, which causes problems when generating a suggestion. Given an -// expression that evaluates to '()' or '!', recursively remove useless braces -// and semi-colons until is suitable for including in the suggestion template -fn reduce_nil_expression<'a>(cx: &LateContext, expr: &'a hir::Expr) -> Option { - if !is_nil_expression(cx, expr) { +/// The expression inside a closure may or may not have surrounding braces and +/// semicolons, which causes problems when generating a suggestion. Given an +/// expression that evaluates to '()' or '!', recursively remove useless braces +/// and semi-colons until is suitable for including in the suggestion template +fn reduce_unit_expression<'a>(cx: &LateContext, expr: &'a hir::Expr) -> Option { + if !is_unit_expression(cx, expr) { return None; } @@ -87,7 +90,7 @@ fn reduce_nil_expression<'a>(cx: &LateContext, expr: &'a hir::Expr) -> Option { // Reduce `{ X }` to `X` - reduce_nil_expression(cx, inner_expr) + reduce_unit_expression(cx, inner_expr) }, (&[ref inner_stmt], None) => { // Reduce `{ X; }` to `X` or `X;` @@ -95,12 +98,12 @@ fn reduce_nil_expression<'a>(cx: &LateContext, expr: &'a hir::Expr) -> Option Some(d.span), hir::StmtExpr(ref e, _) => Some(e.span), hir::StmtSemi(ref e, _) => { - if is_nil_expression(cx, e) { - // `X` returns nil so we can strip the + if is_unit_expression(cx, e) { + // `X` returns unit so we can strip the // semicolon and reduce further - reduce_nil_expression(cx, e) + reduce_unit_expression(cx, e) } else { - // `X` doesn't return nil so it needs a + // `X` doesn't return unit so it needs a // trailing semicolon Some(inner_stmt.span) } @@ -114,14 +117,14 @@ fn reduce_nil_expression<'a>(cx: &LateContext, expr: &'a hir::Expr) -> Option(cx: &LateContext<'a, 'tcx>, expr: &'a hir::Expr) -> Option<(&'tcx hir::Arg, &'a hir::Expr)> { +fn unit_closure<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'a hir::Expr) -> Option<(&'tcx hir::Arg, &'a hir::Expr)> { if let hir::ExprClosure(_, ref decl, inner_expr_id, _, _) = expr.node { let body = cx.tcx.hir.body(inner_expr_id); let body_expr = &body.value; if_chain! { if decl.inputs.len() == 1; - if is_nil_expression(cx, body_expr); + if is_unit_expression(cx, body_expr); if let Some(binding) = iter_input_pats(&decl, body).next(); then { return Some((binding, body_expr)); @@ -131,7 +134,7 @@ fn nil_closure<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'a hir::Expr) -> Opt None } -fn lint_map_nil_fn(cx: &LateContext, stmt: &hir::Stmt, expr: &hir::Expr, map_args: &[hir::Expr]) { +fn lint_map_unit_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]; @@ -139,20 +142,20 @@ fn lint_map_nil_fn(cx: &LateContext, stmt: &hir::Stmt, expr: &hir::Expr, map_arg return; } - if is_nil_function(cx, fn_arg) { - let msg = "called `map(f)` on an Option value where `f` is a nil function"; + if is_unit_function(cx, fn_arg) { + let msg = "called `map(f)` on an Option value where `f` is a unit function"; let suggestion = format!("if let Some(...) = {0} {{ {1}(...) }}", snippet(cx, var_arg.span, "_"), snippet(cx, fn_arg.span, "_")); span_lint_and_then(cx, - OPTION_MAP_NIL_FN, + OPTION_MAP_UNIT_FN, expr.span, msg, |db| { db.span_suggestion(stmt.span, "try this", suggestion); }); - } else if let Some((binding, closure_expr)) = nil_closure(cx, fn_arg) { - let msg = "called `map(f)` on an Option value where `f` is a nil closure"; - let suggestion = if let Some(expr_span) = reduce_nil_expression(cx, closure_expr) { + } else if let Some((binding, closure_expr)) = unit_closure(cx, fn_arg) { + let msg = "called `map(f)` on an Option value where `f` is a unit closure"; + let suggestion = if let Some(expr_span) = reduce_unit_expression(cx, closure_expr) { format!("if let Some({0}) = {1} {{ {2} }}", snippet(cx, binding.pat.span, "_"), snippet(cx, var_arg.span, "_"), @@ -164,7 +167,7 @@ fn lint_map_nil_fn(cx: &LateContext, stmt: &hir::Stmt, expr: &hir::Expr, map_arg }; span_lint_and_then(cx, - OPTION_MAP_NIL_FN, + OPTION_MAP_UNIT_FN, expr.span, msg, |db| { db.span_suggestion(stmt.span, "try this", suggestion); }); @@ -180,7 +183,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { 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]); + lint_map_unit_fn(cx, stmt, expr, arglists[0]); } } } From bcc335fc9cb56aaef9f824322f1288c149ac9afd Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Mon, 9 Apr 2018 08:20:46 +0200 Subject: [PATCH 10/17] Move test to new UI test system --- tests/compile-fail/map_nil_fn.rs | 142 -------------------------- tests/ui/option_map_unit_fn.rs | 81 +++++++++++++++ tests/ui/option_map_unit_fn.stderr | 156 +++++++++++++++++++++++++++++ 3 files changed, 237 insertions(+), 142 deletions(-) delete mode 100644 tests/compile-fail/map_nil_fn.rs create mode 100644 tests/ui/option_map_unit_fn.rs create mode 100644 tests/ui/option_map_unit_fn.stderr diff --git a/tests/compile-fail/map_nil_fn.rs b/tests/compile-fail/map_nil_fn.rs deleted file mode 100644 index b580e53c9d86..000000000000 --- a/tests/compile-fail/map_nil_fn.rs +++ /dev/null @@ -1,142 +0,0 @@ -#![feature(plugin)] -#![feature(const_fn)] -#![plugin(clippy)] - -#![deny(clippy_pedantic)] -#![allow(unused, missing_docs_in_private_items)] - -fn do_nothing(_: T) {} - -fn diverge(_: T) -> ! { - panic!() -} - -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(...) } - - x.field.map(diverge); - //~^ ERROR called `map(f)` on an Option value where `f` is a nil function - //~| HELP try this - //~| SUGGESTION if let Some(...) = x.field { diverge(...) } - - let captured = 10; - if let Some(value) = x.field { do_nothing(value + captured) }; - let _ : Option<()> = x.field.map(|value| do_nothing(value + captured)); - - x.field.map(|value| x.do_option_nothing(value + captured)); - //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure - //~| HELP try this - //~| SUGGESTION if let Some(value) = x.field { x.do_option_nothing(value + captured) } - - x.field.map(|value| { x.do_option_plus_one(value + captured); }); - //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure - //~| HELP try this - //~| SUGGESTION if let Some(value) = x.field { x.do_option_plus_one(value + captured); } - - - x.field.map(|value| do_nothing(value + captured)); - //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure - //~| HELP try this - //~| SUGGESTION if let Some(value) = x.field { do_nothing(value + captured) } - - x.field.map(|value| { do_nothing(value + captured) }); - //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure - //~| HELP try this - //~| SUGGESTION if let Some(value) = x.field { do_nothing(value + captured) } - - x.field.map(|value| { do_nothing(value + captured); }); - //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure - //~| HELP try this - //~| SUGGESTION if let Some(value) = x.field { do_nothing(value + captured) } - - x.field.map(|value| { { do_nothing(value + captured); } }); - //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure - //~| HELP try this - //~| SUGGESTION if let Some(value) = x.field { do_nothing(value + captured) } - - - x.field.map(|value| diverge(value + captured)); - //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure - //~| HELP try this - //~| SUGGESTION if let Some(value) = x.field { diverge(value + captured) } - - x.field.map(|value| { diverge(value + captured) }); - //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure - //~| HELP try this - //~| SUGGESTION if let Some(value) = x.field { diverge(value + captured) } - - x.field.map(|value| { diverge(value + captured); }); - //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure - //~| HELP try this - //~| SUGGESTION if let Some(value) = x.field { diverge(value + captured) } - - x.field.map(|value| { { diverge(value + captured); } }); - //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure - //~| HELP try this - //~| SUGGESTION if let Some(value) = x.field { diverge(value + captured) } - - - x.field.map(|value| plus_one(value + captured)); - x.field.map(|value| { plus_one(value + captured) }); - x.field.map(|value| { let y = plus_one(value + captured); }); - //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure - //~| HELP try this - //~| SUGGESTION if let Some(value) = x.field { let y = plus_one(value + captured); } - - x.field.map(|value| { plus_one(value + captured); }); - //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure - //~| HELP try this - //~| SUGGESTION if let Some(value) = x.field { plus_one(value + captured); } - - x.field.map(|value| { { plus_one(value + captured); } }); - //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure - //~| HELP try this - //~| SUGGESTION if let Some(value) = x.field { plus_one(value + captured); } - - - x.field.map(|ref value| { do_nothing(value + captured) }); - //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure - //~| HELP try this - //~| SUGGESTION if let Some(ref value) = x.field { do_nothing(value + captured) } - - - x.field.map(|value| { do_nothing(value); do_nothing(value) }); - //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure - //~| HELP try this - //~| SUGGESTION if let Some(value) = x.field { ... } - - x.field.map(|value| if value > 0 { do_nothing(value); do_nothing(value) }); - //~^ ERROR called `map(f)` on an Option value where `f` is a nil closure - //~| HELP try this - //~| SUGGESTION if let Some(value) = x.field { ... } -} diff --git a/tests/ui/option_map_unit_fn.rs b/tests/ui/option_map_unit_fn.rs new file mode 100644 index 000000000000..97182764e0d6 --- /dev/null +++ b/tests/ui/option_map_unit_fn.rs @@ -0,0 +1,81 @@ +#![warn(option_map_unit_fn)] +#![allow(unused)] + +fn do_nothing(_: T) {} + +fn diverge(_: T) -> ! { + panic!() +} + +fn plus_one(value: usize) -> usize { + value + 1 +} + +struct HasOption { + field: Option, +} + +impl HasOption { + fn do_option_nothing(self: &Self, value: usize) {} + + fn do_option_plus_one(self: &Self, 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); + + x.field.map(do_nothing); + + x.field.map(diverge); + + let captured = 10; + if let Some(value) = x.field { do_nothing(value + captured) }; + let _ : Option<()> = x.field.map(|value| do_nothing(value + captured)); + + x.field.map(|value| x.do_option_nothing(value + captured)); + + x.field.map(|value| { x.do_option_plus_one(value + captured); }); + + + x.field.map(|value| do_nothing(value + captured)); + + x.field.map(|value| { do_nothing(value + captured) }); + + x.field.map(|value| { do_nothing(value + captured); }); + + x.field.map(|value| { { do_nothing(value + captured); } }); + + + x.field.map(|value| diverge(value + captured)); + + x.field.map(|value| { diverge(value + captured) }); + + x.field.map(|value| { diverge(value + captured); }); + + x.field.map(|value| { { diverge(value + captured); } }); + + + x.field.map(|value| plus_one(value + captured)); + x.field.map(|value| { plus_one(value + captured) }); + x.field.map(|value| { let y = plus_one(value + captured); }); + + x.field.map(|value| { plus_one(value + captured); }); + + x.field.map(|value| { { plus_one(value + captured); } }); + + + x.field.map(|ref value| { do_nothing(value + captured) }); + + + x.field.map(|value| { do_nothing(value); do_nothing(value) }); + + x.field.map(|value| if value > 0 { do_nothing(value); do_nothing(value) }); +} diff --git a/tests/ui/option_map_unit_fn.stderr b/tests/ui/option_map_unit_fn.stderr new file mode 100644 index 000000000000..ee0945d95037 --- /dev/null +++ b/tests/ui/option_map_unit_fn.stderr @@ -0,0 +1,156 @@ +error: called `map(f)` on an Option value where `f` is a unit function + --> $DIR/option_map_unit_fn.rs:33:5 + | +33 | x.field.map(do_nothing); + | ^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(...) = x.field { do_nothing(...) }` + | + = note: `-D option-map-unit-fn` implied by `-D warnings` + +error: called `map(f)` on an Option value where `f` is a unit function + --> $DIR/option_map_unit_fn.rs:35:5 + | +35 | x.field.map(do_nothing); + | ^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(...) = x.field { do_nothing(...) }` + +error: called `map(f)` on an Option value where `f` is a unit function + --> $DIR/option_map_unit_fn.rs:37:5 + | +37 | x.field.map(diverge); + | ^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(...) = x.field { diverge(...) }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/option_map_unit_fn.rs:43:5 + | +43 | x.field.map(|value| x.do_option_nothing(value + captured)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { x.do_option_nothing(value + captured) }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/option_map_unit_fn.rs:45:5 + | +45 | x.field.map(|value| { x.do_option_plus_one(value + captured); }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { x.do_option_plus_one(value + captured); }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/option_map_unit_fn.rs:48:5 + | +48 | x.field.map(|value| do_nothing(value + captured)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { do_nothing(value + captured) }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/option_map_unit_fn.rs:50:5 + | +50 | x.field.map(|value| { do_nothing(value + captured) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { do_nothing(value + captured) }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/option_map_unit_fn.rs:52:5 + | +52 | x.field.map(|value| { do_nothing(value + captured); }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { do_nothing(value + captured) }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/option_map_unit_fn.rs:54:5 + | +54 | x.field.map(|value| { { do_nothing(value + captured); } }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { do_nothing(value + captured) }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/option_map_unit_fn.rs:57:5 + | +57 | x.field.map(|value| diverge(value + captured)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { diverge(value + captured) }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/option_map_unit_fn.rs:59:5 + | +59 | x.field.map(|value| { diverge(value + captured) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { diverge(value + captured) }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/option_map_unit_fn.rs:61:5 + | +61 | x.field.map(|value| { diverge(value + captured); }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { diverge(value + captured) }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/option_map_unit_fn.rs:63:5 + | +63 | x.field.map(|value| { { diverge(value + captured); } }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { diverge(value + captured) }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/option_map_unit_fn.rs:68:5 + | +68 | x.field.map(|value| { let y = plus_one(value + captured); }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { let y = plus_one(value + captured); }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/option_map_unit_fn.rs:70:5 + | +70 | x.field.map(|value| { plus_one(value + captured); }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { plus_one(value + captured); }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/option_map_unit_fn.rs:72:5 + | +72 | x.field.map(|value| { { plus_one(value + captured); } }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { plus_one(value + captured); }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/option_map_unit_fn.rs:75:5 + | +75 | x.field.map(|ref value| { do_nothing(value + captured) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(ref value) = x.field { do_nothing(value + captured) }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/option_map_unit_fn.rs:78:5 + | +78 | x.field.map(|value| { do_nothing(value); do_nothing(value) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { ... }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/option_map_unit_fn.rs:80:5 + | +80 | x.field.map(|value| if value > 0 { do_nothing(value); do_nothing(value) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { ... }` + +error: aborting due to 19 previous errors + From db60c67c5be3b7f2ceb24ba96c4eecb5015ec09b Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Mon, 9 Apr 2018 08:33:57 +0200 Subject: [PATCH 11/17] Allow new lint in ui/eta.rs --- tests/ui/eta.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ui/eta.rs b/tests/ui/eta.rs index e6fad3bb777d..be84f44bfb10 100644 --- a/tests/ui/eta.rs +++ b/tests/ui/eta.rs @@ -1,6 +1,6 @@ -#![allow(unknown_lints, unused, no_effect, redundant_closure_call, many_single_char_names, needless_pass_by_value)] +#![allow(unknown_lints, unused, no_effect, redundant_closure_call, many_single_char_names, needless_pass_by_value, option_map_unit_fn)] #![warn(redundant_closure, needless_borrow)] fn main() { From 7de707fdba126dfb1bea7c97ef5e18c9a879b799 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Tue, 10 Apr 2018 22:13:58 +0200 Subject: [PATCH 12/17] Remove further semicolon reduction --- clippy_lints/src/option_map_unit_fn.rs | 12 +----------- tests/ui/option_map_unit_fn.stderr | 8 ++++---- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/option_map_unit_fn.rs b/clippy_lints/src/option_map_unit_fn.rs index df3c78a344b7..b383f54993cf 100644 --- a/clippy_lints/src/option_map_unit_fn.rs +++ b/clippy_lints/src/option_map_unit_fn.rs @@ -97,17 +97,7 @@ fn reduce_unit_expression<'a>(cx: &LateContext, expr: &'a hir::Expr) -> Option Some(d.span), hir::StmtExpr(ref e, _) => Some(e.span), - hir::StmtSemi(ref e, _) => { - if is_unit_expression(cx, e) { - // `X` returns unit so we can strip the - // semicolon and reduce further - reduce_unit_expression(cx, e) - } else { - // `X` doesn't return unit so it needs a - // trailing semicolon - Some(inner_stmt.span) - } - }, + hir::StmtSemi(_, _) => Some(inner_stmt.span), } }, _ => None, diff --git a/tests/ui/option_map_unit_fn.stderr b/tests/ui/option_map_unit_fn.stderr index ee0945d95037..84dedf9cd604 100644 --- a/tests/ui/option_map_unit_fn.stderr +++ b/tests/ui/option_map_unit_fn.stderr @@ -62,7 +62,7 @@ error: called `map(f)` on an Option value where `f` is a unit closure 52 | x.field.map(|value| { do_nothing(value + captured); }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | - | help: try this: `if let Some(value) = x.field { do_nothing(value + captured) }` + | help: try this: `if let Some(value) = x.field { do_nothing(value + captured); }` error: called `map(f)` on an Option value where `f` is a unit closure --> $DIR/option_map_unit_fn.rs:54:5 @@ -70,7 +70,7 @@ error: called `map(f)` on an Option value where `f` is a unit closure 54 | x.field.map(|value| { { do_nothing(value + captured); } }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | - | help: try this: `if let Some(value) = x.field { do_nothing(value + captured) }` + | help: try this: `if let Some(value) = x.field { do_nothing(value + captured); }` error: called `map(f)` on an Option value where `f` is a unit closure --> $DIR/option_map_unit_fn.rs:57:5 @@ -94,7 +94,7 @@ error: called `map(f)` on an Option value where `f` is a unit closure 61 | x.field.map(|value| { diverge(value + captured); }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | - | help: try this: `if let Some(value) = x.field { diverge(value + captured) }` + | help: try this: `if let Some(value) = x.field { diverge(value + captured); }` error: called `map(f)` on an Option value where `f` is a unit closure --> $DIR/option_map_unit_fn.rs:63:5 @@ -102,7 +102,7 @@ error: called `map(f)` on an Option value where `f` is a unit closure 63 | x.field.map(|value| { { diverge(value + captured); } }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | - | help: try this: `if let Some(value) = x.field { diverge(value + captured) }` + | help: try this: `if let Some(value) = x.field { diverge(value + captured); }` error: called `map(f)` on an Option value where `f` is a unit closure --> $DIR/option_map_unit_fn.rs:68:5 From d87385b4065d3fd319b5c61e9e2719325eb87f0e Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sun, 15 Apr 2018 11:35:20 +0200 Subject: [PATCH 13/17] Use approximate_suggestion for non-reducible closures --- clippy_lints/src/option_map_unit_fn.rs | 52 ++++++++++++++++++-------- tests/ui/option_map_unit_fn.rs | 8 ++++ tests/ui/option_map_unit_fn.stderr | 24 +++++++++++- 3 files changed, 67 insertions(+), 17 deletions(-) diff --git a/clippy_lints/src/option_map_unit_fn.rs b/clippy_lints/src/option_map_unit_fn.rs index b383f54993cf..799576d01b04 100644 --- a/clippy_lints/src/option_map_unit_fn.rs +++ b/clippy_lints/src/option_map_unit_fn.rs @@ -89,18 +89,27 @@ fn reduce_unit_expression<'a>(cx: &LateContext, expr: &'a hir::Expr) -> Option { match (&block.stmts[..], block.expr.as_ref()) { (&[], Some(inner_expr)) => { - // Reduce `{ X }` to `X` + // If block only contains an expression, + // reduce `{ X }` to `X` reduce_unit_expression(cx, inner_expr) }, (&[ref inner_stmt], None) => { - // Reduce `{ X; }` to `X` or `X;` + // If block only contains statements, + // reduce `{ X; }` to `X` or `X;` match inner_stmt.node { hir::StmtDecl(ref d, _) => Some(d.span), hir::StmtExpr(ref e, _) => Some(e.span), hir::StmtSemi(_, _) => Some(inner_stmt.span), } }, - _ => None, + _ => { + // For closures that contain multiple statements + // it's difficult to get a correct suggestion span + // for all cases (multi-line closures specifically) + // + // We do not attempt to build a suggestion for those right now. + None + } } }, _ => None, @@ -142,25 +151,36 @@ fn lint_map_unit_fn(cx: &LateContext, stmt: &hir::Stmt, expr: &hir::Expr, map_ar OPTION_MAP_UNIT_FN, expr.span, msg, - |db| { db.span_suggestion(stmt.span, "try this", suggestion); }); + |db| { db.span_approximate_suggestion(stmt.span, "try this", suggestion); }); } else if let Some((binding, closure_expr)) = unit_closure(cx, fn_arg) { let msg = "called `map(f)` on an Option value where `f` is a unit closure"; + + enum Suggestion { + Full(String), + Approx(String) + } + let suggestion = if let Some(expr_span) = reduce_unit_expression(cx, closure_expr) { - format!("if let Some({0}) = {1} {{ {2} }}", - snippet(cx, binding.pat.span, "_"), - snippet(cx, var_arg.span, "_"), - snippet(cx, expr_span, "_")) + Suggestion::Full( + format!("if let Some({0}) = {1} {{ {2} }}", + snippet(cx, binding.pat.span, "_"), + snippet(cx, var_arg.span, "_"), + snippet(cx, expr_span, "_")) + ) } else { - format!("if let Some({0}) = {1} {{ ... }}", - snippet(cx, binding.pat.span, "_"), - snippet(cx, var_arg.span, "_")) + Suggestion::Approx( + format!("if let Some({0}) = {1} {{ ... }}", + snippet(cx, binding.pat.span, "_"), + snippet(cx, var_arg.span, "_")) + ) }; - span_lint_and_then(cx, - OPTION_MAP_UNIT_FN, - expr.span, - msg, - |db| { db.span_suggestion(stmt.span, "try this", suggestion); }); + span_lint_and_then(cx, OPTION_MAP_UNIT_FN, expr.span, msg, |db| { + match suggestion { + Suggestion::Full(sugg) => db.span_suggestion(stmt.span, "try this", sugg), + Suggestion::Approx(sugg) => db.span_approximate_suggestion(stmt.span, "try this", sugg), + }; + }); } } diff --git a/tests/ui/option_map_unit_fn.rs b/tests/ui/option_map_unit_fn.rs index 97182764e0d6..595f65d2bbb6 100644 --- a/tests/ui/option_map_unit_fn.rs +++ b/tests/ui/option_map_unit_fn.rs @@ -78,4 +78,12 @@ fn main() { x.field.map(|value| { do_nothing(value); do_nothing(value) }); x.field.map(|value| if value > 0 { do_nothing(value); do_nothing(value) }); + + // Suggestion for the let block should be `{ ... }` as it's too difficult to build a + // proper suggestion for these cases + x.field.map(|value| { + do_nothing(value); + do_nothing(value) + }); + x.field.map(|value| { do_nothing(value); do_nothing(value); }); } diff --git a/tests/ui/option_map_unit_fn.stderr b/tests/ui/option_map_unit_fn.stderr index 84dedf9cd604..10320a5a9208 100644 --- a/tests/ui/option_map_unit_fn.stderr +++ b/tests/ui/option_map_unit_fn.stderr @@ -152,5 +152,27 @@ error: called `map(f)` on an Option value where `f` is a unit closure | | | help: try this: `if let Some(value) = x.field { ... }` -error: aborting due to 19 previous errors +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/option_map_unit_fn.rs:84:5 + | +84 | x.field.map(|value| { + | _____^ + | |_____| + | || +85 | || do_nothing(value); +86 | || do_nothing(value) +87 | || }); + | ||______^- help: try this: `if let Some(value) = x.field { ... }` + | |_______| + | + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/option_map_unit_fn.rs:88:5 + | +88 | x.field.map(|value| { do_nothing(value); do_nothing(value); }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { ... }` + +error: aborting due to 21 previous errors From d54f70f1f6ae6fa571117e3db30040620e2890a1 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sun, 15 Apr 2018 11:37:35 +0200 Subject: [PATCH 14/17] Generate let binding variable name for some cases Given a map call like `x.field.map ...` the suggestion will contain: `if let Some(x_field) ...` Given a map call like `x.map ...` the suggestion will contain: `if let Some(_x) ...` Otherwise it will suggest: `if let Some(_) ...` --- clippy_lints/src/option_map_unit_fn.rs | 17 ++++++++++- tests/ui/option_map_unit_fn.rs | 9 ++++++ tests/ui/option_map_unit_fn.stderr | 42 +++++++++++++++++++++++--- 3 files changed, 62 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/option_map_unit_fn.rs b/clippy_lints/src/option_map_unit_fn.rs index 799576d01b04..abbeaabbdb29 100644 --- a/clippy_lints/src/option_map_unit_fn.rs +++ b/clippy_lints/src/option_map_unit_fn.rs @@ -133,6 +133,20 @@ fn unit_closure<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'a hir::Expr) -> Op None } +/// Builds a name for the let binding variable (var_arg) +/// +/// `x.field` => `x_field` +/// `y` => `_y` +/// +/// Anything else will return `_`. +fn let_binding_name(cx: &LateContext, var_arg: &hir::Expr) -> String { + match &var_arg.node { + hir::ExprField(_, _) => snippet(cx, var_arg.span, "_").replace(".", "_"), + hir::ExprPath(_) => format!("_{}", snippet(cx, var_arg.span, "")), + _ => "_".to_string() + } +} + fn lint_map_unit_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]; @@ -143,7 +157,8 @@ fn lint_map_unit_fn(cx: &LateContext, stmt: &hir::Stmt, expr: &hir::Expr, map_ar if is_unit_function(cx, fn_arg) { let msg = "called `map(f)` on an Option value where `f` is a unit function"; - let suggestion = format!("if let Some(...) = {0} {{ {1}(...) }}", + let suggestion = format!("if let Some({0}) = {1} {{ {2}(...) }}", + let_binding_name(cx, var_arg), snippet(cx, var_arg.span, "_"), snippet(cx, fn_arg.span, "_")); diff --git a/tests/ui/option_map_unit_fn.rs b/tests/ui/option_map_unit_fn.rs index 595f65d2bbb6..d9cfc62a51f8 100644 --- a/tests/ui/option_map_unit_fn.rs +++ b/tests/ui/option_map_unit_fn.rs @@ -86,4 +86,13 @@ fn main() { do_nothing(value) }); x.field.map(|value| { do_nothing(value); do_nothing(value); }); + + // The following should suggest `if let Some(_X) ...` as it's difficult to generate a proper let variable name for them + Some(42).map(diverge); + "12".parse::().ok().map(diverge); + Some(plus_one(1)).map(do_nothing); + + // Should suggest `if let Some(_y) ...` to not override the existing foo variable + let y = Some(42); + y.map(do_nothing); } diff --git a/tests/ui/option_map_unit_fn.stderr b/tests/ui/option_map_unit_fn.stderr index 10320a5a9208..bd19fe053293 100644 --- a/tests/ui/option_map_unit_fn.stderr +++ b/tests/ui/option_map_unit_fn.stderr @@ -4,7 +4,7 @@ error: called `map(f)` on an Option value where `f` is a unit function 33 | x.field.map(do_nothing); | ^^^^^^^^^^^^^^^^^^^^^^^- | | - | help: try this: `if let Some(...) = x.field { do_nothing(...) }` + | help: try this: `if let Some(x_field) = x.field { do_nothing(...) }` | = note: `-D option-map-unit-fn` implied by `-D warnings` @@ -14,7 +14,7 @@ error: called `map(f)` on an Option value where `f` is a unit function 35 | x.field.map(do_nothing); | ^^^^^^^^^^^^^^^^^^^^^^^- | | - | help: try this: `if let Some(...) = x.field { do_nothing(...) }` + | help: try this: `if let Some(x_field) = x.field { do_nothing(...) }` error: called `map(f)` on an Option value where `f` is a unit function --> $DIR/option_map_unit_fn.rs:37:5 @@ -22,7 +22,7 @@ error: called `map(f)` on an Option value where `f` is a unit function 37 | x.field.map(diverge); | ^^^^^^^^^^^^^^^^^^^^- | | - | help: try this: `if let Some(...) = x.field { diverge(...) }` + | help: try this: `if let Some(x_field) = x.field { diverge(...) }` error: called `map(f)` on an Option value where `f` is a unit closure --> $DIR/option_map_unit_fn.rs:43:5 @@ -164,7 +164,7 @@ error: called `map(f)` on an Option value where `f` is a unit closure 87 | || }); | ||______^- help: try this: `if let Some(value) = x.field { ... }` | |_______| - | + | error: called `map(f)` on an Option value where `f` is a unit closure --> $DIR/option_map_unit_fn.rs:88:5 @@ -174,5 +174,37 @@ error: called `map(f)` on an Option value where `f` is a unit closure | | | help: try this: `if let Some(value) = x.field { ... }` -error: aborting due to 21 previous errors +error: called `map(f)` on an Option value where `f` is a unit function + --> $DIR/option_map_unit_fn.rs:91:5 + | +91 | Some(42).map(diverge); + | ^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(_) = Some(42) { diverge(...) }` + +error: called `map(f)` on an Option value where `f` is a unit function + --> $DIR/option_map_unit_fn.rs:92:5 + | +92 | "12".parse::().ok().map(diverge); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(_) = "12".parse::().ok() { diverge(...) }` + +error: called `map(f)` on an Option value where `f` is a unit function + --> $DIR/option_map_unit_fn.rs:93:5 + | +93 | Some(plus_one(1)).map(do_nothing); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(_) = Some(plus_one(1)) { do_nothing(...) }` + +error: called `map(f)` on an Option value where `f` is a unit function + --> $DIR/option_map_unit_fn.rs:97:5 + | +97 | y.map(do_nothing); + | ^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(_y) = y { do_nothing(...) }` + +error: aborting due to 25 previous errors From 8307a899e994cf87821547b40ed67dae38a81bee Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sun, 15 Apr 2018 12:30:38 +0200 Subject: [PATCH 15/17] Rename option_map_unit_fn to map_unit_fn --- clippy_lints/src/lib.rs | 6 +- .../{option_map_unit_fn.rs => map_unit_fn.rs} | 4 +- tests/ui/map_unit_fn.stderr | 210 ++++++++++++++++++ tests/ui/option_map_unit_fn.rs | 6 +- tests/ui/option_map_unit_fn.stderr | 106 ++++----- 5 files changed, 272 insertions(+), 60 deletions(-) rename clippy_lints/src/{option_map_unit_fn.rs => map_unit_fn.rs} (98%) create mode 100644 tests/ui/map_unit_fn.stderr diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 87781e4e5f7a..afad923a35ab 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -145,7 +145,7 @@ pub mod lifetimes; pub mod literal_representation; pub mod loops; pub mod map_clone; -pub mod option_map_unit_fn; +pub mod map_unit_fn; pub mod matches; pub mod mem_forget; pub mod methods; @@ -406,7 +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 option_map_unit_fn::Pass); + reg.register_late_lint_pass(box map_unit_fn::Pass); reg.register_lint_group("clippy_restriction", vec![ @@ -443,7 +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, - option_map_unit_fn::OPTION_MAP_UNIT_FN, + map_unit_fn::OPTION_MAP_UNIT_FN, matches::SINGLE_MATCH_ELSE, methods::FILTER_MAP, methods::OPTION_MAP_UNWRAP_OR, diff --git a/clippy_lints/src/option_map_unit_fn.rs b/clippy_lints/src/map_unit_fn.rs similarity index 98% rename from clippy_lints/src/option_map_unit_fn.rs rename to clippy_lints/src/map_unit_fn.rs index abbeaabbdb29..97ff1141986a 100644 --- a/clippy_lints/src/option_map_unit_fn.rs +++ b/clippy_lints/src/map_unit_fn.rs @@ -19,7 +19,7 @@ pub struct Pass; /// **Example:** /// /// ```rust -/// let x : Option<&str> = do_stuff(); +/// let x: Option<&str> = do_stuff(); /// x.map(log_err_msg); /// x.map(|msg| log_err_msg(format_msg(msg))) /// ``` @@ -27,7 +27,7 @@ pub struct Pass; /// The correct use would be: /// /// ```rust -/// let x : Option<&str> = do_stuff(); +/// let x: Option<&str> = do_stuff(); /// if let Some(msg) = x { /// log_err_msg(msg) /// } diff --git a/tests/ui/map_unit_fn.stderr b/tests/ui/map_unit_fn.stderr new file mode 100644 index 000000000000..c4ee0ce92380 --- /dev/null +++ b/tests/ui/map_unit_fn.stderr @@ -0,0 +1,210 @@ +error: called `map(f)` on an Option value where `f` is a unit function + --> $DIR/map_unit_fn.rs:33:5 + | +33 | x.field.map(do_nothing); + | ^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(x_field) = x.field { do_nothing(...) }` + | + = note: `-D option-map-unit-fn` implied by `-D warnings` + +error: called `map(f)` on an Option value where `f` is a unit function + --> $DIR/map_unit_fn.rs:35:5 + | +35 | x.field.map(do_nothing); + | ^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(x_field) = x.field { do_nothing(...) }` + +error: called `map(f)` on an Option value where `f` is a unit function + --> $DIR/map_unit_fn.rs:37:5 + | +37 | x.field.map(diverge); + | ^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(x_field) = x.field { diverge(...) }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/map_unit_fn.rs:43:5 + | +43 | x.field.map(|value| x.do_option_nothing(value + captured)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { x.do_option_nothing(value + captured) }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/map_unit_fn.rs:45:5 + | +45 | x.field.map(|value| { x.do_option_plus_one(value + captured); }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { x.do_option_plus_one(value + captured); }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/map_unit_fn.rs:48:5 + | +48 | x.field.map(|value| do_nothing(value + captured)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { do_nothing(value + captured) }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/map_unit_fn.rs:50:5 + | +50 | x.field.map(|value| { do_nothing(value + captured) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { do_nothing(value + captured) }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/map_unit_fn.rs:52:5 + | +52 | x.field.map(|value| { do_nothing(value + captured); }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { do_nothing(value + captured); }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/map_unit_fn.rs:54:5 + | +54 | x.field.map(|value| { { do_nothing(value + captured); } }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { do_nothing(value + captured); }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/map_unit_fn.rs:57:5 + | +57 | x.field.map(|value| diverge(value + captured)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { diverge(value + captured) }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/map_unit_fn.rs:59:5 + | +59 | x.field.map(|value| { diverge(value + captured) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { diverge(value + captured) }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/map_unit_fn.rs:61:5 + | +61 | x.field.map(|value| { diverge(value + captured); }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { diverge(value + captured); }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/map_unit_fn.rs:63:5 + | +63 | x.field.map(|value| { { diverge(value + captured); } }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { diverge(value + captured); }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/map_unit_fn.rs:68:5 + | +68 | x.field.map(|value| { let y = plus_one(value + captured); }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { let y = plus_one(value + captured); }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/map_unit_fn.rs:70:5 + | +70 | x.field.map(|value| { plus_one(value + captured); }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { plus_one(value + captured); }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/map_unit_fn.rs:72:5 + | +72 | x.field.map(|value| { { plus_one(value + captured); } }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { plus_one(value + captured); }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/map_unit_fn.rs:75:5 + | +75 | x.field.map(|ref value| { do_nothing(value + captured) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(ref value) = x.field { do_nothing(value + captured) }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/map_unit_fn.rs:78:5 + | +78 | x.field.map(|value| { do_nothing(value); do_nothing(value) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { ... }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/map_unit_fn.rs:80:5 + | +80 | x.field.map(|value| if value > 0 { do_nothing(value); do_nothing(value) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { ... }` + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/map_unit_fn.rs:84:5 + | +84 | x.field.map(|value| { + | _____^ + | |_____| + | || +85 | || do_nothing(value); +86 | || do_nothing(value) +87 | || }); + | ||______^- help: try this: `if let Some(value) = x.field { ... }` + | |_______| + | + +error: called `map(f)` on an Option value where `f` is a unit closure + --> $DIR/map_unit_fn.rs:88:5 + | +88 | x.field.map(|value| { do_nothing(value); do_nothing(value); }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(value) = x.field { ... }` + +error: called `map(f)` on an Option value where `f` is a unit function + --> $DIR/map_unit_fn.rs:91:5 + | +91 | Some(42).map(diverge); + | ^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(_) = Some(42) { diverge(...) }` + +error: called `map(f)` on an Option value where `f` is a unit function + --> $DIR/map_unit_fn.rs:92:5 + | +92 | "12".parse::().ok().map(diverge); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(_) = "12".parse::().ok() { diverge(...) }` + +error: called `map(f)` on an Option value where `f` is a unit function + --> $DIR/map_unit_fn.rs:93:5 + | +93 | Some(plus_one(1)).map(do_nothing); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(_) = Some(plus_one(1)) { do_nothing(...) }` + +error: called `map(f)` on an Option value where `f` is a unit function + --> $DIR/map_unit_fn.rs:97:5 + | +97 | y.map(do_nothing); + | ^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Some(_y) = y { do_nothing(...) }` + +error: aborting due to 25 previous errors + diff --git a/tests/ui/option_map_unit_fn.rs b/tests/ui/option_map_unit_fn.rs index d9cfc62a51f8..06531e290326 100644 --- a/tests/ui/option_map_unit_fn.rs +++ b/tests/ui/option_map_unit_fn.rs @@ -23,8 +23,7 @@ impl HasOption { } } -#[cfg_attr(rustfmt, rustfmt_skip)] -fn main() { +fn option_map_unit_fn() { let x = HasOption { field: Some(10) }; x.field.map(plus_one); @@ -96,3 +95,6 @@ fn main() { let y = Some(42); y.map(do_nothing); } + +fn main() { +} diff --git a/tests/ui/option_map_unit_fn.stderr b/tests/ui/option_map_unit_fn.stderr index bd19fe053293..3ca57a65b3f3 100644 --- a/tests/ui/option_map_unit_fn.stderr +++ b/tests/ui/option_map_unit_fn.stderr @@ -1,7 +1,7 @@ error: called `map(f)` on an Option value where `f` is a unit function - --> $DIR/option_map_unit_fn.rs:33:5 + --> $DIR/option_map_unit_fn.rs:32:5 | -33 | x.field.map(do_nothing); +32 | x.field.map(do_nothing); | ^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(x_field) = x.field { do_nothing(...) }` @@ -9,199 +9,199 @@ error: called `map(f)` on an Option value where `f` is a unit function = note: `-D option-map-unit-fn` implied by `-D warnings` error: called `map(f)` on an Option value where `f` is a unit function - --> $DIR/option_map_unit_fn.rs:35:5 + --> $DIR/option_map_unit_fn.rs:34:5 | -35 | x.field.map(do_nothing); +34 | x.field.map(do_nothing); | ^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(x_field) = x.field { do_nothing(...) }` error: called `map(f)` on an Option value where `f` is a unit function - --> $DIR/option_map_unit_fn.rs:37:5 + --> $DIR/option_map_unit_fn.rs:36:5 | -37 | x.field.map(diverge); +36 | x.field.map(diverge); | ^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(x_field) = x.field { diverge(...) }` error: called `map(f)` on an Option value where `f` is a unit closure - --> $DIR/option_map_unit_fn.rs:43:5 + --> $DIR/option_map_unit_fn.rs:42:5 | -43 | x.field.map(|value| x.do_option_nothing(value + captured)); +42 | x.field.map(|value| x.do_option_nothing(value + captured)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(value) = x.field { x.do_option_nothing(value + captured) }` error: called `map(f)` on an Option value where `f` is a unit closure - --> $DIR/option_map_unit_fn.rs:45:5 + --> $DIR/option_map_unit_fn.rs:44:5 | -45 | x.field.map(|value| { x.do_option_plus_one(value + captured); }); +44 | x.field.map(|value| { x.do_option_plus_one(value + captured); }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(value) = x.field { x.do_option_plus_one(value + captured); }` error: called `map(f)` on an Option value where `f` is a unit closure - --> $DIR/option_map_unit_fn.rs:48:5 + --> $DIR/option_map_unit_fn.rs:47:5 | -48 | x.field.map(|value| do_nothing(value + captured)); +47 | x.field.map(|value| do_nothing(value + captured)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(value) = x.field { do_nothing(value + captured) }` error: called `map(f)` on an Option value where `f` is a unit closure - --> $DIR/option_map_unit_fn.rs:50:5 + --> $DIR/option_map_unit_fn.rs:49:5 | -50 | x.field.map(|value| { do_nothing(value + captured) }); +49 | x.field.map(|value| { do_nothing(value + captured) }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(value) = x.field { do_nothing(value + captured) }` error: called `map(f)` on an Option value where `f` is a unit closure - --> $DIR/option_map_unit_fn.rs:52:5 + --> $DIR/option_map_unit_fn.rs:51:5 | -52 | x.field.map(|value| { do_nothing(value + captured); }); +51 | x.field.map(|value| { do_nothing(value + captured); }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(value) = x.field { do_nothing(value + captured); }` error: called `map(f)` on an Option value where `f` is a unit closure - --> $DIR/option_map_unit_fn.rs:54:5 + --> $DIR/option_map_unit_fn.rs:53:5 | -54 | x.field.map(|value| { { do_nothing(value + captured); } }); +53 | x.field.map(|value| { { do_nothing(value + captured); } }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(value) = x.field { do_nothing(value + captured); }` error: called `map(f)` on an Option value where `f` is a unit closure - --> $DIR/option_map_unit_fn.rs:57:5 + --> $DIR/option_map_unit_fn.rs:56:5 | -57 | x.field.map(|value| diverge(value + captured)); +56 | x.field.map(|value| diverge(value + captured)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(value) = x.field { diverge(value + captured) }` error: called `map(f)` on an Option value where `f` is a unit closure - --> $DIR/option_map_unit_fn.rs:59:5 + --> $DIR/option_map_unit_fn.rs:58:5 | -59 | x.field.map(|value| { diverge(value + captured) }); +58 | x.field.map(|value| { diverge(value + captured) }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(value) = x.field { diverge(value + captured) }` error: called `map(f)` on an Option value where `f` is a unit closure - --> $DIR/option_map_unit_fn.rs:61:5 + --> $DIR/option_map_unit_fn.rs:60:5 | -61 | x.field.map(|value| { diverge(value + captured); }); +60 | x.field.map(|value| { diverge(value + captured); }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(value) = x.field { diverge(value + captured); }` error: called `map(f)` on an Option value where `f` is a unit closure - --> $DIR/option_map_unit_fn.rs:63:5 + --> $DIR/option_map_unit_fn.rs:62:5 | -63 | x.field.map(|value| { { diverge(value + captured); } }); +62 | x.field.map(|value| { { diverge(value + captured); } }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(value) = x.field { diverge(value + captured); }` error: called `map(f)` on an Option value where `f` is a unit closure - --> $DIR/option_map_unit_fn.rs:68:5 + --> $DIR/option_map_unit_fn.rs:67:5 | -68 | x.field.map(|value| { let y = plus_one(value + captured); }); +67 | x.field.map(|value| { let y = plus_one(value + captured); }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(value) = x.field { let y = plus_one(value + captured); }` error: called `map(f)` on an Option value where `f` is a unit closure - --> $DIR/option_map_unit_fn.rs:70:5 + --> $DIR/option_map_unit_fn.rs:69:5 | -70 | x.field.map(|value| { plus_one(value + captured); }); +69 | x.field.map(|value| { plus_one(value + captured); }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(value) = x.field { plus_one(value + captured); }` error: called `map(f)` on an Option value where `f` is a unit closure - --> $DIR/option_map_unit_fn.rs:72:5 + --> $DIR/option_map_unit_fn.rs:71:5 | -72 | x.field.map(|value| { { plus_one(value + captured); } }); +71 | x.field.map(|value| { { plus_one(value + captured); } }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(value) = x.field { plus_one(value + captured); }` error: called `map(f)` on an Option value where `f` is a unit closure - --> $DIR/option_map_unit_fn.rs:75:5 + --> $DIR/option_map_unit_fn.rs:74:5 | -75 | x.field.map(|ref value| { do_nothing(value + captured) }); +74 | x.field.map(|ref value| { do_nothing(value + captured) }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(ref value) = x.field { do_nothing(value + captured) }` error: called `map(f)` on an Option value where `f` is a unit closure - --> $DIR/option_map_unit_fn.rs:78:5 + --> $DIR/option_map_unit_fn.rs:77:5 | -78 | x.field.map(|value| { do_nothing(value); do_nothing(value) }); +77 | x.field.map(|value| { do_nothing(value); do_nothing(value) }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(value) = x.field { ... }` error: called `map(f)` on an Option value where `f` is a unit closure - --> $DIR/option_map_unit_fn.rs:80:5 + --> $DIR/option_map_unit_fn.rs:79:5 | -80 | x.field.map(|value| if value > 0 { do_nothing(value); do_nothing(value) }); +79 | x.field.map(|value| if value > 0 { do_nothing(value); do_nothing(value) }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(value) = x.field { ... }` error: called `map(f)` on an Option value where `f` is a unit closure - --> $DIR/option_map_unit_fn.rs:84:5 + --> $DIR/option_map_unit_fn.rs:83:5 | -84 | x.field.map(|value| { +83 | x.field.map(|value| { | _____^ | |_____| | || -85 | || do_nothing(value); -86 | || do_nothing(value) -87 | || }); +84 | || do_nothing(value); +85 | || do_nothing(value) +86 | || }); | ||______^- help: try this: `if let Some(value) = x.field { ... }` | |_______| | error: called `map(f)` on an Option value where `f` is a unit closure - --> $DIR/option_map_unit_fn.rs:88:5 + --> $DIR/option_map_unit_fn.rs:87:5 | -88 | x.field.map(|value| { do_nothing(value); do_nothing(value); }); +87 | x.field.map(|value| { do_nothing(value); do_nothing(value); }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(value) = x.field { ... }` error: called `map(f)` on an Option value where `f` is a unit function - --> $DIR/option_map_unit_fn.rs:91:5 + --> $DIR/option_map_unit_fn.rs:90:5 | -91 | Some(42).map(diverge); +90 | Some(42).map(diverge); | ^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(_) = Some(42) { diverge(...) }` error: called `map(f)` on an Option value where `f` is a unit function - --> $DIR/option_map_unit_fn.rs:92:5 + --> $DIR/option_map_unit_fn.rs:91:5 | -92 | "12".parse::().ok().map(diverge); +91 | "12".parse::().ok().map(diverge); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(_) = "12".parse::().ok() { diverge(...) }` error: called `map(f)` on an Option value where `f` is a unit function - --> $DIR/option_map_unit_fn.rs:93:5 + --> $DIR/option_map_unit_fn.rs:92:5 | -93 | Some(plus_one(1)).map(do_nothing); +92 | Some(plus_one(1)).map(do_nothing); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(_) = Some(plus_one(1)) { do_nothing(...) }` error: called `map(f)` on an Option value where `f` is a unit function - --> $DIR/option_map_unit_fn.rs:97:5 + --> $DIR/option_map_unit_fn.rs:96:5 | -97 | y.map(do_nothing); +96 | y.map(do_nothing); | ^^^^^^^^^^^^^^^^^- | | | help: try this: `if let Some(_y) = y { do_nothing(...) }` From 4f4e20c561b223027e47183e58c4ec17f406d809 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sun, 15 Apr 2018 13:00:12 +0200 Subject: [PATCH 16/17] Also lint Result.map for unit returns --- CHANGELOG.md | 2 +- clippy_lints/src/lib.rs | 1 + clippy_lints/src/map_unit_fn.rs | 77 +++++++++--- tests/ui/result_map_unit_fn.rs | 102 +++++++++++++++ tests/ui/result_map_unit_fn.stderr | 194 +++++++++++++++++++++++++++++ 5 files changed, 360 insertions(+), 16 deletions(-) create mode 100644 tests/ui/result_map_unit_fn.rs create mode 100644 tests/ui/result_map_unit_fn.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 0065ae1a0af2..f66c0086894b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -705,7 +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_unit_fn`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#option_map_unit_fn +[`map_unit_fn`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#map_unit_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 afad923a35ab..ea8478c373b8 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -444,6 +444,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { infinite_iter::MAYBE_INFINITE_ITER, items_after_statements::ITEMS_AFTER_STATEMENTS, map_unit_fn::OPTION_MAP_UNIT_FN, + map_unit_fn::RESULT_MAP_UNIT_FN, matches::SINGLE_MATCH_ELSE, methods::FILTER_MAP, methods::OPTION_MAP_UNWRAP_OR, diff --git a/clippy_lints/src/map_unit_fn.rs b/clippy_lints/src/map_unit_fn.rs index 97ff1141986a..c3fc19699ea9 100644 --- a/clippy_lints/src/map_unit_fn.rs +++ b/clippy_lints/src/map_unit_fn.rs @@ -41,10 +41,43 @@ declare_clippy_lint! { "using `Option.map(f)`, where f is a function or closure that returns ()" } +/// **What it does:** Checks for usage of `Result.map(f)` where f is a function +/// or closure that returns the unit type. +/// +/// **Why is this bad?** Readability, this can be written more clearly with +/// an if statement +/// +/// **Known problems:** None. +/// +/// **Example:** +/// +/// ```rust +/// let x: Result<&str, &str> = do_stuff(); +/// x.map(log_err_msg); +/// x.map(|msg| log_err_msg(format_msg(msg))) +/// ``` +/// +/// The correct use would be: +/// +/// ```rust +/// let x: Result<&str, &str> = do_stuff(); +/// if let Ok(msg) = x { +/// log_err_msg(msg) +/// } +/// if let Ok(msg) = x { +/// log_err_msg(format_msg(msg)) +/// } +/// ``` +declare_clippy_lint! { + pub RESULT_MAP_UNIT_FN, + complexity, + "using `Result.map(f)`, where f is a function or closure that returns ()" +} + impl LintPass for Pass { fn get_lints(&self) -> LintArray { - lint_array!(OPTION_MAP_UNIT_FN) + lint_array!(OPTION_MAP_UNIT_FN, RESULT_MAP_UNIT_FN) } } @@ -147,28 +180,40 @@ fn let_binding_name(cx: &LateContext, var_arg: &hir::Expr) -> String { } } +fn suggestion_msg(function_type: &str, map_type: &str) -> String { + format!( + "called `map(f)` on an {0} value where `f` is a unit {1}", + map_type, + function_type + ) +} + fn lint_map_unit_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 (map_type, variant, lint) = + if match_type(cx, cx.tables.expr_ty(var_arg), &paths::OPTION) { + ("Option", "Some", OPTION_MAP_UNIT_FN) + } else if match_type(cx, cx.tables.expr_ty(var_arg), &paths::RESULT) { + ("Result", "Ok", RESULT_MAP_UNIT_FN) + } else { + return + }; if is_unit_function(cx, fn_arg) { - let msg = "called `map(f)` on an Option value where `f` is a unit function"; - let suggestion = format!("if let Some({0}) = {1} {{ {2}(...) }}", + let msg = suggestion_msg("function", map_type); + let suggestion = format!("if let {0}({1}) = {2} {{ {3}(...) }}", + variant, let_binding_name(cx, var_arg), snippet(cx, var_arg.span, "_"), snippet(cx, fn_arg.span, "_")); - span_lint_and_then(cx, - OPTION_MAP_UNIT_FN, - expr.span, - msg, - |db| { db.span_approximate_suggestion(stmt.span, "try this", suggestion); }); + span_lint_and_then(cx, lint, expr.span, &msg, |db| { + db.span_approximate_suggestion(stmt.span, "try this", suggestion); + }); } else if let Some((binding, closure_expr)) = unit_closure(cx, fn_arg) { - let msg = "called `map(f)` on an Option value where `f` is a unit closure"; + let msg = suggestion_msg("closure", map_type); enum Suggestion { Full(String), @@ -177,20 +222,22 @@ fn lint_map_unit_fn(cx: &LateContext, stmt: &hir::Stmt, expr: &hir::Expr, map_ar let suggestion = if let Some(expr_span) = reduce_unit_expression(cx, closure_expr) { Suggestion::Full( - format!("if let Some({0}) = {1} {{ {2} }}", + format!("if let {0}({1}) = {2} {{ {3} }}", + variant, snippet(cx, binding.pat.span, "_"), snippet(cx, var_arg.span, "_"), snippet(cx, expr_span, "_")) ) } else { Suggestion::Approx( - format!("if let Some({0}) = {1} {{ ... }}", + format!("if let {0}({1}) = {2} {{ ... }}", + variant, snippet(cx, binding.pat.span, "_"), snippet(cx, var_arg.span, "_")) ) }; - span_lint_and_then(cx, OPTION_MAP_UNIT_FN, expr.span, msg, |db| { + span_lint_and_then(cx, lint, expr.span, &msg, |db| { match suggestion { Suggestion::Full(sugg) => db.span_suggestion(stmt.span, "try this", sugg), Suggestion::Approx(sugg) => db.span_approximate_suggestion(stmt.span, "try this", sugg), diff --git a/tests/ui/result_map_unit_fn.rs b/tests/ui/result_map_unit_fn.rs new file mode 100644 index 000000000000..8f3c1579987d --- /dev/null +++ b/tests/ui/result_map_unit_fn.rs @@ -0,0 +1,102 @@ +#![warn(result_map_unit_fn)] +#![allow(unused)] + +fn do_nothing(_: T) {} + +fn diverge(_: T) -> ! { + panic!() +} + +fn plus_one(value: usize) -> usize { + value + 1 +} + +struct HasResult { + field: Result, +} + +impl HasResult { + fn do_result_nothing(self: &Self, value: usize) {} + + fn do_result_plus_one(self: &Self, value: usize) -> usize { + value + 1 + } +} + +fn result_map_unit_fn() { + let x = HasResult { field: Ok(10) }; + + x.field.map(plus_one); + let _ : Result<(), usize> = x.field.map(do_nothing); + + x.field.map(do_nothing); + + x.field.map(do_nothing); + + x.field.map(diverge); + + let captured = 10; + if let Ok(value) = x.field { do_nothing(value + captured) }; + let _ : Result<(), usize> = x.field.map(|value| do_nothing(value + captured)); + + x.field.map(|value| x.do_result_nothing(value + captured)); + + x.field.map(|value| { x.do_result_plus_one(value + captured); }); + + + x.field.map(|value| do_nothing(value + captured)); + + x.field.map(|value| { do_nothing(value + captured) }); + + x.field.map(|value| { do_nothing(value + captured); }); + + x.field.map(|value| { { do_nothing(value + captured); } }); + + + x.field.map(|value| diverge(value + captured)); + + x.field.map(|value| { diverge(value + captured) }); + + x.field.map(|value| { diverge(value + captured); }); + + x.field.map(|value| { { diverge(value + captured); } }); + + + x.field.map(|value| plus_one(value + captured)); + x.field.map(|value| { plus_one(value + captured) }); + x.field.map(|value| { let y = plus_one(value + captured); }); + + x.field.map(|value| { plus_one(value + captured); }); + + x.field.map(|value| { { plus_one(value + captured); } }); + + + x.field.map(|ref value| { do_nothing(value + captured) }); + + + x.field.map(|value| { do_nothing(value); do_nothing(value) }); + + x.field.map(|value| if value > 0 { do_nothing(value); do_nothing(value) }); + + // Suggestion for the let block should be `{ ... }` as it's too difficult to build a + // proper suggestion for these cases + x.field.map(|value| { + do_nothing(value); + do_nothing(value) + }); + x.field.map(|value| { do_nothing(value); do_nothing(value); }); + + // The following should suggest `if let Ok(_X) ...` as it's difficult to generate a proper let variable name for them + let res: Result = Ok(42).map(diverge); + "12".parse::().map(diverge); + + let res: Result<(), usize> = Ok(plus_one(1)).map(do_nothing); + + // Should suggest `if let Ok(_y) ...` to not override the existing foo variable + let y: Result = Ok(42); + y.map(do_nothing); +} + +fn main() { +} + diff --git a/tests/ui/result_map_unit_fn.stderr b/tests/ui/result_map_unit_fn.stderr new file mode 100644 index 000000000000..199f5e7cf97e --- /dev/null +++ b/tests/ui/result_map_unit_fn.stderr @@ -0,0 +1,194 @@ +error: called `map(f)` on an Result value where `f` is a unit function + --> $DIR/result_map_unit_fn.rs:32:5 + | +32 | x.field.map(do_nothing); + | ^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(x_field) = x.field { do_nothing(...) }` + | + = note: `-D result-map-unit-fn` implied by `-D warnings` + +error: called `map(f)` on an Result value where `f` is a unit function + --> $DIR/result_map_unit_fn.rs:34:5 + | +34 | x.field.map(do_nothing); + | ^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(x_field) = x.field { do_nothing(...) }` + +error: called `map(f)` on an Result value where `f` is a unit function + --> $DIR/result_map_unit_fn.rs:36:5 + | +36 | x.field.map(diverge); + | ^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(x_field) = x.field { diverge(...) }` + +error: called `map(f)` on an Result value where `f` is a unit closure + --> $DIR/result_map_unit_fn.rs:42:5 + | +42 | x.field.map(|value| x.do_result_nothing(value + captured)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(value) = x.field { x.do_result_nothing(value + captured) }` + +error: called `map(f)` on an Result value where `f` is a unit closure + --> $DIR/result_map_unit_fn.rs:44:5 + | +44 | x.field.map(|value| { x.do_result_plus_one(value + captured); }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(value) = x.field { x.do_result_plus_one(value + captured); }` + +error: called `map(f)` on an Result value where `f` is a unit closure + --> $DIR/result_map_unit_fn.rs:47:5 + | +47 | x.field.map(|value| do_nothing(value + captured)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(value) = x.field { do_nothing(value + captured) }` + +error: called `map(f)` on an Result value where `f` is a unit closure + --> $DIR/result_map_unit_fn.rs:49:5 + | +49 | x.field.map(|value| { do_nothing(value + captured) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(value) = x.field { do_nothing(value + captured) }` + +error: called `map(f)` on an Result value where `f` is a unit closure + --> $DIR/result_map_unit_fn.rs:51:5 + | +51 | x.field.map(|value| { do_nothing(value + captured); }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(value) = x.field { do_nothing(value + captured); }` + +error: called `map(f)` on an Result value where `f` is a unit closure + --> $DIR/result_map_unit_fn.rs:53:5 + | +53 | x.field.map(|value| { { do_nothing(value + captured); } }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(value) = x.field { do_nothing(value + captured); }` + +error: called `map(f)` on an Result value where `f` is a unit closure + --> $DIR/result_map_unit_fn.rs:56:5 + | +56 | x.field.map(|value| diverge(value + captured)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(value) = x.field { diverge(value + captured) }` + +error: called `map(f)` on an Result value where `f` is a unit closure + --> $DIR/result_map_unit_fn.rs:58:5 + | +58 | x.field.map(|value| { diverge(value + captured) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(value) = x.field { diverge(value + captured) }` + +error: called `map(f)` on an Result value where `f` is a unit closure + --> $DIR/result_map_unit_fn.rs:60:5 + | +60 | x.field.map(|value| { diverge(value + captured); }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(value) = x.field { diverge(value + captured); }` + +error: called `map(f)` on an Result value where `f` is a unit closure + --> $DIR/result_map_unit_fn.rs:62:5 + | +62 | x.field.map(|value| { { diverge(value + captured); } }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(value) = x.field { diverge(value + captured); }` + +error: called `map(f)` on an Result value where `f` is a unit closure + --> $DIR/result_map_unit_fn.rs:67:5 + | +67 | x.field.map(|value| { let y = plus_one(value + captured); }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(value) = x.field { let y = plus_one(value + captured); }` + +error: called `map(f)` on an Result value where `f` is a unit closure + --> $DIR/result_map_unit_fn.rs:69:5 + | +69 | x.field.map(|value| { plus_one(value + captured); }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(value) = x.field { plus_one(value + captured); }` + +error: called `map(f)` on an Result value where `f` is a unit closure + --> $DIR/result_map_unit_fn.rs:71:5 + | +71 | x.field.map(|value| { { plus_one(value + captured); } }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(value) = x.field { plus_one(value + captured); }` + +error: called `map(f)` on an Result value where `f` is a unit closure + --> $DIR/result_map_unit_fn.rs:74:5 + | +74 | x.field.map(|ref value| { do_nothing(value + captured) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(ref value) = x.field { do_nothing(value + captured) }` + +error: called `map(f)` on an Result value where `f` is a unit closure + --> $DIR/result_map_unit_fn.rs:77:5 + | +77 | x.field.map(|value| { do_nothing(value); do_nothing(value) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(value) = x.field { ... }` + +error: called `map(f)` on an Result value where `f` is a unit closure + --> $DIR/result_map_unit_fn.rs:79:5 + | +79 | x.field.map(|value| if value > 0 { do_nothing(value); do_nothing(value) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(value) = x.field { ... }` + +error: called `map(f)` on an Result value where `f` is a unit closure + --> $DIR/result_map_unit_fn.rs:83:5 + | +83 | x.field.map(|value| { + | _____^ + | |_____| + | || +84 | || do_nothing(value); +85 | || do_nothing(value) +86 | || }); + | ||______^- help: try this: `if let Ok(value) = x.field { ... }` + | |_______| + | + +error: called `map(f)` on an Result value where `f` is a unit closure + --> $DIR/result_map_unit_fn.rs:87:5 + | +87 | x.field.map(|value| { do_nothing(value); do_nothing(value); }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(value) = x.field { ... }` + +error: called `map(f)` on an Result value where `f` is a unit function + --> $DIR/result_map_unit_fn.rs:91:5 + | +91 | "12".parse::().map(diverge); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(_) = "12".parse::() { diverge(...) }` + +error: called `map(f)` on an Result value where `f` is a unit function + --> $DIR/result_map_unit_fn.rs:97:5 + | +97 | y.map(do_nothing); + | ^^^^^^^^^^^^^^^^^- + | | + | help: try this: `if let Ok(_y) = y { do_nothing(...) }` + +error: aborting due to 23 previous errors + From 26b9911e079cc1e8fa076ff704ab4785f02aa53d Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sun, 15 Apr 2018 15:37:11 +0200 Subject: [PATCH 17/17] Refactor out enum and address nits --- clippy_lints/src/map_unit_fn.rs | 52 +++++++++++++-------------------- 1 file changed, 20 insertions(+), 32 deletions(-) diff --git a/clippy_lints/src/map_unit_fn.rs b/clippy_lints/src/map_unit_fn.rs index c3fc19699ea9..7defccf4697c 100644 --- a/clippy_lints/src/map_unit_fn.rs +++ b/clippy_lints/src/map_unit_fn.rs @@ -8,11 +8,11 @@ use utils::paths; #[derive(Clone)] pub struct Pass; -/// **What it does:** Checks for usage of `Option.map(f)` where f is a function +/// **What it does:** Checks for usage of `option.map(f)` where f is a function /// or closure that returns the unit type. /// /// **Why is this bad?** Readability, this can be written more clearly with -/// an if statement +/// an if let statement /// /// **Known problems:** None. /// @@ -38,14 +38,14 @@ pub struct Pass; declare_clippy_lint! { pub OPTION_MAP_UNIT_FN, complexity, - "using `Option.map(f)`, where f is a function or closure that returns ()" + "using `option.map(f)`, where f is a function or closure that returns ()" } -/// **What it does:** Checks for usage of `Result.map(f)` where f is a function +/// **What it does:** Checks for usage of `result.map(f)` where f is a function /// or closure that returns the unit type. /// /// **Why is this bad?** Readability, this can be written more clearly with -/// an if statement +/// an if let statement /// /// **Known problems:** None. /// @@ -71,7 +71,7 @@ declare_clippy_lint! { declare_clippy_lint! { pub RESULT_MAP_UNIT_FN, complexity, - "using `Result.map(f)`, where f is a function or closure that returns ()" + "using `result.map(f)`, where f is a function or closure that returns ()" } @@ -215,33 +215,21 @@ fn lint_map_unit_fn(cx: &LateContext, stmt: &hir::Stmt, expr: &hir::Expr, map_ar } else if let Some((binding, closure_expr)) = unit_closure(cx, fn_arg) { let msg = suggestion_msg("closure", map_type); - enum Suggestion { - Full(String), - Approx(String) - } - - let suggestion = if let Some(expr_span) = reduce_unit_expression(cx, closure_expr) { - Suggestion::Full( - format!("if let {0}({1}) = {2} {{ {3} }}", - variant, - snippet(cx, binding.pat.span, "_"), - snippet(cx, var_arg.span, "_"), - snippet(cx, expr_span, "_")) - ) - } else { - Suggestion::Approx( - format!("if let {0}({1}) = {2} {{ ... }}", - variant, - snippet(cx, binding.pat.span, "_"), - snippet(cx, var_arg.span, "_")) - ) - }; - span_lint_and_then(cx, lint, expr.span, &msg, |db| { - match suggestion { - Suggestion::Full(sugg) => db.span_suggestion(stmt.span, "try this", sugg), - Suggestion::Approx(sugg) => db.span_approximate_suggestion(stmt.span, "try this", sugg), - }; + if let Some(reduced_expr_span) = reduce_unit_expression(cx, closure_expr) { + let suggestion = format!("if let {0}({1}) = {2} {{ {3} }}", + variant, + snippet(cx, binding.pat.span, "_"), + snippet(cx, var_arg.span, "_"), + snippet(cx, reduced_expr_span, "_")); + db.span_suggestion(stmt.span, "try this", suggestion); + } else { + let suggestion = format!("if let {0}({1}) = {2} {{ ... }}", + variant, + snippet(cx, binding.pat.span, "_"), + snippet(cx, var_arg.span, "_")); + db.span_approximate_suggestion(stmt.span, "try this", suggestion); + } }); } }