From 2ed4a8a3e6fc8d53225ded631e815a767859184e Mon Sep 17 00:00:00 2001 From: togami2864 Date: Sat, 13 Nov 2021 22:38:28 +0900 Subject: [PATCH 01/11] fix suggestion in option_map_or_none --- clippy_lints/src/methods/option_map_or_none.rs | 6 +++--- tests/ui/option_map_or_none.fixed | 4 ++-- tests/ui/option_map_or_none.stderr | 10 +++++----- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/methods/option_map_or_none.rs b/clippy_lints/src/methods/option_map_or_none.rs index e99b6b07d156..8f61bd97b45c 100644 --- a/clippy_lints/src/methods/option_map_or_none.rs +++ b/clippy_lints/src/methods/option_map_or_none.rs @@ -53,12 +53,12 @@ pub(super) fn check<'tcx>( 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"; + `map(..)` instead"; ( OPTION_MAP_OR_NONE, msg, - "try using `and_then` instead", - format!("{0}.and_then({1})", self_snippet, func_snippet), + "try using `map` instead", + format!("{0}.map({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 \ diff --git a/tests/ui/option_map_or_none.fixed b/tests/ui/option_map_or_none.fixed index d80c3c7c1b72..429ef00003f5 100644 --- a/tests/ui/option_map_or_none.fixed +++ b/tests/ui/option_map_or_none.fixed @@ -7,10 +7,10 @@ fn main() { // Check `OPTION_MAP_OR_NONE`. // Single line case. - let _ = opt.and_then(|x| Some(x + 1)); + let _ = opt.map(|x| Some(x + 1)); // Multi-line case. #[rustfmt::skip] - let _ = opt.and_then(|x| { + let _ = opt.map(|x| { Some(x + 1) }); } diff --git a/tests/ui/option_map_or_none.stderr b/tests/ui/option_map_or_none.stderr index 27d68b85e6f8..b011515934d6 100644 --- a/tests/ui/option_map_or_none.stderr +++ b/tests/ui/option_map_or_none.stderr @@ -1,12 +1,12 @@ -error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `and_then(..)` instead +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:10:13 | LL | let _ = opt.map_or(None, |x| Some(x + 1)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `opt.and_then(|x| Some(x + 1))` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `map` instead: `opt.map(|x| Some(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 +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:13:13 | LL | let _ = opt.map_or(None, |x| { @@ -15,9 +15,9 @@ LL | | Some(x + 1) LL | | }); | |_________________________^ | -help: try using `and_then` instead +help: try using `map` instead | -LL ~ let _ = opt.and_then(|x| { +LL ~ let _ = opt.map(|x| { LL + Some(x + 1) LL ~ }); | From 4f71ff3f847e7da299be5cc3ba00ebfda5899166 Mon Sep 17 00:00:00 2001 From: togami2864 Date: Mon, 15 Nov 2021 15:47:57 +0900 Subject: [PATCH 02/11] add test case for option_map_or_none --- tests/ui/option_map_or_none.fixed | 11 ++++++++--- tests/ui/option_map_or_none.rs | 9 +++++++-- tests/ui/option_map_or_none.stderr | 18 ++++++++++++------ 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/tests/ui/option_map_or_none.fixed b/tests/ui/option_map_or_none.fixed index 429ef00003f5..77ad2d5285a4 100644 --- a/tests/ui/option_map_or_none.fixed +++ b/tests/ui/option_map_or_none.fixed @@ -4,13 +4,18 @@ fn main() { let opt = Some(1); + let bar = |_| { + Some(1) + }; // Check `OPTION_MAP_OR_NONE`. // Single line case. - let _ = opt.map(|x| Some(x + 1)); + let _ :Option = opt.map(|x| x + 1); // Multi-line case. #[rustfmt::skip] - let _ = opt.map(|x| { - Some(x + 1) + let _ :Option = opt.map(|x| { + x + 1 }); + // function returning `Option` + let _ :Option = opt.and_then(bar); } diff --git a/tests/ui/option_map_or_none.rs b/tests/ui/option_map_or_none.rs index 629842419e54..789531b72158 100644 --- a/tests/ui/option_map_or_none.rs +++ b/tests/ui/option_map_or_none.rs @@ -4,13 +4,18 @@ 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); } diff --git a/tests/ui/option_map_or_none.stderr b/tests/ui/option_map_or_none.stderr index b011515934d6..86ac93b9c80f 100644 --- a/tests/ui/option_map_or_none.stderr +++ b/tests/ui/option_map_or_none.stderr @@ -1,15 +1,15 @@ 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:10:13 | -LL | let _ = opt.map_or(None, |x| Some(x + 1)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `map` instead: `opt.map(|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 `map(..)` instead --> $DIR/option_map_or_none.rs:13:13 | -LL | let _ = opt.map_or(None, |x| { +LL | let _ :Option = opt.map_or(None, |x| { | _____________^ LL | | Some(x + 1) LL | | }); @@ -17,10 +17,16 @@ LL | | }); | help: try using `map` instead | -LL ~ let _ = opt.map(|x| { -LL + Some(x + 1) +LL ~ let _ :Option = opt.map(|x| { +LL + x + 1 LL ~ }); | -error: aborting due to 2 previous errors +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:20:26 + | +LL | let _ :Option = opt.map_or(None, bar); + | ^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `opt.and_then(bar)` + +error: aborting due to 3 previous errors From bbffe825ed12231e333bd641eb46aa477cde3b8f Mon Sep 17 00:00:00 2001 From: togami2864 Date: Tue, 16 Nov 2021 02:06:31 +0900 Subject: [PATCH 03/11] add reduce_unit_expression --- .../src/methods/option_map_or_none.rs | 138 +++++++++++------- tests/ui/option_map_or_none.fixed | 6 +- tests/ui/option_map_or_none.stderr | 10 +- 3 files changed, 96 insertions(+), 58 deletions(-) diff --git a/clippy_lints/src/methods/option_map_or_none.rs b/clippy_lints/src/methods/option_map_or_none.rs index 8f61bd97b45c..00a4dda9701d 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,26 @@ use rustc_span::symbol::sym; use super::OPTION_MAP_OR_NONE; use super::RESULT_MAP_OR_INTO_OPTION; +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.expr { + Some(inner_expr) => { + // If block only contains an expression, + // reduce `{ X }` to `X` + 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 +51,78 @@ 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 (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; + }; - 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, ".."); - 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 \ + if is_option { + let self_snippet = snippet(cx, recv.span, ".."); + if let hir::ExprKind::Closure(_, _, id, _, _) = map_arg.kind { + if_chain! { + 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"; - ( - OPTION_MAP_OR_NONE, - msg, - "try using `map` instead", - format!("{0}.map({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; + return span_lint_and_sugg( + cx, + OPTION_MAP_OR_NONE, + expr.span, + msg, + "try using `map` instead", + format!("{0}.map({1})", self_snippet, func_snippet), + Applicability::MachineApplicable, + ); + } + } } - }; - 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"; + 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, ".."); + span_lint_and_sugg( + cx, + RESULT_MAP_OR_INTO_OPTION, + expr.span, + msg, + "try using `ok` instead", + format!("{0}.ok()", self_snippet), + Applicability::MachineApplicable, + ) + } else { + // nothing to lint! + return; + } } diff --git a/tests/ui/option_map_or_none.fixed b/tests/ui/option_map_or_none.fixed index 77ad2d5285a4..9333cb89c9a0 100644 --- a/tests/ui/option_map_or_none.fixed +++ b/tests/ui/option_map_or_none.fixed @@ -5,7 +5,7 @@ fn main() { let opt = Some(1); let bar = |_| { - Some(1) + Some(1) }; // Check `OPTION_MAP_OR_NONE`. @@ -13,9 +13,7 @@ fn main() { let _ :Option = opt.map(|x| x + 1); // Multi-line case. #[rustfmt::skip] - let _ :Option = opt.map(|x| { - x + 1 - }); + let _ :Option = opt.map(|x| x + 1); // function returning `Option` let _ :Option = opt.and_then(bar); } diff --git a/tests/ui/option_map_or_none.stderr b/tests/ui/option_map_or_none.stderr index 86ac93b9c80f..3d75f2776f6d 100644 --- a/tests/ui/option_map_or_none.stderr +++ b/tests/ui/option_map_or_none.stderr @@ -1,16 +1,16 @@ 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:10:13 + --> $DIR/option_map_or_none.rs:13:26 | LL | let _ :Option = opt.map_or(None, |x| Some(x + 1)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `map` instead: `opt.map(|x| 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 `map(..)` instead - --> $DIR/option_map_or_none.rs:13:13 + --> $DIR/option_map_or_none.rs:16:26 | LL | let _ :Option = opt.map_or(None, |x| { - | _____________^ + | __________________________^ LL | | Some(x + 1) LL | | }); | |_________________________^ @@ -22,7 +22,7 @@ LL + x + 1 LL ~ }); | -error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `map(..)` instead +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:20:26 | LL | let _ :Option = opt.map_or(None, bar); From 02e0726149a8fd9963b2b81f8c15587520445cc6 Mon Sep 17 00:00:00 2001 From: togami2864 Date: Tue, 16 Nov 2021 02:29:02 +0900 Subject: [PATCH 04/11] fix a bug that the closure arguments are not displayed --- .../src/methods/option_map_or_none.rs | 19 ++++++++----------- tests/ui/option_map_or_none.stderr | 9 +-------- 2 files changed, 9 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/methods/option_map_or_none.rs b/clippy_lints/src/methods/option_map_or_none.rs index 00a4dda9701d..a18db4d93714 100644 --- a/clippy_lints/src/methods/option_map_or_none.rs +++ b/clippy_lints/src/methods/option_map_or_none.rs @@ -21,7 +21,7 @@ fn reduce_unit_expression<'a>( match block.expr { Some(inner_expr) => { // If block only contains an expression, - // reduce `{ X }` to `X` + // reduce `|x| { x + 1 }` to `|x| x + 1` reduce_unit_expression(cx, inner_expr) }, _ => None, @@ -51,7 +51,6 @@ 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 { @@ -71,7 +70,8 @@ pub(super) fn check<'tcx>( if is_option { let self_snippet = snippet(cx, recv.span, ".."); - if let hir::ExprKind::Closure(_, _, id, _, _) = map_arg.kind { + if let hir::ExprKind::Closure(_, _, id, span, _) = map_arg.kind { + let arg_snippet = snippet(cx, span, ".."); if_chain! { let body = cx.tcx.hir().body(id); if let Some((func, arg_char)) = reduce_unit_expression(cx, &body.value); @@ -89,7 +89,7 @@ pub(super) fn check<'tcx>( expr.span, msg, "try using `map` instead", - format!("{0}.map({1})", self_snippet, func_snippet), + format!("{0}.map({1} {2})", self_snippet, arg_snippet,func_snippet), Applicability::MachineApplicable, ); } @@ -99,7 +99,7 @@ pub(super) fn check<'tcx>( 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"; - span_lint_and_sugg( + return span_lint_and_sugg( cx, OPTION_MAP_OR_NONE, expr.span, @@ -107,12 +107,12 @@ pub(super) fn check<'tcx>( "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, ".."); - span_lint_and_sugg( + return span_lint_and_sugg( cx, RESULT_MAP_OR_INTO_OPTION, expr.span, @@ -120,9 +120,6 @@ pub(super) fn check<'tcx>( "try using `ok` instead", format!("{0}.ok()", self_snippet), Applicability::MachineApplicable, - ) - } else { - // nothing to lint! - return; + ); } } diff --git a/tests/ui/option_map_or_none.stderr b/tests/ui/option_map_or_none.stderr index 3d75f2776f6d..560b767fba2a 100644 --- a/tests/ui/option_map_or_none.stderr +++ b/tests/ui/option_map_or_none.stderr @@ -13,14 +13,7 @@ LL | let _ :Option = opt.map_or(None, |x| { | __________________________^ LL | | Some(x + 1) LL | | }); - | |_________________________^ - | -help: try using `map` instead - | -LL ~ let _ :Option = opt.map(|x| { -LL + 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:20:26 From cd5781648a93ca851459c8397c2bb1dfa3dd393b Mon Sep 17 00:00:00 2001 From: togami2864 Date: Tue, 16 Nov 2021 02:30:54 +0900 Subject: [PATCH 05/11] add comment --- clippy_lints/src/methods/option_map_or_none.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/clippy_lints/src/methods/option_map_or_none.rs b/clippy_lints/src/methods/option_map_or_none.rs index a18db4d93714..ac2a5b70004a 100644 --- a/clippy_lints/src/methods/option_map_or_none.rs +++ b/clippy_lints/src/methods/option_map_or_none.rs @@ -11,6 +11,7 @@ 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 fn reduce_unit_expression<'a>( cx: &LateContext<'_>, expr: &'a hir::Expr<'_>, From 300282c09aa8e922699b4fc7e2c608b4f8d81719 Mon Sep 17 00:00:00 2001 From: togami2864 Date: Tue, 16 Nov 2021 02:53:35 +0900 Subject: [PATCH 06/11] fix fmt --- clippy_lints/src/methods/option_map_or_none.rs | 3 ++- tests/ui/option_map_or_none.fixed | 10 ++++------ tests/ui/option_map_or_none.rs | 10 ++++------ tests/ui/option_map_or_none.stderr | 12 ++++++------ 4 files changed, 16 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/methods/option_map_or_none.rs b/clippy_lints/src/methods/option_map_or_none.rs index ac2a5b70004a..3a93d912e8c7 100644 --- a/clippy_lints/src/methods/option_map_or_none.rs +++ b/clippy_lints/src/methods/option_map_or_none.rs @@ -11,7 +11,8 @@ 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 +// 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<'_>, diff --git a/tests/ui/option_map_or_none.fixed b/tests/ui/option_map_or_none.fixed index 9333cb89c9a0..abaf88be8cae 100644 --- a/tests/ui/option_map_or_none.fixed +++ b/tests/ui/option_map_or_none.fixed @@ -4,16 +4,14 @@ fn main() { let opt = Some(1); - let bar = |_| { - Some(1) - }; + let bar = |_| Some(1); // Check `OPTION_MAP_OR_NONE`. // Single line case. - let _ :Option = opt.map(|x| x + 1); + let _: Option = opt.map(|x| x + 1); // Multi-line case. #[rustfmt::skip] - let _ :Option = opt.map(|x| x + 1); + let _: Option = opt.map(|x| x + 1); // function returning `Option` - let _ :Option = opt.and_then(bar); + let _: Option = opt.and_then(bar); } diff --git a/tests/ui/option_map_or_none.rs b/tests/ui/option_map_or_none.rs index 789531b72158..a63b63c4dc0d 100644 --- a/tests/ui/option_map_or_none.rs +++ b/tests/ui/option_map_or_none.rs @@ -4,18 +4,16 @@ fn main() { let opt = Some(1); - let bar = |_| { - Some(1) - }; + let bar = |_| Some(1); // Check `OPTION_MAP_OR_NONE`. // Single line case. - let _ :Option = opt.map_or(None, |x| Some(x + 1)); + let _: Option = opt.map_or(None, |x| Some(x + 1)); // Multi-line case. #[rustfmt::skip] - let _ :Option = 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, bar); } diff --git a/tests/ui/option_map_or_none.stderr b/tests/ui/option_map_or_none.stderr index 560b767fba2a..aec46cb005f4 100644 --- a/tests/ui/option_map_or_none.stderr +++ b/tests/ui/option_map_or_none.stderr @@ -1,24 +1,24 @@ 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:13:26 + --> $DIR/option_map_or_none.rs:11:26 | -LL | let _ :Option = opt.map_or(None, |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 `map(..)` instead - --> $DIR/option_map_or_none.rs:16:26 + --> $DIR/option_map_or_none.rs:14:26 | -LL | let _ :Option = 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:20:26 + --> $DIR/option_map_or_none.rs:18:26 | -LL | let _ :Option = opt.map_or(None, bar); +LL | let _: Option = opt.map_or(None, bar); | ^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `opt.and_then(bar)` error: aborting due to 3 previous errors From 0a30fdcd3c5ab9803983671a4452dff7e31f9e80 Mon Sep 17 00:00:00 2001 From: togami2864 Date: Tue, 16 Nov 2021 03:02:08 +0900 Subject: [PATCH 07/11] move the let statement out of the macro --- clippy_lints/src/methods/option_map_or_none.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/option_map_or_none.rs b/clippy_lints/src/methods/option_map_or_none.rs index 3a93d912e8c7..12ccaa24c3b0 100644 --- a/clippy_lints/src/methods/option_map_or_none.rs +++ b/clippy_lints/src/methods/option_map_or_none.rs @@ -74,8 +74,8 @@ pub(super) fn check<'tcx>( let self_snippet = snippet(cx, recv.span, ".."); if let hir::ExprKind::Closure(_, _, id, span, _) = map_arg.kind { let arg_snippet = snippet(cx, span, ".."); + let body = cx.tcx.hir().body(id); if_chain! { - 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; From 7605bac2cb72627e29d0a82a70d5496a5a7a1827 Mon Sep 17 00:00:00 2001 From: togami2864 Date: Tue, 16 Nov 2021 23:07:19 +0900 Subject: [PATCH 08/11] resolve CI --- clippy_lints/src/methods/option_map_or_none.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/methods/option_map_or_none.rs b/clippy_lints/src/methods/option_map_or_none.rs index 12ccaa24c3b0..b56528388925 100644 --- a/clippy_lints/src/methods/option_map_or_none.rs +++ b/clippy_lints/src/methods/option_map_or_none.rs @@ -72,10 +72,10 @@ pub(super) fn check<'tcx>( if is_option { let self_snippet = snippet(cx, recv.span, ".."); - if let hir::ExprKind::Closure(_, _, id, span, _) = map_arg.kind { + 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_chain! { 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; @@ -95,7 +95,7 @@ pub(super) fn check<'tcx>( Applicability::MachineApplicable, ); } - } + } let func_snippet = snippet(cx, map_arg.span, ".."); From e34927ecf66500858facab9b4ad0cdfa757318aa Mon Sep 17 00:00:00 2001 From: togami2864 Date: Thu, 18 Nov 2021 00:13:55 +0900 Subject: [PATCH 09/11] add multi-line test case --- tests/ui/option_map_or_none.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/ui/option_map_or_none.rs b/tests/ui/option_map_or_none.rs index a63b63c4dc0d..0d24d754820b 100644 --- a/tests/ui/option_map_or_none.rs +++ b/tests/ui/option_map_or_none.rs @@ -16,4 +16,9 @@ fn main() { }); // function returning `Option` let _: Option = opt.map_or(None, bar); + let _: Option = opt.map_or(None, |x| { + let offset = 0; + let height = 1; + Some(offset + height) + }); } From 006c4426577bf243f7218f951d0c9ada6b4fe737 Mon Sep 17 00:00:00 2001 From: togami2864 Date: Thu, 18 Nov 2021 00:36:11 +0900 Subject: [PATCH 10/11] check whether stmts is empty or not in block --- clippy_lints/src/methods/option_map_or_none.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/methods/option_map_or_none.rs b/clippy_lints/src/methods/option_map_or_none.rs index b56528388925..5e5c1038e829 100644 --- a/clippy_lints/src/methods/option_map_or_none.rs +++ b/clippy_lints/src/methods/option_map_or_none.rs @@ -20,8 +20,8 @@ fn reduce_unit_expression<'a>( match expr.kind { hir::ExprKind::Call(func, arg_char) => Some((func, arg_char)), hir::ExprKind::Block(block, _) => { - match block.expr { - Some(inner_expr) => { + 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) From 8e317f5283969aa775552e659d4219bb2641954a Mon Sep 17 00:00:00 2001 From: togami2864 Date: Thu, 18 Nov 2021 00:43:49 +0900 Subject: [PATCH 11/11] fix suggestion message --- tests/ui/option_map_or_none.fixed | 5 +++++ tests/ui/option_map_or_none.rs | 2 +- tests/ui/option_map_or_none.stderr | 22 +++++++++++++++++++++- 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/tests/ui/option_map_or_none.fixed b/tests/ui/option_map_or_none.fixed index abaf88be8cae..177054f763b2 100644 --- a/tests/ui/option_map_or_none.fixed +++ b/tests/ui/option_map_or_none.fixed @@ -14,4 +14,9 @@ fn main() { 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 0d24d754820b..6908546d3250 100644 --- a/tests/ui/option_map_or_none.rs +++ b/tests/ui/option_map_or_none.rs @@ -18,7 +18,7 @@ fn main() { let _: Option = opt.map_or(None, bar); let _: Option = opt.map_or(None, |x| { let offset = 0; - let height = 1; + 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 aec46cb005f4..11bdd887b6d3 100644 --- a/tests/ui/option_map_or_none.stderr +++ b/tests/ui/option_map_or_none.stderr @@ -21,5 +21,25 @@ error: called `map_or(None, ..)` on an `Option` value. This can be done more dir LL | let _: Option = opt.map_or(None, bar); | ^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `opt.and_then(bar)` -error: aborting due to 3 previous errors +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 _: Option = opt.and_then(|x| { +LL + let offset = 0; +LL + let height = x; +LL + Some(offset + height) +LL ~ }); + | + +error: aborting due to 4 previous errors