From 91d8a804d34b44a414b02ea5eba5305573748fff Mon Sep 17 00:00:00 2001 From: Nick Torres Date: Fri, 3 Apr 2020 23:59:52 -0700 Subject: [PATCH 1/9] result_map_or_into_option: add lint to catch manually adpating Result -> Option Result has an `ok()` method that adapts a Result into an Option. It's possible to get around this adapter by writing Result.map_or(None, Some). This lint is implemented as a new variant of the existing [`option_map_none` lint](https://github.com/rust-lang/rust-clippy/pull/2128) --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 3 + clippy_lints/src/methods/mod.rs | 105 ++++++++++++++++------ src/lintlist/mod.rs | 7 ++ tests/ui/result_map_or_into_option.fixed | 16 ++++ tests/ui/result_map_or_into_option.rs | 14 +++ tests/ui/result_map_or_into_option.stderr | 10 +++ 7 files changed, 131 insertions(+), 25 deletions(-) create mode 100644 tests/ui/result_map_or_into_option.fixed create mode 100644 tests/ui/result_map_or_into_option.rs create mode 100644 tests/ui/result_map_or_into_option.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 894aab21fb37..b7ac3cace204 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1448,6 +1448,7 @@ Released 2018-09-13 [`replace_consts`]: https://rust-lang.github.io/rust-clippy/master/index.html#replace_consts [`rest_pat_in_fully_bound_structs`]: https://rust-lang.github.io/rust-clippy/master/index.html#rest_pat_in_fully_bound_structs [`result_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_expect_used +[`result_map_or_into_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_or_into_option [`result_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unit_fn [`result_map_unwrap_or_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unwrap_or_else [`result_unwrap_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_unwrap_used diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index dfc2a26b06b2..83dcb350e185 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -666,6 +666,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &methods::OPTION_UNWRAP_USED, &methods::OR_FUN_CALL, &methods::RESULT_EXPECT_USED, + &methods::RESULT_MAP_OR_INTO_OPTION, &methods::RESULT_MAP_UNWRAP_OR_ELSE, &methods::RESULT_UNWRAP_USED, &methods::SEARCH_IS_SOME, @@ -1273,6 +1274,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&methods::OPTION_AS_REF_DEREF), LintId::of(&methods::OPTION_MAP_OR_NONE), LintId::of(&methods::OR_FUN_CALL), + LintId::of(&methods::RESULT_MAP_OR_INTO_OPTION), LintId::of(&methods::SEARCH_IS_SOME), LintId::of(&methods::SHOULD_IMPLEMENT_TRAIT), LintId::of(&methods::SINGLE_CHAR_PATTERN), @@ -1453,6 +1455,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&methods::NEW_RET_NO_SELF), LintId::of(&methods::OK_EXPECT), LintId::of(&methods::OPTION_MAP_OR_NONE), + LintId::of(&methods::RESULT_MAP_OR_INTO_OPTION), LintId::of(&methods::SHOULD_IMPLEMENT_TRAIT), LintId::of(&methods::STRING_EXTEND_CHARS), LintId::of(&methods::UNNECESSARY_FOLD), diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 527508af8a31..e8d642ed71e0 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -330,6 +330,24 @@ declare_clippy_lint! { "using `Option.map_or(None, f)`, which is more succinctly expressed as `and_then(f)`" } +declare_clippy_lint! { + /// **What it does:** Checks for usage of `_.map_or(None, Some)`. + /// + /// **Why is this bad?** Readability, this can be written more concisely as + /// `_.ok()`. + /// + /// **Known problems:** + /// + /// **Example:** + /// ```rust + /// # let opt = Some(1); + /// # let r = opt.map_or(None, Some); + /// ``` + pub RESULT_MAP_OR_INTO_OPTION, + style, + "using `Result.map_or(None, Some)`, which is more succinctly expressed as `ok()`" +} + declare_clippy_lint! { /// **What it does:** Checks for usage of `_.and_then(|x| Some(y))`. /// @@ -1248,6 +1266,7 @@ declare_lint_pass!(Methods => [ OPTION_MAP_UNWRAP_OR, OPTION_MAP_UNWRAP_OR_ELSE, RESULT_MAP_UNWRAP_OR_ELSE, + RESULT_MAP_OR_INTO_OPTION, OPTION_MAP_OR_NONE, OPTION_AND_THEN_SOME, OR_FUN_CALL, @@ -2517,37 +2536,73 @@ fn lint_map_unwrap_or_else<'a, 'tcx>( } } -/// lint use of `_.map_or(None, _)` for `Option`s +/// lint use of `_.map_or(None, _)` for `Option`s and `Result`s fn lint_map_or_none<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr<'_>, map_or_args: &'tcx [hir::Expr<'_>], ) { - if match_type(cx, cx.tables.expr_ty(&map_or_args[0]), &paths::OPTION) { - // check if the first non-self argument to map_or() is None - let map_or_arg_is_none = if let hir::ExprKind::Path(ref qpath) = map_or_args[1].kind { - match_qpath(qpath, &paths::OPTION_NONE) - } else { - false - }; + let is_option = match_type(cx, cx.tables.expr_ty(&map_or_args[0]), &paths::OPTION); + let is_result = match_type(cx, cx.tables.expr_ty(&map_or_args[0]), &paths::RESULT); + + // There are two variants of this `map_or` lint: + // (1) using `map_or` as an adapter from `Result` to `Option` + // (2) using `map_or` as a combinator instead of `and_then` + // + // (For this lint) we don't care if any other type calls `map_or` + if !is_option && !is_result { + return; + } - if map_or_arg_is_none { - // lint message - let msg = "called `map_or(None, f)` on an `Option` value. This can be done more directly by calling \ - `and_then(f)` instead"; - let map_or_self_snippet = snippet(cx, map_or_args[0].span, ".."); - let map_or_func_snippet = snippet(cx, map_or_args[2].span, ".."); - let hint = format!("{0}.and_then({1})", map_or_self_snippet, map_or_func_snippet); - span_lint_and_sugg( - cx, - OPTION_MAP_OR_NONE, - expr.span, - msg, - "try using `and_then` instead", - hint, - Applicability::MachineApplicable, - ); - } + let default_arg_is_none = if let hir::ExprKind::Path(ref qpath) = map_or_args[1].kind { + match_qpath(qpath, &paths::OPTION_NONE) + } else { + false + }; + + // This is really only needed if `is_result` holds. Computing it here + // makes `mess`'s assignment a bit easier, so just compute it here. + let f_arg_is_some = if let hir::ExprKind::Path(ref qpath) = map_or_args[2].kind { + match_qpath(qpath, &paths::OPTION_SOME) + } else { + false + }; + + let mess = if is_option && default_arg_is_none { + let self_snippet = snippet(cx, map_or_args[0].span, ".."); + let func_snippet = snippet(cx, map_or_args[2].span, ".."); + let msg = "called `map_or(None, f)` on an `Option` value. This can be done more directly by calling \ + `and_then(f)` instead"; + Some(( + OPTION_MAP_OR_NONE, + msg, + "try using `and_then` instead", + format!("{0}.and_then({1})", self_snippet, func_snippet), + )) + } else if is_result && 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, map_or_args[0].span, ".."); + Some(( + RESULT_MAP_OR_INTO_OPTION, + msg, + "try using `ok` instead", + format!("{0}.ok()", self_snippet), + )) + } else { + None + }; + + if let Some((lint, msg, instead, hint)) = mess { + span_lint_and_sugg( + cx, + lint, + expr.span, + msg, + instead, + hint, + Applicability::MachineApplicable, + ); } } diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 8a6d0af5f8a7..01d1d1a06723 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1823,6 +1823,13 @@ pub static ref ALL_LINTS: Vec = vec![ deprecation: None, module: "methods", }, + Lint { + name: "result_map_or_into_option", + group: "style", + desc: "using `Result.map_or(None, Some)`, which is more succinctly expressed as `ok()`", + deprecation: None, + module: "methods", + }, Lint { name: "result_map_unit_fn", group: "complexity", diff --git a/tests/ui/result_map_or_into_option.fixed b/tests/ui/result_map_or_into_option.fixed new file mode 100644 index 000000000000..948eb6a3aca1 --- /dev/null +++ b/tests/ui/result_map_or_into_option.fixed @@ -0,0 +1,16 @@ +// run-rustfix + +#![warn(clippy::result_map_or_into_option)] + +fn main() { + let opt: Result = Ok(1); + let _ = opt.ok(); + + let rewrap = |s: u32| -> Option { + Some(s) + }; + + // A non-Some `f` arg should not emit the lint + let opt: Result = Ok(1); + let _ = opt.map_or(None, rewrap); +} diff --git a/tests/ui/result_map_or_into_option.rs b/tests/ui/result_map_or_into_option.rs new file mode 100644 index 000000000000..d097c19e44b9 --- /dev/null +++ b/tests/ui/result_map_or_into_option.rs @@ -0,0 +1,14 @@ +// run-rustfix + +#![warn(clippy::result_map_or_into_option)] + +fn main() { + let opt: Result = Ok(1); + let _ = opt.map_or(None, Some); + + let rewrap = |s: u32| -> Option { Some(s) }; + + // A non-Some `f` arg should not emit the lint + let opt: Result = Ok(1); + let _ = opt.map_or(None, rewrap); +} diff --git a/tests/ui/result_map_or_into_option.stderr b/tests/ui/result_map_or_into_option.stderr new file mode 100644 index 000000000000..febf32147d13 --- /dev/null +++ b/tests/ui/result_map_or_into_option.stderr @@ -0,0 +1,10 @@ +error: called `map_or(None, Some)` on a `Result` value. This can be done more directly by calling `ok()` instead + --> $DIR/result_map_or_into_option.rs:7:13 + | +LL | let _ = opt.map_or(None, Some); + | ^^^^^^^^^^^^^^^^^^^^^^ help: try using `ok` instead: `opt.ok()` + | + = note: `-D clippy::result-map-or-into-option` implied by `-D warnings` + +error: aborting due to previous error + From 91759a7582a292c3f85204b1d61e7185c8543959 Mon Sep 17 00:00:00 2001 From: Nick Torres Date: Sat, 4 Apr 2020 13:42:24 -0700 Subject: [PATCH 2/9] result_map_or_into_option: explicitly note absence of known problems --- clippy_lints/src/methods/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index e8d642ed71e0..37bb3710fabc 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -336,7 +336,7 @@ declare_clippy_lint! { /// **Why is this bad?** Readability, this can be written more concisely as /// `_.ok()`. /// - /// **Known problems:** + /// **Known problems:** None. /// /// **Example:** /// ```rust From 3a29aedf8db3af19ee0f64dee6f00489812e6cb0 Mon Sep 17 00:00:00 2001 From: Nick Torres Date: Sat, 4 Apr 2020 13:44:11 -0700 Subject: [PATCH 3/9] result_map_or_into_option: add good and bad examples --- clippy_lints/src/methods/mod.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 37bb3710fabc..7c818232a553 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -340,8 +340,13 @@ declare_clippy_lint! { /// /// **Example:** /// ```rust - /// # let opt = Some(1); - /// # let r = opt.map_or(None, Some); + /// # Bad + /// let r: Result = Ok(1); + /// assert_eq!(Some(1), r.map_or(None, Some)); + /// + /// # Good + /// let r: Result = Ok(1); + /// assert_eq!(Some(1), r.ok()); /// ``` pub RESULT_MAP_OR_INTO_OPTION, style, From d0738bd673fe8e2b42d26b6d116f197f4aecea82 Mon Sep 17 00:00:00 2001 From: Nick Torres Date: Sat, 4 Apr 2020 13:53:08 -0700 Subject: [PATCH 4/9] result_map_or_into_option: destructure lint tuple or return early --- clippy_lints/src/methods/mod.rs | 74 +++++++++++++----------- tests/ui/result_map_or_into_option.fixed | 4 +- 2 files changed, 41 insertions(+), 37 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 7c818232a553..62fcd801bc30 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2573,42 +2573,48 @@ fn lint_map_or_none<'a, 'tcx>( false }; - let mess = if is_option && default_arg_is_none { - let self_snippet = snippet(cx, map_or_args[0].span, ".."); - let func_snippet = snippet(cx, map_or_args[2].span, ".."); - let msg = "called `map_or(None, f)` on an `Option` value. This can be done more directly by calling \ - `and_then(f)` instead"; - Some(( - OPTION_MAP_OR_NONE, - msg, - "try using `and_then` instead", - format!("{0}.and_then({1})", self_snippet, func_snippet), - )) - } else if is_result && 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, map_or_args[0].span, ".."); - Some(( - RESULT_MAP_OR_INTO_OPTION, - msg, - "try using `ok` instead", - format!("{0}.ok()", self_snippet), - )) - } else { - None + let (lint, msg, instead, hint) = { + if !default_arg_is_none { + // nothing to lint! + return; + } + + if is_option { + let self_snippet = snippet(cx, map_or_args[0].span, ".."); + let func_snippet = snippet(cx, map_or_args[2].span, ".."); + let msg = "called `map_or(None, f)` on an `Option` value. This can be done more directly by calling \ + `and_then(f)` 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, map_or_args[0].span, ".."); + ( + RESULT_MAP_OR_INTO_OPTION, + msg, + "try using `ok` instead", + format!("{0}.ok()", self_snippet), + ) + } else { + // nothing to lint! + return; + } }; - if let Some((lint, msg, instead, hint)) = mess { - span_lint_and_sugg( - cx, - lint, - expr.span, - msg, - instead, - hint, - Applicability::MachineApplicable, - ); - } + span_lint_and_sugg( + cx, + lint, + expr.span, + msg, + instead, + hint, + Applicability::MachineApplicable, + ); } /// Lint use of `_.and_then(|x| Some(y))` for `Option`s diff --git a/tests/ui/result_map_or_into_option.fixed b/tests/ui/result_map_or_into_option.fixed index 948eb6a3aca1..07daf19fa250 100644 --- a/tests/ui/result_map_or_into_option.fixed +++ b/tests/ui/result_map_or_into_option.fixed @@ -6,9 +6,7 @@ fn main() { let opt: Result = Ok(1); let _ = opt.ok(); - let rewrap = |s: u32| -> Option { - Some(s) - }; + let rewrap = |s: u32| -> Option { Some(s) }; // A non-Some `f` arg should not emit the lint let opt: Result = Ok(1); From fb276dc3fa1d644ce6de1849ff3b78d3e54f5d9e Mon Sep 17 00:00:00 2001 From: Nick Torres Date: Sat, 4 Apr 2020 14:02:45 -0700 Subject: [PATCH 5/9] result_map_or_into_option: add `opt.map_or(None, |_| Some(y))` test --- tests/ui/result_map_or_into_option.fixed | 5 +++++ tests/ui/result_map_or_into_option.rs | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/tests/ui/result_map_or_into_option.fixed b/tests/ui/result_map_or_into_option.fixed index 07daf19fa250..331531b5165f 100644 --- a/tests/ui/result_map_or_into_option.fixed +++ b/tests/ui/result_map_or_into_option.fixed @@ -11,4 +11,9 @@ fn main() { // A non-Some `f` arg should not emit the lint let opt: Result = Ok(1); let _ = opt.map_or(None, rewrap); + + // A non-Some `f` closure where the argument is not used as the + // return should not emit the lint + let opt: Result = Ok(1); + opt.map_or(None, |_x| Some(1)); } diff --git a/tests/ui/result_map_or_into_option.rs b/tests/ui/result_map_or_into_option.rs index d097c19e44b9..3058480e2ad3 100644 --- a/tests/ui/result_map_or_into_option.rs +++ b/tests/ui/result_map_or_into_option.rs @@ -11,4 +11,9 @@ fn main() { // A non-Some `f` arg should not emit the lint let opt: Result = Ok(1); let _ = opt.map_or(None, rewrap); + + // A non-Some `f` closure where the argument is not used as the + // return should not emit the lint + let opt: Result = Ok(1); + opt.map_or(None, |_x| Some(1)); } From acc3bc1ba27129f6fc5bda6d943046e9e2ad2d45 Mon Sep 17 00:00:00 2001 From: Nick Torres Date: Sat, 4 Apr 2020 14:24:22 -0700 Subject: [PATCH 6/9] result_map_or_into_option: move arg checks into tuple assignment --- clippy_lints/src/methods/mod.rs | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 62fcd801bc30..d57cce47ff9b 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2559,26 +2559,27 @@ fn lint_map_or_none<'a, 'tcx>( return; } - let default_arg_is_none = if let hir::ExprKind::Path(ref qpath) = map_or_args[1].kind { - match_qpath(qpath, &paths::OPTION_NONE) - } else { - false - }; - // This is really only needed if `is_result` holds. Computing it here - // makes `mess`'s assignment a bit easier, so just compute it here. - let f_arg_is_some = if let hir::ExprKind::Path(ref qpath) = map_or_args[2].kind { - match_qpath(qpath, &paths::OPTION_SOME) - } else { - false - }; let (lint, msg, instead, hint) = { + + let default_arg_is_none = if let hir::ExprKind::Path(ref qpath) = map_or_args[1].kind { + match_qpath(qpath, &paths::OPTION_NONE) + } else { + return; + }; + if !default_arg_is_none { // nothing to lint! return; } + let f_arg_is_some = if let hir::ExprKind::Path(ref qpath) = map_or_args[2].kind { + match_qpath(qpath, &paths::OPTION_SOME) + } else { + false + }; + if is_option { let self_snippet = snippet(cx, map_or_args[0].span, ".."); let func_snippet = snippet(cx, map_or_args[2].span, ".."); From 2533f56a0eaa4ebbeb607b6596d12e303f97e008 Mon Sep 17 00:00:00 2001 From: Nick Torres Date: Sat, 4 Apr 2020 14:33:43 -0700 Subject: [PATCH 7/9] result_map_or_into_option: fix `cargo dev fmt --check` errors --- clippy_lints/src/methods/mod.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index d57cce47ff9b..9ff669515cf8 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2559,10 +2559,7 @@ fn lint_map_or_none<'a, 'tcx>( return; } - - let (lint, msg, instead, hint) = { - let default_arg_is_none = if let hir::ExprKind::Path(ref qpath) = map_or_args[1].kind { match_qpath(qpath, &paths::OPTION_NONE) } else { From 325d0b69d2dffccecf48c5d7975ccd100141e1f5 Mon Sep 17 00:00:00 2001 From: Nick Torres Date: Sat, 4 Apr 2020 15:02:38 -0700 Subject: [PATCH 8/9] result_map_or_into: fix dogfood_clippy error => {h,l}int --- clippy_lints/src/methods/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 9ff669515cf8..7f4964da72e7 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2559,7 +2559,7 @@ fn lint_map_or_none<'a, 'tcx>( return; } - let (lint, msg, instead, hint) = { + let (lint_name, msg, instead, hint) = { let default_arg_is_none = if let hir::ExprKind::Path(ref qpath) = map_or_args[1].kind { match_qpath(qpath, &paths::OPTION_NONE) } else { @@ -2606,7 +2606,7 @@ fn lint_map_or_none<'a, 'tcx>( span_lint_and_sugg( cx, - lint, + lint_name, expr.span, msg, instead, From 5d54fbb7914cc1ea5b3bd8cdfd6f24dbacd8f649 Mon Sep 17 00:00:00 2001 From: Nick Torres Date: Sat, 4 Apr 2020 17:20:23 -0700 Subject: [PATCH 9/9] result_map_or_into_option: fix syntax error in example --- clippy_lints/src/methods/mod.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 7f4964da72e7..98d9426bd8a5 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -339,13 +339,16 @@ declare_clippy_lint! { /// **Known problems:** None. /// /// **Example:** + /// + /// Bad: /// ```rust - /// # Bad - /// let r: Result = Ok(1); + /// # let r: Result = Ok(1); /// assert_eq!(Some(1), r.map_or(None, Some)); + /// ``` /// - /// # Good - /// let r: Result = Ok(1); + /// Good: + /// ```rust + /// # let r: Result = Ok(1); /// assert_eq!(Some(1), r.ok()); /// ``` pub RESULT_MAP_OR_INTO_OPTION,