Use structopt for command line arguments parsing in cargo-fmt

[sphynx]

This is part of work on #3310

I've started with only porting cargo-fmt to structopt (since it was causing problems with arguments parsing on Windows). But if this pull request is fine, I can proceed with porting the rest of binaries to structopt.

I would appreciate if someone with Windows machine can check whether it fixes Windows command line issues (e.g. #2694 and others).

[sphynx]

Hm, it looks like it suffers from the same problem as the previous approach. When the program is called with cargo fmt (as opposed to just calling cargo-fmt executable directly), cargo passes the whole command line (including fmt as the first argument) to structopt, which does not expect fmt as an argument.

In order to fix that I can see two options, both are not great:

1. manipulate env::args() directly (dropping the first fmt argument), it's similar to what it is done now and what causes issues on Windows.
2. add optional and hidden argument fmt to structopt arguments description. I'm not 100% it's possible, because it would mean mixing positional arguments and flags. Also it's somewhat "hacky".

[sphynx]

Hm, another option is to treat fmt as an optional subcommand, let me investigate that.

[sphynx]

Another option is just filter out the extra fmt argument passed by cargo, i.e. something like:

    let args = env::args().filter(|x| x != "fmt");
    let opts = Opts::from_iter(args);

It looks like going with an optional subcommand is not a good option, because it requires too much boilerplate just to workaround that extra fmt argument passed by cargo.

I am not quite sure why cargo behaves like that, i.e. if it's invoked with cargo cmd and finds cargo-cmd executable in PATH, then what is the point of passing that cmd argument again to the executable in question? We've already established the mapping between cargo-cmd and cargo cmd... Probably there are some good reasons for that, but I can't see them now.

For now I am choosing the filtering out option (even though it would prevent other cases for options fmt, e.g. if you want to format a file named fmt).

@scampi @topecongiro Please let me know your thoughts on that. Thank you!

[topecongiro]

@sphynx Thank you for your work! I haven't yet used structopt nor clap to implement cargo subcommand, so I don't have any useful advice off the top of my head, sorry. However, skimming through the structopt repo I have found an issue [1] that seems relevant. Would you mind reading it if you haven't?

[1] TeXitoi/structopt#58

[sphynx]

@topecongiro Thank you, I haven't seen this particular issue, but I've read it now. However, I've seen some other examples, e.g. in cargo script, which use clap subcommands to implement cargo subcommands. It's all doable, but I think it slightly complicates the code.

Here is a version with a mandatory subcommand fmt:

use structopt::clap::AppSettings;
use structopt::StructOpt;

const ABOUT: &'static str = "This utility formats all bin and lib files of \
                             the current crate using rustfmt.";

#[derive(StructOpt, Debug)]
#[structopt(
    bin_name = "cargo",
    author = "",
    raw(about = "ABOUT"),
    raw(setting = "AppSettings::DisableHelpSubcommand")
)]
pub enum CargoOpts {
    #[structopt(name = "fmt", author = "", raw(about = "ABOUT"))]
    FmtOpts(Opts),
}

#[derive(StructOpt, Debug)]
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<String>,

    /// Options passed to rustfmt
    // 'raw = true' to make `--` explicit.
    #[structopt(name = "rustfmt_options", raw(raw = "true"))]
    rustfmt_options: Vec<String>,

    /// Format all packages (only usable in workspaces)
    #[structopt(long = "all")]
    format_all: bool,
}

And then I use it as follows in execute function:

    let CargoOpts::FmtOpts(opts) = CargoOpts::from_args();

The rest of the code stays the same. Also note that I'm not filtering out "fmt" from env::args().

This approach has a disadvantage that one has to pass fmt option even if invoking cargo-fmt directly, i.e. one needs something like cargo-fmt fmt --help.

Also, it feels a little bit like a hack: we need to pretend that we are cargo and fmt is our subcommand, even though we only support that single subcommand in cargo-fmt.

[topecongiro]

@sphynx Thank you so much for quickly testing it out! Given the pros and cons, I think the current implementation (the one using the filter) seems better. For the implementation that uses subcommand, I am also worried that the help message gets kind of corrupted.

even though it would prevent other cases for options fmt, e.g. if you want to format a file named fmt

We can prevent this with a small hack:

    let mut found_fmt = false;
    let args = env::args().filter(|x| {
        if found_fmt {
            true
        } else {
            found_fmt = x == "fmt";
            x != "fmt"
        }
    });

[sphynx]

@topecongiro Totally agree with you. I've added your code with found_fmt workaround to my PR.

[sphynx]

@topecongiro Is there anything else I need to do with this PR?

[topecongiro]

This PR is ready to be merged. Also I appreciate bunch of tests added in this PR, it eased my reviewing process.

[scampi]

LGTM!

[scampi]

nice set of tests!