Maintain predicate obligation chain for more detailed diagnostics #69709

estebank · 2020-03-04T19:09:56Z

Keep obligations around instead of just predicates as much as possible, to maintain extra context, including chains of obligations.
Use more specific spans by pointing at bindings that introduce obligations.
Mention implicit T: Sized bound in E0277 errors.

Fix #27964.

rust-highfive · 2020-03-04T19:10:01Z

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

estebank · 2020-03-04T19:10:27Z

cc @nikomatsakis @rust-lang/wg-diagnostics

eddyb · 2020-03-05T07:03:53Z

src/librustc/traits/mod.rs

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

    ImplDerivedObligation(DerivedObligationCause<'tcx>),

+    DerivedCauseCode(Box<ObligationCauseCode<'tcx>>),


Don't we have a system for this already?

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.

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.

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.

eddyb · 2020-03-05T07:04:59Z

src/librustc/traits/structural_impls.rs

@@ -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),


This is stripping DerivedCauseCode, please follow other match arms.

src/librustc/ty/sty.rs

eddyb · 2020-03-05T07:08:41Z

src/librustc_infer/traits/error_reporting/suggestions.rs

@@ -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>>,


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?

cc @oli-obk @flip1995

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

eddyb · 2020-03-05T17:20:59Z

src/test/ui/wf/wf-fn-where-clause.rs

+struct Vec<T> {
+    t: T,
+}


Can you keep the original test and duplicate it with a local Vec definition?
Ideally the original test would also have good diagnostics.

The tests where I did this have good output, they just point into stdlib when their spans are available and changing them to a local type didn't compromise the integrity of what was being tested. It didn't feel necessary to have two types (increasing CI time), but if you insist I can duplicate them.

necessary to have two types (increasing CI time)

Presumably it takes negligible time, it's only 3 tests.

and changing them to a local type didn't compromise the integrity of what was being tested

Can never be too sure, especially for old tests.

eddyb · 2020-03-05T17:23:04Z

src/test/ui/union/union-derive-eq.rs

+// FIXME: missing sysroot spans (#53081)
+// ignore-i586-unknown-linux-gnu
+// ignore-i586-unknown-linux-musl
+// ignore-i686-unknown-linux-musl


eddyb · 2020-03-05T17:24:26Z

src/test/ui/union/union-derive-eq.stderr

+  ::: $SRC_DIR/libcore/cmp.rs:LL:COL
+   |
+LL | pub struct AssertParamIsEq<T: Eq + ?Sized> {
+   |                               -- required by `std::cmp::AssertParamIsEq` here
   |
-   = note: required by `std::cmp::AssertParamIsEq`


cc @nikomatsakis @Centril Finally a use for the per-Predicate Spans! I think I added those for the type alias work that never landed itself.

Although for AssertParamIsEq specifically, we might want a custom #[rustc_on_unimplemented], since it's used by #[derive(Eq)] specifically.

Looking into this, rustc_on_unimplemented would need to have support for targeting bounds in the obligation chain too.

eddyb · 2020-03-05T17:27:26Z

src/test/ui/union/union-derive-clone.stderr

+LL | pub struct AssertParamIsCopy<T: Copy + ?Sized> {
+   |                                 ---- required by `std::clone::AssertParamIsCopy` here
   |
-   = note: required by `std::clone::AssertParamIsCopy`


This could also use #[rustc_on_unimplemented], I think.

I agree. Do you have some suggested target output?

Nope, but feel free to open an issue first to gather suggestions.

eddyb · 2020-03-05T17:30:27Z

src/test/ui/issues/issue-23281.rs

+struct Vec<T> {
+    t: T,
+}


Another instance of a case where I think it would be worth duplicating the test.

eddyb · 2020-03-05T17:31:08Z

src/test/ui/issues/issue-18919.rs

+enum Option<T> {
+    Some(T),
+    None,
+}


Third test, this time Option is being duplicated in the test.

src/librustc_typeck/collect.rs

eddyb · 2020-03-05T17:45:50Z

src/librustc_infer/traits/util.rs

+fn predicate_obligation<'tcx>(
+    predicate: ty::Predicate<'tcx>,
+    span: Option<Span>,
+) -> PredicateObligation<'tcx> {
+    let mut cause = ObligationCause::dummy();
+    if let Some(span) = span {
+        cause.span = span;
+    }
+    Obligation { cause, param_env: ty::ParamEnv::empty(), recursion_depth: 0, predicate }
 }


Might be easier, instead of all of this, to have Elaborator produce (ty::Predicate, Span) pairs, then it's basically just like tcx.predicates_of.

Or Option<Span> I guess.

I thought about those possibilities, but it felt that just dealing with Obligations everywhere will make more sense in the future. If we can move to a point where we only deal with bare Predicate only when evaluating a single one, the we can keep as much diagnostic info as we (really) need.

I suppose, but you're introducing dummy Obligations that could end up getting mixed with true Obligations.

This may make more sense if elaborate_predicates took in Obligation<Vec<Predicate>> or something.

But I suppose I can defer to @nikomatsakis on this.

eddyb · 2020-03-05T17:48:18Z

src/librustc_infer/traits/wf.rs

+                cause.code =
+                    traits::ObligationCauseCode::DerivedCauseCode(Box::new(obligation.cause.code));


So this seems to be the only place where DerivedCauseCode is created?

I think most of the test output changes are because of the extra Span in ItemObligation, not this, am I right?

If so, it might be worth splitting this PR into the non-DerivedCauseCode changes and the DerivedCauseCode changes, because the former seem far less controversial (and easier to understand) to me.

"most" might be the case. Removing this part will greatly diminish the benefit of this change, but I can oblige and let the PRs evolve independently :)

eddyb · 2020-03-05T17:52:52Z

src/librustc_infer/traits/wf.rs

-        let cause = self.cause(traits::ItemObligation(def_id));
        predicates
            .predicates
            .into_iter()
-            .map(|pred| traits::Obligation::new(cause.clone(), self.param_env, pred))
+            .zip(predicates.spans.into_iter())
+            .map(|(pred, span)| {
+                let cause = self.cause(traits::ItemObligation(def_id, Some(span)));
+                traits::Obligation::new(cause, self.param_env, pred)
+            })


And here it is, this (mostly) self-contained change is what I suspect is responsible for most of the test changes, I would love to have this landed ASAP (assuming sticking the Span into ItemObligation is the best way to go about this, I'd defer to @nikomatsakis on that).

Just to be clear, this change is the one that precipitated the shift to keeping PredicateObligations instead of Predicates.

Making a new PR extracting these changes on their own.

eddyb · 2020-03-05T17:55:36Z

src/librustc/traits/mod.rs

    /// Like `ItemObligation`, but with extra detail on the source of the obligation.
    BindingObligation(DefId, Span),


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.

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

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

eddyb · 2020-03-05T17:56:31Z

src/librustc/traits/mod.rs

    /// In an impl of trait `X` for type `Y`, type `Y` must
    /// also implement all supertraits of `X`.


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

estebank · 2020-03-05T20:42:18Z

Made #69745 with only a subset of these changes (no ?Sized and no obligation chain).

estebank · 2020-03-07T00:49:38Z

This PR is fully subsumed by #69793, #69791 and #69745. I'll close next week, but leaving open for now to give a chance to people tagged in inline comments to see them.

@eddyb

…=nikomatsakis,eddyb Use `PredicateObligation`s instead of `Predicate`s Keep more information about trait binding failures. Use more specific spans by pointing at bindings that introduce obligations. Subset of rust-lang#69709. r? @eddyb

@eddyb

…ikomatsakis Maintain chain of derived obligations When evaluating the derived obligations from super traits, maintain a reference to the original obligation in order to give more actionable context in the output. Continuation (and built on) rust-lang#69745, subset of rust-lang#69709. r? @eddyb

estebank added 2 commits March 3, 2020 15:07

Maintain predicate obligation chain for more detailed diagnostics

d6986e0

Mention implicit T: Sized bound in E0277 errors

c2e1482

rust-highfive assigned eddyb Mar 4, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 4, 2020

estebank mentioned this pull request Mar 4, 2020

Note where implicit Sized requirement comes from #27964

Closed

eddyb reviewed Mar 5, 2020

View reviewed changes

src/librustc/ty/sty.rs Show resolved Hide resolved

eddyb reviewed Mar 5, 2020

View reviewed changes

src/librustc_typeck/collect.rs Show resolved Hide resolved

eddyb reviewed Mar 5, 2020

View reviewed changes

estebank mentioned this pull request Mar 5, 2020

Use PredicateObligations instead of Predicates #69745

Merged

This was referenced Mar 6, 2020

Mention implicit T: Sized bound in E0277 errors #69791

Closed

Maintain chain of derived obligations #69793

Merged

estebank closed this Mar 9, 2020

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

		ImplDerivedObligation(DerivedObligationCause<'tcx>),

		DerivedCauseCode(Box<ObligationCauseCode<'tcx>>),

		cause.code =
		traits::ObligationCauseCode::DerivedCauseCode(Box::new(obligation.cause.code));

		/// Like `ItemObligation`, but with extra detail on the source of the obligation.
		BindingObligation(DefId, Span),

		/// In an impl of trait `X` for type `Y`, type `Y` must
		/// also implement all supertraits of `X`.

Maintain predicate obligation chain for more detailed diagnostics #69709

Maintain predicate obligation chain for more detailed diagnostics #69709

Conversation

estebank commented Mar 4, 2020 • edited Loading

rust-highfive commented Mar 4, 2020

estebank commented Mar 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb Mar 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank Mar 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Mar 5, 2020

estebank commented Mar 7, 2020

estebank commented Mar 4, 2020 •

edited

Loading

eddyb Mar 5, 2020 •

edited

Loading

estebank Mar 5, 2020 •

edited

Loading