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

fs::Metadata::is_dir() and is_file() should be deprecated #76487

Closed
marcospb19 opened this issue Sep 8, 2020 · 9 comments
Closed

fs::Metadata::is_dir() and is_file() should be deprecated #76487

marcospb19 opened this issue Sep 8, 2020 · 9 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@marcospb19
Copy link
Contributor

marcospb19 commented Sep 8, 2020

Resume

I'm personally confused about the existence of

fs::Metadata::is_dir() and
fs::Metadata::is_file()

At the same time that we DON'T have a
fs::Metadata::is_symlink()

It doesn't make a lot of sense

Context before we go:

fs::Metadata::file_type() returns a fs::FileType

fs::FileType just contains 3 functions, 2 of them can be accessed by fs::Metadata, they are is_dir() and is_file() (yes, same names!).

But, keep in mind that they are all mutually exclusive, that is, a file can never be of directory type and file type, or file type and symlink type.

Problems (I'm confused about this O.o)

Considering that we got the metadata from a file:

let metadata = fs::symlink_metadata("file").unwrap();

If we want to know if the file is a symbolic link, we have 2 ways:

  1. Without acessing .file_type():
let is_symlink: bool = !(metadata.is_dir() || metadata.is_file()); // This is wrong, see conversation below
  1. Getting the fs::FileType:
metadata.is_symlink();

So, if the information is already accessible without file_type(), why isn't a is_symlink() function at fs::Metadata just like is_dir() and is_file()?

Is some other reason that I'm not aware of?

Conclusion and proposals (need more discussion)

There's no point on having functions in fs::Metadata that are exactly equal to what is available getting fs::FileType by calling file_type().

I originally wanted to propose the removal of is_dir() and is_file() from fs::Metadata, but maybe this would make getting the file_type a little too long, so here are my proposals to fix it sanely.

Ranked by the solutions I personally find the better for the language (please add comments into the debate):

  1. Deprecate is_dir() and is_file() from fs::Metadata, and add fs::file_type that returns directly the fs::FileType.
  2. Just deprecate is_dir() and is_file() from fs::Metadata.
  3. Add another redundant function, fs::Metadata::is_symlink(), to help like is_file() and is_dir() have been doing since 1.1.0 (we're at 1.4.8 at the time of this writing).
@marcospb19
Copy link
Contributor Author

fs::file_type() isn't implemented directly to retrieve fs::FileType because in some operating systems (like unix), to get the file type, you need to fill a struct with all this metadata information that we are familiar with.


Some of this inconsistency seems to be ""explained"" by the order things got added.

Both functions were already present in fs::Metadata by 1.0, and FileType with all it's methods was added in 1.1.

fs::Metadata just remained untouched while the context around it was changing.

@mati865
Copy link
Contributor

mati865 commented Sep 8, 2020

Without going deeply into the issue: stable functions cannot be removed from std.

@marcospb19 marcospb19 changed the title fs::Metadata::is_dir() and is_file() should be removed fs::Metadata::is_dir() and is_file() should be deprecated Sep 8, 2020
@Dylan-DPC-zz Dylan-DPC-zz added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 9, 2020
@jonas-schievink
Copy link
Contributor

I disagree that they should be deprecated. They provide a convenient shorthand for what would otherwise require an additional function call and are widely used in the ecosystem. Deprecating them would produce widespread warnings for no good reason.

@the8472
Copy link
Member

the8472 commented Sep 9, 2020

Opened files are not symlinks because the OS will resolve the symlink before opening (barring cases such as O_PATH), so you do not need to check metadata for symlinks if you got it from a File.
On the other hand when iterating a directory you usually do not get Metadata but FileType because (at least on linux) the latter is directly available in directory entry batches you retrieve via getdents while other metadata must be retrieved with an extra syscall per file.

let is_symlink: bool = !(metadata.is_dir() || metadata.is_file());

This is not correct since there can be file types other than symlinks or directories. See https://doc.rust-lang.org/std/os/unix/fs/trait.FileTypeExt.html

@marcospb19
Copy link
Contributor Author

marcospb19 commented Sep 9, 2020

@the8472

Opened files are not symlinks because the OS will resolve the symlink before opening (barring cases such as O_PATH), so you do not need to check metadata for symlinks if you got it from a File.

Correct, but this isn't related to the issue I explained above, I had to explicitly check for file types in a current project and I found out that there is no is_symlink() for metadata, this is inconsistent with the other functions I already mentioned. I do not want to read from a File, I want to display the file type.

let is_symlink: bool = !(metadata.is_dir() || metadata.is_file());

This is not correct since there can be file types other than symlinks or directories.

You're right, maybe I should take the opportunity to make an PR to make this clearer in the documentation.


On the other hand when iterating a directory you usually do not get Metadata but FileType because (at least on linux) the latter is directly available in directory entry batches you retrieve via getdents while other metadata must be retrieved with an extra syscall per file.

Thanks for this information, indeed, fs::DirEntry.file_type() is a thing, do you think that this does cancels my proposal of fs::file_type()? It look like those are 2 different things, considering that fs::file_type() would not require you to iterate a directory.

@marcospb19
Copy link
Contributor Author

@jonas-schievink

I disagree that they should be deprecated. They provide a convenient shorthand for what would otherwise require an additional function call and are widely used in the ecosystem. Deprecating them would produce widespread warnings for no good reason.

I understand your point, but if this is intended to provide a convenient shorthand, then where is the fs::Metadata::is_symlink()?

This makes code weirder.

let is_file = path.symlink_metadata()?.is_file();
let is_dir = path.symlink_metadata()?.is_dir();
let is_symlink = path.symlink_metadata()?.file_type().is_symlink();

@the8472
Copy link
Member

the8472 commented Sep 9, 2020

This makes code weirder.

You can always write

let is_file = path.symlink_metadata()?.file_type().is_file();
let is_dir = path.symlink_metadata()?.file_type().is_dir();
let is_symlink = path.symlink_metadata()?.file_type().is_symlink();

Nothing says you must use the metadata functions.

You're right, maybe I should take the opportunity to make an PR to make this clearer in the documentation.

Sure, I guess referring to FileType for additional types might help a beginner not well-versed in these filesystem details.

@marcospb19
Copy link
Contributor Author

I see now that suggesting deprecation (and removal) of heavily used functions was reckless by me.

However, I still think that we should add the short hand .is_symlink() for fs::Metadata, we could assume that if is_file() and is_dir() is useful, then is_symlink() would also be useful.

In this case, file_type() would be necessary only when using std::os::unix::fs::FileTypeExt, now it is also needed when we want to check for symbolic links.

@marcospb19
Copy link
Contributor Author

Closing because Metadata::is_symlink was implemented by @maxwase, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants