Implement `fs_native_path` by ChrisDenton · Pull Request #108981 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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 into sys::fs while touching as little existing code as possible, which should then allow platforms to make changes at their own pace.

r? @joshtriplett

(rustbot has picked a reviewer for you, use r? to override)

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.

labels

Mar 10, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

The Miri subtree was changed

cc @rust-lang/miri

Member

@joboet joboet left a comment • Loading

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

At the moment, Path is still used in sys for some things like api and File::open on the smaller platforms. IMHO sys should be slightly refactored so that it always takes NativePath

Yes, I mentioned as much. However, this is more than a slight refactoring in some cases. Like you said, Path is still used in places (as well as PathBuf). I would prefer to delegate as much platform-specific changes to other PRs as possible so that they can get proper attention instead of having one giant PR that changes everything all at once.

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.

⌛ Trying commit 1b5d6cd355deacecbb236cbc553cfc988ec1ed62 with merge 1648938e809d7a7838c36282edd641ee0a4f137f...

☀️ Try build successful - checks-actions
Build commit: 1648938e809d7a7838c36282edd641ee0a4f137f (1648938e809d7a7838c36282edd641ee0a4f137f)

👌 Experiment pr-108981 created and queued.
🤖 Automatically detected try build 1648938e809d7a7838c36282edd641ee0a4f137f
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

🚧 Experiment pr-108981 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

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

rustbot removed the T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

label

Apr 25, 2023

Oops yes, sorry. I think that was a left over auto-label.

use crate::sys::path::PathLike;

pub(crate) fn remove_file<P: PathLike>(path: P) -> io::Result<()> {
path.with_path(fs::unlink)

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!

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.

@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.

#[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)));

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`] for your own types and they'll

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`] for your own types and they'll
/// You can also implement [`AsRef`] for your own types and they'll

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.

@@ -325,14 +377,17 @@ fn cstr(path: &Path) -> io::Result {

impl File {
pub fn open(path: &Path, opts: &OpenOptions) -> io::Result {

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 {
pub(super) fn open(path: &Path, opts: &OpenOptions) -> io::Result {

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.

rustbot added the O-wasi

Operating system: Wasi, Webassembly System Interface

label

Mar 8, 2024

p
);
return Err(io::Error::new(io::ErrorKind::Uncategorized, msg));
fn open_parent(p: &CStr) -> io::Result<(ManuallyDrop, PathBuf)> {

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, U: AsRef>(
/// 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, U: AsRef>(old_path: P, new_path: U) -> io::Result<()> {

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`] for your own types and they'll

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<()> {

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<()> {
}

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 added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Sep 5, 2024

alex-semenyuk added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Oct 30, 2024

@ChrisDenton any updates on this? thanks

Sorry, I've been quite busy lately and this PR is very prone to breakage. I hope to get back to it sometime this month but there are a higher priority things on my todo list that I'd like to get done first.