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

Projects
None yet
8 participants
@scalexm
Copy link
Member

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

This comment has been minimized.

Copy link
Collaborator

commented Aug 10, 2017

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

This comment has been minimized.

Copy link
Member Author

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);

This comment has been minimized.

Copy link
@scalexm

scalexm Aug 10, 2017

Author Member

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

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Aug 23, 2017

Contributor

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

This comment has been minimized.

Copy link
@scalexm

scalexm Aug 23, 2017

Author Member

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

@carols10cents

This comment has been minimized.

Copy link
Member

commented Aug 14, 2017

friendly ping @aturon !

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2017

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

@nikomatsakis
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2017

Ok, will do that soon.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2017

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2017

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

@scalexm

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2017

Yes I think it's good :)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2017

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

@scalexm

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2017

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2017

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2017

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2017

📌 Commit 68fd322 has been approved by nikomatsakis

bors added a commit that referenced this pull request Aug 25, 2017

Auto merge of #43786 - scalexm:issue-43784, r=nikomatsakis
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

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2017

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

@bors

This comment has been minimized.

Copy link
Contributor

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

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@est31 est31 referenced this pull request Sep 9, 2017

Closed

downcast beta regression #44445

@scalexm scalexm deleted the scalexm:issue-43784 branch Oct 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.