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

Coherence test when a generic type param has a default value from an associated type #61535

Conversation

ohadravid
Copy link
Contributor

@ohadravid ohadravid commented Jun 5, 2019

A followup on #61400.
Before re_rebalance_coherence, this fails to compile (even though it should be accepted).
re_rebalance_coherence had no direct test for this, and I wanted to (a) make sure it doesn't regress in the future and (b) get it on record that this is actually the intended behavior.

@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 @alexcrichton (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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 5, 2019
@ohadravid
Copy link
Contributor Author

r? @jonas-schievink

@jonas-schievink
Copy link
Contributor

r? @nikomatsakis

Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I'll let Niko take a look as well since I don't really know much about this feature

@Dylan-DPC-zz
Copy link

ping from triage @nikomatsakis @jonas-schievink any updates on this?

type Owned = String;
}

impl PartialEq<MyString> for LibCow<MyString> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test doesn't really check what the default works out to be, right? Should we do that?

#![allow(dead_code)]
#![feature(re_rebalance_coherence)]

// run-pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we leave a comment elaborating what this test is trying to test?

@ohadravid ohadravid force-pushed the test-generic-with-default-assiociated-type-re-rebalance-coherence branch 2 times, most recently from e8ac568 to ef7d1cd Compare July 9, 2019 15:33
@ohadravid ohadravid force-pushed the test-generic-with-default-assiociated-type-re-rebalance-coherence branch from ef7d1cd to 1948140 Compare July 9, 2019 15:35
@ohadravid
Copy link
Contributor Author

Hi @nikomatsakis!
Thanks for looking at this. I added a comment about what we are testing here.
I also added a check that we really get the expected type.

@nikomatsakis
Copy link
Contributor

@bors r+ rollup

@nikomatsakis
Copy link
Contributor

@bors r+

@nikomatsakis nikomatsakis 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 Jul 12, 2019
@lqd lqd closed this Jul 12, 2019
@lqd lqd reopened this Jul 12, 2019
@lqd
Copy link
Member

lqd commented Jul 12, 2019

@bors r=nikomatsakis rollup

@bors
Copy link
Contributor

bors commented Jul 12, 2019

📌 Commit 1948140 has been approved by nikomatsakis

Centril added a commit to Centril/rust that referenced this pull request Jul 12, 2019
…-assiociated-type-re-rebalance-coherence, r=nikomatsakis

Coherence test when a generic type param has a default value from an associated type

A followup on rust-lang#61400.
Before `re_rebalance_coherence`, this fails to compile (even though it should be accepted).
`re_rebalance_coherence` had no direct test for this, and I wanted to (a) make sure it doesn't regress in the future and (b) get it on record that this is actually the intended behavior.
bors added a commit that referenced this pull request Jul 12, 2019
Rollup of 12 pull requests

Successful merges:

 - #61535 (Coherence test when a generic type param has a default value from an associated type)
 - #62274 (rustc_mir: follow FalseUnwind's real_target edge in qualify_consts.)
 - #62431 (Add messages to `Option`'s and `Result`'s `must_use` annotation for `is_*`)
 - #62453 (in which we suggest anonymizing single-use lifetimes in paths )
 - #62568 (Replace unsafe_destructor_blind_to_params with may_dangle)
 - #62578 (Add test for #49919)
 - #62595 (Document that the crate keyword refers to the project root)
 - #62599 (move mem::uninitialized deprecation back by 1 release, to 1.39)
 - #62605 (Emit dropped unemitted errors to aid in ICE debugging)
 - #62607 (Correctly break out of recovery loop)
 - #62608 (`async unsafe fn` tests)
 - #62623 (downgrade indirect_structural_match lint to allow)

Failed merges:

r? @ghost
@bors bors merged commit 1948140 into rust-lang:master Jul 13, 2019
@ohadravid ohadravid deleted the test-generic-with-default-assiociated-type-re-rebalance-coherence branch October 23, 2019 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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