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 chain of derived obligations #69793

Merged
merged 4 commits into from
Apr 19, 2020

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Mar 7, 2020

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) #69745, subset of #69709.

r? @eddyb

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

This comment has been minimized.

@rust-highfive

This comment has been minimized.

Comment on lines 326 to 329
let parent_trait_ref = obligation
.predicate
.to_opt_poly_trait_ref()
.unwrap_or_else(|| ty::Binder::dummy(*trait_ref));
Copy link
Member

Choose a reason for hiding this comment

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

I think "parent" implies you should always use trait_ref, while obligation.predicate is used for the "child" obligation that this closure produces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's right. Changing this to be let parent_trait_ref = ty::Binder::dummy(*trait_ref); causes the output to be nonsensical.

From:

error[E0277]: the trait bound `T: MyDisplay` is not satisfied
  --> $DIR/issue-65774-2.rs:16:6
   |
LL | trait MPU {
   |       ---
LL |     type MpuConfig: MyDisplay = T;
   |                     --------- required by this bound in `MPU`
...
LL | impl MPU for S { }
   |      ^^^ the trait `MyDisplay` is not implemented for `T`
   |
   = note: required because of the requirements on the impl of `MyDisplay` for `<S as MPU>::MpuConfig`

to

error[E0277]: the trait bound `T: MyDisplay` is not satisfied
  --> $DIR/issue-65774-2.rs:16:6
   |
LL | trait MPU {
   |       ---
LL |     type MpuConfig: MyDisplay = T;
   |                     --------- required by this bound in `MPU`
...
LL | impl MPU for S { }
   |      ^^^ the trait `MyDisplay` is not implemented for `T`
   |
   = note: required because of the requirements on the impl of `MPU` for `S`

In my eyes the first one is clearly correct, while the second one is clearly incorrect.

Copy link
Member

Choose a reason for hiding this comment

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

How is that error even possible? Do we not check associated type defaults?!
cc @nikomatsakis did we create another type alias situation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That error happens if we use ty::Binder::dummy(*trait_ref) directly, it just means that trait_ref is not what we want as the parent_trait_ref.

Copy link
Member

Choose a reason for hiding this comment

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

The associated type default is malformed inside the trait.
There should be no errors in impls, only one in the trait itself.

Copy link
Member

Choose a reason for hiding this comment

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

One of the reasons I'm saying that is because this output:

   = note: required because of the requirements on the impl of `MPU` for `S`

Is the only meaningful cause for something that the impl caused, try examples that don't involve associated type defaults.

It only looks weird because the whole error is nonsensical to begin with.
It shouldn't error in the impl when the trait definition is clearly wrong.

@@ -16,6 +16,8 @@ LL | type MpuConfig: MyDisplay = T;
...
LL | impl MPU for S { }
| ^^^ the trait `MyDisplay` is not implemented for `T`
|
= note: required because of the requirements on the impl of `MyDisplay` for `<S as MPU>::MpuConfig`
Copy link
Member

Choose a reason for hiding this comment

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

Okay we probably do need something different because it's already handled by "required by this bound in MPU" above apparently, and better? So we end up with duplication that mentions "impl" even if none is responsible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that the current code appropriately handles these cases by adding a new DerivedObligation variant. I'd like to unify BuiltingDerivedObligation, ImplDerivedObligation and DerivedObligation and add a field to differentiate between them, but I haven't done so as it'll require changing a couple of extra places that I didn't want to muck about with earlier. I can do that in this PR if you prefer (but will add to the diff).

parent_trait_ref,
parent_code: Rc::new(obligation.cause.code.clone()),
};
cause.code = traits::ObligationCauseCode::ImplDerivedObligation(derived_cause);
extend_cause_with_original_assoc_item_obligation(
Copy link
Member

Choose a reason for hiding this comment

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

Maybe extend_cause_with_original_assoc_item_obligation should use a derived cause only if it doesn't apply an associated type bound cause?

@bors

This comment has been minimized.

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2020
@joelpalmer joelpalmer added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 6, 2020
@Dylan-DPC-zz

This comment has been minimized.

@estebank

This comment has been minimized.

@estebank estebank added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Apr 6, 2020
@estebank
Copy link
Contributor Author

estebank commented Apr 9, 2020

I'm trying out a couple of things to remove the current hack and actually propagate the spans correctly. If I get things right, the whole of extend_cause_with_original_assoc_item_obligation will go away. For now, I slashed a significant part of it and am using it only to point at the correct associated type.

Comment on lines 1 to +5
error[E0271]: type mismatch resolving `<std::vec::IntoIter<i32> as std::iter::Iterator>::Item == u32`
--> $DIR/traits-assoc-type-in-supertrait-bad.rs:11:6
--> $DIR/traits-assoc-type-in-supertrait-bad.rs:12:16
|
LL | impl Foo for IntoIter<i32> {
| ^^^ expected `i32`, found `u32`
LL | type Key = u32;
| ^^^ expected `i32`, found `u32`
Copy link
Contributor Author

@estebank estebank Apr 11, 2020

Choose a reason for hiding this comment

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

I'm looking at keeping enough information around for obligations born out of projections, but this might take a while. At the same time, these are the ones that will be the most useful to fix.

Note to self: For these we need to look in select.rs and project.rs and walk back from confirm_impl_candidate to see how to keep a chain of obligations instead of what we have now where we have a single step.

@estebank estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 11, 2020
@estebank
Copy link
Contributor Author

I think this PR is ready to be reviewed again.

| -------------
...
LL | + Deref<Target = str>
| ------------------- required by this bound in `UncheckedCopy`
Copy link
Contributor

Choose a reason for hiding this comment

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

really, this bound is on the associated type -- I've been wanting to refactor the setup that we use for this for some time, actually...I guess orthogonal from this PR, but we could potentially detect it and underline the associated type. I'm imagining that we look for a bound whose self type is <Self as Trait<...>>::Item basically.

Copy link
Contributor Author

@estebank estebank Apr 16, 2020

Choose a reason for hiding this comment

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

This is true, but for the output to work correctly we'll need to collect more information in BindingObligation. I'd prefer to do so in a separate PR, as this one is just making it more common, not introducing it. I think the changes will have to be in nominal_obligations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Separate PR seems good.

src/test/ui/associated-types/issue-65774-2.stderr Outdated Show resolved Hide resolved
@nikomatsakis
Copy link
Contributor

The code all seems fine to me, to be honest, but I have to admit I'm not really sure why these changes do what they do. =) I think I would need to spend more time reading into the surrounding code to bring it back in cache.

LL | type Ctx;
| --- associated type defined here
| --------- required by this bound in `WithType`
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we want to be careful about the implied Sized bounds? Perhaps something we can leave for follow-up.

...
LL | type Assoc = ChildWrapper<T::Assoc>;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Child<A>` is not implemented for `<T as Parent>::Assoc`
| ^^^^^^^^^^^^^^^^^^^^^^ the trait `Child<A>` is not implemented for `<T as Parent>::Assoc`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is mildly confusing, because the error here is only "indirectly" attributable to the associtaed type -- I assume it steps from the fact that we have an impl<A, T> Child<A> for ChildWrapper<T> where T: Child<A>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. And it's a case I would like to improve. I think that for this we need to look in select.rs or project.rs to keep more information there, right now we're dropping too much on the floor for us to figure these out.

@nikomatsakis
Copy link
Contributor

@bors r+

PS, @eddyb, not trying to step on your toes, just lift your burden -- I think your concerns were resolved, but if I'm wrong, feel free to r- or at least comment.

@bors
Copy link
Contributor

bors commented Apr 17, 2020

📌 Commit 71a763e9345ea790b747ffd8bf8364eb3ed4487f has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 17, 2020
@eddyb
Copy link
Member

eddyb commented Apr 17, 2020

Only thing I can see at a glance is descr methods.
Please use tcx.def_kind(def_id).descr() instead.

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 17, 2020
@eddyb
Copy link
Member

eddyb commented Apr 17, 2020

Although, uhh, #70043 hasn't landed yet, I wonder what's up with that.
EDIT: I'm taking over that PR and blocking this one on it.

@eddyb eddyb added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Apr 17, 2020
@estebank
Copy link
Contributor Author

@eddyb given that that PR is now blocked as well, I'm reworking the last commit to not introduce the new descr methods and instead of labels saying required by a bound in this trait changing them to required by a bound in this. That way we can land this PR and fix that label at a later time.

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

@bors r=nikomatsakis

I'm pushing to merge with the change in the previous comment. The queue is quite slow today so it should give you @eddyb enough time to revert the approval if you disagree :)

@bors
Copy link
Contributor

bors commented Apr 19, 2020

📌 Commit d9a5419 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 19, 2020
@bors
Copy link
Contributor

bors commented Apr 19, 2020

⌛ Testing commit d9a5419 with merge e7497a8...

@bors
Copy link
Contributor

bors commented Apr 19, 2020

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing e7497a8 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants