-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Implement fs_native_path
#108981
base: master
Are you sure you want to change the base?
Implement fs_native_path
#108981
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
The Miri subtree was changed cc @rust-lang/miri |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, didn't read the description properly
Yes, I mentioned as much. However, this is more than a slight refactoring in some cases. Like you said, |
I should do a crater run. There's at least a possibility that some weird inference thing may break this, which would be a blocker. |
This comment was marked as outdated.
This comment was marked as outdated.
Oops, right @bors try |
⌛ Trying commit 1b5d6cd355deacecbb236cbc553cfc988ec1ed62 with merge 1648938e809d7a7838c36282edd641ee0a4f137f... |
☀️ Try build successful - checks-actions |
@craterbot check |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
5 regressions and 3 fixes but none of them look relevant. So it seems like the public trait in this PR wouldn't break anything inference wise. |
Unless I'm mistaken , this looks a purely T-libs patch so I'll remove the T-compiler @rustbot label -T-compiler |
Oops yes, sorry. I think that was a left over auto-label. |
library/std/src/sys/unix/fs.rs
Outdated
use crate::sys::path::PathLike; | ||
|
||
pub(crate) fn remove_file<P: PathLike>(path: P) -> io::Result<()> { | ||
path.with_path(fs::unlink) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is the only remaining work after this PR to swap these to with_native_path
? That's exciting!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's the idea! It may not be trivial in all cases though so I've left that for follow up PRs.
library/std/src/sys/windows/path.rs
Outdated
#[unstable(feature = "fs_native_path_internals", issue = "none")] | ||
#[repr(transparent)] | ||
#[derive(Debug)] | ||
pub struct WindowsPath(pub [u16]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this called WindowsPath
instead of just NativePath
(which is used for all other targets)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking back at the history here it seems I was originally thinking of making an equivalent of CStr
(called WStr
at the time). As the idea here evolved and narrowed in scope this eventually became WindowsPath
,
But yes, I should just call this NativePath
too. If there's a need for a more general type then that can and should be a separate PR.
library/std/src/path.rs
Outdated
/// native paths more directly to system APIs. | ||
#[unstable(feature = "fs_native_path", issue = "108979")] | ||
pub trait AsPath: pathlike::Sealed { | ||
// NOTE: This trait is purely for external documentation purposes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems quite complex since it requires duplicating the PathLike
trait for every OS despite it having the same signature everywhere. Could we do something like this instead:
- Move the
PathLike
methods into this trait, but make them doc-hidden and unstable. - Remove
PathLike
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. This was written awhile ago and underwent a number of revisions both before and during the ACP so I'm still trying to page in what my intentions were. I agree that would be much simpler.
library/std/src/sys/solid/fs.rs
Outdated
@@ -325,14 +378,17 @@ fn cstr(path: &Path) -> io::Result<CString> { | |||
|
|||
impl File { | |||
pub fn open(path: &Path, opts: &OpenOptions) -> io::Result<File> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be some inconsistency between the different targets in offering different combinations of open
, open_c
and open_native
. Could this all be cleaned up to have a single open
method that takes a NativePath
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my intention was to avoid touching as much code as possible and some parts are using Path
quite a bit so it's a bit more involved to change them to use NativePath
or adding in the conversions. But I can take another look.
@Amanieu Given the recent restructuring and other changes in std, rebasing simplified things a lot. In addition I applied your suggest to simplify the trait. |
This comment has been minimized.
This comment has been minimized.
cfc57ac
to
308e304
Compare
This comment has been minimized.
This comment has been minimized.
#[unstable(feature = "fs_native_path", issue = "108979")] | ||
impl NativePathExt for NativePath { | ||
fn from_wide(wide: &[u16]) -> &NativePath { | ||
assert_eq!(crate::sys::unrolled_find_u16s(0, wide), Some(wide.len().saturating_sub(1))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a user-friendly panic message for when assert fails.
/// These types include [`Path`], [`OsStr`] and [`str`] as well as their owned | ||
/// counterparts [`PathBuf`], [`OsString`] and [`String`]. | ||
/// | ||
/// You can also implement you own [`AsRef<Path>`] for your own types and they'll |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// You can also implement you own [`AsRef<Path>`] for your own types and they'll | |
/// You can also implement [`AsRef<Path>`] for your own types and they'll |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to address this typo.
library/std/src/sys/pal/solid/fs.rs
Outdated
@@ -325,14 +377,17 @@ fn cstr(path: &Path) -> io::Result<CString> { | |||
|
|||
impl File { | |||
pub fn open(path: &Path, opts: &OpenOptions) -> io::Result<File> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still still inconsistent: why is open
kept in some targets but removed in other? I would expect everything to use open_native
only and for open
to be removed (or possible just change the signature of open
to take native path).
@@ -294,8 +349,12 @@ impl OpenOptions { | |||
} | |||
|
|||
impl File { | |||
pub fn open(path: &Path, opts: &OpenOptions) -> io::Result<File> { | |||
pub(super) fn open(path: &Path, opts: &OpenOptions) -> io::Result<File> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
And replace `AsRef<Path>` with the `AsPath` trait.
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #122256) made this pull request unmergeable. Please resolve the merge conflicts. |
p | ||
); | ||
return Err(io::Error::new(io::ErrorKind::Uncategorized, msg)); | ||
fn open_parent(p: &CStr) -> io::Result<(ManuallyDrop<WasiFd>, PathBuf)> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should return a CString
to avoid another conversion in open_at
.
@@ -563,7 +563,9 @@ pub fn symlink<P: AsRef<Path>, U: AsRef<Path>>( | |||
/// This is a convenience API similar to `std::os::unix::fs::symlink` and | |||
/// `std::os::windows::fs::symlink_file` and `std::os::windows::fs::symlink_dir`. | |||
pub fn symlink_path<P: AsRef<Path>, U: AsRef<Path>>(old_path: P, new_path: U) -> io::Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these methods be updated to use AsPath
instead?
/// These types include [`Path`], [`OsStr`] and [`str`] as well as their owned | ||
/// counterparts [`PathBuf`], [`OsString`] and [`String`]. | ||
/// | ||
/// You can also implement you own [`AsRef<Path>`] for your own types and they'll |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to address this typo.
let (dir, file) = open_parent(p)?; | ||
dir.unlink_file(osstr2str(file.as_ref())?) | ||
} | ||
|
||
pub fn rename(old: &Path, new: &Path) -> io::Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is only one changed to CStr
but not both? In fact shouldn't all &Path
in this file be changed?
@@ -1132,16 +1186,16 @@ pub fn rmdir(p: &Path) -> io::Result<()> { | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't all the methods above (rename
, unlink
, rmdir
, etc) also be switched to NativePath
?
@rustbot author |
Implements the
fs_native_path
feature. See also rust-lang/libs-team#62.Internal changes
Ultimately, I'd envision that Unix filesystem functions (for example) would simply take either an
&CStr
or maybe an internal&PosixPath
type with relevant helper methods. However, as much as I'd love to transition immediately, I think it's more prudent to take it slower. If nothing else it'll be easier on reviewers. So this PR just moves the responsibility for creating concrete types intosys::fs
while touching as little existing code as possible, which should then allow platforms to make changes at their own pace.