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

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

Closed
sunshowers opened this issue Jul 12, 2023 · 4 comments
Closed

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

sunshowers opened this issue Jul 12, 2023 · 4 comments
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@sunshowers
Copy link

sunshowers commented Jul 12, 2023

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):

  • 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.
@sunshowers sunshowers added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Jul 12, 2023
@pitaj
Copy link

pitaj commented Jul 12, 2023

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

@ChrisDenton
Copy link
Member

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
Copy link
Author

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:

  • If a string is valid UTF-8, it must be stored as such in OsString (implied by OsStr::to_str).
  • On Unix, OsString is a Vec<u8>.
  • On Windows, OsString is a Vec<u8> + an is_known_utf8 field. In the proposed OsString::to_mut_string, you could simply check that is_known_utf8 is true in a fast path. It doesn't have to be reset either.

@Amanieu
Copy link
Member

Amanieu commented Jul 25, 2023

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.

@Amanieu Amanieu closed this as completed Jul 25, 2023
@dtolnay dtolnay closed this as not planned Won't fix, can't repro, duplicate, stale Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

5 participants