Skip to content
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

Improve error messages for recursive GATs implicitly deriving Sized #87304

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 110 additions & 1 deletion compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ pub trait InferCtxtExt<'tcx> {

fn report_overflow_error_cycle(&self, cycle: &[PredicateObligation<'tcx>]) -> !;

fn maybe_report_recursive_gat(
&self,
begin: &PredicateObligation<'tcx>,
end: &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`.
Expand Down Expand Up @@ -213,6 +219,103 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
bug!();
}

fn maybe_report_recursive_gat(
&self,
begin: &PredicateObligation<'tcx>,
end: &PredicateObligation<'tcx>,
) -> bool {
let (head, tail) =
match (begin.predicate.kind().skip_binder(), end.predicate.kind().skip_binder()) {
(ty::PredicateKind::Trait(head, ..), ty::PredicateKind::Trait(tail, ..)) => {
(head, tail)
}
_ => return false,
};

if Some(head.clone().def_id()) != self.tcx.lang_items().sized_trait() {
return false;
}

let head_ty = head.self_ty();
let tail_ty = tail.self_ty();
debug!("report_overflow_error_cycle: head_ty = {:?}, tail_ty = {:?}", head_ty, tail_ty);

let head_kind = head_ty.kind();
let tail_kind = tail_ty.kind();
debug!(
Copy link
Member

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)

"report_overflow_error_cycle: head_kind = {:?}, tail_kind = {:?}",
head_kind, tail_kind
);

let (substs, item_def_id) = match head_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());
Copy link
Member

Choose a reason for hiding this comment

The 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 == &tail_ty.into() { Some(i) } else { None })
})
.flatten()
{
Some(index) => index,
None => return false,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

debug!("report_overflow_error_cycle: index = {:?}", index);

let param = &assoc_generics.params[index];
debug!("report_overflow_error_cycle: param = {:?}", param);

let mut err = struct_span_err!(
self.tcx.sess,
begin.cause.span,
E0275,
"overflow evaluating the requirement `{}`",
head
);

err.note("detected recursive derive for `Sized` in a generic associated type");

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
Copy link
Contributor Author

@jedel1043 jedel1043 Jul 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there should be a flag about implicit Sized somewhere and we should probably honor that.


err.emit();
self.tcx.sess.abort_if_errors();
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,
Expand All @@ -224,7 +327,13 @@ 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 end = &cycle[cycle.len() - 1];

if !self.maybe_report_recursive_gat(begin, end) {
self.report_overflow_error(begin, false);
}
bug!()
}

fn report_selection_error(
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_trait_selection/src/traits/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -327,8 +327,7 @@ impl<'a, 'b, 'tcx> AssocTypeNormalizer<'a, 'b, 'tcx> {

fn fold<T: TypeFoldable<'tcx>>(&mut self, value: T) -> T {
let value = self.selcx.infcx().resolve_vars_if_possible(value);
debug!(?value);

debug!(?value, "fold");
assert!(
!value.has_escaping_bound_vars(),
"Normalizing {:?} without wrapping in a `Binder`",
Expand Down
15 changes: 14 additions & 1 deletion compiler/rustc_trait_selection/src/traits/select/confirmation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,12 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
let tcx = self.tcx();

let trait_predicate = self.infcx.shallow_resolve(obligation.predicate);
debug!(?trait_predicate);

let placeholder_trait_predicate =
self.infcx().replace_bound_vars_with_placeholders(trait_predicate);
debug!(?placeholder_trait_predicate);

let placeholder_self_ty = placeholder_trait_predicate.self_ty();
let (def_id, substs) = match *placeholder_self_ty.kind() {
ty::Projection(proj) => (proj.item_def_id, proj.substs),
Expand All @@ -140,9 +144,13 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
};

let candidate_predicate = tcx.item_bounds(def_id)[idx].subst(tcx, substs);
debug!("candidate predicate before poly trait = {:?}", candidate_predicate);

let candidate = candidate_predicate
.to_opt_poly_trait_ref()
.expect("projection candidate is not a trait predicate");
debug!("candidate predicate after poly trait = {:?}", candidate_predicate);

let mut obligations = Vec::new();
let candidate = normalize_with_depth_to(
self,
Expand All @@ -162,7 +170,11 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
})?);

if let ty::Projection(..) = placeholder_self_ty.kind() {
for predicate in tcx.predicates_of(def_id).instantiate_own(tcx, substs).predicates {
let predicates = tcx.predicates_of(def_id);
debug!(?predicates);
let instantiated_predicates = predicates.instantiate_own(tcx, substs);
debug!(?instantiated_predicates);
for predicate in instantiated_predicates.predicates {
let normalized = normalize_with_depth_to(
self,
obligation.param_env,
Expand All @@ -179,6 +191,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
));
}
}
debug!(?obligations);

Ok(obligations)
})
Expand Down
8 changes: 6 additions & 2 deletions compiler/rustc_trait_selection/src/traits/select/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -868,7 +868,10 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
where
I: Iterator<Item = ty::Predicate<'tcx>>,
{
cycle.all(|predicate| self.coinductive_predicate(predicate))
cycle.all(|predicate| {
debug!(?predicate, "coinductive_match");
self.coinductive_predicate(predicate)
})
}

fn coinductive_predicate(&self, predicate: ty::Predicate<'tcx>) -> bool {
Expand Down Expand Up @@ -1572,8 +1575,9 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {

// NOTE: binder moved to (*)
let self_ty = self.infcx.shallow_resolve(obligation.predicate.skip_binder().self_ty());
debug!(?self_ty, "sized_conditions");

match self_ty.kind() {
match *self_ty.kind() {
ty::Infer(ty::IntVar(_) | ty::FloatVar(_))
| ty::Uint(_)
| ty::Int(_)
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_ty_utils/src/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The 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 Sized for the struct to be well formed. (This check is done elsewhere.)

.flat_map(|f| sized_constraint_for_ty(tcx, def, tcx.type_of(f.did))),
);

Expand Down
11 changes: 11 additions & 0 deletions compiler/rustc_typeck/src/collect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1978,6 +1978,7 @@ fn gather_explicit_predicates_of(tcx: TyCtxt<'_>, def_id: DefId) -> ty::GenericP

let hir_id = tcx.hir().local_def_id_to_hir_id(def_id.expect_local());
let node = tcx.hir().get(hir_id);
debug!("explicit_predicates_of: node = {:?}", node);

let mut is_trait = None;
let mut is_default_impl_trait = None;
Expand Down Expand Up @@ -2070,6 +2071,7 @@ fn gather_explicit_predicates_of(tcx: TyCtxt<'_>, def_id: DefId) -> ty::GenericP
if let Some(_trait_ref) = is_trait {
predicates.extend(tcx.super_predicates_of(def_id).predicates.iter().cloned());
}
debug!("explicit_predicates_of: super predicates = {:?}", predicates);

// In default impls, we can assume that the self type implements
// the trait. So in:
Expand All @@ -2085,6 +2087,7 @@ fn gather_explicit_predicates_of(tcx: TyCtxt<'_>, def_id: DefId) -> ty::GenericP
tcx.def_span(def_id),
));
}
debug!("explicit_predicates_of: default impls = {:?}", predicates);

// Collect the region predicates that were declared inline as
// well. In the case of parameters declared on a fn or method, we
Expand Down Expand Up @@ -2113,6 +2116,8 @@ fn gather_explicit_predicates_of(tcx: TyCtxt<'_>, def_id: DefId) -> ty::GenericP
}
}

debug!("explicit_predicates_of: region predicates = {:?}", predicates);

// Collect the predicates that were written inline by the user on each
// type parameter (e.g., `<T: Foo>`).
for param in ast_generics.params {
Expand All @@ -2132,6 +2137,7 @@ fn gather_explicit_predicates_of(tcx: TyCtxt<'_>, def_id: DefId) -> ty::GenericP
sized,
param.span,
);
debug!("explicit_predicates_of: bounds = {:?}", bounds);
predicates.extend(bounds.predicates(tcx, param_ty));
}
GenericParamKind::Const { .. } => {
Expand All @@ -2142,6 +2148,8 @@ fn gather_explicit_predicates_of(tcx: TyCtxt<'_>, def_id: DefId) -> ty::GenericP
}
}

debug!("explicit_predicates_of: inline predicates = {:?}", predicates);

// Add in the bounds that appear in the where-clause.
let where_clause = &ast_generics.where_clause;
for predicate in where_clause.predicates {
Expand Down Expand Up @@ -2251,8 +2259,11 @@ fn gather_explicit_predicates_of(tcx: TyCtxt<'_>, def_id: DefId) -> ty::GenericP
}
}

debug!("explicit_predicates_of: where predicates = {:?}", predicates);

if tcx.features().const_evaluatable_checked {
predicates.extend(const_evaluatable_predicates_of(tcx, def_id.expect_local()));
debug!("explicit_predicates_of: const predicates = {:?}", predicates);
}

let mut predicates: Vec<_> = predicates.into_iter().collect();
Expand Down
60 changes: 60 additions & 0 deletions src/test/ui/generic-associated-types/issue-80626-pass.rs
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;
}
}
}
Loading