diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 7f95adba870d..373d71f52f14 100644 --- a/clippy_lints/src/question_mark.rs +++ b/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); diff --git a/tests/ui/question_mark.fixed b/tests/ui/question_mark.fixed index 32effaf99ddd..e1e11601f4ab 100644 --- a/tests/ui/question_mark.fixed +++ b/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 { + &None + } + + fn return_ref_mut_option() -> &'static mut Option { + Box::leak(Box::new(Some(42))) + } + + fn return_ref_result() -> &'static Result { + &Ok(42) + } + + fn option_match(v: Option) -> Option { + // `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(()) + } +} diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs index 18e8404dea22..437f9dd35f71 100644 --- a/tests/ui/question_mark.rs +++ b/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 { + &None + } + + fn return_ref_mut_option() -> &'static mut Option { + Box::leak(Box::new(Some(42))) + } + + fn return_ref_result() -> &'static Result { + &Ok(42) + } + + fn option_match(v: Option) -> Option { + // `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(()) + } +} diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr index abb4c9531754..f9bc0babc468 100644 --- a/tests/ui/question_mark.stderr +++ b/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