Rust: (request) Extend `#[must_use]` to Pin

[jebrosen]

A type T with #[must_use] does not imply that Pin<T> is #[must_use]. This means that there is no hint to .await a Pin<Box<dyn Future>>, which is one of the current workarounds for the lack of aysnc fn in traits. This is used by async-trait in its expansion, and in some crates that have traits that "ought to" have async fns.

See also #39524, which would propagate must_use "everywhere" but was rejected, and #62228, which special-cased must_use propagation through Box only.

cc @varkor, who indicated they would provide mentoring advice.

[Centril]

If we do this I'd like to see an attribute #[rustc_conditional_must_use] or something to encode things in a less ad-hoc fashion.

[jonas-schievink]

impl<T: MustUse> MustUse for Pin<T> {}

[Centril]

@jonas-schievink's idea seems like an elegant way to afford users a lot of flexibility while also limiting (and reducing) the overall compiler complexity cost. It would still require some user intervention, but it seems well balanced.

cc @oli-obk -- I assume we'd use roughly the following impl strategy:

1. Introduce #[rustc_diagnostic_item = "must_use_trait"] trait MustUse {} in std::hint or maybe std::marker (cc @rust-lang/libs on location). It can be unstable for now while evaluating the usage inside the standard library and then considering whether to expose it to users.

   To cover #[must_use = "the reason text"], the full trait definition probably needs to be:

   #[rustc_diagnostic_item = "must_use_trait"]
   trait MustUse {
      const REASON: Option<&'static str> = None;
   }
   

2. When linting, first check if the trait is implemented for the trait, evaluate the REASON, if Some(reason), then use the reason and otherwise the effect is the same as #[must_use] with no reason provided.

3. Fall back to other mechanisms for e.g. trait objects.

[oli-obk]

At that point we can make #[must_use] for types to just expand to

impl MustUse for TheType {
    const REASON: Option<&'static str> = "the must use reason string";
}

Wrt marker types: Should MustUse leak from impl Trait returning methods just like Send and Sync?

[Centril]

At that point we can make #[must_use] for types to just expand to

If we use expansion, we need to think about #[must_use] on trait and fn definitions as well. For traits we should expand to MustUse for dyn TheTrait but we cannot do that for MustUse for impl TheTrait so we might need to expand to some other rustc-internal attribute or do something else. For fns we have a similar but bigger problem (since it's about function calls, not merely using the function's path). cc @petrochenkov

Wrt marker types: Should MustUse leak from impl Trait returning methods just like Send and Sync?

Currently there's no such leakage:

#![allow(dead_code)]

#[must_use]
struct A;

trait T {}

impl T for A {}

fn f() -> impl T { A }

fn g() {
    f();
}

I believe we also didn't want the structural auto-trait behavior here so I don't think it should leak. (Also, auto traits are all entirely empty at the moment whereas MustUse has REASON, and we should be careful of changing the trait system itself here.)

[nikomatsakis]

We discussed this in our @rust-lang/lang meeting on 2020-01-02. A few notes:

We don't have to block extending #[must_use] to Pin on finding some generic mechanism. We could add a rustc-internal mechanism like the one that @Centril initially proposed as an implementation mechanism, but without necessarily the expectation that this is something we would ever stabilize.

In general, there wasn't much consensus in the meeting about the idea of exposing this mechanism to end-users. At minimum an RFC would be required to hash out the design. (I personally am not that keen on the trait-based design, but I'll leave those concerns to a follow-up comment.)

[nikomatsakis]

(I personally am not that keen on the trait-based design, but I'll leave those concerns to a follow-up comment.)

As far as the idea of adding a trait for must-use, I have various concerns. First and foremost, that opens up a lot of expressive power, and I am far from convinced it makes sense. Like, do we really want the ability to have types that are "must use" but only if their argument types are T: Debug or something? This would imply that the lint has to start doing full trait solving and so forth as well. It just feels like way overkill for this problem.

Going along with the overkill theme, now this trait is something people can write code against. So they can write fn too<T: MustUse> -- does that even make sense? The function can only be used with MustUse types? (You would, presumably, then start to get lints if you fail to must-use.)

Finally, the trait doesn't solve the entire problem. It covers one use -- must-use types -- but not must-use functions. I feel like it's overall "not a net win" to have two distinct ways of declaring things must-use.

So yeah, as of this moment it seems to me like some kind of more limited #[must_use] annotation that can be placed on Box and Pin and indicates "this is must use if the type arugment is must use" would be "good enough".

[nikomatsakis]

That said, I am 👍 on extending #[must_use] to Pin in some simple way for now.

[Centril]

So yeah, as of this moment it seems to me like some kind of more limited #[must_use] annotation that can be placed on Box and Pin and indicates "this is must use if the type arugment is must use" would be "good enough".

I think my preferred generalization for now is to consider Deref types rather than just Box or Pin specifically. That also gives some degree of users being able to emulate this as well.

[nikomatsakis]

I think my preferred generalization for now is to consider Deref types rather than just Box or Pin specifically. That also gives some degree of users being able to emulate this as well.

I would be happy with that.

[nikomatsakis]

I guess it also introduces the trait system into the lint :) but other than that it seems ok.

[withoutboats]

I'm not convinced deref types that don't imply ownership should be must_use since the target value could be used by another piece of code.

[nikomatsakis]

Yeah, it may be overkill. Are you thinking of "convenience" Deref impls that (for example) emulate subclassing, @withoutboats?

In any case I'd be happy to start with just an attribute and apply it to Pin, personally.

[withoutboats]

Yeah, it may be overkill. Are you thinking of "convenience" Deref impls that (for example) emulate subclassing, @withoutboats?

No I'm thinking of references. For example I don't think an &mut dyn Iterator or Pin<&mut dyn Future> should be must_use since dropping them does not drop the iterator/future. There are other deref types which when applied to these types couldn't even be used at all (e.g. &Future or Arc<Iterator>). Only Box and Pin<Box> actually have semantics that make sense for the must_use lint in my opinion.

[nikomatsakis]

Good point. Seems correct that &Result<...> isn't obviously "must use".

[tmandry]

Can we extend this to Option<T> as well? See #71368.

[jimblandy]

Another case: it'd be nice if executors' JoinHandle<T> type could be must_use when T is must_use. I just wasted a bit of time on something that reduced to this (playground):

async fn foo() { ... }
...
tokio::spawn(async { foo() });

Here, spawn returns a JoinHandle<impl Future>, the future of foo's completion, which is dropped without comment.

Of course, the bug there is actually a missing await in the async block. But is a Future<Output=impl Future> suspicious enough to warn about?

[nikomatsakis]

Interesting example. In a recent @rust-lang/lang meeting we were wondering about the extent to which we ought to warn about a type Foo<X> where X is must-use (we had previously decided in https://github.com/rust-lang/rust/pull/62262 to be more conservative, although that PR took a more aggressive approach, actually checking field types and not just type arguments). It feels like we need to collect this design discussion in one place and examine the alternatives more holistically.