Format tests and benches with rustfmt #2097

dtolnay · 2022-04-30T18:41:31Z

4 commits:

Add rustfmt::skip to some files — handwritten; some files that rustfmt crashes on, or that I could tell have meaningful bespoke formatting that shouldn't be autoformatted.
Rustfmt tests/**/*.rs — generated by find . name '*.rs' | xargs rustfmt +nightly --edition=2021
Bless stderr files to update line numbers — generated by compiletest
Manual fixes to compiletest annotations — handwritten, changing //~ ERROR to //~^ ERROR and similar.

Four of the files being skipped here are because rustfmt is buggy (see the error messages below). The others have clearly preferable manual formatting. error[internal]: left behind trailing whitespace --> /git/miri/tests/compile-fail/validity/transmute_through_ptr.rs:18:18:1 | 18 | | ^^^^ | warning: rustfmt has failed to format. See previous 1 errors. error[internal]: left behind trailing whitespace --> /git/miri/tests/compile-fail/stacked_borrows/illegal_read2.rs:10:10:1 | 10 | | ^^^^ | warning: rustfmt has failed to format. See previous 1 errors. error[internal]: left behind trailing whitespace --> /git/miri/tests/compile-fail/stacked_borrows/illegal_read5.rs:15:15:1 | 15 | | ^^^^ | warning: rustfmt has failed to format. See previous 1 errors. error[internal]: left behind trailing whitespace --> /git/miri/tests/compile-fail/stacked_borrows/illegal_read1.rs:10:10:1 | 10 | | ^^^^ | warning: rustfmt has failed to format. See previous 1 errors.

Generated by: find . name '*.rs' | xargs rustfmt +nightly --edition=2021

bors · 2022-05-01T07:08:00Z

☔ The latest upstream changes (presumably #2095) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung · 2022-05-01T07:37:57Z

I think formatting those files only really makes sense if we then also check their formatting in the corresponding CI job. How hard would that be?

RalfJung · 2022-05-01T07:39:12Z

test-cargo-miri/tests/test.rs

+fn do_panic() {
+    // In large, friendly letters :)


Suggested change

fn do_panic() {

// In large, friendly letters :)

fn do_panic() {

What rustfmt does here makes no sense (the comment comments on the fn signature, not the body). It's also not a critical comment...

RalfJung · 2022-05-01T07:41:29Z

tests/compile-fail/intrinsics/copy_null.rs

-    unsafe { copy_nonoverlapping(std::ptr::null(), ptr, 0); } //~ ERROR: memory access failed: null pointer is not a valid pointer
+    unsafe {
+        copy_nonoverlapping(std::ptr::null(), ptr, 0);
+    } //~^ ERROR: memory access failed: null pointer is not a valid pointer


IMO for cases like this (of which we have plenty) it'd be better to keep the comment in the same line as the function call, rather than have it after the unsafe block.

So, I did this based on line length. Notice for example tests/compile-fail/box-cell-alias.rs which has a shorter function call and shorter comment, the change is like this:

- unsafe { (*ptr).set(20); } //~ ERROR does not exist in the borrow stack + unsafe { + (*ptr).set(20); //~ ERROR does not exist in the borrow stack + }

Basically if the comment fit without exceeding ~80 columns, I put it on the same line, otherwise the next line. It's clear to compiletest from the //~ vs //~^ whether the expected error is talking about the current line vs previous line.

The alternatives I see are the following, but I find both less good than what the PR is currently doing:

// very long line unsafe { copy_nonoverlapping(std::ptr::null(), ptr, 0); //~ ERROR: memory access failed: null pointer is not a valid pointer }

// more vertical space for no real benefit, I think unsafe { copy_nonoverlapping(std::ptr::null(), ptr, 0); //~^ ERROR: memory access failed: null pointer is not a valid pointer }

I find both better than what the PR is doing. 🤷
Having the comment after the } is rather odd IMO, it goes against the structure of the code.

Okay! Will update.

RalfJung

I am in general on board with formatting our tests, bud sadly rustfmt likes to move comments around in a way that changes their meaning by attaching them to different parts of the code. So this needs to be checked by hand.

GH doesn't even render the diffs of the last tests any more, so it'd probably be better to do this over multiple PRs.

RalfJung · 2022-05-01T07:42:35Z

tests/compile-fail/stacked_borrows/illegal_read2.rs

@@ -1,6 +1,7 @@
 // A callee may not read the destination of our `&mut` without
 // us noticing.

+#[rustfmt::skip]


Why does this one need skipping?

This is mentioned in the commit message — it's because rustfmt is buggy and produces a noisy error message whenever this file is formatted.

error[internal]: left behind trailing whitespace --> tests/compile-fail/stacked_borrows/illegal_read2.rs:10:10:1 | 10 | | ^^^^ | warning: rustfmt has failed to format. See previous 1 errors.

I could alternatively try to tweak the contents of this file to work around the rustfmt bug.

Oh, strange.
Ideally the attribute would come with a comment referencing the rustfmt bugreport. But just a quick // work around rustfmt bug would also do it.

RalfJung · 2022-05-01T07:43:34Z

tests/compile-fail/stacked_borrows/illegal_read6.rs

-    let _val = *raw; //~ ERROR borrow stack
-} }
+fn main() {
+    unsafe {


I'm not a fan of the double-indentation rustfmt introduces here... but I guess it doesn't really matter since these are just tests.

RalfJung · 2022-05-01T07:44:03Z

tests/compile-fail/stacked_borrows/illegal_write1.rs

+        let x: *mut u32 = xref as *const _ as *mut _;
+        unsafe {
+            *x = 42;
+        } // invalidates shared ref, activates raw


The comment should be with the assignment, not the end of the block

RalfJung · 2022-05-01T07:44:50Z

tests/compile-fail/unaligned_pointers/unaligned_ptr3.rs

@@ -2,7 +2,8 @@
 // compile-flags: -Zmiri-disable-validation -Zmiri-disable-stacked-borrows

 fn main() {
-    for _ in 0..10 { // Try many times as this might work by chance.
+    for _ in 0..10 {
+        // Try many times as this might work by chance.


Another case of rusfmt moving a comment to the wrong spot (the next few tests in the diff have the same issue)

RalfJung · 2022-05-01T07:45:31Z

tests/compile-fail/validity/transmute_through_ptr.rs

-    unsafe { *x = 44; } // out-of-bounds enum tag
+    unsafe {
+        *x = 44;
+    } // out-of-bounds enum tag


Comment should be with assignment

RalfJung · 2022-05-01T07:46:12Z

tests/run-pass/concurrency/linux-futex.rs

+                    10, // Wake up at most 10 threads.
+                ),
+                1
+            ); // Woken up one thread.


Comment moved to wrong line

RalfJung · 2022-05-01T07:46:22Z

tests/run-pass/concurrency/linux-futex.rs

+                    0b1001, // bitset
+                ),
+                0
+            ); // Didn't match any thread.


Another comment on the wrong line

RalfJung · 2022-05-01T07:46:30Z

tests/run-pass/concurrency/linux-futex.rs

+                    0b0110, // bitset
+                ),
+                1
+            ); // Woken up one thread.


And another one

RalfJung · 2022-05-01T07:47:20Z

tests/run-pass/heap_allocator.rs

+    for &size in &[2, 8, 64] {
+        // size less than and bigger than alignment
+        for &align in &[4, 8, 16, 32] {
+            // Be sure to cover less than and bigger than `MIN_ALIGN` for all architectures


These comments also moved to the wrong spot

dtolnay

I am coming back to this after getting derailed by an apartment move.

Let me close this PR and I'll follow up in multiple more easily reviewable PRs, to incorporate all of the feedback above, and to avoid GitHub concealing the diffs beyond a certain number of files.

Add rustfmt::skip to some files Extracted from #2097. Five of the files being skipped here are because rustfmt is buggy (rust-lang/rustfmt#5391; see the error messages below). The other two have clearly preferable manual formatting. ```console error[internal]: left behind trailing whitespace --> tests/fail/validity/transmute_through_ptr.rs:18:18:1 | 18 | | ^^^^ | warning: rustfmt has failed to format. See previous 1 errors. error[internal]: left behind trailing whitespace --> tests/fail/stacked_borrows/illegal_read2.rs:10:10:1 | 10 | | ^^^^ | warning: rustfmt has failed to format. See previous 1 errors. error[internal]: left behind trailing whitespace --> tests/fail/stacked_borrows/illegal_read5.rs:15:15:1 | 15 | | ^^^^ | warning: rustfmt has failed to format. See previous 1 errors. error[internal]: left behind trailing whitespace --> tests/fail/stacked_borrows/illegal_read1.rs:10:10:1 | 10 | | ^^^^ | warning: rustfmt has failed to format. See previous 1 errors. error[internal]: left behind trailing whitespace --> /git/miri/tests/fail/erroneous_const2.rs:9:9:1 | 9 | | ^^^^ | warning: rustfmt has failed to format. See previous 1 errors. ```

RalfJung · 2022-06-20T06:07:58Z

Thanks!
So, do you have a plan for having CI check this formatting? How tricky are the commands you have to run locally to test if all formatting was done successfully?

Format tests and benches with rustfmt (1-50 of 300) Extracted from #2097. I filtered this PR to contain exclusively "easy" cases to start off with, i.e. where there is no compiletest_rs (or other) comment in the vicinity that might need to get manually repositioned.

Format tests with rustfmt (101-150 of 300) Extracted from #2097. Like #2244, these are "easy" cases that do not involve moving around comments.

Format tests with rustfmt (51-100 of 300) Extracted from #2097. Like #2244, this is intended to be "easy" cases which don't involve comments in the vicinity.

Format tests with rustfmt (151-200 of 300) Extracted from #2097. This PR is still only doing the easy cases with no comments involved. In the next PRs after this, I'll start grouping by common comment patterns, e.g. all the cases resembling #2097 (comment) together in one PR.

Format tests with rustfmt (201-224 of 300) Extracted from #2097. Last of the easy cases which do not involve moving around a comment.

Format tests with rustfmt (276-287 of 299) Extracted from #2097. This is one half of the last 24 files (left for last because they require more unique attention than the first 275 "easy" files). I'll comment below to call attention to cases where I exercised my own judgement in how to format the test. rust-lang/rustfmt#3255 is especially annoying: rustfmt does not like `…( //` and `…{ //`.

Format tests with rustfmt (225-275 of 300) Extracted from #2097. These cases all involve a line comment at the end of a block that rustfmt has chosen to wrap. ```diff - unsafe { (*ptr).set(20); } //~ ERROR does not exist in the borrow stack + unsafe { + (*ptr).set(20); + } //~ ERROR does not exist in the borrow stack ``` I have moved all of those comments back onto the same line as the content of the block instead, as was indicated being `@RalfJung's` preference in #2097 (comment). ```diff + unsafe { + (*ptr).set(20); //~ ERROR does not exist in the borrow stack + } ```

Format tests with rustfmt (288-299 of 299) Extracted from #2097. I'll make a separate PR to enable checking the `tests` directory's formatting in CI. I'll need to rebase that after both this and #2254 have landed, and if any new non-rustfmt-formatted files appear in the meantime, we can include formatting those in the same PR that enables the CI.

dtolnay added 4 commits April 30, 2022 11:42

Rustfmt tests/**/*.rs

5f4ce32

Generated by: find . name '*.rs' | xargs rustfmt +nightly --edition=2021

Bless stderr files to update line numbers

bdd0ff0

Manual fixes to compiletest annotations

22c3a04

dtolnay force-pushed the rustfmt branch from d0949a0 to 22c3a04 Compare April 30, 2022 18:42

RalfJung reviewed May 1, 2022

View reviewed changes

dtolnay marked this pull request as draft May 17, 2022 00:26

RalfJung added the S-waiting-on-author Status: Waiting for the PR author to address review comments label May 23, 2022

dtolnay mentioned this pull request Jun 20, 2022

Add rustfmt::skip to some files #2242

Merged

dtolnay commented Jun 20, 2022

View reviewed changes

dtolnay closed this Jun 20, 2022

dtolnay deleted the rustfmt branch June 20, 2022 00:21

dtolnay mentioned this pull request Jun 20, 2022

Format tests and benches with rustfmt (1-50 of 300) #2244

Merged

This was referenced Jun 20, 2022

Format tests with rustfmt (51-100 of 300) #2246

Merged

Format tests with rustfmt (101-150 of 300) #2247

Merged

bors added a commit that referenced this pull request Jun 21, 2022

Auto merge of #2247 - dtolnay-contrib:rustfmt2, r=RalfJung

bdf2a8a

Format tests with rustfmt (101-150 of 300) Extracted from #2097. Like #2244, these are "easy" cases that do not involve moving around comments.

bors added a commit that referenced this pull request Jun 21, 2022

Auto merge of #2246 - dtolnay-contrib:rustfmt1, r=oli-obk

332f289

Format tests with rustfmt (51-100 of 300) Extracted from #2097. Like #2244, this is intended to be "easy" cases which don't involve comments in the vicinity.

dtolnay mentioned this pull request Jun 21, 2022

Format tests with rustfmt (151-200 of 300) #2249

Merged

This was referenced Jun 21, 2022

Format tests with rustfmt (201-224 of 300) #2251

Merged

Format tests with rustfmt (225-275 of 300) #2252

Merged

bors added a commit that referenced this pull request Jun 22, 2022

Auto merge of #2251 - dtolnay-contrib:rustfmt4, r=RalfJung

7a1b08e

Format tests with rustfmt (201-224 of 300) Extracted from #2097. Last of the easy cases which do not involve moving around a comment.

dtolnay mentioned this pull request Jun 22, 2022

Format tests with rustfmt (276-287 of 299) #2254

Merged

dtolnay mentioned this pull request Jun 22, 2022

Format tests with rustfmt (288-299 of 299) #2255

Merged

	fn do_panic() {
	// In large, friendly letters :)
	fn do_panic() {

Format tests and benches with rustfmt #2097

Format tests and benches with rustfmt #2097

Uh oh!

Conversation

dtolnay commented Apr 30, 2022

Uh oh!

bors commented May 1, 2022

Uh oh!

RalfJung commented May 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtolnay left a comment

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Jun 20, 2022

Uh oh!

Uh oh!