add let_underscore_lock lint

[basil-cow]

closes #1574
changelog: add let_underscore_lock lint

I am not entirely sure about my docs/messages wording here, improvements are welcome

[JohnTitor]

I think it'd be nice to separate test files.

[basil-cow]

Oh, and I forgot to ask your input about this: is there a good reason to not lint let _ = <expr with MutexGuard/RWGuard ty>?

[flip1995]

is there a good reason to not lint let _ = <expr with MutexGuard/RWGuard ty>?

I cannot come up with one. When a guard is produced in any way, dropping it immediately is probably a mistake.

This can be nicely checked with TyS::walk.

// Pseudocode ahead
expr_ty(init_expr).walk().any(|ty| GUARD_TYPES.any(|gty| match_ty(gty, ty)));

[basil-cow]

I just thought, someone might use let _ = mutex_lock to drop the lock prematurely. Maybe we should lint all the functions that return a guard.

[flip1995]

I just thought, someone might use let _ = mutex_lock to drop the lock prematurely.

In that case, I think drop(mutex_lock) would be way better.

[basil-cow]

@flip1995 from the documentation it seems like TyS::walk walks generic parameters on a type recursively, why is it relevant here?

[flip1995]

Result<_Guard, _> is pretty usual, I think. And we also want to detect this Guard types.

[basil-cow]

Oh, that's what you mean, gotcha

[flip1995]

LGTM, only formatting left to do.

[flip1995]

double indentation

[basil-cow]

Why doesn't fmt do that?

[flip1995]

rustfmt currently only formats in some macros. The if_chain macro is too complex, for rustfmt to format the code in it reliably.

[flip1995]

@bors r+

Travis has currently problems, so I marked it as S-waiting-on-rustup to merge this as soon as travis works again.

[bors]

📌 Commit 63ab7a5 has been approved by flip1995

[bors]

⌛ Testing commit 63ab7a5 with merge c13268a...

[bors]

💥 Test timed out

[flip1995]

@bors retry

[bors]

⌛ Testing commit 63ab7a5 with merge 668bc48...

[bors]

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing 668bc48 to master...

[mleduque]

I see this in my code now.
There are
let _ = SHARED_OBJECT.lock().map(|content| { ... });

Is that wrong ?

[flip1995]

Hm thinking more about this: With your map(..) call, content is not the thing inside the Mutex, but the MutexGuard itself. So your type is probably (?):

let _ = SHARED_OBJECT.lock().map(|content: MutexGuard<impl Iterator<Item = _>>| { ... });

From the lock documentation:

If another user of this mutex panicked while holding the mutex, then this call will return an error once the mutex is acquired.

So mapping the result of the lock() call only really makes sense, if you do some error handling (other thread paniced) for the above case in there. But I wouldn't do error handling in a map call, but rather in a match _.lock() { Ok(_) => { ... }, Err(_) => panic!("..."),. Or you do

SHARED_OBJECT.lock().expect("..").map(|content: <Iterator>::Item>| { ... })

[mleduque]

I had to check, I didn't write the code, it was just pointed to me by clippy (that's totally the point of clippy, eh! Thanks by the way).

It seems the error case is actually ignored, like I wrote above. This is probably a bug in itself (or maybe not, I'll have to check the intent with the original authors, I guess).

[flip1995]

I'll have to check the intent with the original authors

If it should be a bug of this lint, feel free to open an issue about it 👍

[mleduque]

I don't think I'm qualified enough to decide if there is a bug or not :D

I also found some
let _ = SHARED_OBJECT.lock().map_or_else(|error| {...}, |content| { ... })

So I think some of the map ones may actually be as intended (but possibly not all of them, but I haven't yet had the authors input on the subject).