Skip to content
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

Added try_exists() method to std::path::Path #81822

Merged
merged 2 commits into from
Mar 16, 2021

Conversation

Kixunil
Copy link
Contributor

@Kixunil Kixunil commented Feb 6, 2021

This method is similar to the existing exists() method, except it
doesn't silently ignore the errors, leading to less error-prone code.

This change intentionally does NOT touch the documentation of exists()
nor recommend people to use this method while it's unstable.
Such changes are reserved for stabilization to prevent confusing people.

Apart from that it avoids conflicts with #80979.

@joshtriplett requested this PR in internals discussion

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 6, 2021
@rust-log-analyzer

This comment has been minimized.

library/std/src/path.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@ChrisDenton
Copy link
Member

One thing I'm unclear on is how this should work on other platforms. I'm thinking in particular of Windows which does not have the same folder issue as Linux but has its own way of working. E.g. GetFileAttributes can have one of the following outcomes:

  • Success: the file exists
  • Access denied error: the file exists but the attributes can't be read.
  • File not found error: the file does not exist
  • Other error: Something very unexpected happened.

If a file does exist but reading its metadata fails, then should it return an error? Or true?

@Kixunil
Copy link
Contributor Author

Kixunil commented Feb 6, 2021

@ChrisDenton access denied is just one (obvious) example, there can be various other reasons depending on the underlying OS.

If a file does exist but reading its metadata fails

If metadata() fails with any error other than NotFound then we don't know if it exists. The only reasonable return value is thus Err. That was the entire point of previous discussion.

@ChrisDenton
Copy link
Member

ChrisDenton commented Feb 6, 2021

there can be various other reasons depending on the underlying OS

Yes the behaviour can be very OS specific, which is the point I was trying to make using Windows as an example. But if the idea is to return Ok(true) only if the file's metadata is readable then I do think the documentation should make that clear in some way.

@rust-log-analyzer

This comment has been minimized.

library/std/src/path.rs Outdated Show resolved Hide resolved
@joshtriplett joshtriplett added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 6, 2021
@joshtriplett
Copy link
Member

joshtriplett commented Feb 6, 2021

This looks good to me (with the suggested fix applied).

Thank you for working on this!

This method is similar to the existing `exists()` method, except it
doesn't silently ignore the errors, leading to less error-prone code.

This change intentionally does NOT touch the documentation of `exists()`
nor recommend people to use this method while it's unstable.
Such changes are reserved for stabilization to prevent confusing people.

Apart from that it avoids conflicts with rust-lang#80979.
@Kixunil
Copy link
Contributor Author

Kixunil commented Feb 6, 2021

@joshtriplett I guess you meant fix that match, so I did. Happy to have helped!

Should I open the tracking issue already or wait for something first?

@Kixunil
Copy link
Contributor Author

Kixunil commented Feb 6, 2021

@ChrisDenton do I understand correctly that Windows has some other function to check if file exists and also a function to read metadata and the latter could fail even if the former doesn't?

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 23, 2021
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 13, 2021
@ChrisDenton
Copy link
Member

@ChrisDenton do I understand correctly that Windows has some other function to check if file exists and also a function to read metadata and the latter could fail even if the former doesn't?

There are two issues I think. The first one is really about how exists is implemented in terms of fs::metadata. The Windows implementation of fs::metadata involves two or more OS calls (first to open the file and then to query metadata). Querying metadata can fail but we only need to open the file to confirm the file exists.

The second issue is that exists and try_exists can fail for files that do exist and that the error itself indicates the file does indeed exist. For example, when accessing a local file both ERROR_SHARING_VIOLATION and ERROR_ACCESS_DENIED indicate the file exists but cannot be opened. Note that, unlike Linux, opening won't be denied by permissions of the parent directory because only the file's permissions are taking into account.

I don't think we necessarily need to handle or document all possibilities but I think there could be some more robust wording about what an error can mean. Though I'm struggling with how to phrase it.

@Kixunil
Copy link
Contributor Author

Kixunil commented Mar 15, 2021

@ChrisDenton thank you for explaining!

I'd actually prefer to handle those cases properly. If the function name indicates checking of existence of a file it should be as close to reality as possible. It also avoids wordy documentation of such behavior.

/// ```
// FIXME: stabilization should modify documentation of `exists()` to recommend this method
// instead.
#[unstable(feature = "path_try_exists", issue = "none")]
Copy link
Member

Choose a reason for hiding this comment

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

@rust-lang/libs if no one opposes i'll r+ after the tracking issue is filed.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took it as "yes" answer to my question about opening the tracking issue, so I opened it (#83186) and filled.

This adds the ID of the tracking issue to the feature.
@kennytm
Copy link
Member

kennytm commented Mar 16, 2021

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 16, 2021

📌 Commit 4330268 has been approved by kennytm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 16, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 16, 2021
Rollup of 10 pull requests

Successful merges:

 - rust-lang#81822 (Added `try_exists()` method to `std::path::Path`)
 - rust-lang#83072 (Update `Vec` docs)
 - rust-lang#83077 (rustdoc: reduce GC work during search)
 - rust-lang#83091 (Constify `copy` related functions)
 - rust-lang#83156 (Fall-back to sans-serif if Arial is not available)
 - rust-lang#83157 (No background for code in portability snippets)
 - rust-lang#83160 (Deprecate RustcEncodable and RustcDecodable.)
 - rust-lang#83162 (Specify *.woff2 files as binary)
 - rust-lang#83172 (More informative diagnotic from `x.py test` attempt atop beta checkout)
 - rust-lang#83196 (Use delay_span_bug instead of panic in layout_scalar_valid_range)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 62d38da into rust-lang:master Mar 16, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 16, 2021
@Kixunil Kixunil deleted the path_try_exists branch March 17, 2021 08:01
cgwalters added a commit to cgwalters/cap-std that referenced this pull request Feb 8, 2022
I want to use this for the same reason as
rust-lang/rust#81822
was added to std.  While it's currently unstable, I can't imagine
it would change.
cgwalters added a commit to cgwalters/cap-std that referenced this pull request Feb 8, 2022
I want to use this for the same reason as
rust-lang/rust#81822
was added to std.  While it's currently unstable, I can't imagine
it would change.
cgwalters added a commit to cgwalters/cap-std that referenced this pull request Feb 11, 2022
I want to use this for the same reason as
rust-lang/rust#81822
was added to std.  While it's currently unstable, I can't imagine
it would change.
sunfishcode pushed a commit to bytecodealliance/cap-std that referenced this pull request Feb 11, 2022
I want to use this for the same reason as
rust-lang/rust#81822
was added to std.  While it's currently unstable, I can't imagine
it would change.
snev68 added a commit to snev68/cap-std that referenced this pull request Aug 5, 2024
I want to use this for the same reason as
rust-lang/rust#81822
was added to std.  While it's currently unstable, I can't imagine
it would change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.