Fix question_mark suggests wrongly when match scrutinee is a reference type

clippy_lints/src/question_mark.rs

@@ -471,6 +471,21 @@ fn check_if_try_match<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
         && !span_contains_cfg(cx, expr.span)
         && let Some(if_let_or_match_then) = check_arms_are_try(cx, mode, arm1, arm2)
     {
+        // `Try` is not implemented for `&Option` / `&mut Option`, so when the
+        // scrutinee is a reference type we need `.as_ref()?` / `.as_mut()?`.
+        // Skip `&expr` / `&mut expr` scrutinees because `&expr?` already applies
+        // `?` to the inner `expr` via operator precedence.
+        let scrutinee_ty = cx.typeck_results().expr_ty_adjusted(scrutinee);
+        let ref_suffix = if matches!(scrutinee.kind, ExprKind::AddrOf(..)) {
+            ""
+        } else {
+            match scrutinee_ty.kind() {
+                ty::Ref(_, _, Mutability::Mut) => ".as_mut()",
+                ty::Ref(_, _, Mutability::Not) => ".as_ref()",
+                _ => "",
+            }
+        };
+
         span_lint_and_then(
             cx,
             QUESTION_MARK,
@@ -485,7 +500,7 @@ fn check_if_try_match<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
                         diag.span_suggestion(
                             expr.span,
                             "try instead",
-                            scrutinee_snippet.into_owned() + "?",
+                            format!("{scrutinee_snippet}{ref_suffix}?"),
                             applicability,
                         );
                     },
@@ -498,12 +513,12 @@ fn check_if_try_match<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
                         if matches!(arm_body.kind, ExprKind::Block(..)) && sugg.starts_with('{') {
                             sugg.insert_str(
                                 1,
-                                &format!("\n{inner_indent}let {binding_snippet} = {scrutinee_snippet}?;"),
+                                &format!("\n{inner_indent}let {binding_snippet} = {scrutinee_snippet}{ref_suffix}?;"),
                             );
                         } else {
                             let outer_indent = " ".repeat(indent);
                             sugg = format!(
-                                "{{\n{inner_indent}let {binding_snippet} = {scrutinee_snippet}?;\n{inner_indent}{sugg}\n{outer_indent}}}"
+                                "{{\n{inner_indent}let {binding_snippet} = {scrutinee_snippet}{ref_suffix}?;\n{inner_indent}{sugg}\n{outer_indent}}}"
                             );
                         }
                         diag.span_suggestion(expr.span, "try instead", sugg, applicability);

tests/ui/question_mark.fixed

@@ -575,3 +575,49 @@ fn issue_destructuring_assignment() -> Option<(i32, i32)> {
     }
     Some((a, b))
 }
+
+mod issue17386 {
+    fn return_ref_option() -> &'static Option<i32> {
+        &None
+    }
+
+    fn return_ref_mut_option() -> &'static mut Option<i32> {
+        Box::leak(Box::new(Some(42)))
+    }
+
+    fn return_ref_result() -> &'static Result<i32, i32> {
+        &Ok(42)
+    }
+
+    fn option_match(v: Option<usize>) -> Option<usize> {
+        // `match` on a function returning `&Option` — needs `.as_ref()?`
+        {
+            let it = return_ref_option().as_ref()?;
+            dbg!(it);
+        };
+        let _x: &i32 = return_ref_option().as_ref()?;
+        // `match` on a function returning `&mut Option` — needs `.as_mut()?`
+        {
+            let it = return_ref_mut_option().as_mut()?;
+            *it += 1;
+        };
+        let _x: &mut i32 = return_ref_mut_option().as_mut()?;
+        // `match &v` still produces `&v?`, unchanged
+        {
+            let n = &v?;
+            println!("{n}");
+            Some(42)
+        };
+        Some(42)
+    }
+
+    fn result_match() -> Result<(), &'static i32> {
+        // `match` on a function returning `&Result` — needs `.as_ref()?`
+        {
+            let val = return_ref_result().as_ref()?;
+            dbg!(val);
+        };
+        let _x: &i32 = return_ref_result().as_ref()?;
+        Ok(())
+    }
+}

tests/ui/question_mark.rs

@@ -722,3 +722,73 @@ fn issue_destructuring_assignment() -> Option<(i32, i32)> {
     }
     Some((a, b))
 }
+
+mod issue17386 {
+    fn return_ref_option() -> &'static Option<i32> {
+        &None
+    }
+
+    fn return_ref_mut_option() -> &'static mut Option<i32> {
+        Box::leak(Box::new(Some(42)))
+    }
+
+    fn return_ref_result() -> &'static Result<i32, i32> {
+        &Ok(42)
+    }
+
+    fn option_match(v: Option<usize>) -> Option<usize> {
+        // `match` on a function returning `&Option` — needs `.as_ref()?`
+        match return_ref_option() {
+            //~^ question_mark
+            Some(it) => {
+                dbg!(it);
+            },
+            None => return None,
+        };
+        let _x: &i32 = match return_ref_option() {
+            //~^ question_mark
+            Some(it) => it,
+            None => return None,
+        };
+        // `match` on a function returning `&mut Option` — needs `.as_mut()?`
+        match return_ref_mut_option() {
+            //~^ question_mark
+            Some(it) => {
+                *it += 1;
+            },
+            None => return None,
+        };
+        let _x: &mut i32 = match return_ref_mut_option() {
+            //~^ question_mark
+            Some(it) => it,
+            None => return None,
+        };
+        // `match &v` still produces `&v?`, unchanged
+        match &v {
+            //~^ question_mark
+            Some(n) => {
+                println!("{n}");
+                Some(42)
+            },
+            None => return None,
+        };
+        Some(42)
+    }
+
+    fn result_match() -> Result<(), &'static i32> {
+        // `match` on a function returning `&Result` — needs `.as_ref()?`
+        match return_ref_result() {
+            //~^ question_mark
+            Ok(val) => {
+                dbg!(val);
+            },
+            Err(e) => return Err(e),
+        };
+        let _x: &i32 = match return_ref_result() {
+            //~^ question_mark
+            Ok(val) => val,
+            Err(e) => return Err(e),
+        };
+        Ok(())
+    }
+}

tests/ui/question_mark.stderr

@@ -516,5 +516,119 @@ LL +         (a, b) = x
 LL +     }
    |
 
-error: aborting due to 47 previous errors
+error: this `match` expression can be replaced with `?`
+  --> tests/ui/question_mark.rs:741:9
+   |
+LL | /         match return_ref_option() {
+LL | |
+LL | |             Some(it) => {
+LL | |                 dbg!(it);
+LL | |             },
+LL | |             None => return None,
+LL | |         };
+   | |_________^
+   |
+help: try instead
+   |
+LL ~         {
+LL +             let it = return_ref_option().as_ref()?;
+LL +             dbg!(it);
+LL ~         };
+   |
+
+error: this `match` expression can be replaced with `?`
+  --> tests/ui/question_mark.rs:748:24
+   |
+LL |           let _x: &i32 = match return_ref_option() {
+   |  ________________________^
+LL | |
+LL | |             Some(it) => it,
+LL | |             None => return None,
+LL | |         };
+   | |_________^ help: try instead: `return_ref_option().as_ref()?`
+
+error: this `match` expression can be replaced with `?`
+  --> tests/ui/question_mark.rs:754:9
+   |
+LL | /         match return_ref_mut_option() {
+LL | |
+LL | |             Some(it) => {
+LL | |                 *it += 1;
+LL | |             },
+LL | |             None => return None,
+LL | |         };
+   | |_________^
+   |
+help: try instead
+   |
+LL ~         {
+LL +             let it = return_ref_mut_option().as_mut()?;
+LL +             *it += 1;
+LL ~         };
+   |
+
+error: this `match` expression can be replaced with `?`
+  --> tests/ui/question_mark.rs:761:28
+   |
+LL |           let _x: &mut i32 = match return_ref_mut_option() {
+   |  ____________________________^
+LL | |
+LL | |             Some(it) => it,
+LL | |             None => return None,
+LL | |         };
+   | |_________^ help: try instead: `return_ref_mut_option().as_mut()?`
+
+error: this `match` expression can be replaced with `?`
+  --> tests/ui/question_mark.rs:767:9
+   |
+LL | /         match &v {
+LL | |
+LL | |             Some(n) => {
+LL | |                 println!("{n}");
+...  |
+LL | |             None => return None,
+LL | |         };
+   | |_________^
+   |
+help: try instead
+   |
+LL ~         {
+LL +             let n = &v?;
+LL +             println!("{n}");
+LL +             Some(42)
+LL ~         };
+   |
+
+error: this `match` expression can be replaced with `?`
+  --> tests/ui/question_mark.rs:780:9
+   |
+LL | /         match return_ref_result() {
+LL | |
+LL | |             Ok(val) => {
+LL | |                 dbg!(val);
+LL | |             },
+LL | |             Err(e) => return Err(e),
+LL | |         };
+   | |_________^
+   |
+help: try instead
+   |
+LL ~         {
+LL +             let val = return_ref_result().as_ref()?;
+LL +             dbg!(val);
+LL ~         };
+   |
+
+error: this `match` expression can be replaced with `?`
+  --> tests/ui/question_mark.rs:787:24
+   |
+LL |           let _x: &i32 = match return_ref_result() {
+   |  ________________________^
+LL | |
+LL | |             Ok(val) => val,
+LL | |             Err(e) => return Err(e),
+LL | |         };
+   | |_________^ help: try instead: `return_ref_result().as_ref()?`
+
+error: aborting due to 54 previous errors