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

Maintain predicate obligation chain for more detailed diagnostics #69709

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
4 changes: 3 additions & 1 deletion src/librustc/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ pub enum ObligationCauseCode<'tcx> {

/// In an impl of trait `X` for type `Y`, type `Y` must
/// also implement all supertraits of `X`.
Comment on lines 139 to 140
Copy link
Member

Choose a reason for hiding this comment

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

Oh wow this comment is incredibly outdated (/me pokes @nikomatsakis).

ItemObligation(DefId),
ItemObligation(DefId, Option<Span>),

/// Like `ItemObligation`, but with extra detail on the source of the obligation.
BindingObligation(DefId, Span),
Comment on lines 143 to 144
Copy link
Member

Choose a reason for hiding this comment

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

Wait, looks like a version with a Span already exists, or is it too restricted to work for you?

Or I guess BindingObligation itself could be replaced with ItemObligation now.

Copy link
Member

Choose a reason for hiding this comment

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

At the very least seeing this tells me that having a Span in ObligationCauseCode is probably fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#69745 acknowledges this and uses BindingObligation instead. Some of the output is slightly worse, but can be iterated on.

Expand Down Expand Up @@ -193,6 +193,8 @@ pub enum ObligationCauseCode<'tcx> {

ImplDerivedObligation(DerivedObligationCause<'tcx>),

DerivedCauseCode(Box<ObligationCauseCode<'tcx>>),
Copy link
Member

Choose a reason for hiding this comment

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

Don't we have a system for this already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The closest is what is above but that requires a predicates to be available always, and that wasn't feasible for the cases I was handling.

Copy link
Member

@eddyb eddyb Mar 5, 2020

Choose a reason for hiding this comment

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

WF definitely has WF predicates, so at least that should be doable.

I'll take a second look at the diff and note the places DerivedCauseCode is created in.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I see, DerivedObligationCause is a bit limited, you want to replace the TraitRef with a Predicate (which would let you put a WF predicate in there).

But still, I'd prefer if this side of the PR was done separately from the ItemObligation side.


/// Error derived when matching traits/impls; see ObligationCause for more details
CompareImplMethodObligation {
item_name: ast::Name,
Expand Down
3 changes: 2 additions & 1 deletion src/librustc/traits/structural_impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ impl<'a, 'tcx> Lift<'tcx> for traits::ObligationCauseCode<'a> {
super::SliceOrArrayElem => Some(super::SliceOrArrayElem),
super::TupleElem => Some(super::TupleElem),
super::ProjectionWf(proj) => tcx.lift(&proj).map(super::ProjectionWf),
super::ItemObligation(def_id) => Some(super::ItemObligation(def_id)),
super::ItemObligation(def_id, span) => Some(super::ItemObligation(def_id, span)),
super::BindingObligation(def_id, span) => Some(super::BindingObligation(def_id, span)),
super::ReferenceOutlivesReferent(ty) => {
tcx.lift(&ty).map(super::ReferenceOutlivesReferent)
Expand Down Expand Up @@ -442,6 +442,7 @@ impl<'a, 'tcx> Lift<'tcx> for traits::ObligationCauseCode<'a> {
super::ImplDerivedObligation(ref cause) => {
tcx.lift(cause).map(super::ImplDerivedObligation)
}
super::DerivedCauseCode(ref x) => tcx.lift(&**x).map(|x| x),
Copy link
Member

Choose a reason for hiding this comment

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

This is stripping DerivedCauseCode, please follow other match arms.

super::CompareImplMethodObligation {
item_name,
impl_item_def_id,
Expand Down
8 changes: 8 additions & 0 deletions src/librustc/ty/sty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1932,6 +1932,14 @@ impl<'tcx> TyS<'tcx> {
}
}

#[inline]
pub fn is_some_param(&self) -> bool {
estebank marked this conversation as resolved.
Show resolved Hide resolved
match self.kind {
ty::Param(_) => true,
_ => false,
}
}

#[inline]
pub fn is_slice(&self) -> bool {
match self.kind {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ impl NiceRegionError<'me, 'tcx> {
format!("trait `{}` defined here", self.tcx().def_path_str(trait_def_id)),
);

let leading_ellipsis = if let ObligationCauseCode::ItemObligation(def_id) = cause.code {
let leading_ellipsis = if let ObligationCauseCode::ItemObligation(def_id, _) = cause.code {
err.span_label(span, "doesn't satisfy where-clause");
err.span_label(
self.tcx().def_span(def_id),
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_infer/infer/opaque_types/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1295,8 +1295,8 @@ crate fn required_region_bounds(
assert!(!erased_self_ty.has_escaping_bound_vars());

traits::elaborate_predicates(tcx, predicates)
.filter_map(|predicate| {
match predicate {
.filter_map(|obligation| {
match obligation.predicate {
ty::Predicate::Projection(..)
| ty::Predicate::Trait(..)
| ty::Predicate::Subtype(..)
Expand Down
5 changes: 4 additions & 1 deletion src/librustc_infer/infer/outlives/verify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,10 @@ impl<'cx, 'tcx> VerifyBoundCx<'cx, 'tcx> {
let identity_proj = tcx.mk_projection(assoc_item_def_id, identity_substs);
self.collect_outlives_from_predicate_list(
move |ty| ty == identity_proj,
traits::elaborate_predicates(tcx, trait_predicates),
traits::elaborate_predicates(tcx, trait_predicates)
.into_iter()
.map(|o| o.predicate)
.collect::<Vec<_>>(),
)
.map(|b| b.1)
}
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_infer/traits/auto_trait.rs
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,8 @@ impl AutoTraitFinder<'tcx> {

computed_preds.extend(user_computed_preds.iter().cloned());
let normalized_preds =
elaborate_predicates(tcx, computed_preds.iter().cloned().collect());
elaborate_predicates(tcx, computed_preds.iter().cloned().collect())
.map(|o| o.predicate);
new_env =
ty::ParamEnv::new(tcx.mk_predicates(normalized_preds), param_env.reveal, None);
}
Expand Down
46 changes: 29 additions & 17 deletions src/librustc_infer/traits/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,8 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
}
};

for implication in super::elaborate_predicates(self.tcx, vec![*cond]) {
if let ty::Predicate::Trait(implication, _) = implication {
for obligation in super::elaborate_predicates(self.tcx, vec![*cond]) {
if let ty::Predicate::Trait(implication, _) = obligation.predicate {
let error = error.to_poly_trait_ref();
let implication = implication.to_poly_trait_ref();
// FIXME: I'm just not taking associated types at all here.
Expand Down Expand Up @@ -233,7 +233,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
);

let is_normalized_ty_expected = match &obligation.cause.code {
ObligationCauseCode::ItemObligation(_)
ObligationCauseCode::ItemObligation(..)
| ObligationCauseCode::BindingObligation(_, _)
| ObligationCauseCode::ObjectCastObligation(_) => false,
_ => true,
Expand Down Expand Up @@ -423,7 +423,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {

self.note_obligation_cause_code(
&mut err,
&obligation.predicate,
Some(&obligation.predicate),
&obligation.cause.code,
&mut vec![],
);
Expand Down Expand Up @@ -584,17 +584,29 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
))
);

let explanation =
if obligation.cause.code == ObligationCauseCode::MainFunctionType {
"consider using `()`, or a `Result`".to_owned()
} else {
format!(
"{}the trait `{}` is not implemented for `{}`",
pre_message,
trait_ref.print_only_trait_path(),
trait_ref.self_ty(),
)
};
let explanation = if obligation.cause.code
== ObligationCauseCode::MainFunctionType
{
"consider using `()`, or a `Result`".to_owned()
} else if self.tcx.lang_items().sized_trait().map_or(false, |sized_id| {
let self_ty = trait_ref.self_ty();
sized_id == trait_ref.def_id()
&& self_ty.is_some_param()
&& self_ty != self.tcx.types.self_param
}) {
// Detect type parameters with an implied `Sized` bound and explain
// it instead of giving a generic message. This will be displayed
// as a `help`.
"type parameters have an implicit `Sized` requirement by default"
.to_string()
} else {
format!(
"{}the trait `{}` is not implemented for `{}`",
pre_message,
trait_ref.print_only_trait_path(),
trait_ref.self_ty(),
)
};

if self.suggest_add_reference_to_arg(
&obligation,
Expand Down Expand Up @@ -1164,7 +1176,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
}
let mut err = self.need_type_info_err(body_id, span, self_ty, ErrorCode::E0283);
err.note(&format!("cannot resolve `{}`", predicate));
if let ObligationCauseCode::ItemObligation(def_id) = obligation.cause.code {
if let ObligationCauseCode::ItemObligation(def_id, _) = obligation.cause.code {
self.suggest_fully_qualified_path(&mut err, def_id, span, trait_ref.def_id());
} else if let (
Ok(ref snippet),
Expand Down Expand Up @@ -1333,7 +1345,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
if !self.maybe_note_obligation_cause_for_async_await(err, obligation) {
self.note_obligation_cause_code(
err,
&obligation.predicate,
Some(&obligation.predicate),
&obligation.cause.code,
&mut vec![],
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
}
}

if let ObligationCauseCode::ItemObligation(item) = obligation.cause.code {
if let ObligationCauseCode::ItemObligation(item, _) = obligation.cause.code {
// FIXME: maybe also have some way of handling methods
// from other traits? That would require name resolution,
// which we might want to be some sort of hygienic.
Expand Down
53 changes: 36 additions & 17 deletions src/librustc_infer/traits/error_reporting/suggestions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
body_id: hir::HirId,
) {
let self_ty = trait_ref.self_ty();
let (param_ty, projection) = match &self_ty.kind {
ty::Param(_) => (true, None),
ty::Projection(projection) => (false, Some(projection)),
let projection = match &self_ty.kind {
ty::Param(_) => None,
ty::Projection(projection) => Some(projection),
_ => return,
};

Expand Down Expand Up @@ -64,7 +64,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
generics,
kind: hir::TraitItemKind::Method(..),
..
}) if param_ty && self_ty == self.tcx.types.self_param => {
}) if self_ty.is_some_param() && self_ty == self.tcx.types.self_param => {
// Restricting `Self` for a single method.
suggest_restriction(&generics, "`Self`", err);
return;
Expand Down Expand Up @@ -138,7 +138,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
})
| hir::Node::TraitItem(hir::TraitItem { generics, span, .. })
| hir::Node::ImplItem(hir::ImplItem { generics, span, .. })
if param_ty =>
if self_ty.is_some_param() =>
{
// Missing generic type parameter bound.
let param_name = self_ty.to_string();
Expand Down Expand Up @@ -1421,7 +1421,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
debug!("note_obligation_cause_for_async_await: next_code={:?}", next_code);
self.note_obligation_cause_code(
err,
&obligation.predicate,
Some(&obligation.predicate),
next_code.unwrap(),
&mut Vec::new(),
);
Expand All @@ -1430,7 +1430,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
crate fn note_obligation_cause_code<T>(
&self,
err: &mut DiagnosticBuilder<'_>,
predicate: &T,
predicate: Option<&T>,
cause_code: &ObligationCauseCode<'tcx>,
obligated_types: &mut Vec<&ty::TyS<'tcx>>,
Copy link
Member

Choose a reason for hiding this comment

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

This is a weird name and type. I assume this was meant to be &mut Vec<Ty<'tcx>>? How is this not tripping the lint against types like TyS?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

This should indeed trip the internal lint. I want to revisit this lint anyway, to improve its suggestions. I'll look into this then.

) where
Expand Down Expand Up @@ -1470,15 +1470,30 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
region, object_ty,
));
}
ObligationCauseCode::ItemObligation(item_def_id) => {
ObligationCauseCode::ItemObligation(item_def_id, bound_span) => {
let item_name = tcx.def_path_str(item_def_id);
let msg = format!("required by `{}`", item_name);

if let Some(sp) = tcx.hir().span_if_local(item_def_id) {
let sp = tcx.sess.source_map().def_span(sp);
err.span_label(sp, &msg);
} else {
err.note(&msg);
match (
tcx.hir().span_if_local(item_def_id).map(|s| tcx.sess.source_map().def_span(s)),
bound_span,
) {
(Some(item_span), Some(bound_span)) if item_span.overlaps(bound_span) => {
err.span_label(bound_span, &format!("{} here", msg));
}
(Some(item_span), Some(bound_span)) => {
err.span_label(item_span, &msg);
err.span_label(bound_span, &format!("{} here", msg));
}
(None, Some(bound_span)) => {
err.span_label(bound_span, &format!("{} here", msg));
}
(Some(item_span), None) => {
err.span_label(item_span, &msg);
}
_ => {
err.note(&msg);
}
}
}
ObligationCauseCode::BindingObligation(item_def_id, span) => {
Expand Down Expand Up @@ -1576,6 +1591,10 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
ObligationCauseCode::SharedStatic => {
err.note("shared static variables must have a type that implements `Sync`");
}
ObligationCauseCode::DerivedCauseCode(ref data) => {
let mut obligated_types = vec![];
self.note_obligation_cause_code(err, None::<&T>, &**data, &mut obligated_types);
}
ObligationCauseCode::BuiltinDerivedObligation(ref data) => {
let parent_trait_ref = self.resolve_vars_if_possible(&data.parent_trait_ref);
let ty = parent_trait_ref.skip_binder().self_ty();
Expand All @@ -1586,7 +1605,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
if !self.is_recursive_obligation(obligated_types, &data.parent_code) {
self.note_obligation_cause_code(
err,
&parent_predicate,
Some(&parent_predicate),
&data.parent_code,
obligated_types,
);
Expand All @@ -1602,7 +1621,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
let parent_predicate = parent_trait_ref.without_const().to_predicate();
self.note_obligation_cause_code(
err,
&parent_predicate,
Some(&parent_predicate),
&data.parent_code,
obligated_types,
);
Expand All @@ -1611,14 +1630,14 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
err.note(&format!(
"the requirement `{}` appears on the impl method \
but not on the corresponding trait method",
predicate
predicate.unwrap()
));
}
ObligationCauseCode::CompareImplTypeObligation { .. } => {
err.note(&format!(
"the requirement `{}` appears on the associated impl type \
but not on the corresponding associated trait type",
predicate
predicate.unwrap()
));
}
ObligationCauseCode::ReturnType
Expand Down
8 changes: 6 additions & 2 deletions src/librustc_infer/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,9 @@ pub use self::specialize::{specialization_graph, translate_substs, OverlapError}
pub use self::structural_match::search_for_structural_match_violation;
pub use self::structural_match::type_marked_structural;
pub use self::structural_match::NonStructuralMatchTy;
pub use self::util::{elaborate_predicates, elaborate_trait_ref, elaborate_trait_refs};
pub use self::util::{
elaborate_obligations, elaborate_predicates, elaborate_trait_ref, elaborate_trait_refs,
};
pub use self::util::{expand_trait_aliases, TraitAliasExpander};
pub use self::util::{
get_vtable_index_of_object_method, impl_is_default, impl_item_is_final,
Expand Down Expand Up @@ -361,7 +363,9 @@ pub fn normalize_param_env_or_error<'tcx>(
);

let mut predicates: Vec<_> =
util::elaborate_predicates(tcx, unnormalized_env.caller_bounds.to_vec()).collect();
util::elaborate_predicates(tcx, unnormalized_env.caller_bounds.to_vec())
.map(|o| o.predicate)
.collect();

debug!("normalize_param_env_or_error: elaborated-predicates={:?}", predicates);

Expand Down
2 changes: 1 addition & 1 deletion src/librustc_infer/traits/object_safety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ fn generics_require_sized_self(tcx: TyCtxt<'_>, def_id: DefId) -> bool {
// Search for a predicate like `Self : Sized` amongst the trait bounds.
let predicates = tcx.predicates_of(def_id);
let predicates = predicates.instantiate_identity(tcx).predicates;
elaborate_predicates(tcx, predicates).any(|predicate| match predicate {
elaborate_predicates(tcx, predicates).any(|obligation| match obligation.predicate {
ty::Predicate::Trait(ref trait_pred, _) => {
trait_pred.def_id() == sized_def_id && trait_pred.skip_binder().self_ty().is_param(0)
}
Expand Down
6 changes: 3 additions & 3 deletions src/librustc_infer/traits/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -893,7 +893,7 @@ fn assemble_candidates_from_param_env<'cx, 'tcx>(
///
/// ```
/// trait Foo {
/// type FooT : Bar<BarT=i32>
/// type FooT: Bar<BarT=i32>
/// }
/// ```
///
Expand Down Expand Up @@ -923,7 +923,7 @@ fn assemble_candidates_from_trait_def<'cx, 'tcx>(
// If so, extract what we know from the trait and try to come up with a good answer.
let trait_predicates = tcx.predicates_of(def_id);
let bounds = trait_predicates.instantiate(tcx, substs);
let bounds = elaborate_predicates(tcx, bounds.predicates);
let bounds = elaborate_predicates(tcx, bounds.predicates).map(|o| o.predicate);
assemble_candidates_from_predicates(
selcx,
obligation,
Expand Down Expand Up @@ -1212,7 +1212,7 @@ fn confirm_object_candidate<'cx, 'tcx>(

// select only those projections that are actually projecting an
// item with the correct name
let env_predicates = env_predicates.filter_map(|p| match p {
let env_predicates = env_predicates.filter_map(|o| match o.predicate {
ty::Predicate::Projection(data) => {
if data.projection_def_id() == obligation.predicate.item_def_id {
Some(data)
Expand Down
Loading