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

Remove unfixable AsRef FIXMEs. #45378

Closed
wants to merge 1 commit into from
Closed

Conversation

cramertj
Copy link
Member

@cramertj cramertj commented Oct 19, 2017

A long time ago, FIXMEs were left to implement
AsRef and AsRef for all types which implemented
Deref and DerefMut. This wasn't done at the time
because of #23442.

Unfortunately, it's not possible to add these
implementations backwards-compabily, since they
can cause conflicting implementation overlaps.

If anyone has any ideas about how we could add these in a backwards-compatible way, please say something! I'd really like to have these impls, as well as an impl<T> AsRef<T> for T. However, all of these impls cause overlaps with things like the impl<T> AsRef<Vec<T>> for Vec<T>.

@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

A long time ago, FIXMEs were left to implement
AsRef and AsRef for all types which implemented
Deref and DerefMut. This wasn't done at the time
because of rust-lang#23442.

Unfortunately, it's not possible to add these
implementations backwards-compabily, since they
can cause conflicting implementation overlaps.
@cramertj
Copy link
Member Author

If anyone is curious, the exact errors you get when simply commenting out these impls and deleting the impls for &T and &mut T are as follows:

error[E0119]: conflicting implementations of trait `core::convert::AsRef<_>` for type `boxed::Box<_>`:
   --> src/liballoc/boxed.rs:855:1
    |
855 | / impl<T: ?Sized> AsRef<T> for Box<T> {
856 | |     fn as_ref(&self) -> &T {
857 | |         &**self
858 | |     }
859 | | }
    | |_^
    |
    = note: conflicting implementation in crate `core`

error[E0119]: conflicting implementations of trait `core::convert::AsRef<_>` for type `arc::Arc<_>`:
    --> src/liballoc/arc.rs:1857:1
     |
1857 | / impl<T: ?Sized> AsRef<T> for Arc<T> {
1858 | |     fn as_ref(&self) -> &T {
1859 | |         &**self
1860 | |     }
1861 | | }
     | |_^
     |
     = note: conflicting implementation in crate `core`

error[E0119]: conflicting implementations of trait `core::convert::AsRef<_>` for type `rc::Rc<_>`:
    --> src/liballoc/rc.rs:1798:1
     |
1798 | / impl<T: ?Sized> AsRef<T> for Rc<T> {
1799 | |     fn as_ref(&self) -> &T {
1800 | |         &**self
1801 | |     }
1802 | | }
     | |_^
     |
     = note: conflicting implementation in crate `core`

error[E0119]: conflicting implementations of trait `core::convert::AsRef<_>` for type `borrow::Cow<'_, _>`:
   --> src/liballoc/borrow.rs:359:1
    |
359 | / impl<'a, T: ?Sized + ToOwned> AsRef<T> for Cow<'a, T> {
360 | |     fn as_ref(&self) -> &T {
361 | |         self
362 | |     }
363 | | }
    | |_^
    |
    = note: conflicting implementation in crate `core`

error[E0119]: conflicting implementations of trait `core::convert::AsRef<vec::Vec<_>>` for type `vec::Vec<_>`:
    --> src/liballoc/vec.rs:2150:1
     |
2150 | / impl<T> AsRef<Vec<T>> for Vec<T> {
2151 | |     fn as_ref(&self) -> &Vec<T> {
2152 | |         self
2153 | |     }
2154 | | }
     | |_^
     |
     = note: conflicting implementation in crate `core`

error[E0119]: conflicting implementations of trait `core::convert::AsMut<_>` for type `boxed::Box<_>`:
   --> src/liballoc/boxed.rs:862:1
    |
862 | / impl<T: ?Sized> AsMut<T> for Box<T> {
863 | |     fn as_mut(&mut self) -> &mut T {
864 | |         &mut **self
865 | |     }
866 | | }
    | |_^
    |
    = note: conflicting implementation in crate `core`

error[E0119]: conflicting implementations of trait `core::convert::AsMut<vec::Vec<_>>` for type `vec::Vec<_>`:
    --> src/liballoc/vec.rs:2157:1
     |
2157 | / impl<T> AsMut<Vec<T>> for Vec<T> {
2158 | |     fn as_mut(&mut self) -> &mut Vec<T> {
2159 | |         self
2160 | |     }
2161 | | }
     | |_^
     |
     = note: conflicting implementation in crate `core`

With the exception of the impl<T> AsRef<Vec<T>> for Vec<T>, these can all be fixed pretty easily since it's just a matter of removing the duplicate Deref and AsRef impls. However, the same issue would be encountered for any custom container type which implemented both Deref and AsRef for its contents.

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

review ping for you @aturon, pinging you on irc too!

@carols10cents
Copy link
Member

ping @rust-lang/lang, could one of yinz be a new reviewer for this please?

@withoutboats
Copy link
Contributor

@cramertj haven't read this closely but aren't these enabled by intersection specialization? I think the FIXMEs were put there to track them for when that is implemented

(at the time we thought it would be much sooner!)

@shepmaster
Copy link
Member

Moving to another lang-team member...

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned aturon Nov 3, 2017
@shepmaster shepmaster added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Nov 3, 2017
@cramertj
Copy link
Member Author

cramertj commented Nov 3, 2017

@withoutboats I don't see quite how lattice/intersection specialization would work here. What pair of intersection impls are you imagining? (it's entirely possible that there is a solution through specialization, but I don't know what it would be)

@cramertj
Copy link
Member Author

cramertj commented Nov 3, 2017

Since it seems like there is some discussion that needs to happen here, I think an issue better for this than a PR. I've opened #45742 to track the resolution of the FIXMEs.

@cramertj cramertj closed this Nov 3, 2017
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. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants