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

ACP: PathLike trait #62

Closed
ChrisDenton opened this issue Jul 4, 2022 · 11 comments
Closed

ACP: PathLike trait #62

ChrisDenton opened this issue Jul 4, 2022 · 11 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

@ChrisDenton
Copy link
Member

ChrisDenton commented Jul 4, 2022

Proposal

Problem statement

Currently all filesystem operations that act on paths have to go through Path. This then requires a conversion from Path to the platform path type before use. This conversion is exasperated when the user already has a native path type and, when calling stdlib filesystem functions, has to convert it to Path only for it to be immediately converted back to the native form.

Another issue Windows has is that Path (and OsStr) are opaque. So if the user wants to do anything not provided by stdlib functions then they have to convert to another type and then back. Either a str (which incurs a UTF-8 check and is potentially lossy) or by using encode_wide (which is more expensive and may require an allocation).

Motivation, use-cases

For Windows in particular something like this conversion needs to be done a lot:

use std::ffi::OsString;
use std::path::PathBuf;
use std::os::windows::ffi::OsStrExt;

// Where `path` is an `&Path`
// Make sure there are not any nulls in the path.
ensure_no_nulls(path)?;
// This allocates a `Vec` and the `encode_wide` method does the heavy lifting to convert from WTF-8 to WTF-16.
let native_path: Vec<u16> = path.as_os_str().encode_wide().chain([0]).collect();

There are ways to mitigate the cost (e.g. using the stack for shorter paths instead of allocating) but it still must always be done one way or another.

If you already have a native path (e.g. from a -sys crate) then this essentially means round tripping through Path to use the standard library. It's roughly the equivalent of doing this, which ends up as a particularly expensive no-op:

use std::ffi::OsString;
use std::os::windows::ffi::{OsStringExt, OsStrExt};
use std::path::PathBuf;

// Assume `native_path` is a `Vec<u16>` that was filled from a manual call to the Windows API.
let path: PathBuf = OsString::from_wide(&native_path).into();
ensure_no_nulls(&path)?;
let native_path: Vec<u16> = path.as_os_str().encode_wide().chain([0]).collect();
// use `native_path` in a Windows API call

Solution sketches

I propose allowing users to provide their own path types. To facilitate this we would add a PathLike trait that can be used by functions to take anything that can be converted to a native path, without mandating how that happens. There would be a blanket implementation for AsRef<Path> so that his doesn't break (n.b. would this need a crater run to confirm?).

// Replace `AsRef<Path>` with the `PathLike` trait.
pub fn remove_file<P: PathLike>(path: P) -> io::Result<()>;

PathLike would initially be an unstable trait but its name would instantly appear in stable method signatures. Therefore any bikeshedding of the trait name would need to happen before it's merged and the trait itself would need to be documented with that in mind (the pattern trait is in a similar position).

I do not think the exact definition of PathLike needs to be finalized in this ACP (feel free to disagree!) but my aim would be to work towards something like this:

pub trait PathLike {
    /// Borrow a std `Path`
    fn with_path<T, F: FnOnce(&Path) -> T>(&self, f: F) -> T;

    /// Borrow a native path.
    ///
    /// Here `NativePath` could be a platform specific alias (e.g. `CStr` for POSIX platforms)
    /// or even its own FFI type.
    ///
    /// It returns the path in an `FnOnce` so as to allow using the stack or any other way
    /// of getting a `NativePath`
    fn with_native_path<T, F: FnOnce(&NativePath) -> T>(&self, f: F) -> T;
}

I don't think we need to immediately decide what form NativePath should take for each platform. This could, for example, be a type alias in core::ffi (which already has OS specific type aliases). Or a more complex solution would be to have an opaque struct with extension methods for each platform (perhaps better for documentation but a lot more indirection).

Alternatives

I considered having a ToPlatformStr (or similar) as a replacement for both AsRef<OsStr> and AsRef<Path>. However, we do currently make a distinction between a Path and an OsStr (even though they're really the same thing) so I decided to stick with that for now as I do think it will be useful for custom types. In the future we may also want to make finer distinction for other OsStr types. For example, environment variable keys or process::Command application names. But I think I can avoid those questions for now.

I also considered Into<NativePath> but I think using a dedicated trait is more versatile and also provides a better place to hang documentation on.

Links and related work

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.

@ChrisDenton ChrisDenton added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Jul 4, 2022
@m-ou-se m-ou-se added the I-libs-api-nominated Indicates that an issue has been nominated for discussion during a team meeting. label Jan 31, 2023
@ChrisDenton
Copy link
Member Author

ChrisDenton commented Mar 2, 2023

This was discussed at a recent libs-api meeting. There was general interest but it was felt like we should have the minimal surface area necessary for experimentation outside of std.

To that end I'm thinking we just need a NativePath type that simply wraps an inner platform specific type. Then std::os::ffi extension traits provide OS specific ways to create or unwrap a NativePath but do not provide any other methods. It will not implement general traits like From or AsRef as these may be confusing if they're implemented differently on different platforms (or at least they're more difficult to document). A NativePathBuf could also be used for functions that need to return a path.

This still requires a trait to replace AsRef<Path> in filesystem functions but this is more of an implementation detail. I've renamed the public trait AsPath because it's kind of instantly stable as it appears in documentation so I wanted to keep the name close to AsRef<Path>.

In summary, this is what I'm proposing:

// std::path::NativePath
// An opaque type. Use methods in std::os::ffi::$PLATFORM::NativePathExt to create or unwrap a NativePath.
pub struct NativePath(_);

// std::os::ffi::windows::NativePathExt
// On Windows NativePath wraps a [u16],
pub trait NativePathExt: Sealed {
    fn from_wide(wide: &[u16]) -> &NativePath;
    fn as_wide(&self) -> &[u16];
}

// std::os::ffi::unix::NativePathExt
// On Unix NativePath wraps a CStr,
pub trait NativePathExt: Sealed {
    fn from_cstr(cstr: &CStr) -> &NativePath;
    fn as_cstr(&self) -> &CStr;
}

// std::path::AsPath
// Sealed trait that implements no methods itself.
pub trait AsPath: Sealed {}
impl<P: AsRef<Path>> AsPath for P {}
impl AsPath for &NativePath {}

@the8472
Copy link
Member

the8472 commented Mar 2, 2023

I've renamed the public trait AsPath because it's kind of instantly stable as it appears in documentation

Public but unstable and unimplementable isn't so bad, users won't be able to use it in bounds so we can still rename it later.

@ChrisDenton
Copy link
Member Author

For sure, I just meant if a name appears publicly in stable fs function signatures then it's best to have a name that doesn't invite more questions than necessary.

@programmerjake
Copy link
Member

// std::path::AsPath
// Sealed trait that implements no methods itself.
pub trait AsPath: Sealed {}
impl<P: AsRef<Path>> AsPath for P {}
impl AsPath for &NativePath {}

shouldn't that be P: AsRef<Path> + ?Sized? that way str impls AsPath.

@ChrisDenton
Copy link
Member Author

Hm, is that not already implied by AsRef<Path>? Do you have an example where something breaks without the extra ?Sized? Note that the generic fs methods, such as metadata, do not currently use ?Sized.

@programmerjake
Copy link
Member

programmerjake commented Mar 3, 2023

Hm, is that not already implied by AsRef<Path>? Do you have an example where something breaks without the extra ?Sized?

no and no, but imho consistency with AsRef<Path> is a good idea.

Note that the generic fs methods, such as metadata, do not currently use ?Sized.

ok, then i guess it's just opinion-based rather than a breaking change

@SUPERCILEX
Copy link

SUPERCILEX commented Mar 27, 2023

Haven't looked at this in detail, but it might be interesting to see if we can provide something similar to https://docs.rs/rustix/latest/rustix/path/trait.Arg.html or https://docs.rs/nix/latest/nix/trait.NixPath.html. Currently, std always has to either copy the path on the stack or allocate. That's bummed me out when I know I can produce a CStr that can be passed directly to the OS.

@ChrisDenton
Copy link
Member Author

Yes, that's the idea. Originally this was done through a trait the users could implement on their own type. This has now been simplified to a NativePath type that wraps a platform-specific type (which indeed is CStr for *nix platforms).

@SUPERCILEX
Copy link

Awesome, that would be cool!

@m-ou-se m-ou-se removed the I-libs-api-nominated Indicates that an issue has been nominated for discussion during a team meeting. label Mar 28, 2023
@m-ou-se
Copy link
Member

m-ou-se commented May 17, 2023

Closing this as this was already discussed in an earlier libs-api meeting. Looking forward to the next steps.

Edit: to clarify, what was proposed in this comment seems fine. ^^

@m-ou-se m-ou-se closed this as completed May 17, 2023
@ChrisDenton
Copy link
Member Author

PR is here: rust-lang/rust#108981

Sorry, it seems I forgot to link back to the ACP.

@dtolnay dtolnay added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label Nov 23, 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

6 participants