ACP: `impl TryFrom<{OsString, &OsStr}> for String`

[Kixunil]

Proposal

Problem statement

When writing a function which is supposed to check the contents of a string and then conditionally store String and where it is expected that the string might have come from OS, the best type for the argument is impl AsRef<OsStr> + TryInto<String> because:

• &str and String don't support OS strings
• &OsStr can't take advantage of the existing allocation if the caller happens to have OsString and doesn't need it anymore
• OsString forces a caller which only has &OsStr to allocate even when the callee wouldn't need the owned string

Motivating examples or use cases

I'm writing a crate that has a type supposed to store either a systemd socket name or Unix socket address:

enum SocketAddr {
    Path(std::os::unix::net::SocketAddr),
    Systemd(SocketName /* just wrapped String */),
}

The format is defined to start with systemd:// for systemd sockets, so I need to first check which kind it is first and then attempt to parse that kind. unix::net::SocketAddr doesn't need an allocated string but SocketName does.

Solution sketch

Just write the obvious impl that checks the string first and converts if it is UTF-8.

Alternatives

Force everyone to newtype the type or write their own traits. I think this is way too annoying for something that looks like it should've been implemented a long time ago.

Links and related work

Notably, there already are similar conversions for other types.

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

• We think this problem seems worth solving, and the standard library might be the right place to solve it.
• We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

• We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
• We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.

[kennytm]

what are the Error types of these TryFrom impl? i suppose they are these?

impl TryFrom<OsString> for String {
    type Error = std::string::FromUtf8Error;
}
impl TryFrom<&OsStr> for String {
    type Error = std::str::Utf8Error;
}

[Kixunil]

Those definitely can be used on Unix but I''m not sure if they can on Windows. If someone figuring it out is a requirement for the ACP to be approved I'll do it. I didn't so far because that seems like a minor detail and I don't want to spend time on something I'm not sure will be of any use.

[programmerjake]

We should probably have an error type that gives you back an OsString when you fail to convert OsString to String.

a valid implementation that works everywhere is:

impl<'a> TryFrom<&'a OsStr> for String {
    type Error = std::str::Utf8Error;
    fn try_from(v: &'a OsStr) -> Result<Self, Self::Error> {
        Ok(str::try_from(v)?.into())
    }
}

// in std::ffi -- based off std::ffi::IntoStringError for CString
#[derive(Clone, PartialEq, Eq, Debug)]
pub struct OsIntoStringError {
    inner: OsString,
    error: Utf8Error,
}

impl OsIntoStringError {
    /// Consumes this error, returning original [`OsString`] which generated the
    /// error.
    #[must_use = "`self` will be dropped if the result is not used"]
    pub fn into_os_string(self) -> OsString {
        self.inner
    }

    /// Access the underlying UTF-8 error that was the cause of this error.
    #[must_use]
    pub fn utf8_error(&self) -> Utf8Error {
        self.error
    }
}

impl std::fmt::Display for OsIntoStringError { ... }
impl std::error::Error for OsIntoStringError { ... }

impl TryFrom<OsString> for String {
    type Error = OsIntoStringError;
    fn try_from(v: OsString) -> Result<Self, Self::Error> {
        String::from_utf8(v.into_encoded_bytes()).map_err(|e| OsIntoStringError {
            error: e.utf8_error(),
            // Safety: got from into_encoded_bytes
            inner: unsafe { OsString::from_encoded_bytes_unchecked(e.into_bytes()) },
        })
    }
}

[kennytm]

@Kixunil

Those definitely can be used on Unix but I''m not sure if they can on Windows.

We already have the non-allocating impl TryFrom<&OsStr> for &str (rust-lang/rust#99031), available on all platforms, and stabilized since 1.72, so the &OsStr → String conversion can definitely reuse Utf8Error.

[Kixunil]

@programmerjake oh, yes, you're right. That looks great.
@kennytm oh, great point, thanks!

[joshtriplett]

We reviewed this in today's @rust-lang/libs-api meeting, and we approve of adding these trait impls.

We would love to see this done by making FromUtf8Error generic; then, the into_os_string method on that error can only exist on FromUtf8Error<OsString> version.

However, adding that will need a crater run to make sure it doesn't break any type inference. If that crater run turns up any failures, then we could have two separate types instead.

[Kixunil]

There is an issue with making it generic where FromUtf8Error is defined in alloc but OsString is in std, so std needs a way to construct FromUtf8Error. Should I just add an unstable public constructor?

There is also into_utf8_lossy method which presumably should be available for OsString which would require a trait implemented by both Vec<u8> and OsString.

Oh, and the into_os_string method may only be implemented in the alloc crate. A possible solution is to name it into_input or something along those lines and just return the inner value.

[programmerjake]

into_os_string can use #[rustc_allow_incoherent_impl] to allow you ro implement it in std despite Self being defined in alloc.

alternatively we could decide to move OsStr/OsString to core/alloc since like c_long they depend on the target but don't actually call anything from the OS (except the memory allocator).

[Kixunil]

I'm getting

#[rustc_allow_incoherent_impl] attribute cannot be used on inherent impl blocks

[programmerjake]

I'm getting

#[rustc_allow_incoherent_impl] attribute cannot be used on inherent impl blocks

it doesn't go on the impl Ty { ... } directly, instead you write it on the functions inside the impl block. e.g. https://doc.rust-lang.org/1.93.0/src/alloc/ffi/c_str.rs.html#1185
with the extraneous stuff removed:

impl CStr {
    #[rustc_allow_incoherent_impl]
    pub fn to_string_lossy(&self) -> Cow<'_, str> {
        String::from_utf8_lossy(self.to_bytes())
    }
}