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

Elaborate trait obligations when typechecking impls #43786

Merged
merged 6 commits into from
Aug 25, 2017

Conversation

scalexm
Copy link
Member

@scalexm scalexm commented Aug 10, 2017

When typechecking trait impl declarations, we only checked that bounds explictly written on the trait declaration hold.

We now also check that bounds which would have been implied by the trait reference do hold.

Fixes #43784.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@scalexm
Copy link
Member Author

scalexm commented Aug 10, 2017

r? @aturon

@rust-highfive rust-highfive assigned aturon and unassigned pnkfelix Aug 10, 2017
@@ -156,7 +175,7 @@ impl<'a, 'gcx, 'tcx> WfPredicates<'a, 'gcx, 'tcx> {
// WF and (b) the trait-ref holds. (It may also be
// normalizable and be WF that way.)
let trait_ref = data.trait_ref(self.infcx.tcx);
self.compute_trait_ref(&trait_ref);
self.compute_trait_ref(&trait_ref, Elaborate::All);
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we could just have Elaborate::None here, since the implied obligations are proved once and for all when typechecking the impl declaration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably .. yes ..? Should we change this then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes we should change this (I'll let you do that :) )

@alexcrichton alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2017
@carols10cents
Copy link
Member

friendly ping @aturon !

@nikomatsakis
Copy link
Contributor

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned aturon Aug 18, 2017
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

OK, I finally got over jet lag enough to understand #43784 and to see why this will fix the problem. My one complaint with this PR, really, is that what is happening here is non-obvious. I'd like to see a comment on the Elaborate enum that links to #43784 and explains the purpose of this enum.

@scalexm
Copy link
Member Author

scalexm commented Aug 23, 2017

Ok, will do that soon.

@nikomatsakis
Copy link
Contributor

@scalexm just did it =) maybe check over what I wrote and see if you agree

@nikomatsakis
Copy link
Contributor

(I wanted to write out the argument to see if I really understood it ;)

@scalexm
Copy link
Member Author

scalexm commented Aug 23, 2017

Yes I think it's good :)

@nikomatsakis
Copy link
Contributor

@scalexm should we change this do you think ? #43786 (comment)

@scalexm
Copy link
Member Author

scalexm commented Aug 23, 2017

@nikomatsakis yes I think we should change to Elaborate::None.

@nikomatsakis
Copy link
Contributor

@scalexm ok, care to push a change? want me to do it?

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 25, 2017

📌 Commit 68fd322 has been approved by nikomatsakis

bors added a commit that referenced this pull request Aug 25, 2017
Elaborate trait obligations when typechecking impls

When typechecking trait impl declarations, we only checked that bounds explictly written on the trait declaration hold.

We now also check that bounds which would have been implied by the trait reference do hold.

Fixes #43784.
@bors
Copy link
Contributor

bors commented Aug 25, 2017

⌛ Testing commit 68fd322 with merge 426711d...

@bors
Copy link
Contributor

bors commented Aug 25, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 426711d to master...

@bors bors merged commit 68fd322 into rust-lang:master Aug 25, 2017
@est31 est31 mentioned this pull request Sep 9, 2017
@scalexm scalexm deleted the issue-43784 branch October 22, 2018 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants