Stabilize `is_symlink()` for `Metadata` and `Path` by maxwase · Pull Request #89...

Copy link

Contributor

rust-highfive commented 13 days ago

r? @kennytm

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

rfcbot commented 13 days ago •

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

• should-return-result resolved by #89677 (comment)

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.

nagisa commented 12 days ago •

I'm not fully sure about since version, correct me if I'm wrong

Its accurate provided this PR lands before sometime in October 21st.

Kixunil commented 12 days ago

Shouldn't this return Result in the spirit of try_exists? Or is it considered too inconsistent given existing methods return bool?

joshtriplett commented 11 days ago

@Kixunil Good catch!

@rfcbot concern should-return-result

maxwase commented 11 days ago •

Shouldn't this return Result in the spirit of try_exists? Or is it considered too inconsistent given existing methods return bool?

I don't think it should. There are is_dir and if_file methods with this signature (stable since 1.5.0). This question is also discussed in #76487 and #83186.

Personally, I think it would be inconvenient to return Result in this method, but it might be useful to have try_exists_symlink to some extend, but to what extent?

For now it works well playground

@Kixunil @joshtriplett

Kixunil commented 11 days ago

I personally think that bools are footguns here - easy to forget about the case that syscall may fail. It does make sense on Metadata, which already performed such check though.

The question is will we deprecate exists&co eventually? If so stabilizing this just to deprecate it later doesn't sound well.

? is one-character operator and if you can't be bothered to adjust error types because you don't believe it will ever be error, just unwrap is better for debugging.

joshtriplett commented 11 days ago

I agree that on Metadata we can just return bool. On Path, I think we should return a result.

maxwase commented 10 days ago •

@joshtriplett, Wouldn't it be illogical to have is_dir() -> bool and is_file() -> bool, but is_symlink() -> Result<bool, Error>?

I see 3 ways to solve this:

1. Deprecate is_dir and is_file -> bool signatures and replace these with try_is_.... This will brake A LOT of code and make these methods so inconviniet to use due to error handling. These methods alwo used in functions that retuns a lot of types of errors, so you can't just use ?, also you can't use .unwrap() because it will be strange to see in production code, so you will use .unwrap_or(false), as currently implemented. A programmer who wants to handle methadata error can do it by himself.
2. Accept this PR to comlete a "method hole" and continue thinking about it.
3. Accept this PR and open new, not deprecating old methods. New PR will add try_is..., so it will be convinient to use for everyone.

Kixunil commented 10 days ago

make these methods so inconviniet to use

It's only inconvenient if you intend to write dubious code that silently ignores possibly serious issue. Rust is generally designed to not ignore errors.

also you can't use .unwrap() because it will be strange to see in production code, so you will use .unwrap_or(false)

If you can't use unwrap() in production you can't use unwrap_or(false) either, that's just silently swallowing possibly serious issue. Actually I'd rather use unwrap() in production than implicit unwrap_or(false) because that way I at least learn that something is wrong. Silent errors are super-nightmare of debugging.

Given that it'd be strange that there are methods with is_ that return bool it makes most sense to me to add remaining try_ methods and rename this one to begin with try_.

maxwase commented 10 days ago

@Kixunil I agree with you. So we change is_symlink to try_is_symlink and add in new PR try_ versions for is_file and is_dir? What about deprecation?

nagisa commented 10 days ago

Does try_is_* provide enough value over try { metadata()?.is_*() } to warrant the method? Not to mention that metadata version naturally directs users towards reuse of the metadata query in case they want to check for multiple things (e.g. symlink || file)

Kixunil commented 10 days ago

@maxwase that's what I believe would be the best. Not sure about deprecation but I would be in favor of it once try_* methods are in std long enough.

@nagisa sure, I think simple method is much more likely to stabilize than try { } block. ;)

nagisa commented 10 days ago

try is quite likely addition to the language AFAIK. If we stabilized try within a year's time, is the value provided by such a method over this period really all that high to warrant us dealing with its existence for the remaining lifetime of the language?

My personal opinion is that we should either stabilize Path::is_symlink as is for consistency with Path::is_file and Path::is_dir; or we shouldn't add any function to Path at all, direct users towards metadata and wait for try {} to improve ergonomics somewhat.

yaahc commented 10 days ago •

I think I'm in agreement with @nagisa and @maxwase. I don't believe it would be a good idea to add a Result returning function here to Path directly. All of the related methods on Path have documentation pointing users towards metadata() when one wants to handle errors explicitly, separate from the boolean return from is_file and friends. I think in this case we should be prioritizing internal consistency with other related Path APIs over fears of error-proneness. If we think that these helper APIs that return bools are particularly error-prone then that is a separate issue and we should deprecate them and universally push users towards metadata and symlink_metadata, not introduce a third way to call the same API.

That said, while all of the other methods on Path have a note indicating what API to use when explicit error handling is required, is_symlink does not. I would like to see a similar section added to the documentation for is_symlink to point users towards the fs::symlink_metadata method when explicit error handling is required.

joshtriplett commented 9 days ago

I do personally feel that we should have try versions of these methods, but that doesn't need to be a blocker here.

@rfcbot resolved should-return-result

rfcbot commented 9 days ago

This is now entering its final comment period, as per the review above.

bors commented 9 days ago

The latest upstream changes (presumably #89813) made this pull request unmergeable. Please resolve the merge conflicts.

kennytm commented 7 days ago

if people think that Path::{is_file, is_dir, is_symlink} are error-prone they could first create a clippy lint for these functions

(these exists a clippy::filetype_is_file lint but is checking something else)