run-make-support: add #[must_use] and drop bombs for command wrappers

Especially for command wrappers like `Rustc`, it's very easy to build up
a command invocation but forget to actually execute it, e.g. by using
`run()`. This commit adds "drop bombs" to command wrappers, which are
armed on command wrapper construction, and only defused if the command
is executed (through `run`, `run_fail` or `run_fail_assert_exit_code`).
If the test writer forgets to execute the command, the drop bomb will
"explode" and panic with an error message. This is so that tests don't
silently pass with constructed-but-not-executed command wrappers.

We don't add `#[must_use]` for command wrapper helper methods because
they return `&mut Self` and can be annoying e.g. if a helper method is
conditionally called, such as

```
if condition {
    cmd.arg("-Cprefer-dynamic"); // <- unused_must_use fires
}
cmd.run(); // <- even though cmd is eventually executed
```

We also add `#[must_use]` attributes to functions in the support library
where suitable to help catch unintentionally discarded values.

Author: jieyouxu

src/tools/run-make-support/src/cc.rs

@@ -2,12 +2,14 @@ use std::env;
 use std::path::Path;
 use std::process::Command;
 
+use crate::drop_bomb::DropBomb;
 use crate::{bin_name, cygpath_windows, handle_failed_output, is_msvc, is_windows, tmp_dir, uname};
 
 /// Construct a new platform-specific C compiler invocation.
 ///
 /// WARNING: This means that what flags are accepted by the underlying C compiler is
 /// platform- AND compiler-specific. Consult the relevant docs for `gcc`, `clang` and `mvsc`.
+#[must_use]
 pub fn cc() -> Cc {
     Cc::new()
 }
@@ -17,6 +19,7 @@ pub fn cc() -> Cc {
 #[derive(Debug)]
 pub struct Cc {
     cmd: Command,
+    drop_bomb: DropBomb,
 }
 
 crate::impl_common_helpers!(Cc);
@@ -36,7 +39,7 @@ impl Cc {
             cmd.arg(flag);
         }
 
-        Self { cmd }
+        Self { cmd, drop_bomb: DropBomb::arm("cc invocation must be executed") }
     }
 
     /// Specify path of the input file.
@@ -87,6 +90,7 @@ impl Cc {
 }
 
 /// `EXTRACFLAGS`
+#[must_use]
 pub fn extra_c_flags() -> Vec<&'static str> {
     // Adapted from tools.mk (trimmed):
     //
@@ -145,6 +149,7 @@ pub fn extra_c_flags() -> Vec<&'static str> {
 }
 
 /// `EXTRACXXFLAGS`
+#[must_use]
 pub fn extra_cxx_flags() -> Vec<&'static str> {
     // Adapted from tools.mk (trimmed):
     //

src/tools/run-make-support/src/clang.rs

@@ -2,9 +2,11 @@ use std::env;
 use std::path::Path;
 use std::process::Command;
 
+use crate::drop_bomb::DropBomb;
 use crate::{bin_name, handle_failed_output, tmp_dir};
 
 /// Construct a new `clang` invocation. `clang` is not always available for all targets.
+#[must_use]
 pub fn clang() -> Clang {
     Clang::new()
 }
@@ -13,59 +15,68 @@ pub fn clang() -> Clang {
 #[derive(Debug)]
 pub struct Clang {
     cmd: Command,
+    drop_bomb: DropBomb,
 }
 
 crate::impl_common_helpers!(Clang);
 
 impl Clang {
     /// Construct a new `clang` invocation. `clang` is not always available for all targets.
+    #[must_use]
     pub fn new() -> Self {
         let clang =
             env::var("CLANG").expect("`CLANG` not specified, but this is required to find `clang`");
         let cmd = Command::new(clang);
-        Self { cmd }
+        Self { cmd, drop_bomb: DropBomb::arm("clang invocation must be executed") }
     }
 
     /// Provide an input file.
+    #[must_use]
     pub fn input<P: AsRef<Path>>(&mut self, path: P) -> &mut Self {
         self.cmd.arg(path.as_ref());
         self
     }
 
     /// Specify the name of the executable. The executable will be placed under `$TMPDIR`, and the
     /// extension will be determined by [`bin_name`].
+    #[must_use]
     pub fn out_exe(&mut self, name: &str) -> &mut Self {
         self.cmd.arg("-o");
         self.cmd.arg(tmp_dir().join(bin_name(name)));
         self
     }
 
     /// Specify which target triple clang should target.
+    #[must_use]
     pub fn target(&mut self, target_triple: &str) -> &mut Self {
         self.cmd.arg("-target");
         self.cmd.arg(target_triple);
         self
     }
 
     /// Pass `-nostdlib` to disable linking the C standard library.
+    #[must_use]
     pub fn no_stdlib(&mut self) -> &mut Self {
         self.cmd.arg("-nostdlib");
         self
     }
 
     /// Specify architecture.
+    #[must_use]
     pub fn arch(&mut self, arch: &str) -> &mut Self {
         self.cmd.arg(format!("-march={arch}"));
         self
     }
 
     /// Specify LTO settings.
+    #[must_use]
     pub fn lto(&mut self, lto: &str) -> &mut Self {
         self.cmd.arg(format!("-flto={lto}"));
         self
     }
 
     /// Specify which ld to use.
+    #[must_use]
     pub fn use_ld(&mut self, ld: &str) -> &mut Self {
         self.cmd.arg(format!("-fuse-ld={ld}"));
         self

src/tools/run-make-support/src/diff/mod.rs

@@ -2,9 +2,12 @@ use regex::Regex;
 use similar::TextDiff;
 use std::path::Path;
 
+use crate::drop_bomb::DropBomb;
+
 #[cfg(test)]
 mod tests;
 
+#[must_use]
 pub fn diff() -> Diff {
     Diff::new()
 }
@@ -16,17 +19,20 @@ pub struct Diff {
     actual: Option<String>,
     actual_name: Option<String>,
     normalizers: Vec<(String, String)>,
+    drop_bomb: DropBomb,
 }
 
 impl Diff {
     /// Construct a bare `diff` invocation.
+    #[must_use]
     pub fn new() -> Self {
         Self {
             expected: None,
             expected_name: None,
             actual: None,
             actual_name: None,
             normalizers: Vec::new(),
+            drop_bomb: DropBomb::arm("diff invocation must be executed"),
         }
     }
 
@@ -79,9 +85,9 @@ impl Diff {
         self
     }
 
-    /// Executes the diff process, prints any differences to the standard error.
     #[track_caller]
-    pub fn run(&self) {
+    pub fn run(&mut self) {
+        self.drop_bomb.defuse();
         let expected = self.expected.as_ref().expect("expected text not set");
         let mut actual = self.actual.as_ref().expect("actual text not set").to_string();
         let expected_name = self.expected_name.as_ref().unwrap();

src/tools/run-make-support/src/drop_bomb/mod.rs

@@ -0,0 +1,37 @@
+//! This module implements "drop bombs" intended for use by command wrappers to ensure that the
+//! constructed commands are *eventually* executed. This is exactly like `rustc_errors::Diag`
+//! where we force every `Diag` to be consumed or we emit a bug, but we panic instead.
+//!
+//! This is inspired by <https://docs.rs/drop_bomb/latest/drop_bomb/>.
+
+use std::borrow::Cow;
+
+#[cfg(test)]
+mod tests;
+
+#[derive(Debug)]
+pub(crate) struct DropBomb {
+    msg: Cow<'static, str>,
+    defused: bool,
+}
+
+impl DropBomb {
+    /// Arm a [`DropBomb`]. If the value is dropped without being [`defused`][Self::defused], then
+    /// it will panic.
+    pub(crate) fn arm<S: Into<Cow<'static, str>>>(message: S) -> DropBomb {
+        DropBomb { msg: message.into(), defused: false }
+    }
+
+    /// Defuse the [`DropBomb`]. This will prevent the drop bomb from panicking when dropped.
+    pub(crate) fn defuse(&mut self) {
+        self.defused = true;
+    }
+}
+
+impl Drop for DropBomb {
+    fn drop(&mut self) {
+        if !self.defused && !std::thread::panicking() {
+            panic!("{}", self.msg)
+        }
+    }
+}

src/tools/run-make-support/src/drop_bomb/tests.rs

@@ -0,0 +1,15 @@
+use super::DropBomb;
+
+#[test]
+#[should_panic]
+fn test_arm() {
+    let bomb = DropBomb::arm("hi :3");
+    drop(bomb); // <- armed bomb should explode when not defused
+}
+
+#[test]
+fn test_defuse() {
+    let mut bomb = DropBomb::arm("hi :3");
+    bomb.defuse();
+    drop(bomb); // <- defused bomb should not explode
+}