Skip to content

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

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 2 commits into from
Nov 26, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
compiletest: remove pretty-expanded directive and infra
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 #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 #23616` linking to [There are very few tests for `-Z unpretty`
expansion #23616][issue-23616]. But this was arguably backwards and
somewhat misleading, as noted in
<#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]: #23598
[issue-23616]: #23616
  • Loading branch information
jieyouxu committed Nov 25, 2024
commit f62753f84f355d2c8c67b257db6746c7ae58be2e
1 change: 0 additions & 1 deletion src/tools/compiletest/src/directive-list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,6 @@ const KNOWN_DIRECTIVE_NAMES: &[&str] = &[
"only-x86_64-unknown-linux-gnu",
"pp-exact",
"pretty-compare-only",
"pretty-expanded",
"pretty-mode",
"reference",
"regex-error-pattern",
Expand Down
5 changes: 0 additions & 5 deletions src/tools/compiletest/src/header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,6 @@ pub struct TestProps {
// a proc-macro and needs `#![crate_type = "proc-macro"]`. This ensures
// that the aux file is compiled as a `proc-macro` and not as a `dylib`.
pub no_prefer_dynamic: bool,
// Run -Zunpretty expanded when running pretty printing tests
pub pretty_expanded: bool,
// Which pretty mode are we testing with, default to 'normal'
pub pretty_mode: String,
// Only compare pretty output and don't try compiling
Expand Down Expand Up @@ -218,7 +216,6 @@ mod directives {
pub const DONT_CHECK_COMPILER_STDOUT: &'static str = "dont-check-compiler-stdout";
pub const DONT_CHECK_COMPILER_STDERR: &'static str = "dont-check-compiler-stderr";
pub const NO_PREFER_DYNAMIC: &'static str = "no-prefer-dynamic";
pub const PRETTY_EXPANDED: &'static str = "pretty-expanded";
pub const PRETTY_MODE: &'static str = "pretty-mode";
pub const PRETTY_COMPARE_ONLY: &'static str = "pretty-compare-only";
pub const AUX_BIN: &'static str = "aux-bin";
Expand Down Expand Up @@ -278,7 +275,6 @@ impl TestProps {
dont_check_compiler_stderr: false,
compare_output_lines_by_subset: false,
no_prefer_dynamic: false,
pretty_expanded: false,
pretty_mode: "normal".to_string(),
pretty_compare_only: false,
forbid_output: vec![],
Expand Down Expand Up @@ -425,7 +421,6 @@ impl TestProps {
&mut self.dont_check_compiler_stderr,
);
config.set_name_directive(ln, NO_PREFER_DYNAMIC, &mut self.no_prefer_dynamic);
config.set_name_directive(ln, PRETTY_EXPANDED, &mut self.pretty_expanded);

if let Some(m) = config.parse_name_value_directive(ln, PRETTY_MODE) {
self.pretty_mode = m;
Expand Down
16 changes: 0 additions & 16 deletions src/tools/compiletest/src/runtest/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,21 +84,5 @@ impl TestCx<'_> {
if !proc_res.status.success() {
self.fatal_proc_rec("pretty-printed source does not typecheck", &proc_res);
}

if !self.props.pretty_expanded {
return;
}

// additionally, run `-Zunpretty=expanded` and try to build it.
let proc_res = self.print_source(ReadFrom::Path, "expanded");
if !proc_res.status.success() {
self.fatal_proc_rec("pretty-printing (expanded) failed", &proc_res);
}

let ProcRes { stdout: expanded_src, .. } = proc_res;
let proc_res = self.typecheck_source(expanded_src);
if !proc_res.status.success() {
self.fatal_proc_rec("pretty-printed source (expanded) does not typecheck", &proc_res);
}
}
}