From fe63dc3715da756036fe993348e6c0d83d8cfef6 Mon Sep 17 00:00:00 2001
From: Ralf Jung <post@ralfj.de>
Date: Sat, 13 May 2023 19:02:11 +0200
Subject: [PATCH] cargo-miri: fix forwarding arguments to cargo

---
 src/tools/miri/cargo-miri/src/arg.rs       |  8 ++--
 src/tools/miri/cargo-miri/src/phases.rs    | 52 +++++++++++-----------
 src/tools/miri/test-cargo-miri/run-test.py |  3 +-
 3 files changed, 33 insertions(+), 30 deletions(-)

diff --git a/src/tools/miri/cargo-miri/src/arg.rs b/src/tools/miri/cargo-miri/src/arg.rs
index e8bac4625f710..d7216060358cb 100644
--- a/src/tools/miri/cargo-miri/src/arg.rs
+++ b/src/tools/miri/cargo-miri/src/arg.rs
@@ -40,7 +40,8 @@ impl<'s, I: Iterator<Item = Cow<'s, str>>> Iterator for ArgSplitFlagValue<'_, I>
         if arg == "--" {
             // Stop searching at `--`.
             self.args = None;
-            return None;
+            // But yield the `--` so that it does not get lost!
+            return Some(Err(Cow::Borrowed("--")));
         }
         // These branches cannot be merged if we want to avoid the allocation in the `Borrowed` branch.
         match &arg {
@@ -79,9 +80,8 @@ impl<'a, I: Iterator<Item = String> + 'a> ArgSplitFlagValue<'a, I> {
     ) -> impl Iterator<Item = Result<String, String>> + 'a {
         ArgSplitFlagValue::new(args.map(Cow::Owned), name).map(|x| {
             match x {
-                Ok(Cow::Owned(s)) => Ok(s),
-                Err(Cow::Owned(s)) => Err(s),
-                _ => panic!("iterator converted owned to borrowed"),
+                Ok(s) => Ok(s.into_owned()),
+                Err(s) => Err(s.into_owned()),
             }
         })
     }
diff --git a/src/tools/miri/cargo-miri/src/phases.rs b/src/tools/miri/cargo-miri/src/phases.rs
index 37f66d0033f06..465e4a1b2d2b2 100644
--- a/src/tools/miri/cargo-miri/src/phases.rs
+++ b/src/tools/miri/cargo-miri/src/phases.rs
@@ -113,30 +113,17 @@ pub fn phase_cargo_miri(mut args: impl Iterator<Item = String>) {
     };
     let metadata = get_cargo_metadata();
     let mut cmd = cargo();
-    cmd.arg(cargo_cmd);
-
-    // Forward all arguments before `--` other than `--target-dir` and its value to Cargo.
-    // (We want to *change* the target-dir value, so we must not forward it.)
-    let mut target_dir = None;
-    for arg in ArgSplitFlagValue::from_string_iter(&mut args, "--target-dir") {
-        match arg {
-            Ok(value) => {
-                if target_dir.is_some() {
-                    show_error!("`--target-dir` is provided more than once");
-                }
-                target_dir = Some(value.into());
-            }
-            Err(arg) => {
-                cmd.arg(arg);
-            }
-        }
+    cmd.arg(&cargo_cmd);
+    // In nextest we have to also forward the main `verb`.
+    if cargo_cmd == "nextest" {
+        cmd.arg(
+            args.next()
+                .unwrap_or_else(|| show_error!("`cargo miri nextest` expects a verb (e.g. `run`)")),
+        );
     }
-    // Detect the target directory if it's not specified via `--target-dir`.
-    // (`cargo metadata` does not support `--target-dir`, that's why we have to handle this ourselves.)
-    let target_dir = target_dir.get_or_insert_with(|| metadata.target_directory.clone());
-    // Set `--target-dir` to `miri` inside the original target directory.
-    target_dir.push("miri");
-    cmd.arg("--target-dir").arg(target_dir);
+    // We set the following flags *before* forwarding more arguments.
+    // This is needed to fix <https://github.com/rust-lang/miri/issues/2829>: cargo will stop
+    // interpreting things as flags when it sees the first positional argument.
 
     // Make sure the build target is explicitly set.
     // This is needed to make the `target.runner` settings do something,
@@ -154,8 +141,23 @@ pub fn phase_cargo_miri(mut args: impl Iterator<Item = String>) {
     cmd.arg("--config")
         .arg(format!("target.'cfg(all())'.runner=[{cargo_miri_path_for_toml}, 'runner']"));
 
-    // Forward all further arguments after `--` to cargo.
-    cmd.arg("--").args(args);
+    // Set `--target-dir` to `miri` inside the original target directory.
+    let mut target_dir = match get_arg_flag_value("--target-dir") {
+        Some(dir) => PathBuf::from(dir),
+        None => metadata.target_directory.clone().into_std_path_buf(),
+    };
+    target_dir.push("miri");
+    cmd.arg("--target-dir").arg(target_dir);
+
+    // *After* we set all the flags that need setting, forward everything else. Make sure to skip
+    // `--target-dir` (which would otherwise be set twice).
+    for arg in
+        ArgSplitFlagValue::from_string_iter(&mut args, "--target-dir").filter_map(Result::err)
+    {
+        cmd.arg(arg);
+    }
+    // Forward all further arguments (not consumed by `ArgSplitFlagValue`) to cargo.
+    cmd.args(args);
 
     // Set `RUSTC_WRAPPER` to ourselves.  Cargo will prepend that binary to its usual invocation,
     // i.e., the first argument is `rustc` -- which is what we use in `main` to distinguish
diff --git a/src/tools/miri/test-cargo-miri/run-test.py b/src/tools/miri/test-cargo-miri/run-test.py
index 46b3afa70e5b0..9df90c725e40f 100755
--- a/src/tools/miri/test-cargo-miri/run-test.py
+++ b/src/tools/miri/test-cargo-miri/run-test.py
@@ -108,8 +108,9 @@ def test_cargo_miri_run():
         env={'MIRITESTVAR': "wrongval"}, # changing the env var causes a rebuild (re-runs build.rs),
                                          # so keep it set
     )
+    # This also covers passing arguments without `--`: Cargo will forward unused positional arguments to the program.
     test("`cargo miri run` (with arguments and target)",
-        cargo_miri("run") + ["--bin", "cargo-miri-test", "--", "hello world", '"hello world"', r'he\\llo\"world'],
+        cargo_miri("run") + ["--bin", "cargo-miri-test", "hello world", '"hello world"', r'he\\llo\"world'],
         "run.args.stdout.ref", "run.args.stderr.ref",
     )
     test("`cargo miri r` (subcrate, no isolation)",