Auto merge of #6233 - montrivo:manual_ok_or, r=flip1995

add manual_ok_or lint

Implements partially #5923

changelog: add lint manual_ok_or

CHANGELOG.md

@@ -1797,6 +1797,7 @@ Released 2018-09-13
 [`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn
 [`manual_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy
 [`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive
+[`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or
 [`manual_range_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains
 [`manual_saturating_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_saturating_arithmetic
 [`manual_strip`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_strip

clippy_lints/src/lib.rs

@@ -234,6 +234,7 @@ mod macro_use;
 mod main_recursion;
 mod manual_async_fn;
 mod manual_non_exhaustive;
+mod manual_ok_or;
 mod manual_strip;
 mod manual_unwrap_or;
 mod map_clone;
@@ -651,6 +652,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &main_recursion::MAIN_RECURSION,
         &manual_async_fn::MANUAL_ASYNC_FN,
         &manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE,
+        &manual_ok_or::MANUAL_OK_OR,
         &manual_strip::MANUAL_STRIP,
         &manual_unwrap_or::MANUAL_UNWRAP_OR,
         &map_clone::MAP_CLONE,
@@ -1152,6 +1154,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box unwrap_in_result::UnwrapInResult);
     store.register_late_pass(|| box self_assignment::SelfAssignment);
     store.register_late_pass(|| box manual_unwrap_or::ManualUnwrapOr);
+    store.register_late_pass(|| box manual_ok_or::ManualOkOr);
     store.register_late_pass(|| box float_equality_without_abs::FloatEqualityWithoutAbs);
     store.register_late_pass(|| box async_yields_async::AsyncYieldsAsync);
     store.register_late_pass(|| box manual_strip::ManualStrip);
@@ -1240,6 +1243,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&loops::EXPLICIT_INTO_ITER_LOOP),
         LintId::of(&loops::EXPLICIT_ITER_LOOP),
         LintId::of(&macro_use::MACRO_USE_IMPORTS),
+        LintId::of(&manual_ok_or::MANUAL_OK_OR),
         LintId::of(&map_err_ignore::MAP_ERR_IGNORE),
         LintId::of(&match_on_vec_items::MATCH_ON_VEC_ITEMS),
         LintId::of(&matches::MATCH_BOOL),

clippy_lints/src/manual_ok_or.rs

@@ -0,0 +1,99 @@
+use crate::utils::{
+    indent_of, is_type_diagnostic_item, match_qpath, paths, reindent_multiline, snippet_opt, span_lint_and_sugg,
+};
+use if_chain::if_chain;
+use rustc_errors::Applicability;
+use rustc_hir::{def, Expr, ExprKind, PatKind, QPath};
+use rustc_lint::LintContext;
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::lint::in_external_macro;
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+declare_clippy_lint! {
+    /// **What it does:**
+    /// Finds patterns that reimplement `Option::ok_or`.
+    ///
+    /// **Why is this bad?**
+    /// Concise code helps focusing on behavior instead of boilerplate.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Examples:**
+    /// ```rust
+    /// let foo: Option<i32> = None;
+    /// foo.map_or(Err("error"), |v| Ok(v));
+    ///
+    /// let foo: Option<i32> = None;
+    /// foo.map_or(Err("error"), |v| Ok(v));
+    /// ```
+    ///
+    /// Use instead:
+    /// ```rust
+    /// let foo: Option<i32> = None;
+    /// foo.ok_or("error");
+    /// ```
+    pub MANUAL_OK_OR,
+    pedantic,
+    "finds patterns that can be encoded more concisely with `Option::ok_or`"
+}
+
+declare_lint_pass!(ManualOkOr => [MANUAL_OK_OR]);
+
+impl LateLintPass<'_> for ManualOkOr {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, scrutinee: &'tcx Expr<'tcx>) {
+        if in_external_macro(cx.sess(), scrutinee.span) {
+            return;
+        }
+
+        if_chain! {
+            if let ExprKind::MethodCall(method_segment, _, args, _) = scrutinee.kind;
+            if method_segment.ident.name == sym!(map_or);
+            if args.len() == 3;
+            let method_receiver = &args[0];
+            let ty = cx.typeck_results().expr_ty(method_receiver);
+            if is_type_diagnostic_item(cx, ty, sym!(option_type));
+            let or_expr = &args[1];
+            if is_ok_wrapping(cx, &args[2]);
+            if let ExprKind::Call(Expr { kind: ExprKind::Path(err_path), .. }, &[ref err_arg]) = or_expr.kind;
+            if match_qpath(err_path, &paths::RESULT_ERR);
+            if let Some(method_receiver_snippet) = snippet_opt(cx, method_receiver.span);
+            if let Some(err_arg_snippet) = snippet_opt(cx, err_arg.span);
+            if let Some(indent) = indent_of(cx, scrutinee.span);
+            then {
+                let reindented_err_arg_snippet =
+                    reindent_multiline(err_arg_snippet.into(), true, Some(indent + 4));
+                span_lint_and_sugg(
+                    cx,
+                    MANUAL_OK_OR,
+                    scrutinee.span,
+                    "this pattern reimplements `Option::ok_or`",
+                    "replace with",
+                    format!(
+                        "{}.ok_or({})",
+                        method_receiver_snippet,
+                        reindented_err_arg_snippet
+                    ),
+                    Applicability::MachineApplicable,
+                );
+            }
+        }
+    }
+}
+
+fn is_ok_wrapping(cx: &LateContext<'_>, map_expr: &Expr<'_>) -> bool {
+    if let ExprKind::Path(ref qpath) = map_expr.kind {
+        if match_qpath(qpath, &paths::RESULT_OK) {
+            return true;
+        }
+    }
+    if_chain! {
+        if let ExprKind::Closure(_, _, body_id, ..) = map_expr.kind;
+        let body = cx.tcx.hir().body(body_id);
+        if let PatKind::Binding(_, param_id, ..) = body.params[0].pat.kind;
+        if let ExprKind::Call(Expr { kind: ExprKind::Path(ok_path), .. }, &[ref ok_arg]) = body.value.kind;
+        if match_qpath(ok_path, &paths::RESULT_OK);
+        if let ExprKind::Path(QPath::Resolved(_, ok_arg_path)) = ok_arg.kind;
+        if let def::Res::Local(ok_arg_path_id) = ok_arg_path.res;
+        then { param_id == ok_arg_path_id } else { false }
+    }
+}

src/lintlist/mod.rs

@@ -1187,6 +1187,13 @@ vec![
         deprecation: None,
         module: "manual_non_exhaustive",
     },
+    Lint {
+        name: "manual_ok_or",
+        group: "pedantic",
+        desc: "finds patterns that can be encoded more concisely with `Option::ok_or`",
+        deprecation: None,
+        module: "manual_ok_or",
+    },
     Lint {
         name: "manual_range_contains",
         group: "style",

tests/ui/manual_ok_or.fixed

@@ -0,0 +1,40 @@
+// run-rustfix
+#![warn(clippy::manual_ok_or)]
+#![allow(clippy::blacklisted_name)]
+#![allow(clippy::redundant_closure)]
+#![allow(dead_code)]
+#![allow(unused_must_use)]
+
+fn main() {
+    // basic case
+    let foo: Option<i32> = None;
+    foo.ok_or("error");
+
+    // eta expansion case
+    foo.ok_or("error");
+
+    // turbo fish syntax
+    None::<i32>.ok_or("error");
+
+    // multiline case
+    #[rustfmt::skip]
+    foo.ok_or(&format!(
+        "{}{}{}{}{}{}{}",
+        "Alice", "Bob", "Sarah", "Marc", "Sandra", "Eric", "Jenifer"));
+
+    // not applicable, closure isn't direct `Ok` wrapping
+    foo.map_or(Err("error"), |v| Ok(v + 1));
+
+    // not applicable, or side isn't `Result::Err`
+    foo.map_or(Ok::<i32, &str>(1), |v| Ok(v));
+
+    // not applicatble, expr is not a `Result` value
+    foo.map_or(42, |v| v);
+
+    // TODO patterns not covered yet
+    match foo {
+        Some(v) => Ok(v),
+        None => Err("error"),
+    };
+    foo.map_or_else(|| Err("error"), |v| Ok(v));
+}

tests/ui/manual_ok_or.rs

@@ -0,0 +1,44 @@
+// run-rustfix
+#![warn(clippy::manual_ok_or)]
+#![allow(clippy::blacklisted_name)]
+#![allow(clippy::redundant_closure)]
+#![allow(dead_code)]
+#![allow(unused_must_use)]
+
+fn main() {
+    // basic case
+    let foo: Option<i32> = None;
+    foo.map_or(Err("error"), |v| Ok(v));
+
+    // eta expansion case
+    foo.map_or(Err("error"), Ok);
+
+    // turbo fish syntax
+    None::<i32>.map_or(Err("error"), |v| Ok(v));
+
+    // multiline case
+    #[rustfmt::skip]
+    foo.map_or(Err::<i32, &str>(
+        &format!(
+            "{}{}{}{}{}{}{}",
+            "Alice", "Bob", "Sarah", "Marc", "Sandra", "Eric", "Jenifer")
+        ),
+        |v| Ok(v),
+    );
+
+    // not applicable, closure isn't direct `Ok` wrapping
+    foo.map_or(Err("error"), |v| Ok(v + 1));
+
+    // not applicable, or side isn't `Result::Err`
+    foo.map_or(Ok::<i32, &str>(1), |v| Ok(v));
+
+    // not applicatble, expr is not a `Result` value
+    foo.map_or(42, |v| v);
+
+    // TODO patterns not covered yet
+    match foo {
+        Some(v) => Ok(v),
+        None => Err("error"),
+    };
+    foo.map_or_else(|| Err("error"), |v| Ok(v));
+}

tests/ui/manual_ok_or.stderr

@@ -0,0 +1,41 @@
+error: this pattern reimplements `Option::ok_or`
+  --> $DIR/manual_ok_or.rs:11:5
+   |
+LL |     foo.map_or(Err("error"), |v| Ok(v));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `foo.ok_or("error")`
+   |
+   = note: `-D clippy::manual-ok-or` implied by `-D warnings`
+
+error: this pattern reimplements `Option::ok_or`
+  --> $DIR/manual_ok_or.rs:14:5
+   |
+LL |     foo.map_or(Err("error"), Ok);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `foo.ok_or("error")`
+
+error: this pattern reimplements `Option::ok_or`
+  --> $DIR/manual_ok_or.rs:17:5
+   |
+LL |     None::<i32>.map_or(Err("error"), |v| Ok(v));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `None::<i32>.ok_or("error")`
+
+error: this pattern reimplements `Option::ok_or`
+  --> $DIR/manual_ok_or.rs:21:5
+   |
+LL | /     foo.map_or(Err::<i32, &str>(
+LL | |         &format!(
+LL | |             "{}{}{}{}{}{}{}",
+LL | |             "Alice", "Bob", "Sarah", "Marc", "Sandra", "Eric", "Jenifer")
+LL | |         ),
+LL | |         |v| Ok(v),
+LL | |     );
+   | |_____^
+   |
+help: replace with
+   |
+LL |     foo.ok_or(&format!(
+LL |         "{}{}{}{}{}{}{}",
+LL |         "Alice", "Bob", "Sarah", "Marc", "Sandra", "Eric", "Jenifer"));
+   |
+
+error: aborting due to 4 previous errors
+