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

Blanket impl of AsRef for Deref #45742

Open
cramertj opened this issue Nov 3, 2017 · 5 comments
Open

Blanket impl of AsRef for Deref #45742

cramertj opened this issue Nov 3, 2017 · 5 comments
Labels
A-specialization Area: Trait impl specialization C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-blocked Status: Blocked on something else such as an RFC or other implementation work.

Comments

@cramertj
Copy link
Member

cramertj commented Nov 3, 2017

There are currently FIXMEs here and here in the core AsRef and AsMut impls to change the implementations for & and &mut to a blanket impl over Deref and DerefMut. However, this blanket impl results in a number of conflicts (even with deref). Because of this, I opened #45378 to remove the FIXMEs, but @withoutboats pointed out that we could potentially use intersection specialization to resolve the issue.

I'm not sure exactly what this impl (or set of impls) would look like, so I've closed the PR and opened this issue to track the resolution of the FIXMEs.

@TimNN TimNN added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Nov 7, 2017
@Mark-Simulacrum Mark-Simulacrum added A-specialization Area: Trait impl specialization S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jun 1, 2018
@meggarr
Copy link

meggarr commented Jul 29, 2019

Hi, & may I have a question here about the blanket impl,

impl<T: ?Sized, U: ?Sized> AsRef<U> for &T where T: AsRef<U>
{
    fn as_ref(&self) -> &U {
        <T as AsRef<U>>::as_ref(*self)
    }
}

It is implementing impl AsRef<U> for &T, then what is the type of self in fn as_ref(&self) -> &U? My understanding is self: &&T, is this correct?

JanBeh added a commit to JanBeh/rust that referenced this issue Jul 19, 2022
- Explicitly mention that `AsRef` and `AsMut` do not auto-dereference
  generally for all dereferencable types (but only if inner type is a
  shared and/or mutable reference)
- Give advice to not use `AsRef` or `AsMut` for the sole purpose of
  dereferencing
- Suggest providing a transitive `AsRef` or `AsMut` implementation for
  types which implement `Deref`
- Add new section "Reflexivity" in documentation comments for `AsRef`
  and `AsMut`
- Provide better example for `AsMut`
- Added heading "Relation to `Borrow`" in `AsRef`'s docs to improve
  structure

Issue rust-lang#45742 and a corresponding FIXME in the libcore suggest that
`AsRef` and `AsMut` should provide a blanket implementation over
`Deref`. As that is difficult to realize at the moment, this commit
updates the documentation to better describe the status-quo and to give
advice on how to use `AsRef` and `AsMut`.
@JanBeh
Copy link
Contributor

JanBeh commented Jul 20, 2022

There's currently a FIXME in the libcore AsRef and AsMut impls to change the implementations for & and &mut to a blanket impl over AsRef.

@cramertj: I assume that text is supposed to read: "to change the implementations for & and &mut to a blanket impl over AsRef Deref." Working links to the corresponding FIXMEs can be found here and here.

@JanBeh
Copy link
Contributor

JanBeh commented Jul 20, 2022

It is implementing impl AsRef<U> for &T, then what is the type of self in fn as_ref(&self) -> &U? My understanding is self: &&T, is this correct?

Yes, see Playground.

@JanBeh
Copy link
Contributor

JanBeh commented Jul 20, 2022

However, this blanket impl results in a number of conflicts (even with deref).

Disregarding other existing implementations, there would still be some problems with the mutable case (DerefMut), because types which implement DerefMut might not want to only provide an AsMut implementation in regard to their Deref::Target but also, for example, AsMut<Vec<T>> for Vec<T>. See this post on IRLO and the following example:

// This blanket implementation conflicts with the following 6 implementations
/*
impl<T, U> AsMut<U> for T
where
    T: ?Sized + DerefMut,
    U: ?Sized,
    <T as Deref>::Target: AsMut<U>,
{
    fn as_mut(&mut self) -> &mut U {
        self.deref_mut().as_mut()
    }
}
*/

// Would be covered by blanket implementation
impl<'a, T, U> AsMut<U> for &'a mut T
where
    T: ?Sized + AsMut<U>,
    U: ?Sized,
{
    fn as_mut(&mut self) -> &mut U {
        self.deref_mut().as_mut()
    }
}

// Would be covered by blanket implementation
impl<T, U> AsMut<U> for Box<T>
where
    T: ?Sized + AsMut<U>,
    U: ?Sized,
{
    fn as_mut(&mut self) -> &mut U {
        self.deref_mut().as_mut()
    }
}

// Would NOT covered by blanket implementation
// because `<String as Deref>::Target` is `str` and not `String`
impl AsMut<String> for String {
    fn as_mut(&mut self) -> &mut String {
        self
    }
}

// Would be covered by blanket implementation
// because `<String as Deref>::Target` is `str`
// and `str` implements `AsMut<str>`
impl AsMut<str> for String {
    fn as_mut(&mut self) -> &mut str {
        self
    }
}

// Would NOT covered by blanket implementation
// because `<Vec<T> as Deref>::Target` is `[T]` and not `Vec<T>`
impl<T> AsMut<Vec<T>> for Vec<T> {
    fn as_mut(&mut self) -> &mut Vec<T> {
        self
    }
}

// Would be covered by blanket implementation
// because `<Vec<T> as Deref>::Target` is `[T]`
// and `[T]` implements `AsMut<[T]>`
impl<T> AsMut<[T]> for Vec<T> {
    fn as_mut(&mut self) -> &mut [T] {
        self
    }
}

(Playground)

This problem doesn't seem to be exist for the AsRef case.

AFAIK, even if all existing implementations could be rewritten, the conflict above cannot be solved through negative bounds or specialization yet (though this might change in future). However, it would still possible to provide individual/manual implementations like the above for generic smart pointers such as Box.

That said, PR #28811 introduced different implementations for smart pointers, which was arguably a mistake as pointed out in this post on IRLO by me.

This mistake is difficult to fix. Generally code should refrain from using .as_ref() or .as_mut() for the sole purpose of dereferencing a smart pointer, but some code may rely on things like the following (example from the docs) to work:

fn add_one<T: AsMut<u64>>(num: &mut T) {
    *num.as_mut() += 1;
}

let mut boxed_num = Box::new(0);
add_one(&mut boxed_num);
assert_eq!(*boxed_num, 1);

(Note that in PR #99460 I suggest to replace that example, due to the reasons given here, such that new code will not expose this problem.)

Moreover, problems with type inference (i.e. missing type annotations) may arise when adding new implementations to AsRef or AsMut due to the way these traits are currently unfortunately used. (Playground to demonstrate this, and post on IRLO on that matter)

A potential solution has been outlined by @CAD97 here:

std could perhaps incrementally move towards this by introducing a lint against calling .as_ref()/.borrow() on concrete types (challenge: only when the result is not uniquely constrained by a separate constraint than available impls). Then, new impls causing inference breakage only can be provided with pre-warned breakage (alternatively, use edition-dependent method lookup to avoid the breakage).

This may or may not be feasible, but doesn't seem to be easy in either way. I would like if this issue could be fixed because it would provide a consistent API (see Playground). In regard to inconsistencies, also refer to #98905 and the first post in the referenced IRLO thread.

@cuviper
Copy link
Member

cuviper commented Sep 15, 2022

@JanBeh

I assume that text is supposed to read: "to change the implementations for & and &mut to a blanket impl over AsRef Deref." Working links to the corresponding FIXMEs can be found here and here.

I've updated the top issue text -- hope that helps!

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 3, 2022
…riplett

docs: Improve AsRef / AsMut docs on blanket impls

There are several issues with the current state of `AsRef` and `AsMut` as [discussed here on IRLO](https://internals.rust-lang.org/t/semantics-of-asref/17016). See also rust-lang#39397, rust-lang#45742, rust-lang#73390, rust-lang#98905, and the FIXMEs [here](https://github.com/rust-lang/rust/blob/1.62.0/library/core/src/convert/mod.rs#L509-L515) and [here](https://github.com/rust-lang/rust/blob/1.62.0/library/core/src/convert/mod.rs#L530-L536). These issues are difficult to fix. This PR aims to update the documentation to better reflect the status-quo and to give advice on how `AsRef` and `AsMut` should be used.

In particular:

- Explicitly mention that `AsRef` and `AsMut` do not auto-dereference generally for all dereferencable types (but only if inner type is a shared and/or mutable reference)
- Give advice to not use `AsRef` or `AsMut` for the sole purpose of dereferencing
- Suggest providing a transitive `AsRef` or `AsMut` implementation for types which implement `Deref`
- Add new section "Reflexivity" in documentation comments for `AsRef` and `AsMut`
- Provide better example for `AsMut`
- Added heading "Relation to `Borrow`" in `AsRef`'s docs to improve structure
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 3, 2022
…riplett

docs: Improve AsRef / AsMut docs on blanket impls

There are several issues with the current state of `AsRef` and `AsMut` as [discussed here on IRLO](https://internals.rust-lang.org/t/semantics-of-asref/17016). See also rust-lang#39397, rust-lang#45742, rust-lang#73390, rust-lang#98905, and the FIXMEs [here](https://github.com/rust-lang/rust/blob/1.62.0/library/core/src/convert/mod.rs#L509-L515) and [here](https://github.com/rust-lang/rust/blob/1.62.0/library/core/src/convert/mod.rs#L530-L536). These issues are difficult to fix. This PR aims to update the documentation to better reflect the status-quo and to give advice on how `AsRef` and `AsMut` should be used.

In particular:

- Explicitly mention that `AsRef` and `AsMut` do not auto-dereference generally for all dereferencable types (but only if inner type is a shared and/or mutable reference)
- Give advice to not use `AsRef` or `AsMut` for the sole purpose of dereferencing
- Suggest providing a transitive `AsRef` or `AsMut` implementation for types which implement `Deref`
- Add new section "Reflexivity" in documentation comments for `AsRef` and `AsMut`
- Provide better example for `AsMut`
- Added heading "Relation to `Borrow`" in `AsRef`'s docs to improve structure
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 3, 2022
…riplett

docs: Improve AsRef / AsMut docs on blanket impls

There are several issues with the current state of `AsRef` and `AsMut` as [discussed here on IRLO](https://internals.rust-lang.org/t/semantics-of-asref/17016). See also rust-lang#39397, rust-lang#45742, rust-lang#73390, rust-lang#98905, and the FIXMEs [here](https://github.com/rust-lang/rust/blob/1.62.0/library/core/src/convert/mod.rs#L509-L515) and [here](https://github.com/rust-lang/rust/blob/1.62.0/library/core/src/convert/mod.rs#L530-L536). These issues are difficult to fix. This PR aims to update the documentation to better reflect the status-quo and to give advice on how `AsRef` and `AsMut` should be used.

In particular:

- Explicitly mention that `AsRef` and `AsMut` do not auto-dereference generally for all dereferencable types (but only if inner type is a shared and/or mutable reference)
- Give advice to not use `AsRef` or `AsMut` for the sole purpose of dereferencing
- Suggest providing a transitive `AsRef` or `AsMut` implementation for types which implement `Deref`
- Add new section "Reflexivity" in documentation comments for `AsRef` and `AsMut`
- Provide better example for `AsMut`
- Added heading "Relation to `Borrow`" in `AsRef`'s docs to improve structure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-specialization Area: Trait impl specialization C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-blocked Status: Blocked on something else such as an RFC or other implementation work.
Projects
None yet
Development

No branches or pull requests

6 participants