Add tidy rule against issue-[0-9]+.rs tests

[workingjubilee]

Proposal

WHEN in the Course of human events, it becomes necessary for some programmers to dissolve the bad decisions which have connected them with their bad habits, a decent respect to the opinions of humanity requires that they should declare the causes which impel them to the separation.

We hold these truths to be self-evident:

• That mere integral indices lack semantic information.
• That the purpose of a file's name is for the human eye, and not a machine inode.
• And that despite naming being one of the hardest things in computer science, it must be done anyways.

The tests in tests/ui/ have long had a habit of overwhelming contributors with increasingly meaningless series of integers, in the pattern of issue-[0-9]+.rs, so it is impossible to identify which tests are for what. They have further injured contributors by sorting unrelated issues adjacent to each other, making it impossible to further organize them except by manual inspection. And they have made it so it is impossible to identify relationships between tests if one does not cross-reference the issues themselves, which is not easy without reference to a source external to the git repository per se.

THEREFORE

we herewith propose that such names be abolished permanently from the codebase going forward, if they cannot add so much as a single additional iota of information that will be useful to the human eye to disambiguate them. Further, that these issue numbers, if they are placed anywhere in the file name, should be sorted towards the end and not the beginning of the file name.

Mentors or Reviewers

Myself. See also:

• Tidy rule against issue-* filenames in tests? rust#113345
• add tidy check that forbids issue-XXXX test filenames rust#113583

Process

The main points of the Major Change Process are as follows:

• File an issue describing the proposal.
• A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
  • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
  • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
• Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

[rustbot]

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

[compiler-errors]

Strongly agree that we shouldn't be committing issue-###.rs pretty much ever.

PR authors who are committing tests with their PR should leave at least a couple of words either relating the test to the PR's changes (e.g. check-static-values-constraints.rs), or explaining what the test actually exercises (e.g. illegal-upcast-from-impl.rs).

The only case where we don't "really know" what to name a test other than issue-###.rs is when a issue goes from ICE/fail -> pass and it's only caught by manual inspection or glacier. But with the existence of cargo-bisect-rustc, we should be able to find out what PR fixes the issue, and then work backwards from there to give the test a better name.

@rustbot second

[RalfJung]

To clarify, the information about the issue still absolutely must be present, right? Having a comment in the file "regression test for issue XXX" is *not* optional, if the file name no longer indicates the number. Often the issue contains a lot more valuable information than can be put into the filename.

[oli-obk]

Yes, ideally all tests have a doc comment at the top explaining its context (with links/ids) and its purpose in a bit more than a file name can convey.

[matthiaskrgr]

If we could, I would like to make it mandatory to have an issue number in the file name but we probably can't enforce that without it being a PITA for false positives.
If you add a new language feature in pr 12345 and all your files that are already in a ${feature_name} directory having to be named trait-impl-timelifes-12345.rs that is probably not helpful.

If we forbid issue-[0-9]+.rs should we also have an eye out for ice-1234.rs, pr456.rs or 6789.rs ?
(I think it is kind of ironic that 1235.rs which is obviously worse would NOT be flagged by the regex as it is now 🙃 )

The previous pr said "UI test{} should use a name that describes the test and link the issue in a comment instead.", in the warning, which sounds to me like we would disencourage people from adding the issue number to the file which I am very much against.
One of my usecases is that when I check for new compiler crashes¹, and I see that test21345.rs causes an ICE; I can immediately just look up 21345 in the issue tracker to get an idea if the original issue was already fixed or is still open, if there is a PR for it, without having to manually open files, which does not sound like a lot but a regular run just across a normal rustc repo can already yield around 200-300 findings of all sorts, so it definitely adds up.

I would prefer we encourage a naming scheme where we include an issue number next to a short human readable description like 10983-crash-during-normalizsation-of-inf-recursion.rs or malformed-lifetime-in-whatever-1234.rs

Having a machine readable way of storing issue ids inside the files also sounds nice but should not rule out issue numbers in filenames imo.

Footnotes

1. https://github.com/matthiaskrgr/icemaker/ ↩

[bryangarza]

I would prefer we encourage a naming scheme where we include an issue number next to a short human readable description like 10983-crash-during-normalizsation-of-inf-recursion.rs or malformed-lifetime-in-whatever-1234.rs

Yeah rather than excluding certain filename formats (which may miss variations as you said), it'd be nice to instead enforce a consistent format like what you described. Then going forwards, all the tests will be both human-readable and machine-readable.

[workingjubilee]

This is discussion in the issue and not in the Zulip stream, rather than summarizing that discussion or anything else, and I have avoided responding in a further reply to this issue because it is against protocol to do so.

[apiraino]

@rustbot label -final-comment-period +major-change-accepted