Skip to content
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

Merged
merged 4 commits into from
Feb 18, 2022

Conversation

flip1995
Copy link
Member

This adds tests for RwLock and parking_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 for parking_lot. (Too many people read his blog, he's too powerful)

Some more things:

changelog: [await_holding_lock]: Now also lints for parking_lot::{Mutex, RwLock}

@rust-highfive
Copy link

r? @giraffate

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 12, 2022
Copy link
Contributor

@llogiq llogiq left a 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?

@@ -1,64 +1,194 @@
#![warn(clippy::await_holding_lock)]

use std::sync::Mutex;
extern crate parking_lot;
Copy link
Contributor

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.

Copy link
Member Author

@flip1995 flip1995 Feb 15, 2022

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.

@flip1995
Copy link
Member Author

One point: if this still has bugs, we should document them in the lint docs.

I think the bug for which I added a test is already documented under Known Problems, but the documentation could be improved.

Otherwise should we re-group this to suspicious so it at least warns by default?

I'll try to do that and run the lintcheck tool.

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.
@flip1995
Copy link
Member Author

I'll try to do that and run the lintcheck tool.

I ran the lintcheck tool on the top 279 downloaded crates on crates.io, where grepping for .await gave 7645 matches. The await_holding_lock lint didn't trigger once. So IMO we could move it to suspicious.

Actual changes in this rebase to previous version:

  • Moved lints to suspicious
  • removed extern crate from test
  • Improved documentation (This is now doc-tested)

@llogiq
Copy link
Contributor

llogiq commented Feb 18, 2022

Great job! Fingers crossed that it'll work as great in production.

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 18, 2022

📌 Commit ea0ce7b has been approved by llogiq

@bors
Copy link
Collaborator

bors commented Feb 18, 2022

⌛ Testing commit ea0ce7b with merge 02f3c17...

@bors
Copy link
Collaborator

bors commented Feb 18, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 02f3c17 to master...

@bors bors merged commit 02f3c17 into rust-lang:master Feb 18, 2022
@flip1995 flip1995 deleted the await_parking_alot branch February 18, 2022 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants