Auto merge of #114150 - clubby789:improve-option-ref-suggestion, r=WaffleLapkin

Refactor + improve diagnostics for `&mut T`/`T` mismatch inside Option/Result

Follow up to #114052. This also makes the diagnostics structured + translatable.

r? `@WaffleLapkin`

Author: bors

compiler/rustc_hir_typeck/messages.ftl

@@ -77,6 +77,10 @@ hir_typeck_note_edition_guide = for more on editions, read https://doc.rust-lang
 
 hir_typeck_op_trait_generic_params = `{$method_name}` must not have any generic parameters
 
+hir_typeck_option_result_asref = use `{$def_path}::as_ref` to convert `{$expected_ty}` to `{$expr_ty}`
+hir_typeck_option_result_cloned = use `{$def_path}::cloned` to clone the value inside the `{$def_path}`
+hir_typeck_option_result_copied = use `{$def_path}::copied` to copy the value inside the `{$def_path}`
+
 hir_typeck_return_stmt_outside_of_fn_body =
     {$statement_kind} statement outside of function body
     .encl_body_label = the {$statement_kind} is part of this body...

compiler/rustc_hir_typeck/src/errors.rs

@@ -252,6 +252,45 @@ impl HelpUseLatestEdition {
     }
 }
 
+#[derive(Subdiagnostic)]
+pub enum OptionResultRefMismatch<'tcx> {
+    #[suggestion(
+        hir_typeck_option_result_copied,
+        code = ".copied()",
+        style = "verbose",
+        applicability = "machine-applicable"
+    )]
+    Copied {
+        #[primary_span]
+        span: Span,
+        def_path: String,
+    },
+    #[suggestion(
+        hir_typeck_option_result_cloned,
+        code = ".cloned()",
+        style = "verbose",
+        applicability = "machine-applicable"
+    )]
+    Cloned {
+        #[primary_span]
+        span: Span,
+        def_path: String,
+    },
+    #[suggestion(
+        hir_typeck_option_result_asref,
+        code = ".as_ref()",
+        style = "verbose",
+        applicability = "machine-applicable"
+    )]
+    AsRef {
+        #[primary_span]
+        span: Span,
+        def_path: String,
+        expected_ty: Ty<'tcx>,
+        expr_ty: Ty<'tcx>,
+    },
+}
+
 #[derive(Diagnostic)]
 #[diag(hir_typeck_const_select_must_be_const)]
 #[help]

compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs

@@ -1,8 +1,6 @@
 use super::FnCtxt;
 
-use crate::errors::{
-    AddReturnTypeSuggestion, ExpectedReturnTypeLabel, SuggestBoxing, SuggestConvertViaMethod,
-};
+use crate::errors;
 use crate::fluent_generated as fluent;
 use crate::method::probe::{IsSuggestion, Mode, ProbeScope};
 use rustc_ast::util::parser::{ExprPrecedence, PREC_POSTFIX};
@@ -434,7 +432,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             // FIXME: This could/should be extended to suggest `as_mut` and `as_deref_mut`,
             // but those checks need to be a bit more delicate and the benefit is diminishing.
             if self.can_eq(self.param_env, found_ty_inner, peeled) && error_tys_equate_as_ref {
-                err.subdiagnostic(SuggestConvertViaMethod {
+                err.subdiagnostic(errors::SuggestConvertViaMethod {
                     span: expr.span.shrink_to_hi(),
                     sugg: ".as_ref()",
                     expected,
@@ -447,7 +445,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 && self.can_eq(self.param_env, deref_ty, peeled)
                 && error_tys_equate_as_ref
             {
-                err.subdiagnostic(SuggestConvertViaMethod {
+                err.subdiagnostic(errors::SuggestConvertViaMethod {
                     span: expr.span.shrink_to_hi(),
                     sugg: ".as_deref()",
                     expected,
@@ -521,17 +519,20 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         if self.can_coerce(Ty::new_box(self.tcx, found), expected) {
             let suggest_boxing = match found.kind() {
                 ty::Tuple(tuple) if tuple.is_empty() => {
-                    SuggestBoxing::Unit { start: span.shrink_to_lo(), end: span }
+                    errors::SuggestBoxing::Unit { start: span.shrink_to_lo(), end: span }
                 }
                 ty::Generator(def_id, ..)
                     if matches!(
                         self.tcx.generator_kind(def_id),
                         Some(GeneratorKind::Async(AsyncGeneratorKind::Closure))
                     ) =>
                 {
-                    SuggestBoxing::AsyncBody
+                    errors::SuggestBoxing::AsyncBody
                 }
-                _ => SuggestBoxing::Other { start: span.shrink_to_lo(), end: span.shrink_to_hi() },
+                _ => errors::SuggestBoxing::Other {
+                    start: span.shrink_to_lo(),
+                    end: span.shrink_to_hi(),
+                },
             };
             err.subdiagnostic(suggest_boxing);
 
@@ -756,23 +757,23 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         match &fn_decl.output {
             &hir::FnRetTy::DefaultReturn(span) if expected.is_unit() && !can_suggest => {
                 // `fn main()` must return `()`, do not suggest changing return type
-                err.subdiagnostic(ExpectedReturnTypeLabel::Unit { span });
+                err.subdiagnostic(errors::ExpectedReturnTypeLabel::Unit { span });
                 return true;
             }
             &hir::FnRetTy::DefaultReturn(span) if expected.is_unit() => {
                 if let Some(found) = found.make_suggestable(self.tcx, false) {
-                    err.subdiagnostic(AddReturnTypeSuggestion::Add { span, found: found.to_string() });
+                    err.subdiagnostic(errors::AddReturnTypeSuggestion::Add { span, found: found.to_string() });
                     return true;
                 } else if let ty::Closure(_, args) = found.kind()
                     // FIXME(compiler-errors): Get better at printing binders...
                     && let closure = args.as_closure()
                     && closure.sig().is_suggestable(self.tcx, false)
                 {
-                    err.subdiagnostic(AddReturnTypeSuggestion::Add { span, found: closure.print_as_impl_trait().to_string() });
+                    err.subdiagnostic(errors::AddReturnTypeSuggestion::Add { span, found: closure.print_as_impl_trait().to_string() });
                     return true;
                 } else {
                     // FIXME: if `found` could be `impl Iterator` we should suggest that.
-                    err.subdiagnostic(AddReturnTypeSuggestion::MissingHere { span });
+                    err.subdiagnostic(errors::AddReturnTypeSuggestion::MissingHere { span });
                     return true
                 }
             }
@@ -794,10 +795,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                     debug!(?found);
                     if found.is_suggestable(self.tcx, false) {
                         if term.span.is_empty() {
-                            err.subdiagnostic(AddReturnTypeSuggestion::Add { span, found: found.to_string() });
+                            err.subdiagnostic(errors::AddReturnTypeSuggestion::Add { span, found: found.to_string() });
                             return true;
                         } else {
-                            err.subdiagnostic(ExpectedReturnTypeLabel::Other { span, expected });
+                            err.subdiagnostic(errors::ExpectedReturnTypeLabel::Other { span, expected });
                         }
                     }
                 }
@@ -813,7 +814,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 let ty = self.normalize(span, ty);
                 let ty = self.tcx.erase_late_bound_regions(ty);
                 if self.can_coerce(expected, ty) {
-                    err.subdiagnostic(ExpectedReturnTypeLabel::Other { span, expected });
+                    err.subdiagnostic(errors::ExpectedReturnTypeLabel::Other { span, expected });
                     self.try_suggest_return_impl_trait(err, expected, ty, fn_id);
                     return true;
                 }
@@ -1103,65 +1104,46 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             return false;
         }
 
-        let mut suggest_copied_cloned_or_as_ref = || {
+        if Some(adt_def.did()) == self.tcx.get_diagnostic_item(sym::Result)
+            && self.can_eq(self.param_env, args.type_at(1), expected_args.type_at(1))
+            || Some(adt_def.did()) == self.tcx.get_diagnostic_item(sym::Option)
+        {
             let expr_inner_ty = args.type_at(0);
             let expected_inner_ty = expected_args.type_at(0);
-            if let &ty::Ref(_, ty, hir::Mutability::Not) = expr_inner_ty.kind()
-                && self.can_eq(self.param_env, ty, expected_inner_ty)
-            {
-                let def_path = self.tcx.def_path_str(adt_def.did());
-                if self.type_is_copy_modulo_regions(self.param_env, ty) {
-                    diag.span_suggestion_verbose(
-                        expr.span.shrink_to_hi(),
-                        format!(
-                            "use `{def_path}::copied` to copy the value inside the `{def_path}`"
-                        ),
-                        ".copied()",
-                        Applicability::MachineApplicable,
-                    );
-                    return true;
-                } else if let Some(expected_ty_expr) = expected_ty_expr {
-                    diag.span_suggestion_verbose(
-                        expected_ty_expr.span.shrink_to_hi(),
-                        format!(
-                            "use `{def_path}::as_ref()` to convert `{expected_ty}` to `{expr_ty}`"
-                        ),
-                        ".as_ref()",
-                        Applicability::MachineApplicable,
-                    );
-                    return true;
-                } else if let Some(clone_did) = self.tcx.lang_items().clone_trait()
-                    && rustc_trait_selection::traits::type_known_to_meet_bound_modulo_regions(
-                        self,
-                        self.param_env,
-                        ty,
-                        clone_did,
-                    )
+            if let &ty::Ref(_, ty, mutability) = expr_inner_ty.kind()
+                    && self.can_eq(self.param_env, ty, expected_inner_ty)
                 {
-                    diag.span_suggestion_verbose(
-                        expr.span.shrink_to_hi(),
-                        format!(
-                            "use `{def_path}::cloned` to clone the value inside the `{def_path}`"
-                        ),
-                        ".cloned()",
-                        Applicability::MachineApplicable,
-                    );
+                    let def_path = self.tcx.def_path_str(adt_def.did());
+                    let span = expr.span.shrink_to_hi();
+                    let subdiag = if self.type_is_copy_modulo_regions(self.param_env, ty) {
+                        errors::OptionResultRefMismatch::Copied {
+                            span, def_path
+                        }
+                    } else if let Some(expected_ty_expr) = expected_ty_expr
+                            // FIXME: suggest changes to both expressions to convert both to
+                            // Option/Result<&T>
+                            && mutability.is_not()
+                        {
+                        errors::OptionResultRefMismatch::AsRef {
+                            span: expected_ty_expr.span.shrink_to_hi(), expected_ty, expr_ty, def_path
+                        }
+                    } else if let Some(clone_did) = self.tcx.lang_items().clone_trait()
+                        && rustc_trait_selection::traits::type_known_to_meet_bound_modulo_regions(
+                            self,
+                            self.param_env,
+                            ty,
+                            clone_did,
+                        )
+                    {
+                        errors::OptionResultRefMismatch::Cloned {
+                            span, def_path
+                        }
+                    } else {
+                        return false;
+                    };
+                    diag.subdiagnostic(subdiag);
                     return true;
                 }
-            }
-            false
-        };
-
-        if let Some(result_did) = self.tcx.get_diagnostic_item(sym::Result)
-            && adt_def.did() == result_did
-            // Check that the error types are equal
-            && self.can_eq(self.param_env, args.type_at(1), expected_args.type_at(1))
-        {
-            return suggest_copied_cloned_or_as_ref();
-        } else if let Some(option_did) = self.tcx.get_diagnostic_item(sym::Option)
-            && adt_def.did() == option_did
-        {
-            return suggest_copied_cloned_or_as_ref();
         }
 
         false

tests/ui/associated-types/dont-suggest-cyclic-constraint.stderr

@@ -6,7 +6,7 @@ LL |     debug_assert_eq!(iter.next(), Some(value));
    |
    = note: expected enum `Option<<I as Iterator>::Item>`
               found enum `Option<&<I as Iterator>::Item>`
-help: use `Option::as_ref()` to convert `Option<<I as Iterator>::Item>` to `Option<&<I as Iterator>::Item>`
+help: use `Option::as_ref` to convert `Option<<I as Iterator>::Item>` to `Option<&<I as Iterator>::Item>`
    |
 LL |     debug_assert_eq!(iter.next().as_ref(), Some(value));
    |                                 +++++++++

tests/ui/suggestions/copied-and-cloned.fixed

@@ -26,6 +26,20 @@ fn main() {
     let y = Some(&s);
     println!("{}", x.as_ref() == y);
     //~^ ERROR mismatched types
-    //~| HELP use `Option::as_ref()` to convert `Option<String>` to `Option<&String>`
+    //~| HELP use `Option::as_ref` to convert `Option<String>` to `Option<&String>`
 
+
+    let mut s = ();
+    let x = Some(s);
+    let y = Some(&mut s);
+    println!("{}", x == y.copied());
+    //~^ ERROR mismatched types
+    //~| HELP use `Option::copied` to copy the value inside the `Option`
+
+    let mut s = String::new();
+    let x = Some(s.clone());
+    let y = Some(&mut s);
+    println!("{}", x == y.cloned());
+    //~^ ERROR mismatched types
+    //~| HELP use `Option::cloned` to clone the value inside the `Option`
 }

tests/ui/suggestions/copied-and-cloned.rs

@@ -26,6 +26,20 @@ fn main() {
     let y = Some(&s);
     println!("{}", x == y);
     //~^ ERROR mismatched types
-    //~| HELP use `Option::as_ref()` to convert `Option<String>` to `Option<&String>`
+    //~| HELP use `Option::as_ref` to convert `Option<String>` to `Option<&String>`
 
+
+    let mut s = ();
+    let x = Some(s);
+    let y = Some(&mut s);
+    println!("{}", x == y);
+    //~^ ERROR mismatched types
+    //~| HELP use `Option::copied` to copy the value inside the `Option`
+
+    let mut s = String::new();
+    let x = Some(s.clone());
+    let y = Some(&mut s);
+    println!("{}", x == y);
+    //~^ ERROR mismatched types
+    //~| HELP use `Option::cloned` to clone the value inside the `Option`
 }

tests/ui/suggestions/copied-and-cloned.stderr

@@ -86,11 +86,37 @@ LL |     println!("{}", x == y);
    |
    = note: expected enum `Option<String>`
               found enum `Option<&String>`
-help: use `Option::as_ref()` to convert `Option<String>` to `Option<&String>`
+help: use `Option::as_ref` to convert `Option<String>` to `Option<&String>`
    |
 LL |     println!("{}", x.as_ref() == y);
    |                     +++++++++
 
-error: aborting due to 5 previous errors
+error[E0308]: mismatched types
+  --> $DIR/copied-and-cloned.rs:35:25
+   |
+LL |     println!("{}", x == y);
+   |                         ^ expected `Option<()>`, found `Option<&mut ()>`
+   |
+   = note: expected enum `Option<()>`
+              found enum `Option<&mut ()>`
+help: use `Option::copied` to copy the value inside the `Option`
+   |
+LL |     println!("{}", x == y.copied());
+   |                          +++++++++
+
+error[E0308]: mismatched types
+  --> $DIR/copied-and-cloned.rs:42:25
+   |
+LL |     println!("{}", x == y);
+   |                         ^ expected `Option<String>`, found `Option<&mut String>`
+   |
+   = note: expected enum `Option<String>`
+              found enum `Option<&mut String>`
+help: use `Option::cloned` to clone the value inside the `Option`
+   |
+LL |     println!("{}", x == y.cloned());
+   |                          +++++++++
+
+error: aborting due to 7 previous errors
 
 For more information about this error, try `rustc --explain E0308`.