Auto merge of #8561 - FoseFx:use_unwrap_or, r=xFrednet

add `or_then_unwrap`

Closes #8557

changelog: New lint [`or_then_unwrap`]

Author: bors

CHANGELOG.md

@@ -3367,6 +3367,7 @@ Released 2018-09-13
 [`option_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unit_fn
 [`option_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_option
 [`or_fun_call`]: https://rust-lang.github.io/rust-clippy/master/index.html#or_fun_call
+[`or_then_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#or_then_unwrap
 [`out_of_bounds_indexing`]: https://rust-lang.github.io/rust-clippy/master/index.html#out_of_bounds_indexing
 [`overflow_check_conditional`]: https://rust-lang.github.io/rust-clippy/master/index.html#overflow_check_conditional
 [`panic`]: https://rust-lang.github.io/rust-clippy/master/index.html#panic

clippy_lints/src/lib.register_all.rs

@@ -182,6 +182,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(methods::OPTION_FILTER_MAP),
     LintId::of(methods::OPTION_MAP_OR_NONE),
     LintId::of(methods::OR_FUN_CALL),
+    LintId::of(methods::OR_THEN_UNWRAP),
     LintId::of(methods::RESULT_MAP_OR_INTO_OPTION),
     LintId::of(methods::SEARCH_IS_SOME),
     LintId::of(methods::SHOULD_IMPLEMENT_TRAIT),

clippy_lints/src/lib.register_complexity.rs

@@ -47,6 +47,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec!
     LintId::of(methods::NEEDLESS_SPLITN),
     LintId::of(methods::OPTION_AS_REF_DEREF),
     LintId::of(methods::OPTION_FILTER_MAP),
+    LintId::of(methods::OR_THEN_UNWRAP),
     LintId::of(methods::SEARCH_IS_SOME),
     LintId::of(methods::SKIP_WHILE_NEXT),
     LintId::of(methods::UNNECESSARY_FILTER_MAP),

clippy_lints/src/lib.register_lints.rs

@@ -320,6 +320,7 @@ store.register_lints(&[
     methods::OPTION_FILTER_MAP,
     methods::OPTION_MAP_OR_NONE,
     methods::OR_FUN_CALL,
+    methods::OR_THEN_UNWRAP,
     methods::RESULT_MAP_OR_INTO_OPTION,
     methods::SEARCH_IS_SOME,
     methods::SHOULD_IMPLEMENT_TRAIT,

clippy_lints/src/methods/mod.rs

@@ -45,6 +45,7 @@ mod option_as_ref_deref;
 mod option_map_or_none;
 mod option_map_unwrap_or;
 mod or_fun_call;
+mod or_then_unwrap;
 mod search_is_some;
 mod single_char_add_str;
 mod single_char_insert_string;
@@ -778,6 +779,42 @@ declare_clippy_lint! {
     "using any `*or` method with a function call, which suggests `*or_else`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for `.or(…).unwrap()` calls to Options and Results.
+    ///
+    /// ### Why is this bad?
+    /// You should use `.unwrap_or(…)` instead for clarity.
+    ///
+    /// ### Example
+    /// ```rust
+    /// # let fallback = "fallback";
+    /// // Result
+    /// # type Error = &'static str;
+    /// # let result: Result<&str, Error> = Err("error");
+    /// let value = result.or::<Error>(Ok(fallback)).unwrap();
+    ///
+    /// // Option
+    /// # let option: Option<&str> = None;
+    /// let value = option.or(Some(fallback)).unwrap();
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// # let fallback = "fallback";
+    /// // Result
+    /// # let result: Result<&str, &str> = Err("error");
+    /// let value = result.unwrap_or(fallback);
+    ///
+    /// // Option
+    /// # let option: Option<&str> = None;
+    /// let value = option.unwrap_or(fallback);
+    /// ```
+    #[clippy::version = "1.61.0"]
+    pub OR_THEN_UNWRAP,
+    complexity,
+    "checks for `.or(…).unwrap()` calls to Options and Results."
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for calls to `.expect(&format!(...))`, `.expect(foo(..))`,
@@ -2039,6 +2076,7 @@ impl_lint_pass!(Methods => [
     OPTION_MAP_OR_NONE,
     BIND_INSTEAD_OF_MAP,
     OR_FUN_CALL,
+    OR_THEN_UNWRAP,
     EXPECT_FUN_CALL,
     CHARS_NEXT_CMP,
     CHARS_LAST_CMP,
@@ -2474,6 +2512,9 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
                     Some(("get_mut", [recv, get_arg], _)) => {
                         get_unwrap::check(cx, expr, recv, get_arg, true);
                     },
+                    Some(("or", [recv, or_arg], or_span)) => {
+                        or_then_unwrap::check(cx, expr, recv, or_arg, or_span);
+                    },
                     _ => {},
                 }
                 unwrap_used::check(cx, expr, recv);

clippy_lints/src/methods/or_then_unwrap.rs

@@ -0,0 +1,68 @@
+use clippy_utils::source::snippet_with_applicability;
+use clippy_utils::ty::is_type_diagnostic_item;
+use clippy_utils::{diagnostics::span_lint_and_sugg, is_lang_ctor};
+use rustc_errors::Applicability;
+use rustc_hir::{lang_items::LangItem, Expr, ExprKind};
+use rustc_lint::LateContext;
+use rustc_span::{sym, Span};
+
+use super::OR_THEN_UNWRAP;
+
+pub(super) fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    unwrap_expr: &Expr<'_>,
+    recv: &'tcx Expr<'tcx>,
+    or_arg: &'tcx Expr<'_>,
+    or_span: Span,
+) {
+    let ty = cx.typeck_results().expr_ty(recv); // get type of x (we later check if it's Option or Result)
+    let title;
+    let or_arg_content: Span;
+
+    if is_type_diagnostic_item(cx, ty, sym::Option) {
+        title = "found `.or(Some(…)).unwrap()`";
+        if let Some(content) = get_content_if_ctor_matches(cx, or_arg, LangItem::OptionSome) {
+            or_arg_content = content;
+        } else {
+            return;
+        }
+    } else if is_type_diagnostic_item(cx, ty, sym::Result) {
+        title = "found `.or(Ok(…)).unwrap()`";
+        if let Some(content) = get_content_if_ctor_matches(cx, or_arg, LangItem::ResultOk) {
+            or_arg_content = content;
+        } else {
+            return;
+        }
+    } else {
+        // Someone has implemented a struct with .or(...).unwrap() chaining,
+        // but it's not an Option or a Result, so bail
+        return;
+    }
+
+    let mut applicability = Applicability::MachineApplicable;
+    let suggestion = format!(
+        "unwrap_or({})",
+        snippet_with_applicability(cx, or_arg_content, "..", &mut applicability)
+    );
+
+    span_lint_and_sugg(
+        cx,
+        OR_THEN_UNWRAP,
+        unwrap_expr.span.with_lo(or_span.lo()),
+        title,
+        "try this",
+        suggestion,
+        applicability,
+    );
+}
+
+fn get_content_if_ctor_matches(cx: &LateContext<'_>, expr: &Expr<'_>, item: LangItem) -> Option<Span> {
+    if let ExprKind::Call(some_expr, [arg]) = expr.kind
+        && let ExprKind::Path(qpath) = &some_expr.kind
+        && is_lang_ctor(cx, qpath, item)
+    {
+        Some(arg.span)
+    } else {
+        None
+    }
+}

tests/ui/or_then_unwrap.fixed

@@ -0,0 +1,52 @@
+// run-rustfix
+
+#![warn(clippy::or_then_unwrap)]
+#![allow(clippy::map_identity)]
+
+struct SomeStruct {}
+impl SomeStruct {
+    fn or(self, _: Option<Self>) -> Self {
+        self
+    }
+    fn unwrap(&self) {}
+}
+
+struct SomeOtherStruct {}
+impl SomeOtherStruct {
+    fn or(self) -> Self {
+        self
+    }
+    fn unwrap(&self) {}
+}
+
+fn main() {
+    let option: Option<&str> = None;
+    let _ = option.unwrap_or("fallback"); // should trigger lint
+
+    let result: Result<&str, &str> = Err("Error");
+    let _ = result.unwrap_or("fallback"); // should trigger lint
+
+    // as part of a method chain
+    let option: Option<&str> = None;
+    let _ = option.map(|v| v).unwrap_or("fallback").to_string().chars(); // should trigger lint
+
+    // Not Option/Result
+    let instance = SomeStruct {};
+    let _ = instance.or(Some(SomeStruct {})).unwrap(); // should not trigger lint
+
+    // or takes no argument
+    let instance = SomeOtherStruct {};
+    let _ = instance.or().unwrap(); // should not trigger lint and should not panic
+
+    // None in or
+    let option: Option<&str> = None;
+    let _ = option.or(None).unwrap(); // should not trigger lint
+
+    // Not Err in or
+    let result: Result<&str, &str> = Err("Error");
+    let _ = result.or::<&str>(Err("Other Error")).unwrap(); // should not trigger lint
+
+    // other function between
+    let option: Option<&str> = None;
+    let _ = option.or(Some("fallback")).map(|v| v).unwrap(); // should not trigger lint
+}

tests/ui/or_then_unwrap.rs

@@ -0,0 +1,52 @@
+// run-rustfix
+
+#![warn(clippy::or_then_unwrap)]
+#![allow(clippy::map_identity)]
+
+struct SomeStruct {}
+impl SomeStruct {
+    fn or(self, _: Option<Self>) -> Self {
+        self
+    }
+    fn unwrap(&self) {}
+}
+
+struct SomeOtherStruct {}
+impl SomeOtherStruct {
+    fn or(self) -> Self {
+        self
+    }
+    fn unwrap(&self) {}
+}
+
+fn main() {
+    let option: Option<&str> = None;
+    let _ = option.or(Some("fallback")).unwrap(); // should trigger lint
+
+    let result: Result<&str, &str> = Err("Error");
+    let _ = result.or::<&str>(Ok("fallback")).unwrap(); // should trigger lint
+
+    // as part of a method chain
+    let option: Option<&str> = None;
+    let _ = option.map(|v| v).or(Some("fallback")).unwrap().to_string().chars(); // should trigger lint
+
+    // Not Option/Result
+    let instance = SomeStruct {};
+    let _ = instance.or(Some(SomeStruct {})).unwrap(); // should not trigger lint
+
+    // or takes no argument
+    let instance = SomeOtherStruct {};
+    let _ = instance.or().unwrap(); // should not trigger lint and should not panic
+
+    // None in or
+    let option: Option<&str> = None;
+    let _ = option.or(None).unwrap(); // should not trigger lint
+
+    // Not Err in or
+    let result: Result<&str, &str> = Err("Error");
+    let _ = result.or::<&str>(Err("Other Error")).unwrap(); // should not trigger lint
+
+    // other function between
+    let option: Option<&str> = None;
+    let _ = option.or(Some("fallback")).map(|v| v).unwrap(); // should not trigger lint
+}

tests/ui/or_then_unwrap.stderr

@@ -0,0 +1,22 @@
+error: found `.or(Some(…)).unwrap()`
+  --> $DIR/or_then_unwrap.rs:24:20
+   |
+LL |     let _ = option.or(Some("fallback")).unwrap(); // should trigger lint
+   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or("fallback")`
+   |
+   = note: `-D clippy::or-then-unwrap` implied by `-D warnings`
+
+error: found `.or(Ok(…)).unwrap()`
+  --> $DIR/or_then_unwrap.rs:27:20
+   |
+LL |     let _ = result.or::<&str>(Ok("fallback")).unwrap(); // should trigger lint
+   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or("fallback")`
+
+error: found `.or(Some(…)).unwrap()`
+  --> $DIR/or_then_unwrap.rs:31:31
+   |
+LL |     let _ = option.map(|v| v).or(Some("fallback")).unwrap().to_string().chars(); // should trigger lint
+   |                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or("fallback")`
+
+error: aborting due to 3 previous errors
+