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.

Sign up for GitHub

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

rustfmt: Make error handling more idiomatic #914

Merged
merged 3 commits into from
Apr 13, 2016
Merged

rustfmt: Make error handling more idiomatic #914

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b55e50f
rustfmt: Move getopts::Options creation to its own function
kamalmarhubi Apr 10, 2016
fe5fa87
rustfmt: Make error handling more idiomatic
kamalmarhubi Apr 9, 2016
7242735
rustfmt: Simplify match in project file lookup loop
kamalmarhubi Apr 10, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 44 additions & 55 deletions src/bin/rustfmt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,16 @@ extern crate getopts;
use rustfmt::{run, Input};
use rustfmt::config::{Config, WriteMode};

use std::env;
use std::{env, error};
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 FmtError = Box<error::Error + Send + Sync>;
type FmtResult<T> = std::result::Result<T, FmtError>;

/// Rustfmt operations.
enum Operation {
Expand All @@ -42,10 +44,6 @@ enum Operation {
Version,
/// Print detailed configuration help.
ConfigHelp,
/// Invalid program input.
InvalidInput {
reason: String,
},
/// No file specified, read from stdin
Stdin {
input: String,
Expand All @@ -55,7 +53,7 @@ enum Operation {

/// Try to find a project file in the given directory and its parents. Returns the path of a the
/// nearest project file if one exists, or `None` if no project file was found.
fn lookup_project_file(dir: &Path) -> io::Result<Option<PathBuf>> {
fn lookup_project_file(dir: &Path) -> FmtResult<Option<PathBuf>> {
let mut current = if dir.is_relative() {
try!(env::current_dir()).join(dir)
} else {
Expand All @@ -67,19 +65,17 @@ fn lookup_project_file(dir: &Path) -> io::Result<Option<PathBuf>> {
loop {
let config_file = current.join("rustfmt.toml");
match fs::metadata(&config_file) {
Ok(md) => {
// Properly handle unlikely situation of a directory named `rustfmt.toml`.
if md.is_file() {
return Ok(Some(config_file));
}
}
// If it's not found, we continue searching; otherwise something went wrong and we
// return the error.
// Only return if it's a file to handle the unlikely situation of a directory named
// `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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return Err(e);
return Err(FmtError::from(e));
}
}
_ => {}
}

// If the current directory has no parent, we're done searching.
Expand All @@ -93,7 +89,7 @@ fn lookup_project_file(dir: &Path) -> io::Result<Option<PathBuf>> {
///
/// Returns the `Config` to use, and the path of the project file if there was
/// one.
fn resolve_config(dir: &Path) -> io::Result<(Config, Option<PathBuf>)> {
fn resolve_config(dir: &Path) -> FmtResult<(Config, Option<PathBuf>)> {
let path = try!(lookup_project_file(dir));
if path.is_none() {
return Ok((Config::default(), None));
Expand All @@ -108,7 +104,7 @@ fn resolve_config(dir: &Path) -> io::Result<(Config, Option<PathBuf>)> {
/// read the given config file path recursively if present else read the project file path
fn match_cli_path_or_file(config_path: Option<PathBuf>,
input_file: &Path)
-> io::Result<(Config, Option<PathBuf>)> {
-> FmtResult<(Config, Option<PathBuf>)> {

if let Some(config_file) = config_path {
let (toml, path) = try!(resolve_config(config_file.as_ref()));
Expand All @@ -119,7 +115,7 @@ fn match_cli_path_or_file(config_path: Option<PathBuf>,
resolve_config(input_file)
}

fn update_config(config: &mut Config, matches: &Matches) -> Result<(), String> {
fn update_config(config: &mut Config, matches: &Matches) -> FmtResult<()> {
config.verbose = matches.opt_present("verbose");
config.skip_children = matches.opt_present("skip-children");

Expand All @@ -130,11 +126,14 @@ fn update_config(config: &mut Config, matches: &Matches) -> Result<(), String> {
config.write_mode = write_mode;
Ok(())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refactoring doesn't seem to be an improvement to me. Admittedly, the original match was a bit complex, but now we have two nested if lets, an early return, and a separate return value, which seems worse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, I can drop it. I kept it as a second commit because it wasn't the same "flavour" as the first. :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, I think this match change is in the first commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you're right. My bad!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fixed now.

}
Some(Err(_)) => Err(format!("Invalid write-mode: {}", write_mode.expect("cannot happen"))),
Some(Err(_)) => {
Err(FmtError::from(format!("Invalid write-mode: {}",
write_mode.expect("cannot happen"))))
}
}
}

fn execute() -> i32 {
fn make_opts() -> Options {
let mut opts = Options::new();
opts.optflag("h", "help", "show this message");
opts.optflag("V", "version", "show version information");
Expand All @@ -154,32 +153,21 @@ fn execute() -> i32 {
found reverts to the input file path",
"[Path for the configuration file]");

let matches = match opts.parse(env::args().skip(1)) {
Ok(m) => m,
Err(e) => {
print_usage(&opts, &e.to_string());
return 1;
}
};
opts
}

let operation = determine_operation(&matches);
fn execute(opts: &Options) -> FmtResult<()> {
let matches = try!(opts.parse(env::args().skip(1)));

match operation {
Operation::InvalidInput { reason } => {
print_usage(&opts, &reason);
1
}
match try!(determine_operation(&matches)) {
Operation::Help => {
print_usage(&opts, "");
0
}
Operation::Version => {
print_version();
0
}
Operation::ConfigHelp => {
Config::print_docs();
0
}
Operation::Stdin { input, config_path } => {
// try to read config from local directory
Expand All @@ -190,7 +178,6 @@ fn execute() -> i32 {
config.write_mode = WriteMode::Plain;

run(Input::Text(input), &config);
0
}
Operation::Format { files, config_path } => {
let mut config = Config::default();
Expand Down Expand Up @@ -221,21 +208,26 @@ fn execute() -> i32 {
config = config_tmp;
}

if let Err(e) = update_config(&mut config, &matches) {
print_usage(&opts, &e);
return 1;
}
try!(update_config(&mut config, &matches));
run(Input::File(file), &config);
}
0
}
}
Ok(())
}

fn main() {
let _ = env_logger::init();
let exit_code = execute();

let opts = make_opts();

let exit_code = match execute(&opts) {
Ok(..) => 0,
Err(e) => {
print_usage(&opts, &e.to_string());
1
}
};
// Make sure standard output is flushed before we exit.
std::io::stdout().flush().unwrap();

Expand All @@ -261,17 +253,17 @@ fn print_version() {
option_env!("CARGO_PKG_VERSION_PRE").unwrap_or(""));
}

fn determine_operation(matches: &Matches) -> Operation {
fn determine_operation(matches: &Matches) -> FmtResult<Operation> {
if matches.opt_present("h") {
return Operation::Help;
return Ok(Operation::Help);
}

if matches.opt_present("config-help") {
return Operation::ConfigHelp;
return Ok(Operation::ConfigHelp);
}

if matches.opt_present("version") {
return Operation::Version;
return Ok(Operation::Version);
}

// Read the config_path and convert to parent dir if a file is provided.
Expand All @@ -288,21 +280,18 @@ fn determine_operation(matches: &Matches) -> Operation {
if matches.free.is_empty() {

let mut buffer = String::new();
match io::stdin().read_to_string(&mut buffer) {
Ok(..) => (),
Err(e) => return Operation::InvalidInput { reason: e.to_string() },
}
try!(io::stdin().read_to_string(&mut buffer));

return Operation::Stdin {
return Ok(Operation::Stdin {
input: buffer,
config_path: config_path,
};
});
}

let files: Vec<_> = matches.free.iter().map(PathBuf::from).collect();

Operation::Format {
Ok(Operation::Format {
files: files,
config_path: config_path,
}
})
}