Add new lint: rc_mutex
#7316
Add new lint: rc_mutex
#7316
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
clippy_lints/src/types/mod.rs
Outdated
| declare_clippy_lint! { | ||
| /// **What it does:** Checks for `Rc<Mutex<T>>`. | ||
| /// | ||
| /// **Why is this bad?** `Rc<Mutex<T>>` may introduce a deadlock in single thread. Consider |
There was a problem hiding this comment.
This would also apply to Arc<Mutex<T>>. The distinction is that Rc is not thread-safe, and thus the Mutex can never be shared.
This may also mean that Arc<Mutex<T>> was intended, so suggesting Rc<RefCell<T>> would go into the wrong direction.
clippy_lints/src/types/rc_mutex.rs
Outdated
| if let TyKind::Path(ref qpath_inner)=ty.kind; | ||
|
|
||
| then{ | ||
| let mut applicability = Applicability::MachineApplicable; |
There was a problem hiding this comment.
Unless RefCell has the same or a superset of Mutex's interface, applying this automatically can lead to compiler errors. Therefore we should not make this auto-applicable.
Worse, it's never sufficient to just change one type.
clippy_lints/src/types/rc_mutex.rs
Outdated
| cx, | ||
| RC_MUTEX, | ||
| hir_ty.span, | ||
| "you seem to be trying to use `Rc<Mutex<T>>`. Consider using `Rc<RefCell<T>>`", |
There was a problem hiding this comment.
| "you seem to be trying to use `Rc<Mutex<T>>`. Consider using `Rc<RefCell<T>>`", | |
| "found `Rc<Mutex<_>>`. Consider using `Rc<RefCell<_>>` instead", |
| Two, | ||
| } | ||
|
|
||
| pub fn test1<T>(foo: Rc<Mutex<T>>) {} |
There was a problem hiding this comment.
I'd like to see a test that actually creates an instance.
There was a problem hiding this comment.
Could you please check if I added correct test in my latest commit?
I'm not sure I did it right.
tests/ui/rc_mutex.rs
Outdated
|
|
||
| pub fn test3(foo: Rc<Mutex<SubT<usize>>>) {} | ||
|
|
||
| fn main() {} |
There was a problem hiding this comment.
How about calling test1, test2, etc. from here?
This is something the lint might want to consider, btw., and also the reason not to apply it automatically.
|
Is there an issue open for this lint? Or was it discussed in any other place beforehand? If so, please link to it in the PR description. |
|
There is no related issue and discussion. |
clippy_lints/src/types/mod.rs
Outdated
| /// **Why is this bad?** `Rc` is used in single thread and `Mutex` is used in multi thread. | ||
| /// Consider using `Rc<RefCell<T>>` in single thread or `Arc<Mutex<T>>` in multi thread. | ||
| /// | ||
| /// **Known problems:** None. |
There was a problem hiding this comment.
I think there may be instances where combining generics and traits could require this actual type combination, and following the suggestion would make it impossible to implement the traits.
We should consider those cases false positives and make a note of a possible problem. I don't think this problem is worrisome for the lint (it's unlikely to happen in practise), but we should note it nonetheless.
|
Sorry for letting you wait for so long. I had some $life keeping me busy. I'd like a more fleshed out explanation of the possible false positives. How about: "Sometimes combining generic types can lead to the requirement that a type use |
|
Thank you! @bors r+ |
|
📌 Commit b9cc2fe has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
changelog: Add new lint
rc_mutex.It lints on
Rc<Mutex<T>>.Rc<Mutex<T>>should be corrected toRc<RefCell<T>>