Skip to content

Improved documentation of Path::exists #80979

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

Improved documentation of Path::exists #80979

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
36672e1
Improved documentation of Path::exists
Kixunil Jan 13, 2021
a62309f
Use canonical invocation of fs::metadata
Kixunil Mar 30, 2021
c6f83bf
Applied feedback from review
Kixunil Mar 30, 2021
5422764
Fixed error handling example
Kixunil Mar 30, 2021
fc4b83a
Update formatting
Kixunil Mar 30, 2021
4682f32
Type error fix
Kixunil Mar 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 28 additions & 3 deletions library/std/src/path.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2386,13 +2386,26 @@ impl Path {
fs::read_dir(self)
}

/// Returns `true` if the path points at an existing entity.
/// Returns `true` if the path points at an existing entity and syscall succeeds.
Copy link
Member

@nagisa nagisa Jan 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I'm not sure if wording this in terms of syscalls is the best way to approach this, not to mention that it is not entirely clear what kind of syscall is being referenced here. This also, in some ways, leaks the implementation details which aren't necessarily true on all targets or in the future.

There's already some elaboration on additional cases where false can be returned in the next paragraph. I think that may be a better place for this note. For example:

This function will traverse symbolic links to query information about the destination file. In the case where the information cannot be queried successfully (e.g. there are broken symbolic links) this will return false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I also felt it's not perfect but didn't come up with anything else. Now that I think of it again, maybe "and no internal failure occurs"?

This also, in some ways, leaks the implementation details which aren't necessarily true on all targets or in the future.

I don't think it's physically possible to check existence of a file without some kind of syscall and would be highly surprised if there was one guaranteed to never fail.

There's already some elaboration on additional cases where false can be returned in the next paragraph.

I believe it'd be valuable to have at least some small head up in the summary, because the method looks simpler than it actually is. Someone could just read the description and signature, think "ah, that's obvious" and go write the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @nagisa's comment and would prefer if this were not worded specifically around "if the syscall succeeds", but rather in terms of the state of the underlying system which std is providing the caller an API over.

Maybe like:

Returns true if we can tell that the path points at an existing entity. False if it doesn't or if we can't tell.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That wording sounds good.

///
/// This function will traverse symbolic links to query information about the
/// destination file. In case of broken symbolic links this will return `false`.
///
/// If you cannot access the directory containing the file, e.g., because of a
/// permission error, this will return `false`.
/// # Caveats
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't generally call out the caveats as a separate section elsewhere in the documentation of the standard library, as it is very often implied (and similarly, the paragraph about traversing the symbolic links could also be considered a caveat, but is outside of this section).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So just simple bold?

the paragraph about traversing the symbolic links could also be considered a caveat, but is outside of this section

I actually don't find this behavior surprising at all as all other applications behave exactly same. Willing to reconsider if there's a bunch of people who find it surprising.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am on board with a Caveats heading here, it seems appropriate.

///
/// Checking if a file exists and then attempting to use it is vulnerable to race
/// conditions. It is better to attempt the operation and handle the error when it
/// fails.
///
/// If you cannot access the metadata of the file, e.g., because of a permission
/// error on the containing directory, this will return `false`. In many cases this
/// is not desirable as it silently ignores error that might be serious.
///
/// Sometimes existence of a file is not what's actually interesting for an
/// application but its accessibility. This function does **not** check if the file
/// is accessible by the calling user. Calling `access` on Unix (or a similar
/// syscall) would be more appropriate. Rust `std` currently does not provide such
/// method.
///
/// # Examples
///
Expand All @@ -2405,6 +2418,18 @@ impl Path {
///
/// This is a convenience function that coerces errors to false. If you want to
/// check errors, call [`fs::metadata`].
///
/// An example of proper error handling.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it perhaps make more sense to just link readers to metadata which will have its own examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. My intention was to provide a simple snippet people could copy-paste to do the job properly. So I'd agree to moving the example to metadata. I'm not entirely sure people will find it when linked from exists so maybe add a simple note but maybe that's excessive.

/// ```no_run
/// use std::path::Path;
/// use std::io::ErrorKind;
/// use std::fs::metadata;
/// let exists = metadata(Path::new("does_not_exist.txt"))
/// .map(|_| true)
/// .or_else(|error| if error.kind() == ErrorKind::NotFound { Ok(false) } else { Err(error) } )
/// .expect("failed to check existence of a file");
/// assert!(!exists);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be clearer with the 3 cases separated as such, rather than having to reason through these not-a-great-fit combinators.

use std::fs;
use std::io::ErrorKind;

let exists = match fs::metadata("does_not_exist.txt") {
    Ok(_) => true,
    Err(err) if err.kind() == ErrorKind::NotFound => false,
    Err(err) => panic!("failed to check existence of file: {}", err),
};

assert!(!exists);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separately, it strikes me as odd to refer to a panic (expect) as "proper error handling". Could this be reworded?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, same change was made in the new try_exists() method. I'm not sure if/how the documentation should be changed taking into account there's a method in nightly. I guess referring to it from stable is not usual, right?

I assumed panic is OK in this context because of the assert! below, but changing to return Err(err) would make sense.

/// ```
#[stable(feature = "path_ext", since = "1.5.0")]
pub fn exists(&self) -> bool {
fs::metadata(self).is_ok()
Expand Down