RFC: applying #[warn(clippy::let_underscore_must_use)] tree-wide

[iliana]

Opening an issue for this, as I am planning to do this after a Rust 1.81 toolchain bump (so that I can use #[expect]), which we should do after R11 is out, so I'd love to gather some comments before I pick up this work.

Rationale

In my opinion (which, notably, diverges from the Rust reference book), let _ = ... is a code smell. This is a "non-binding let", whose semantics (as best as I can tell) are best explained by first demonstrating how they're defined in the context of match patterns, such as this manual implementation of Result::ok:

let value = match something_that_could_fail() {
    Ok(value) => Some(value),
    Err(_) => None,
};

That is, if the value we're matching on is the Err variant, we pretty explicitly do not care about what the value is. Rust doesn't bind it to anything, and thus it immediately goes out of scope. Roughly the same thing happens if we create a named binding:

let value = match something_that_could_fail() {
    Ok(value) => Some(value),
    Err(_err) => None,
};

Except there is a subtle difference. Here, the value is bound to _err, which we then don't use, and is dropped at the end of scope (in this case, the match arm). When we use _, Rust simply does not bind the value to a name, and the value immediately goes out of scope.

Something not immediately obvious is that let is a lot closer to a match pattern than anything else; for instance, you can do destructuring with it, with the same syntax of a match pattern:

use std::process::Output;
let Output { status, stdout, .. } = command.output()?;

And thus the semantics of _ are the same with let, too.

// Binds the `MutexGuard` to `guard`, but we don't use it, so the compiler complains.
let guard = mutex.lock();
// This isn't correct either, because the `MutexGuard` isn't bound to anything
// and goes immediately out of scope, releasing the lock.
mutex.lock();
// The programmer could try two things: they could try this, which would bind the guard
// until `_guard` goes out of scope...
let _guard = mutex.lock();
// or they might try this, but they'd be _wrong_, because it immediately goes out of scope!
let _ = mutex.lock();

This behavior is really non-obvious: _ does not create a binding, but _guard does. Nothing makes it clear that _ is special. I've learned it the hard way, and have preemptively taught this to people interacting with RAII guards, usually to the other programmer's shock and horror.

In the particular case of lock guards (for std and parking_lot) and futures, Rust and Clippy have on-by-default lints that cover these. But for other types that are marked #[must_use], there is no on-by-default lint. This is likely because let _ = ... is considered by the Rust reference to be idiomatic Rust for explicilty ignoring #[must_use] (see also the discussion on rust-lang/rust-clippy#8246). But from the perspective of someone using a guard of any kind (e.g. tracing::span::Entered, xshell::PushDir) it is not initially obvious that let _ is different from let _guard.

Proposed work

I would like to enable Clippy's let_underscore_must_use lint in our workspace lints configuration. This lint is allow-by-default (for the reason noted above). There are currently 151 expressions that trigger this lint.

The PR would follow this strategy:

1. The most common class of expression to fix is where we want to explicitly ignore a Result<T, E> (that is, we do not care if an error occurred, and we also don't care about the successful return value). In cases where T is not also #[must_use], my plan is to instead quiet the unused_must_use lint by calling Result::ok.
2. Where it is clear without any shred of doubt that immediately dropping something marked #[must_use] is correct, I will replace let _ = ... with drop().
3. Other types of expression would be spot-checked for bugs. If I can't identify anything immediately wrong with the code, I'll apply #[expect(clippy::let_underscore_must_use)] to the expression. (This is why I want to wait until we update to 1.81.)

Alternatives

It would also be nice to live in a world where let _: Result<T, E> = ... is fine (if T is not #[must_use]), but still have this lint run on anything else that might be a guard, so we could send a patch to Clippy to add a configuration parameter for the let_underscore_must_use lint. But the let_underscore_* lints are kind of disorganized so I think any patch would have a high chance of being bikeshedded (should the option be an exclude or include list? should it apply to let_underscore_must_use or another lint? what happens if you add an exception that's covered by another lint? etc etc)

[iliana]

I might also apply #[warn(let_underscore_drop)], which I only didn't notice because it was uplifted from Clippy to Rust: https://doc.rust-lang.org/rustc/lints/listing/allowed-by-default.html#let-underscore-drop