Add lint warning for inner function marked as #[test]

[estebank]

Fix #36629.

[rust-highfive]

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

[Centril]

I'm in favor of this change; but it is my understanding that adding a new lint requires an RFC per notes in https://github.com/rust-lang/rfcs/blob/master/lang_changes.md (which is either outdated and needs to be changed, or is accurate..).

[estebank]

@Centril in that case I'll create an RFC. This felt small enough as it's warning about already existing behavior, but I can see why we wouldn't want to make exceptions.

[Centril]

@estebank yeah I agree that it seems like a small, good and uncontroversial change :)

It might be a good idea to document what lints need RFCs and which don't.

[mark-i-m]

I don't think it needs an RFC. This seems like a bug-fix to me. Perhaps an FCP is sufficient?

[zackmdavis]

untestable_method is a very confusing name.

[zackmdavis]

Whatever name is chosen (untestable_test_functions??), it should respect RFC 344 (see #50879).

[bors]

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

[pnkfelix]

This code seems fine. The only thing left is to resolve the name of the lint itself.

How about unnameable_test_methods? The lint diagnostic itself can be the thing that points out that such #[test] methods are in fact not run by the test harness, while the lint name itself will serve to imply why they are not supported.

[zackmdavis]

This isn't a "method"; it's a function (as I hear the words usually used).

[pnkfelix]

@zackmdavis I don't disagree. The high-order bit of my suggestion was meant to be the "unnameable" part, not the use of the word "methods."

[estebank]

Changed lint name.

[zackmdavis]

(RFC 344 seems to suggest the plural: unnameable-test-functions?)

The basic rule is: the lint name should make sense when read as "allow lint-name" or "allow lint-name items". For example, "allow deprecated items" and "allow dead_code" makes sense, while "allow unsafe_block" is ungrammatical (should be plural).

[estebank]

Ping. Anything left for me to do?

[emilyalbini]

Ping from triage @pnkfelix! This PR needs your review.

[bors]

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

[bors]

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

[estebank]

Ping.

[TimNN]

Ping from triage, @pnkfelix / @rust-lang/compiler: This PR requires your review!

[cramertj]

@pnkfelix said "The only thing left is to resolve the name of the lint itself", and the name was resolved, so

@bors r=pnkfelix

[bors]

📌 Commit 51a0425 has been approved by pnkfelix

[bors]

⌛ Testing commit 51a0425 with merge 739320a...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: @pnkfelix
Pushing 739320a to master...