Skip to content

Commit e97a239

Browse files
Linshu YangLinshu Yang
Linshu Yang
authored and
Linshu Yang
committed
fix: question_mark suggests wrongly when match scrutinee is a reference type
1 parent 8b6044b commit e97a239

4 files changed

Lines changed: 249 additions & 4 deletions

File tree

‎clippy_lints/src/question_mark.rs‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -471,6 +471,21 @@ fn check_if_try_match<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
471471
&& !span_contains_cfg(cx, expr.span)
472472
&& let Some(if_let_or_match_then) = check_arms_are_try(cx, mode, arm1, arm2)
473473
{
474+
// `Try` is not implemented for `&Option` / `&mut Option`, so when the
475+
// scrutinee is a reference type we need `.as_ref()?` / `.as_mut()?`.
476+
// Skip `&expr` / `&mut expr` scrutinees because `&expr?` already applies
477+
// `?` to the inner `expr` via operator precedence.
478+
let scrutinee_ty = cx.typeck_results().expr_ty_adjusted(scrutinee);
479+
let ref_suffix = if matches!(scrutinee.kind, ExprKind::AddrOf(..)) {
480+
""
481+
} else {
482+
match scrutinee_ty.kind() {
483+
ty::Ref(_, _, Mutability::Mut) => ".as_mut()",
484+
ty::Ref(_, _, Mutability::Not) => ".as_ref()",
485+
_ => "",
486+
}
487+
};
488+
474489
span_lint_and_then(
475490
cx,
476491
QUESTION_MARK,
@@ -485,7 +500,7 @@ fn check_if_try_match<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
485500
diag.span_suggestion(
486501
expr.span,
487502
"try instead",
488-
scrutinee_snippet.into_owned() + "?",
503+
format!("{scrutinee_snippet}{ref_suffix}?"),
489504
applicability,
490505
);
491506
},
@@ -498,12 +513,12 @@ fn check_if_try_match<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
498513
if matches!(arm_body.kind, ExprKind::Block(..)) && sugg.starts_with('{') {
499514
sugg.insert_str(
500515
1,
501-
&format!("\n{inner_indent}let {binding_snippet} = {scrutinee_snippet}?;"),
516+
&format!("\n{inner_indent}let {binding_snippet} = {scrutinee_snippet}{ref_suffix}?;"),
502517
);
503518
} else {
504519
let outer_indent = " ".repeat(indent);
505520
sugg = format!(
506-
"{{\n{inner_indent}let {binding_snippet} = {scrutinee_snippet}?;\n{inner_indent}{sugg}\n{outer_indent}}}"
521+
"{{\n{inner_indent}let {binding_snippet} = {scrutinee_snippet}{ref_suffix}?;\n{inner_indent}{sugg}\n{outer_indent}}}"
507522
);
508523
}
509524
diag.span_suggestion(expr.span, "try instead", sugg, applicability);

‎tests/ui/question_mark.fixed‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -575,3 +575,49 @@ fn issue_destructuring_assignment() -> Option<(i32, i32)> {
575575
}
576576
Some((a, b))
577577
}
578+
579+
mod issue17386 {
580+
fn return_ref_option() -> &'static Option<i32> {
581+
&None
582+
}
583+
584+
fn return_ref_mut_option() -> &'static mut Option<i32> {
585+
Box::leak(Box::new(Some(42)))
586+
}
587+
588+
fn return_ref_result() -> &'static Result<i32, i32> {
589+
&Ok(42)
590+
}
591+
592+
fn option_match(v: Option<usize>) -> Option<usize> {
593+
// `match` on a function returning `&Option` — needs `.as_ref()?`
594+
{
595+
let it = return_ref_option().as_ref()?;
596+
dbg!(it);
597+
};
598+
let _x: &i32 = return_ref_option().as_ref()?;
599+
// `match` on a function returning `&mut Option` — needs `.as_mut()?`
600+
{
601+
let it = return_ref_mut_option().as_mut()?;
602+
*it += 1;
603+
};
604+
let _x: &mut i32 = return_ref_mut_option().as_mut()?;
605+
// `match &v` still produces `&v?`, unchanged
606+
{
607+
let n = &v?;
608+
println!("{n}");
609+
Some(42)
610+
};
611+
Some(42)
612+
}
613+
614+
fn result_match() -> Result<(), &'static i32> {
615+
// `match` on a function returning `&Result` — needs `.as_ref()?`
616+
{
617+
let val = return_ref_result().as_ref()?;
618+
dbg!(val);
619+
};
620+
let _x: &i32 = return_ref_result().as_ref()?;
621+
Ok(())
622+
}
623+
}

‎tests/ui/question_mark.rs‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -722,3 +722,73 @@ fn issue_destructuring_assignment() -> Option<(i32, i32)> {
722722
}
723723
Some((a, b))
724724
}
725+
726+
mod issue17386 {
727+
fn return_ref_option() -> &'static Option<i32> {
728+
&None
729+
}
730+
731+
fn return_ref_mut_option() -> &'static mut Option<i32> {
732+
Box::leak(Box::new(Some(42)))
733+
}
734+
735+
fn return_ref_result() -> &'static Result<i32, i32> {
736+
&Ok(42)
737+
}
738+
739+
fn option_match(v: Option<usize>) -> Option<usize> {
740+
// `match` on a function returning `&Option` — needs `.as_ref()?`
741+
match return_ref_option() {
742+
//~^ question_mark
743+
Some(it) => {
744+
dbg!(it);
745+
},
746+
None => return None,
747+
};
748+
let _x: &i32 = match return_ref_option() {
749+
//~^ question_mark
750+
Some(it) => it,
751+
None => return None,
752+
};
753+
// `match` on a function returning `&mut Option` — needs `.as_mut()?`
754+
match return_ref_mut_option() {
755+
//~^ question_mark
756+
Some(it) => {
757+
*it += 1;
758+
},
759+
None => return None,
760+
};
761+
let _x: &mut i32 = match return_ref_mut_option() {
762+
//~^ question_mark
763+
Some(it) => it,
764+
None => return None,
765+
};
766+
// `match &v` still produces `&v?`, unchanged
767+
match &v {
768+
//~^ question_mark
769+
Some(n) => {
770+
println!("{n}");
771+
Some(42)
772+
},
773+
None => return None,
774+
};
775+
Some(42)
776+
}
777+
778+
fn result_match() -> Result<(), &'static i32> {
779+
// `match` on a function returning `&Result` — needs `.as_ref()?`
780+
match return_ref_result() {
781+
//~^ question_mark
782+
Ok(val) => {
783+
dbg!(val);
784+
},
785+
Err(e) => return Err(e),
786+
};
787+
let _x: &i32 = match return_ref_result() {
788+
//~^ question_mark
789+
Ok(val) => val,
790+
Err(e) => return Err(e),
791+
};
792+
Ok(())
793+
}
794+
}

‎tests/ui/question_mark.stderr‎

Lines changed: 115 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -516,5 +516,119 @@ LL + (a, b) = x
516516
LL + }
517517
|
518518

519-
error: aborting due to 47 previous errors
519+
error: this `match` expression can be replaced with `?`
520+
--> tests/ui/question_mark.rs:741:9
521+
|
522+
LL | / match return_ref_option() {
523+
LL | |
524+
LL | | Some(it) => {
525+
LL | | dbg!(it);
526+
LL | | },
527+
LL | | None => return None,
528+
LL | | };
529+
| |_________^
530+
|
531+
help: try instead
532+
|
533+
LL ~ {
534+
LL + let it = return_ref_option().as_ref()?;
535+
LL + dbg!(it);
536+
LL ~ };
537+
|
538+
539+
error: this `match` expression can be replaced with `?`
540+
--> tests/ui/question_mark.rs:748:24
541+
|
542+
LL | let _x: &i32 = match return_ref_option() {
543+
| ________________________^
544+
LL | |
545+
LL | | Some(it) => it,
546+
LL | | None => return None,
547+
LL | | };
548+
| |_________^ help: try instead: `return_ref_option().as_ref()?`
549+
550+
error: this `match` expression can be replaced with `?`
551+
--> tests/ui/question_mark.rs:754:9
552+
|
553+
LL | / match return_ref_mut_option() {
554+
LL | |
555+
LL | | Some(it) => {
556+
LL | | *it += 1;
557+
LL | | },
558+
LL | | None => return None,
559+
LL | | };
560+
| |_________^
561+
|
562+
help: try instead
563+
|
564+
LL ~ {
565+
LL + let it = return_ref_mut_option().as_mut()?;
566+
LL + *it += 1;
567+
LL ~ };
568+
|
569+
570+
error: this `match` expression can be replaced with `?`
571+
--> tests/ui/question_mark.rs:761:28
572+
|
573+
LL | let _x: &mut i32 = match return_ref_mut_option() {
574+
| ____________________________^
575+
LL | |
576+
LL | | Some(it) => it,
577+
LL | | None => return None,
578+
LL | | };
579+
| |_________^ help: try instead: `return_ref_mut_option().as_mut()?`
580+
581+
error: this `match` expression can be replaced with `?`
582+
--> tests/ui/question_mark.rs:767:9
583+
|
584+
LL | / match &v {
585+
LL | |
586+
LL | | Some(n) => {
587+
LL | | println!("{n}");
588+
... |
589+
LL | | None => return None,
590+
LL | | };
591+
| |_________^
592+
|
593+
help: try instead
594+
|
595+
LL ~ {
596+
LL + let n = &v?;
597+
LL + println!("{n}");
598+
LL + Some(42)
599+
LL ~ };
600+
|
601+
602+
error: this `match` expression can be replaced with `?`
603+
--> tests/ui/question_mark.rs:780:9
604+
|
605+
LL | / match return_ref_result() {
606+
LL | |
607+
LL | | Ok(val) => {
608+
LL | | dbg!(val);
609+
LL | | },
610+
LL | | Err(e) => return Err(e),
611+
LL | | };
612+
| |_________^
613+
|
614+
help: try instead
615+
|
616+
LL ~ {
617+
LL + let val = return_ref_result().as_ref()?;
618+
LL + dbg!(val);
619+
LL ~ };
620+
|
621+
622+
error: this `match` expression can be replaced with `?`
623+
--> tests/ui/question_mark.rs:787:24
624+
|
625+
LL | let _x: &i32 = match return_ref_result() {
626+
| ________________________^
627+
LL | |
628+
LL | | Ok(val) => val,
629+
LL | | Err(e) => return Err(e),
630+
LL | | };
631+
| |_________^ help: try instead: `return_ref_result().as_ref()?`
632+
633+
error: aborting due to 54 previous errors
520634

0 commit comments

Comments
 (0)