-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
read_dir
has unexpected behavior for ""
#114149
Comments
The behavior of
On (my) Windows,
Though if anything, I find the Windows behavior to be the one that's odd of the two. |
Tbh, I would agree this looks like a bug in the Windows implementation of |
Paths prefixed by
I do not consider the behavior of
Do you mean a bug in the Unix implementation? |
No I meant the Windows implementation. Empty paths are always considered invalid. The fact that it isn't in this one implementation of this one function was not intentional. |
As this is a request to change the behaviour of |
If empty paths are always considered invalid, why does |
Sorry, I was imprecise. It's invalid at the point of use. OS APIs will reject this but due to a quirk in the Windows API we're using it happens to work in that case (the string we pass to the OS is never empty). |
This was discussed in a recent libs-api meeting. There was broad agreement that the failure of However, there was no consensus on the proposed solution to interpret an empty path as the current directory for There were also a few related things discussed:
|
I personally think that I agree that if we see
In all cases, the Perhaps it's just weird that In the team meeting, I was asked: "why not just use for parent in ["", "src"] {
for f in std::fs::read_dir(parent)? {
println!("{}", f?.path().display());
}
} (Output: I could of course just use The follow-up question was why not just canonicalize or normalize the path before using it, to get rid of the And last but not least, That said, I'm curious to hear more about potential downsides. Will accepting |
Btw, @BurntSushi should probably be specifically cc'd here, both as a libs-api member and as the maintainer of the popular walkdir crate. I think whatever is decided here should also be reflected in the wider ecosystem and it'd be nice to coordinate any changes if possible. |
I would also like to note, I first became aware of this issue because |
All other APIs that take a path do error on
I think that's more the semantics that I'd ascribe to something like
Well, except that this errors on unix. So it never worked portably, so there shouldn't be any software relying on this behavior.
This seems like a somewhat narrow case. Anything more complex and I'd already reach for Overall I think While |
For whatever it's worth, I went looking at other standard libraries and tools to see what they do and they mostly seem to treat Python's Powershell will error for I can also think of a few examples in other contexts where the empty path is treated as the current directory. I.e. Solaris treats an empty symlink the same as I've not fully surveyed every language so it's possible I'm missing some precedent. |
I do personally find it odd to treat the empty string as equivalent to the current working directory. The empty string isn't a valid path, so it makes sense to me that it would return an error. The interaction with If we did end up deciding to make |
We discussed this in the libs-api meeting today and the team was still somewhat split on the issue. With that in mind, and taking @BurntSushi's comments into account, we decided to not add support for treating As a consequence #116606 will make Windows match the current behavior on Unix. @rfcbot fcp close |
Team member @Amanieu has proposed to close this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The issue rust-lang/rust#114149 has an FCP with disposition "close" (and not "merge"). Update the text to account for this
The final comment period, with a disposition to close, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. |
On Windows make `read_dir` error on the empty path This makes Windows consistent with other platforms. Note that this should not be taken to imply any decision on rust-lang#114149 has been taken. However it was felt that while there is a lack of libs-api consensus, we should be consistent across platforms in the meantime. This is a change in behaviour for Windows so will also need an fcp before merging. r? libs-api
Fixed by #116606. |
The issue rust-lang/rust#114149 has an FCP with disposition "close" (and not "merge"). Update the text to account for this
Path::parent
returnsSome("")
for relative paths likefoo.txt
, butread_dir
throws an error when given""
, so code likeif let Some(parent) = path.parent() { std::fs::read_dir(parent) }
fails for files in the working directory.Additionally, calling
read_dir
on a relative file without a./
prefix returns file paths without./
prefixes, but there is no way to achieve similar behavior for the working directory becauseread_dir("")
throws an error.read_dir(".")
returns file paths with./
prefixes and is not a solution.This was tested on Unix, and may be Unix-specific, but I lack a Windows machine to easily test against.
The text was updated successfully, but these errors were encountered: