Strongly type differing compiler args for clarity (#4220)

### Context
When working on #4211 & #4213, I was initially very confused by all the
machinery in
[`call_cargo.rs`](https://github.com/model-checking/kani/blob/main/kani-driver/src/call_cargo.rs)
and
[`call_single_file.rs`](https://github.com/model-checking/kani/blob/main/kani-driver/src/call_single_file.rs)
that we used to pass arguments to the different layers of the compiler.
The main source of my confusion was that there were many arguments all
destined for different places (e.g. cargo, rustc, or kani-compiler) that
all had to be encoded in differing ways (e.g. through the `--llvm-args`
flag or the `CARGO_ENCODED_RUSTFLAGS` environment variable) but were
mixed together sharing the same `String` type and similar-sounding
variable names.

Although misconfiguration would often be caught at runtime with a
section of the compiler complaining about unexpected arguments, this
confusion silently bit me a few times where argument names were shared.

### Fix
This PR represents an attempt to encode the way arguments target
different portions of the compiler into the type system using specific
wrapper types for each kind of argument we use. You can convert
transparently into these wrapper types, but then can only convert
between them or pass them to a cargo command using the proper encoding
methods.

Although not perfect (you could still convert into the wrong kind of
argument wrapper initially), this does seem to add some slight
guardrails on the way we call cargo, especially against smaller changes
to the existing patterns like the ones I was making in my PRs. It also
may make the code easier to parse.

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 and MIT licenses.

Author: AlexanderPortland

kani-driver/src/args/cargo.rs

@@ -3,8 +3,8 @@
 //! Module that define parsers that mimic Cargo options.
 
 use crate::args::ValidateArgs;
+use crate::util::args::CargoArg;
 use clap::error::Error;
-use std::ffi::OsString;
 use std::path::PathBuf;
 
 /// Arguments that Kani pass down into Cargo essentially uninterpreted.
@@ -61,8 +61,8 @@ impl CargoCommonArgs {
 
     /// Convert the arguments back to a format that cargo can understand.
     /// Note that the `exclude` option requires special processing and it's not included here.
-    pub fn to_cargo_args(&self) -> Vec<OsString> {
-        let mut cargo_args: Vec<OsString> = vec![];
+    pub fn to_cargo_args(&self) -> Vec<CargoArg> {
+        let mut cargo_args: Vec<CargoArg> = vec![];
         if self.all_features {
             cargo_args.push("--all-features".into());
         }
@@ -117,12 +117,12 @@ pub struct CargoTargetArgs {
 
 impl CargoTargetArgs {
     /// Convert the arguments back to a format that cargo can understand.
-    pub fn to_cargo_args(&self) -> Vec<OsString> {
+    pub fn to_cargo_args(&self) -> Vec<CargoArg> {
         let mut cargo_args = self
             .bin
             .iter()
             .map(|binary| format!("--bin={binary}").into())
-            .collect::<Vec<OsString>>();
+            .collect::<Vec<CargoArg>>();
 
         if self.bins {
             cargo_args.push("--bins".into());
@@ -168,7 +168,7 @@ pub struct CargoTestArgs {
 
 impl CargoTestArgs {
     /// Convert the arguments back to a format that cargo can understand.
-    pub fn to_cargo_args(&self) -> Vec<OsString> {
+    pub fn to_cargo_args(&self) -> Vec<CargoArg> {
         let mut cargo_args = self.common.to_cargo_args();
         cargo_args.append(&mut self.target.to_cargo_args());
         cargo_args

kani-driver/src/call_cargo.rs

@@ -2,13 +2,14 @@
 // SPDX-License-Identifier: Apache-2.0 OR MIT
 
 use crate::args::VerificationArgs;
-use crate::call_single_file::{LibConfig, to_rustc_arg};
+use crate::call_single_file::LibConfig;
 use crate::project::Artifact;
 use crate::session::{
     KaniSession, get_cargo_path, lib_folder, lib_no_core_folder, setup_cargo_command,
     setup_cargo_command_inner,
 };
 use crate::util;
+use crate::util::args::{CargoArg, CommandWrapper as _, KaniArg, PassTo, encode_as_rustc_arg};
 use anyhow::{Context, Result, bail};
 use cargo_metadata::diagnostic::{Diagnostic, DiagnosticLevel};
 use cargo_metadata::{
@@ -17,7 +18,6 @@ use cargo_metadata::{
 };
 use kani_metadata::{ArtifactType, CompilerArtifactStub};
 use std::collections::HashMap;
-use std::ffi::{OsStr, OsString};
 use std::fmt::{self, Display};
 use std::fs::{self, File};
 use std::io::IsTerminal;
@@ -77,13 +77,19 @@ crate-type = ["lib"]
     pub fn cargo_build_std(&self, std_path: &Path, krate_path: &Path) -> Result<Vec<Artifact>> {
         let lib_path = lib_no_core_folder().unwrap();
         let mut rustc_args = self.kani_rustc_flags(LibConfig::new_no_core(lib_path));
-        rustc_args.push(to_rustc_arg(self.kani_compiler_local_flags()).into());
+
+        // In theory, these could be passed just to the local crate rather than all crates,
+        // but the `cargo build` command we use for building `std` doesn't allow you to pass `rustc`
+        // arguments, so we have to pass them through the environment variable instead.
+        rustc_args.push(encode_as_rustc_arg(&self.kani_compiler_local_flags()));
+
         // Ignore global assembly, since `compiler_builtins` has some.
-        rustc_args.push(
-            to_rustc_arg(vec!["--ignore-global-asm".to_string(), self.reachability_arg()]).into(),
-        );
+        rustc_args.push(encode_as_rustc_arg(&[
+            KaniArg::from("--ignore-global-asm"),
+            self.reachability_arg(),
+        ]));
 
-        let mut cargo_args: Vec<OsString> = vec!["build".into()];
+        let mut cargo_args: Vec<CargoArg> = vec!["build".into()];
         cargo_args.append(&mut cargo_config_args());
 
         // Configuration needed to parse cargo compilation status.
@@ -103,12 +109,10 @@ crate-type = ["lib"]
 
         // Since we are verifying the standard library, we set the reachability to all crates.
         let mut cmd = setup_cargo_command()?;
-        cmd.args(&cargo_args)
+        cmd.pass_cargo_args(&cargo_args)
             .current_dir(krate_path)
             .env("RUSTC", &self.kani_compiler)
-            // Use CARGO_ENCODED_RUSTFLAGS instead of RUSTFLAGS is preferred. See
-            // https://doc.rust-lang.org/cargo/reference/environment-variables.html
-            .env("CARGO_ENCODED_RUSTFLAGS", rustc_args.join(OsStr::new("\x1f")))
+            .pass_rustc_args(&rustc_args, PassTo::AllCrates)
             .env("CARGO_TERM_PROGRESS_WHEN", "never")
             .env("__CARGO_TESTS_ONLY_SRC_ROOT", full_path.as_os_str());
 
@@ -146,9 +150,9 @@ crate-type = ["lib"]
 
         let lib_path = lib_folder().unwrap();
         let mut rustc_args = self.kani_rustc_flags(LibConfig::new(lib_path));
-        rustc_args.push(to_rustc_arg(self.kani_compiler_dependency_flags()).into());
+        rustc_args.push(encode_as_rustc_arg(&self.kani_compiler_dependency_flags()));
 
-        let mut cargo_args: Vec<OsString> = vec!["rustc".into()];
+        let mut cargo_args: Vec<CargoArg> = vec!["rustc".into()];
         if let Some(path) = &self.args.cargo.manifest_path {
             cargo_args.push("--manifest-path".into());
             cargo_args.push(path.into());
@@ -201,9 +205,6 @@ crate-type = ["lib"]
         let mut kani_pkg_args = vec![self.reachability_arg()];
         kani_pkg_args.extend(self.kani_compiler_local_flags());
 
-        // Convert package args to rustc args for passing
-        let pkg_args = vec!["--".into(), to_rustc_arg(kani_pkg_args)];
-
         let mut found_target = false;
         let packages = self.packages_to_verify(&self.args, &metadata)?;
         let mut artifacts = vec![];
@@ -212,14 +213,13 @@ crate-type = ["lib"]
             for verification_target in package_targets(&self.args, package) {
                 let mut cmd =
                     setup_cargo_command_inner(Some(verification_target.target().name.clone()))?;
-                cmd.args(&cargo_args)
+                cmd.pass_cargo_args(&cargo_args)
                     .args(vec!["-p", &package.id.to_string()])
                     .args(verification_target.to_args())
-                    .args(&pkg_args)
+                    .arg("--") // Add this delimiter so we start passing args to rustc and not Cargo
                     .env("RUSTC", &self.kani_compiler)
-                    // Use CARGO_ENCODED_RUSTFLAGS instead of RUSTFLAGS is preferred. See
-                    // https://doc.rust-lang.org/cargo/reference/environment-variables.html
-                    .env("CARGO_ENCODED_RUSTFLAGS", rustc_args.join(OsStr::new("\x1f")))
+                    .pass_rustc_args(&rustc_args, PassTo::AllCrates)
+                    .pass_rustc_arg(encode_as_rustc_arg(&kani_pkg_args), PassTo::OnlyLocalCrate)
                     // This is only required for stable but is a no-op for nightly channels
                     .env("RUSTC_BOOTSTRAP", "1")
                     .env("CARGO_TERM_PROGRESS_WHEN", "never");
@@ -472,7 +472,7 @@ crate-type = ["lib"]
     }
 }
 
-pub fn cargo_config_args() -> Vec<OsString> {
+pub fn cargo_config_args() -> Vec<CargoArg> {
     [
         "--target",
         env!("TARGET"),
@@ -481,7 +481,7 @@ pub fn cargo_config_args() -> Vec<OsString> {
         "-Ztarget-applies-to-host",
         "--config=host.rustflags=[\"--cfg=kani_host\"]",
     ]
-    .map(OsString::from)
+    .map(CargoArg::from)
     .to_vec()
 }
 

kani-driver/src/call_single_file.rs

@@ -3,14 +3,14 @@
 
 use anyhow::Result;
 use kani_metadata::UnstableFeature;
-use std::ffi::OsString;
 use std::path::{Path, PathBuf};
 use std::process::Command;
 
 use crate::session::{KaniSession, lib_folder};
+use crate::util::args::{CommandWrapper, KaniArg, PassTo, RustcArg, encode_as_rustc_arg};
 
 pub struct LibConfig {
-    args: Vec<OsString>,
+    args: Vec<RustcArg>,
 }
 
 impl LibConfig {
@@ -28,15 +28,15 @@ impl LibConfig {
             "--extern",
             kani_std_wrapper.as_str(),
         ]
-        .map(OsString::from)
+        .map(RustcArg::from)
         .to_vec();
         LibConfig { args }
     }
 
     pub fn new_no_core(path: PathBuf) -> LibConfig {
         LibConfig {
             args: ["-L", path.to_str().unwrap(), "--extern", "kani_core"]
-                .map(OsString::from)
+                .map(RustcArg::from)
                 .to_vec(),
         }
     }
@@ -52,13 +52,13 @@ impl KaniSession {
         outdir: &Path,
     ) -> Result<()> {
         let mut kani_args = self.kani_compiler_local_flags();
-        kani_args.push(format!("--reachability={}", self.reachability_mode()));
+        kani_args.push(format!("--reachability={}", self.reachability_mode()).into());
 
         let lib_path = lib_folder().unwrap();
         let mut rustc_args = self.kani_rustc_flags(LibConfig::new(lib_path));
         rustc_args.push(file.into());
         rustc_args.push("--out-dir".into());
-        rustc_args.push(OsString::from(outdir.as_os_str()));
+        rustc_args.push(RustcArg::from(outdir.as_os_str()));
         rustc_args.push("--crate-name".into());
         rustc_args.push(crate_name.into());
 
@@ -79,8 +79,9 @@ impl KaniSession {
         // Note that the order of arguments is important. Kani specific flags should precede
         // rustc ones.
         let mut cmd = Command::new(&self.kani_compiler);
-        let kani_compiler_args = to_rustc_arg(kani_args);
-        cmd.arg(kani_compiler_args).args(rustc_args);
+
+        cmd.pass_rustc_arg(encode_as_rustc_arg(&kani_args), PassTo::OnlyLocalCrate)
+            .pass_rustc_args(&rustc_args, PassTo::OnlyLocalCrate);
 
         // This is only required for stable but is a no-op for nightly channels
         cmd.env("RUSTC_BOOTSTRAP", "1");
@@ -94,13 +95,13 @@ impl KaniSession {
     }
 
     /// Create a compiler option that represents the reachability mode.
-    pub fn reachability_arg(&self) -> String {
-        format!("--reachability={}", self.reachability_mode())
+    pub fn reachability_arg(&self) -> KaniArg {
+        format!("--reachability={}", self.reachability_mode()).into()
     }
 
     /// The `kani-compiler`-specific arguments that should be passed when building all crates,
     /// including dependencies.
-    pub fn kani_compiler_dependency_flags(&self) -> Vec<String> {
+    pub fn kani_compiler_dependency_flags(&self) -> Vec<KaniArg> {
         let mut flags = vec![check_version()];
 
         if self.args.ignore_global_asm {
@@ -112,8 +113,8 @@ impl KaniSession {
 
     /// The `kani-compiler`-specific arguments that should be passed only to the local crate
     /// being compiled.
-    pub fn kani_compiler_local_flags(&self) -> Vec<String> {
-        let mut flags = vec![];
+    pub fn kani_compiler_local_flags(&self) -> Vec<KaniArg> {
+        let mut flags: Vec<KaniArg> = vec![];
 
         if self.args.common_args.debug {
             flags.push("--log-level=debug".into());
@@ -163,29 +164,29 @@ impl KaniSession {
         }
 
         for harness in &self.args.harnesses {
-            flags.push(format!("--harness {harness}"));
+            flags.push(format!("--harness {harness}").into());
         }
 
         if self.args.exact {
             flags.push("--exact".into());
         }
 
         if let Some(args) = self.autoharness_compiler_flags.clone() {
-            flags.extend(args);
+            flags.extend(args.into_iter().map(KaniArg::from));
         }
 
-        flags.extend(self.args.common_args.unstable_features.as_arguments().map(str::to_string));
+        flags.extend(self.args.common_args.unstable_features.as_arguments().map(KaniArg::from));
 
         flags
     }
 
     /// This function generates all rustc configurations required by our goto-c codegen.
-    pub fn kani_rustc_flags(&self, lib_config: LibConfig) -> Vec<OsString> {
+    pub fn kani_rustc_flags(&self, lib_config: LibConfig) -> Vec<RustcArg> {
         let mut flags: Vec<_> = base_rustc_flags(lib_config);
         // We only use panic abort strategy for verification since we cannot handle unwind logic.
         if self.args.coverage {
             flags.extend_from_slice(
-                &["-C", "instrument-coverage", "-Z", "no-profiler-runtime"].map(OsString::from),
+                &["-C", "instrument-coverage", "-Z", "no-profiler-runtime"].map(RustcArg::from),
             );
         }
         flags.extend_from_slice(
@@ -202,7 +203,7 @@ impl KaniSession {
                 // Do not invoke the linker since the compiler will not generate real object files
                 "-Clinker=echo",
             ]
-            .map(OsString::from),
+            .map(RustcArg::from),
         );
 
         if self.args.no_codegen {
@@ -232,7 +233,7 @@ impl KaniSession {
 }
 
 /// Common flags used for compiling user code for verification and playback flow.
-pub fn base_rustc_flags(lib_config: LibConfig) -> Vec<OsString> {
+pub fn base_rustc_flags(lib_config: LibConfig) -> Vec<RustcArg> {
     let mut flags = [
         "-C",
         "overflow-checks=on",
@@ -250,38 +251,24 @@ pub fn base_rustc_flags(lib_config: LibConfig) -> Vec<OsString> {
         "-Z",
         "crate-attr=register_tool(kanitool)",
     ]
-    .map(OsString::from)
+    .map(RustcArg::from)
     .to_vec();
 
     flags.extend(lib_config.args);
 
     // e.g. compiletest will set 'compile-flags' here and we should pass those down to rustc
     // and we fail in `tests/kani/Match/match_bool.rs`
     if let Ok(str) = std::env::var("RUSTFLAGS") {
-        flags.extend(str.split(' ').map(OsString::from));
+        flags.extend(str.split(' ').map(RustcArg::from));
     }
 
     flags
 }
 
-/// This function can be used to convert Kani compiler specific arguments into a rustc one.
-/// We currently pass Kani specific arguments using the `--llvm-args` structure which is the
-/// hacky mechanism used by other rustc backend to receive arguments unknown to rustc.
-///
-/// Note that Cargo caching mechanism takes the building context into consideration, which
-/// includes the value of the rust flags. By using `--llvm-args`, we ensure that Cargo takes into
-/// consideration all arguments that are used to configure Kani compiler. For example, enabling the
-/// reachability checks will force recompilation if they were disabled in previous build.
-/// For more details on this caching mechanism, see the
-/// [fingerprint documentation](https://github.com/rust-lang/cargo/blob/82c3bb79e3a19a5164e33819ef81bfc2c984bc56/src/cargo/core/compiler/fingerprint/mod.rs)
-pub fn to_rustc_arg(kani_args: Vec<String>) -> String {
-    format!(r#"-Cllvm-args={}"#, kani_args.join(" "))
-}
-
 /// Function that returns a `--check-version` argument to be added to the compiler flags.
 /// This is really just used to force the compiler to recompile everything from scratch when a user
 /// upgrades Kani. Cargo currently ignores the codegen backend version.
 /// See <https://github.com/model-checking/kani/issues/2140> for more context.
-fn check_version() -> String {
-    format!("--check-version={}", env!("CARGO_PKG_VERSION"))
+fn check_version() -> KaniArg {
+    format!("--check-version={}", env!("CARGO_PKG_VERSION")).into()
 }