Skip to content

Commit ef2e85e

Browse files
borsbors
committed
Auto merge of #126197 - jieyouxu:rmake-must-use, r=<try>
run-make: annotate library with `#[must_use]` and enforce `unused_must_use` in rmake.rs Commits up to 706abf6 are from #125752. This PR adds `#[must_use]` annotations to functions of the `run_make_support` library where it makes sense, and adjusts compiletest to compile rmake.rs with `-Dunused_must_use`. The rationale is that it's highly likely that unused `#[must_use]` values in rmake.rs test files are bugs. For example, unused fs/io results are often crucial to the correctness of the test and often unchecked fs/io results allow the test to silently pass where it would've failed if the result was checked. try-job: test-various try-job: x86_64-msvc
2 parents 212841e + 8d182e3 commit ef2e85e

File tree

27 files changed

+332
-87
lines changed

27 files changed

+332
-87
lines changed

‎Cargo.lock

+1-1
Original file line numberDiff line numberDiff line change
@@ -3452,7 +3452,7 @@ dependencies = [
34523452

34533453
[[package]]
34543454
name = "run_make_support"
3455-
version = "0.0.0"
3455+
version = "0.2.0"
34563456
dependencies = [
34573457
"gimli 0.28.1",
34583458
"object 0.34.0",

‎src/tools/compiletest/src/runtest.rs

+4
Original file line numberDiff line numberDiff line change
@@ -3551,6 +3551,10 @@ impl<'test> TestCx<'test> {
35513551
.env("TARGET_RPATH_DIR", cwd.join(&self.config.run_lib_path))
35523552
.env("LLVM_COMPONENTS", &self.config.llvm_components);
35533553

3554+
// In test code we want to be very pedantic about values being silently discarded that are
3555+
// annotated with `#[must_use]`.
3556+
cmd.arg("-Dunused_must_use");
3557+
35543558
if std::env::var_os("COMPILETEST_FORCE_STAGE0").is_some() {
35553559
let mut stage0_sysroot = build_root.clone();
35563560
stage0_sysroot.push("stage0-sysroot");

‎src/tools/run-make-support/CHANGELOG.md

+73
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
# Changelog
2+
3+
All notable changes to the `run_make_support` library should be documented in this file.
4+
5+
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) and the support
6+
library should adhere to [Semantic Versioning](https://semver.org/spec/v2.0.0.html) even if it's
7+
not intended for public consumption (it's moreso to help internally, to help test writers track
8+
changes to the support library).
9+
10+
This support library will probably never reach 1.0. Please bump the minor version in `Cargo.toml` if
11+
you make any breaking changes or other significant changes, or bump the patch version for bug fixes.
12+
13+
## [0.2.0] - 2024-06-09
14+
15+
### Changed
16+
17+
- Marked many functions with `#[must_use]`, and rmake.rs are now compiled with `-Dmust_use`.
18+
19+
## [0.1.0] - 2024-06-09
20+
21+
### Changed
22+
23+
- Use *drop bombs* to enforce that commands are executed; a command invocation will panic if it is
24+
constructed but never executed. Execution methods `Command::{run, run_fail}` will defuse the drop
25+
bomb.
26+
- Added `Command` helpers that forward to `std::process::Command` counterparts.
27+
28+
### Removed
29+
30+
- The `env_var` method which was incorrectly named and is `env_clear` underneath and is a footgun
31+
from `impl_common_helpers`. For example, removing `TMPDIR` on Unix and `TMP`/`TEMP` breaks
32+
`std::env::temp_dir` and wrecks anything using that, such as rustc's codgen.
33+
- Removed `Deref`/`DerefMut` for `run_make_support::Command` -> `std::process::Command` because it
34+
causes a method chain like `htmldocck().arg().run()` to fail, because `arg()` resolves to
35+
`std::process::Command` which also returns a `&mut std::process::Command`, causing the `run()` to
36+
be not found.
37+
38+
## [0.0.0] - 2024-06-09
39+
40+
Consider this version to contain all changes made to the support library before we started to track
41+
changes in this changelog.
42+
43+
### Added
44+
45+
- Custom command wrappers around `std::process::Command` (`run_make_support::Command`) and custom
46+
wrapper around `std::process::Output` (`CompletedProcess`) to make it more convenient to work with
47+
commands and their output, and help avoid forgetting to check for exit status.
48+
- `Command`: `set_stdin`, `run`, `run_fail`.
49+
- `CompletedProcess`: `std{err,out}_utf8`, `status`, `assert_std{err,out}_{equals, contains,
50+
not_contains}`, `assert_exit_code`.
51+
- `impl_common_helpers` macro to avoid repeating adding common convenience methods, including:
52+
- Environment manipulation methods: `env`, `env_remove`
53+
- Command argument providers: `arg`, `args`
54+
- Common invocation inspection (of the command invocation up until `inspect` is called):
55+
`inspect`
56+
- Execution methods: `run` (for commands expected to succeed execution, exit status `0`) and
57+
`run_fail` (for commands expected to fail execution, exit status non-zero).
58+
- Command wrappers around: `rustc`, `clang`, `cc`, `rustc`, `rustdoc`, `llvm-readobj`.
59+
- Thin helpers to construct `python` and `htmldocck` commands.
60+
- `run` and `run_fail` (like `Command::{run, run_fail}`) for running binaries, which sets suitable
61+
env vars (like `LD_LIB_PATH` or equivalent, `TARGET_RPATH_ENV`, `PATH` on Windows).
62+
- Pseudo command `diff` which has similar functionality as the cli util but not the same API.
63+
- Convenience panic-on-fail helpers `env_var`, `env_var_os`, `cwd` for their `std::env` conterparts.
64+
- Convenience panic-on-fail helpers for reading respective env vars: `target`, `source_root`.
65+
- Platform check helpers: `is_windows`, `is_msvc`, `cygpath_windows`, `uname`.
66+
- fs helpers: `copy_dir_all`.
67+
- `recursive_diff` helper.
68+
- Generic `assert_not_contains` helper.
69+
- Scoped run-with-teardown helper `run_in_tmpdir` which is designed to run commands in a temporary
70+
directory that is cleared when closure returns.
71+
- Helpers for constructing the name of binaries and libraries: `rust_lib_name`, `static_lib_name`,
72+
`bin_name`, `dynamic_lib_name`.
73+
- Re-export libraries: `gimli`, `object`, `regex`, `wasmparsmer`.

‎src/tools/run-make-support/Cargo.toml

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
[package]
22
name = "run_make_support"
3-
version = "0.0.0"
3+
version = "0.2.0"
44
edition = "2021"
55

66
[dependencies]

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

+3-5
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,15 @@ use crate::{bin_name, cygpath_windows, env_var, is_msvc, is_windows, uname};
77
///
88
/// WARNING: This means that what flags are accepted by the underlying C compiler is
99
/// platform- AND compiler-specific. Consult the relevant docs for `gcc`, `clang` and `mvsc`.
10+
#[track_caller]
1011
pub fn cc() -> Cc {
1112
Cc::new()
1213
}
1314

1415
/// A platform-specific C compiler invocation builder. The specific C compiler used is
1516
/// passed down from compiletest.
1617
#[derive(Debug)]
18+
#[must_use]
1719
pub struct Cc {
1820
cmd: Command,
1921
}
@@ -25,6 +27,7 @@ impl Cc {
2527
///
2628
/// WARNING: This means that what flags are accepted by the underlying C compile is
2729
/// platform- AND compiler-specific. Consult the relevant docs for `gcc`, `clang` and `mvsc`.
30+
#[track_caller]
2831
pub fn new() -> Self {
2932
let compiler = env_var("CC");
3033

@@ -84,11 +87,6 @@ impl Cc {
8487
self.cmd.arg(path.as_ref());
8588
self
8689
}
87-
88-
/// Get the [`Output`][::std::process::Output] of the finished process.
89-
pub fn command_output(&mut self) -> ::std::process::Output {
90-
self.cmd.output().expect("failed to get output of finished process")
91-
}
9290
}
9391

9492
/// `EXTRACFLAGS`

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

+3
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,14 @@ use crate::command::Command;
44
use crate::{bin_name, env_var};
55

66
/// Construct a new `clang` invocation. `clang` is not always available for all targets.
7+
#[track_caller]
78
pub fn clang() -> Clang {
89
Clang::new()
910
}
1011

1112
/// A `clang` invocation builder.
1213
#[derive(Debug)]
14+
#[must_use]
1315
pub struct Clang {
1416
cmd: Command,
1517
}
@@ -18,6 +20,7 @@ crate::impl_common_helpers!(Clang);
1820

1921
impl Clang {
2022
/// Construct a new `clang` invocation. `clang` is not always available for all targets.
23+
#[track_caller]
2124
pub fn new() -> Self {
2225
let clang = env_var("CLANG");
2326
let cmd = Command::new(clang);

‎src/tools/run-make-support/src/command.rs

+89-30
Original file line numberDiff line numberDiff line change
@@ -1,55 +1,125 @@
1-
use crate::{assert_not_contains, handle_failed_output};
1+
use std::ffi;
22
use std::ffi::OsStr;
33
use std::io::Write;
4-
use std::ops::{Deref, DerefMut};
4+
use std::panic;
5+
use std::path::Path;
56
use std::process::{Command as StdCommand, ExitStatus, Output, Stdio};
67

7-
/// This is a custom command wrapper that simplifies working with commands
8-
/// and makes it easier to ensure that we check the exit status of executed
9-
/// processes.
8+
use crate::drop_bomb::DropBomb;
9+
use crate::{assert_not_contains, handle_failed_output};
10+
11+
/// This is a custom command wrapper that simplifies working with commands and makes it easier to
12+
/// ensure that we check the exit status of executed processes.
13+
///
14+
/// # A [`Command`] must be executed
15+
///
16+
/// A [`Command`] is armed by a [`DropBomb`] on construction to enforce that it will be executed. If
17+
/// a [`Commmand`] is constructed but never executed, the drop bomb will explode and cause the test
18+
/// to panic. Execution methods [`run`] and [`run_fail`] will defuse the drop bomb. A test
19+
/// containing constructed but never executed commands is dangerous because it can give a false
20+
/// sense of confidence.
21+
///
22+
/// [`run`]: Self::run
23+
/// [`run_fail`]: Self::run
1024
#[derive(Debug)]
1125
pub struct Command {
1226
cmd: StdCommand,
1327
stdin: Option<Box<[u8]>>,
28+
drop_bomb: DropBomb,
1429
}
1530

1631
impl Command {
17-
pub fn new<S: AsRef<OsStr>>(program: S) -> Self {
18-
Self { cmd: StdCommand::new(program), stdin: None }
32+
#[track_caller]
33+
pub fn new<P: AsRef<OsStr>>(program: P) -> Self {
34+
let program = program.as_ref();
35+
Self { cmd: StdCommand::new(program), stdin: None, drop_bomb: DropBomb::arm(program) }
1936
}
2037

2138
pub fn set_stdin(&mut self, stdin: Box<[u8]>) {
2239
self.stdin = Some(stdin);
2340
}
2441

42+
/// Specify an environment variable.
43+
pub fn env<K, V>(&mut self, key: K, value: V) -> &mut Self
44+
where
45+
K: AsRef<ffi::OsStr>,
46+
V: AsRef<ffi::OsStr>,
47+
{
48+
self.cmd.env(key, value);
49+
self
50+
}
51+
52+
/// Remove an environmental variable.
53+
pub fn env_remove<K>(&mut self, key: K) -> &mut Self
54+
where
55+
K: AsRef<ffi::OsStr>,
56+
{
57+
self.cmd.env_remove(key);
58+
self
59+
}
60+
61+
/// Generic command argument provider. Prefer specific helper methods if possible.
62+
/// Note that for some executables, arguments might be platform specific. For C/C++
63+
/// compilers, arguments might be platform *and* compiler specific.
64+
pub fn arg<S>(&mut self, arg: S) -> &mut Self
65+
where
66+
S: AsRef<ffi::OsStr>,
67+
{
68+
self.cmd.arg(arg);
69+
self
70+
}
71+
72+
/// Generic command arguments provider. Prefer specific helper methods if possible.
73+
/// Note that for some executables, arguments might be platform specific. For C/C++
74+
/// compilers, arguments might be platform *and* compiler specific.
75+
pub fn args<S>(&mut self, args: &[S]) -> &mut Self
76+
where
77+
S: AsRef<ffi::OsStr>,
78+
{
79+
self.cmd.args(args);
80+
self
81+
}
82+
83+
/// Inspect what the underlying [`std::process::Command`] is up to the
84+
/// current construction.
85+
pub fn inspect<I>(&mut self, inspector: I) -> &mut Self
86+
where
87+
I: FnOnce(&StdCommand),
88+
{
89+
inspector(&self.cmd);
90+
self
91+
}
92+
93+
/// Set the path where the command will be run.
94+
pub fn current_dir<P: AsRef<Path>>(&mut self, path: P) -> &mut Self {
95+
self.cmd.current_dir(path);
96+
self
97+
}
98+
2599
/// Run the constructed command and assert that it is successfully run.
26100
#[track_caller]
27101
pub fn run(&mut self) -> CompletedProcess {
28-
let caller_location = std::panic::Location::caller();
29-
let caller_line_number = caller_location.line();
30-
102+
self.drop_bomb.defuse();
31103
let output = self.command_output();
32104
if !output.status().success() {
33-
handle_failed_output(&self, output, caller_line_number);
105+
handle_failed_output(&self, output, panic::Location::caller().line());
34106
}
35107
output
36108
}
37109

38110
/// Run the constructed command and assert that it does not successfully run.
39111
#[track_caller]
40112
pub fn run_fail(&mut self) -> CompletedProcess {
41-
let caller_location = std::panic::Location::caller();
42-
let caller_line_number = caller_location.line();
43-
113+
self.drop_bomb.defuse();
44114
let output = self.command_output();
45115
if output.status().success() {
46-
handle_failed_output(&self, output, caller_line_number);
116+
handle_failed_output(&self, output, panic::Location::caller().line());
47117
}
48118
output
49119
}
50120

51121
#[track_caller]
52-
pub(crate) fn command_output(&mut self) -> CompletedProcess {
122+
fn command_output(&mut self) -> CompletedProcess {
53123
// let's make sure we piped all the input and outputs
54124
self.cmd.stdin(Stdio::piped());
55125
self.cmd.stdout(Stdio::piped());
@@ -71,20 +141,6 @@ impl Command {
71141
}
72142
}
73143

74-
impl Deref for Command {
75-
type Target = StdCommand;
76-
77-
fn deref(&self) -> &Self::Target {
78-
&self.cmd
79-
}
80-
}
81-
82-
impl DerefMut for Command {
83-
fn deref_mut(&mut self) -> &mut Self::Target {
84-
&mut self.cmd
85-
}
86-
}
87-
88144
/// Represents the result of an executed process.
89145
/// The various `assert_` helper methods should preferably be used for
90146
/// checking the contents of stdout/stderr.
@@ -93,14 +149,17 @@ pub struct CompletedProcess {
93149
}
94150

95151
impl CompletedProcess {
152+
#[must_use]
96153
pub fn stdout_utf8(&self) -> String {
97154
String::from_utf8(self.output.stdout.clone()).expect("stdout is not valid UTF-8")
98155
}
99156

157+
#[must_use]
100158
pub fn stderr_utf8(&self) -> String {
101159
String::from_utf8(self.output.stderr.clone()).expect("stderr is not valid UTF-8")
102160
}
103161

162+
#[must_use]
104163
pub fn status(&self) -> ExitStatus {
105164
self.output.status
106165
}

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

+9-2
Original file line numberDiff line numberDiff line change
@@ -2,31 +2,38 @@ 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

10+
#[track_caller]
811
pub fn diff() -> Diff {
912
Diff::new()
1013
}
1114

1215
#[derive(Debug)]
16+
#[must_use]
1317
pub struct Diff {
1418
expected: Option<String>,
1519
expected_name: Option<String>,
1620
actual: Option<String>,
1721
actual_name: Option<String>,
1822
normalizers: Vec<(String, String)>,
23+
drop_bomb: DropBomb,
1924
}
2025

2126
impl Diff {
2227
/// Construct a bare `diff` invocation.
28+
#[track_caller]
2329
pub fn new() -> Self {
2430
Self {
2531
expected: None,
2632
expected_name: None,
2733
actual: None,
2834
actual_name: None,
2935
normalizers: Vec::new(),
36+
drop_bomb: DropBomb::arm("diff"),
3037
}
3138
}
3239

@@ -79,9 +86,9 @@ impl Diff {
7986
self
8087
}
8188

82-
/// Executes the diff process, prints any differences to the standard error.
8389
#[track_caller]
84-
pub fn run(&self) {
90+
pub fn run(&mut self) {
91+
self.drop_bomb.defuse();
8592
let expected = self.expected.as_ref().expect("expected text not set");
8693
let mut actual = self.actual.as_ref().expect("actual text not set").to_string();
8794
let expected_name = self.expected_name.as_ref().unwrap();

0 commit comments

Comments
 (0)