From 36672e17eb05551b4f1f3eb521f0fa2110535c01 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Wed, 13 Jan 2021 10:56:43 +0100 Subject: [PATCH 1/6] Improved documentation of Path::exists This improves the documentation of `Path::exists` in several ways: * Clarifies that the method only returns true if the syscall succeeds * Doesn't make it look like there has to be something wrong with the directory for it to fail. * Adds "Caveats" section to make possible dangers more visible * Mentions race conditions which, while known by experienced programmers, is more welcoming towards newbies * Mentions that `access()` may be sometimes more appropriate * Adds an actual example of proper error handling so that users are not detracted from it by having to come up with complicated code. --- library/std/src/path.rs | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/library/std/src/path.rs b/library/std/src/path.rs index 243761e389784..5cf85c2a7cdea 100644 --- a/library/std/src/path.rs +++ b/library/std/src/path.rs @@ -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. /// /// 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 + /// + /// 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 /// @@ -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. + /// ```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); + /// ``` #[stable(feature = "path_ext", since = "1.5.0")] pub fn exists(&self) -> bool { fs::metadata(self).is_ok() From a62309f288a91f8b49456ebd7b1d7e84235755c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Habov=C5=A1tiak?= Date: Tue, 30 Mar 2021 11:14:36 +0200 Subject: [PATCH 2/6] Use canonical invocation of fs::metadata Co-authored-by: David Tolnay --- library/std/src/path.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/path.rs b/library/std/src/path.rs index 5cf85c2a7cdea..444a8554b6a58 100644 --- a/library/std/src/path.rs +++ b/library/std/src/path.rs @@ -2424,7 +2424,7 @@ impl Path { /// use std::path::Path; /// use std::io::ErrorKind; /// use std::fs::metadata; - /// let exists = metadata(Path::new("does_not_exist.txt")) + /// let exists = fs::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"); From c6f83bfefbf0682c58d357676077a6622ba68a7b Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Tue, 30 Mar 2021 11:31:27 +0200 Subject: [PATCH 3/6] Applied feedback from review This makes the error handling example cleaner and rewords things. --- library/std/src/path.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/library/std/src/path.rs b/library/std/src/path.rs index 444a8554b6a58..784be873b3002 100644 --- a/library/std/src/path.rs +++ b/library/std/src/path.rs @@ -2386,7 +2386,8 @@ impl Path { fs::read_dir(self) } - /// Returns `true` if the path points at an existing entity and syscall succeeds. + /// Returns `true` if the path points at an existing entity. False if it doesn't + /// **or** we can't tell. /// /// This function will traverse symbolic links to query information about the /// destination file. In case of broken symbolic links this will return `false`. @@ -2423,11 +2424,13 @@ impl Path { /// ```no_run /// use std::path::Path; /// use std::io::ErrorKind; - /// use std::fs::metadata; - /// let exists = fs::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"); + /// use std::fs; + /// let exists = match fs::metadata(Path::new("does_not_exist.txt")) { + /// Ok(_) => true, + /// Err(err) if err.kind() == ErrorKind::NotFound => false, + /// Err(err) => return Err(err), + /// }; + /// /// assert!(!exists); /// ``` #[stable(feature = "path_ext", since = "1.5.0")] From 5422764f1fb9b878b77e4dd24569ba9b4725a25e Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Tue, 30 Mar 2021 23:56:49 +0200 Subject: [PATCH 4/6] Fixed error handling example --- library/std/src/path.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/library/std/src/path.rs b/library/std/src/path.rs index 784be873b3002..9af88e6237033 100644 --- a/library/std/src/path.rs +++ b/library/std/src/path.rs @@ -2423,15 +2423,18 @@ impl Path { /// An example of proper error handling. /// ```no_run /// use std::path::Path; - /// use std::io::ErrorKind; + /// use std::io::{ self, ErrorKind}; /// use std::fs; - /// let exists = match fs::metadata(Path::new("does_not_exist.txt")) { - /// Ok(_) => true, - /// Err(err) if err.kind() == ErrorKind::NotFound => false, - /// Err(err) => return Err(err), - /// }; /// - /// assert!(!exists); + /// fn main() -> io::Result<()> { + /// let exists = match fs::metadata(Path::new("does_not_exist.txt")) { + /// Ok(_) => true, + /// Err(err) if err.kind() == ErrorKind::NotFound => false, + /// Err(err) => return Err(err), + /// }; + /// + /// assert!(!exists); + /// } /// ``` #[stable(feature = "path_ext", since = "1.5.0")] pub fn exists(&self) -> bool { From fc4b83aae3604424ec6338b25eeffaf3dcd5560a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Habov=C5=A1tiak?= Date: Wed, 31 Mar 2021 00:19:03 +0200 Subject: [PATCH 5/6] Update formatting Co-authored-by: David Tolnay --- library/std/src/path.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/std/src/path.rs b/library/std/src/path.rs index 9af88e6237033..bec4407d78c9d 100644 --- a/library/std/src/path.rs +++ b/library/std/src/path.rs @@ -2422,9 +2422,9 @@ impl Path { /// /// An example of proper error handling. /// ```no_run - /// use std::path::Path; - /// use std::io::{ self, ErrorKind}; /// use std::fs; + /// use std::io::{self, ErrorKind}; + /// use std::path::Path; /// /// fn main() -> io::Result<()> { /// let exists = match fs::metadata(Path::new("does_not_exist.txt")) { From 4682f322a711bcef9e5cb74718361a56501cffed Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Wed, 31 Mar 2021 01:04:21 +0200 Subject: [PATCH 6/6] Type error fix --- library/std/src/path.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/library/std/src/path.rs b/library/std/src/path.rs index bec4407d78c9d..a61ccf01b9920 100644 --- a/library/std/src/path.rs +++ b/library/std/src/path.rs @@ -2434,6 +2434,7 @@ impl Path { /// }; /// /// assert!(!exists); + /// Ok(()) /// } /// ``` #[stable(feature = "path_ext", since = "1.5.0")]