From 4f4e20c561b223027e47183e58c4ec17f406d809 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sun, 15 Apr 2018 13:00:12 +0200 Subject: [PATCH] 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 0065ae1a0..f66c00868 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 afad923a3..ea8478c37 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 97ff11419..c3fc19699 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 000000000..8f3c15799 --- /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 000000000..199f5e7cf --- /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 +