Auto merge of #5196 - matklad:clapclapclap, r=alexcrichton

Fix a regression with parsing multivalue options

By default, clap interprets

```
cargo run --bin foo bar baz
```

as

```
cargo run --bin foo --bin bar --bin baz
```

This behavior is different from docopt and does not play nicely with
positional arguments at all. Luckily, clap has a flag to get the
behavior we want, it just not the default! It will become the default in
the next version of clap, but, until that time, we should be careful
when using the combination of `.long`, `.value_name` and
`.multiple(true)`, and don't forget to specify `.number_of_values(1)` as
well.

@alexcrichton I'd love to merge this fix before updating cargo at rust-lang/rust :)

Author: bors

src/bin/cli.rs

@@ -165,6 +165,7 @@ See 'cargo help <command>' for more information on a specific command.",
                 .short("Z")
                 .value_name("FLAG")
                 .multiple(true)
+                .number_of_values(1)
                 .global(true),
         )
         .subcommands(commands::builtin());

src/bin/command_prelude.rs

@@ -15,17 +15,22 @@ pub type App = clap::App<'static, 'static>;
 pub trait AppExt: Sized {
     fn _arg(self, arg: Arg<'static, 'static>) -> Self;
 
-    fn arg_package(self, package: &'static str, all: &'static str, exclude: &'static str) -> Self {
-        self._arg(
-            opt("package", package)
-                .short("p")
-                .value_name("SPEC")
-                .multiple(true),
-        )._arg(opt("all", all))
-            ._arg(opt("exclude", exclude).value_name("SPEC").multiple(true))
+    fn arg_package_spec(
+        self,
+        package: &'static str,
+        all: &'static str,
+        exclude: &'static str,
+    ) -> Self {
+        self.arg_package_spec_simple(package)
+            ._arg(opt("all", all))
+            ._arg(multi_opt("exclude", "SPEC", exclude))
     }
 
-    fn arg_single_package(self, package: &'static str) -> Self {
+    fn arg_package_spec_simple(self, package: &'static str) -> Self {
+        self._arg(multi_opt("package", "SPEC", package).short("p"))
+    }
+
+    fn arg_package(self, package: &'static str) -> Self {
         self._arg(opt("package", package).short("p").value_name("SPEC"))
     }
 
@@ -51,18 +56,18 @@ pub trait AppExt: Sized {
         all: &'static str,
     ) -> Self {
         self.arg_targets_lib_bin(lib, bin, bins)
-            ._arg(opt("example", examle).value_name("NAME").multiple(true))
+            ._arg(multi_opt("example", "NAME", examle))
             ._arg(opt("examples", examles))
-            ._arg(opt("test", test).value_name("NAME").multiple(true))
+            ._arg(multi_opt("test", "NAME", test))
             ._arg(opt("tests", tests))
-            ._arg(opt("bench", bench).value_name("NAME").multiple(true))
+            ._arg(multi_opt("bench", "NAME", bench))
             ._arg(opt("benches", benchs))
             ._arg(opt("all-targets", all))
     }
 
     fn arg_targets_lib_bin(self, lib: &'static str, bin: &'static str, bins: &'static str) -> Self {
         self._arg(opt("lib", lib))
-            ._arg(opt("bin", bin).value_name("NAME").multiple(true))
+            ._arg(multi_opt("bin", "NAME", bin))
             ._arg(opt("bins", bins))
     }
 
@@ -73,15 +78,15 @@ pub trait AppExt: Sized {
         example: &'static str,
         examples: &'static str,
     ) -> Self {
-        self._arg(opt("bin", bin).value_name("NAME").multiple(true))
+        self._arg(multi_opt("bin", "NAME", bin))
             ._arg(opt("bins", bins))
-            ._arg(opt("example", example).value_name("NAME").multiple(true))
+            ._arg(multi_opt("example", "NAME", example))
             ._arg(opt("examples", examples))
     }
 
     fn arg_targets_bin_example(self, bin: &'static str, example: &'static str) -> Self {
-        self._arg(opt("bin", bin).value_name("NAME").multiple(true))
-            ._arg(opt("example", example).value_name("NAME").multiple(true))
+        self._arg(multi_opt("bin", "NAME", bin))
+            ._arg(multi_opt("example", "NAME", example))
     }
 
     fn arg_features(self) -> Self {
@@ -157,6 +162,21 @@ pub fn opt(name: &'static str, help: &'static str) -> Arg<'static, 'static> {
     Arg::with_name(name).long(name).help(help)
 }
 
+pub fn multi_opt(
+    name: &'static str,
+    value_name: &'static str,
+    help: &'static str,
+) -> Arg<'static, 'static> {
+    // Note that all `.multiple(true)` arguments in Cargo should specify
+    // `.number_of_values(1)` as well, so that `--foo val1 val2` is
+    // **not** parsed as `foo` with values ["val1", "val2"].
+    // `number_of_values` should become the default in clap 3.
+    opt(name, help)
+        .value_name(value_name)
+        .multiple(true)
+        .number_of_values(1)
+}
+
 pub fn subcommand(name: &'static str) -> App {
     SubCommand::with_name(name).settings(&[
         AppSettings::UnifiedHelpMessage,

src/bin/commands/bench.rs

@@ -29,7 +29,7 @@ pub fn cli() -> App {
             "Benchmark all targets (default)",
         )
         .arg(opt("no-run", "Compile, but don't run benchmarks"))
-        .arg_package(
+        .arg_package_spec(
             "Package to run benchmarks for",
             "Benchmark all packages in the workspace",
             "Exclude packages from the benchmark",

src/bin/commands/build.rs

@@ -6,7 +6,7 @@ pub fn cli() -> App {
     subcommand("build")
         .alias("b")
         .about("Compile a local package and all of its dependencies")
-        .arg_package(
+        .arg_package_spec(
             "Package to build",
             "Build all packages in the workspace",
             "Exclude packages from the build",

src/bin/commands/check.rs

@@ -5,7 +5,7 @@ use cargo::ops::{self, CompileMode};
 pub fn cli() -> App {
     subcommand("check")
         .about("Check a local package and all of its dependencies for errors")
-        .arg_package(
+        .arg_package_spec(
             "Package(s) to check",
             "Check all packages in the workspace",
             "Exclude packages from the check",

src/bin/commands/clean.rs

@@ -5,12 +5,7 @@ use cargo::ops::{self, CleanOptions};
 pub fn cli() -> App {
     subcommand("clean")
         .about("Remove artifacts that cargo has generated in the past")
-        .arg(
-            opt("package", "Package to clean artifacts for")
-                .short("p")
-                .value_name("SPEC")
-                .multiple(true),
-        )
+        .arg_package_spec_simple("Package to clean artifacts for")
         .arg_manifest_path()
         .arg_target_triple("Target triple to clean output for (default all)")
         .arg_release("Whether or not to clean release artifacts")

src/bin/commands/doc.rs

@@ -9,7 +9,7 @@ pub fn cli() -> App {
             "open",
             "Opens the docs in a browser after the operation",
         ))
-        .arg_package(
+        .arg_package_spec(
             "Package to document",
             "Document all packages in the workspace",
             "Exclude packages from the build",

src/bin/commands/owner.rs

@@ -6,17 +6,13 @@ pub fn cli() -> App {
     subcommand("owner")
         .about("Manage the owners of a crate on the registry")
         .arg(Arg::with_name("crate"))
+        .arg(multi_opt("add", "LOGIN", "Name of a user or team to add as an owner").short("a"))
         .arg(
-            opt("add", "Name of a user or team to add as an owner")
-                .short("a")
-                .value_name("LOGIN")
-                .multiple(true),
-        )
-        .arg(
-            opt("remove", "Name of a user or team to remove as an owner")
-                .short("r")
-                .value_name("LOGIN")
-                .multiple(true),
+            multi_opt(
+                "remove",
+                "LOGIN",
+                "Name of a user or team to remove as an owner",
+            ).short("r"),
         )
         .arg(opt("list", "List owners of a crate").short("l"))
         .arg(opt("index", "Registry index to modify owners for").value_name("INDEX"))

src/bin/commands/pkgid.rs

@@ -6,7 +6,7 @@ pub fn cli() -> App {
     subcommand("pkgid")
         .about("Print a fully qualified package specification")
         .arg(Arg::with_name("spec"))
-        .arg_single_package("Argument to get the package id specifier for")
+        .arg_package("Argument to get the package id specifier for")
         .arg_manifest_path()
         .after_help(
             "\

src/bin/commands/run.rs

@@ -13,7 +13,7 @@ pub fn cli() -> App {
             "Name of the bin target to run",
             "Name of the example target to run",
         )
-        .arg_single_package("Package with the target to run")
+        .arg_package("Package with the target to run")
         .arg_jobs()
         .arg_release("Build artifacts in release mode, with optimizations")
         .arg_features()

src/bin/commands/rustc.rs

@@ -7,7 +7,7 @@ pub fn cli() -> App {
         .setting(AppSettings::TrailingVarArg)
         .about("Compile a package and all of its dependencies")
         .arg(Arg::with_name("args").multiple(true))
-        .arg_single_package("Package to build")
+        .arg_package("Package to build")
         .arg_jobs()
         .arg_targets_all(
             "Build only this package's library",

src/bin/commands/rustdoc.rs

@@ -11,7 +11,7 @@ pub fn cli() -> App {
             "open",
             "Opens the docs in a browser after the operation",
         ))
-        .arg_single_package("Package to document")
+        .arg_package("Package to document")
         .arg_jobs()
         .arg_targets_all(
             "Build only this package's library",

src/bin/commands/test.rs

@@ -32,7 +32,7 @@ pub fn cli() -> App {
         .arg(opt("doc", "Test only this library's documentation"))
         .arg(opt("no-run", "Compile, but don't run tests"))
         .arg(opt("no-fail-fast", "Run all tests regardless of failure"))
-        .arg_package(
+        .arg_package_spec(
             "Package to run tests for",
             "Test all packages in the workspace",
             "Exclude packages from the test",

src/bin/commands/uninstall.rs

@@ -6,11 +6,7 @@ pub fn cli() -> App {
     subcommand("uninstall")
         .about("Remove a Rust binary")
         .arg(Arg::with_name("spec").multiple(true))
-        .arg(
-            opt("bin", "Only uninstall the binary NAME")
-                .value_name("NAME")
-                .multiple(true),
-        )
+        .arg(multi_opt("bin", "NAME", "Only uninstall the binary NAME"))
         .arg(opt("root", "Directory to uninstall packages from").value_name("DIR"))
         .after_help(
             "\

src/bin/commands/update.rs

@@ -5,12 +5,7 @@ use cargo::ops::{self, UpdateOptions};
 pub fn cli() -> App {
     subcommand("update")
         .about("Update dependencies as recorded in the local lock file")
-        .arg(
-            opt("package", "Package to clean artifacts for")
-                .short("p")
-                .value_name("SPEC")
-                .multiple(true),
-        )
+        .arg_package_spec_simple("Package to update")
         .arg(opt(
             "aggressive",
             "Force updating all dependencies of <name> as well",

tests/testsuite/run.rs

@@ -1109,3 +1109,36 @@ fn run_multiple_packages() {
             .with_stderr_contains("[ERROR] package `d3` is not a member of the workspace"),
     );
 }
+
+#[test]
+fn explicit_bin_with_args() {
+    let p = project("foo")
+        .file(
+            "Cargo.toml",
+            r#"
+            [project]
+            name = "foo"
+            version = "0.0.1"
+            authors = []
+        "#,
+        )
+        .file(
+            "src/main.rs",
+            r#"
+            fn main() {
+                assert_eq!(std::env::args().nth(1).unwrap(), "hello");
+                assert_eq!(std::env::args().nth(2).unwrap(), "world");
+            }
+        "#,
+        )
+        .build();
+
+    assert_that(
+        p.cargo("run")
+            .arg("--bin")
+            .arg("foo")
+            .arg("hello")
+            .arg("world"),
+        execs().with_status(0),
+    );
+}