Closed Kixunil closed 4 months ago
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 https://github.com/rust-lang/rust/pull/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:
std::fs::exists
as you suggest.std::fs::try_exists
. I'd still want there to be some kind of fs::try_exists
so that different platforms can specialize the implementation. But that can be purely internal to the standard library.std::fs::exists
is that Path::exists
is just a convenience wrapper around std::fs::metadata(path).is_ok()
. A similar rationale could be applied when Path::exists
is implemented as std::fs::try_exists(path).unwrap_or(false)
.@roblabla clearly the doc was copied from Path::try_exist
. Would be nice to fix.
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 https://github.com/rust-lang/rust/pull/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?
No, this should be returning false because the path doesn't exist. As far as I know, the error is only returned when path exists but an error occurs in reading the metadata (e.g. permissions error)
On Tue, Apr 11, 2023, 6:42 PM kaan taha köken @.***> wrote:
Am I missing something std::Path::try_exists("wrong_path")? This always returns false. Should not be returning an error?
— Reply to this email directly, view it on GitHub https://github.com/rust-lang/rust/issues/83186#issuecomment-1503379158, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALJJ7WNLTDV65IZZUAQPFDDXAVNVNANCNFSM4ZIACLTQ . You are receiving this because you commented.Message ID: @.***>
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.
:bell: This is now entering its final comment period, as per the review above. :bell:
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.
Feature gate:
#![feature(path_try_exists)]
This is a tracking issue for
try_exists()
method onstd::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 returnsio::Result<bool>
instead of justbool
.Public API
Steps / History
Unresolved Questions