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.

diff --git a/src/bin/cli.rs b/src/bin/cli.rs
index ae96579bb2c8bf0014e00dca8251bab40969af0d..a6ca961af58721eb3545efcefb5ae29538f47744 100644 (file)
--- a/src/bin/cli.rs
+++ b/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());

diff --git a/src/bin/command_prelude.rs b/src/bin/command_prelude.rs
index 450abd7a6bc17856729ab91e3f99f8694b166840..b93d16dc26ce34dcc76e8e60aa357e797cef248c 100644 (file)
--- a/src/bin/command_prelude.rs
+++ b/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,

diff --git a/src/bin/commands/bench.rs b/src/bin/commands/bench.rs
index b9ee85d539c075af471156da1398f951792a5e18..01676b51500f622c6d8a54d5a5477c6ae07df676 100644 (file)
--- a/src/bin/commands/bench.rs
+++ b/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",

diff --git a/src/bin/commands/build.rs b/src/bin/commands/build.rs
index 5b0630653324080c05f56f851b0b1e526d53fb85..2a772e03da23ced9731d0293847ee13b57775840 100644 (file)
--- a/src/bin/commands/build.rs
+++ b/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",

diff --git a/src/bin/commands/check.rs b/src/bin/commands/check.rs
index a6ce5d7b2cec315aa6960b3bb3ce143540ff8bea..45361740d6182e46c92a9d593682f0f2d6a9c41d 100644 (file)
--- a/src/bin/commands/check.rs
+++ b/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",

diff --git a/src/bin/commands/clean.rs b/src/bin/commands/clean.rs
index 908a0c1ee64a77a2647bf23ec5cc5f3b73349d68..8d0928cc2b2091e2009c7e892741e6a750b82f7b 100644 (file)
--- a/src/bin/commands/clean.rs
+++ b/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")

diff --git a/src/bin/commands/doc.rs b/src/bin/commands/doc.rs
index 0a503a49b04afbb9b566f0aa7a7c53f00499a652..a6a2546750ff9afc1f2422123077823607b7d60c 100644 (file)
--- a/src/bin/commands/doc.rs
+++ b/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",

diff --git a/src/bin/commands/owner.rs b/src/bin/commands/owner.rs
index 267da4c8fa6e13c8d01cf066670363cb53ed5809..f20be31b191e54352732bce273967b5fe3906475 100644 (file)
--- a/src/bin/commands/owner.rs
+++ b/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"))

diff --git a/src/bin/commands/pkgid.rs b/src/bin/commands/pkgid.rs
index 4653f898d09956fbf619ec60f2228e1299c8caf8..7010092d60e9ad27539a3b7974c516580260f873 100644 (file)
--- a/src/bin/commands/pkgid.rs
+++ b/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(
             "\

diff --git a/src/bin/commands/run.rs b/src/bin/commands/run.rs
index 4f618f430401b7971e803c7da872856a831cc658..77e5d8e13b9ef22e0aeea0067d312ed327771684 100644 (file)
--- a/src/bin/commands/run.rs
+++ b/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()

diff --git a/src/bin/commands/rustc.rs b/src/bin/commands/rustc.rs
index 268b835a63073b91c08c8ebb0dee7e41770c8baa..f0c32e697568cf9d80a1b00221d9df2e97a3cdce 100644 (file)
--- a/src/bin/commands/rustc.rs
+++ b/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",

diff --git a/src/bin/commands/rustdoc.rs b/src/bin/commands/rustdoc.rs
index f3744cebf1662c9773765111a203dcc386215093..6f039c8051bc666d6674c9d820725a8576f29f18 100644 (file)
--- a/src/bin/commands/rustdoc.rs
+++ b/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",

diff --git a/src/bin/commands/test.rs b/src/bin/commands/test.rs
index e0f8e0014eedb2aeefb270930c617976be9562d9..93db6dccd953ba80446412c8efc001f7fade8810 100644 (file)
--- a/src/bin/commands/test.rs
+++ b/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",

diff --git a/src/bin/commands/uninstall.rs b/src/bin/commands/uninstall.rs
index 3ed9ec5160589cd8eeb4f81993dcae18509a7ceb..2031851195b6d5ee67a7627ec795c765fd57b19d 100644 (file)
--- a/src/bin/commands/uninstall.rs
+++ b/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(
             "\

diff --git a/src/bin/commands/update.rs b/src/bin/commands/update.rs
index 8b93c368266344f8f5ae29970a2f1bb1935b09ca..c5a992a3da17f0cb64d105e9b1e1ef56e88da49d 100644 (file)
--- a/src/bin/commands/update.rs
+++ b/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",

diff --git a/tests/testsuite/run.rs b/tests/testsuite/run.rs
index 2c298aaef0003e1531441d5000e6d0c5f558a345..c691891052465cef8a40ce7421b006df71982817 100644 (file)
--- a/tests/testsuite/run.rs
+++ b/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),
+    );
+}