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

Add new lint: rc_mutex #7316

Merged
merged 8 commits into from
Jul 3, 2021
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2611,6 +2611,7 @@ Released 2018-09-13
[`range_step_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_step_by_zero
[`range_zip_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_zip_with_len
[`rc_buffer`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_buffer
[`rc_mutex`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_mutex
[`redundant_allocation`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_allocation
[`redundant_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_clone
[`redundant_closure`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -931,6 +931,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
types::LINKEDLIST,
types::OPTION_OPTION,
types::RC_BUFFER,
types::RC_MUTEX,
types::REDUNDANT_ALLOCATION,
types::TYPE_COMPLEXITY,
types::VEC_BOX,
Expand Down Expand Up @@ -1027,6 +1028,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(strings::STRING_TO_STRING),
LintId::of(strings::STR_TO_STRING),
LintId::of(types::RC_BUFFER),
LintId::of(types::RC_MUTEX),
LintId::of(unnecessary_self_imports::UNNECESSARY_SELF_IMPORTS),
LintId::of(unwrap_in_result::UNWRAP_IN_RESULT),
LintId::of(verbose_file_reads::VERBOSE_FILE_READS),
Expand Down
31 changes: 30 additions & 1 deletion clippy_lints/src/types/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ mod box_vec;
mod linked_list;
mod option_option;
mod rc_buffer;
mod rc_mutex;
mod redundant_allocation;
mod type_complexity;
mod utils;
Expand Down Expand Up @@ -250,12 +251,39 @@ declare_clippy_lint! {
"usage of very complex types that might be better factored into `type` definitions"
}

declare_clippy_lint! {
/// **What it does:** Checks for `Rc<Mutex<T>>`.
///
/// **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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

///
/// **Example:**
/// ```rust,ignore
/// use std::rc::Rc;
/// use std::sync::Mutex;
/// fn foo(interned: Rc<Mutex<i32>>) { ... }
/// ```
///
/// Better:
///
/// ```rust,ignore
/// use std::rc::Rc;
/// use std::cell::RefCell
/// fn foo(interned: Rc<RefCell<i32>>) { ... }
/// ```
pub RC_MUTEX,
restriction,
"usage of `Rc<Mutex<T>>`"
}

pub struct Types {
vec_box_size_threshold: u64,
type_complexity_threshold: u64,
}

impl_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, REDUNDANT_ALLOCATION, RC_BUFFER, TYPE_COMPLEXITY]);
impl_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, REDUNDANT_ALLOCATION, RC_BUFFER, RC_MUTEX, TYPE_COMPLEXITY]);

impl<'tcx> LateLintPass<'tcx> for Types {
fn check_fn(&mut self, cx: &LateContext<'_>, _: FnKind<'_>, decl: &FnDecl<'_>, _: &Body<'_>, _: Span, id: HirId) {
Expand Down Expand Up @@ -375,6 +403,7 @@ impl Types {
triggered |= vec_box::check(cx, hir_ty, qpath, def_id, self.vec_box_size_threshold);
triggered |= option_option::check(cx, hir_ty, qpath, def_id);
triggered |= linked_list::check(cx, hir_ty, def_id);
triggered |= rc_mutex::check(cx, hir_ty, qpath, def_id);

if triggered {
return;
Expand Down
27 changes: 27 additions & 0 deletions clippy_lints/src/types/rc_mutex.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
use clippy_utils::diagnostics::span_lint;
use clippy_utils::is_ty_param_diagnostic_item;
use if_chain::if_chain;
use rustc_hir::{self as hir, def_id::DefId, QPath};
use rustc_lint::LateContext;
use rustc_span::symbol::sym;

use super::RC_MUTEX;

pub(super) fn check(cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>, qpath: &QPath<'_>, def_id: DefId) -> bool {
if_chain! {
if cx.tcx.is_diagnostic_item(sym::Rc, def_id) ;
if let Some(_) = is_ty_param_diagnostic_item(cx, qpath, sym!(mutex_type)) ;

then{
span_lint(
cx,
RC_MUTEX,
hir_ty.span,
"found `Rc<Mutex<_>>`. Consider using `Rc<RefCell<_>>` or `Arc<Mutex<_>>` instead",
);
return true;
}
}

false
}
34 changes: 34 additions & 0 deletions tests/ui/rc_mutex.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
#![warn(clippy::rc_mutex)]
#![allow(clippy::blacklisted_name)]

use std::rc::Rc;
use std::sync::Mutex;

pub struct MyStruct {
foo: Rc<Mutex<i32>>,
}

pub struct SubT<T> {
foo: T,
}

pub enum MyEnum {
One,
Two,
}

pub fn test1<T>(foo: Rc<Mutex<T>>) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see a test that actually creates an instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please check if I added correct test in my latest commit?
I'm not sure I did it right.


pub fn test2(foo: Rc<Mutex<MyEnum>>) {}

pub fn test3(foo: Rc<Mutex<SubT<usize>>>) {}

fn main() {
test1(Rc::new(Mutex::new(1)));
test2(Rc::new(Mutex::new(MyEnum::One)));
test3(Rc::new(Mutex::new(SubT { foo: 1 })));

let _my_struct = MyStruct {
foo: Rc::new(Mutex::new(1)),
};
}
28 changes: 28 additions & 0 deletions tests/ui/rc_mutex.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
error: found `Rc<Mutex<_>>`. Consider using `Rc<RefCell<_>>` or `Arc<Mutex<_>>` instead
--> $DIR/rc_mutex.rs:8:10
|
LL | foo: Rc<Mutex<i32>>,
| ^^^^^^^^^^^^^^
|
= note: `-D clippy::rc-mutex` implied by `-D warnings`

error: found `Rc<Mutex<_>>`. Consider using `Rc<RefCell<_>>` or `Arc<Mutex<_>>` instead
--> $DIR/rc_mutex.rs:20:22
|
LL | pub fn test1<T>(foo: Rc<Mutex<T>>) {}
| ^^^^^^^^^^^^

error: found `Rc<Mutex<_>>`. Consider using `Rc<RefCell<_>>` or `Arc<Mutex<_>>` instead
--> $DIR/rc_mutex.rs:22:19
|
LL | pub fn test2(foo: Rc<Mutex<MyEnum>>) {}
| ^^^^^^^^^^^^^^^^^

error: found `Rc<Mutex<_>>`. Consider using `Rc<RefCell<_>>` or `Arc<Mutex<_>>` instead
--> $DIR/rc_mutex.rs:24:19
|
LL | pub fn test3(foo: Rc<Mutex<SubT<usize>>>) {}
| ^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 4 previous errors