-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix await_holding_lock
not linting parking_lot
Mutex/RwLock
#8419
Conversation
r? @giraffate (rust-highfive has picked a reviewer for you, use r? to override) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the reviewer, but this looks good in general. One point: if this still has bugs, we should document them in the lint docs. Otherwise should we re-group this to suspicious
so it at least warns by default?
tests/ui/await_holding_lock.rs
Outdated
@@ -1,64 +1,194 @@ | |||
#![warn(clippy::await_holding_lock)] | |||
|
|||
use std::sync::Mutex; | |||
extern crate parking_lot; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed because we're in a UI test? Elsewhere I'd just use
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. I just copied this from the let_underscore
test, that also tests for the parking_lot crate. I'll try if it also works with use
, now that we're using the 2018/2021 (?) edition for testing.
I think the bug for which I added a test is already documented under
I'll try to do that and run the |
Improves the message of the lints await_holding_lock and await_holding_refcell_ref. Now also actually tests RwLock.
This adapts the paths for the parking_lot mutex guards, so that parking_lot mutexes and RwLocks actually get linted. This is now also tested.
Even though the FP for that the lints were moved to pedantic isn't fixed yet, running the lintcheck tool over the most popular 279 crates didn't trigger this lint once. I would say that this lint is valuable enough, despite the known FP, to be warn-by-default. Especially since a pretty nice workaround exists.
775b795
to
ea0ce7b
Compare
I ran the lintcheck tool on the top 279 downloaded crates on crates.io, where grepping for Actual changes in this rebase to previous version:
|
Great job! Fingers crossed that it'll work as great in production. @bors r+ |
📌 Commit ea0ce7b has been approved by |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
This adds tests for
RwLock
andparking_lot::{Mutex, RwLock}
, which were added before in 2dc8c08, but never tested in UI tests. I noticed this while reading fasterthanli.me latest blog post, complaining that Clippy doesn't catch this forparking_lot
. (Too many people read his blog, he's too powerful)Some more things:
changelog: [
await_holding_lock
]: Now also lints forparking_lot::{Mutex, RwLock}