Open aclysma opened 2 years ago
clippy::let_underscore_lock
exists and is enabled by default, but it is special cased to specific locks so it doesn't catch much. Two other lints are let_underscore_drop
lint (in pedantic group) and let_underscore_must_use
(in restriction group) which are both disabled by default.
In the the case of https://github.com/amethyst/distill/commit/e7e1d254816096dad220cb4571b0cdd6bf245eb9, I think it would have been caught with let_underscore_must_use
enabled since tokio
's EnterGuard
has #[must_use]
.
I can imagine a new lint that warns on every use of let _ =
as you mention, but inside the restriction category to be selectively enabled rather than the correctness group, which are mainly deny by default and contains the drop_copy
lint that could conflict with using drop
as an alternative.
Fwiw I've encountered this with profiling score guards as well, this is not really limited to locks.
Lots of people probably like the let _ = X construction for things like receiving a result as a return value and explicitly deciding to not check it. This is by many considered the idiomatic way to ignore a return value.
For what it's worth the Rust Reference book does state that it is idiomatic for ignoring must-used return values: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_use-attribute
Note: It is idiomatic to use a let statement with a pattern of
_
when a must-used value is purposely discarded.
What it does
If you write code like this:
The guard will drop and you will not hold the lock. This catches a lot of people by surprise and has potentially disastrous consequences. (See prior discussion of this here https://github.com/rust-lang/rust/issues/10488)
If instead you write
You will hold the lock until _guard is dropped out of scope. This is almost always what people want for something RAII-like such as a lock (or file handle, DB transaction, etc.)
If the
let _ = lock.guard()
construction was considered hazardous, we could instead use the following alternatives to be explicit about what we want:drop(lock.lock());
(which in this case makes the mistake obvious)let _guard = lock.lock();
Because
let _ = ...
is subtly hazardous, and there are more explicit constructions to do the same thing, I would personally like to ban this pattern in my own code.NOTE: As a more practical example in real code, I made a fix in some code a while back https://github.com/amethyst/distill/commit/e7e1d254816096dad220cb4571b0cdd6bf245eb9 and originally tried to write it as
let _ = self.0.runtime.enter();
which was no different from before. There are also other examples of people hitting this in https://github.com/rust-lang/rust/issues/10488Lint Name
let_underscore_can_be_hazardous
Category
correctness
Advantage
Potentially catches serious resource management problems
Drawbacks
Lots of people probably like the
let _ = X
construction for things like receiving a result as a return value and explicitly deciding to not check it. This is by many considered the idiomatic way to ignore a return value.It would be nice if there was a way to apply this lint only when the returned object is an RAII-style object. (Similar to how unused
Results
are flagged.) But I'm not sure how practical that would be, especially considering containers and opaque trait objects.Example
let _ = function_call(x);
Depending on intent, could be written as:
drop(function_call(x));
orlet _unused = function_call(x);