Uh oh!
There was an error while loading. Please reload this page.
Improve error messages for recursive GATs implicitly deriving Sized - #87304
Improve error messages for recursive GATs implicitly deriving Sized#87304jedel1043 wants to merge 4 commits into
Sized#87304Conversation
rust-highfive
commented
Jul 20, 2021
r? @cjgillot (rust-highfive has picked a reviewer for you, use r? to override) |
| let (span, separator) = match param.bounds { | ||
| [] => (param.span.shrink_to_hi(), ":"), | ||
| [.., bound] => (bound.span().shrink_to_hi(), " +"), | ||
| }; | ||
| err.span_suggestion_verbose( | ||
| span, | ||
| "consider relaxing the implicit `Sized` restriction", | ||
| format!("{} ?Sized", separator), | ||
| Applicability::MachineApplicable, | ||
| ); |
There was a problem hiding this comment.
When we have something like Type<T: Sized> this suggestion will recommend to do Type<T: Sized + ?Sized>, which is just incorrect. Maybe we could instead suggest to replace Sized with ?Sized when there's a Sized inside param.bounds.
Nonetheless, this works for the trivial case.
There was a problem hiding this comment.
Yeah, there should be a flag about implicit Sized somewhere and we should probably honor that.
| let parameter_size = assoc_generics.params.len(); | ||
| if parameter_size == 0 { | ||
| return false; | ||
| } | ||
| assert!(substs.len() >= parameter_size); | ||
| let index = match substs | ||
| .get(substs.len() - parameter_size..) | ||
| .map(|gens| { | ||
| gens.iter() | ||
| .enumerate() | ||
| .find_map(|(i, param)| if param == &tail_ty.into() { Some(i) } else { None }) | ||
| }) | ||
| .flatten() | ||
| { | ||
| Some(index) => index, | ||
| None => return false, | ||
| }; |
There was a problem hiding this comment.
Is there a better way to do this? I did what I could, but I'm still not 100% sure this would work on every case.
jackh726
commented
Jul 20, 2021
r? @jackh726 |
jackh726
left a comment
There was a problem hiding this comment.
Ran out of time, but some comments...
Overall, I guess this is just to give the "you can add a ?Sized" suggestion? I'm on the fence on if that's actually helpful. Niko and I discussed this but yesterday; the proper fix is to make Sized coinductive (#83647), but that's a bit more than just GATs.
| type Allocated<T> = Box<T>; | ||
| } | ||
| fn main() { |
There was a problem hiding this comment.
I don't think we need to test runtime behavior here; this can just be empty with a // check-pass
| help: consider relaxing the implicit `Sized` restriction | ||
| | | ||
| LL | type Allocated<T: ?Sized>; | ||
| | ^^^^^^^^ | ||
There was a problem hiding this comment.
On one hand...yes, this is a suggestion that should work. On the other, is this a suggestion that is normally helpful?
| def.variants | ||
| .iter() | ||
| .flat_map(|v| v.fields.last()) | ||
| .flat_map(|v| &v.fields) |
There was a problem hiding this comment.
So, this isn't needed (i.e. this isn't a bug). We know that all fields except the last for all variants must be Sized for the struct to be well formed. (This check is done elsewhere.)
| let parent_kind = parent_ty.kind(); | ||
| let child_kind = child_ty.kind(); | ||
| debug!( |
There was a problem hiding this comment.
FYI you can do debug!(?parent_kind, ?child_kind). But, I feel like there are a lot of redundant debug statements here...(for example, we should get all the same info from the debug call above)
| _ => return false, | ||
| }; | ||
| let hir_id = self.tcx.hir().local_def_id_to_hir_id(item_def_id.expect_local()); |
There was a problem hiding this comment.
So, this would fail if the parent is defined in another crate?
| let (assoc_generics, _assoc_span) = match node { | ||
| Node::TraitItem(hir::TraitItem { | ||
| generics, | ||
| kind: hir::TraitItemKind::Type(..), | ||
| span, | ||
| .. | ||
| }) => (generics, span), | ||
| _ => return false, | ||
| }; | ||
| let parameter_size = assoc_generics.params.len(); | ||
| if parameter_size == 0 { | ||
| return false; | ||
| } | ||
| assert!(substs.len() >= parameter_size); | ||
| let index = match substs | ||
| .get(substs.len() - parameter_size..) | ||
| .map(|gens| { | ||
| gens.iter() | ||
| .enumerate() | ||
| .find_map(|(i, param)| if param == &child_ty.into() { Some(i) } else { None }) | ||
| }) | ||
| .flatten() | ||
| { | ||
| Some(index) => index, | ||
| None => return false, | ||
| }; | ||
| debug!("report_overflow_error_cycle: index = {:?}", index); | ||
| let param = &assoc_generics.params[index]; |
There was a problem hiding this comment.
This all feels a bit weird to me. But running out of time right now to think more about this.
| let (span, separator) = match param.bounds { | ||
| [] => (param.span.shrink_to_hi(), ":"), | ||
| [.., bound] => (bound.span().shrink_to_hi(), " +"), | ||
| }; | ||
| err.span_suggestion_verbose( | ||
| span, | ||
| "consider relaxing the implicit `Sized` restriction", | ||
| format!("{} ?Sized", separator), | ||
| Applicability::MachineApplicable, | ||
| ); |
There was a problem hiding this comment.
Yeah, there should be a flag about implicit Sized somewhere and we should probably honor that.
So the proper fix would be to make |
Fixes#80626
?Sizedconstraint in case a recursive GAT is found.Sizedif it wasn't on the last position of an ADT.