about summary refs log tree commit diff
diff options
context:
space:
mode:
-rw-r--r--clippy_lints/src/methods/or_then_unwrap.rs30
-rw-r--r--tests/ui/or_then_unwrap.fixed48
-rw-r--r--tests/ui/or_then_unwrap.rs2
-rw-r--r--tests/ui/or_then_unwrap.stderr8
4 files changed, 75 insertions, 13 deletions
diff --git a/clippy_lints/src/methods/or_then_unwrap.rs b/clippy_lints/src/methods/or_then_unwrap.rs
index 578f898da78..4e2dcf67231 100644
--- a/clippy_lints/src/methods/or_then_unwrap.rs
+++ b/clippy_lints/src/methods/or_then_unwrap.rs
@@ -1,4 +1,5 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::source::snippet_with_applicability;
 use clippy_utils::ty::is_type_diagnostic_item;
 use if_chain::if_chain;
 use rustc_errors::Applicability;
@@ -17,15 +18,20 @@ pub(super) fn check<'tcx>(
 ) {
     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 = ".or(Some(…)).unwrap() found";
-        if !is(or_arg, "Some") {
+        if let Some(content) = get_content_if_is(or_arg, "Some") {
+            or_arg_content = content;
+        } else {
             return;
         }
     } else if is_type_diagnostic_item(cx, ty, sym::Result) {
         title = ".or(Ok(…)).unwrap() found";
-        if !is(or_arg, "Ok") {
+        if let Some(content) = get_content_if_is(or_arg, "Ok") {
+            or_arg_content = content;
+        } else {
             return;
         }
     } else {
@@ -41,30 +47,36 @@ pub(super) fn check<'tcx>(
         unwrap_expr.span
     };
 
+    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,
         or_span.to(unwrap_span),
         title,
         "try this",
-        "unwrap_or(...)".into(),
-        Applicability::HasPlaceholders,
+        suggestion,
+        applicability,
     );
 }
 
-/// is expr a Call to name?
+/// is expr a Call to name? if so, return what it's wrapping
 /// name might be "Some", "Ok", "Err", etc.
-fn is<'a>(expr: &Expr<'a>, name: &str) -> bool {
+fn get_content_if_is<'a>(expr: &Expr<'a>, name: &str) -> Option<Span> {
     if_chain! {
-        if let ExprKind::Call(some_expr, _some_args) = expr.kind;
+        if let ExprKind::Call(some_expr, [arg]) = expr.kind;
         if let ExprKind::Path(QPath::Resolved(_, path)) = &some_expr.kind;
         if let Some(path_segment) = path.segments.first();
         if path_segment.ident.name.as_str() == name;
         then {
-            true
+            Some(arg.span)
         }
         else {
-            false
+            None
         }
     }
 }
diff --git a/tests/ui/or_then_unwrap.fixed b/tests/ui/or_then_unwrap.fixed
new file mode 100644
index 00000000000..b1e69ce2d26
--- /dev/null
+++ b/tests/ui/or_then_unwrap.fixed
@@ -0,0 +1,48 @@
+// 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
+
+    // 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
+}
diff --git a/tests/ui/or_then_unwrap.rs b/tests/ui/or_then_unwrap.rs
index 8d7e50ad405..dc66e86be6f 100644
--- a/tests/ui/or_then_unwrap.rs
+++ b/tests/ui/or_then_unwrap.rs
@@ -1,3 +1,5 @@
+// run-rustfix
+
 #![warn(clippy::or_then_unwrap)]
 #![allow(clippy::map_identity)]
 
diff --git a/tests/ui/or_then_unwrap.stderr b/tests/ui/or_then_unwrap.stderr
index 7eade84bd65..301a54530cf 100644
--- a/tests/ui/or_then_unwrap.stderr
+++ b/tests/ui/or_then_unwrap.stderr
@@ -1,16 +1,16 @@
 error: .or(Some(…)).unwrap() found
-  --> $DIR/or_then_unwrap.rs:22:20
+  --> $DIR/or_then_unwrap.rs:24:20
    |
 LL |     let _ = option.or(Some("fallback")).unwrap(); // should trigger lint
-   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or(...)`
+   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or("fallback")`
    |
    = note: `-D clippy::or-then-unwrap` implied by `-D warnings`
 
 error: .or(Ok(…)).unwrap() found
-  --> $DIR/or_then_unwrap.rs:25:20
+  --> $DIR/or_then_unwrap.rs:27:20
    |
 LL |     let _ = result.or::<&str>(Ok("fallback")).unwrap(); // should trigger lint
-   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or(...)`
+   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or("fallback")`
 
 error: aborting due to 2 previous errors