Skip to content

Commit 7df2b08

Browse files
committed
Auto merge of rust-lang#125752 - jieyouxu:kaboom, r=Kobzol
run-make: enforce `#[must_use]` and arm command wrappers with drop bombs This PR is one in a series of cleanups to run-make tests and the run-make-support library. ### Summary It's easy to forget to actually executed constructed command wrappers, e.g. `rustc().input("foo.rs")` but forget the `run()`, so to help catch these mistakes, we: - Add `#[must_use]` annotations to functions where suitable and compile rmake.rs recipes with `-Dunused_must_use`. - Arm command wrappers with drop bombs on construction to force them to be executed by test code. ### Details 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 ``` This PR is best reviewed commit-by-commit. Fixes rust-lang#125703. Because `command_output()` doesn't defuse the drop bomb, it also fixes rust-lang#125617.
2 parents 2a2c29a + 7867fb7 commit 7df2b08

File tree

19 files changed

+182
-85
lines changed

19 files changed

+182
-85
lines changed

src/tools/compiletest/src/runtest.rs

+4
Original file line numberDiff line numberDiff line change
@@ -3507,6 +3507,10 @@ impl<'test> TestCx<'test> {
35073507
.env_remove("MFLAGS")
35083508
.env_remove("CARGO_MAKEFLAGS");
35093509

3510+
// In test code we want to be very pedantic about values being silently discarded that are
3511+
// annotated with `#[must_use]`.
3512+
cmd.arg("-Dunused_must_use");
3513+
35103514
if std::env::var_os("COMPILETEST_FORCE_STAGE0").is_some() {
35113515
let mut stage0_sysroot = build_root.clone();
35123516
stage0_sysroot.push("stage0-sysroot");

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

+6-1
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ use std::env;
22
use std::path::Path;
33
use std::process::Command;
44

5+
use crate::drop_bomb::DropBomb;
56
use crate::{bin_name, cygpath_windows, handle_failed_output, is_msvc, is_windows, tmp_dir, uname};
67

78
/// Construct a new platform-specific C compiler invocation.
@@ -14,9 +15,11 @@ pub fn cc() -> Cc {
1415

1516
/// A platform-specific C compiler invocation builder. The specific C compiler used is
1617
/// passed down from compiletest.
18+
#[must_use]
1719
#[derive(Debug)]
1820
pub struct Cc {
1921
cmd: Command,
22+
drop_bomb: DropBomb,
2023
}
2124

2225
crate::impl_common_helpers!(Cc);
@@ -36,7 +39,7 @@ impl Cc {
3639
cmd.arg(flag);
3740
}
3841

39-
Self { cmd }
42+
Self { cmd, drop_bomb: DropBomb::arm("cc invocation must be executed") }
4043
}
4144

4245
/// Specify path of the input file.
@@ -87,6 +90,7 @@ impl Cc {
8790
}
8891

8992
/// `EXTRACFLAGS`
93+
#[must_use]
9094
pub fn extra_c_flags() -> Vec<&'static str> {
9195
// Adapted from tools.mk (trimmed):
9296
//
@@ -145,6 +149,7 @@ pub fn extra_c_flags() -> Vec<&'static str> {
145149
}
146150

147151
/// `EXTRACXXFLAGS`
152+
#[must_use]
148153
pub fn extra_cxx_flags() -> Vec<&'static str> {
149154
// Adapted from tools.mk (trimmed):
150155
//

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

+12-1
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ use std::env;
22
use std::path::Path;
33
use std::process::Command;
44

5+
use crate::drop_bomb::DropBomb;
56
use crate::{bin_name, handle_failed_output, tmp_dir};
67

78
/// Construct a new `clang` invocation. `clang` is not always available for all targets.
@@ -10,62 +11,72 @@ pub fn clang() -> Clang {
1011
}
1112

1213
/// A `clang` invocation builder.
14+
#[must_use]
1315
#[derive(Debug)]
1416
pub struct Clang {
1517
cmd: Command,
18+
drop_bomb: DropBomb,
1619
}
1720

1821
crate::impl_common_helpers!(Clang);
1922

2023
impl Clang {
2124
/// Construct a new `clang` invocation. `clang` is not always available for all targets.
25+
#[must_use]
2226
pub fn new() -> Self {
2327
let clang =
2428
env::var("CLANG").expect("`CLANG` not specified, but this is required to find `clang`");
2529
let cmd = Command::new(clang);
26-
Self { cmd }
30+
Self { cmd, drop_bomb: DropBomb::arm("clang invocation must be executed") }
2731
}
2832

2933
/// Provide an input file.
34+
#[must_use]
3035
pub fn input<P: AsRef<Path>>(&mut self, path: P) -> &mut Self {
3136
self.cmd.arg(path.as_ref());
3237
self
3338
}
3439

3540
/// Specify the name of the executable. The executable will be placed under `$TMPDIR`, and the
3641
/// extension will be determined by [`bin_name`].
42+
#[must_use]
3743
pub fn out_exe(&mut self, name: &str) -> &mut Self {
3844
self.cmd.arg("-o");
3945
self.cmd.arg(tmp_dir().join(bin_name(name)));
4046
self
4147
}
4248

4349
/// Specify which target triple clang should target.
50+
#[must_use]
4451
pub fn target(&mut self, target_triple: &str) -> &mut Self {
4552
self.cmd.arg("-target");
4653
self.cmd.arg(target_triple);
4754
self
4855
}
4956

5057
/// Pass `-nostdlib` to disable linking the C standard library.
58+
#[must_use]
5159
pub fn no_stdlib(&mut self) -> &mut Self {
5260
self.cmd.arg("-nostdlib");
5361
self
5462
}
5563

5664
/// Specify architecture.
65+
#[must_use]
5766
pub fn arch(&mut self, arch: &str) -> &mut Self {
5867
self.cmd.arg(format!("-march={arch}"));
5968
self
6069
}
6170

6271
/// Specify LTO settings.
72+
#[must_use]
6373
pub fn lto(&mut self, lto: &str) -> &mut Self {
6474
self.cmd.arg(format!("-flto={lto}"));
6575
self
6676
}
6777

6878
/// Specify which ld to use.
79+
#[must_use]
6980
pub fn use_ld(&mut self, ld: &str) -> &mut Self {
7081
self.cmd.arg(format!("-fuse-ld={ld}"));
7182
self

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

+8-2
Original file line numberDiff line numberDiff line change
@@ -2,31 +2,37 @@ use regex::Regex;
22
use similar::TextDiff;
33
use std::path::Path;
44

5+
use crate::drop_bomb::DropBomb;
6+
57
#[cfg(test)]
68
mod tests;
79

810
pub fn diff() -> Diff {
911
Diff::new()
1012
}
1113

14+
#[must_use]
1215
#[derive(Debug)]
1316
pub struct Diff {
1417
expected: Option<String>,
1518
expected_name: Option<String>,
1619
actual: Option<String>,
1720
actual_name: Option<String>,
1821
normalizers: Vec<(String, String)>,
22+
drop_bomb: DropBomb,
1923
}
2024

2125
impl Diff {
2226
/// Construct a bare `diff` invocation.
27+
#[must_use]
2328
pub fn new() -> Self {
2429
Self {
2530
expected: None,
2631
expected_name: None,
2732
actual: None,
2833
actual_name: None,
2934
normalizers: Vec::new(),
35+
drop_bomb: DropBomb::arm("diff invocation must be executed"),
3036
}
3137
}
3238

@@ -79,9 +85,9 @@ impl Diff {
7985
self
8086
}
8187

82-
/// Executes the diff process, prints any differences to the standard error.
8388
#[track_caller]
84-
pub fn run(&self) {
89+
pub fn run(&mut self) {
90+
self.drop_bomb.defuse();
8591
let expected = self.expected.as_ref().expect("expected text not set");
8692
let mut actual = self.actual.as_ref().expect("actual text not set").to_string();
8793
let expected_name = self.expected_name.as_ref().unwrap();
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
//! This module implements "drop bombs" intended for use by command wrappers to ensure that the
2+
//! constructed commands are *eventually* executed. This is exactly like `rustc_errors::Diag`
3+
//! where we force every `Diag` to be consumed or we emit a bug, but we panic instead.
4+
//!
5+
//! This is inspired by <https://docs.rs/drop_bomb/latest/drop_bomb/>.
6+
7+
use std::borrow::Cow;
8+
9+
#[cfg(test)]
10+
mod tests;
11+
12+
#[derive(Debug)]
13+
pub(crate) struct DropBomb {
14+
msg: Cow<'static, str>,
15+
defused: bool,
16+
}
17+
18+
impl DropBomb {
19+
/// Arm a [`DropBomb`]. If the value is dropped without being [`defused`][Self::defused], then
20+
/// it will panic.
21+
pub(crate) fn arm<S: Into<Cow<'static, str>>>(message: S) -> DropBomb {
22+
DropBomb { msg: message.into(), defused: false }
23+
}
24+
25+
/// Defuse the [`DropBomb`]. This will prevent the drop bomb from panicking when dropped.
26+
pub(crate) fn defuse(&mut self) {
27+
self.defused = true;
28+
}
29+
}
30+
31+
impl Drop for DropBomb {
32+
fn drop(&mut self) {
33+
if !self.defused && !std::thread::panicking() {
34+
panic!("{}", self.msg)
35+
}
36+
}
37+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
use super::DropBomb;
2+
3+
#[test]
4+
#[should_panic]
5+
fn test_arm() {
6+
let bomb = DropBomb::arm("hi :3");
7+
drop(bomb); // <- armed bomb should explode when not defused
8+
}
9+
10+
#[test]
11+
fn test_defuse() {
12+
let mut bomb = DropBomb::arm("hi :3");
13+
bomb.defuse();
14+
drop(bomb); // <- defused bomb should not explode
15+
}

0 commit comments

Comments
 (0)