From 15499b160399d3e4442f48550aacf0fdf4948617 Mon Sep 17 00:00:00 2001 From: Veera Date: Sat, 13 Jul 2024 12:05:16 -0400 Subject: [PATCH] Suggest `impl Trait` for References to Bare Trait in Function Header --- compiler/rustc_hir/src/hir.rs | 14 +++ .../src/hir_ty_lowering/lint.rs | 117 ++++++++++++++---- .../src/hir_ty_lowering/mod.rs | 2 +- .../src/error_reporting/traits/suggestions.rs | 80 ++++++++++-- ...t-in-fn-inputs-and-outputs-issue-125139.rs | 2 +- 5 files changed, 173 insertions(+), 42 deletions(-) diff --git a/compiler/rustc_hir/src/hir.rs b/compiler/rustc_hir/src/hir.rs index 4561f9d9b49df..b99652743a64f 100644 --- a/compiler/rustc_hir/src/hir.rs +++ b/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 { use crate::intravisit::Visitor; struct MyVisitor(Vec); diff --git a/compiler/rustc_hir_analysis/src/hir_ty_lowering/lint.rs b/compiler/rustc_hir_analysis/src/hir_ty_lowering/lint.rs index 29c71c3fa50b5..6ac18394f6b44 100644 --- a/compiler/rustc_hir_analysis/src/hir_ty_lowering/lint.rs +++ b/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` // and suggest `Trait0`. + // 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,6 +176,12 @@ 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 { @@ -174,6 +189,8 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ { }; 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` 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` + // suggestion. + let suggestion = if borrowed { + vec![(ty.span, format!("Box"))] + } else { vec![ (ty.span.shrink_to_lo(), "Box".to_string()), - ], + ] + }; + + diag.multipart_suggestion_verbose( + "alternatively, you can return an owned trait object", + suggestion, Applicability::MachineApplicable, ); } else if is_downgradable { @@ -217,24 +276,24 @@ 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 { @@ -242,14 +301,18 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ { 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!( diff --git a/compiler/rustc_hir_analysis/src/hir_ty_lowering/mod.rs b/compiler/rustc_hir_analysis/src/hir_ty_lowering/mod.rs index a665306f2c6a8..085a3471ae29d 100644 --- a/compiler/rustc_hir_analysis/src/hir_ty_lowering/mod.rs +++ b/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, diff --git a/compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs b/compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs index 2cf808f962f08..29bc112258a2e 100644 --- a/compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs +++ b/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) -> Option>( + 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::>(); + + // 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::>(); - 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") } } diff --git a/tests/ui/object-safety/reference-to-bare-trait-in-fn-inputs-and-outputs-issue-125139.rs b/tests/ui/object-safety/reference-to-bare-trait-in-fn-inputs-and-outputs-issue-125139.rs index dceb6e0fe710d..dd90c001fd922 100644 --- a/tests/ui/object-safety/reference-to-bare-trait-in-fn-inputs-and-outputs-issue-125139.rs +++ b/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