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

Cow<'_, T> should not implement AsRef<T> but AsRef<U> where T: AsRef<U> #98905

Closed
JanBeh opened this issue Jul 4, 2022 · 13 comments
Closed

Cow<'_, T> should not implement AsRef<T> but AsRef<U> where T: AsRef<U> #98905

JanBeh opened this issue Jul 4, 2022 · 13 comments
Labels
C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@JanBeh
Copy link
Contributor

JanBeh commented Jul 4, 2022

I tried this code:

use std::borrow::Cow;
use std::path::Path;

fn func(_: impl AsRef<Path>) {}

fn main() {
    let s: &str = ".";
    let path: &Path = Path::new(&s);
    func(path);
    func(&path);
    func(Cow::Borrowed(path));
    func(s);
    func(&s);
    func(Cow::Borrowed(s));
}

I expected to see this happen: Code compiles and runs without error.

Instead, this happened:

error[E0277]: the trait bound `Cow<'_, str>: AsRef<Path>` is not satisfied
  --> src/main.rs:14:10
   |
14 |     func(Cow::Borrowed(s));
   |     ---- ^^^^^^^^^^^^^^^^ the trait `AsRef<Path>` is not implemented for `Cow<'_, str>`
   |     |
   |     required by a bound introduced by this call
   |
   = help: the following other types implement trait `AsRef<T>`:
             <Cow<'_, OsStr> as AsRef<Path>>
             <Cow<'_, T> as AsRef<T>>
note: required by a bound in `func`
  --> src/main.rs:4:17
   |
4  | fn func(_: impl AsRef<Path>) {}
   |                 ^^^^^^^^^^^ required by this bound in `func`

For more information about this error, try `rustc --explain E0277`.

More examples of (allegedly) inconsistent behavior can be found in this comment below. The cause is subtle and I suspect it is in std. I believe that

impl<T: ?Sized + ToOwned> AsRef<T> for Cow<'_, T> {
    fn as_ref(&self) -> &T {
        self
    }
}

should actually be:

impl<T, U> AsRef<U> for Cow<'_, T>
where
    T: ?Sized + ToOwned + AsRef<U>,
    U: ?Sized,
{
    fn as_ref(&self) -> &U {
        self.deref().as_ref()
    }
}

That is because .as_ref() can't generally be used to go from T to &T. It doesn't need to go from Cow<'_, T> to &T either. We have .borrow() for that, because there is an impl<T: ?Sized> Borrow<T> for T in std.

Instead, Cow should be transparent in regard to AsRef, i.e. if a type T implements AsRef<U>, then Cow<'_, T> should implement AsRef<U> too. Compare &T, which implements AsRef<U> when T: AsRef<U> (which is the reason why we can't have a generic impl<T: ?Sized> AsRef<T> for T).

See also this post on URLO.

Fixing this may be a breaking change and/or require a new edition, I guess.

Update moved up from comment below:

Note that Rc and Arc behave consistent to Cow:

All three are inconsistent with &:

I think that generic(!) smart pointers such as Cow, Rc, and Arc should behave the same as ordinary shared references in regard to AsRef, but they do not. Maybe there is a good reason why this isn't the case. In either case, it's not possible to solve this without breaking a lot of existing code, so I propose:

  • Keeping this issue in mind for future overhaul of std (if ever possible).
  • Documenting this issue properly.

Meta

rustc --version --verbose:

rustc 1.64.0-nightly (495b21669 2022-07-03)
binary: rustc
commit-hash: 495b216696ccbc27c73d6bdc486bf4621d610f4b
commit-date: 2022-07-03
host: x86_64-unknown-freebsd
release: 1.64.0-nightly
LLVM version: 14.0.6
@JanBeh JanBeh added the C-bug Category: This is a bug. label Jul 4, 2022
@JanBeh
Copy link
Contributor Author

JanBeh commented Jul 4, 2022

Also compare @conradludgate's Owned implementation, which contains a correct AsRef implementation (apart from missing U: ?Sized) and the crate deref_owned version 0.8.2, which also has a correct AsRef implementation for the Owned wrapper (which is similar to the enum variant Cow::Owned).

@Rageking8
Copy link
Contributor

@rustbot label T-compiler

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 5, 2022
@QuineDot
Copy link

QuineDot commented Jul 5, 2022

Arguably for your example, all that's missing is PR #73390. See that PR to see how it breaks inference in existing code, which your suggestion would do as well.

Fixing this may be a breaking change

Definitely breaking without PR #39397 (which would probably be more disruptive than PR #73390), as Cow<'_, T> will no longer implement AsRef<T> when T doesn't implement AsRef<T>.

@compiler-errors compiler-errors added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 5, 2022
@JanBeh
Copy link
Contributor Author

JanBeh commented Jul 5, 2022

Arguably for your example, all that's missing is PR #73390.

PR #73390 would be a workaround instead of solving the problem by fixing its cause. Note that it's not possible for external code to provide this workaround for types that are not defined in std.

See this extended example:

use std::borrow::Cow;
use std::path::Path;

#[derive(Clone)]
struct CurDir;

impl AsRef<Path> for CurDir {
    fn as_ref(&self) -> &Path {
        ".".as_ref()
    }
}

fn func(_: impl AsRef<Path>) {}

fn main() {
    let s: &str = ".";
    let path: &Path = Path::new(&s);
    func(path);
    func(&path);
    func(Cow::Borrowed(path));
    func(CurDir);
    func(&CurDir);
    func(Cow::Borrowed(&CurDir));
    func(s);
    func(&s);
    func(Cow::Borrowed(s));
}

I would also expect this code to compile and run without error, but it doesn't:

error[E0277]: the trait bound `Cow<'_, CurDir>: AsRef<Path>` is not satisfied
  --> src/main.rs:23:10
   |
23 |     func(Cow::Borrowed(&CurDir));
   |     ---- ^^^^^^^^^^^^^^^^^^^^^^ the trait `AsRef<Path>` is not implemented for `Cow<'_, CurDir>`
   |     |
   |     required by a bound introduced by this call
   |
   = help: the following other types implement trait `AsRef<T>`:
             <Cow<'_, OsStr> as AsRef<Path>>
             <Cow<'_, T> as AsRef<T>>
note: required by a bound in `func`
  --> src/main.rs:13:17
   |
13 | fn func(_: impl AsRef<Path>) {}
   |                 ^^^^^^^^^^^ required by this bound in `func`

error[E0277]: the trait bound `Cow<'_, str>: AsRef<Path>` is not satisfied
  --> src/main.rs:26:10
   |
26 |     func(Cow::Borrowed(s));
   |     ---- ^^^^^^^^^^^^^^^^ the trait `AsRef<Path>` is not implemented for `Cow<'_, str>`
   |     |
   |     required by a bound introduced by this call
   |
   = help: the following other types implement trait `AsRef<T>`:
             <Cow<'_, OsStr> as AsRef<Path>>
             <Cow<'_, T> as AsRef<T>>
note: required by a bound in `func`
  --> src/main.rs:13:17
   |
13 | fn func(_: impl AsRef<Path>) {}
   |                 ^^^^^^^^^^^ required by this bound in `func`

For more information about this error, try `rustc --explain E0277`.

It's also (obviously) not possible to add a workaround where needed:

use std::borrow::Cow;
use std::path::Path;

#[derive(Clone)]
struct CurDir;

impl AsRef<Path> for CurDir {
    fn as_ref(&self) -> &Path {
        ".".as_ref()
    }
}

impl<'a> AsRef<Path> for Cow<'a, CurDir> {
    fn as_ref(&self) -> &Path {
        self.deref().as_ref()
    }
}

Which gives (this is not unexpected):

error[E0117]: only traits defined in the current crate can be implemented for types defined outside of the crate
  --> src/main.rs:13:1
   |
13 | impl<'a> AsRef<Path> for Cow<'a, CurDir> {
   | ^^^^^^^^^-----------^^^^^---------------
   | |        |               |
   | |        |               `Cow` is not defined in the current crate
   | |        `Path` is not defined in the current crate
   | impl doesn't use only types from inside the current crate
   |
   = note: define and implement a trait or new type instead

For more information about this error, try `rustc --explain E0117`.

I conclude that PR #73390 is semantically wrong. Instead the cause of the problem should be fixed as proposed in my original post.

I understand that this is breaking. However, given the current semantics of AsRef and Borrow, I consider impl<'a, T: ?Sized> AsRef<T> for Cow<'a, T> to be an error. I also consider the following example code to be semantically wrong (i.e. I do not think that the following example must compile (edit: unless we have impl<T: ?Sized> AsRef<T> for T one day), even though it may and does):

use std::borrow::{Borrow, Cow};

#[derive(Clone)]
struct Unit;

fn main() {
    let unit_ref = &Unit;
    let unit_cow: Cow<'_, Unit> = Cow::Borrowed(unit_ref);
    // This does not work (we should use `.borrow()` here):
    // let _: &Unit = Unit.as_ref();
    // But this works (but should not be guaranteed to work):
    let _: &Unit = unit_cow.as_ref();
    // Semantically correct is to use `.borrow()` instead of `.as_ref()` in both cases:
    let _: &Unit = Unit.borrow();
    let _: &Unit = unit_cow.borrow();
}

If the cause of the issue cannot be fixed, it should be at least documented as an error in Cow's documentation.

However, leaving Cow as it is is pretty ugly, considering that Cow is a pretty basic data type and should act like a reference replacement.

@JanBeh
Copy link
Contributor Author

JanBeh commented Jul 5, 2022

Fixing this may be a breaking change

Definitely breaking without PR #39397

However, leaving Cow as it is is pretty ugly, considering that Cow is a pretty basic data type […]

For a meta discussion on this kind of problems, I opened a new thread on IRLO: Rust's complexity + Rust's stability policy = ?

@QuineDot
Copy link

QuineDot commented Jul 5, 2022

PR #73390 would be a workaround instead of solving the problem by fixing its cause. Note that it's not possible for external code to provide this workaround for types that are not defined in std.

That's why I used the phrasing I did. The point is, even fixing the single use-case of the original example was deemed too disruptive, and it isn't even a major breaking change.

I conclude that PR #73390 is semantically wrong. Instead the cause of the problem should be fixed as proposed in my original post.

Even if there's a way to keep the inference breakage from being too disruptive, it's a major breaking change that can't happen before intersection specialization or Rust 2.0, modulo some other reversion of Rust's backwards compatibility guarantees. That is, the working example you give should indeed and is indeed guaranteed to work, must compile, as per the blanket implementation on Cow which has existed since (before) Rust 1.0.0.

I agree the existing medley of traits and implementations are not perfect, but sometimes theoretical purity must bow to practicality.

@JanBeh
Copy link
Contributor Author

JanBeh commented Jul 5, 2022

A potential migration path might be to provide something like deref_owned::GenericCow, deprecate Cow, and reintroduce a fixed version of Cow, where both the old and the new version of Cow implement GenericCow.

@JanBeh
Copy link
Contributor Author

JanBeh commented Jul 5, 2022

I had an error in my original proposed solution. This has been corrected (by editing) as follows:

 impl<T, U> AsRef<U> for Cow<'_, T>
 where
-    T: AsRef<U>,
+    T: ?Sized + ToOwned + AsRef<U>,
     U: ?Sized,
 {
     fn as_ref(&self) -> &U {
         self.deref().as_ref()
     }
 }

@JanBeh
Copy link
Contributor Author

JanBeh commented Jul 6, 2022

Instead of seeing the cause of the problem in Cows AsRef implementation (as assumed in my original post), the real cause might also be seen much deeper here:

// As lifts over &
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_unstable(feature = "const_convert", issue = "88674")]
impl<T: ?Sized, U: ?Sized> const AsRef<U> for &T
where
    T: ~const AsRef<U>,
{
    #[inline]
    fn as_ref(&self) -> &U {
        <T as AsRef<U>>::as_ref(*self)
    }
}

See @QuineDot's post on URLO. Following his reasoning, many occurrences of P where P: impl AsRef<Path> should rather be &P where P: impl ?Sized + AsRef<Path>.

All this is non-fixable, of course.

However, for the sake of completeness, I would like to outline another (hypothetical) non-breaking solution for this issue in the following.

I previously proposed:

A potential migration path might be to provide something like deref_owned::GenericCow, deprecate Cow, and reintroduce a fixed version of Cow, where both the old and the new version of Cow implement GenericCow.

But instead, AsRef could be deprecated and be replaced with a new version (calling it AsRef2 for purpose of referencing) that doesn't provide an impl<T: ?Sized, U: ?Sized> AsRef2<U> for &T but instead impl<T: ?Sized> AsRef2<T> for T. AsRef2 would need to be used differently, for example:

-pub fn open<P: AsRef<Path>>(path: P) -> Result<File>`
+pub fn open<P: ?Sized + AsRef2<Path>>(path: &P) -> Result<File>

Then Cow might or might not implement AsRef2 in the same way as Cow currently implements AsRef, or not implement AsRef2 at all (this is a detail question, I believe).

Please note that this is not a suggested solution but merely a thought-experiment to better understand the cause of the problem.

@JanBeh
Copy link
Contributor Author

JanBeh commented Jul 7, 2022

I would like to state that the thesis / topic of this issue report

Cow<'_, T> should not implement AsRef<T> but AsRef<U> where T: AsRef<U>

might require far more discussion, see second (technical) part of my post here.

I.e. I'm not sure if my proposed (hypothetical) solutions really are the right way to go (apart from being practical). The underlying problems might be more complicated/tricky.

I would like to clarify that I do not propose any immediate action on this issue but wanted to point out inconsistencies, of which some can be seen here:

fn main() {
    // using `.borrow()`:
    let _: &i32 = 0i32.borrow();
    let _: &i32 = Cow::<i32>::Owned(0i32).borrow();
    let _: &i32 = Cow::Borrowed(&0i32).borrow();
    /* … */
    // using deref-coercion:
    let _: &i32 = &0i32;
    let _: &i32 = &Cow::<i32>::Owned(0i32);
    let _: &i32 = &Cow::Borrowed(&0i32);
    /* … */
    // using `.as_ref()`:
    //let _: &i32 = 0i32.as_ref(); // Won't work
    let _: &i32 = Cow::<i32>::Owned(0i32).as_ref();
    let _: &i32 = Cow::Borrowed(&0i32).as_ref();
    /* … */
}

(Playground)

And here:

use std::borrow::Cow;
use std::ffi::OsStr;
use std::path::Path;

fn foo(_: impl AsRef<Path>) {}
fn bar(_: impl AsRef<i32>) {}

fn main() {
    foo("Hi!");
    foo("How are you?".to_string());
    foo(&&&&&&("I'm fine.".to_string()));
    foo(&&&&&&&&&&&&&&&&&&&"Still doing well here.");
    //bar(5); // ouch!
    //bar(&5); // hmmm!?
    bar(Cow::Borrowed(&5)); // phew!
    foo(Cow::Borrowed(OsStr::new("Okay, let me help!")));
    foo(&&&&&&&&Cow::Borrowed(OsStr::new("This rocks!!")));
    //foo(&&&&&&&&Cow::Borrowed("BANG!!"));
}

(Playground)

@JanBeh
Copy link
Contributor Author

JanBeh commented Jul 7, 2022

There would be two potential ways to fix this, of which none are really feasible at this point, I believe:

Migration path A:

[…] provide something like deref_owned::GenericCow, deprecate Cow, and reintroduce a fixed version of Cow, where both the old and the new version of Cow implement GenericCow.

Note that this would also need to be done with Rc, Arc, and maybe more types (which would make migration even more painful).

Migration path B:

AsRef could be deprecated and be replaced with a new version (calling it AsRef2 for purpose of referencing) […]

Choosing migration path B, one of the following mutually exclusive implementations could be chosen:

  • impl<T: ?Sized> AsRef2<U> for &T (variant B1)
  • impl<T: ?Sized + AsRef2<U>, U: ?Sized> for &T (variant B2)

It is possible that variant B1 would cause practical implications due to lack of transitivity of AsRef/AsRef2 (which is likely why variant B2 was originally chosen for AsRef in Rust).


In no case either of these migration paths appear feasible by now, but the inconsistency remains.

Edit: Moved some parts to the OP for a better overview.

@JanBeh
Copy link
Contributor Author

JanBeh commented Jul 9, 2022

Using .borrow() as a generic way to go to the borrowed type may introduce a need for extra type annotations in some cases. That may be one reason why Cow<'_, T> implements AsRef<T>, despite the inconsistencies (in regard to &) that arise from it.

@JanBeh
Copy link
Contributor Author

JanBeh commented Jul 15, 2022

There is an older issue #45742 (Blanket impl of AsRef for Deref) referring to an old FIXME comment in library/core/src/convert/mod.rs:

// As lifts over &
/* … */
impl<T: ?Sized, U: ?Sized> const AsRef<U> for &T
where
    T: ~const AsRef<U>,
{
    /* … */
}

// As lifts over &mut
/* … */
impl<T: ?Sized, U: ?Sized> const AsRef<U> for &mut T
where
    T: ~const AsRef<U>,
{
    /* … */
}

// FIXME (#45742): replace the above impls for &/&mut with the following more general one:
// // As lifts over Deref
// impl<D: ?Sized + Deref<Target: AsRef<U>>, U: ?Sized> AsRef<U> for D {
//     fn as_ref(&self) -> &U {
//         self.deref().as_ref()
//     }
// }

Since this issue (#98905) is a special case (only addressing Cow), I would propose closing this issue and refer to #45742 and the already existing FIXME in the source.

Documentation should still be improved on that matter. I started a discussion regarding the semantics of AsRef on IRLO. Depending on the outcome of that discussion, a documentation issue could be opened or a PR to improve documentation of the status-quo accordingly.

@JanBeh JanBeh closed this as not planned Won't fix, can't repro, duplicate, stale Jul 15, 2022
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
C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants