From ea733172e69844d772b4c9e80daf2745ac3355b1 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 24 Nov 2023 17:56:47 +0100 Subject: [PATCH 1/3] Create new lint `option_map_or_err_ok` --- clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/methods/mod.rs | 28 +++++++++++++ .../src/methods/option_map_or_err_ok.rs | 41 +++++++++++++++++++ 3 files changed, 70 insertions(+) create mode 100644 clippy_lints/src/methods/option_map_or_err_ok.rs diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 7d4ab9fde1f9..5a109fcc2ccd 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -405,6 +405,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::methods::OK_EXPECT_INFO, crate::methods::OPTION_AS_REF_DEREF_INFO, crate::methods::OPTION_FILTER_MAP_INFO, + crate::methods::OPTION_MAP_OR_ERR_OK_INFO, crate::methods::OPTION_MAP_OR_NONE_INFO, crate::methods::OR_FUN_CALL_INFO, crate::methods::OR_THEN_UNWRAP_INFO, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 71de77acfc1d..7ce14242cae4 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -70,6 +70,7 @@ mod obfuscated_if_else; mod ok_expect; mod open_options; mod option_as_ref_deref; +mod option_map_or_err_ok; mod option_map_or_none; mod option_map_unwrap_or; mod or_fun_call; @@ -3726,6 +3727,31 @@ declare_clippy_lint! { "calls to `Path::join` which will overwrite the original path" } +declare_clippy_lint! { + /// ### What it does + /// Checks for usage of `_.map_or(Err(_), Ok)`. + /// + /// ### Why is this bad? + /// Readability, this can be written more concisely as + /// `_.ok_or(_)`. + /// + /// ### Example + /// ```no_run + /// # let opt = Some(1); + /// opt.map_or(Err("error"), Ok); + /// ``` + /// + /// Use instead: + /// ```no_run + /// # let opt = Some(1); + /// opt.ok_or("error"); + /// ``` + #[clippy::version = "1.76.0"] + pub OPTION_MAP_OR_ERR_OK, + style, + "using `Option.map_or(Err(_), Ok)`, which is more succinctly expressed as `Option.ok_or(_)`" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Msrv, @@ -3876,6 +3902,7 @@ impl_lint_pass!(Methods => [ WAKER_CLONE_WAKE, UNNECESSARY_FALLIBLE_CONVERSIONS, JOIN_ABSOLUTE_PATHS, + OPTION_MAP_OR_ERR_OK, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -4335,6 +4362,7 @@ impl Methods { ("map_or", [def, map]) => { option_map_or_none::check(cx, expr, recv, def, map); manual_ok_or::check(cx, expr, recv, def, map); + option_map_or_err_ok::check(cx, expr, recv, def, map); }, ("map_or_else", [def, map]) => { result_map_or_else_none::check(cx, expr, recv, def, map); diff --git a/clippy_lints/src/methods/option_map_or_err_ok.rs b/clippy_lints/src/methods/option_map_or_err_ok.rs new file mode 100644 index 000000000000..91e39d5a1cd2 --- /dev/null +++ b/clippy_lints/src/methods/option_map_or_err_ok.rs @@ -0,0 +1,41 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::snippet; +use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::{is_res_lang_ctor, path_res}; +use rustc_errors::Applicability; +use rustc_hir::LangItem::{ResultErr, ResultOk}; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::LateContext; +use rustc_span::symbol::sym; + +use super::OPTION_MAP_OR_ERR_OK; + +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, + recv: &'tcx Expr<'_>, + or_expr: &'tcx Expr<'_>, + map_expr: &'tcx Expr<'_>, +) { + // We check that it's called on an `Option` type. + if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), sym::Option) + // We check that first we pass an `Err`. + && let ExprKind::Call(call, &[arg]) = or_expr.kind + && is_res_lang_ctor(cx, path_res(cx, call), ResultErr) + // And finally we check that it is mapped as `Ok`. + && is_res_lang_ctor(cx, path_res(cx, map_expr), ResultOk) + { + let msg = "called `map_or(Err(_), Ok)` on an `Option` value"; + let self_snippet = snippet(cx, recv.span, ".."); + let err_snippet = snippet(cx, arg.span, ".."); + span_lint_and_sugg( + cx, + OPTION_MAP_OR_ERR_OK, + expr.span, + msg, + "try using `ok_or` instead", + format!("{self_snippet}.ok_or({err_snippet})"), + Applicability::MachineApplicable, + ); + } +} From 3a3773fa3036fd549b017902455678717d011a0d Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 24 Nov 2023 17:57:13 +0100 Subject: [PATCH 2/3] Add test for `option_map_or_err_ok` lint --- tests/ui/manual_ok_or.stderr | 11 ++++++++++- tests/ui/option_map_or_err_ok.fixed | 7 +++++++ tests/ui/option_map_or_err_ok.rs | 7 +++++++ tests/ui/option_map_or_err_ok.stderr | 11 +++++++++++ 4 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 tests/ui/option_map_or_err_ok.fixed create mode 100644 tests/ui/option_map_or_err_ok.rs create mode 100644 tests/ui/option_map_or_err_ok.stderr diff --git a/tests/ui/manual_ok_or.stderr b/tests/ui/manual_ok_or.stderr index ddb2cf261e40..b277d22e59b6 100644 --- a/tests/ui/manual_ok_or.stderr +++ b/tests/ui/manual_ok_or.stderr @@ -13,6 +13,15 @@ error: this pattern reimplements `Option::ok_or` LL | foo.map_or(Err("error"), Ok); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `foo.ok_or("error")` +error: called `map_or(Err(_), Ok)` on an `Option` value + --> $DIR/manual_ok_or.rs:14:5 + | +LL | foo.map_or(Err("error"), Ok); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `ok_or` instead: `foo.ok_or("error")` + | + = note: `-D clippy::option-map-or-err-ok` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::option_map_or_err_ok)]` + error: this pattern reimplements `Option::ok_or` --> $DIR/manual_ok_or.rs:17:5 | @@ -38,5 +47,5 @@ LL + "{}{}{}{}{}{}{}", LL ~ "Alice", "Bob", "Sarah", "Marc", "Sandra", "Eric", "Jenifer")); | -error: aborting due to 4 previous errors +error: aborting due to 5 previous errors diff --git a/tests/ui/option_map_or_err_ok.fixed b/tests/ui/option_map_or_err_ok.fixed new file mode 100644 index 000000000000..131f4b2093e7 --- /dev/null +++ b/tests/ui/option_map_or_err_ok.fixed @@ -0,0 +1,7 @@ +#![warn(clippy::option_map_or_err_ok)] + +fn main() { + let x = Some("a"); + let _ = x.ok_or("a"); + //~^ ERROR: called `map_or(Err(_), Ok)` on an `Option` value +} diff --git a/tests/ui/option_map_or_err_ok.rs b/tests/ui/option_map_or_err_ok.rs new file mode 100644 index 000000000000..0f07a592ae5f --- /dev/null +++ b/tests/ui/option_map_or_err_ok.rs @@ -0,0 +1,7 @@ +#![warn(clippy::option_map_or_err_ok)] + +fn main() { + let x = Some("a"); + let _ = x.map_or(Err("a"), Ok); + //~^ ERROR: called `map_or(Err(_), Ok)` on an `Option` value +} diff --git a/tests/ui/option_map_or_err_ok.stderr b/tests/ui/option_map_or_err_ok.stderr new file mode 100644 index 000000000000..8476881aef77 --- /dev/null +++ b/tests/ui/option_map_or_err_ok.stderr @@ -0,0 +1,11 @@ +error: called `map_or(Err(_), Ok)` on an `Option` value + --> $DIR/option_map_or_err_ok.rs:5:13 + | +LL | let _ = x.map_or(Err("a"), Ok); + | ^^^^^^^^^^^^^^^^^^^^^^ help: try using `ok_or` instead: `x.ok_or("a")` + | + = note: `-D clippy::option-map-or-err-ok` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::option_map_or_err_ok)]` + +error: aborting due to previous error + From ef38969e06127a664ea532e0fd154638b3431ccb Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 24 Nov 2023 18:02:42 +0100 Subject: [PATCH 3/3] Update changelog to add new `option_map_or_err_ok` lint --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 11a2d1e7ef9a..abe975fa42b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5382,6 +5382,7 @@ Released 2018-09-13 [`option_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_expect_used [`option_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_filter_map [`option_if_let_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_if_let_else +[`option_map_or_err_ok`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_or_err_ok [`option_map_or_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_or_none [`option_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unit_fn [`option_map_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unwrap_or