Tracking Issue for must_not_suspend lint (RFC #3014)

[nikomatsakis]

This is a tracking issue for the RFC 3014 (rust-lang/rfcs#3014).
The feature gate for the issue is #![feature(must_not_await_lint)].

Status: This is waiting on more precise generator captures to reduce false positives before stabilizing and turning the lint on by default.

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

• Implement the RFC (cc @rust-lang/compiler -- can anyone write up mentoring
  instructions?)
• Improve generator drop tracking (Dropped variables still included in generator type #57478, Tracking issue for more precise coroutine captures #69663)
• Adjust documentation (see instructions on rustc-dev-guide)
• Stabilization PR (see instructions on rustc-dev-guide)

Unresolved Questions

• Propagate the lint in nested structs/enums. Similar to the use case for the must_use attribute. These likely should be solved together.
• Should the implementation be done with an auto trait, to avoid the need for the hand-written logic for walking types like tuples and arrays. This would also solve the nesting problem above.
• The diagnostics should be improved when the lint triggers on async fn calls that don't obviously store a must_not_suspend type (see https://github.com/rust-lang/rust/blob/08e026675ee38bb4ca81106e766a6456c8b7382e/src/test/ui/lint/must_not_suspend/dedup.rs)

Implementation history

• Implement #[must_not_suspend] #88865
• Feature gate + make must_not_suspend allow-by-default #89826

[bIgBV]

I am fairly busy right now, but I can pick this up in a few weeks' time. While researching the lint I dug into what changes would be required to the compiler and I'm fairly certain I have those written down somewhere. I would definitely appreciate any other pointers though.

[bIgBV]

@rustbot claim

[bIgBV]

@nikomatsakis @tmandry I've begun working on this implementation, but I would really appreciate if you or someone you can point to can drop some mentoring instructions to help me get started. The general idea I have so far is this:

1. Create a new TypeFlag for tracking if a type has the #[must_not_suspend] attribute present on it
2. During type construction (when the interned Ty is constructed?) check for the presence of this flag, and apply it any compound types (structs, tuples and enums. Am I missing anything?)
3. During the computation of the types a generator captures, check whether the type has the new TypeFlag and emit a warning.
   • Create a new lint for this purpose.

Am I missing something? Right now my main questions are where the a type is constructed and where the propagation of the flag would fit in.

[bIgBV]

As a note, I think made an implicit assumption of the propagation behavior in my previous comment. I think we should propagate the lint to nested types. I would like to do the work to apply this to the #[must_use] lint as well.

[bIgBV]

The other approach I was thinking of was provide a new Query which answers the presence of a flag or the presence of an attribute on a type. I think this approach would allow us to apply this behavior to the #[must_use] attribute as well.

[nikomatsakis]

@bIgBV

Happy to help! Here are some bits of code to investigate to get started.

First, you might want to look a bit at the must_use lint, although I think that this lint wants to be implemented somewhat differently. (I wouldn't use a lint pass, for example.) That said, the logic that must_use uses to decide if a type is MustUse is probably roughly what we want. That code doesn't use a TypeFlags. I'd avoid those, they seem a bit like overkill for your purposes, but instead has a function that, given a Ty<'tcx>, checks whether the type is "must-use":

https://github.com/rust-lang/rust/blob/4dc3316557522277db403954286a3f55c6ac8d16/compiler/rustc_lint/src/unused.rs#L173-L181

Generators already have some logic to figure out what types outlive an await. We also have some logic to track why the type appears in that list, which we use for giving better error messages.

Here is the code that adds a type into that set and tracks various spans associated with it:

https://github.com/rust-lang/rust/blob/4dc3316557522277db403954286a3f55c6ac8d16/compiler/rustc_typeck/src/check/generator_interior.rs#L108-L114

I think we could issue this lint exactly at this point -- we could basically analyze that type and determine whether it is "must not suspend". If so, we could issue the lint. Issuing a lint is done with some code roughly like this (from the check_unused lint):

https://github.com/rust-lang/rust/blob/4dc3316557522277db403954286a3f55c6ac8d16/compiler/rustc_typeck/src/check_unused.rs#L128-L144

[bIgBV]

@nikomatsakis thanks for the pointers! My initial plan was actually to follow the implementation of the must_use lint, but @tmandry suggested to go down the TypeFlags route if we wanted to propagate the attribute to nested types as it would be more performant.

Also, does my suggestion of using a query make sense? I was coming it at from a forward looking perspective, as there seem to be many other must_* lints being suggested (must_not_drop, may_blockcome to mind) and a query for checking for the presence of such an attribute on a type might come in handy (from my extremely rudimentary understanding of the new compiler architecture).

[Aaron1011]

Bikeshed - using must in the attribute name seems too strong for something that triggers an #[allow]-able lint. I think something like #[should_not_suspend] would better reflect the fact there may be legitimate reasons to suspend with such a type in scope, and than the annotated type itself can not assume that consumers will never suspend.

While there is precedent in the form of #[must_use], I think that 'use' has a broader meaning than 'suspend', so it's less confusing to see a type 'used' via let _ = create_must_use(), or #[allow]-ed.

[nikomatsakis]

@bIgBV well, so type-flags can help to propagate through the types that are "visible". So for example Vec<Foo> would combine the type flags of Vec and Foo. But if you are going to look at the felds defined in Vec, TypeFlags isn't really what you want. This kind of thing is always a touch tricky to do.

I guess there's a question of which part to work on first. I personally would work on the code to create the lint first and have it fire in simpler cases, then come back and experiment with the best ways for it to fire.

I have to go re-read the RFC: i still feel pretty strongly that we ought to make #[must_use] and this lint work in the same way, whatever that is. I think i'm being slowly convinced about a more "nested" approach but I'm not sure.

[bIgBV]

@nikomatsakis ah, that's fair. And yes, I agree, I think we need to solve the problem of propagating attributes for both this and the #[must_use] attributes. I've begun working on a basic implementation based on the notes that you've provided, but I'd love to come up with ideas on how to unify the behavior for both these lints, but also for the future ones. I think a syntax like #[must(not_suspend, use, not_block)] might be something worth exploring.

[nikomatsakis]

@bIgBV oh, interesting thought

[guswynn]

@bIgBV let me know if you are still working on this! I may start looking at it this weekend

[Kobzol]

Just wondering, will this attribute be added to selected std items like Ref (corresponding to the await_holding_refcell_ref clippy lint), or will it be left up to users to use newtypes with the attribute on top of these types to enable the warning? (sorry if this is the wrong place to ask)

[estebank]

@Kobzol, appropriate std items will be annotated with this attribute, likely on individual PRs independent of the lint's implementation.

[dtolnay]

#89303 adds #[must_not_suspend] to MutexGuard, RwLockReadGuard, RwLockWriteGuard, and the two RefCell guards Ref and RefMut.

[camelid]

The #[must_not_suspend] attribute is unstable, but the lint itself is insta-stable. For example, #![warn(must_not_suspend)] compiles successfully on nightly with no feature flags. Is this intentional?

[tmandry]

I think that's how lints usually work.

[Mark-Simulacrum]

No, lints can definitely be landed as unstable first, it's what I would expect here. There's not much risk since they're mostly just "a name", but we should still avoid accidental stabilizations.

[camelid]

Ok, I opened an issue: #89798.

[guswynn]

No, lints can definitely be landed as unstable first, it's what I would expect here. There's not much risk since they're mostly just "a name", but we should still avoid accidental stabilizations.

interesting, I did not know this

[yvt]

I'm wondering: can unsafe code in a macro rely on #[forbid(must_not_suspend)] to guarantee that a local variable is never placed outside a stack? It does seem to be true at a glance, but I'm worried that there might be a loophole, considering that it's just a lint.

[guswynn]

@yvt themust_not_suspend lint is just that: a lint. I can be skipped, can fail analysis, etc, and rustc will continue to compile the code. Unsafe should not rely on it. Are you looking for something like: https://docs.rs/futures/0.3.17/futures/macro.pin_mut.html?

[yvt]

@guswynn Thank you for the answer. Actually I'm looking for something stronger than pinning, a guarantee that Drop::drop is called before any of the containing references are invalidated. In synchronous code, this can be easily accomplished by pin_mut!, which I short-sightedly assumed would also hold in async code when it, in fact, doesn't. So I was hoping that there might be some way to mitigate the resulting soundness problem.