-
Notifications
You must be signed in to change notification settings - Fork 13.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix dyn -> param suggestion in struct ICEs #138238
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,13 +78,16 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ { | |
} | ||
|
||
if self_ty.span.edition().at_least_rust_2021() { | ||
let msg = "expected a type, found a trait"; | ||
let label = "you can add the `dyn` keyword if you want a trait object"; | ||
let mut diag = | ||
rustc_errors::struct_span_code_err!(self.dcx(), self_ty.span, E0782, "{}", msg); | ||
let mut diag = rustc_errors::struct_span_code_err!( | ||
self.dcx(), | ||
self_ty.span, | ||
E0782, | ||
"{}", | ||
"expected a type, found a trait" | ||
); | ||
if self_ty.span.can_be_used_for_suggestions() | ||
&& !self.maybe_suggest_impl_trait(self_ty, &mut diag) | ||
&& !self.maybe_suggest_dyn_trait(self_ty, label, sugg, &mut diag) | ||
&& !self.maybe_suggest_dyn_trait(self_ty, sugg, &mut diag) | ||
{ | ||
self.maybe_suggest_add_generic_impl_trait(self_ty, &mut diag); | ||
} | ||
|
@@ -123,31 +126,62 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ { | |
} | ||
} | ||
|
||
/// For a struct or enum with an invalid bare trait object field, suggest turning | ||
/// it into a generic type bound. | ||
fn maybe_suggest_add_generic_impl_trait( | ||
&self, | ||
self_ty: &hir::Ty<'_>, | ||
diag: &mut Diag<'_>, | ||
) -> bool { | ||
let tcx = self.tcx(); | ||
let msg = "you might be missing a type parameter"; | ||
let mut sugg = vec![]; | ||
|
||
let parent_id = tcx.hir_get_parent_item(self_ty.hir_id).def_id; | ||
let parent_item = tcx.hir_node_by_def_id(parent_id).expect_item(); | ||
match parent_item.kind { | ||
hir::ItemKind::Struct(_, generics) | hir::ItemKind::Enum(_, generics) => { | ||
sugg.push(( | ||
generics.where_clause_span, | ||
format!( | ||
"<T: {}>", | ||
self.tcx().sess.source_map().span_to_snippet(self_ty.span).unwrap() | ||
), | ||
)); | ||
sugg.push((self_ty.span, "T".to_string())); | ||
let parent_hir_id = tcx.parent_hir_id(self_ty.hir_id); | ||
let parent_item = tcx.hir_get_parent_item(self_ty.hir_id).def_id; | ||
|
||
let generics = match tcx.hir_node_by_def_id(parent_item) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a check that we're suggesting this on a field directly. |
||
hir::Node::Item(hir::Item { | ||
kind: hir::ItemKind::Struct(variant, generics), .. | ||
}) => { | ||
if !variant.fields().iter().any(|field| field.hir_id == parent_hir_id) { | ||
return false; | ||
} | ||
generics | ||
} | ||
_ => {} | ||
hir::Node::Item(hir::Item { kind: hir::ItemKind::Enum(def, generics), .. }) => { | ||
if !def | ||
.variants | ||
.iter() | ||
.flat_map(|variant| variant.data.fields().iter()) | ||
.any(|field| field.hir_id == parent_hir_id) | ||
{ | ||
return false; | ||
} | ||
generics | ||
} | ||
_ => return false, | ||
}; | ||
|
||
let Ok(rendered_ty) = tcx.sess.source_map().span_to_snippet(self_ty.span) else { | ||
return false; | ||
}; | ||
|
||
let param = "TUV" | ||
.chars() | ||
.map(|c| c.to_string()) | ||
.chain((0..).map(|i| format!("P{i}"))) | ||
.find(|s| !generics.params.iter().any(|param| param.name.ident().as_str() == s)) | ||
.expect("we definitely can find at least one param name to generate"); | ||
Comment on lines
+168
to
+173
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logic didn't account for preexisting generic args, so this tries its best to suggest |
||
let mut sugg = vec![(self_ty.span, param.to_string())]; | ||
if let Some(insertion_span) = generics.span_for_param_suggestion() { | ||
sugg.push((insertion_span, format!(", {param}: {}", rendered_ty))); | ||
} else { | ||
sugg.push((generics.where_clause_span, format!("<{param}: {}>", rendered_ty))); | ||
} | ||
diag.multipart_suggestion_verbose(msg, sugg, Applicability::MachineApplicable); | ||
diag.multipart_suggestion_verbose( | ||
"you might be missing a type parameter", | ||
sugg, | ||
Applicability::MachineApplicable, | ||
); | ||
true | ||
} | ||
/// Make sure that we are in the condition to suggest the blanket implementation. | ||
|
@@ -198,32 +232,59 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ { | |
} | ||
} | ||
|
||
/// Try our best to approximate when adding `dyn` would be helpful for a bare | ||
/// trait object. | ||
/// | ||
/// Right now, this is if the type is either directly nested in another ty, | ||
/// or if it's in the tail field within a struct. This approximates what the | ||
/// user would've gotten on edition 2015, except for the case where we have | ||
/// an *obvious* knock-on `Sized` error. | ||
fn maybe_suggest_dyn_trait( | ||
&self, | ||
self_ty: &hir::Ty<'_>, | ||
label: &str, | ||
sugg: Vec<(Span, String)>, | ||
diag: &mut Diag<'_>, | ||
) -> bool { | ||
let tcx = self.tcx(); | ||
let parent_id = tcx.hir_get_parent_item(self_ty.hir_id).def_id; | ||
let parent_item = tcx.hir_node_by_def_id(parent_id).expect_item(); | ||
|
||
// If the parent item is an enum, don't suggest the dyn trait. | ||
if let hir::ItemKind::Enum(..) = parent_item.kind { | ||
return false; | ||
} | ||
// Look at the direct HIR parent, since we care about the relationship between | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was also
Into this:
My general logic here is that if the ty's hir node parent is another ty, then it's probably just a misspelled |
||
// the type and the thing that directly encloses it. | ||
match tcx.parent_hir_node(self_ty.hir_id) { | ||
// These are all generally ok. Namely, when a trait object is nested | ||
// into another expression or ty, it's either very certain that they | ||
// missed the ty (e.g. `&Trait`) or it's not really possible to tell | ||
// what their intention is, so let's not give confusing suggestions and | ||
// just mention `dyn`. The user can make up their mind what to do here. | ||
hir::Node::Ty(_) | ||
| hir::Node::Expr(_) | ||
| hir::Node::PatExpr(_) | ||
| hir::Node::PathSegment(_) | ||
| hir::Node::AssocItemConstraint(_) | ||
| hir::Node::TraitRef(_) | ||
| hir::Node::Item(_) | ||
| hir::Node::WherePredicate(_) => {} | ||
|
||
// If the parent item is a struct, check if self_ty is the last field. | ||
if let hir::ItemKind::Struct(variant_data, _) = parent_item.kind { | ||
if variant_data.fields().last().unwrap().ty.span != self_ty.span { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to use span comparisons here, |
||
return false; | ||
hir::Node::Field(field) => { | ||
// Enums can't have unsized fields, fields can only have an unsized tail field. | ||
if let hir::Node::Item(hir::Item { | ||
kind: hir::ItemKind::Struct(variant, _), .. | ||
}) = tcx.parent_hir_node(field.hir_id) | ||
&& variant | ||
.fields() | ||
.last() | ||
.is_some_and(|tail_field| tail_field.hir_id == field.hir_id) | ||
{ | ||
// Ok | ||
} else { | ||
return false; | ||
} | ||
} | ||
_ => return false, | ||
} | ||
|
||
// FIXME: Only emit this suggestion if the trait is dyn-compatible. | ||
diag.multipart_suggestion_verbose( | ||
label.to_string(), | ||
"you can add the `dyn` keyword if you want a trait object", | ||
sugg, | ||
Applicability::MachineApplicable, | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,6 @@ struct Foo2 { | |
//~^ ERROR expected a type, found a trait | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved this test out of |
||
} | ||
|
||
|
||
enum Enum1 { | ||
A(Trait), | ||
//~^ ERROR expected a type, found a trait | ||
|
@@ -26,5 +25,17 @@ enum Enum2 { | |
//~^ ERROR expected a type, found a trait | ||
} | ||
|
||
// Regression test for <https://github.com/rust-lang/rust/issues/138229>. | ||
pub struct InWhereClause | ||
where | ||
Trait:, {} | ||
//~^ ERROR expected a type, found a trait | ||
|
||
struct HasGenerics<T> { | ||
f: Trait, | ||
//~^ ERROR expected a type, found a trait | ||
t: T, | ||
} | ||
|
||
|
||
fn main() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect_item
here is not valid, since there are other kinds of HIR parents that can own thisdyn Trait
, like extern items (or trait/impl items, closures, etc).