diff --git a/clippy_lints/src/methods/option_map_or_none.rs b/clippy_lints/src/methods/option_map_or_none.rs index e99b6b07d156..5e5c1038e829 100644 --- a/clippy_lints/src/methods/option_map_or_none.rs +++ b/clippy_lints/src/methods/option_map_or_none.rs @@ -1,7 +1,7 @@ use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::is_lang_ctor; use clippy_utils::source::snippet; use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::{is_lang_ctor, single_segment_path}; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_hir::LangItem::{OptionNone, OptionSome}; @@ -11,6 +11,28 @@ use rustc_span::symbol::sym; use super::OPTION_MAP_OR_NONE; use super::RESULT_MAP_OR_INTO_OPTION; +// The expression inside a closure may or may not have surrounding braces +// which causes problems when generating a suggestion. +fn reduce_unit_expression<'a>( + cx: &LateContext<'_>, + expr: &'a hir::Expr<'_>, +) -> Option<(&'a hir::Expr<'a>, &'a [hir::Expr<'a>])> { + match expr.kind { + hir::ExprKind::Call(func, arg_char) => Some((func, arg_char)), + hir::ExprKind::Block(block, _) => { + match (block.stmts, block.expr) { + (&[], Some(inner_expr)) => { + // If block only contains an expression, + // reduce `|x| { x + 1 }` to `|x| x + 1` + reduce_unit_expression(cx, inner_expr) + }, + _ => None, + } + }, + _ => None, + } +} + /// lint use of `_.map_or(None, _)` for `Option`s and `Result`s pub(super) fn check<'tcx>( cx: &LateContext<'tcx>, @@ -31,58 +53,75 @@ pub(super) fn check<'tcx>( return; } - let (lint_name, msg, instead, hint) = { - let default_arg_is_none = if let hir::ExprKind::Path(ref qpath) = def_arg.kind { - is_lang_ctor(cx, qpath, OptionNone) - } else { - return; - }; + let default_arg_is_none = if let hir::ExprKind::Path(ref qpath) = def_arg.kind { + is_lang_ctor(cx, qpath, OptionNone) + } else { + return; + }; - if !default_arg_is_none { - // nothing to lint! - return; - } + if !default_arg_is_none { + // nothing to lint! + return; + } - let f_arg_is_some = if let hir::ExprKind::Path(ref qpath) = map_arg.kind { - is_lang_ctor(cx, qpath, OptionSome) - } else { - false - }; + let f_arg_is_some = if let hir::ExprKind::Path(ref qpath) = map_arg.kind { + is_lang_ctor(cx, qpath, OptionSome) + } else { + false + }; + + if is_option { + let self_snippet = snippet(cx, recv.span, ".."); + if_chain! { + if let hir::ExprKind::Closure(_, _, id, span, _) = map_arg.kind; + let arg_snippet = snippet(cx, span, ".."); + let body = cx.tcx.hir().body(id); + if let Some((func, arg_char)) = reduce_unit_expression(cx, &body.value); + if arg_char.len() == 1; + if let hir::ExprKind::Path(ref qpath) = func.kind; + if let Some(segment) = single_segment_path(qpath); + if segment.ident.name == sym::Some; + then { + let func_snippet = snippet(cx, arg_char[0].span, ".."); + let msg = "called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling \ + `map(..)` instead"; + return span_lint_and_sugg( + cx, + OPTION_MAP_OR_NONE, + expr.span, + msg, + "try using `map` instead", + format!("{0}.map({1} {2})", self_snippet, arg_snippet,func_snippet), + Applicability::MachineApplicable, + ); + } - if is_option { - let self_snippet = snippet(cx, recv.span, ".."); - let func_snippet = snippet(cx, map_arg.span, ".."); - let msg = "called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling \ - `and_then(..)` instead"; - ( - OPTION_MAP_OR_NONE, - msg, - "try using `and_then` instead", - format!("{0}.and_then({1})", self_snippet, func_snippet), - ) - } else if f_arg_is_some { - let msg = "called `map_or(None, Some)` on a `Result` value. This can be done more directly by calling \ - `ok()` instead"; - let self_snippet = snippet(cx, recv.span, ".."); - ( - RESULT_MAP_OR_INTO_OPTION, - msg, - "try using `ok` instead", - format!("{0}.ok()", self_snippet), - ) - } else { - // nothing to lint! - return; } - }; - span_lint_and_sugg( - cx, - lint_name, - expr.span, - msg, - instead, - hint, - Applicability::MachineApplicable, - ); + let func_snippet = snippet(cx, map_arg.span, ".."); + let msg = "called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling \ + `and_then(..)` instead"; + return span_lint_and_sugg( + cx, + OPTION_MAP_OR_NONE, + expr.span, + msg, + "try using `and_then` instead", + format!("{0}.and_then({1})", self_snippet, func_snippet), + Applicability::MachineApplicable, + ); + } else if f_arg_is_some { + let msg = "called `map_or(None, Some)` on a `Result` value. This can be done more directly by calling \ + `ok()` instead"; + let self_snippet = snippet(cx, recv.span, ".."); + return span_lint_and_sugg( + cx, + RESULT_MAP_OR_INTO_OPTION, + expr.span, + msg, + "try using `ok` instead", + format!("{0}.ok()", self_snippet), + Applicability::MachineApplicable, + ); + } } diff --git a/tests/ui/option_map_or_none.fixed b/tests/ui/option_map_or_none.fixed index d80c3c7c1b72..177054f763b2 100644 --- a/tests/ui/option_map_or_none.fixed +++ b/tests/ui/option_map_or_none.fixed @@ -4,13 +4,19 @@ fn main() { let opt = Some(1); + let bar = |_| Some(1); // Check `OPTION_MAP_OR_NONE`. // Single line case. - let _ = opt.and_then(|x| Some(x + 1)); + let _: Option = opt.map(|x| x + 1); // Multi-line case. #[rustfmt::skip] - let _ = opt.and_then(|x| { - Some(x + 1) - }); + let _: Option = opt.map(|x| x + 1); + // function returning `Option` + let _: Option = opt.and_then(bar); + let _: Option = opt.and_then(|x| { + let offset = 0; + let height = x; + Some(offset + height) + }); } diff --git a/tests/ui/option_map_or_none.rs b/tests/ui/option_map_or_none.rs index 629842419e54..6908546d3250 100644 --- a/tests/ui/option_map_or_none.rs +++ b/tests/ui/option_map_or_none.rs @@ -4,13 +4,21 @@ fn main() { let opt = Some(1); + let bar = |_| Some(1); // Check `OPTION_MAP_OR_NONE`. // Single line case. - let _ = opt.map_or(None, |x| Some(x + 1)); + let _: Option = opt.map_or(None, |x| Some(x + 1)); // Multi-line case. #[rustfmt::skip] - let _ = opt.map_or(None, |x| { + let _: Option = opt.map_or(None, |x| { Some(x + 1) }); + // function returning `Option` + let _: Option = opt.map_or(None, bar); + let _: Option = opt.map_or(None, |x| { + let offset = 0; + let height = x; + Some(offset + height) + }); } diff --git a/tests/ui/option_map_or_none.stderr b/tests/ui/option_map_or_none.stderr index 27d68b85e6f8..11bdd887b6d3 100644 --- a/tests/ui/option_map_or_none.stderr +++ b/tests/ui/option_map_or_none.stderr @@ -1,26 +1,45 @@ -error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `and_then(..)` instead - --> $DIR/option_map_or_none.rs:10:13 +error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `map(..)` instead + --> $DIR/option_map_or_none.rs:11:26 | -LL | let _ = opt.map_or(None, |x| Some(x + 1)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `opt.and_then(|x| Some(x + 1))` +LL | let _: Option = opt.map_or(None, |x| Some(x + 1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `map` instead: `opt.map(|x| x + 1)` | = note: `-D clippy::option-map-or-none` implied by `-D warnings` -error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `and_then(..)` instead - --> $DIR/option_map_or_none.rs:13:13 +error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `map(..)` instead + --> $DIR/option_map_or_none.rs:14:26 | -LL | let _ = opt.map_or(None, |x| { - | _____________^ +LL | let _: Option = opt.map_or(None, |x| { + | __________________________^ LL | | Some(x + 1) LL | | }); - | |_________________________^ + | |_________________________^ help: try using `map` instead: `opt.map(|x| x + 1)` + +error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `and_then(..)` instead + --> $DIR/option_map_or_none.rs:18:26 + | +LL | let _: Option = opt.map_or(None, bar); + | ^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `opt.and_then(bar)` + +error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `and_then(..)` instead + --> $DIR/option_map_or_none.rs:19:26 + | +LL | let _: Option = opt.map_or(None, |x| { + | __________________________^ +LL | | let offset = 0; +LL | | let height = x; +LL | | Some(offset + height) +LL | | }); + | |______^ | help: try using `and_then` instead | -LL ~ let _ = opt.and_then(|x| { -LL + Some(x + 1) -LL ~ }); +LL ~ let _: Option = opt.and_then(|x| { +LL + let offset = 0; +LL + let height = x; +LL + Some(offset + height) +LL ~ }); | -error: aborting due to 2 previous errors +error: aborting due to 4 previous errors