Add async_fn_in_trait lint

[compiler-errors]

cc #115822 (comment)

Mostly unsure what the messaging should be. Feedback required.

r? @tmandry

[petrochenkov]

This should probably be restricted to effective_visibilities.is_reachable(...) traits.
For private traits it's always possible to adjust them as necessary if the problems with Send etc. occur.

[rakshith-ravi]

Just looking at this from the point of view of new comers, and I feel like explicit suggestions would be more helpful for them

[rakshith-ravi]

Can we perhaps rephrase this to be more helpful to new users? Like so:

you can suppress this lint if you do not care about auto traits like `Send` on the future with the `#[allow(async_fn_in_trait)]` attribute

[compiler-errors]

I'd rather just make it a structured suggestion, but also, I don't think any other lint goes out of its way to tell you how to suppress it.

[rakshith-ravi]

I'm sorry, but what's a structured suggestion?

Also, yes, other lints don't do this, but most other lints aren't elevated to be shown by default unless it's guaranteed to not have false positives. Additionally, new-comers probably wouldn't understand a lot about what Send bounds are and why it's needed. While yes, they should eventually learn it, it only increases the barrier to getting started with rust for them. Giving them a simple suggestions is more of a "here's the simple, ugly way out, but you should probably look into this when you understand what's happening"

[rakshith-ravi]

On second thought, it would also encourage users to do the wrong thing unless they know what they're doing, which isn't exactly ideal.

Hmm, idk. I'm conflicted now. I've put both sides of the coin on the table. Open for suggestions

[jonhoo]

Thank you for implementing this! I think the lint should also warn about the backwards compatibility hazard associated with making the trait method return impl Future + Send, namely that one cannot remove the Send in the future (e.g., when RTN stabilizes) without that being a breaking change. It does mean that the lint boils down to "pick your poison", but that's sort of the point! Maybe it could also suggest ATPIT as another feasible path forward (assuming that stabilizes in time)?

[tmandry]

@bors r+

[bors]

📌 Commit 2f52490 has been approved by tmandry

It is now in the queue for this repository.

[bors]

⌛ Testing commit 2f52490 with merge b781645...

[bors]

☀️ Test successful - checks-actions
Approved by: tmandry
Pushing b781645 to master...

[rust-timer]

Finished benchmarking commit (b781645): 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)	4.0%	[3.0%, 5.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

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

Binary size

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

Bootstrap: 622.444s -> 623.73s (0.21%)
Artifact size: 272.02 MiB -> 272.01 MiB (-0.01%)

[jonhoo]

The backwards compatibility hazard around going the + Send route isn't currently highlighted by the lint. I think it's essential to add that in there, because as written folks will see the suggestion and go "oh, sweet, that fixes my problem, let me just do that!", not realizing that they've now committed to + Send until the next major release.

[compiler-errors]

I'll make a TODO to make that present in the diagnostic. For the record, the lint description does note this:

    /// This still allows the use of `async fn` within impls of the trait.
    /// However, it also means that the trait will never be compatible with
    /// impls where the returned [`Future`] of the method does not implement
    /// `Send`.

[jonhoo]

Yeah, I think it specifically needs to be mentioned in the .suggestion — I've found that it's pretty rare someone actually reads (or even knows how to read) lint descriptions.

I saw that bit, and it's close, but I think missing a crucial bi-sentence. Something like:

... without a breaking change to the trait where the + Send bound is removed again.

[tmandry]

Yeah, I think it specifically needs to be mentioned in the .suggestion — I've found that it's pretty rare someone actually reads (or even knows how to read) lint descriptions.

I don't agree that it should go in the suggestion; the implication of the syntax should really be clear to anyone who is publishing a trait for public consumption. If someone is unsure, that's a good reason to dig deeper by reading the lint description, the blog post we'll publish about releasing AFIT, etc.

... without a breaking change to the trait where the + Send bound is removed again.

I'm not opposed to saying something like this, though editorially, it's already a pretty long sentence. I'm happy to review a PR.

[jonhoo]

the implication of the syntax should really be clear to anyone who is publishing a trait for public consumption

I agree on the "should", but disagree on the implied "would". My hope was that we could help trait definers understand the trade-off they're making, but if you feel strongly that the backwards-compat hazard here will be obvious to all relevant users, then I won't belabor the point. I just don't share your conviction that that is true 😅

dig deeper by reading the lint description

I didn't even known lints had descriptions until this discussion. How does one find them? I can't find a reference anywhere to how to print a lint description? Does it require digging into the rustc src? If so, I think no-one who's at risk from this hazard will go dig that up.

the blog post we'll publish about releasing AFIT

Here's a thought: maybe we could link to the blog post from the lint (once it's out, so one release later)? That feels like it might be a good way to give folks more context than the little note.

[tmandry]

How does one find them?

Normally the error comes with instructions to run a command like rustc --explain E0046 to print them, but I just noticed this lint doesn't have an error code. @compiler-errors how can we make this description accessible?

For what it's worth, they're also available online in the error codes index. I don't expect anyone to be digging into the compiler source :).

maybe we could link to the blog post from the lint

We could link to it from the lint or from the lint description, yeah.

[compiler-errors]

@tmandry: As far as I can tell, lints are not typically associated with error codes. The only one I could find is E0602, but that doesn't use the same hir-based lint infrastructure that we do to emit this lint. Error code docs would also need to copy the lint description, since they're populated from separate markdown files.

If we think additional information should be attached, then instead of error code docs, I think we should just put another note on the diagnostic.

[jonhoo]

According to the rustc dev guide at least:

Diagnostics created by lints don't have a code in the emitted message.

Which matches what I thought was the case 😅 And I agree with @compiler-errors that, given this is the case, we should probably surface some more info from the description in the notes.