Reimpl meaningful test name lint MCP658 #120628

workingjubilee · 2024-02-03T23:17:18Z

This reintroduces the tidy rule originally proposed in #113583 that then became an MCP in rust-lang/compiler-team#658 which eventually surfaced a quite-reasonable request for a diagnostic enhancement. I have added that to the rule. It produces output like this:

tidy error: file `ui/unsized/issue-115809.rs` must begin with a descriptive name, try `{reason}-issue-115809.rs`
tidy error: file `ui/unsized/issue-115203.rs` must begin with a descriptive name, try `{reason}-issue-115203.rs`
tidy error: file `ui/privacy/issue-113860-2.rs` must begin with a descriptive name, try `{reason}-issue-113860.rs`
tidy error: file `ui/privacy/issue-117997.rs` must begin with a descriptive name, try `{reason}-issue-117997.rs`
tidy error: file `ui/privacy/issue-119463.rs` must begin with a descriptive name, try `{reason}-issue-119463.rs`
tidy error: file `ui/privacy/auxiliary/issue-117997.rs` must begin with a descriptive name, try `{reason}-issue-117997.rs`
tidy error: file `ui/privacy/auxiliary/issue-119463-extern.rs` must begin with a descriptive name, try `{reason}-issue-119463.rs`
tidy error: file `ui/privacy/issue-113860-1.rs` must begin with a descriptive name, try `{reason}-issue-113860.rs`
tidy error: file `ui/privacy/issue-113860.rs` must begin with a descriptive name, try `{reason}-issue-113860.rs`
tidy error: file `ui/const-generics/generic_const_exprs/const_kind_expr/issue_114151.rs` must begin with a descriptive name, try `{reason}-issue-114151.rs`
tidy error: file `ui/did_you_mean/issue-114112.rs` must begin with a descriptive name, try `{reason}-issue-114112.rs`
tidy error: file `ui/did_you_mean/issue-105225.rs` must begin with a descriptive name, try `{reason}-issue-105225.rs`
tidy error: file `ui/did_you_mean/issue-105225-named-args.rs` must begin with a descriptive name, try `{reason}-issue-105225.rs`

You get the idea.

There are some tests which merely would require reordering of the name according to the rule. I could modify the diagnostic further to identify those, but doing such would make it prone to bad suggestions. I have opted to trust contributors to recognize the diagnostic is robotic, as the pattern we are linting on is easier to match if we do not speculate on what parts of the name are meaningful: sometimes a word is a reason, but sometimes it is a mere "tag", such as with a pair like:

issue-314159265-blue.rs
issue-314159265-red.rs

Starting them with red- and blue- means they do not sort together, despite being related, and the color names are still not very descriptive. Recognizing a good name is an open-ended task, though this pair might be:

colored-circle-gen-blue.rs
colored-circle-gen-red.rs

Deciding exactly how to solve this is not the business of tidy, only recognizing a what.

r? @compiler-errors

workingjubilee · 2024-02-03T23:49:28Z

...of course.

Noratrieb · 2024-02-04T07:31:35Z

@bors p=20
let's not interfere with the current rollups and get them through, but after that and another rebase of yours this really needs some p

Noratrieb · 2024-02-05T07:38:59Z

src/tools/tidy/src/issues.txt

@@ -0,0 +1,4376 @@
+/*


~4000/~15000.... that's a lot of grandfathers 🫠

Noratrieb · 2024-02-05T07:41:13Z

src/tools/tidy/src/ui_tests.rs

+            let mut p = PathBuf::from(path);
+            p.push(file_name);
+            tidy_error!(
+                bad,


it would be very cool if tidy --blessing removed it for you

Hmm, that sounds plausible.

compiler-errors · 2024-02-05T20:48:53Z

I'm in favor of this change, but given its size, do you mind if we wait a few days for the bors queue to calm down a bit?

workingjubilee · 2024-02-06T04:56:32Z

No problem if you help me remember when / give me a poke to rebase it by the usual venues.

compiler-errors · 2024-02-12T15:58:01Z

howdy @workingjubilee, I think I'm ready to review this if you're able to rebase it. I'd also really like if ./x.py test tidy --bless did what nils asked for above!

@rustbot author

This reverts commit 686b3de.

As the reason for tidy failing the test name may seem slightly opaque, provide a suggestion on how to rename the file. There are some tests which merely would require reordering of the name according to the rule. I could modify the diagnostic further to identify those, but doing such would make it prone to bad suggestions. I have opted to trust contributors to recognize the diagnostic is robotic, as the pattern we are linting on is easier to match if we do not speculate on what parts of the name are meaningful: sometimes a word is a reason, but sometimes it is a mere "tag", such as with a pair like: issue-314159265-blue.rs issue-314159265-red.rs Starting them with `red-` and `blue-` means they do not sort together, despite being related, and the color names are still not very descriptive. Recognizing a good name is an open-ended task, though this pair might be: colored-circle-gen-blue.rs colored-circle-gen-red.rs Deciding exactly how to solve this is not the business of tidy, only recognizing a what.

~130 new entries, depending on how you count, with the rest being sorting churn.

workingjubilee · 2024-02-18T03:39:34Z

@rustbot review

workingjubilee · 2024-02-18T04:04:44Z

src/tools/tidy/src/issues.txt

+"ui/c-variadic/issue-32201.rs",
+"ui/c-variadic/issue-86053-1.rs",
+"ui/c-variadic/issue-86053-2.rs",


The new order was not quite stable for this round, as I had been using sort --unique for the last sort, but it should be in the future, as BTreeSet yields items in order when iterated and uses the Ord of String.

compiler-errors · 2024-02-20T01:47:07Z

@bors r+ rollup=iffy

bors · 2024-02-20T01:47:10Z

📌 Commit ce72a3f has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2024-02-20T02:31:21Z

⌛ Testing commit ce72a3f with merge 0b9f6ad...

bors · 2024-02-20T04:30:11Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 0b9f6ad to master...

rust-timer · 2024-02-20T05:43:37Z

Finished benchmarking commit (0b9f6ad): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.9%	[-2.9%, -2.9%]	1
Improvements ✅ (secondary)	-4.1%	[-6.1%, -2.1%]	2
All ❌✅ (primary)	-2.9%	[-2.9%, -2.9%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.2%	[3.2%, 3.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.9%	[-5.0%, -2.9%]	3
All ❌✅ (primary)	-	-	0

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Bootstrap: 638.5s -> 638.786s (0.04%)
Artifact size: 308.65 MiB -> 308.61 MiB (-0.01%)

petrochenkov · 2024-03-01T18:04:47Z

@workingjubilee
On Windows issues.txt is always regenerated with backslashes in paths, instead of forward slashes like in the committed version of the file.

workingjubilee · 2024-03-07T03:49:45Z

On Windows issues.txt is always regenerated with backslashes in paths, instead of forward slashes like in the committed version of the file.

Odd! Noted and will fix, apologies.

…ld-belongs-to-unix, r=ChrisDenton Fix `tidy --bless` on ̶X̶e̶n̶i̶x̶ Windows As reported in rust-lang#120628 (comment) the requested `tidy --bless` implementation didn't take into account the fact that earlier the linting code canonicalized things to use the OS path separator. This makes it so that the path separator is always rewritten back as '/', which should fix the variance there. r? `@ChrisDenton`

…ld-belongs-to-unix, r=ChrisDenton Fix `tidy --bless` on ̶X̶e̶n̶i̶x̶ Windows As reported in rust-lang#120628 (comment) the requested `tidy --bless` implementation didn't take into account the fact that earlier the linting code canonicalized things to use the OS path separator. This makes it so that the path separator is always rewritten back as '/', which should fix the variance there. r? ``@ChrisDenton``

Rollup merge of rust-lang#122126 - workingjubilee:every-os-in-the-world-belongs-to-unix, r=ChrisDenton Fix `tidy --bless` on ̶X̶e̶n̶i̶x̶ Windows As reported in rust-lang#120628 (comment) the requested `tidy --bless` implementation didn't take into account the fact that earlier the linting code canonicalized things to use the OS path separator. This makes it so that the path separator is always rewritten back as '/', which should fix the variance there. r? ``@ChrisDenton``

…s-to-unix, r=ChrisDenton Fix `tidy --bless` on ̶X̶e̶n̶i̶x̶ Windows As reported in rust-lang/rust#120628 (comment) the requested `tidy --bless` implementation didn't take into account the fact that earlier the linting code canonicalized things to use the OS path separator. This makes it so that the path separator is always rewritten back as '/', which should fix the variance there. r? ``@ChrisDenton``

rustbot assigned compiler-errors Feb 3, 2024

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Feb 3, 2024

This comment has been minimized.

Sign in to view

workingjubilee force-pushed the reimpl-meaningful-test-name-lint branch 2 times, most recently from 11fb805 to 0c3a300 Compare February 4, 2024 00:28

Noratrieb reviewed Feb 5, 2024

View reviewed changes

Noratrieb mentioned this pull request Feb 5, 2024

Add parallel rustc ui tests #120664

Merged

rustbot 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 Feb 12, 2024

workingjubilee added 5 commits February 17, 2024 14:31

Reapply "add tidy check that forbids issue ui test filenames"

a416fde

This reverts commit 686b3de.

Grandfather new questionably-named tests

12641b0

~130 new entries, depending on how you count, with the rest being sorting churn.

Implement --bless for the issues lint

cb57c42

Remove old issues.txt entries with --bless

ce72a3f

workingjubilee force-pushed the reimpl-meaningful-test-name-lint branch from 0c3a300 to ce72a3f Compare February 18, 2024 03:38

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

workingjubilee commented Feb 18, 2024

View reviewed changes

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 Feb 20, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 20, 2024

bors merged commit 0b9f6ad into rust-lang:master Feb 20, 2024
12 checks passed

rustbot added this to the 1.78.0 milestone Feb 20, 2024

workingjubilee deleted the reimpl-meaningful-test-name-lint branch February 20, 2024 05:41

c410-f3r mentioned this pull request Mar 3, 2024

Move some tests #121248

Merged

workingjubilee mentioned this pull request Mar 7, 2024

Fix tidy --bless on ̶X̶e̶n̶i̶x̶ Windows #122126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimpl meaningful test name lint MCP658 #120628

Reimpl meaningful test name lint MCP658 #120628

workingjubilee commented Feb 3, 2024 •

edited

Loading

This comment has been minimized.

workingjubilee commented Feb 3, 2024

Noratrieb commented Feb 4, 2024

Noratrieb Feb 5, 2024

Noratrieb Feb 5, 2024

workingjubilee Feb 6, 2024

compiler-errors commented Feb 5, 2024

workingjubilee commented Feb 6, 2024

compiler-errors commented Feb 12, 2024

workingjubilee commented Feb 18, 2024

workingjubilee Feb 18, 2024 •

edited

Loading

compiler-errors commented Feb 20, 2024

bors commented Feb 20, 2024

bors commented Feb 20, 2024

bors commented Feb 20, 2024

rust-timer commented Feb 20, 2024

petrochenkov commented Mar 1, 2024

workingjubilee commented Mar 7, 2024

Reimpl meaningful test name lint MCP658 #120628

Reimpl meaningful test name lint MCP658 #120628

Conversation

workingjubilee commented Feb 3, 2024 • edited Loading

This comment has been minimized.

workingjubilee commented Feb 3, 2024

Noratrieb commented Feb 4, 2024

Noratrieb Feb 5, 2024

Choose a reason for hiding this comment

Noratrieb Feb 5, 2024

Choose a reason for hiding this comment

workingjubilee Feb 6, 2024

Choose a reason for hiding this comment

compiler-errors commented Feb 5, 2024

workingjubilee commented Feb 6, 2024

compiler-errors commented Feb 12, 2024

workingjubilee commented Feb 18, 2024

workingjubilee Feb 18, 2024 • edited Loading

Choose a reason for hiding this comment

compiler-errors commented Feb 20, 2024

bors commented Feb 20, 2024

bors commented Feb 20, 2024

bors commented Feb 20, 2024

rust-timer commented Feb 20, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

petrochenkov commented Mar 1, 2024

workingjubilee commented Mar 7, 2024

workingjubilee commented Feb 3, 2024 •

edited

Loading

workingjubilee Feb 18, 2024 •

edited

Loading