unused_must_use: Don't warn on Result<(), Uninhabited> or ControlFlow<Uninhabited, ()>

[joshtriplett]

This suppresses warnings on things like Result<(), !>, which helps simplify code using the common pattern of having an Error associated type: code will only have to check the error if there is a possibility of error.

This will, for instance, help with future refactorings of write! in the standard library.

As this is a user-visible change to lint behavior, it'll require a lang FCP.

---

My proposal, here, is that we handle this simple case to make common patterns more usable. This does not rule out the possibility of adding more cases in the future, including general trait-based cases. However, I don't think we should make this common case wait on the more general cases. In particular, this solution does not close any doors on replacing this special case with a general case.

This would unblock some planned work in the standard library to make write! more usable for infallible cases (e.g. writing into a Vec or String).

[rustbot]

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[fmease]

I'll be waiting for the outcome of T-lang's triage meeting before reviewing this since the proposed rules might very well change drastically (e.g., trait-driven vs. not trait-driven; cc Zulip discussion).

[joshtriplett]

Summarizing the Zulip discussion:

• What this PR does: We could do this by looking for any uninhabited type. This takes into account types that are not publicly known to be uninhabited, and types that are marked as #[non_exhaustive] (so they may not always be uninhabited).
• We could do this by hardcoding types like ! or Infallible.
• We could do this by identifying the types in some other way, such as a dedicated attribute, or a trait.
• We could require people to use Result::into_ok() (once stabilized). This would be less readable, though.
• Some discussion about cases like enums with variants that are sometimes configured out.

[joshtriplett]

My proposal, here, is that we handle this simple case to make common patterns more usable. This does not rule out the possibility of adding more cases in the future, including general trait-based cases. However, I don't think we should make this common case wait on the more general cases. In particular, this solution does not close any doors on replacing this special case with a general case.

This would unblock some planned work in the standard library to make write! more usable for infallible cases (e.g. writing into a Vec or String).

[Amanieu]

As a general rule I think this should follow the same rules what let Ok(()) = foo would accept.

[joshtriplett]

I've updated this to follow the same pattern as other inhabitedness checking, of only caring about non_exhaustive from external crates.

I've also updated it to handle ControlFlow.

[niacdoial]

huh...
github doesn't let me choose your branch as a PR destination.
er... anyway, I would like to contribute a test (making sure unresolved type aliases behave like inhabited types), if you'll let me:
https://github.com/niacdoial/rust-contrib-fork/tree/must_use_uninhab

[joshtriplett]

huh... github doesn't let me choose your branch as a PR destination. er... anyway, I would like to contribute a test (making sure unresolved type aliases behave like inhabited types), if you'll let me: https://github.com/niacdoial/rust-contrib-fork/tree/must_use_uninhab

Added, thanks!

[scottmcm]

Is this the same as "don't must_use warn on things that are ZSTs"?

Thus it could be phrased as "change the ()-specific rule to any ZST, not just the unit ZST".

[joshtriplett]

@scottmcm I don't want to generalize that here yet, because someone might e.g. have some #[must_use] ZST that represents "capture this token and pass it to some other function to prove you got an Ok", and I wouldn't want to override that. Some more general design could argue for that, but () is the case that I'm confident any more general solution will encompass.

[traviscross]

This makes sense to me, in particular how this helps with write!, etc., and the test cases seem right.

@rfcbot fcp merge

[rust-rfcbot]

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

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[scottmcm]

This feels weird to me. ControlFlow is all about not giving a particular "human intent" to one or the other (just getting the ? polarity you want), so I would expect ControlFlow<(), !> and ControlFlow<!, ()> to either both lint or both not lint.

(Not supporting Result<!, ()> I understand since we generally tell people not to use () there in the first place, but for ControlFlow both ways are plausible.)

[joshtriplett]

@scottmcm I don't have any strong opinions about ControlFlow here. My own impression was that it'd be confusing to do that, but I'd love to hear feedback from other users of ControlFlow.

[joshtriplett]

#t-lang > Making `#[must_use]` not warn about `Result<(), !>` @ 💬

[scottmcm]

Absolutely support the intent here, so
@rfcbot reviewed

I do have a non-quite-concern about the current implementation for ControlFlow that I left above. (If we had niko's new framework it'd be a "let's talk if you disagree", not a "need an FCP to override" kind of thing.)

[rust-rfcbot]

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

[nikomatsakis]

@rfcbot reviewed

Seems sensible.

[tmandry]

@rfcbot reviewed

I think this is a positive change for the language. I'd be happy to see a generalization down the line so that other types can make use of it. Maybe in the form of a diagnostic attribute on a type parameter, e.g.

#[must_use]
pub enum Result<T, #[diagnostic::may_discard_if_uninhabited] E> {
  Ok(T),
  Err(E),
}

[scottmcm]

I feel like if we're generalizing we might as well go to a trait-based solution so that, for example, impl<T: MustUse> MustUse for Option<T> becomes possible. (Maybe even things like impl !MustUse for () and such too.)

Otherwise we're recreating all these systems in diagnostics instead...

[tmandry]

Interesting. That would be a nice direction too. The biggest design question I can think of there is whether removing a MustUse impl (or changing its bounds, as this PR would be doing) is considered semver-breaking. Probably it shouldn't be.

[joshtriplett]

@tmandry Ideally it'd be impossible to reference the trait outside of impl MustUse for blocks.

Also, it's not clear how to implement the exceptions to MustUse with the structure of our trait system (e.g. Result<T, E> is MustUse except if...).

All that said, I'd love to see a design proposal for a more general version, and I'm also glad we're shipping this version. :)