Skip to content

Coherence should allow fundamental types to impl traits when they are local - #65738

Merged
bors merged 1 commit into
rust-lang:masterfrom
ohadravid:re-rebalance-coherence-allow-fundamental-local
Oct 27, 2019
Merged

Coherence should allow fundamental types to impl traits when they are local#65738
bors merged 1 commit into
rust-lang:masterfrom
ohadravid:re-rebalance-coherence-allow-fundamental-local

Conversation

@ohadravid

@ohadravidohadravid commented Oct 23, 2019

Copy link
Copy Markdown
Contributor

After #64414, impl<T> Remote for Box<T> { } is disallowed, but it is also disallowed in liballoc, where Box is a local type!

Enabling #![feature(re_rebalance_coherence)] in liballoc results in:

error[E0210]: type parameter `F` must be used as the type parameter for some local type (e.g., `MyStruct<F>`)
--> src\liballoc\boxed.rs:1098:1
|
1098 | impl<F: ?Sized + Future + Unpin> Future for Box<F> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type parameter `F` must be used as the type parameter for some local type

This PR relaxes uncover_fundamental_ty to skip local fundamental types.
I didn't add a test since liballoc already fails to compile, but I can add one if needed.

r? @nikomatsakis

cc #63599

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 23, 2019
@ohadravid
ohadravidforce-pushed the re-rebalance-coherence-allow-fundamental-local branch from f36742a to 67953fdCompareOctober 23, 2019 21:40
@CentrilCentril added the F-re_rebalance_coherence `#![feature(re_rebalance_coherence)]` label Oct 24, 2019
@nikomatsakis

Copy link
Copy Markdown
Contributor

Heh, good catch @ohadravid -- I'm torn about the test. You're right that we don't need one in some sense but I think if you don't mind adding one it would help us to prevent similar oversights in the future.

@ohadravid
ohadravidforce-pushed the re-rebalance-coherence-allow-fundamental-local branch from 67953fd to 8f988bdCompareOctober 26, 2019 11:09
@ohadravid

ohadravid commented Oct 26, 2019

Copy link
Copy Markdown
ContributorAuthor

@nikomatsakis I added two tests which should prevent a regression.

BTW I can also open the stabilization PR for re_rebalance_coherence, I just wasn't sure if it was taken or not.

Original failures are:

LL | impl Remote for Local {}
| ^^^^^^^^^^^^^^^^^^^^^ impl doesn't use types inside crate

and

LL | impl<T> Remote for MyBox<T> {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ type parameter `T` must be used as the type parameter for some local type 

@nikomatsakis

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Oct 26, 2019

Copy link
Copy Markdown
Collaborator

📌 Commit 8f988bd has been approved by nikomatsakis

@borsbors 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 Oct 26, 2019
@nikomatsakis

Copy link
Copy Markdown
Contributor

@ohadravid thanks! And yes, a stabilization PR would be great! Please explicitly add r? @nikomatsakis on the PR, too.

Centril added a commit to Centril/rust that referenced this pull request Oct 26, 2019
…low-fundamental-local, r=nikomatsakis
Coherence should allow fundamental types to impl traits when they are local
After rust-lang#64414, `impl<T> Remote for Box<T> { }` is disallowed, but it is also disallowed in liballoc, where `Box` is a local type!
Enabling `#![feature(re_rebalance_coherence)]` in `liballoc` results in:
```
error[E0210]: type parameter `F` must be used as the type parameter for some local type (e.g., `MyStruct<F>`)
--> src\liballoc\boxed.rs:1098:1
|
1098 | impl<F: ?Sized + Future + Unpin> Future for Box<F> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type parameter `F` must be used as the type parameter for some local type
```
This PR relaxes `uncover_fundamental_ty` to skip local fundamental types.
I didn't add a test since `liballoc` already fails to compile, but I can add one if needed.
r? @nikomatsakis
cc rust-lang#63599
@CentrilCentril mentioned this pull request Oct 26, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 26, 2019
…low-fundamental-local, r=nikomatsakis
Coherence should allow fundamental types to impl traits when they are local
After rust-lang#64414, `impl<T> Remote for Box<T> { }` is disallowed, but it is also disallowed in liballoc, where `Box` is a local type!
Enabling `#![feature(re_rebalance_coherence)]` in `liballoc` results in:
```
error[E0210]: type parameter `F` must be used as the type parameter for some local type (e.g., `MyStruct<F>`)
--> src\liballoc\boxed.rs:1098:1
|
1098 | impl<F: ?Sized + Future + Unpin> Future for Box<F> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type parameter `F` must be used as the type parameter for some local type
```
This PR relaxes `uncover_fundamental_ty` to skip local fundamental types.
I didn't add a test since `liballoc` already fails to compile, but I can add one if needed.
r? @nikomatsakis
cc rust-lang#63599
@CentrilCentril mentioned this pull request Oct 26, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 27, 2019
…low-fundamental-local, r=nikomatsakis
Coherence should allow fundamental types to impl traits when they are local
After rust-lang#64414, `impl<T> Remote for Box<T> { }` is disallowed, but it is also disallowed in liballoc, where `Box` is a local type!
Enabling `#![feature(re_rebalance_coherence)]` in `liballoc` results in:
```
error[E0210]: type parameter `F` must be used as the type parameter for some local type (e.g., `MyStruct<F>`)
--> src\liballoc\boxed.rs:1098:1
|
1098 | impl<F: ?Sized + Future + Unpin> Future for Box<F> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type parameter `F` must be used as the type parameter for some local type
```
This PR relaxes `uncover_fundamental_ty` to skip local fundamental types.
I didn't add a test since `liballoc` already fails to compile, but I can add one if needed.
r? @nikomatsakis
cc rust-lang#63599
@CentrilCentril mentioned this pull request Oct 27, 2019
bors added a commit that referenced this pull request Oct 27, 2019
Rollup of 6 pull requests
Successful merges:
- #65566 (Use heuristics to suggest assignment)
- #65738 (Coherence should allow fundamental types to impl traits when they are local)
- #65777 (Don't ICE for completely unexpandable `impl Trait` types)
- #65834 (Remove lint callback from driver)
- #65839 (Clean up `check_consts` now that new promotion pass is implemented)
- #65855 (Add long error explaination for E0666)
Failed merges:
r? @ghost
@bors
bors merged commit 8f988bd into rust-lang:masterOct 27, 2019
@bors

bors commented Oct 27, 2019

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #65869) made this pull request unmergeable. Please resolve the merge conflicts.

@borsbors 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 Oct 27, 2019
@ohadravid
ohadravid deleted the re-rebalance-coherence-allow-fundamental-local branch December 2, 2019 11:20
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

F-re_rebalance_coherence`#![feature(re_rebalance_coherence)]`S-waiting-on-authorStatus: This is awaiting some action (such as code changes or more information) from the author.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@ohadravid@nikomatsakis@bors@Centril@rust-highfive