Skip to content

Commit

Permalink
Rollup merge of rust-lang#127692 - veera-sivarajan:bugfix-125139, r=e…
Browse files Browse the repository at this point in the history
…stebank

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
  • Loading branch information
matthiaskrgr committed Sep 3, 2024
2 parents d6c8169 + 12de141 commit 4b38ef6
Show file tree
Hide file tree
Showing 5 changed files with 891 additions and 28 deletions.
78 changes: 55 additions & 23 deletions compiler/rustc_hir_analysis/src/hir_ty_lowering/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -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, ")
Expand All @@ -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 {
Expand All @@ -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!(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
Expand Down
10 changes: 9 additions & 1 deletion tests/ui/dyn-keyword/dyn-2021-edition-error.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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>) {
| +++
Expand Down
Original file line number Diff line number Diff line change
@@ -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() {}
Loading

0 comments on commit 4b38ef6

Please sign in to comment.