Auto merge of #3561 - fuerstenau:master, r=oli-obk

Suggest `.as_ref()?` instead of `?` in certain circumstances.

3 files changed, 107 insertions, 39 deletions

diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs
index 057b4850e4f..76fb6350681 100644
--- a/clippy_lints/src/question_mark.rs
+++ b/clippy_lints/src/question_mark.rs
@@ -72,6 +72,8 @@ impl Pass {
             if Self::is_option(cx, subject);
 
             then {
+                let receiver_str = &Sugg::hir(cx, subject, "..");
+                let mut replacement: Option<String> = None;
                 if let Some(else_) = else_ {
                     if_chain! {
                         if let ExprKind::Block(block, None) = &else_.node;
@@ -79,45 +81,41 @@ impl Pass {
                         if let Some(block_expr) = &block.expr;
                         if SpanlessEq::new(cx).ignore_fn().eq_expr(subject, block_expr);
                         then {
-                            span_lint_and_then(
-                                cx,
-                                QUESTION_MARK,
-                                expr.span,
-                                "this block may be rewritten with the `?` operator",
-                                |db| {
-                                    db.span_suggestion_with_applicability(
-                                        expr.span,
-                                        "replace_it_with",
-                                        format!("Some({}?)", Sugg::hir(cx, subject, "..")),
-                                        Applicability::MaybeIncorrect, // snippet
-                                    );
-                                }
-                            )
+                            replacement = Some(format!("Some({}?)", receiver_str));
                         }
                     }
-                    return;
+                } else if Self::moves_by_default(cx, subject) {
+                        replacement = Some(format!("{}.as_ref()?;", receiver_str));
+                } else {
+                        replacement = Some(format!("{}?;", receiver_str));
                 }
 
-                span_lint_and_then(
-                    cx,
-                    QUESTION_MARK,
-                    expr.span,
-                    "this block may be rewritten with the `?` operator",
-                    |db| {
-                        let receiver_str = &Sugg::hir(cx, subject, "..");
-
-                        db.span_suggestion_with_applicability(
-                            expr.span,
-                            "replace_it_with",
-                            format!("{}?;", receiver_str),
-                            Applicability::MaybeIncorrect, // snippet
-                        );
-                    }
-                )
+                if let Some(replacement_str) = replacement {
+                    span_lint_and_then(
+                        cx,
+                        QUESTION_MARK,
+                        expr.span,
+                        "this block may be rewritten with the `?` operator",
+                        |db| {
+                            db.span_suggestion_with_applicability(
+                                expr.span,
+                                "replace_it_with",
+                                replacement_str,
+                                Applicability::MaybeIncorrect, // snippet
+                            );
+                        }
+                    )
+               }
             }
         }
     }
 
+    fn moves_by_default(cx: &LateContext<'_, '_>, expression: &Expr) -> bool {
+        let expr_ty = cx.tables.expr_ty(expression);
+
+        expr_ty.moves_by_default(cx.tcx, cx.param_env, expression.span)
+    }
+
     fn is_option(cx: &LateContext<'_, '_>, expression: &Expr) -> bool {
         let expr_ty = cx.tables.expr_ty(expression);
 
diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs
index b1edec32eee..7e749d164ca 100644
--- a/tests/ui/question_mark.rs
+++ b/tests/ui/question_mark.rs
@@ -15,6 +15,15 @@ fn some_func(a: Option<u32>) -> Option<u32> {
     a
 }
 
+fn some_other_func(a: Option<u32>) -> Option<u32> {
+    if a.is_none() {
+        return None;
+    } else {
+        return Some(0);
+    }
+    unreachable!()
+}
+
 pub enum SeemsOption<T> {
     Some(T),
     None,
@@ -37,11 +46,11 @@ fn returns_something_similar_to_option(a: SeemsOption<u32>) -> SeemsOption<u32>
     a
 }
 
-pub struct SomeStruct {
+pub struct CopyStruct {
     pub opt: Option<u32>,
 }
 
-impl SomeStruct {
+impl CopyStruct {
     #[rustfmt::skip]
     pub fn func(&self) -> Option<u32> {
         if (self.opt).is_none() {
@@ -62,12 +71,49 @@ impl SomeStruct {
     }
 }
 
+#[derive(Clone)]
+pub struct MoveStruct {
+    pub opt: Option<Vec<u32>>,
+}
+
+impl MoveStruct {
+    pub fn ref_func(&self) -> Option<Vec<u32>> {
+        if self.opt.is_none() {
+            return None;
+        }
+
+        self.opt.clone()
+    }
+
+    pub fn mov_func_reuse(self) -> Option<Vec<u32>> {
+        if self.opt.is_none() {
+            return None;
+        }
+
+        self.opt
+    }
+
+    pub fn mov_func_no_use(self) -> Option<Vec<u32>> {
+        if self.opt.is_none() {
+            return None;
+        }
+        Some(Vec::new())
+    }
+}
+
 fn main() {
     some_func(Some(42));
     some_func(None);
 
-    let some_struct = SomeStruct { opt: Some(54) };
-    some_struct.func();
+    let copy_struct = CopyStruct { opt: Some(54) };
+    copy_struct.func();
+
+    let move_struct = MoveStruct {
+        opt: Some(vec![42, 1337]),
+    };
+    move_struct.ref_func();
+    move_struct.clone().mov_func_reuse();
+    move_struct.clone().mov_func_no_use();
 
     let so = SeemsOption::Some(45);
     returns_something_similar_to_option(so);
diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr
index 341f45eef78..a0b87813770 100644
--- a/tests/ui/question_mark.stderr
+++ b/tests/ui/question_mark.stderr
@@ -9,7 +9,7 @@ LL | |     }
    = note: `-D clippy::question-mark` implied by `-D warnings`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:47:9
+  --> $DIR/question_mark.rs:56:9
    |
 LL | /         if (self.opt).is_none() {
 LL | |             return None;
@@ -17,7 +17,7 @@ LL | |         }
    | |_________^ help: replace_it_with: `(self.opt)?;`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:51:9
+  --> $DIR/question_mark.rs:60:9
    |
 LL | /         if self.opt.is_none() {
 LL | |             return None
@@ -25,7 +25,7 @@ LL | |         }
    | |_________^ help: replace_it_with: `self.opt?;`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:55:17
+  --> $DIR/question_mark.rs:64:17
    |
 LL |           let _ = if self.opt.is_none() {
    |  _________________^
@@ -35,5 +35,29 @@ LL | |             self.opt
 LL | |         };
    | |_________^ help: replace_it_with: `Some(self.opt?)`
 
-error: aborting due to 4 previous errors
+error: this block may be rewritten with the `?` operator
+  --> $DIR/question_mark.rs:81:9
+   |
+LL | /         if self.opt.is_none() {
+LL | |             return None;
+LL | |         }
+   | |_________^ help: replace_it_with: `self.opt.as_ref()?;`
+
+error: this block may be rewritten with the `?` operator
+  --> $DIR/question_mark.rs:89:9
+   |
+LL | /         if self.opt.is_none() {
+LL | |             return None;
+LL | |         }
+   | |_________^ help: replace_it_with: `self.opt.as_ref()?;`
+
+error: this block may be rewritten with the `?` operator
+  --> $DIR/question_mark.rs:97:9
+   |
+LL | /         if self.opt.is_none() {
+LL | |             return None;
+LL | |         }
+   | |_________^ help: replace_it_with: `self.opt.as_ref()?;`
+
+error: aborting due to 7 previous errors