Rollup merge of rust-lang#127692 - veera-sivarajan:bugfix-125139, r=estebank

Suggest `impl Trait` for References to Bare Trait in Function Header

Fixes rust-lang#125139

This PR suggests `impl Trait` when `&Trait` is found as a function parameter type or return type. This makes use of existing diagnostics by adding `peel_refs()` when checking for type equality.

Additionaly, it makes a few other improvements:
1. Checks if functions inside impl blocks have bare trait in their headers.
2. Introduces a trait `NextLifetimeParamName` similar to the existing `NextTypeParamName` for suggesting a lifetime name. Also, abstracts out the common logic between the two trait impls.

### Related Issues
I ran into a bunch of related diagnostic issues but couldn't fix them within the scope of this PR. So, I have created the following issues:
1. [Misleading Suggestion when Returning a Reference to a Bare Trait from a Function](rust-lang#127689)
2. [Verbose Error When a Function Takes a Bare Trait as Parameter](rust-lang#127690)
3. [Incorrect Suggestion when Returning a Bare Trait from a Function](rust-lang#127691)

r​? `@estebank` since you implemented  rust-lang#119148

Author: matthiaskrgr

compiler/rustc_hir_analysis/src/hir_ty_lowering/lint.rs

@@ -133,9 +133,6 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
                 return;
             };
             let sugg = self.add_generic_param_suggestion(generics, self_ty.span, &impl_trait_name);
-            if sugg.is_empty() {
-                return;
-            };
             diag.multipart_suggestion(
                 format!(
                     "alternatively use a blanket implementation to implement `{of_trait_name}` for \
@@ -170,6 +167,10 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
         let parent_id = tcx.hir().get_parent_item(self_ty.hir_id).def_id;
         // FIXME: If `type_alias_impl_trait` is enabled, also look for `Trait0<Ty = Trait1>`
         //        and suggest `Trait0<Ty = impl Trait1>`.
+        // Functions are found in three different contexts.
+        // 1. Independent functions
+        // 2. Functions inside trait blocks
+        // 3. Functions inside impl blocks
         let (sig, generics, owner) = match tcx.hir_node_by_def_id(parent_id) {
             hir::Node::Item(hir::Item { kind: hir::ItemKind::Fn(sig, generics, _), .. }) => {
                 (sig, generics, None)
@@ -180,13 +181,21 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
                 owner_id,
                 ..
             }) => (sig, generics, Some(tcx.parent(owner_id.to_def_id()))),
+            hir::Node::ImplItem(hir::ImplItem {
+                kind: hir::ImplItemKind::Fn(sig, _),
+                generics,
+                owner_id,
+                ..
+            }) => (sig, generics, Some(tcx.parent(owner_id.to_def_id()))),
             _ => return false,
         };
         let Ok(trait_name) = tcx.sess.source_map().span_to_snippet(self_ty.span) else {
             return false;
         };
         let impl_sugg = vec![(self_ty.span.shrink_to_lo(), "impl ".to_string())];
         let mut is_downgradable = true;
+
+        // Check if trait object is safe for suggesting dynamic dispatch.
         let is_object_safe = match self_ty.kind {
             hir::TyKind::TraitObject(objects, ..) => {
                 objects.iter().all(|(o, _)| match o.trait_ref.path.res {
@@ -202,8 +211,15 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
             }
             _ => false,
         };
+
+        let borrowed = matches!(
+            tcx.parent_hir_node(self_ty.hir_id),
+            hir::Node::Ty(hir::Ty { kind: hir::TyKind::Ref(..), .. })
+        );
+
+        // Suggestions for function return type.
         if let hir::FnRetTy::Return(ty) = sig.decl.output
-            && ty.hir_id == self_ty.hir_id
+            && ty.peel_refs().hir_id == self_ty.hir_id
         {
             let pre = if !is_object_safe {
                 format!("`{trait_name}` is not object safe, ")
@@ -214,14 +230,26 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
                 "{pre}use `impl {trait_name}` to return an opaque type, as long as you return a \
                  single underlying type",
             );
+
             diag.multipart_suggestion_verbose(msg, impl_sugg, Applicability::MachineApplicable);
+
+            // Suggest `Box<dyn Trait>` for return type
             if is_object_safe {
-                diag.multipart_suggestion_verbose(
-                    "alternatively, you can return an owned trait object",
+                // If the return type is `&Trait`, we don't want
+                // the ampersand to be displayed in the `Box<dyn Trait>`
+                // suggestion.
+                let suggestion = if borrowed {
+                    vec![(ty.span, format!("Box<dyn {trait_name}>"))]
+                } else {
                     vec![
                         (ty.span.shrink_to_lo(), "Box<dyn ".to_string()),
                         (ty.span.shrink_to_hi(), ">".to_string()),
-                    ],
+                    ]
+                };
+
+                diag.multipart_suggestion_verbose(
+                    "alternatively, you can return an owned trait object",
+                    suggestion,
                     Applicability::MachineApplicable,
                 );
             } else if is_downgradable {
@@ -230,39 +258,43 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
             }
             return true;
         }
+
+        // Suggestions for function parameters.
         for ty in sig.decl.inputs {
-            if ty.hir_id != self_ty.hir_id {
+            if ty.peel_refs().hir_id != self_ty.hir_id {
                 continue;
             }
             let sugg = self.add_generic_param_suggestion(generics, self_ty.span, &trait_name);
-            if !sugg.is_empty() {
-                diag.multipart_suggestion_verbose(
-                    format!("use a new generic type parameter, constrained by `{trait_name}`"),
-                    sugg,
-                    Applicability::MachineApplicable,
-                );
-                diag.multipart_suggestion_verbose(
-                    "you can also use an opaque type, but users won't be able to specify the type \
-                     parameter when calling the `fn`, having to rely exclusively on type inference",
-                    impl_sugg,
-                    Applicability::MachineApplicable,
-                );
-            }
+            diag.multipart_suggestion_verbose(
+                format!("use a new generic type parameter, constrained by `{trait_name}`"),
+                sugg,
+                Applicability::MachineApplicable,
+            );
+            diag.multipart_suggestion_verbose(
+                "you can also use an opaque type, but users won't be able to specify the type \
+                 parameter when calling the `fn`, having to rely exclusively on type inference",
+                impl_sugg,
+                Applicability::MachineApplicable,
+            );
             if !is_object_safe {
                 diag.note(format!("`{trait_name}` it is not object safe, so it can't be `dyn`"));
                 if is_downgradable {
                     // We'll emit the object safety error already, with a structured suggestion.
                     diag.downgrade_to_delayed_bug();
                 }
             } else {
+                // No ampersand in suggestion if it's borrowed already
+                let (dyn_str, paren_dyn_str) =
+                    if borrowed { ("dyn ", "(dyn ") } else { ("&dyn ", "&(dyn ") };
+
                 let sugg = if let hir::TyKind::TraitObject([_, _, ..], _, _) = self_ty.kind {
                     // There are more than one trait bound, we need surrounding parentheses.
                     vec![
-                        (self_ty.span.shrink_to_lo(), "&(dyn ".to_string()),
+                        (self_ty.span.shrink_to_lo(), paren_dyn_str.to_string()),
                         (self_ty.span.shrink_to_hi(), ")".to_string()),
                     ]
                 } else {
-                    vec![(self_ty.span.shrink_to_lo(), "&dyn ".to_string())]
+                    vec![(self_ty.span.shrink_to_lo(), dyn_str.to_string())]
                 };
                 diag.multipart_suggestion_verbose(
                     format!(

compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs

@@ -5023,24 +5023,32 @@ impl<'v> Visitor<'v> for AwaitsVisitor {
     }
 }
 
+/// Suggest a new type parameter name for diagnostic purposes.
+///
+/// `name` is the preferred name you'd like to suggest if it's not in use already.
 pub trait NextTypeParamName {
     fn next_type_param_name(&self, name: Option<&str>) -> String;
 }
 
 impl NextTypeParamName for &[hir::GenericParam<'_>] {
     fn next_type_param_name(&self, name: Option<&str>) -> String {
-        // This is the list of possible parameter names that we might suggest.
+        // Type names are usually single letters in uppercase. So convert the first letter of input string to uppercase.
         let name = name.and_then(|n| n.chars().next()).map(|c| c.to_uppercase().to_string());
         let name = name.as_deref();
+
+        // This is the list of possible parameter names that we might suggest.
         let possible_names = [name.unwrap_or("T"), "T", "U", "V", "X", "Y", "Z", "A", "B", "C"];
-        let used_names = self
+
+        // Filter out used names based on `filter_fn`.
+        let used_names: Vec<Symbol> = self
             .iter()
-            .filter_map(|p| match p.name {
+            .filter_map(|param| match param.name {
                 hir::ParamName::Plain(ident) => Some(ident.name),
                 _ => None,
             })
-            .collect::<Vec<_>>();
+            .collect();
 
+        // Find a name from `possible_names` that is not in `used_names`.
         possible_names
             .iter()
             .find(|n| !used_names.contains(&Symbol::intern(n)))

tests/ui/dyn-keyword/dyn-2021-edition-error.stderr

@@ -4,7 +4,15 @@ error[E0782]: trait objects must include the `dyn` keyword
 LL | fn function(x: &SomeTrait, y: Box<SomeTrait>) {
    |                 ^^^^^^^^^
    |
-help: add `dyn` keyword before this trait
+help: use a new generic type parameter, constrained by `SomeTrait`
+   |
+LL | fn function<T: SomeTrait>(x: &T, y: Box<SomeTrait>) {
+   |            ++++++++++++++     ~
+help: you can also use an opaque type, but users won't be able to specify the type parameter when calling the `fn`, having to rely exclusively on type inference
+   |
+LL | fn function(x: &impl SomeTrait, y: Box<SomeTrait>) {
+   |                 ++++
+help: alternatively, use a trait object to accept any type that implements `SomeTrait`, accessing its methods at runtime using dynamic dispatch
    |
 LL | fn function(x: &dyn SomeTrait, y: Box<SomeTrait>) {
    |                 +++

tests/ui/object-safety/reference-to-bare-trait-in-fn-inputs-and-outputs-issue-125139.rs

@@ -0,0 +1,142 @@
+//@ edition:2021
+
+trait Trait {}
+
+struct IceCream;
+
+impl IceCream {
+    fn foo(_: &Trait) {}
+    //~^ ERROR: trait objects must include the `dyn` keyword
+
+    fn bar(self, _: &'a Trait) {}
+    //~^ ERROR: trait objects must include the `dyn` keyword
+    //~| ERROR: use of undeclared lifetime name
+
+    fn alice<'a>(&self, _: &Trait) {}
+    //~^ ERROR: trait objects must include the `dyn` keyword
+
+    fn bob<'a>(_: &'a Trait) {}
+    //~^ ERROR: trait objects must include the `dyn` keyword
+
+    fn cat() -> &Trait {
+        //~^ ERROR: missing lifetime specifier
+        //~| ERROR: trait objects must include the `dyn` keyword
+        &Type
+    }
+
+    fn dog<'a>() -> &Trait {
+        //~^ ERROR: missing lifetime specifier
+        //~| ERROR: trait objects must include the `dyn` keyword
+        &Type
+    }
+
+    fn kitten() -> &'a Trait {
+        //~^ ERROR: use of undeclared lifetime name
+        //~| ERROR: trait objects must include the `dyn` keyword
+        &Type
+    }
+
+    fn puppy<'a>() -> &'a Trait {
+        //~^ ERROR: trait objects must include the `dyn` keyword
+        &Type
+    }
+
+    fn parrot() -> &mut Trait {
+        //~^ ERROR: missing lifetime specifier
+        //~| ERROR: trait objects must include the `dyn` keyword
+        &mut Type
+        //~^ ERROR: cannot return reference to temporary value
+    }
+}
+
+trait Sing {
+    fn foo(_: &Trait);
+    //~^ ERROR: trait objects must include the `dyn` keyword
+
+    fn bar(_: &'a Trait);
+    //~^ ERROR: trait objects must include the `dyn` keyword
+    //~| ERROR: use of undeclared lifetime name
+
+    fn alice<'a>(_: &Trait);
+    //~^ ERROR: trait objects must include the `dyn` keyword
+
+    fn bob<'a>(_: &'a Trait);
+    //~^ ERROR: trait objects must include the `dyn` keyword
+
+    fn cat() -> &Trait;
+        //~^ ERROR: missing lifetime specifier
+        //~| ERROR: trait objects must include the `dyn` keyword
+
+    fn dog<'a>() -> &Trait {
+        //~^ ERROR: missing lifetime specifier
+        //~| ERROR: trait objects must include the `dyn` keyword
+        &Type
+    }
+
+    fn kitten() -> &'a Trait {
+        //~^ ERROR: use of undeclared lifetime name
+        //~| ERROR: trait objects must include the `dyn` keyword
+        &Type
+    }
+
+    fn puppy<'a>() -> &'a Trait {
+        //~^ ERROR: trait objects must include the `dyn` keyword
+        &Type
+    }
+
+    fn parrot() -> &mut Trait {
+        //~^ ERROR: missing lifetime specifier
+        //~| ERROR: trait objects must include the `dyn` keyword
+        &mut Type
+        //~^ ERROR: cannot return reference to temporary value
+    }
+}
+
+fn foo(_: &Trait) {}
+//~^ ERROR: trait objects must include the `dyn` keyword
+
+fn bar(_: &'a Trait) {}
+//~^ ERROR: trait objects must include the `dyn` keyword
+//~| ERROR: use of undeclared lifetime name
+
+fn alice<'a>(_: &Trait) {}
+//~^ ERROR: trait objects must include the `dyn` keyword
+
+fn bob<'a>(_: &'a Trait) {}
+//~^ ERROR: trait objects must include the `dyn` keyword
+
+struct Type;
+
+impl Trait for Type {}
+
+fn cat() -> &Trait {
+//~^ ERROR: missing lifetime specifier
+//~| ERROR: trait objects must include the `dyn` keyword
+    &Type
+}
+
+fn dog<'a>() -> &Trait {
+//~^ ERROR: missing lifetime specifier
+//~| ERROR: trait objects must include the `dyn` keyword
+    &Type
+}
+
+fn kitten() -> &'a Trait {
+//~^ ERROR: use of undeclared lifetime name
+//~| ERROR: trait objects must include the `dyn` keyword
+    &Type
+}
+
+fn puppy<'a>() -> &'a Trait {
+//~^ ERROR: trait objects must include the `dyn` keyword
+    &Type
+}
+
+fn parrot() -> &mut Trait {
+    //~^ ERROR: missing lifetime specifier
+    //~| ERROR: trait objects must include the `dyn` keyword
+    &mut Type
+    //~^ ERROR: cannot return reference to temporary value
+}
+
+fn main() {}