Skip to content

compiletest: Make SUGGESTION annotations viral #139618

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 13, 2025

Conversation

petrochenkov
Copy link
Contributor

If one of them is expected in a test file, then others should be annotated as well, in the same way as with HELPs and NOTEs.
This doesn't require much of an additional annotation burden, but simplifies the rules.

r? @jieyouxu

@rustbot rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 10, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 10, 2025

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@@ -19,6 +19,7 @@ fn fn_mut() -> _ {
let x = String::new();
//~^ HELP: consider changing this to be mutable
//~| NOTE binding `x` declared here
//~| SUGGESTION mut
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rule of thumb to maximize annotations while minimizing test changes: if just one additional SUGGESTION is required, then I add it, otherwise I add //@ dont-require-annotations:SUGGESTION.
I'm going to do the same thing when adding HELPs and NOTEs in a next PR.

@jieyouxu
Copy link
Member

I'll look at this tmrw. I'm planning to write the next issue of the test infra internal blog post this Saturday, I'll include this one too re. significant test writing changes.

@rustbot rustbot added the A-rustc-dev-guide Area: rustc-dev-guide label Apr 10, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 10, 2025

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @jieyouxu, @Kobzol

@petrochenkov
Copy link
Contributor Author

So I wrote the docs and then realized that they are wrong, because //@ dont-require-annotations: NOTE doesn't actually work because of the whitespace in the value.
So I've added one more commit trimming values for this directive as well.

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes and docs look good, thanks for the consistency improvement!

@jieyouxu
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 11, 2025

📌 Commit 06dd9e2 has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 11, 2025
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Apr 11, 2025
compiletest: Make `SUGGESTION` annotations viral

If one of them is expected in a test file, then others should be annotated as well, in the same way as with `HELP`s and `NOTE`s.
This doesn't require much of an additional annotation burden, but simplifies the rules.

r? `@jieyouxu`
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 12, 2025
Rollup of 5 pull requests

Successful merges:

 - rust-lang#139107 (std: make `cmath` functions safe)
 - rust-lang#139618 (compiletest: Make `SUGGESTION` annotations viral)
 - rust-lang#139677 (Fix profiler_builtins build script to handle full path to profiler lib)
 - rust-lang#139683 (Use `with_native_path` for Windows)
 - rust-lang#139691 (Document that `opt-dist` requires metrics to be enabled)

r? `@ghost`
`@rustbot` modify labels: rollup
ChrisDenton added a commit to ChrisDenton/rust that referenced this pull request Apr 13, 2025
compiletest: Make `SUGGESTION` annotations viral

If one of them is expected in a test file, then others should be annotated as well, in the same way as with `HELP`s and `NOTE`s.
This doesn't require much of an additional annotation burden, but simplifies the rules.

r? ``@jieyouxu``
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 13, 2025
…enton

Rollup of 11 pull requests

Successful merges:

 - rust-lang#138972 (std: Fix build for NuttX targets)
 - rust-lang#139177 (Use -C target-cpu=z13 on s390x vector test)
 - rust-lang#139511 (libtest: Pass the test's panic payload as Option instead of Result)
 - rust-lang#139605 (update ```miniz_oxide``` to 0.8.8)
 - rust-lang#139618 (compiletest: Make `SUGGESTION` annotations viral)
 - rust-lang#139677 (Fix profiler_builtins build script to handle full path to profiler lib)
 - rust-lang#139683 (Use `with_native_path` for Windows)
 - rust-lang#139695 (compiletest: consistently use `camino::{Utf8Path,Utf8PathBuf}` throughout)
 - rust-lang#139710 (Move `args` into `std::sys`)
 - rust-lang#139721 (End all lines in src/stage0 with trailing newline)
 - rust-lang#139726 (Move `select_unpredictable` to the `hint` module)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 13, 2025
…enton

Rollup of 10 pull requests

Successful merges:

 - rust-lang#138972 (std: Fix build for NuttX targets)
 - rust-lang#139177 (Use -C target-cpu=z13 on s390x vector test)
 - rust-lang#139511 (libtest: Pass the test's panic payload as Option instead of Result)
 - rust-lang#139605 (update ```miniz_oxide``` to 0.8.8)
 - rust-lang#139618 (compiletest: Make `SUGGESTION` annotations viral)
 - rust-lang#139677 (Fix profiler_builtins build script to handle full path to profiler lib)
 - rust-lang#139683 (Use `with_native_path` for Windows)
 - rust-lang#139710 (Move `args` into `std::sys`)
 - rust-lang#139721 (End all lines in src/stage0 with trailing newline)
 - rust-lang#139726 (Move `select_unpredictable` to the `hint` module)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 423e7b8 into rust-lang:master Apr 13, 2025
6 checks passed
@rustbot rustbot added this to the 1.88.0 milestone Apr 13, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 13, 2025
Rollup merge of rust-lang#139618 - petrochenkov:virsugg, r=jieyouxu

compiletest: Make `SUGGESTION` annotations viral

If one of them is expected in a test file, then others should be annotated as well, in the same way as with `HELP`s and `NOTE`s.
This doesn't require much of an additional annotation burden, but simplifies the rules.

r? ```@jieyouxu```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiletest Area: The compiletest test runner A-rustc-dev-guide Area: rustc-dev-guide A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants