run-make-support: add #[must_use] to helpers where suitable
#125703
Assignees
Labels
A-run-make
Area: port run-make Makefiles to rmake.rs
A-testsuite
Area: The testsuite used to check the correctness of rustc
C-enhancement
Category: An issue proposing an enhancement or a PR with one.
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
Comments
jieyouxu
added
A-testsuite
rustbot
added
the
needs-triage
jieyouxu
removed
the
needs-triage
|
Maybe even consider drop bombs for command wrappers? |
jieyouxu
added a commit
to jieyouxu/rust
that referenced
this issue
May 31, 2024
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.
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
May 31, 2024
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.
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Jun 3, 2024
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.
try-job: x86_64-msvc
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Jun 3, 2024
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.
try-job: x86_64-msvc
|
Closing as mostly addressed in #125752. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
For command wrappers like
Rustc, they often have intermediate helper methods and actual "terminal" functions that executes the built command (e.g.run,run_fail,run_fail_assert_exit_code). Currently, it is easy to forget to call "terminal" functions.For example,
does not actually execute and can unexpectedly silently pass. We should annotate intermediate helper methods with
#[must_use]to ensure the built command is consumed, i.e. by executing the command.The text was updated successfully, but these errors were encountered: