Auto merge of rust-lang#2474 - RalfJung:cargo-metadata-args, r=RalfJung

bors · bors · commit a52244252187 · 2022-08-07T02:49:33.000Z
also forward --manifest-path to 'cargo metadata'

and then I went on to refactor the argument flag splitting thing a bit, and, uh, that snowballed...
diff --git a/cargo-miri/src/arg.rs b/cargo-miri/src/arg.rs
@@ -0,0 +1,134 @@
+//! Utilities for dealing with argument flags
+
+use std::borrow::Cow;
+use std::env;
+
+/// Determines whether a `--flag` is present.
+pub fn has_arg_flag(name: &str) -> bool {
+    num_arg_flag(name) > 0
+}
+
+/// Determines how many times a `--flag` is present.
+pub fn num_arg_flag(name: &str) -> usize {
+    env::args().take_while(|val| val != "--").filter(|val| val == name).count()
+}
+
+/// Yields all values of command line flag `name` as `Ok(arg)`, and all other arguments except
+/// the flag as `Err(arg)`. (The flag `name` itself is not yielded at all, only its values are.)
+pub struct ArgSplitFlagValue<'a, I> {
+    args: Option<I>,
+    name: &'a str,
+}
+
+impl<'a, I: Iterator> ArgSplitFlagValue<'a, I> {
+    fn new(args: I, name: &'a str) -> Self {
+        Self { args: Some(args), name }
+    }
+}
+
+impl<'s, I: Iterator<Item = Cow<'s, str>>> Iterator for ArgSplitFlagValue<'_, I> {
+    // If the original iterator was all `Owned`, then we will only ever yield `Owned`
+    // (so `into_owned()` is cheap).
+    type Item = Result<Cow<'s, str>, Cow<'s, str>>;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        let Some(args) = self.args.as_mut() else {
+            // We already canceled this iterator.
+            return None;
+        };
+        let arg = args.next()?;
+        if arg == "--" {
+            // Stop searching at `--`.
+            self.args = None;
+            return None;
+        }
+        // These branches cannot be merged if we want to avoid the allocation in the `Borrowed` branch.
+        match &arg {
+            Cow::Borrowed(arg) =>
+                if let Some(suffix) = arg.strip_prefix(self.name) {
+                    // Strip leading `name`.
+                    if suffix.is_empty() {
+                        // This argument is exactly `name`; the next one is the value.
+                        return args.next().map(Ok);
+                    } else if let Some(suffix) = suffix.strip_prefix('=') {
+                        // This argument is `name=value`; get the value.
+                        return Some(Ok(Cow::Borrowed(suffix)));
+                    }
+                },
+            Cow::Owned(arg) =>
+                if let Some(suffix) = arg.strip_prefix(self.name) {
+                    // Strip leading `name`.
+                    if suffix.is_empty() {
+                        // This argument is exactly `name`; the next one is the value.
+                        return args.next().map(Ok);
+                    } else if let Some(suffix) = suffix.strip_prefix('=') {
+                        // This argument is `name=value`; get the value. We need to do an allocation
+                        // here as a `String` cannot be subsliced (what would the lifetime be?).
+                        return Some(Ok(Cow::Owned(suffix.to_owned())));
+                    }
+                },
+        }
+        Some(Err(arg))
+    }
+}
+
+impl<'a, I: Iterator<Item = String> + 'a> ArgSplitFlagValue<'a, I> {
+    pub fn from_string_iter(
+        args: I,
+        name: &'a str,
+    ) -> 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"),
+            }
+        })
+    }
+}
+
+impl<'x: 'a, 'a, I: Iterator<Item = &'x str> + 'a> ArgSplitFlagValue<'a, I> {
+    pub fn from_str_iter(
+        args: I,
+        name: &'a str,
+    ) -> impl Iterator<Item = Result<&'x str, &'x str>> + 'a {
+        ArgSplitFlagValue::new(args.map(Cow::Borrowed), name).map(|x| {
+            match x {
+                Ok(Cow::Borrowed(s)) => Ok(s),
+                Err(Cow::Borrowed(s)) => Err(s),
+                _ => panic!("iterator converted borrowed to owned"),
+            }
+        })
+    }
+}
+
+/// Yields all values of command line flag `name`.
+pub struct ArgFlagValueIter;
+
+impl ArgFlagValueIter {
+    pub fn from_string_iter<'a, I: Iterator<Item = String> + 'a>(
+        args: I,
+        name: &'a str,
+    ) -> impl Iterator<Item = String> + 'a {
+        ArgSplitFlagValue::from_string_iter(args, name).filter_map(Result::ok)
+    }
+}
+
+impl ArgFlagValueIter {
+    pub fn from_str_iter<'x: 'a, 'a, I: Iterator<Item = &'x str> + 'a>(
+        args: I,
+        name: &'a str,
+    ) -> impl Iterator<Item = &'x str> + 'a {
+        ArgSplitFlagValue::from_str_iter(args, name).filter_map(Result::ok)
+    }
+}
+
+/// Gets the values of a `--flag`.
+pub fn get_arg_flag_values(name: &str) -> impl Iterator<Item = String> + '_ {
+    ArgFlagValueIter::from_string_iter(env::args(), name)
+}
+
+/// Gets the value of a `--flag`.
+pub fn get_arg_flag_value(name: &str) -> Option<String> {
+    get_arg_flag_values(name).next()
+}
diff --git a/cargo-miri/src/main.rs b/cargo-miri/src/main.rs
@@ -1,6 +1,7 @@
 #![feature(let_else)]
 #![allow(clippy::useless_format, clippy::derive_partial_eq_without_eq)]
 
+mod arg;
 mod phases;
 mod setup;
 mod util;
diff --git a/cargo-miri/src/phases.rs b/cargo-miri/src/phases.rs
@@ -117,8 +117,9 @@ pub fn phase_cargo_miri(mut args: impl Iterator<Item = String>) {
     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::new(&mut args, "--target-dir") {
+    for arg in ArgSplitFlagValue::from_string_iter(&mut args, "--target-dir") {
         match arg {
             Ok(value) => {
                 if target_dir.is_some() {
@@ -309,17 +310,18 @@ pub fn phase_rustc(mut args: impl Iterator<Item = String>, phase: RustcPhase) {
             let mut cmd = miri();
 
             // Ensure --emit argument for a check-only build is present.
-            // We cannot use the usual helpers since we need to check specifically in `env.args`.
-            if let Some(i) = env.args.iter().position(|arg| arg.starts_with("--emit=")) {
+            if let Some(val) =
+                ArgFlagValueIter::from_str_iter(env.args.iter().map(|s| s as &str), "--emit").next()
+            {
                 // For `no_run` tests, rustdoc passes a `--emit` flag; make sure it has the right shape.
-                assert_eq!(env.args[i], "--emit=metadata");
+                assert_eq!(val, "metadata");
             } else {
                 // For all other kinds of tests, we can just add our flag.
                 cmd.arg("--emit=metadata");
             }
 
             // Alter the `-o` parameter so that it does not overwrite the JSON file we stored above.
-            let mut args = env.args.clone();
+            let mut args = env.args;
             for i in 0..args.len() {
                 if args[i] == "-o" {
                     args[i + 1].push_str(".miri");
@@ -343,7 +345,7 @@ pub fn phase_rustc(mut args: impl Iterator<Item = String>, phase: RustcPhase) {
         return;
     }
 
-    if runnable_crate && ArgFlagValueIter::new("--extern").any(|krate| krate == "proc_macro") {
+    if runnable_crate && get_arg_flag_values("--extern").any(|krate| krate == "proc_macro") {
         // This is a "runnable" `proc-macro` crate (unit tests). We do not support
         // interpreting that under Miri now, so we write a JSON file to (display a
         // helpful message and) skip it in the runner phase.
@@ -567,7 +569,7 @@ pub fn phase_rustdoc(mut args: impl Iterator<Item = String>) {
 
     // Doctests of `proc-macro` crates (and their dependencies) are always built for the host,
     // so we are not able to run them in Miri.
-    if ArgFlagValueIter::new("--crate-type").any(|crate_type| crate_type == "proc-macro") {
+    if get_arg_flag_values("--crate-type").any(|crate_type| crate_type == "proc-macro") {
         eprintln!("Running doctests of `proc-macro` crates is not currently supported by Miri.");
         return;
     }
diff --git a/cargo-miri/src/util.rs b/cargo-miri/src/util.rs
@@ -4,7 +4,6 @@ use std::ffi::OsString;
 use std::fmt::Write as _;
 use std::fs::{self, File};
 use std::io::{self, BufWriter, Read, Write};
-use std::iter::TakeWhile;
 use std::ops::Not;
 use std::path::{Path, PathBuf};
 use std::process::Command;
@@ -13,6 +12,8 @@ use cargo_metadata::{Metadata, MetadataCommand};
 use rustc_version::VersionMeta;
 use serde::{Deserialize, Serialize};
 
+pub use crate::arg::*;
+
 /// The information to run a crate with the given environment.
 #[derive(Clone, Serialize, Deserialize)]
 pub struct CrateRunEnv {
@@ -74,78 +75,6 @@ pub fn show_error(msg: String) -> ! {
     std::process::exit(1)
 }
 
-/// Determines whether a `--flag` is present.
-pub fn has_arg_flag(name: &str) -> bool {
-    num_arg_flag(name) > 0
-}
-
-/// Determines how many times a `--flag` is present.
-pub fn num_arg_flag(name: &str) -> usize {
-    std::env::args().take_while(|val| val != "--").filter(|val| val == name).count()
-}
-
-/// Yields all values of command line flag `name` as `Ok(arg)`, and all other arguments except
-/// the flag as `Err(arg)`. (The flag `name` itself is not yielded at all, only its values are.)
-pub struct ArgSplitFlagValue<'a, I> {
-    args: TakeWhile<I, fn(&String) -> bool>,
-    name: &'a str,
-}
-
-impl<'a, I: Iterator<Item = String>> ArgSplitFlagValue<'a, I> {
-    pub fn new(args: I, name: &'a str) -> Self {
-        Self {
-            // Stop searching at `--`.
-            args: args.take_while(|val| val != "--"),
-            name,
-        }
-    }
-}
-
-impl<I: Iterator<Item = String>> Iterator for ArgSplitFlagValue<'_, I> {
-    type Item = Result<String, String>;
-
-    fn next(&mut self) -> Option<Self::Item> {
-        let arg = self.args.next()?;
-        if let Some(suffix) = arg.strip_prefix(self.name) {
-            // Strip leading `name`.
-            if suffix.is_empty() {
-                // This argument is exactly `name`; the next one is the value.
-                return self.args.next().map(Ok);
-            } else if let Some(suffix) = suffix.strip_prefix('=') {
-                // This argument is `name=value`; get the value.
-                return Some(Ok(suffix.to_owned()));
-            }
-        }
-        Some(Err(arg))
-    }
-}
-
-/// Yields all values of command line flag `name`.
-pub struct ArgFlagValueIter<'a>(ArgSplitFlagValue<'a, env::Args>);
-
-impl<'a> ArgFlagValueIter<'a> {
-    pub fn new(name: &'a str) -> Self {
-        Self(ArgSplitFlagValue::new(env::args(), name))
-    }
-}
-
-impl Iterator for ArgFlagValueIter<'_> {
-    type Item = String;
-
-    fn next(&mut self) -> Option<Self::Item> {
-        loop {
-            if let Ok(value) = self.0.next()? {
-                return Some(value);
-            }
-        }
-    }
-}
-
-/// Gets the value of a `--flag`.
-pub fn get_arg_flag_value(name: &str) -> Option<String> {
-    ArgFlagValueIter::new(name).next()
-}
-
 /// Escapes `s` in a way that is suitable for using it as a string literal in TOML syntax.
 pub fn escape_for_toml(s: &str) -> String {
     // We want to surround this string in quotes `"`. So we first escape all quotes,
@@ -287,29 +216,33 @@ pub fn write_to_file(filename: &Path, content: &str) {
     fs::rename(temp_filename, filename).unwrap();
 }
 
-pub fn get_cargo_metadata() -> Metadata {
-    // The `build.target-dir` config can be passed by `--config` flags, so forward them to
-    // `cargo metadata`.
-    let mut additional_options = Vec::new();
+// Computes the extra flags that need to be passed to cargo to make it behave like the current
+// cargo invocation.
+fn cargo_extra_flags() -> Vec<String> {
+    let mut flags = Vec::new();
     // `-Zunstable-options` is required by `--config`.
-    additional_options.push("-Zunstable-options".to_string());
+    flags.push("-Zunstable-options".to_string());
 
+    // Forward `--config` flags.
     let config_flag = "--config";
-    for arg in ArgSplitFlagValue::new(
-        env::args().skip(3), // skip the program name, "miri" and "run" / "test"
-        config_flag,
-    )
-    // Only look at `Ok`
-    .flatten()
-    {
-        additional_options.push(config_flag.to_string());
-        additional_options.push(arg);
+    for arg in get_arg_flag_values(config_flag) {
+        flags.push(config_flag.to_string());
+        flags.push(arg);
     }
 
-    let metadata =
-        MetadataCommand::new().no_deps().other_options(additional_options).exec().unwrap();
+    // Forward `--manifest-path`.
+    let manifest_flag = "--manifest-path";
+    if let Some(manifest) = get_arg_flag_value(manifest_flag) {
+        flags.push(manifest_flag.to_string());
+        flags.push(manifest);
+    }
+
+    flags
+}
 
-    metadata
+pub fn get_cargo_metadata() -> Metadata {
+    // This will honor the `CARGO` env var the same way our `cargo()` does.
+    MetadataCommand::new().no_deps().other_options(cargo_extra_flags()).exec().unwrap()
 }
 
 /// Pulls all the crates in this workspace from the cargo metadata.
diff --git a/ci.sh b/ci.sh
@@ -54,8 +54,8 @@ function run_tests {
   unset RUSTC MIRI
   rm -rf .cargo
 
-  # Ensure that our benchmarks all work, on the host at least.
-  if [ -z "${MIRI_TEST_TARGET+exists}" ]; then
+  # Ensure that our benchmarks all work, but only on Linux hosts.
+  if [ -z "${MIRI_TEST_TARGET+exists}" ] && [ "$HOST_TARGET" = x86_64-unknown-linux-gnu ] ; then
     for BENCH in $(ls "bench-cargo-miri"); do
       cargo miri run --manifest-path bench-cargo-miri/$BENCH/Cargo.toml
     done
diff --git a/miri b/miri
@@ -90,13 +90,13 @@ bench)
     # Make sure we have an up-to-date Miri installed
     "$0" install
     # Run the requested benchmarks
-    if [ -z "$@" ]; then
+    if [ -z "${1+exists}" ]; then
         BENCHES=( $(ls "$MIRIDIR/bench-cargo-miri" ) )
     else
         BENCHES=("$@")
     fi
     for BENCH in "${BENCHES[@]}"; do
-        hyperfine -w 1 -m 5 --shell=none "cargo +$TOOLCHAIN miri run --manifest-path bench-cargo-miri/$BENCH/Cargo.toml"
+        hyperfine -w 1 -m 5 --shell=none "cargo +$TOOLCHAIN miri run --manifest-path $MIRIDIR/bench-cargo-miri/$BENCH/Cargo.toml"
     done
     exit 0
     ;;
