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

Add as_mut_os_string to &mut PathBuf and as_mut_os_str to &mut Path #140

Closed
zertosh opened this issue Nov 28, 2022 · 2 comments
Closed

Add as_mut_os_string to &mut PathBuf and as_mut_os_str to &mut Path #140

zertosh opened this issue Nov 28, 2022 · 2 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@zertosh
Copy link

zertosh commented Nov 28, 2022

Proposal

Problem statement

Currently there's:

  1. No way to get a &mut OsString (or &mut OsStr) from &mut PathBuf (only from PathBuf via into_os_string), and;
  2. No way to get a &mut OsStr from &mut Path.

This puts &mut OsString's push, and &mut OsStr's make_ascii_lowercase / make_ascii_uppercase, out of reach for &mut PathBuf and &mut Path.

Motivation, use-cases

The proposed API change makes it possible for &mut PathBuf / &mut OsStr to take advantage of the APIs available on &mut OsString and &mut OsStr:

  • &mut OsString's push.
    • This is the most useful API this proposal enables. PathBuf's push extends self by adding path components, and can even clear the underlying path when pushing an absolute path. OsString always appends as-is.
    • This makes it possible, for example, to gradually build a filename on an existing &mut PathBuf without having to do an extra allocation in an OsString to then append/replace on the &mut PathBuf.
  • &mut OsString's clear, reserve, try_reserve, reserve_exact, try_reserve_exact, shrink_to_fit, shrink_to.
    • Equivalent methods already exist on &mut PathBuf that are passthroughs to the PathBuf's underlying OsString. So these would be of limited usefulness.
  • &mut OsStr's make_ascii_lowercase / make_ascii_uppercase.

There is already precedent for exposing a type's inner store via as_mut_* methods. String has a as_mut_vec and as_bytes_mut (via Deref<Target = str>). These are marked unsafe on String because with them it is possible for the caller to break the type's "valid UTF-8" invariant. On PathBuf, the proposed as_mut_os_string and as_mut_os_str need not be marked unsafe because PathBuf doesn't make any guarantees that can be broken by OsString.

Solution sketches

In library/std/src/path.rs:

impl PathBuf {
    // ...

    pub fn as_mut_os_string(&mut self) -> &mut OsString {
        &mut self.inner
    }
  
    // ...
}

impl Path {
    // ...

    pub fn as_mut_os_str(&mut self) -> &mut OsStr {
        &mut self.inner
    }

    // ...
}

Links and related work

  • Should the proposed as_mut_os_string or as_mut_os_str be marked unsafe?
    • Currently there's no guarantee offered by PathBuf that would be broken by exposing the underlying OsString. So making unsafe is unnecessary. However, this proposal would make it impossible to offer any guarantee in the future. Given the stability of the API though, the possibility of offering some guarantee that would be broken by exposing safe as_mut_os_string or as_mut_os_str seems remote enough to not warrant the ergonomics loss of marking them unsafe.

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 in its weekly meeting. You should receive feedback within a week or two.

@zertosh zertosh added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Nov 28, 2022
@dtolnay
Copy link
Member

dtolnay commented Nov 28, 2022

Seconded. Paths are arbitrary OS strings (per the existence of https://doc.rust-lang.org/std/path/struct.Path.html#method.new, among other things) so exposing mutation of one as an OS string should not be an issue.

Please send a libstd PR.

@dtolnay dtolnay added the initial-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Nov 28, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 29, 2022
Add `PathBuf::as_mut_os_string` and `Path::as_mut_os_str`

Implements rust-lang/libs-team#140 (tracking issue rust-lang#105021).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 29, 2022
Add `PathBuf::as_mut_os_string` and `Path::as_mut_os_str`

Implements rust-lang/libs-team#140 (tracking issue rust-lang#105021).
@dtolnay
Copy link
Member

dtolnay commented Dec 5, 2022

Unstable implementation PR: rust-lang/rust#105002
Tracking issue: rust-lang/rust#105021

@dtolnay dtolnay closed this as completed Dec 5, 2022
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Feb 10, 2023
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Feb 10, 2023
Add `PathBuf::as_mut_os_string` and `Path::as_mut_os_str`

Implements rust-lang/libs-team#140 (tracking issue #105021).
@dtolnay dtolnay added ACP-accepted API Change Proposal is accepted (seconded with no objections) and removed initial-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) 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

2 participants