rustfmt: Make error handling more idiomatic #914

kamalmarhubi · 2016-04-09T20:51:09Z

These commits improve error handling in the rustfmt binary more
idiomatic by

replacing the Operation::InvalidInput variant with Result
using the try!() macro instead of explicit matching
using guard patterns in a match expression

kamalmarhubi · 2016-04-09T21:25:35Z

I'll reopen this in a bit; missed some things.

kamalmarhubi · 2016-04-10T00:25:00Z

Ugh I pushed and can't reopen. Sadness.

kamalmarhubi · 2016-04-10T00:25:08Z

Oh but now I can?

nrc · 2016-04-10T21:25:00Z

src/bin/rustfmt.rs

 use std::fs::{self, File};
 use std::io::{self, ErrorKind, Read, Write};
 use std::path::{Path, PathBuf};
 use std::str::FromStr;

 use getopts::{Matches, Options};

+type Error = Box<error::Error + Send + Sync>;


Do we need this error type? Can we just use String as our error type?

The try! macro won't work with String unless we put .map_err(ToString::to_string) or something like that all over. The Box<Error+Send+Sync allows String or any other Error impl.

In which case can we have FmtError rather than just Error for the same reason as Result below.

nrc · 2016-04-10T21:42:50Z

Thanks for the PR! I like the overall thrust of moving towards Results, etc. There are a few minor comments inline to address, in particular I'm curious if we can avoid the complex Error type or if you have plans for that in the future or anything.

I'm not keen on the refactoring in the second commit. That seems to make the match slightly more complex (IMO) by adding an extra branch, although it's not a big difference either way.

This commit replaces the `Operation::InvalidInput` variant with `Result`, and uses the `try!()` macro instead of explicit matching.

kamalmarhubi · 2016-04-10T22:07:45Z

in particular I'm curious if we can avoid the complex Error type or if you have plans for that in the future or anything.

For now, this is me peeling off bits of my work on #434, where I was getting frustrated by the error handling in the binary. Let me know if you want a bit of "advance warning" on what I'm working on there, or if peeling self-contained bits as I've done here and, eg, in #857, #891, #910 is ok.

I do have a plan to use Result instead of panicking throughout the code base, but it's a much more disruptive change and so far isn't in the way of what I've been doing for #434. That would involve a more specialized Error type that would live in the lib rather than the binary.

nrc · 2016-04-10T22:19:17Z

Peeling bits off is fine, I think. If you have any huge changes in mind, it might be better to discuss in advance, but I think you know the code well enough that small to medium things should be fine.

It is kind of annoying about the String/Error thing, but it sounds like limitation of try!. I wonder if we can fix that in Rust with the move to ?.

kamalmarhubi · 2016-04-10T22:38:36Z

It is kind of annoying about the String/Error thing, but it sounds like limitation of try!. I wonder if we can fix that in Rust with the move to ?.

My reading of the RFC is it'lI still go through From::from. Specifically:

Naturally, in this case the types of the "exceptions thrown by" foo() and
bar() must unify. Like the current try!() macro, the ? operator will also
perform an implicit "upcast" on the exception type.
―RFC 243

I think you could get the behavior you want with a blanket impl<E: Error> From<E> for String that used Error::description. No idea if that would be accepted into std, but it would be nice!

This commit changes the match in `lookup_project_file` to use pattern guards.

kamalmarhubi · 2016-04-10T23:07:56Z

I think you could get the behavior you want with a blanket impl<E: Error> From for String that used Error::description. No idea if that would be accepted into std, but it would be nice!

Runs afoul of coherence.

matklad · 2016-04-11T20:09:51Z

src/bin/rustfmt.rs

+            // `rustfmt.toml`.
+            Ok(ref md) if md.is_file() => return Ok(Some(config_file)),
+            // Return the error if it's something other than `NotFound`; otherwise we didn't find
+            // the project file yet, and continue searching.
            Err(e) => {
                if e.kind() != ErrorKind::NotFound {


I think you can legitimately get ErrorKind::PermissionDenied here. Maybe it makes sense to return Ok(None) for an unknown error?

I think you can legitimately get ErrorKind::PermissionDenied here.

I think that erroring out makes more sense. It is more likley that they've borked their permissions than they actually want an unreadable file named rustfmt.toml to be in the tree.

Maybe it makes sense to return Ok(None) for an unknown error?

I don't think so. If somone has a project rustfmt.toml file and there's a filesystem hiccup, I don't think they'd expect or want their project reformatted according to the formating defaults.

(As a quick note, this PR is a refactor and isn't intended to change any user visible behavior.)

It is more likley that they've borked their permissions than they actually want an unreadable file named rustfmt.toml to be in the tree.

I meant a hypothetical use case when the user doesn't have rustfmt.toml at all, but also doesn't have read permissions outside the home directory. For this use case, this would Err(PermissionDenied) instead of reformatting with default settings.

fs::metadata() is stat(2) on unix, which requires search permission on the path prefix rather than read permission on the file. As I understand it, it's not possible to have a situation where calling it would succeed at a lower level in the path hierarchy but fail at a higher level.

I don't know enough about Windows to say anything about how permissions work there.

kamalmarhubi · 2016-04-12T00:13:02Z

(Moved comment to be a "line note" at #914 (comment))

kamalmarhubi · 2016-04-13T04:24:35Z

@nrc ping! Is there anything else you'd like done here?

nrc · 2016-04-13T20:41:01Z

Looks good, thanks!

kamalmarhubi closed this Apr 9, 2016

kamalmarhubi reopened this Apr 10, 2016

kamalmarhubi force-pushed the invalid-operation-result branch 3 times, most recently from 781c79e to 383b69c Compare April 10, 2016 06:16

nrc reviewed Apr 10, 2016
View reviewed changes

kamalmarhubi added 2 commits April 10, 2016 17:57

rustfmt: Move getopts::Options creation to its own function

b55e50f

rustfmt: Make error handling more idiomatic

fe5fa87

This commit replaces the `Operation::InvalidInput` variant with `Result`, and uses the `try!()` macro instead of explicit matching.

kamalmarhubi force-pushed the invalid-operation-result branch from 383b69c to e7220e1 Compare April 10, 2016 22:08

rustfmt: Simplify match in project file lookup loop

7242735

This commit changes the match in `lookup_project_file` to use pattern guards.

kamalmarhubi force-pushed the invalid-operation-result branch from e7220e1 to 7242735 Compare April 10, 2016 22:43

marcusklaas mentioned this pull request Apr 11, 2016

unify format_string and format_file #921

Merged

matklad reviewed Apr 11, 2016
View reviewed changes

nrc merged commit 1824669 into rust-lang:master Apr 13, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustfmt: Make error handling more idiomatic #914

rustfmt: Make error handling more idiomatic #914

kamalmarhubi commented Apr 9, 2016

kamalmarhubi commented Apr 9, 2016

kamalmarhubi commented Apr 10, 2016

kamalmarhubi commented Apr 10, 2016

nrc Apr 10, 2016

kamalmarhubi Apr 10, 2016

nrc Apr 10, 2016

nrc commented Apr 10, 2016

kamalmarhubi commented Apr 10, 2016

nrc commented Apr 10, 2016

kamalmarhubi commented Apr 10, 2016

kamalmarhubi commented Apr 10, 2016

matklad Apr 11, 2016

kamalmarhubi Apr 11, 2016

kamalmarhubi Apr 11, 2016

matklad Apr 11, 2016

kamalmarhubi Apr 12, 2016

kamalmarhubi commented Apr 12, 2016

kamalmarhubi commented Apr 13, 2016

nrc commented Apr 13, 2016

rustfmt: Make error handling more idiomatic #914

rustfmt: Make error handling more idiomatic #914

Conversation

kamalmarhubi commented Apr 9, 2016

kamalmarhubi commented Apr 9, 2016

kamalmarhubi commented Apr 10, 2016

kamalmarhubi commented Apr 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrc commented Apr 10, 2016

kamalmarhubi commented Apr 10, 2016

nrc commented Apr 10, 2016

kamalmarhubi commented Apr 10, 2016

kamalmarhubi commented Apr 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamalmarhubi commented Apr 12, 2016

kamalmarhubi commented Apr 13, 2016

nrc commented Apr 13, 2016