Skip to content
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

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

Merged
merged 1 commit into from
Sep 26, 2021

Conversation

est31
Copy link
Member

@est31 est31 commented Aug 25, 2021

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
Copy link
Collaborator

r? @Mark-Simulacrum

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 25, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Aug 25, 2021

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
Copy link
Member Author

est31 commented Aug 25, 2021

@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
Copy link
Member

jyn514 commented Aug 25, 2021

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
Copy link
Member

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
Copy link
Member Author

est31 commented Aug 25, 2021

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
Copy link
Member

jyn514 commented Aug 25, 2021

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
Copy link
Member

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
Copy link
Member Author

est31 commented Aug 25, 2021

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
Copy link
Member Author

est31 commented Aug 29, 2021

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

@Mark-Simulacrum
Copy link
Member

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
Copy link
Member Author

est31 commented Aug 30, 2021

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

@Mark-Simulacrum
Copy link
Member

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.

@Mark-Simulacrum Mark-Simulacrum 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 Sep 4, 2021
@est31
Copy link
Member Author

est31 commented Sep 4, 2021

@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
Copy link
Member

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
Copy link
Member Author

est31 commented Sep 5, 2021

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
Copy link
Member Author

est31 commented Sep 5, 2021

re-r? @Mark-Simulacrum

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 25, 2021
@bors
Copy link
Contributor

bors commented Sep 25, 2021

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

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Sep 26, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 26, 2021
@est31
Copy link
Member Author

est31 commented Sep 26, 2021

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
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 26, 2021

📌 Commit 6550021 has been approved by Mark-Simulacrum

@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 Sep 26, 2021
@bors
Copy link
Contributor

bors commented Sep 26, 2021

⌛ Testing commit 6550021 with merge c09d637...

assert_eq!(a, 10);
}
})
_ { panic!(); }
}*/
}
Copy link
Member

Choose a reason for hiding this comment

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

The comments in this test contain ancient match syntax.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@bors
Copy link
Contributor

bors commented Sep 26, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 26, 2021
@bors bors merged commit c09d637 into rust-lang:master Sep 26, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 26, 2021
@rust-timer
Copy link
Collaborator

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

@est31 est31 mentioned this pull request Feb 3, 2022
est31 added a commit to est31/rust that referenced this pull request Aug 7, 2022
Prior work, notably 6550021 from rust-lang#88316
has removed box syntax from most of the testsuite. However,
some tests were left out.
This commit removes box_syntax uses from more locations in src/test.
Some tests that are very box syntax specific are not being migrated.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 7, 2022
…ulacrum

Remove even more box syntax uses from src/test

Prior work, notably rust-lang#88316 has removed box syntax from most of the testsuite.
However, some tests were left out.
This commit removes box_syntax uses from more locations in src/test.
This migrates the tests where `box` is mostly an "implementation detail" and not the primary thing being tested by the test.
Furthermore, some tests from the mir-opt test suite are not being migrated.
@est31 est31 mentioned this pull request Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants