Cleanup: delete `//@ pretty-expanded` directive #133470

jieyouxu · 2024-11-25T18:13:00Z

This PR removes the //@ pretty-expanded directive support in compiletest and removes its usage inside ui tests because it does not actually do anything, and its existence is itself misleading. This PR is split into two commits:

The first commit just drops pretty-expanded directive support in compiletest.
The second commit is created by sd '//@ pretty-expanded.*\n' '' tests/ui/**/*.rs¹, reblessing, and slightly adjusting some leading whitespace in a few tests.

We can tell this is fully removed because compiletest doesn't complain about unknown directive when running the ui test suite.

cc #23616

History

Originally, there was some effort to introduce more test coverage for -Z unpretty=expanded (in 2015 this was called --pretty=expanded). In Make it an error to not declare used features #23598, there was a flip from //@ no-pretty-expanded (opt-out of -Z unpretty=expanded test) to //@ pretty-expanded (opt-in to -Z unpretty=expanded test). This was needed because back then the dedicated tests/pretty ("pretty") test suite did not existed, and the pretty tests were grouped together under run-pass tests (I believe the ui test suite didn't exist back then either). Unfortunately, in this process the replacement //@ pretty-expanded directives contained a FIXME #23616 linking to There are very few tests for -Z unpretty expansion #23616. But this was arguably backwards and somewhat misleading, as noted in #23616:

The attribute is off by default and things just work if you don't
test it, people have not been adding the `pretty-expanded`
annotation to new tests even if it would work.

Which basically renders this useless.

Current status

As of Nov 2024, we have a dedicated pretty test suite, and some time over the years the split between run-pass into ui and pretty test suites caused all the //@ pretty-expanded in ui tests to do absolutely nothing: the compiletest logic for pretty-expanded only triggers in the pretty test suite, but none of the pretty tests use it. Oops.

Nobody remembers this, nobody uses this, it's misleading in ui tests. Let's get rid of this directive altogether.

Follow-ups

Yeet this directive from rustc-dev-guide docs. Remove pretty-expanded as it no longer exists rustc-dev-guide#2147

r? compiler

https://github.com/chmln/sd ↩

compiler-errors

LGTM. r=me when green; I replicated these changes why resetting the first PR and redoing the steps (deleting the directive, blessing, fixing tests w/ leading empty lines).

jieyouxu · 2024-11-25T18:41:28Z

Wait, is this load-bearing..? Let me double-check.
EDIT: it's because I'm dumb and deleted the other way around, lol.

Foreword ======== Let us begin the journey to rediscover what the `//@ pretty-expanded` directive does, brave traveller -- "My good friend, [..] when I wrote that passage, God and I knew what it meant. It is possible that God knows it still; but as for me, I have totally forgotten." -- Johann Paul Friedrich Richter, 1826 We must retrace the steps of those before us, for history shall guide us in the present and inform us of the future. The Past ======== Originally there was some effort to introduce more test coverage for `-Z unpretty=expanded` (in 2015 this was called `--pretty=expanded`). In [Make it an error to not declare used features rust-lang#23598][pr-23598], there was a flip from `//@ no-pretty-expanded` (opt-out of `-Z unpretty=expanded` test) to `//@ pretty-expanded` (opt-in to `-Z unpretty=expanded` test). This was needed because back then the dedicated `tests/pretty` ("pretty") test suite did not existed, and the pretty tests were grouped together under `run-pass` tests (I believe `ui` test suite didn't exist back then either). Unfortunately, in this process the replacement `//@ pretty-expanded` directives contained a `FIXME rust-lang#23616` linking to [There are very few tests for `-Z unpretty` expansion rust-lang#23616][issue-23616]. But this was arguably backwards and somewhat misleading, as noted in <rust-lang#23616 (comment)>: The attribute is off by default and things just work if you don't test it, people have not been adding the `pretty-expanded` annotation to new tests even if it would work. Which basically renders this useless. The Present =========== As of Nov 2024, we have a dedicated `pretty` test suite, and some time over the years the split between `run-pass` into `ui` and `pretty` test suites caused all of the `//@ pretty-expanded` in `ui` tests to do absolutely nothing -- the compiletest logic for `pretty-expanded` only triggered in the *pretty* test suite, but none of the pretty tests use it. Oops. The Future ========== Nobody remembers this, nobody uses this, it's misleading in ui tests. Let's get rid of this directive altogether. [pr-23598]: rust-lang#23598 [issue-23616]: rust-lang#23616

Done with ```bash sd '//@ pretty-expanded.*\n' '' tests/ui/**/*.rs ``` and ``` sd '//@pretty-expanded.*\n' '' tests/ui/**/*.rs ```

jieyouxu · 2024-11-25T18:52:49Z

I just failed to delete the actual expanded logic after the early return 😅 https://github.com/rust-lang/rust/compare/62630686e1fd04317273e10f46c219c2298d8658..95ff642797536f055c04ae2486bc8ab3156535b5

jieyouxu · 2024-11-25T20:18:22Z

PR CI is green, no test changes.
@bors r=@compiler-errors rollup

bors · 2024-11-25T20:18:26Z

📌 Commit 95ff642 has been approved by compiler-errors

It is now in the queue for this repository.

Cleanup: delete `//@ pretty-expanded` directive This PR removes the `//@ pretty-expanded` directive support in compiletest and removes its usage inside ui tests because it does not actually do anything, and its existence is itself misleading. This PR is split into two commits: 1. The first commit just drops `pretty-expanded` directive support in compiletest. 2. The second commit is created by `sd '//@ pretty-expanded.*\n' '' tests/ui/**/*.rs`[^1], reblessing, and slightly adjusting some leading whitespace in a few tests. We can tell this is fully removed because compiletest doesn't complain about unknown directive when running the `ui` test suite. cc rust-lang#23616 ### History Originally, there was some effort to introduce more test coverage for `-Z unpretty=expanded` (in 2015 this was called `--pretty=expanded`). In [Make it an error to not declare used features rust-lang#23598][pr-23598], there was a flip from `//@ no-pretty-expanded` (opt-out of `-Z unpretty=expanded` test) to `//@ pretty-expanded` (opt-in to `-Z unpretty=expanded` test). This was needed because back then the dedicated `tests/pretty` ("pretty") test suite did not existed, and the pretty tests were grouped together under `run-pass` tests (I believe the `ui` test suite didn't exist back then either). Unfortunately, in this process the replacement `//@ pretty-expanded` directives contained a `FIXME rust-lang#23616` linking to [There are very few tests for `-Z unpretty` expansion rust-lang#23616][issue-23616]. But this was arguably backwards and somewhat misleading, as noted in [rust-lang#23616](rust-lang#23616 (comment)): The attribute is off by default and things just work if you don't test it, people have not been adding the `pretty-expanded` annotation to new tests even if it would work. Which basically renders this useless. ### Current status As of Nov 2024, we have a dedicated `pretty` test suite, and some time over the years the split between `run-pass` into `ui` and `pretty` test suites caused all the `//@ pretty-expanded` in `ui` tests to do absolutely nothing: the compiletest logic for `pretty-expanded` only triggers in the *pretty* test suite, but none of the pretty tests use it. Oops. Nobody remembers this, nobody uses this, it's misleading in ui tests. Let's get rid of this directive altogether. [pr-23598]: rust-lang#23598 [issue-23616]: rust-lang#23616 ### Follow-ups - [x] Yeet this directive from rustc-dev-guide docs. rust-lang/rustc-dev-guide#2147 [^1]: https://github.com/chmln/sd r? compiler

Rollup of 28 pull requests Successful merges: - rust-lang#132605 (CI: increase timeout from 4h to 6h) - rust-lang#133042 (btree: add `{Entry,VacantEntry}::insert_entry`) - rust-lang#133070 (Lexer tweaks) - rust-lang#133136 (Support ranges in `<[T]>::get_many_mut()`) - rust-lang#133140 (Inline ExprPrecedence::order into Expr::precedence) - rust-lang#133248 (CI: split x86_64-msvc-ext job) - rust-lang#133282 (Shorten the `MaybeUninit` `Debug` implementation) - rust-lang#133304 (Revert diagnostics hack to fix ICE 132920) - rust-lang#133326 (Remove the `DefinitelyInitializedPlaces` analysis.) - rust-lang#133362 (No need to re-sort existential preds in relate impl) - rust-lang#133367 (Simplify array length mismatch error reporting (to not try to turn consts into target usizes)) - rust-lang#133394 (Bail on more errors in dyn ty lowering) - rust-lang#133410 (target check_consistency: ensure target feature string makes some basic sense) - rust-lang#133411 (the emscripten OS no longer exists on non-wasm targets) - rust-lang#133419 (Added a doc test for std::path::strip_prefix) - rust-lang#133430 (Tweak parameter mismatch explanation to not say `{unknown}`) - rust-lang#133435 (miri: disable test_downgrade_observe test on macOS) - rust-lang#133443 (Remove dead code stemming from the old effects desugaring (II)) - rust-lang#133449 (std: expose `const_io_error!` as `const_error!`) - rust-lang#133450 (remove "onur-ozkan" from users_on_vacation) - rust-lang#133454 (Update test expectations to accept LLVM 'initializes' attribute) - rust-lang#133458 (Fix `Result` and `Option` not getting a jump to def link generated) - rust-lang#133462 (Use ReadCache for archive reading in bootstrap) - rust-lang#133464 (std::thread: avoid leading whitespace in some panic messages) - rust-lang#133467 (tests: Add recursive associated type bound regression tests) - rust-lang#133470 (Cleanup: delete `//@ pretty-expanded` directive) - rust-lang#133473 (tests: Add regression test for recursive enum with Cow and Clone) - rust-lang#133481 (Disable `avr-rjmp-offset` on Windows for now) r? `@ghost` `@rustbot` modify labels: rollup

…llaumeGomez Rollup of 13 pull requests Successful merges: - rust-lang#133411 (the emscripten OS no longer exists on non-wasm targets) - rust-lang#133419 (Added a doc test for std::path::strip_prefix) - rust-lang#133430 (Tweak parameter mismatch explanation to not say `{unknown}`) - rust-lang#133443 (Remove dead code stemming from the old effects desugaring (II)) - rust-lang#133450 (remove "onur-ozkan" from users_on_vacation) - rust-lang#133454 (Update test expectations to accept LLVM 'initializes' attribute) - rust-lang#133462 (Use ReadCache for archive reading in bootstrap) - rust-lang#133464 (std::thread: avoid leading whitespace in some panic messages) - rust-lang#133467 (tests: Add recursive associated type bound regression tests) - rust-lang#133470 (Cleanup: delete `//@ pretty-expanded` directive) - rust-lang#133473 (tests: Add regression test for recursive enum with Cow and Clone) - rust-lang#133481 (Disable `avr-rjmp-offset` on Windows for now) - rust-lang#133495 (add test for alias-bound shadowing, rename folder) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#133470 - jieyouxu:ugly, r=compiler-errors Cleanup: delete `//@ pretty-expanded` directive This PR removes the `//@ pretty-expanded` directive support in compiletest and removes its usage inside ui tests because it does not actually do anything, and its existence is itself misleading. This PR is split into two commits: 1. The first commit just drops `pretty-expanded` directive support in compiletest. 2. The second commit is created by `sd '//@ pretty-expanded.*\n' '' tests/ui/**/*.rs`[^1], reblessing, and slightly adjusting some leading whitespace in a few tests. We can tell this is fully removed because compiletest doesn't complain about unknown directive when running the `ui` test suite. cc rust-lang#23616 ### History Originally, there was some effort to introduce more test coverage for `-Z unpretty=expanded` (in 2015 this was called `--pretty=expanded`). In [Make it an error to not declare used features rust-lang#23598][pr-23598], there was a flip from `//@ no-pretty-expanded` (opt-out of `-Z unpretty=expanded` test) to `//@ pretty-expanded` (opt-in to `-Z unpretty=expanded` test). This was needed because back then the dedicated `tests/pretty` ("pretty") test suite did not existed, and the pretty tests were grouped together under `run-pass` tests (I believe the `ui` test suite didn't exist back then either). Unfortunately, in this process the replacement `//@ pretty-expanded` directives contained a `FIXME rust-lang#23616` linking to [There are very few tests for `-Z unpretty` expansion rust-lang#23616][issue-23616]. But this was arguably backwards and somewhat misleading, as noted in [rust-lang#23616](rust-lang#23616 (comment)): The attribute is off by default and things just work if you don't test it, people have not been adding the `pretty-expanded` annotation to new tests even if it would work. Which basically renders this useless. ### Current status As of Nov 2024, we have a dedicated `pretty` test suite, and some time over the years the split between `run-pass` into `ui` and `pretty` test suites caused all the `//@ pretty-expanded` in `ui` tests to do absolutely nothing: the compiletest logic for `pretty-expanded` only triggers in the *pretty* test suite, but none of the pretty tests use it. Oops. Nobody remembers this, nobody uses this, it's misleading in ui tests. Let's get rid of this directive altogether. [pr-23598]: rust-lang#23598 [issue-23616]: rust-lang#23616 ### Follow-ups - [x] Yeet this directive from rustc-dev-guide docs. rust-lang/rustc-dev-guide#2147 [^1]: https://github.com/chmln/sd r? compiler

jieyouxu added A-testsuite Area: The testsuite used to check the correctness of rustc A-compiletest Area: The compiletest test runner labels Nov 25, 2024

rustbot assigned compiler-errors Nov 25, 2024

jieyouxu changed the title ~~compiletest: remove pretty-expanded directive and infra~~ Cleanup: delete //@ pretty-expanded directive Nov 25, 2024

jieyouxu mentioned this pull request Nov 25, 2024

Remove pretty-expanded as it no longer exists rust-lang/rustc-dev-guide#2147

Merged

compiler-errors reviewed Nov 25, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

jieyouxu added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 25, 2024

jieyouxu and others added 2 commits November 26, 2024 02:50

tests: remove //@ pretty-expanded usages

95ff642

Done with ```bash sd '//@ pretty-expanded.*\n' '' tests/ui/**/*.rs ``` and ``` sd '//@pretty-expanded.*\n' '' tests/ui/**/*.rs ```

jieyouxu force-pushed the ugly branch from 6263068 to 95ff642 Compare November 25, 2024 18:51

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 25, 2024

jhpratt mentioned this pull request Nov 26, 2024

Rollup of 28 pull requests #133490

Closed

GuillaumeGomez mentioned this pull request Nov 26, 2024

Rollup of 13 pull requests #133500

Merged

bors merged commit 0d9a57d into rust-lang:master Nov 26, 2024
6 checks passed

rustbot added this to the 1.85.0 milestone Nov 26, 2024

jieyouxu deleted the ugly branch November 26, 2024 20:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup: delete `//@ pretty-expanded` directive #133470

Cleanup: delete `//@ pretty-expanded` directive #133470

jieyouxu commented Nov 25, 2024 •

edited

Loading

compiler-errors left a comment

This comment has been minimized.

jieyouxu commented Nov 25, 2024 •

edited

Loading

jieyouxu commented Nov 25, 2024

jieyouxu commented Nov 25, 2024

bors commented Nov 25, 2024

Cleanup: delete //@ pretty-expanded directive #133470

Cleanup: delete //@ pretty-expanded directive #133470

Conversation

jieyouxu commented Nov 25, 2024 • edited Loading

History

Current status

Follow-ups

Footnotes

compiler-errors left a comment

Choose a reason for hiding this comment

This comment has been minimized.

jieyouxu commented Nov 25, 2024 • edited Loading

jieyouxu commented Nov 25, 2024

jieyouxu commented Nov 25, 2024

bors commented Nov 25, 2024

Cleanup: delete `//@ pretty-expanded` directive #133470

Cleanup: delete `//@ pretty-expanded` directive #133470

jieyouxu commented Nov 25, 2024 •

edited

Loading

jieyouxu commented Nov 25, 2024 •

edited

Loading