Remove most box syntax uses from the testsuite except for src/test/ui/issues

[est31]

Removes most box syntax uses from the testsuite outside of the src/test/ui/issues directory. The goal was to only change tests where box syntax is an implementation detail instead of the actual feature being tested. So some tests were left out, like the regression test for #87935, or tests where the obtained error message changed significantly.

Mostly this replaces box syntax with Box::new, but there are some minor drive by improvements, like formatting improvements or assert_eq instead of assert!( == ).

Prior PR that removed box syntax from the compiler and tools: #87781

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[jyn514]

I have the same comment as #87781 (comment) - This seems like a really bad idea. How will you possibly ensure that it's testing the same thing that it did before?

[est31]

@jyn514 that's a valid concern, and I've had the same one (see PR description). I checked stuff like comments in each test file as well as its name, directory, etc. For most tests it's obvious that they don't have anything to do with box syntax but are testing a different feature. Say some test which adds a lint pass to the compiler but uses box syntax for adding the pass (which used to exist in the compiler as well until #87781). I suggest you give this PR a proper review and see for yourself. If there are files where a reviewer wants to keep box syntax, I'm ok with keeping it. I've already done so in some instances where the test wouldn't have made sense had I used Box::new instead. Signs of this are e.g. error messages changing, or errors being introduced to run-pass tests, etc. In one instance a huge array was created using box syntax. I kept box syntax in there because it might run into a similar issue as in #87781 (comment) , which is me being very cautious to not regress on test suite speed.

Also, even if it tests a different feature, which in 99% of cases it doesn't, I'd argue that removing box syntax doesn't reduce overall diversity of the test suite, and the test suite still remains a highly useful one for a Rust compiler. box syntax is not a core part of the Rust language, nor is it a particularly complicated feature with complex interactions with other features.

[jyn514]

I suggest you give this PR a proper review and see for yourself.

Well I would, except that this is a +1,074 −1,468 diff. That seems like an awful lot of churn for not much benefit.

[Mark-Simulacrum]

I've already done so in some instances where the test wouldn't have made sense had I used Box::new instead

FWIW, it seems good to file a separate PR that adds comments to tests saying explicitly "this is intentional box syntax", given that it sounds like many tests aren't intentionally using it.

For this PR itself, I would appreciate it if instead of just removing the feature gate line you instead replace it with an empty line -- that will mean that stderr files aren't changed (line numbers) in many of the cases that they're getting changed today. More broadly, in a few of the cases it looks like rustfmt or similar is rewrapping and causing stderr changes and such, which seems unnecessary (even more needless churn).

I share @jyn514's concern broadly that it doesn't feel like we are currently on any trajectory to need to remove box syntax, and so creating work for that (e.g., reviewing this PR) doesn't seem like a good idea. #87781 seemed OK since it was focused on "read" code where it's nice for average users to not encounter a syntax not really widely used, but tests aren't generally read much in practice so it seems less useful here.

[est31]

FWIW, it seems good to file a separate PR that adds comments to tests saying explicitly "this is intentional box syntax", given that it sounds like many tests aren't intentionally using it.

Good idea. I should maybe add something like // box-syntax-intentional or something when it's intended part of the test. Note though that if it's an error containing test file it'll cause some churn however, and you expressed dismay over churn. Is that churn okay?

I would appreciate it if instead of just removing the feature gate line you instead replace it with an empty line

In some tests this might work, especially when that feature wasn't at the top. However, when it's at the top, tidy will complain about leading empty lines. How do you feel about me adding a comment?

it doesn't feel like we are currently on any trajectory to need to remove box syntax

So we aren't removing box syntax because it's used in the compiler, but it shouldn't be removed from the compiler because box syntax isn't being removed? That's a circular argument.

in a few of the cases it looks like rustfmt or similar is rewrapping and causing stderr changes and such, which seems unnecessary (even more needless churn).

Yeah in a few places I couldn't endure the sometimes outdated formatting, like putting the entire main function onto one line. There was no tool involvement however. Also note that box syntax is sugar, and sugar is sweet. So adding Box::new to such a situation will make it so much worse that I thought I'd improve formatting a little. I never made changes to the entire file though or such, but always kept changes close to the uses of box syntax. Also note that sometimes a solution is even required because addition of Box::new would make lines longer than 100 chars which tidy complains about.

---

Anyways, here are some stats on the improvement this PR brings. Before the PR:

$ rg "box_syntax" src/test/ | wc -l
373

After the PR:

$ rg "box_syntax" src/test/ | wc -l
84

[jyn514]

So we aren't removing box syntax because it's used in the compiler, but it shouldn't be removed from the compiler because box syntax isn't being removed? That's a circular argument.

It shouldn't be removed from the compiler because it has no replacement with the same effect. It is not just syntax sugar, it has a semantic difference.

[Mark-Simulacrum]

In some tests this might work, especially when that feature wasn't at the top. However, when it's at the top, tidy will complain about leading empty lines. How do you feel about me adding a comment?

I'd prefer to reshuffle lines - usually you can add a newline after the feature gates for example, and this avoids any changes to stderr files. I'd expect very few cases to need more than that, and in those it's probably OK to review stderr bumps by hand.

So we aren't removing box syntax because it's used in the compiler, but it shouldn't be removed from the compiler because box syntax isn't being removed? That's a circular argument.

This is not my (intended) argument. I am saying that in the compiler (and tools), removing the use of a "interesting" piece of code in favor of something more standard (such as using Box::new instead of box syntax) is generally a good cleanup. But in tests, which are mostly write-once and don't really benefit from cleanup as such, this churn is not really aiding a goal unless there is an overall agreement that some feature (such as box_syntax) will just be removed or replaced; right now it seems somewhat unlikely that this will happen in the near future at least.

Yeah in a few places I couldn't endure the sometimes outdated formatting, like putting the entire main function onto one line. There was no tool involvement however. Also note that box syntax is sugar, and sugar is sweet. So adding Box::new to such a situation will make it so much worse that I thought I'd improve formatting a little. I never made changes to the entire file though or such, but always kept changes close to the uses of box syntax. Also note that sometimes a solution is even required because addition of Box::new would make lines longer than 100 chars which tidy complains about.

To some extent at least personally I'd rather not have to spend review time making sure that the test files are still testing the same thing when there's .stderr changes just because the original test was "ugly". It feels reasonable to ask to not do drive-by reformatting when that strictly hurts review.

If we're running against tidy limits, then a small reformat is OK, but I'd expect the vast majority of current cases in this PR to not run up against this.

---

I'll just reiterate that all the discussion about small improvements (e.g., ways of avoiding stderr file churn) may be a bit beside the point, and I don't want to waste your time on them if I at least am not really sure we should accept this PR in any form. I at least am not really feeling up for reviewing ~289 tests to make sure they're all "benign" in terms of not actually depending on box syntax -- this seems like a lot of work and not really easy work either.

To some extent we could just say "seems OK" and not worry about that review, but since I'm not yet clear on the benefits of this PR at this time I'm not sold that investing in removing box syntax from tests is a good investment.

[est31]

I've given the stderr files a skim. From that skim I got the feeling that the biggest cause for churn are the line numbers, the second biggest the snippet churn (when it quotes a line that included box which now includes Box). Almost no "formatting" churn I can see.

I have a proposal. Could you check out the first two commits of this PR? They are removing box syntax from non-ui tests and don't contain any stderr churn. I offer to put them into a separate PR if wanted. Maybe it's a bit more digestable for review if it's split up.

But I do agree that it's best to wait until there is general sentiment to merge this PR before I invest more time into this. I'm ready to reduce the line number churn to a minimum. I think it should be easy to integrate into the git history thanks to git absorb, so blames won't be affected.

[est31]

@Mark-Simulacrum so do you want this PR or do you not want it?

[Mark-Simulacrum]

It's in my review queue, I just haven't had a chance yet to take a look at your latest comments and provide feedback.

[est31]

@Mark-Simulacrum okay, thanks for the feedback, take your time! 👍

[Mark-Simulacrum]

OK, I've thought some more about this and looked at the first couple commits.

I don't personally have time to review this PR for each test. On the other hand, it seems pretty unlikely we'll ever be able to really be sure (even if I or someone had that review time). In that sense, maybe we should just accept this and hope that we're not losing any valuable tests. It seems pretty unlikely that we'd have too much dependence on box syntax in the majority of tests.

I think we should still try to decrease the amount of churn by squashing everything into one commit (easier to filter out if needed and doesn't really make review any harder), and doing some work on avoiding line churn. But that's mostly just decreasing the amount of interference with git blame.

I think we should also drop anything that's not just line churn into a separate PR -- that should be reviewed more closely, and hopefully only a very small number of tests change in that way.

[est31]

@Mark-Simulacrum sounds like a plan. I'll squash the PR. Should I try to reduce the line number churn as offered or can it be kept?

[Mark-Simulacrum]

I think the easier cases -- e.g., no need to add comments or other weirdness, just shuffling some lines or adding a blank one -- we should, since that'll make the impact of the PR on blame and such lower, but otherwise no need to invest too much time in the complicated cases.

[est31]

I've changed the PR to reduce the line number spam by adding newlines in various places, and also squashed the PR to a single commit. I've also removed the changes of the src/test/ui/reachable/expr_box.rs test because it seemed too box syntax specific to me.

[est31]

re-r? @Mark-Simulacrum

[bors]

⌛ Testing commit 2fd87ffba192f2c729ba9608b50f9511628edc03 with merge 946e34a6da6012c67540cc532182146e409875ae...

[bors]

💔 Test failed - checks-actions

[est31]

Hmm seems I've been overzealous in replacing box foo with Box::new(foo) and replaced a pattern usage, which caused compilation to fail, and I didn't notice it because i couldn't test it. I manually copied the example to the playground and it compiles now but I still can't test it.

[Mark-Simulacrum]

@bors r+

[bors]

📌 Commit 6550021 has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit 6550021 with merge c09d637...

[bjorn3]

The comments in this test contain ancient match syntax.

[est31]

Yeah the PR touched a bunch of extremely ancient tests, observable on things like different code styles or such.

[bors]

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing c09d637 to master...

[rust-timer]

Finished benchmarking commit (c09d637): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression