New failable API for SmartPointer<OsStr> to SmartPointer<str>, etc

[NobodyXu]

Proposal

Problem statement

cc uses Arc<OsStr> a lot (for env and flags) and we sometimes need them converted to a string.

Since these Arcs are wrapped with lock, it can't return &str but would have to convert them to String.

Motivating examples or use cases

https://github.com/rust-lang/cc-rs/blob/6fa9ea631509f0dc2f6373121e2ec9db4e5f8001/src/lib.rs#L3558

while we can always convert it to String it is less efficient and less ergonomic over an API to convert Arc<OsStr> to Arc<str> in a faillable way.

Solution sketch

impl TryFrom<Arc<OsStr>> for Arc<str> {
    type Err = Utf8Error;
    fn try_from(os_str: Arc<OsStr>) -> Self;
}
impl TryFrom<Rc<OsStr>> for Rc<str> {
    type Err = Utf8Error;
    fn try_from(os_str: Rc<OsStr>) -> Self;
}
impl TryFrom<Box<OsStr>> for Box<str> {
    type Err = Utf8Error;
    fn try_from(os_str: Box<OsStr>) -> Self;
}

impl From<Arc<OsStr>> for Arc<Path> {}
impl From<Rc<OsStr>> for Rc<Path> {}
impl From<Box<OsStr>> for Box<Path> {}

impl From<Arc<str>> for Arc<OsStr> {}
impl From<Rc<str>> for Rc<OsStr> {}
impl From<Box<str>> for Box<OsStr> {}

impl From<Arc<Path>> for Arc<OsStr> {}
impl From<Rc<Path>> for Rc<OsStr> {}
impl From<Box<Path>> for Box<OsStr> {}

Alternatives

While the proposed API would fix it for smart pointers in stdlib, it won't work for custom smart pointers.

And adding such APIs to every string type is annoying and it doesn't scale, makes it harder to write custom smart pointers.

Perhaps rust should consider introduce a trait for smart pointers to make such conversion trivial:

trait SafeCastable<T>
where
    T: ?Sized,
    layout(T) == layout(Self),
{
   fn cast(self) -> T;
}

impl<T, U> SafeCastable<U> for T
where
    T: AsRef<U> + ?Sized,
    U: ?Sized,
    layout(T) == layout(U),
{...}

impl<T, U> SafeCastable<U> for T
where
    T: Into<U> + ?Sized,
    U: ?Sized,
    layout(T) == layout(U),
{...}

trait SafeTryCastable<T>
where
    T: ?Sized,
    layout(T) == layout(U),
{
   type R: Try<T>;
   fn cast(self) -> R;
}

impl<T, U> SafeTryCastable<U> for T
where
    T: TryInto<U> + ?Sized,
    U: ?Sized,
    layout(T) == layout(U),
{...}

trait CastableSmartPtr {
    type Item: ?Sized;
    type SmartPtr<T: ?Sized>;

    unsafe fn cast_unchecked<T>(self) -> Self::SmartPtr<T>
    where
        T: ?Sized,
        layout(T) == layout(Self::Item); // either need const generics or compiler-generated auto-imlemented traits

    // provided methods
    fn cast<T>(self) -> Self::SmartPtr<T>
    where
        T: ?Sized,
        Self::Item: SafeCastable<T>,
    {...}

    fn try_cast<T>(self) -> Try<Self::SmartPtr<T>, <Self::Item as SafeTryCastable<T>>::R::Residual>
    where
        T: ?Sized,
        Self::Item: SafeTryCastable<T>,
    {...}
}

Related: rust-lang/rust#44874

[joshtriplett]

What's the reason to make these all dedicated methods, rather than implementing TryFrom for the fallible conversions and From for the infallible ones?

[NobodyXu]

i tried it and figured out that rust's generic system is a bit too limited for this

To implement TryFrom/From for any smart pointer, you'd need a for<T> SmartPtr<T>

[kennytm]

I think josh is talking about

impl TryFrom<Arc<OsStr>> for Arc<str> {}
impl TryFrom<Rc<OsStr>> for Rc<str> {}
impl TryFrom<Box<OsStr>> for Box<str> {}
impl From<Arc<str>> for Arc<OsStr> {}
impl From<Rc<str>> for Rc<OsStr> {}
impl From<Box<str>> for Box<OsStr> {}

not the higher-kinded generic alternative.

[NobodyXu]

Thanks @kennytm !

cc @joshtriplett I've added the TryFrom/From API you requested.

[joshtriplett]

Looks good to me. Seconded, assuming no objections from other team members.

[joshtriplett]

We discussed this in today's meeting. We're approving this, but since it's insta-stable, the PR will need to be FCPed.

[joshtriplett]

Also: There's already an impl From<String> for OsString, but there's no impl TryFrom<OsString> for String (though there's a to_str method). There was some interest in the meeting in seeing that added as well.

[NobodyXu]

@joshtriplett I'm working on it , however the orphan rules prevents me from doing it since Arc/Rc/Box is defined in alloc, not in std, and str is in core.

Is there anyway I can workaround that?

Or maybe we shall have a different API?

[workingjubilee]

I think this should be reopened because the problem remains and libs-api has glossed over the entire "the Orphan Rule is a thing" in closing this.

[NobodyXu]

Agreed, it's not clear how to implement it given the orphan rule constraint.

TBF I kind of think that core/alloc/std should be considered as one crate, from outside of the stdlib it really works like one crate.

core and alloc exists merely because rust needs to support no-std, core and alloc is effectively a subset of stdlib in terms of functionality.

[kennytm]

I think the Box impls are not prevented by orphan rules since Box is #[fundamental]?

Rc and Arc do have problems though.

[joshtriplett]

It's not immediately obvious to me why Arc and Rc aren't fundamental.

That aside, I'm surprised if this can't be defined in any of the three crates, since it seems like it should be definable in a crate introducing one of the types...

[NobodyXu]

That aside, I'm surprised if this can't be defined in any of the three crates

From/TryFrom is in core, Arc and Rc is in alloc and not fundamental.

But yeah I can already implement conversion for Box.

[NobodyXu]

cc @joshtriplett shall I open a PR to set Arc and Rc as fundamental?

[cuviper]

#[fundamental] changes what coherence considers covered, which can break downstream code, e.g.:

pub struct Foo(pub [u8]);

impl<T> AsRef<Foo> for std::sync::Arc<T> {
    fn as_ref(&self) -> &Foo {
        todo!()
    }
}

impl<T> AsRef<Foo> for std::rc::Rc<T> {
    fn as_ref(&self) -> &Foo {
        todo!()
    }
}

impl<T> AsRef<Foo> for Box<T> {
    fn as_ref(&self) -> &Foo {
        todo!()
    }
}

playground

error[E0210]: type parameter `T` must be covered by another type when it appears before the first local type (`Foo`)
  --> src/lib.rs:15:6
   |
15 | impl<T> AsRef<Foo> for Box<T> {
   |      ^ type parameter `T` must be covered by another type when it appears before the first local type (`Foo`)
   |
   = note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local, and no uncovered type parameters appear before that first local type
   = note: in this case, 'before' refers to the following order: `impl<..> ForeignTrait<T1, ..., Tn> for T0`, where `T0` is the first and `Tn` is the last

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

So that Box impl isn't allowed, but Rc and Arc currently are -- and those would break too if made fundamental.

It's a silly example, and I have no idea if real world code currently relies on this coherence coverage, but we could use crater to find out. I think this would also need T-lang signoff, not just libs.

[cuviper]

Actually, adding #[fundamental] doesn't even get past alloc itself:

error[E0119]: conflicting implementations of trait `TryFrom<Rc<[_], _>>` for type `Rc<[_; _], _>`
    --> alloc/src/rc.rs:2745:1
     |
2745 | impl<T, A: Allocator, const N: usize> TryFrom<Rc<[T], A>> for Rc<[T; N], A> {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: conflicting implementation in crate `core`:
             - impl<T, U> TryFrom<U> for T
               where U: Into<T>;
     = note: downstream crates may implement trait `core::convert::From<rc::Rc<[_], _>>` for type `rc::Rc<[_; _], _>`

error[E0119]: conflicting implementations of trait `TryFrom<Arc<[_], _>>` for type `Arc<[_; _], _>`
    --> alloc/src/sync.rs:3703:1
     |
3703 | impl<T, A: Allocator, const N: usize> TryFrom<Arc<[T], A>> for Arc<[T; N], A> {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: conflicting implementation in crate `core`:
             - impl<T, U> TryFrom<U> for T
               where U: Into<T>;
     = note: downstream crates may implement trait `core::convert::From<sync::Arc<[_], _>>` for type `sync::Arc<[_; _], _>`

... although I don't see how those downstream impls would be allowed at all. :/

edit: oh, that's probably for downstream A.

[kennytm]

Box workedaround it by implementing the TryFrom for A = Global only 😅

I tried adding a negative-impl impl !From<Rc<[T], A>> for Rc<[T; N], A> but all it does is getting rid of the 2nd note ("downstream crates may implement trait ..."), the error E0119 remains. Maybe it is something the current trait solver can't support.

[NobodyXu]

@cuviper is there anyway of implementing Arc<OsStr> for Arc<str> in alloc?

[cuviper]

Not in alloc, because that has no access to the OsStr type. It might be hackable in std using #[rustc_allow_incoherent_impl], but "the only currently supported targets are inherent methods" -- not trait impls.

[cuviper]

Or, we could (secretly) promote those types to core::ffi / alloc::ffi, and the coherence problems will disappear.

[NobodyXu]

Thanks @cuviper , I could open a PR to promote those types to core/alloc.

It doesn't look like they use any syscall, only platform-dependent parsing, so it should be possible.

[NobodyXu]

@cuviper Just realize that Path, actually does use syscalls.

So I can move OsStr to core and OsString to alloc, but I can't move everything in Path to core - there are some methods exists, try_exists, is_dir, is_file, is_symlink, metadata, symlink_metadata, read_dir and read_link, which cannot be in core.

One way to fix it would be to put them in a sealed traits implemented in std, but the trait has to be imported for it to work, otherwise it'd break backwards compatibility.

I suppose that importing the trait in prelude could maintain that backwards compatibility, but I'm not sure if it possible to opt-out of importing prelude.

If it is possible, then this won't work.

[pitaj]

You can use rustc_allow_incoherent_impl to add those inherent functions in std.

[NobodyXu]

Thanks, @pitaj that's exactly the magic token I need for.

[NobodyXu]

Is there anyway to mark a type/API as permanently unstable?

I need to move the Wtf8, os_str::Slice and etc into core, since the sys abstraction layer still uses it, I have to mark it as public.

However since it won't be stablised, I'd need to mark it as permanent unstable for now (if someone wants it stablised, they can open a separate issue here in this repo).

[pitaj]

Generally you'd give it a feature name with "internal" in there somewhere and not provide an issue number. Like this:

https://github.com/rust-lang/rust/blob/334e509912d82323ff0fca016a2e69dbbee559fd/library/core/src/panicking.rs#L25

[NobodyXu]

Thank you!

[cuviper]

You can make them #[doc(hidden)] too.

FWIW, I would move as little as you can get away with, keeping most of the impls in std if possible. But I guess trait impls will have to move for the same reason you're trying this in the first place.

[NobodyXu]

Well Wtf8 is actually the internal details of OsStr, and they are used in the sys module of crate std, so I have to move them and make them public.

And yeah the trait impls should be moved as well, oitherwise leaving it half done doesn't make sense..

[workingjubilee]

Yes, the Wtf8 code has to move to core.