diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | clippy_lints/src/lib.rs | 3 | ||||
| -rw-r--r-- | clippy_lints/src/methods/mod.rs | 105 | ||||
| -rw-r--r-- | src/lintlist/mod.rs | 7 | ||||
| -rw-r--r-- | tests/ui/result_map_or_into_option.fixed | 16 | ||||
| -rw-r--r-- | tests/ui/result_map_or_into_option.rs | 14 | ||||
| -rw-r--r-- | tests/ui/result_map_or_into_option.stderr | 10 |
7 files changed, 131 insertions, 25 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 894aab21fb3..b7ac3cace20 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 dfc2a26b06b..83dcb350e18 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 527508af8a3..e8d642ed71e 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -331,6 +331,24 @@ declare_clippy_lint! { } 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))`. /// /// **Why is this bad?** Readability, this can be written more concisely as @@ -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<T,E>` to `Option<T>` + // (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 8a6d0af5f8a..01d1d1a0672 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1824,6 +1824,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![ 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", desc: "using `result.map(f)`, where `f` is a function or closure that returns `()`", 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 00000000000..948eb6a3aca --- /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<u32, &str> = Ok(1); + let _ = opt.ok(); + + let rewrap = |s: u32| -> Option<u32> { + Some(s) + }; + + // A non-Some `f` arg should not emit the lint + let opt: Result<u32, &str> = 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 00000000000..d097c19e44b --- /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<u32, &str> = Ok(1); + let _ = opt.map_or(None, Some); + + let rewrap = |s: u32| -> Option<u32> { Some(s) }; + + // A non-Some `f` arg should not emit the lint + let opt: Result<u32, &str> = 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 00000000000..febf32147d1 --- /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 + |