question_mark: suggest as_ref/as_mut if ref binding is used

Author: notriddle

Diff for: clippy_lints/src/question_mark.rs

@@ -145,8 +145,47 @@ fn check_let_some_else_return_none(cx: &LateContext<'_>, stmt: &Stmt<'_>) {
     {
         let mut applicability = Applicability::MaybeIncorrect;
         let init_expr_str = snippet_with_applicability(cx, init_expr.span, "..", &mut applicability);
-        let receiver_str = snippet_with_applicability(cx, inner_pat.span, "..", &mut applicability);
-        let sugg = format!("let {receiver_str} = {init_expr_str}?;",);
+        // Take care when binding is `ref`
+        let sugg = if let PatKind::Binding(
+            BindingMode(ByRef::Yes(ref_mutability), binding_mutability),
+            _hir_id,
+            ident,
+            subpattern,
+        ) = inner_pat.kind
+        {
+            let (from_method, replace_to) = match ref_mutability {
+                Mutability::Mut => (".as_mut()", "&mut "),
+                Mutability::Not => (".as_ref()", "&"),
+            };
+
+            let mutability_str = match binding_mutability {
+                Mutability::Mut => "mut ",
+                Mutability::Not => "",
+            };
+
+            // Handle subpattern (@ subpattern)
+            let maybe_subpattern = match subpattern {
+                Some(Pat {
+                    kind: PatKind::Binding(BindingMode(ByRef::Yes(_), _), _, subident, None),
+                    ..
+                }) => {
+                    // avoid `&ref`
+                    // note that, because you can't have aliased, mutable references, we don't have to worry about
+                    // the outer and inner mutability being different
+                    format!(" @ {subident}")
+                },
+                Some(subpattern) => {
+                    let substr = snippet_with_applicability(cx, subpattern.span, "..", &mut applicability);
+                    format!(" @ {replace_to}{substr}")
+                },
+                None => String::new(),
+            };
+
+            format!("let {mutability_str}{ident}{maybe_subpattern} = {init_expr_str}{from_method}?;")
+        } else {
+            let receiver_str = snippet_with_applicability(cx, inner_pat.span, "..", &mut applicability);
+            format!("let {receiver_str} = {init_expr_str}?;")
+        };
         span_lint_and_sugg(
             cx,
             QUESTION_MARK,

Diff for: tests/ui/question_mark.fixed

@@ -375,3 +375,37 @@ fn issue12412(foo: &Foo, bar: &Bar) -> Option<()> {
     //~^^^ question_mark
     Some(())
 }
+
+struct StructWithOptionString {
+    opt_x: Option<String>,
+}
+
+struct WrapperStructWithString(String);
+
+#[allow(clippy::disallowed_names)]
+fn issue_13417(foo: &mut StructWithOptionString) -> Option<String> {
+    let x = foo.opt_x.as_ref()?;
+    //~^^^ question_mark
+    let opt_y = Some(x.clone());
+    std::mem::replace(&mut foo.opt_x, opt_y)
+}
+
+#[allow(clippy::disallowed_names)]
+fn issue_13417_mut(foo: &mut StructWithOptionString) -> Option<String> {
+    let x = foo.opt_x.as_mut()?;
+    //~^^^ question_mark
+    let opt_y = Some(x.clone());
+    std::mem::replace(&mut foo.opt_x, opt_y)
+}
+
+#[allow(clippy::disallowed_names)]
+#[allow(unused)]
+fn issue_13417_weirder(foo: &mut StructWithOptionString, mut bar: Option<WrapperStructWithString>) -> Option<()> {
+    let x @ y = foo.opt_x.as_ref()?;
+    //~^^^ question_mark
+    let x @ &WrapperStructWithString(_) = bar.as_ref()?;
+    //~^^^ question_mark
+    let x @ &mut WrapperStructWithString(_) = bar.as_mut()?;
+    //~^^^ question_mark
+    Some(())
+}

Diff for: tests/ui/question_mark.rs

@@ -452,3 +452,47 @@ fn issue12412(foo: &Foo, bar: &Bar) -> Option<()> {
     //~^^^ question_mark
     Some(())
 }
+
+struct StructWithOptionString {
+    opt_x: Option<String>,
+}
+
+struct WrapperStructWithString(String);
+
+#[allow(clippy::disallowed_names)]
+fn issue_13417(foo: &mut StructWithOptionString) -> Option<String> {
+    let Some(ref x) = foo.opt_x else {
+        return None;
+    };
+    //~^^^ question_mark
+    let opt_y = Some(x.clone());
+    std::mem::replace(&mut foo.opt_x, opt_y)
+}
+
+#[allow(clippy::disallowed_names)]
+fn issue_13417_mut(foo: &mut StructWithOptionString) -> Option<String> {
+    let Some(ref mut x) = foo.opt_x else {
+        return None;
+    };
+    //~^^^ question_mark
+    let opt_y = Some(x.clone());
+    std::mem::replace(&mut foo.opt_x, opt_y)
+}
+
+#[allow(clippy::disallowed_names)]
+#[allow(unused)]
+fn issue_13417_weirder(foo: &mut StructWithOptionString, mut bar: Option<WrapperStructWithString>) -> Option<()> {
+    let Some(ref x @ ref y) = foo.opt_x else {
+        return None;
+    };
+    //~^^^ question_mark
+    let Some(ref x @ WrapperStructWithString(_)) = bar else {
+        return None;
+    };
+    //~^^^ question_mark
+    let Some(ref mut x @ WrapperStructWithString(_)) = bar else {
+        return None;
+    };
+    //~^^^ question_mark
+    Some(())
+}

Diff for: tests/ui/question_mark.stderr

@@ -215,5 +215,45 @@ LL | |         return None;
 LL | |     };
    | |______^ help: replace it with: `let v = bar.foo.owned.clone()?;`
 
-error: aborting due to 22 previous errors
+error: this `let...else` may be rewritten with the `?` operator
+  --> tests/ui/question_mark.rs:464:5
+   |
+LL | /     let Some(ref x) = foo.opt_x else {
+LL | |         return None;
+LL | |     };
+   | |______^ help: replace it with: `let x = foo.opt_x.as_ref()?;`
+
+error: this `let...else` may be rewritten with the `?` operator
+  --> tests/ui/question_mark.rs:474:5
+   |
+LL | /     let Some(ref mut x) = foo.opt_x else {
+LL | |         return None;
+LL | |     };
+   | |______^ help: replace it with: `let x = foo.opt_x.as_mut()?;`
+
+error: this `let...else` may be rewritten with the `?` operator
+  --> tests/ui/question_mark.rs:485:5
+   |
+LL | /     let Some(ref x @ ref y) = foo.opt_x else {
+LL | |         return None;
+LL | |     };
+   | |______^ help: replace it with: `let x @ y = foo.opt_x.as_ref()?;`
+
+error: this `let...else` may be rewritten with the `?` operator
+  --> tests/ui/question_mark.rs:489:5
+   |
+LL | /     let Some(ref x @ WrapperStructWithString(_)) = bar else {
+LL | |         return None;
+LL | |     };
+   | |______^ help: replace it with: `let x @ &WrapperStructWithString(_) = bar.as_ref()?;`
+
+error: this `let...else` may be rewritten with the `?` operator
+  --> tests/ui/question_mark.rs:493:5
+   |
+LL | /     let Some(ref mut x @ WrapperStructWithString(_)) = bar else {
+LL | |         return None;
+LL | |     };
+   | |______^ help: replace it with: `let x @ &mut WrapperStructWithString(_) = bar.as_mut()?;`
+
+error: aborting due to 27 previous errors