Tracking Issue for fs_try_exists
Closed this issue · 25 comments
Feature gate: #![feature(path_try_exists)]
This is a tracking issue for try_exists() method on std::path::Path.
This method is similar to exists() method except it does not silently ignore errors that made it impossible to find out if the path actually exists (e.g. lack of permission on parent dir). Thus it naturally returns io::Result<bool> instead of just bool.
Public API
mod fs {
pub fn try_exists<P: AsRef<Path>>(path: P) -> Result<bool>;
}
impl Path {
#[stable(since = "1.63.0")]
pub fn try_exists(&self) -> io::Result<bool>;
}Steps / History
Unresolved Questions
- None yet.
Could try_exists be added as a free function to std::fs and then the std::path::Path method forwards to that? This would be similar to the metadata function.
My motivation for this is to make it easier for platforms to override the metadata based implementation.
In the internals discussion @matklad pointed out that is_file() and is_dir() have same issue. I believe it may be even worse in their case. I wonder would it be reasonable to add those method within the scope of this feature or as a separate change?
Since #85060, there's now std::fs::try_exists but no std::fs::exists (the method only exists on Path). This is somewhat confusing, especially since the documentation for std::fs::try_exists says:
As opposed to the
exists()method, this one doesn’t silently ignore errors unrelated to the path not existing.
Without linking the appropriate exists method.
I think if we're going to have an std::fs::try_exists, we'll want to make an std::fs::exists method as well for symmetry.
I think there are a number of options here:
- Add
std::fs::existsas you suggest. - Don't stabilize
std::fs::try_exists. I'd still want there to be some kind offs::try_existsso that different platforms can specialize the implementation. But that can be purely internal to the standard library. - Keep the current API but improve the docs. The original rationale for not having an
std::fs::existsis thatPath::existsis just a convenience wrapper aroundstd::fs::metadata(path).is_ok(). A similar rationale could be applied whenPath::existsis implemented asstd::fs::try_exists(path).unwrap_or(false).
I've submitted the small PR above which you may want to review.
Nominating to consider stabilization.
We discussed this in today's @rust-lang/libs-api meeting.
We're in favor of stabilizing Path::try_exists. We'd support a partial stabilization for that.
We had a long conversation about whether we should stabilize std::fs::try_exists as-is, stabilize it as std::fs::exists (with the same signature), or providing both std::fs::exists and std::fs::try_exists (with the same signature).
@joshtriplett can I send a stabilization PR now or is something else needed first?
@Kixunil We usually do the FCP on the tracking issue, before opening the stabilization PR. But in this case, since it's a partial stabilization, it's probably better to open a stabilization PR first and do the FCP there. Then it's very clear what exactly we are and aren't stabilizing.
So yes, go ahead. :) (Thanks!)
(You might need to change the feature name for std::fs::try_exists, to avoid having the same feature name being both stable and unstable.)
Yeah, I suspected something along those lines, submitted!
@joshtriplett I was going to edit the issue to be about fs::try_exists since Path::try_exists stabilization PR was merged but I noticed you did the opposite change. What to do? My reasoning is that people interested in the unstable method will find the issue more interesting than people interested in the stable method - those cans still view edit history.
Maybe it's best to open a new tracking issue for fs::try_exists, and close this one as stabilized.
Is there any chance the docs could be clarified a bit? (assuming that's relevant to talk about in this thread, I don't know any better)
As opposed to the Path::exists method, this one doesn’t silently ignore errors
unrelated to the path not existing. (E.g. it will return Err(_) in case of permission
denied on some of the parent directories.)
It's kind of a quadruple negative doesn't + silently ignore + unrelated + not existing and it's been a bit tricky wrapping my brain around. If I understand it correctly, I think something like the following might be a bit easier to read:
`Path::exists` only checks whether or not the file was found and readable. By
contrast, `Path::try_exists()` will return `Ok(true)` or `Ok(false)`, respectively, if
the file was verified to exist or verified not to exist, but propegate an Err(_)
if its existance cannot be confirmed or denied. This can be the case if e.g.,
listing permission is denied on one of the parent directories.
Edit: did this in #110266
What's preventing stabilization of this? I can make a stabilization PR if all is good.
Note that std::Path::try_exists is stable. The only question now is if we want to stabilize std::fs::try_exists or remove it.
I guess that is a question for the libs team (not sure how to ping them here). They can decide how to move forward with it
Mara suggested opening a new tracking issue for std::fs::try_exists and then this can be closed.
Am I missing something std::Path::new("wrong_path")try_exists()? This always returns false. Should not be returning an error?
What @hamza1311 says is correct and intended behavior.
@rust-lang/libs-api:
@rfcbot fcp merge
I am interested in stabilizing fs::try_exists with a rename to fs::exists.
https://doc.rust-lang.org/nightly/std/fs/fn.try_exists.html
Example usage:
// build.rs
fn main() -> Result<()> {
let header = "libstable/include/libstable.h";
println!("cargo:rerun-if-changed={}", header);
if !fs::exists(header)? {
Command::new("git")
.args(["submodule", "update", "--init", "libstable"])
.status()
.exit_ok()?;
}
// bindgen stuff
}fs::exists(pathlike) is a more pleasant way to write Path::new(&pathlike).try_exists(), similar to how fs::metadata(pathlike) is equivalent to Path::new(&pathlike).metadata().
std::path::Path::try_exists (https://doc.rust-lang.org/nightly/std/path/struct.Path.html#method.try_exists) has been stable since Rust 1.63, and retains its usefulness when you are already working with a Path, such as:
fn find_cargo_manifest() -> Result<PathBuf> {
let dir = env::current_dir()?;
let mut dir = dir.as_path();
loop {
let path = dir.join("Cargo.toml");
if path.try_exists()? {
return Ok(path);
}
dir = match dir.parent() {
Some(parent) => parent,
None => return Err(Error::new(ErrorKind::NotFound, "Cargo.toml not found")),
};
}
}Regarding the choice of name fs::exists vs fs::try_exists, I think there is no need for try_ in the name because we can decide to rule out adding a -> bool version of exists in std::fs. If we are able to rule that out now, then keeping try_ in the name no longer serves a purpose. Likewise, fs::metadata is not called fs::try_metadata despite returning Result.
Team member @dtolnay has proposed to merge 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 final comment period, with a disposition to merge, 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.
This will be merged soon.