Extend #[must_use] to nested structures

[varkor]

Extends the #[must_use] lint to apply when #[must_use] types are nested within structs (or one-variant enums), making the lint much more generally useful. This is in line with #61100 extending the lint to tuples.

Fixes #39524.

cc @rust-lang/lang and @rust-lang/compiler for discussion in case this is a controversial change. In particular, we might want to consider allowing annotations on fields containing #[must_use] types in user-defined types (e.g. #[allow(unused_must_use)]) to opt out of this behaviour, if there are cases where we this this is likely to have frequent false positives.

(This is based on top of #62235.)

[rust-highfive]

r? @estebank

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

[rust-highfive]

⚠️ Warning ⚠️

• These commits modify submodules.

[Centril]

This should probably go into a separate method and the comments above can become doc comments...

[estebank]

I'm not available for the next couple of days due to personal reasons. I would appreciate it if someone else could take over review.

[Mark-Simulacrum]

I don't think the review is in any rush - this definitely need lang-team sign off (and maybe libs?); I feel like we've discussed this previously and concluded that we shouldn't do it, IIRC because it expands it far enough that it feels like it "always" works which isn't actually the case. I could be projecting my own thoughts on the matter though.

[Centril]

I don't think the review is in any rush - this definitely need lang-team sign off (and maybe libs?);

I nominated this for the lang team. The libs team doesn't deal with compiler lints.

---

I personally feel that the false-positive cases are edge cases that are rare enough that they don't outweigh the big benefit this PR brings.

We could however tweak this a bit to look for a Drop implementations for the ADT and then bail on the structural logic tho that might be too subtle. An opt-out would also be decent.

[Mark-Simulacrum]

I disagree a little bit about the libs team bit here -- it seems like this lint is predominantly used for libraries, not language features/things, and so their opinion matters more. At the very least, they should be notified (cc @rust-lang/libs).

[Nemo157]

Would it make sense to do a crater run of this to see how useful the warnings it produces in real code are?

[nikomatsakis]

Discussed briefly in @rust-lang/lang -- we agree a crater run would be the logical next step before making any further decisions. Can we set this to deny by default and do a run?

[estebank]

r? @eddyb

[varkor]

@bors try

[bors]

⌛ Trying commit 8bcf64a with merge ea663bb...

[bors]

☀️ Try build successful - checks-azure
Build commit: ea663bb (ea663bba38739867a4b75ac820991b4f5d093c3b)

[varkor]

@craterbot check

[craterbot]

👌 Experiment pr-62262-1 created and queued.
🤖 Automatically detected try build ea663bb
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-62262-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-62262-1 is completed!
📊 97 regressed and 0 fixed (74234 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[varkor]

23 crates produced the new lint (most of the regressions caught by crater were downstream). They all seem genuine to me, though in most cases they were deliberately (though not explicitly) ignored (in things like drop impls, etc.). In these cases, let _ = ...; should be appropriate.

I think this crater run demonstrates the change is not very invasive, but can catch genuine errors, and is worth merging.

• arraydeque
• midir
• web-view
• safe-graph
• sodium
• renamefs
• geniza
• tetris_piston
• prometrics
• synchronize_images
• corona
• lingo_proxy
• tox
• tox
• connetr
• runtime
• slice-deque
• stdweb
• frappe
• botana-api-rs
• lifegame
• mdl
• sodium-rust

[estebank]

The one case I noticed that might be reasonable to allow is calling drain.

[varkor]

I don't think that pattern is common: the only reason that instance was linted is because they were implementing clear using drain(..). It seems reasonable to want let _ = self.drain(..); in this case.

[varkor]

Nominating for lang team re-review, now that the original concerns have been addressed.

[cramertj]

We discussed this in the lang meeting today and there was general agreement that the lint as-implemented in this PR isn't something we're interested in landing. This change would make #[must_use] operate similarly to an auto-trait in that #[must_used]-ness would be inherited from fields, but with no ability to turn off the #[must_used]-ness of a type which contains a #[must_use] type. A number of the examples in the crater run above (e.g. Drain) demonstrate that this is not what authors intend by #[must_use], and in many cases creates warnings that are misleading or unhelpful.

There was some interest in a "conditional-must-use" based on type parameters, e.g. Box<T>/Option<T> could be #[must_use] iff T is #[must_use], but extending this behavior to user-defined types would require an RFC. A #[rustc_conditional_must_use] or similar could be appropriate if we want to limit this to std types.

[eddyb]

@cramertj "conditional #[must_use]" sounds like #[must_use] on data types should've been a trait with an associated const (for the reason), since then you can just rely on bounds.

I think we can still do that with an unstable trait, and hook it up either through the trait system, or macro expansion, so both #[must_use] and the trait work with the lint.