rust-lang / libs-team

The home of the library team
Apache License 2.0
116 stars 18 forks source link

Proposal: Add OsStr::to_mut_str and OsString::to_mut_string #247

Closed sunshowers closed 1 year ago

sunshowers commented 1 year ago

Proposal

Problem statement

In Rust 1.70, new methods PathBuf::as_mut_os_string and Path::as_mut_os_str were added. I'd like to add support for analogues to these methods in camino: Utf8PathBuf::as_mut_string and Utf8Path::as_mut_str.

Motivating examples or use cases

Making camino have API parity with std::path::Path -- the arguments in #140 also apply to camino. Camino's paths are wrappers around PathBuf and Path, which means that there's currently no way to obtain a &mut String for callers to work with.

(Note it is possible to cast a &mut OsStr to &mut str, but I suspect that's UB.)

I'm guessing there are other use cases too that would benefit from the solution below.

Solution sketch

Add methods to OsString that return &mut String if the contents are valid UTF-8. The names are similar to Path::to_str.

struct OsString {
    fn to_mut_string(&mut self) -> Option<&mut String> { ... }
    fn to_string(&self) -> Option<&String> { ... } // I don't know how useful this is?
}

struct OsStr {
    fn to_mut_str(&mut self) -> Option<&mut str> { ... }
    // OsStr::to_str already exists
}

These APIs are valid because every UTF-8 string is a valid OS string, and &mut str/&mut String can only contain non-UTF-8 data temporarily before it becomes UB.

Alternatives

Not aware of any ways to achieve OsString::to_mut_string currently. OsStr::to_mut_str can be achieved via an unsafe cast (and works in every version of Rust released so far) but I suspect it is UB.

The other alternative is for camino to implement its own path handling top-to-bottom rather than being a wrapper around Path/PathBuf, but that seems excessive and error-prone compared to adding this API.

Links and related work

140 - ACP to add as_mut_os_string to &mut PathBuf and as_mut_os_str to &mut Path.

What happens now?

This issue is part of the libs-api team API change proposal process. 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):

Second, if there's a concrete solution:

pitaj commented 1 year ago

It isn't possible to convert &mut OsString to &mut String because the two don't have identical layout.

ChrisDenton commented 1 year ago

We could make it so they have compatible layouts (e.g. all start with a Vec<u8>). We would need tests to ensure their layouts maintain compatibility though.

sunshowers commented 1 year ago

It isn't possible to convert &mut OsString to &mut String because the two don't have identical layout.

I agree that it isn't possible to cast a &mut OsString to a &mut String, but I think it is certainly possible to have a method as mentioned above:

Amanieu commented 1 year ago

We discussed this in the libs-api meeting today. We had a look at camino, and it seems that the need for these methods could be avoided by changing Utf8Path to wrap a str, and Utf8PathBuf to wrap a String. There already exist free conversions from str to Path and String to PathBuf since OsStr is guaranteed to be a superset of UTF-8.

However, please do reopen if you've tried this and you encounter a significant blocker which still makes these methods necessary.