Fix dyn -> param suggestion in struct ICEs #138238
Conversation
rustbot
added
S-waiting-on-review
|
HIR ty lowering was modified cc @fmease |
|
|
||
| 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(); |
There was a problem hiding this comment.
expect_item here is not valid, since there are other kinds of HIR parents that can own this dyn Trait, like extern items (or trait/impl items, closures, etc).
| 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.
I added a check that we're suggesting this on a field directly. dyn may show up in other positions like in a where clause or in an expr on a default field value.
| 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.
This was also expect_iteming unnecessarily. I took the logic and made it focus on the relationship between the ty and its parent node, so we don't suggest turning:
struct Foo { f: Box<Any>, /* more fields */ }
Into this:
struct Foo<T: Any> { f: Box<T>, /* more fields */ }
My general logic here is that if the ty's hir node parent is another ty, then it's probably just a misspelled dyn Trait. There is no perfect heuristic here, but I think it's less invasive to suggest the most naive thing here, which is just to insert dyn, unless we're very confident it will be wrong.
| @@ -13,7 +13,6 @@ struct Foo2 { | |||
| //~^ ERROR expected a type, found a trait | |||
There was a problem hiding this comment.
I moved this test out of suggestions/ since that's a kitchen sink
|
|
||
| // 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.
We don't need to use span comparisons here, hir_id comparisons are more faithful.
| 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"); |
There was a problem hiding this comment.
This logic didn't account for preexisting generic args, so this tries its best to suggest T, U, V, P1, P2, ...
Makes the logic from #138042 a bit less ICEy and more clean. Also fixes an incorrect suggestion when the struct already has generics. I'll point out the major changes and observations in the code.
Fixes #138229
Fixes #138211
r? nnethercote since you reviewed the original pr, or re-roll if you don't want to review this