From ed073a9b7cc7b9e68094d673e3830324c15392cb Mon Sep 17 00:00:00 2001 From: Ivan Veselov Date: Mon, 20 May 2019 10:23:03 +0100 Subject: [PATCH 1/3] Use structopt for command line arguments parsing in cargo-fmt --- Cargo.lock | 68 +++++++++++ Cargo.toml | 1 + src/cargo-fmt/main.rs | 264 ++++++++++++++++++++++++++++++------------ 3 files changed, 257 insertions(+), 76 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b1e1a4b0226..b5eec3599de 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -135,6 +135,20 @@ name = "cfg-if" version = "0.1.7" source = "registry+https://github.com/rust-lang/crates.io-index" +[[package]] +name = "clap" +version = "2.33.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "ansi_term 0.11.0 (registry+https://github.com/rust-lang/crates.io-index)", + "atty 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)", + "bitflags 1.0.4 (registry+https://github.com/rust-lang/crates.io-index)", + "strsim 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)", + "textwrap 0.11.0 (registry+https://github.com/rust-lang/crates.io-index)", + "unicode-width 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)", + "vec_map 0.8.1 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "cloudabi" version = "0.0.3" @@ -314,6 +328,14 @@ dependencies = [ "regex 1.1.6 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "heck" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "unicode-segmentation 1.2.1 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "humantime" version = "1.2.0" @@ -799,6 +821,7 @@ dependencies = [ "rustc-workspace-hack 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.90 (registry+https://github.com/rust-lang/crates.io-index)", "serde_json 1.0.39 (registry+https://github.com/rust-lang/crates.io-index)", + "structopt 0.2.15 (registry+https://github.com/rust-lang/crates.io-index)", "term 0.5.2 (registry+https://github.com/rust-lang/crates.io-index)", "toml 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)", "unicode-segmentation 1.2.1 (registry+https://github.com/rust-lang/crates.io-index)", @@ -886,6 +909,31 @@ name = "stable_deref_trait" version = "1.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" +[[package]] +name = "strsim" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" + +[[package]] +name = "structopt" +version = "0.2.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "clap 2.33.0 (registry+https://github.com/rust-lang/crates.io-index)", + "structopt-derive 0.2.15 (registry+https://github.com/rust-lang/crates.io-index)", +] + +[[package]] +name = "structopt-derive" +version = "0.2.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "heck 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", + "proc-macro2 0.4.27 (registry+https://github.com/rust-lang/crates.io-index)", + "quote 0.6.12 (registry+https://github.com/rust-lang/crates.io-index)", + "syn 0.15.32 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "syn" version = "0.15.32" @@ -935,6 +983,14 @@ dependencies = [ "redox_termios 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "textwrap" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "unicode-width 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "thread_local" version = "0.3.6" @@ -981,6 +1037,11 @@ name = "utf8-ranges" version = "1.0.2" source = "registry+https://github.com/rust-lang/crates.io-index" +[[package]] +name = "vec_map" +version = "0.8.1" +source = "registry+https://github.com/rust-lang/crates.io-index" + [[package]] name = "walkdir" version = "2.2.7" @@ -1045,6 +1106,7 @@ dependencies = [ "checksum cargo_metadata 0.7.4 (registry+https://github.com/rust-lang/crates.io-index)" = "178d62b240c34223f265a4c1e275e37d62da163d421fc8d7f7e3ee340f803c57" "checksum cc 1.0.35 (registry+https://github.com/rust-lang/crates.io-index)" = "5e5f3fee5eeb60324c2781f1e41286bdee933850fff9b3c672587fed5ec58c83" "checksum cfg-if 0.1.7 (registry+https://github.com/rust-lang/crates.io-index)" = "11d43355396e872eefb45ce6342e4374ed7bc2b3a502d1b28e36d6e23c05d1f4" +"checksum clap 2.33.0 (registry+https://github.com/rust-lang/crates.io-index)" = "5067f5bb2d80ef5d68b4c87db81601f0b75bca627bc2ef76b141d7b846a3c6d9" "checksum cloudabi 0.0.3 (registry+https://github.com/rust-lang/crates.io-index)" = "ddfc5b9aa5d4507acaf872de71051dfd0e309860e88966e1051e462a077aac4f" "checksum constant_time_eq 0.1.3 (registry+https://github.com/rust-lang/crates.io-index)" = "8ff012e225ce166d4422e0e78419d901719760f62ae2b7969ca6b564d1b54a9e" "checksum crossbeam-channel 0.3.8 (registry+https://github.com/rust-lang/crates.io-index)" = "0f0ed1a4de2235cabda8558ff5840bffb97fcb64c97827f354a451307df5f72b" @@ -1065,6 +1127,7 @@ dependencies = [ "checksum fuchsia-cprng 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "a06f77d526c1a601b7c4cdd98f54b5eaabffc14d5f2f0296febdc7f357c6d3ba" "checksum getopts 0.2.18 (registry+https://github.com/rust-lang/crates.io-index)" = "0a7292d30132fb5424b354f5dc02512a86e4c516fe544bb7a25e7f266951b797" "checksum globset 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)" = "ef4feaabe24a0a658fd9cf4a9acf6ed284f045c77df0f49020ba3245cfb7b454" +"checksum heck 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)" = "20564e78d53d2bb135c343b3f47714a56af2061f1c928fdb541dc7b9fdd94205" "checksum humantime 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)" = "3ca7e5f2e110db35f93b837c81797f3714500b81d517bf20c431b16d3ca4f114" "checksum ignore 0.4.7 (registry+https://github.com/rust-lang/crates.io-index)" = "8dc57fa12805f367736a38541ac1a9fc6a52812a0ca959b1d4d4b640a89eb002" "checksum itertools 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)" = "5b8467d9c1cebe26feb08c640139247fac215782d35371ade9a2136ed6085358" @@ -1128,11 +1191,15 @@ dependencies = [ "checksum serde_json 1.0.39 (registry+https://github.com/rust-lang/crates.io-index)" = "5a23aa71d4a4d43fdbfaac00eff68ba8a06a51759a89ac3304323e800c4dd40d" "checksum smallvec 0.6.9 (registry+https://github.com/rust-lang/crates.io-index)" = "c4488ae950c49d403731982257768f48fada354a5203fe81f9bb6f43ca9002be" "checksum stable_deref_trait 1.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "dba1a27d3efae4351c8051072d619e3ade2820635c3958d826bfea39d59b54c8" +"checksum strsim 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)" = "8ea5119cdb4c55b55d432abb513a0429384878c15dde60cc77b1c99de1a95a6a" +"checksum structopt 0.2.15 (registry+https://github.com/rust-lang/crates.io-index)" = "3d0760c312538987d363c36c42339b55f5ee176ea8808bbe4543d484a291c8d1" +"checksum structopt-derive 0.2.15 (registry+https://github.com/rust-lang/crates.io-index)" = "528aeb7351d042e6ffbc2a6fb76a86f9b622fdf7c25932798e7a82cb03bc94c6" "checksum syn 0.15.32 (registry+https://github.com/rust-lang/crates.io-index)" = "846620ec526c1599c070eff393bfeeeb88a93afa2513fc3b49f1fea84cf7b0ed" "checksum synstructure 0.10.1 (registry+https://github.com/rust-lang/crates.io-index)" = "73687139bf99285483c96ac0add482c3776528beac1d97d444f6e91f203a2015" "checksum term 0.5.2 (registry+https://github.com/rust-lang/crates.io-index)" = "edd106a334b7657c10b7c540a0106114feadeb4dc314513e97df481d5d966f42" "checksum termcolor 1.0.4 (registry+https://github.com/rust-lang/crates.io-index)" = "4096add70612622289f2fdcdbd5086dc81c1e2675e6ae58d6c4f62a16c6d7f2f" "checksum termion 1.5.1 (registry+https://github.com/rust-lang/crates.io-index)" = "689a3bdfaab439fd92bc87df5c4c78417d3cbe537487274e9b0b2dce76e92096" +"checksum textwrap 0.11.0 (registry+https://github.com/rust-lang/crates.io-index)" = "d326610f408c7a4eb6f51c37c330e496b08506c9457c9d34287ecc38809fb060" "checksum thread_local 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)" = "c6b53e329000edc2b34dbe8545fd20e55a333362d0a321909685a19bd28c3f1b" "checksum toml 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)" = "87c5890a989fa47ecdc7bcb4c63a77a82c18f306714104b1decfd722db17b39e" "checksum ucd-util 0.1.3 (registry+https://github.com/rust-lang/crates.io-index)" = "535c204ee4d8434478593480b8f86ab45ec9aae0e83c568ca81abf0fd0e88f86" @@ -1141,6 +1208,7 @@ dependencies = [ "checksum unicode-xid 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)" = "fc72304796d0818e357ead4e000d19c9c174ab23dc11093ac919054d20a6a7fc" "checksum unicode_categories 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "39ec24b3121d976906ece63c9daad25b85969647682eee313cb5779fdd69e14e" "checksum utf8-ranges 1.0.2 (registry+https://github.com/rust-lang/crates.io-index)" = "796f7e48bef87609f7ade7e06495a87d5cd06c7866e6a5cbfceffc558a243737" +"checksum vec_map 0.8.1 (registry+https://github.com/rust-lang/crates.io-index)" = "05c78687fb1a80548ae3250346c3db86a80a7cdd77bda190189f2d0a0987c81a" "checksum walkdir 2.2.7 (registry+https://github.com/rust-lang/crates.io-index)" = "9d9d7ed3431229a144296213105a390676cc49c9b6a72bd19f3176c98e129fa1" "checksum winapi 0.3.7 (registry+https://github.com/rust-lang/crates.io-index)" = "f10e386af2b13e47c89e7236a7a14a086791a2b88ebad6df9bf42040195cf770" "checksum winapi-i686-pc-windows-gnu 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)" = "ac3b87c63620426dd9b991e5ce0329eff545bccbbb34f3be09ff6fb6ab51b7b6" diff --git a/Cargo.toml b/Cargo.toml index 4e332ec43b9..1321f0e84ef 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -58,6 +58,7 @@ unicode_categories = "0.1.1" dirs = "1.0.4" ignore = "0.4.6" annotate-snippets = { version = "0.5.0", features = ["ansi_term"] } +structopt = "0.2.15" config_proc_macro = { path = "config_proc_macro" } diff --git a/src/cargo-fmt/main.rs b/src/cargo-fmt/main.rs index 14d9c7f2423..82c8743587e 100644 --- a/src/cargo-fmt/main.rs +++ b/src/cargo-fmt/main.rs @@ -1,10 +1,8 @@ // Inspired by Paul Woolcock's cargo-fmt (https://github.com/pwoolcoc/cargo-fmt/). -#![cfg(not(test))] #![deny(warnings)] use cargo_metadata; -use getopts; use std::cmp::Ordering; use std::collections::{BTreeMap, BTreeSet}; @@ -17,7 +15,41 @@ use std::path::{Path, PathBuf}; use std::process::Command; use std::str; -use getopts::{Matches, Options}; +use structopt::StructOpt; + +#[derive(StructOpt, Debug)] +#[structopt( + bin_name = "cargo fmt", + author = "", + about = "This utility formats all bin and lib files of \ + the current crate using rustfmt." +)] +pub struct Opts { + /// No output printed to stdout + #[structopt(short = "q", long = "quiet")] + quiet: bool, + + /// Use verbose output + #[structopt(short = "v", long = "verbose")] + verbose: bool, + + /// Print rustfmt version and exit + #[structopt(long = "version")] + version: bool, + + /// Specify package to format (only usable in workspaces) + #[structopt(short = "p", long = "package", value_name = "package")] + packages: Vec, + + /// Options passed to rustfmt + // 'raw = true' to make `--` explicit. + #[structopt(name = "rustfmt_options", raw(raw = "true"))] + rustfmt_options: Vec, + + /// Format all packages (only usable in workspaces) + #[structopt(long = "all")] + format_all: bool, +} fn main() { let exit_status = execute(); @@ -29,80 +61,32 @@ const SUCCESS: i32 = 0; const FAILURE: i32 = 1; fn execute() -> i32 { - let mut opts = getopts::Options::new(); - opts.optflag("h", "help", "show this message"); - opts.optflag("q", "quiet", "no output printed to stdout"); - opts.optflag("v", "verbose", "use verbose output"); - opts.optmulti( - "p", - "package", - "specify package to format (only usable in workspaces)", - "", - ); - opts.optflag("", "version", "print rustfmt version and exit"); - opts.optflag("", "all", "format all packages (only usable in workspaces)"); + let opts = Opts::from_args(); - // If there is any invalid argument passed to `cargo fmt`, return without formatting. - let mut is_package_arg = false; - for arg in env::args().skip(2).take_while(|a| a != "--") { - if arg.starts_with('-') { - is_package_arg = arg.starts_with("--package") | arg.starts_with("-p"); - } else if !is_package_arg { - print_usage_to_stderr(&opts, &format!("Invalid argument: `{}`.", arg)); - return FAILURE; - } else { - is_package_arg = false; - } - } - - let matches = match opts.parse(env::args().skip(1).take_while(|a| a != "--")) { - Ok(m) => m, - Err(e) => { - print_usage_to_stderr(&opts, &e.to_string()); - return FAILURE; - } - }; - - let verbosity = match (matches.opt_present("v"), matches.opt_present("q")) { + let verbosity = match (opts.verbose, opts.quiet) { (false, false) => Verbosity::Normal, (false, true) => Verbosity::Quiet, (true, false) => Verbosity::Verbose, (true, true) => { - print_usage_to_stderr(&opts, "quiet mode and verbose mode are not compatible"); + print_usage_to_stderr("quiet mode and verbose mode are not compatible"); return FAILURE; } }; - if matches.opt_present("h") { - print_usage_to_stdout(&opts, ""); - return SUCCESS; + if opts.version { + return handle_command_status(get_version()); } - if matches.opt_present("version") { - return handle_command_status(get_version(), &opts); - } + let strategy = CargoFmtStrategy::from_opts(&opts); - let strategy = CargoFmtStrategy::from_matches(&matches); - handle_command_status(format_crate(verbosity, &strategy), &opts) + handle_command_status(format_crate(verbosity, &strategy, opts.rustfmt_options)) } -macro_rules! print_usage { - ($print:ident, $opts:ident, $reason:expr) => {{ - let msg = format!("{}\nusage: cargo fmt [options]", $reason); - $print!( - "{}\nThis utility formats all bin and lib files of the current crate using rustfmt. \ - Arguments after `--` are passed to rustfmt.", - $opts.usage(&msg) - ); - }}; -} - -fn print_usage_to_stdout(opts: &Options, reason: &str) { - print_usage!(println, opts, reason); -} - -fn print_usage_to_stderr(opts: &Options, reason: &str) { - print_usage!(eprintln, opts, reason); +fn print_usage_to_stderr(reason: &str) { + eprintln!("{}", reason); + let app = Opts::clap(); + app.write_help(&mut io::stderr()) + .expect("failed to write to stderr"); } #[derive(Debug, Clone, Copy, PartialEq)] @@ -112,10 +96,10 @@ pub enum Verbosity { Quiet, } -fn handle_command_status(status: Result, opts: &getopts::Options) -> i32 { +fn handle_command_status(status: Result) -> i32 { match status { Err(e) => { - print_usage_to_stderr(opts, &e.to_string()); + print_usage_to_stderr(&e.to_string()); FAILURE } Ok(status) => status, @@ -142,8 +126,11 @@ fn get_version() -> Result { } } -fn format_crate(verbosity: Verbosity, strategy: &CargoFmtStrategy) -> Result { - let rustfmt_args = get_fmt_args(); +fn format_crate( + verbosity: Verbosity, + strategy: &CargoFmtStrategy, + rustfmt_args: Vec, +) -> Result { let targets = if rustfmt_args .iter() .any(|s| ["--print-config", "-h", "--help", "-V", "--version"].contains(&s.as_str())) @@ -157,11 +144,6 @@ fn format_crate(verbosity: Verbosity, strategy: &CargoFmtStrategy) -> Result Vec { - // All arguments after -- are passed to rustfmt. - env::args().skip_while(|a| a != "--").skip(1).collect() -} - /// Target uses a `path` field for equality and hashing. #[derive(Debug)] pub struct Target { @@ -223,11 +205,11 @@ pub enum CargoFmtStrategy { } impl CargoFmtStrategy { - pub fn from_matches(matches: &Matches) -> CargoFmtStrategy { - match (matches.opt_present("all"), matches.opt_present("p")) { - (false, false) => CargoFmtStrategy::Root, + pub fn from_opts(opts: &Opts) -> CargoFmtStrategy { + match (opts.format_all, opts.packages.is_empty()) { + (false, true) => CargoFmtStrategy::Root, (true, _) => CargoFmtStrategy::All, - (false, true) => CargoFmtStrategy::Some(matches.opt_strs("p")), + (false, false) => CargoFmtStrategy::Some(opts.packages.clone()), } } } @@ -403,3 +385,133 @@ fn get_cargo_metadata(manifest_path: Option<&Path>) -> Result Err(io::Error::new(io::ErrorKind::Other, error.to_string())), } } + +#[cfg(test)] +mod cargo_fmt_tests { + use super::*; + + #[test] + fn default_options() { + let empty: Vec = vec![]; + let o = Opts::from_iter(&empty); + assert_eq!(false, o.quiet); + assert_eq!(false, o.verbose); + assert_eq!(false, o.version); + assert_eq!(empty, o.packages); + assert_eq!(empty, o.rustfmt_options); + assert_eq!(false, o.format_all); + } + + #[test] + fn good_options() { + let o = Opts::from_iter(&[ + "test", + "-q", + "-p", + "p1", + "-p", + "p2", + "--", + "--edition", + "2018", + ]); + assert_eq!(true, o.quiet); + assert_eq!(false, o.verbose); + assert_eq!(false, o.version); + assert_eq!(vec!["p1", "p2"], o.packages); + assert_eq!(vec!["--edition", "2018"], o.rustfmt_options); + assert_eq!(false, o.format_all); + } + + #[test] + fn unexpected_option() { + assert!( + Opts::clap() + .get_matches_from_safe(&["test", "unexpected"]) + .is_err() + ); + } + + #[test] + fn unexpected_flag() { + assert!( + Opts::clap() + .get_matches_from_safe(&["test", "--flag"]) + .is_err() + ); + } + + #[test] + fn mandatory_separator() { + assert!( + Opts::clap() + .get_matches_from_safe(&["test", "--check"]) + .is_err() + ); + assert!( + !Opts::clap() + .get_matches_from_safe(&["test", "--", "--check"]) + .is_err() + ); + } + + #[test] + fn multiple_packages_one_by_one() { + let o = Opts::from_iter(&[ + "test", + "-p", + "package1", + "--package", + "package2", + "-p", + "package3", + ]); + assert_eq!(3, o.packages.len()); + } + + #[test] + fn multiple_packages_grouped() { + let o = Opts::from_iter(&[ + "test", + "--package", + "package1", + "package2", + "-p", + "package3", + "package4", + ]); + assert_eq!(4, o.packages.len()); + } + + #[test] + fn empty_packages_1() { + assert!(Opts::clap().get_matches_from_safe(&["test", "-p"]).is_err()); + } + + #[test] + fn empty_packages_2() { + assert!( + Opts::clap() + .get_matches_from_safe(&["test", "-p", "--", "--check"]) + .is_err() + ); + } + + #[test] + fn empty_packages_3() { + assert!( + Opts::clap() + .get_matches_from_safe(&["test", "-p", "--verbose"]) + .is_err() + ); + } + + #[test] + fn empty_packages_4() { + assert!( + Opts::clap() + .get_matches_from_safe(&["test", "-p", "--check"]) + .is_err() + ); + } +} From 32fbe75a4cd23cfd64465d3e3419df2e79eb6828 Mon Sep 17 00:00:00 2001 From: Ivan Veselov Date: Mon, 20 May 2019 13:17:32 +0100 Subject: [PATCH 2/3] Drop extra `fmt` command line argument provided by cargo --- src/cargo-fmt/main.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/cargo-fmt/main.rs b/src/cargo-fmt/main.rs index 82c8743587e..a265347e033 100644 --- a/src/cargo-fmt/main.rs +++ b/src/cargo-fmt/main.rs @@ -61,7 +61,9 @@ const SUCCESS: i32 = 0; const FAILURE: i32 = 1; fn execute() -> i32 { - let opts = Opts::from_args(); + // Drop extra `fmt` argument provided by `cargo`. + let args = env::args().filter(|x| x != "fmt"); + let opts = Opts::from_iter(args); let verbosity = match (opts.verbose, opts.quiet) { (false, false) => Verbosity::Normal, From a7afdeb9b841b7007a7ed9d3b45834ef7953772c Mon Sep 17 00:00:00 2001 From: Ivan Veselov Date: Tue, 21 May 2019 16:33:51 +0100 Subject: [PATCH 3/3] Drop only the first occurrence of `fmt` while preparing CLI arguments --- src/cargo-fmt/main.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/cargo-fmt/main.rs b/src/cargo-fmt/main.rs index a265347e033..3c4d1c08b08 100644 --- a/src/cargo-fmt/main.rs +++ b/src/cargo-fmt/main.rs @@ -62,7 +62,16 @@ const FAILURE: i32 = 1; fn execute() -> i32 { // Drop extra `fmt` argument provided by `cargo`. - let args = env::args().filter(|x| x != "fmt"); + let mut found_fmt = false; + let args = env::args().filter(|x| { + if found_fmt { + true + } else { + found_fmt = x == "fmt"; + x != "fmt" + } + }); + let opts = Opts::from_iter(args); let verbosity = match (opts.verbose, opts.quiet) {