diff --git a/CHANGELOG.md b/CHANGELOG.md index 8995604e1bf5..1d69de699968 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -710,6 +710,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 +[`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 7e0a692c2da9..bdbdff92d3c3 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_unit_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_unit_fn::Pass); reg.register_lint_group("clippy_restriction", vec![ @@ -441,6 +443,8 @@ 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_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 new file mode 100644 index 000000000000..7defccf4697c --- /dev/null +++ b/clippy_lints/src/map_unit_fn.rs @@ -0,0 +1,251 @@ +use rustc::hir; +use rustc::lint::*; +use rustc::ty; +use syntax::codemap::Span; +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 function +/// or closure that returns the unit type. +/// +/// **Why is this bad?** Readability, this can be written more clearly with +/// an if let statement +/// +/// **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 { +/// log_err_msg(msg) +/// } +/// if let Some(msg) = x { +/// log_err_msg(format_msg(msg)) +/// } +/// ``` +declare_clippy_lint! { + pub OPTION_MAP_UNIT_FN, + complexity, + "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 let 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, RESULT_MAP_UNIT_FN) + } +} + +fn is_unit_type(ty: ty::Ty) -> bool { + match ty.sty { + ty::TyTuple(slice) => slice.is_empty(), + ty::TyNever => true, + _ => false, + } +} + +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_unit_type(fn_type.output()); + } + } + false +} + +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_unit_expression<'a>(cx: &LateContext, expr: &'a hir::Expr) -> Option { + if !is_unit_expression(cx, expr) { + return None; + } + + match expr.node { + hir::ExprCall(_, _) | + hir::ExprMethodCall(_, _, _) => { + // Calls can't be reduced any more + Some(expr.span) + }, + hir::ExprBlock(ref block) => { + match (&block.stmts[..], block.expr.as_ref()) { + (&[], Some(inner_expr)) => { + // If block only contains an expression, + // reduce `{ X }` to `X` + reduce_unit_expression(cx, inner_expr) + }, + (&[ref inner_stmt], None) => { + // 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), + } + }, + _ => { + // 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, + } +} + +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_unit_expression(cx, body_expr); + if let Some(binding) = iter_input_pats(&decl, body).next(); + then { + return Some((binding, body_expr)); + } + } + } + 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 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]; + + 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 = 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, 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 = suggestion_msg("closure", map_type); + + span_lint_and_then(cx, lint, expr.span, &msg, |db| { + 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); + } + }); + } +} + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { + fn check_stmt(&mut self, cx: &LateContext, stmt: &hir::Stmt) { + if in_macro(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_unit_fn(cx, stmt, expr, arglists[0]); + } + } + } + } +} diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index e9b6865f0c99..797a871d72b8 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -432,16 +432,16 @@ 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 let Some(seg) = path.segments.last() { 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/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() { 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 new file mode 100644 index 000000000000..06531e290326 --- /dev/null +++ b/tests/ui/option_map_unit_fn.rs @@ -0,0 +1,100 @@ +#![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 + } +} + +fn option_map_unit_fn() { + 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) }); + + // 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 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); +} + +fn main() { +} diff --git a/tests/ui/option_map_unit_fn.stderr b/tests/ui/option_map_unit_fn.stderr new file mode 100644 index 000000000000..3ca57a65b3f3 --- /dev/null +++ b/tests/ui/option_map_unit_fn.stderr @@ -0,0 +1,210 @@ +error: called `map(f)` on an Option value where `f` is a unit function + --> $DIR/option_map_unit_fn.rs:32:5 + | +32 | 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/option_map_unit_fn.rs:34:5 + | +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:36:5 + | +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:42:5 + | +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:44:5 + | +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:47:5 + | +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:49:5 + | +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:51:5 + | +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:53:5 + | +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:56:5 + | +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:58:5 + | +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:60:5 + | +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:62:5 + | +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:67:5 + | +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:69:5 + | +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:71:5 + | +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:74:5 + | +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:77:5 + | +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:79:5 + | +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:83:5 + | +83 | x.field.map(|value| { + | _____^ + | |_____| + | || +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:87:5 + | +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:90:5 + | +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:91:5 + | +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:92:5 + | +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:96:5 + | +96 | 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/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 +