-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Improve error messages for recursive GATs implicitly deriving Sized
#87304
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
Changes from all commits
b005f29
220c9cd
1668dab
c18350d
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 |
---|---|---|
|
@@ -55,6 +55,13 @@ pub trait InferCtxtExt<'tcx> { | |
|
||
fn report_overflow_error_cycle(&self, cycle: &[PredicateObligation<'tcx>]) -> !; | ||
|
||
fn maybe_report_recursive_gat( | ||
&self, | ||
err: &mut DiagnosticBuilder<'_>, | ||
parent: &PredicateObligation<'tcx>, | ||
child: &PredicateObligation<'tcx>, | ||
) -> bool; | ||
|
||
/// The `root_obligation` parameter should be the `root_obligation` field | ||
/// from a `FulfillmentError`. If no `FulfillmentError` is available, | ||
/// then it should be the same as `obligation`. | ||
|
@@ -213,6 +220,95 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { | |
bug!(); | ||
} | ||
|
||
fn maybe_report_recursive_gat( | ||
&self, | ||
err: &mut DiagnosticBuilder<'_>, | ||
parent: &PredicateObligation<'tcx>, | ||
child: &PredicateObligation<'tcx>, | ||
) -> bool { | ||
let (parent, child) = | ||
match (parent.predicate.kind().skip_binder(), child.predicate.kind().skip_binder()) { | ||
(ty::PredicateKind::Trait(head, ..), ty::PredicateKind::Trait(tail, ..)) => { | ||
(head, tail) | ||
} | ||
_ => return false, | ||
}; | ||
|
||
if Some(parent.clone().def_id()) != self.tcx.lang_items().sized_trait() { | ||
return false; | ||
} | ||
|
||
let parent_ty = parent.self_ty(); | ||
let child_ty = child.self_ty(); | ||
debug!( | ||
"report_overflow_error_cycle: parent_ty = {:?}, child_ty = {:?}", | ||
parent_ty, child_ty | ||
); | ||
|
||
let parent_kind = parent_ty.kind(); | ||
let child_kind = child_ty.kind(); | ||
debug!( | ||
"report_overflow_error_cycle: parent_kind = {:?}, child_kind = {:?}", | ||
parent_kind, child_kind | ||
); | ||
|
||
let (substs, item_def_id) = match parent_kind { | ||
ty::Projection(ty::ProjectionTy { substs, item_def_id }) => (substs, item_def_id), | ||
_ => 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, this would fail if the parent is defined in another crate? |
||
let node = self.tcx.hir().get(hir_id); | ||
debug!("report_overflow_error_cycle: node = {:?}", node); | ||
|
||
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]; | ||
Comment on lines
+264
to
+295
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 all feels a bit weird to me. But running out of time right now to think more about this. |
||
debug!("report_overflow_error_cycle: param = {:?}", param); | ||
|
||
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, | ||
); | ||
Comment on lines
+298
to
+307
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. When we have something like 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. Yeah, there should be a flag about implicit |
||
|
||
true | ||
} | ||
|
||
/// Reports that a cycle was detected which led to overflow and halts | ||
/// compilation. This is equivalent to `report_overflow_error` except | ||
/// that we can give a more helpful error message (and, in particular, | ||
|
@@ -224,7 +320,25 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { | |
|
||
debug!("report_overflow_error_cycle: cycle={:?}", cycle); | ||
|
||
self.report_overflow_error(&cycle[0], false); | ||
let begin = &cycle[0]; | ||
let mut err = struct_span_err!( | ||
self.tcx.sess, | ||
begin.cause.span, | ||
E0275, | ||
"overflow evaluating the requirement `{}`", | ||
begin.predicate | ||
); | ||
err.note("detected recursive derive for `Sized` in a generic associated type"); | ||
|
||
if cycle.windows(2).fold(false, |valid_error, preds| { | ||
valid_error || self.maybe_report_recursive_gat(&mut err, &preds[0], &preds[1]) | ||
}) { | ||
err.emit(); | ||
self.tcx.sess.abort_if_errors(); | ||
bug!() | ||
} else { | ||
self.report_overflow_error(begin, false); | ||
} | ||
} | ||
|
||
fn report_selection_error( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -193,7 +193,7 @@ fn adt_sized_constraint(tcx: TyCtxt<'_>, def_id: DefId) -> ty::AdtSizedConstrain | |
let result = tcx.mk_type_list( | ||
def.variants | ||
.iter() | ||
.flat_map(|v| v.fields.last()) | ||
.flat_map(|v| &v.fields) | ||
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. 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 |
||
.flat_map(|f| sized_constraint_for_ty(tcx, def, tcx.type_of(f.did))), | ||
); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
#![feature(generic_associated_types)] | ||
|
||
// check-pass | ||
|
||
trait Allocator { | ||
type Allocated<T: ?Sized>; | ||
} | ||
|
||
enum LinkedList<A: Allocator> { | ||
Head, | ||
Next(u8, A::Allocated<Self>), | ||
} | ||
|
||
enum LinkedList2<A: Allocator> { | ||
Head, | ||
Next(A::Allocated<Self>, u8), | ||
} | ||
|
||
impl Allocator for () { | ||
type Allocated<T: ?Sized> = Box<T>; | ||
} | ||
|
||
fn main() { | ||
{ | ||
use LinkedList::{Head, Next}; | ||
let mut ll: LinkedList<()> = Next( | ||
8, | ||
Box::new(Next( | ||
0, | ||
Box::new(Next( | ||
6, | ||
Box::new(Next(2, Box::new(Next(6, Box::new(Head))))), | ||
)), | ||
)), | ||
); | ||
|
||
while let Next(num, next) = ll { | ||
println!("{}", num); | ||
ll = *next; | ||
} | ||
} | ||
{ | ||
use LinkedList2::{Head, Next}; | ||
let mut ll: LinkedList2<()> = Next( | ||
Box::new(Next( | ||
Box::new(Next( | ||
Box::new(Next(Box::new(Next(Box::new(Head), 6)), 2)), | ||
6, | ||
)), | ||
0, | ||
)), | ||
8, | ||
); | ||
|
||
while let Next(next, num) = ll { | ||
println!("{}", num); | ||
ll = *next; | ||
} | ||
} | ||
} |
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.
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)