Consider Result<T, Uninhabited> and ControlFlow<Uninhabited, T> to be equivalent to T for must use lint

[WaffleLapkin]

This is an extension to #147382.

With this PR Result<T, Uninhabited> and ControlFlow<Uninhabited, T> considered as must use iif T must be used.

For such cases the lint will mention that T is wrapped in a Result/ControlFlow with an uninhabited error/break.

The reasoning here is that Result<T, Uninhabited> is equivalent to T in which values can be represented and thus the must-used-ness should also be equivalent.

[rustbot]

r? @fee1-dead

rustbot has assigned @fee1-dead.
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

[fee1-dead]

impl LGTM

View changes since this review

[jackh726]

So, the lang team discussed this in the triage meeting today. Overall sentiment was positive that this is probably something we want to do at some point, but there was one concern: people may rely on this today to ensure that people "observe" the state of the Ok variant, since we don't have a nice way today to mark that as must_use without making a newtype. So, to give a concrete example:

fn foo<E>() -> Result<u32, E>; // people may expect that you will check the value of the `u32`

I suggested that we could actually just get some real word data on this using crater. My thought was, we could do an experiment were we error on any function that (after monomorphization) returns Result<T, !> for any T that is not must_use (and maybe separately for any T that is must_use to get a counter set).

I do expect that this will result in a non-small amount of cases, but I think it might be a small enough set that we could browse through and get a sense of if this type of thing is relied upon.

Would you be interested in doing that experiment @WaffleLapkin? If not, I'm happy to help out and try that myself (or I can help if you do want to be involved).

[WaffleLapkin]

@jackh726 I'd be happy to do the experiment. I might reach out for some help though ^^'

[jackh726]

Sounds good! Let me know, happy to help if you run into issues.

[jackh726]

Copying this from #148577 (comment)

So, what's very clear is that this pattern comes in a lot of places. Definitely more than I would have expected coming into this. With this being said, I've gone through and opened a few crater results to see if I could spot any examples of @joshtriplett's concern, which was basically like "People may be relying on that Result<T, E> is always must_use to ensure that T is also must_use". The idea (from what I can try to summarize) was that perhaps there is some state in T that would be need to be observed in order to avoid subtle bugs.

So, going into this, I'm looking for types where the Ok variant seems to encode some "meta-state" that must be observed for correctness. There are obviously way too many examples for me to be exhaustive here. And, I have yet to find a situation that follows the pattern to be worried about.

That being said, given that this comes up so much, I think this decision is likely not as "light" as the lang team originally expected. Though, it's completely backwards- and forwards- compatible to make this change and undo later. Here a couple of examples of code I saw, though far from exhaustive.

no_std_net

pub trait ToSocketAddrs {
    type Iter: Iterator<Item = SocketAddr>;
    fn to_socket_addrs(&self) -> Result<Self::Iter, ToSocketAddrError>;
}
pub enum ToSocketAddrError {}

zino_http

let session_id = self
    .get_header("x-session-id")
    .or_else(|| self.get_header("session_id"))
    .and_then(|s| s.parse().ok()); // <- this parse relies `<String as FromStr>` which has `Err = Infallible`
ctx.set_session_id(session_id);

zerocopy

fn checked_shr(self, rhs: Self) -> Option<Self> { self.checked_shr(rhs.try_into().unwrap_or(u32::MAX)) }
                                                                                                     //^^^^^^^^ returns `Result<u32, Infallible>`

clap things

(this comes up a bunch, so just putting an example error, as I haven't dug into the actual pattern; it's also not super clear to me what the actual lint would be)

error: this type will no longer be must used: Result<std::string::String, Infallible>
  --> src/main.rs:72:9
   |
72 | /         /// A path to a project directory or a `Cargo.toml` file. If this is
73 | |         /// not provided, the current directory will be searched.
74 | |         #[clap(verbatim_doc_comment)]
75 | |         path: Option<String>,

[jackh726]

Going to renominate for lang, since we got some crater results and worth discussing.

[tmandry]

@rfcbot fcp merge lang

I think we should merge this, for a few reasons.

1. It's intuitive that the lint should work this way.
2. It reduces noise of #[must_use] lints, leading people to pay more attention to them.
3. For any case where this removes a lint where that removal was not desired, there has always been a more direct way to express your intent: Putting #[must_use] on the function.

We talked about this in the lang call and some people brought up that returning Result<u32, Uninhabited> could be used to make sure a caller looks at the u32. But we have #[must_use] on functions which should be the way that pattern is expressed.

For code that already returns Result<_, Uninhabited> it's true that this removes some cases that lint, but it's not overly likely that the author intended this; it's just as likely that this lint is noise. If the author did intend this they could have, and arguably, should have put #[must_use] on their function with a message to make the intent clear.

@scottmcm noticed that today, clippy lints on the case where you do this:

#[must_use]
fn foo() -> Result<i32, Uninhabited> { Ok(42) }

warning: this function has a `#[must_use]` attribute with no message, but returns a type already marked as `#[must_use]`
 --> src/lib.rs:2:1
  |
2 | fn foo() -> Result<i32, Uninhabited> {todo!()}
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: either add some descriptive message or remove the attribute
  = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.91.0/index.html#double_must_use
  = note: `#[warn(clippy::double_must_use)]` on by default

However, that lint is only if the #[must_use] attribute includes no message, and arguably the function should have a message to explain why the return value must be used.

While I think we should tweak this clippy lint so it no longer fires on Result<_, Uninhabited>, I don't think its existence should be enough to deter us from making this change now, because I don't think it will change anything substantially. This is the way I think we all expect must_use to work, and without changing it there's no way to create pressure for crates that happened to rely on an unintended consequence of its current behavior (which to my knowledge, we haven't actually found any examples of) to add #[must_use] to their functions.

[rust-rfcbot]

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

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

Concerns:

• need-to-stop-warning-on-explicit-must_use-on-function-returning-Result-with-uninhabited (Consider Result<T, Uninhabited> and ControlFlow<Uninhabited, T> to be equivalent to T for must use lint #148214 (comment))

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.

[traviscross]

Agree with @tmandry.

@rfcbot reviewed

[joshtriplett]

I think we should merge this as well. I also think we need to decide on a transition plan. That transition plan could be anything from "loud release notes" to "new lint", though the latter would need some thought on how to mark functions you don't want to add #[must_use] to.

Not filing this as a concern at this time, though.

[joshtriplett]

However, one thing I will go ahead and file a concern on: today, if you try to add #[must_use] to a function returning Result<T, !> because you want to preserve the #[must_use] after this change, you'll get a clippy warning. People should not be in the position where old Rust complains about something needed for compatibility with the very next Rust version.

@rfcbot concern need-to-stop-warning-on-explicit-must_use-on-function-returning-Result-with-uninhabited

[WaffleLapkin]

@joshtriplett currently clippy re-implements the is_must_use check, and does so incorrectly¹ (yay fun). Would it be enough for your concern if clippy would be changed to use the function from rustc and then clippy behavior would be changed at the same time as this merging?

Footnotes

1. it doesn't account for some updates that have happened in rustc ↩

[Darksonn]

However, one thing I will go ahead and file a concern on: today, if you try to add #[must_use] to a function returning Result<T, !> because you want to preserve the #[must_use] after this change, you'll get a clippy warning. People should not be in the position where old Rust complains about something needed for compatibility with the very next Rust version.

rfcbot concern need-to-stop-warning-on-explicit-must_use-on-function-returning-Result-with-uninhabited

Don't we have many examples of violating this? One that comes to mind right away is making &raw const (*union).field into a safe operation, which triggers warnings from unused unsafe blocks if you want to support the compiler prior to that change.

[scottmcm]

Don't we have many examples of violating this?

We generally try to make it so there's a 1-release transition window. We don't need to worry about years-old versions, but we generally don't want to say that people have to update immediately because both things happen at once.

So for example, here we'd give people one release to add the function-level must_use where it's not warned on, then the next release change the lint to need it.

(Like how we stopped unneeded_unsafe warning on unsafe blocks in unsafe fn at least one release before insisting on them.)

[traviscross]

cc @rust-lang/clippy @rust-lang/clippy-contributors

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.