Flag let _ = ... as dangerous

[aclysma]

What it does

If you write code like this:

let _ = lock.lock();

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 rust-lang/rust#10488)

If instead you write

let _guard = lock.lock();

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:

• Either explicitly drop: drop(lock.lock()); (which in this case makes the mistake obvious)
• Or name the variable: 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 amethyst/distill@e7e1d25 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 rust-lang/rust#10488

Lint 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)); or let _unused = function_call(x);

[memoryruins]

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 amethyst/distill@e7e1d25, 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.

[Jasper-Bekkers]

Fwiw I've encountered this with profiling score guards as well, this is not really limited to locks.

[X-m7]

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.