Skip to content

Commit

Permalink
Auto merge of #126197 - jieyouxu:rmake-must-use, r=Kobzol
Browse files Browse the repository at this point in the history
run-make: annotate library with `#[must_use]` and enforce `unused_must_use` in rmake.rs

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 load-bearing 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.

This PR is best reviewed commit-by-commit.

try-job: test-various
try-job: x86_64-msvc
  • Loading branch information
bors committed Jun 13, 2024
2 parents 56e112a + 5b126ed commit 921645c
Show file tree
Hide file tree
Showing 12 changed files with 48 additions and 2 deletions.
2 changes: 1 addition & 1 deletion Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3452,7 +3452,7 @@ dependencies = [

[[package]]
name = "run_make_support"
version = "0.1.0"
version = "0.2.0"
dependencies = [
"gimli 0.28.1",
"object 0.34.0",
Expand Down
4 changes: 4 additions & 0 deletions src/tools/compiletest/src/runtest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3551,6 +3551,10 @@ impl<'test> TestCx<'test> {
.env("TARGET_RPATH_DIR", cwd.join(&self.config.run_lib_path))
.env("LLVM_COMPONENTS", &self.config.llvm_components);

// In test code we want to be very pedantic about values being silently discarded that are
// annotated with `#[must_use]`.
cmd.arg("-Dunused_must_use");

if std::env::var_os("COMPILETEST_FORCE_STAGE0").is_some() {
let mut stage0_sysroot = build_root.clone();
stage0_sysroot.push("stage0-sysroot");
Expand Down
16 changes: 16 additions & 0 deletions src/tools/run-make-support/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,22 @@ changes to the support library).
This support library will probably never reach 1.0. Please bump the minor version in `Cargo.toml` if
you make any breaking changes or other significant changes, or bump the patch version for bug fixes.

## [0.2.0] - 2024-06-11

### Added

- Added `fs_wrapper` module which provides panic-on-fail helpers for their respective `std::fs`
counterparts, the motivation is to:
- Reduce littering `.unwrap()` or `.expect()` everywhere for fs operations
- Help the test writer avoid forgetting to check fs results (even though enforced by
`-Dunused_must_use`)
- Provide better panic messages by default
- Added `path()` helper which creates a `Path` relative to `cwd()` (but is less noisy).

### Changed

- Marked many functions with `#[must_use]`, and rmake.rs are now compiled with `-Dunused_must_use`.

## [0.1.0] - 2024-06-09

### Changed
Expand Down
2 changes: 1 addition & 1 deletion src/tools/run-make-support/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "run_make_support"
version = "0.1.0"
version = "0.2.0"
edition = "2021"

[dependencies]
Expand Down
1 change: 1 addition & 0 deletions src/tools/run-make-support/src/cc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ pub fn cc() -> Cc {
/// A platform-specific C compiler invocation builder. The specific C compiler used is
/// passed down from compiletest.
#[derive(Debug)]
#[must_use]
pub struct Cc {
cmd: Command,
}
Expand Down
1 change: 1 addition & 0 deletions src/tools/run-make-support/src/clang.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ pub fn clang() -> Clang {

/// A `clang` invocation builder.
#[derive(Debug)]
#[must_use]
pub struct Clang {
cmd: Command,
}
Expand Down
3 changes: 3 additions & 0 deletions src/tools/run-make-support/src/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,14 +148,17 @@ pub struct CompletedProcess {
}

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

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

#[must_use]
pub fn status(&self) -> ExitStatus {
self.output.status
}
Expand Down
1 change: 1 addition & 0 deletions src/tools/run-make-support/src/diff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ pub fn diff() -> Diff {
}

#[derive(Debug)]
#[must_use]
pub struct Diff {
expected: Option<String>,
expected_name: Option<String>,
Expand Down
17 changes: 17 additions & 0 deletions src/tools/run-make-support/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ pub use rustc::{aux_build, rustc, Rustc};
pub use rustdoc::{bare_rustdoc, rustdoc, Rustdoc};

#[track_caller]
#[must_use]
pub fn env_var(name: &str) -> String {
match env::var(name) {
Ok(v) => v,
Expand All @@ -45,6 +46,7 @@ pub fn env_var(name: &str) -> String {
}

#[track_caller]
#[must_use]
pub fn env_var_os(name: &str) -> OsString {
match env::var_os(name) {
Some(v) => v,
Expand All @@ -53,32 +55,38 @@ pub fn env_var_os(name: &str) -> OsString {
}

/// `TARGET`
#[must_use]
pub fn target() -> String {
env_var("TARGET")
}

/// Check if target is windows-like.
#[must_use]
pub fn is_windows() -> bool {
target().contains("windows")
}

/// Check if target uses msvc.
#[must_use]
pub fn is_msvc() -> bool {
target().contains("msvc")
}

/// Check if target uses macOS.
#[must_use]
pub fn is_darwin() -> bool {
target().contains("darwin")
}

#[track_caller]
#[must_use]
pub fn python_command() -> Command {
let python_path = env_var("PYTHON");
Command::new(python_path)
}

#[track_caller]
#[must_use]
pub fn htmldocck() -> Command {
let mut python = python_command();
python.arg(source_root().join("src/etc/htmldocck.py"));
Expand All @@ -91,6 +99,7 @@ pub fn path<P: AsRef<Path>>(p: P) -> PathBuf {
}

/// Path to the root rust-lang/rust source checkout.
#[must_use]
pub fn source_root() -> PathBuf {
env_var("SOURCE_ROOT").into()
}
Expand Down Expand Up @@ -124,6 +133,7 @@ pub fn create_symlink<P: AsRef<Path>, Q: AsRef<Path>>(original: P, link: Q) {
}

/// Construct the static library name based on the platform.
#[must_use]
pub fn static_lib_name(name: &str) -> String {
// See tools.mk (irrelevant lines omitted):
//
Expand All @@ -148,6 +158,7 @@ pub fn static_lib_name(name: &str) -> String {
}

/// Construct the dynamic library name based on the platform.
#[must_use]
pub fn dynamic_lib_name(name: &str) -> String {
// See tools.mk (irrelevant lines omitted):
//
Expand All @@ -174,6 +185,7 @@ pub fn dynamic_lib_name(name: &str) -> String {
}
}

#[must_use]
pub fn dynamic_lib_extension() -> &'static str {
if is_darwin() {
"dylib"
Expand All @@ -185,23 +197,27 @@ pub fn dynamic_lib_extension() -> &'static str {
}

/// Generate the name a rust library (rlib) would have.
#[must_use]
pub fn rust_lib_name(name: &str) -> String {
format!("lib{name}.rlib")
}

/// Construct the binary name based on platform.
#[must_use]
pub fn bin_name(name: &str) -> String {
if is_windows() { format!("{name}.exe") } else { name.to_string() }
}

/// Return the current working directory.
#[must_use]
pub fn cwd() -> PathBuf {
env::current_dir().unwrap()
}

/// Use `cygpath -w` on a path to get a Windows path string back. This assumes that `cygpath` is
/// available on the platform!
#[track_caller]
#[must_use]
pub fn cygpath_windows<P: AsRef<Path>>(path: P) -> String {
let caller = panic::Location::caller();
let mut cygpath = Command::new("cygpath");
Expand All @@ -217,6 +233,7 @@ pub fn cygpath_windows<P: AsRef<Path>>(path: P) -> String {

/// Run `uname`. This assumes that `uname` is available on the platform!
#[track_caller]
#[must_use]
pub fn uname() -> String {
let caller = panic::Location::caller();
let mut uname = Command::new("uname");
Expand Down
1 change: 1 addition & 0 deletions src/tools/run-make-support/src/llvm_readobj.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ pub fn llvm_readobj() -> LlvmReadobj {

/// A `llvm-readobj` invocation builder.
#[derive(Debug)]
#[must_use]
pub struct LlvmReadobj {
cmd: Command,
}
Expand Down
1 change: 1 addition & 0 deletions src/tools/run-make-support/src/rustc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ pub fn aux_build() -> Rustc {

/// A `rustc` invocation builder.
#[derive(Debug)]
#[must_use]
pub struct Rustc {
cmd: Command,
}
Expand Down
1 change: 1 addition & 0 deletions src/tools/run-make-support/src/rustdoc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ pub fn rustdoc() -> Rustdoc {
}

#[derive(Debug)]
#[must_use]
pub struct Rustdoc {
cmd: Command,
}
Expand Down

0 comments on commit 921645c

Please sign in to comment.