Add #[must_use] annotation to Result::ok

[yotamofek]

I've encountered a few instances of people (@ work) writing these types of snippets:

some_fallible_op().inspect_err(|e| eprintln!("Got an error: {e}")).ok();

IMHO the idiomatic way to write this would be:

if let Err(e) = some_fallible_op() { eprintln!("Got an error: {e}"); }

or, if you're just discarding the error and don't need to log it

let _ = some_fallible_op();

I think using .ok() for discarding an error is misleading and makes the code harder to read. I wouldn't propose adding #[must_use] to Option itself (as opposed to Result) is wrong, but it makes a lot of sense to me that .ok() is missing its purpose if you're not using its return value!

Let's see what people feel about this...

(Edit: fixes #66116 , which I missed before opening this)

[rustbot]

r? @jhpratt

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

[Noratrieb]

It is a fairly common pattern to write .ok() to ignore errors. Not everyone does it and I don't think it's officially recommended, but it probably shouldn't be listed against.

[Noratrieb]

If this is a hill you're willing to die on, it would probably make sense to nominate this for libs api team discussion on whether this should be done or not.

[yotamofek]

I'm willing to climb the hill and quickly run back down if I find people are trying to kill me. 😬
How do I nominate it?

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[RUSTC-TIMING] ctrlc test:false 0.066
   Compiling derive_arbitrary v1.3.2
[RUSTC-TIMING] proc_macro_srv test:false 0.370
    Checking arbitrary v1.3.2
error: unused return value of `std::result::Result::<T, E>::ok` that must be used
   --> src/tools/rust-analyzer/crates/ide-db/src/prime_caches.rs:135:13
    |
135 |             work_sender.send((crate_id, name, PrimingPhase::DefMap)).ok();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D unused-must-use` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(unused_must_use)]`
help: use `let _ = ...` to ignore the resulting value
    |
135 |             let _ = work_sender.send((crate_id, name, PrimingPhase::DefMap)).ok();
    |             +++++++

error: unused return value of `std::result::Result::<T, E>::ok` that must be used
   --> src/tools/rust-analyzer/crates/ide-db/src/prime_caches.rs:176:9
    |
176 |         work_sender.send(w).ok();
    |         ^^^^^^^^^^^^^^^^^^^^^^^^
    |
help: use `let _ = ...` to ignore the resulting value
    |
176 |         let _ = work_sender.send(w).ok();
    |         +++++++

[RUSTC-TIMING] ide_db test:false 2.134
error: could not compile `ide-db` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...

[jieyouxu]

I'm willing to climb the hill and quickly run back down if I find people are trying to kill me. 😬 How do I nominate it?

You can use @rustbot label +I-libs-api-nominated, but please do include what you want libs-api to discuss (with proper context) and what outcomes/decisions you are looking for in the nomination message so ppl don't need to look around for context.

[mohe2015]

An alternative would be a clippy lint, right?

[RalfJung]

Personally I often use ok() to explicitly say that I am ignoring the error, so I'm not a fan of this change. I think let _ = ... looks quite a bit worse and it makes it even less clear what we are ignoring -- ok() at least ensures we are ignoring a Result. If the return type changes, let _ = ... can end up being quite surprising.

But I'll accept whatever t-libs-api says about the preferred style here, this is not a strong opinion.

[tbu-]

Personally I often use ok() to explicitly say that I am ignoring the error

When reading code, I didn't understand that ok() was meant to explicitly ignore the error.

I think it doesn't make sense to use a relatively unrelated method on Result to silence the "result must be used" warning.

[slanterns]

An alternative would be a clippy lint, right?

https://rust-lang.github.io/rust-clippy/rust-1.85.0/index.html#unused_result_ok

[yotamofek]

After thinking about this a bit more, I'm reluctant to actually nominate this for libs team consideration.

I think that even if 100% of people disliked the use of .ok() for ignoring errors, the churn that would be caused by this change would be something to dissuade the team from accepting it. Given that some people find this usage to be preferable (and @RalfJung made a very strong point, IMO, about let _ = .. being less explicit about the type being ignored), I find it hard to believe that t-libs will accept this, and I don't see the point in wasting peoples' time with a proposal that doesn't have a decent chance of being accepted.

Also, the clippy lint is really a good enough solution for me personally.

Closing this PR for now.

(Edit: though I still prefer let _ = .. over .ok() for ignoring errors)