Improve error messages for io::fs #14629

wycats · Jun 3, 2014

This commit improves the error descriptions and details for the functions and methods exposed in io::fs.

The basic idea is that the description should always map onto the high-level operation that the user was trying to perform, with the detail containing the original message. The IoErrorKind is always maintained. The detail also includes additional information (most notably the relevant path, but also other parameters or contextual information).

I added an UpdateIoError trait to make it easier to do these in-place modification, and I'm eager for feedback on the specific strategy.

alexcrichton · Jun 3, 2014

alexcrichton reviewed Jun 3, 2014

View changes

alexcrichton · Jun 3, 2014

src/libstd/io/fs.rs

@@ -134,6 +135,8 @@ impl File {
                    last_nread: -1
                }
            })
+        }).update_err("Couldn't open file", |e| {


Our convention, at least for the compiler, is to start with lowercase letters as opposed to uppercase letters.

alexcrichton · Jun 3, 2014

alexcrichton reviewed Jun 3, 2014

View changes

alexcrichton · Jun 3, 2014

src/libstd/io/fs.rs

@@ -134,6 +135,8 @@ impl File {
                    last_nread: -1
                }
            })
+        }).update_err("Couldn't open file", |e| {
+            format!("{}; path={}; mode={}; access={}", e, path.display(), mode_string(mode), access_string(access))


Could you wrap this line and the ones below to 80 characters?

alexcrichton · Jun 3, 2014

alexcrichton reviewed Jun 3, 2014

View changes

alexcrichton · Jun 3, 2014

src/libstd/io/fs.rs

 }

+//fn io_err_for_path(path: &Path) -> (|&mut IoError| -> String) {
+    //|err| format!("{}; path={}", err, path.display())
+//}


This may have stuck around by accident

alexcrichton · Jun 3, 2014

alexcrichton reviewed Jun 3, 2014

View changes

alexcrichton · Jun 3, 2014

src/libstd/io/fs.rs

+        super::ReadWrite => "readwrite"
+    }
+}
+
 #[cfg(test)]
 #[allow(unused_imports)]
 mod test {


Could you add some tests to make sure there's some more contextual information in the errors returned?

alexcrichton · Jun 3, 2014

alexcrichton reviewed Jun 3, 2014

View changes

alexcrichton · Jun 3, 2014

src/libstd/io/mod.rs

+    fn update_desc(self, desc: &'static str) -> IoResult<T> {
+        self.map_err(|mut e| { e.desc = desc; e })
+    }
+}


This is an interesting pattern, I wonder if it could be generalized? Not a worry for now, though, it's just a private detail.

alexcrichton · Jun 6, 2014

alexcrichton reviewed Jun 6, 2014

View changes

alexcrichton · Jun 6, 2014

src/libstd/io/mod.rs

-            Some(ref s) => write!(fmt, " ({})", *s),
-            None => Ok(())
+        match *self {
+            IoError { kind: OtherIoError, desc: "unknown error", detail: Some(ref detail), .. } =>


I'm actually a little surprised that the compiler lets you get away with , .. here, you matched all the fields!

Heh, good call!

alexcrichton · Jun 6, 2014

alexcrichton reviewed Jun 6, 2014

View changes

alexcrichton · Jun 6, 2014

src/libstd/io/mod.rs

-            detail: if detail {
-                Some(os::error_string(errno))
+            detail: if detail && kind == OtherIoError {
+                Some(os::error_string(errno).as_slice().chars().map(|c| c.to_lowercase()).collect())


Did you lowercase just to stick with our same conventions? I'd be wary about changing what the OS is giving us, and if it's against our conventions it's not really our fault.

Yeah. I think we should try to have consistency between error messages that are produced in userland (like in libuv, for example) and ones that are produced by the OS.

alexcrichton · Jun 6, 2014

alexcrichton reviewed Jun 6, 2014

View changes

alexcrichton · Jun 6, 2014

src/libstd/io/mod.rs

@@ -303,6 +303,8 @@ impl IoError {
    /// struct is filled with an allocated string describing the error
    /// in more detail, retrieved from the operating system.
    pub fn from_errno(errno: uint, detail: bool) -> IoError {
+        use str::Str;


Could you put this at the top of the file instead?

alexcrichton · Jun 6, 2014

alexcrichton reviewed Jun 6, 2014

View changes

alexcrichton · Jun 6, 2014

src/libstd/io/fs.rs

+    macro_rules! error( ($e:expr, $s:expr) => (
+        match $e {
+            Ok(val) => fail!("Should have been an error, was {:?}", val),
+            Err(ref err) => assert!(err.to_str().as_slice().contains($s.as_slice()), format!("`{}` did not contain `{}`", err, $s))


This line may be approaching the 100 character line limit.

It looked like tests had a different limit?

alexcrichton · Jun 6, 2014

alexcrichton reviewed Jun 6, 2014

View changes

alexcrichton · Jun 6, 2014

src/libstd/io/fs.rs

+                |e| format!("{}; path={}", e, file.path.display()))
+        }
+
+        let result: IoResult<int> = update_err(self.fd.read(buf), self);


Is this type ascription necessary? I would have thought that rustc could figure it out.

alexcrichton · Jun 6, 2014

alexcrichton reviewed Jun 6, 2014

View changes

alexcrichton · Jun 6, 2014

src/libstd/io/fs.rs

@@ -452,6 +481,8 @@ pub fn mkdir(path: &Path, mode: FilePermission) -> IoResult<()> {
 /// directory at the provided path, or if the directory isn't empty.
 pub fn rmdir(path: &Path) -> IoResult<()> {
    LocalIo::maybe_raise(|io| io.fs_rmdir(&path.to_c_str()))
+        .update_err("couldn't remove directory",
+            |e| format!("{}; path={}", e, path.display()))


Stylistically, we normally line up the | right after the open-paren so the arguments are all aligned, like:

.update_err("...", |e| ...);

I feel like there is a conspiracy to make each method call at least 10 lines ;)

wycats · Jun 6, 2014

I believe I have responded to all of your comments. 😄

alexcrichton · Jun 6, 2014

r+

bors · Jun 7, 2014

saw approval from alexcrichton
at wycats@5ae6e93

merging wycats/rust/improve-fs-errors = 5ae6e93 into auto

wycats/rust/improve-fs-errors = 5ae6e93 merged ok, testing candidate = 67b0569

alexcrichton · Jun 11, 2014

I'll rebase and take over from here (talked with @wycats on IRC)

Improve error messages for io::fs

Loading status checks…

5ae6e93

alexcrichton closed this Jun 11, 2014

rust-lang/rust

Join GitHub today

Improve error messages for io::fs #14629

Conversation

wycats commented Jun 3, 2014

alexcrichton reviewed Jun 3, 2014 View changes

This comment has been minimized.

alexcrichton reviewed Jun 3, 2014 View changes

This comment has been minimized.

alexcrichton reviewed Jun 3, 2014 View changes

This comment has been minimized.

alexcrichton reviewed Jun 3, 2014 View changes

This comment has been minimized.

alexcrichton reviewed Jun 3, 2014 View changes

This comment has been minimized.

alexcrichton reviewed Jun 6, 2014 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jun 6, 2014 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jun 6, 2014 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jun 6, 2014 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jun 6, 2014 View changes

This comment has been minimized.

alexcrichton reviewed Jun 6, 2014 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

wycats commented Jun 6, 2014

This comment has been minimized.

alexcrichton commented on 5ae6e93 Jun 6, 2014

This comment has been minimized.

bors commented on 5ae6e93 Jun 7, 2014

This comment has been minimized.

bors replied Jun 7, 2014

This comment has been minimized.

bors replied Jun 7, 2014

This comment has been minimized.

bors replied Jun 7, 2014

bors added a commit that referenced this pull request Jun 7, 2014

This comment has been minimized.

alexcrichton commented Jun 11, 2014

alexcrichton closed this Jun 11, 2014

alexcrichton reviewed Jun 3, 2014

View changes

alexcrichton reviewed Jun 3, 2014

View changes

alexcrichton reviewed Jun 3, 2014

View changes

alexcrichton reviewed Jun 3, 2014

View changes

alexcrichton reviewed Jun 3, 2014

View changes

alexcrichton reviewed Jun 6, 2014

View changes

alexcrichton reviewed Jun 6, 2014

View changes

alexcrichton reviewed Jun 6, 2014

View changes

alexcrichton reviewed Jun 6, 2014

View changes

alexcrichton reviewed Jun 6, 2014

View changes

alexcrichton reviewed Jun 6, 2014

View changes

alexcrichton commented on `5ae6e93` Jun 6, 2014

bors commented on `5ae6e93` Jun 7, 2014