Adds must_not_suspend_lint RFC

[bIgBV]

Rendered

Pre RFC discussion

• Draft RFC to add a must_not_await lint wg-async#16

Status

Merged!

[comex]

Editorial: The title of this issue shouldn't say #[must_not_await_lint]. The RFC name is must_not_await_lint, the attribute is #[must_not_await].

Bikeshed: Decorating a type with "must not await" sounds like it's forbidding you from awaiting values of that type – not from capturing values of that type while awaiting some unrelated object. However, I can't think of a better name that isn't considerably more verbose.

[bovinebuddha]

Bikeshed: #[must_not_suspend]?

[LucioFranco]

@bovinebuddha I personally think that is more confusing, since most users interact with await and not the concept of suspending a task. We originally had must_not_yield and that was too low level imo.

[erickt]

bikeshed: I could see a feature like this being useful in generators and coroutines, since we also wouldn't want to hold mutexes across those yield points as well. So going for more generic language like #[must_not_suspend] would help avoid churn down the road.

[ghost]

#[must_not_halt]

[bIgBV]

@erickt that's a fair point. We did not consider extending the use of this attribute to be used for generators in the future, but I can see how it would be possible. I think #[must_not_suspend] is a good name for the attribute considering this.

[LucioFranco]

@erickt My concern about going a bit lower level with the generator based name is that it has some sort of disconnect from async/await. We may want to just consider to add another version when we finally stabilize generators?

[nikomatsakis]

We discussed this in the @rust-lang/lang meeting today and came to the conclusion that we would like to move this forward to the implementation stage, modulo whatever bikeshedding makes sense for the name.

@rfcbot fcp merge

[rfcbot]

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

• @cramertj
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[joshtriplett]

One thing I'd like to see added to the open questions and alternatives sections: would it suffice to only support this on functions, and not on types? With #[must_use], a great deal of complexity and potential future surface area arises around what happens if you embed that type inside some other type, and whether #[must_use] still applies then. The "Unresolved questions" section mentions solving this problem for #[must_not_await] as well, but another alternative would be avoiding this problem entirely by only applying this to function return values.

I don't know if this is a good idea, or if it'd result in having to add far more annotations. I think it's worth discussing the tradeoff in the RFC.

[tmandry]

rust-lang/wg-async#19 is a related (but separate) idea that folks should be aware of. This is another attribute that would warn when certain functions are called in an async context. It might be worth mentioning this as a future direction in the RFC.

[bIgBV]

@tmandry I think we should mention it in the RFC as a way to talk about other work in this area, but aren't these two separate concerns?

[tmandry]

Yes, they're separate, but conceptually they are related. I just think the RFC should mention it as you say.

I don't mean to imply that it would be a future direction in the sense that it would build on this RFC, but that it's simply another thing we might consider doing in the future.

[lahwran]

that's a fair argument. since this is a bikeshed type of discussion, here are some grades I'd give different shed colors among these options:

• #[must_not_await]: This does seem unacceptably unclear to me. I'd give it -2
• #[must_not_suspend]: while literally true that the type gets suspended as part of a suspended async function state machine, I'm convinced by the argument that it should be clearer on first reading this name; it's nice that it's shorter than other options, though. I'd give it a 0
• #[must_not_hold_across_await]: seems solid, I'd prefer for it to be ready to apply to generators as well since it's sensible to expect that nearly all uses will apply to both; I can't think of one where generators are fine but await isn't. its length is mildly irritating, though. I'd give it a 0
• #[must_not_hold_across_suspend]: solid, improved generality, but pretty long; I'd give it +1
• #[must_not_own_suspended]: still seems to convey the right meaning, but it's a bit less clear; +1
• #[must_drop_before_suspend]: seems a bit clearer to me, still not quite as long as the longest option, and it's possible to get rid of a local by move rather than by drop, though I don't think that's unclear; +2
• #[must_drop_by_suspend]: possibly the few characters shorter is an improvement, but "by" is a bit linguistically vague vs "before". +1

I'm considering this my final input and will not comment on the naming issue further.

[bIgBV]

I think there's been a lot of great discussion around the naming of this feature which lead to a lot of good options. There's two main ways to go forward from here:

1. Punt the name finalization as a requirement for stabilization
2. Get consensus on a name during the RFC stage.

I personally would like to finalize it in this stage as it would leave fewer open questions for the implementation stage. As far as deciding on a name goes, I think we should go with #[must_not_suspend] and these are my reasons for this choice:

• It is succinct: having a long lint name might not be the most ergonomic option for users who will want to use this lint. Plus given this it is a lint similar to the style of the #[must_use] lint, the similarity will help signal the intent.
• It alludes to suspension of execution: This is the crux of what behavior this lint is trying to warn against. Any type marked as such must not be suspended as apart of asynchronous execution. Plus, it also will work with generators (if and when they stabilize)

I agree, that this is not the best possible option for a name, but it is certainly an acceptable one; one which fulfills the requirements of signaling the intent as well as being succinct enough to be remembered.

Any further elaboration on the behavior of the lint should be a part of the warning text displayed as well as the documentation for the lint, as no name truly would be able to capture all the nuances of the complex conditions in which the lint would be a valid warning.

[bIgBV]

It's been almost ten days since the last discussion on this PR, therefore in the interest of moving the RFC forward, I think we should finalize on name for the lint to be #[must_not_suspend]. I'll wait until 8:00 PM PST tomorrow (2021-01-25) to update the RFC to reflect the same.

[SOF3]

that's a fair argument. since this is a bikeshed type of discussion, here are some grades I'd give different shed colors among these options:

* `#[must_not_await]`: This does seem unacceptably unclear to me. I'd give it -2

* `#[must_not_suspend]`: while literally true that the type gets suspended as part of a suspended async function state machine, I'm convinced by the argument that it should be clearer on first reading this name; it's nice that it's shorter than other options, though. I'd give it a 0

* `#[must_not_hold_across_await]`: seems solid, I'd prefer for it to be ready to apply to generators as well since it's sensible to expect that nearly all uses will apply to both; I can't think of one where generators are fine but await isn't. its length is mildly irritating, though. I'd give it a 0

* `#[must_not_hold_across_suspend]`: solid, improved generality, but pretty long; I'd give it +1

* `#[must_not_own_suspended]`: still seems to convey the right meaning, but it's a bit less clear; +1

* `#[must_drop_before_suspend]`: seems a bit clearer to me, still not quite as long as the longest option, and it's possible to get rid of a local by move rather than by drop, though I don't think that's unclear; +2

* `#[must_drop_by_suspend]`: possibly the few characters shorter is an improvement, but "by" is a bit linguistically vague vs "before". +1

I'm considering this my final input and will not comment on the naming issue further.

why not drop the word must, like #[drop_by_suspend]? #[must_not_await] is grammatically strange without must, but #[drop_by_suspend] seems to make sense.

[bIgBV]

@SOF3 that's in order to keep parity with the #[must_use] lint, since both fall under the same class of lints.

[erickt]

For some more bikeshedding (which is orthogonal to this RFC, and thus I don't think passage of this), we could try to make things more generic, and instead deprecate #[must_use], make a #[must(...)] attribute, where we could mark things #[must(use, not_suspend, ...)].

[shepmaster]

to update the RFC to reflect the same.

You might want to update the title of the RFC PR as well (not much to do for the branch name):

[bIgBV]

Now that the naming conversation has reached a conclusion, what can be done to move this forward? @pnkfelix @scottmcm ?

[nikomatsakis]

(I checked @withoutboats's box, as they're not active on lang team at the moment.)

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[RalfJung]

To be sure I understand this correctly -- no attempt is made at detecting "must_not_await" types that hide behind generic types, right? So this does not prevent MutexGuard from being held across an await, it just captures the "obvious" cases?

That seems fine to me, but the RFC should call it out explicitly. It talks about false positives but does not mention false negatives.

[danielzgtg]

Suggested change

	Going through the prior are we see two systems currently which provide simailar/semantically similar behavior:
	Going through the prior art we see two systems currently which provide similar/semantically similar behavior:

[danielzgtg]

What should happen if the value returned implements Copy?

What should happen in the following two examples?

#[must_not_suspend]
fn foo() -> i32 { 5i32 }

async fn foo() {
  let mut baz = 0i32; // EDIT: Fixed compile
  {
    let bar = foo();
    baz = bar;
    drop(bar);
  }
  my_async_op.await;
  println!("{:?}", baz);
  // Ok or not?
}

#[must_not_suspend]
fn foo() -> i32 { 5i32 }

fn qux(a: i32) -> i32 { a }

async fn foo() {
  let bar = qux(foo());
  my_async_op.await;
  println!("{:?}", bar);
  // Ok or not?
}

[tmandry]

The relevant question is whether the value is used again after an await. Copy doesn't affect that at all. EDIT: nevermind, my answer for Copy is the same as for the other examples

As for your examples, I don't know of a use case for supporting the attribute on functions and would prefer to only support it on types for now.

[danielzgtg]

attribute on functions

I was only using the functions to obtain a value with must_not_suspend.

Copy doesn't affect that at all

The thing about Copy is that when you have one value, you can easily end up with two, three, or more values resulting from copying the original value.

My question is basically will the copies will inherit the must_not_suspend?

• If they do, are we just going to make all functions that take must_not_suspend values return results that are also must_not_suspend? Otherwise, would we make Copy special and why?
• if they don't, then it will become easy to shoot oneself in the foot again if one doesn't realize that they copied a value before an await

The same logic could apply to Clone, but it's not much of a problem there because having to write out the .clone() makes things obvious.

my answer for Copy is the same as for the other examples

So I guess that means the must_not_suspend DOES inherit across unmarked functions from the parameters to the return value?

[bIgBV]

My question is basically will the copies will inherit the must_not_suspend?

If we limit the behavior to just the types, then the copies will inherit the behavior, since any instance of a type marked #[must_not_suspend] should not be suspended across await points. I think of it as less inheriting behavior, and more applying behavior applicable to an instance of the type. I'll update the doc to reflect this.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

[nikomatsakis]

I merged this, but I must have messed up the merge commit somehow. Closing manually.

[nikomatsakis]

Rendered: https://github.com/rust-lang/rfcs/blob/master/text/3014-must-not-suspend-lint.md