Suggest impl Trait for References to Bare Trait in Function Header

Author: veera-sivarajan

compiler/rustc_hir/src/hir.rs

@@ -2515,6 +2515,20 @@ impl<'hir> Ty<'hir> {
         final_ty
     }
 
+    /// Check whether type has an anonymous lifetime
+    pub fn has_anonymous_lifetime(&self) -> bool {
+        if let TyKind::Ref(lifetime, MutTy { mutbl: Mutability::Not, .. }) = self.kind {
+            lifetime.is_anonymous()
+        } else {
+            false
+        }
+    }
+
+    /// Check whether type is a mutable reference to some type
+    pub fn is_mut_ref(&self) -> bool {
+        matches!(self.kind, TyKind::Ref(_, MutTy { mutbl: Mutability::Mut, .. }))
+    }
+
     pub fn find_self_aliases(&self) -> Vec<Span> {
         use crate::intravisit::Visitor;
         struct MyVisitor(Vec<Span>);

compiler/rustc_hir_analysis/src/hir_ty_lowering/lint.rs

@@ -4,7 +4,9 @@ use rustc_hir as hir;
 use rustc_hir::def::{DefKind, Res};
 use rustc_lint_defs::{builtin::BARE_TRAIT_OBJECTS, Applicability};
 use rustc_span::Span;
-use rustc_trait_selection::error_reporting::traits::suggestions::NextTypeParamName;
+use rustc_trait_selection::error_reporting::traits::suggestions::{
+    NextLifetimeParamName, NextTypeParamName,
+};
 
 use super::HirTyLowerer;
 
@@ -17,6 +19,7 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
         &self,
         self_ty: &hir::Ty<'_>,
         in_path: bool,
+        borrowed: bool,
     ) {
         let tcx = self.tcx();
 
@@ -62,7 +65,7 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
             let mut diag =
                 rustc_errors::struct_span_code_err!(self.dcx(), self_ty.span, E0782, "{}", msg);
             if self_ty.span.can_be_used_for_suggestions()
-                && !self.maybe_suggest_impl_trait(self_ty, &mut diag)
+                && !self.maybe_suggest_impl_trait(self_ty, &mut diag, borrowed)
             {
                 // FIXME: Only emit this suggestion if the trait is object safe.
                 diag.multipart_suggestion_verbose(label, sugg, Applicability::MachineApplicable);
@@ -120,9 +123,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 \
@@ -152,11 +152,20 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
     }
 
     /// Make sure that we are in the condition to suggest `impl Trait`.
-    fn maybe_suggest_impl_trait(&self, self_ty: &hir::Ty<'_>, diag: &mut Diag<'_>) -> bool {
+    fn maybe_suggest_impl_trait(
+        &self,
+        self_ty: &hir::Ty<'_>,
+        diag: &mut Diag<'_>,
+        borrowed: bool,
+    ) -> bool {
         let tcx = self.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)
@@ -167,13 +176,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 {
@@ -189,8 +206,10 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
             }
             _ => false,
         };
+
+        // 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, ")
@@ -201,14 +220,54 @@ 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);
+
+            // Six different cases to consider when suggesting `impl Trait` for a return type:
+            // 1. `fn fun() -> Trait {}`         => Suggest `impl Trait` without mentioning anything about lifetime
+            // 2. `fn fun<'a>() -> &'a Trait {}` => Suggest `impl Trait` without mentioning anything about lifetime
+            // 3. `fn fun() -> &'a Trait {}`     => Suggest `impl Trait` and an other error (E0261) will suggest to declare the lifetime
+            // 4. `fn fun() -> &Trait {}`        => Suggest to declare and use a fresh lifetime
+            // 5. `fn fun<'a>() -> &Trait {}`    => Suggest to declare and use a fresh lifetime
+            // 6. `fn fun() -> &mut Trait {}`    => Suggest `impl Trait` and mention that returning a mutable reference to a bare trait is impossible
+            let suggestion = if ty.is_mut_ref() {
+                // case 6
+                diag.primary_message("cannot return a mutable reference to a bare trait");
+                vec![(ty.span, format!("impl {trait_name}"))]
+            } else if ty.has_anonymous_lifetime() {
+                // cases 4 and 5
+                let lifetime = generics.params.next_lifetime_param_name(None);
+
+                let lifetime_decl = if let Some(span) = generics.span_for_lifetime_suggestion() {
+                    (span, format!("{lifetime}, "))
+                } else {
+                    (generics.span, format!("<{lifetime}>"))
+                };
+
+                let impl_with_lifetime = (self_ty.span.shrink_to_lo(), format!("{lifetime} impl "));
+                vec![lifetime_decl, impl_with_lifetime]
+            } else {
+                // cases 1, 2, and 3
+                impl_sugg
+            };
+
+            diag.multipart_suggestion_verbose(msg, suggestion, 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 {
@@ -217,39 +276,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_hir_analysis/src/hir_ty_lowering/mod.rs

@@ -2078,7 +2078,7 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
                 )
             }
             hir::TyKind::TraitObject(bounds, lifetime, repr) => {
-                self.prohibit_or_lint_bare_trait_object_ty(hir_ty, in_path);
+                self.prohibit_or_lint_bare_trait_object_ty(hir_ty, in_path, borrowed);
 
                 let repr = match repr {
                     TraitObjectSyntax::Dyn | TraitObjectSyntax::None => ty::Dyn,

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

@@ -4925,29 +4925,83 @@ impl<'v> Visitor<'v> for AwaitsVisitor {
     }
 }
 
+/// Function to suggest a type or lifetime parameter.
+///
+/// `possible_names` is the list of names from which a name will be choosen.
+/// `used_names` is the list of `hir::GenericsParam`s already in use.
+/// `filter_fn` is a function to filter out the desired names from `used_names`.
+/// `default_name` is the name to suggest when all `possible_names` are in `used_names`.
+fn next_param_name<F: FnMut(&hir::GenericParam<'_>) -> Option<Symbol>>(
+    possible_names: [&str; 10],
+    used_names: &[hir::GenericParam<'_>],
+    filter_fn: F,
+    default_name: &str,
+) -> String {
+    // Filter out used names based on `filter_fn`.
+    let used_names = used_names.iter().filter_map(filter_fn).collect::<Vec<_>>();
+
+    // Find a name from `possible_names` that is not in `used_names`.
+    possible_names
+        .iter()
+        .find(|n| !used_names.contains(&Symbol::intern(n)))
+        .unwrap_or(&default_name)
+        .to_string()
+}
+
+/// 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
-            .iter()
-            .filter_map(|p| match p.name {
-                hir::ParamName::Plain(ident) => Some(ident.name),
-                _ => None,
-            })
-            .collect::<Vec<_>>();
 
-        possible_names
-            .iter()
-            .find(|n| !used_names.contains(&Symbol::intern(n)))
-            .unwrap_or(&"ParamName")
-            .to_string()
+        // Filter out the existing type parameter names.
+        let filter_fn = |p: &hir::GenericParam<'_>| match p.name {
+            hir::ParamName::Plain(ident) => Some(ident.name),
+            _ => None,
+        };
+        next_param_name(possible_names, self, filter_fn, "ParamName")
+    }
+}
+
+/// Suggest a new lifetime parameter name for diagnostic purposes.
+///
+/// `name` is the preferred name you'd like to suggest if it's not in use already.
+/// Note: `name`, if provided, should begin with an apostrophe and followed by a lifetime name.
+/// The output string will also begin with an apostrophe and follwed by a lifetime name.
+pub trait NextLifetimeParamName {
+    fn next_lifetime_param_name(&self, name: Option<&str>) -> String;
+}
+
+impl NextLifetimeParamName for &[hir::GenericParam<'_>] {
+    fn next_lifetime_param_name(&self, name: Option<&str>) -> String {
+        // Lifetimes are usually in lowercase. So transform input string to lowercase.
+        let name = name.map(|n| n.to_lowercase());
+        let name = name.as_deref();
+        // This is the list of possible lifetime names that we might suggest.
+        let possible_names =
+            [name.unwrap_or("'a"), "'a", "'b", "'c", "'d", "'e", "'f", "'g", "'h", "'i"];
+        // Filter out the existing lifetime names
+        let filter_fn = |p: &hir::GenericParam<'_>| {
+            if matches!(p.kind, hir::GenericParamKind::Lifetime { .. }) {
+                match p.name {
+                    hir::ParamName::Plain(ident) => Some(ident.name),
+                    _ => None,
+                }
+            } else {
+                None
+            }
+        };
+        next_param_name(possible_names, self, filter_fn, "LifetimeName")
     }
 }
 

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

@@ -91,7 +91,7 @@ trait Sing {
         //~^ ERROR: cannot return reference to temporary value
     }
 }
-    
+
 fn foo(_: &Trait) {}
 //~^ ERROR: trait objects must include the `dyn` keyword