diff --git a/CHANGELOG.md b/CHANGELOG.md index 251f32d2e6e..2655d93599e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5181,6 +5181,7 @@ Released 2018-09-13 [`rc_mutex`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_mutex [`read_line_without_trim`]: https://rust-lang.github.io/rust-clippy/master/index.html#read_line_without_trim [`read_zero_byte_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#read_zero_byte_vec +[`readonly_write_lock`]: https://rust-lang.github.io/rust-clippy/master/index.html#readonly_write_lock [`recursive_format_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#recursive_format_impl [`redundant_allocation`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_allocation [`redundant_async_block`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_async_block diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index e084471433d..d4e0d286334 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -398,6 +398,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::methods::OR_THEN_UNWRAP_INFO, crate::methods::PATH_BUF_PUSH_OVERWRITE_INFO, crate::methods::RANGE_ZIP_WITH_LEN_INFO, + crate::methods::READONLY_WRITE_LOCK_INFO, crate::methods::READ_LINE_WITHOUT_TRIM_INFO, crate::methods::REPEAT_ONCE_INFO, crate::methods::RESULT_MAP_OR_INTO_OPTION_INFO, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index e774450018e..bca2039860d 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -76,6 +76,7 @@ mod or_then_unwrap; mod path_buf_push_overwrite; mod range_zip_with_len; mod read_line_without_trim; +mod readonly_write_lock; mod repeat_once; mod search_is_some; mod seek_from_current; @@ -3507,6 +3508,37 @@ declare_clippy_lint! { "checks for usage of `bool::then` in `Iterator::filter_map`" } +declare_clippy_lint! { + /// ### What it does + /// Looks for calls to `RwLock::write` where the lock is only used for reading. + /// + /// ### Why is this bad? + /// The write portion of `RwLock` is exclusive, meaning that no other thread + /// can access the lock while this writer is active. + /// + /// ### Example + /// ```rust + /// use std::sync::RwLock; + /// fn assert_is_zero(lock: &RwLock) { + /// let num = lock.write().unwrap(); + /// assert_eq!(*num, 0); + /// } + /// ``` + /// + /// Use instead: + /// ```rust + /// use std::sync::RwLock; + /// fn assert_is_zero(lock: &RwLock) { + /// let num = lock.read().unwrap(); + /// assert_eq!(*num, 0); + /// } + /// ``` + #[clippy::version = "1.73.0"] + pub READONLY_WRITE_LOCK, + nursery, + "acquiring a write lock when a read lock would work" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Msrv, @@ -3645,6 +3677,7 @@ impl_lint_pass!(Methods => [ STRING_LIT_CHARS_ANY, ITER_SKIP_ZERO, FILTER_MAP_BOOL_THEN, + READONLY_WRITE_LOCK ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -4188,6 +4221,9 @@ impl Methods { range_zip_with_len::check(cx, expr, iter_recv, arg); } }, + ("write", []) => { + readonly_write_lock::check(cx, expr, recv); + } _ => {}, } } diff --git a/clippy_lints/src/methods/readonly_write_lock.rs b/clippy_lints/src/methods/readonly_write_lock.rs new file mode 100644 index 00000000000..e3ec921da0c --- /dev/null +++ b/clippy_lints/src/methods/readonly_write_lock.rs @@ -0,0 +1,52 @@ +use super::READONLY_WRITE_LOCK; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::mir::{enclosing_mir, visit_local_usage}; +use clippy_utils::source::snippet; +use clippy_utils::ty::is_type_diagnostic_item; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind, Node}; +use rustc_lint::LateContext; +use rustc_middle::mir::{Location, START_BLOCK}; +use rustc_span::sym; + +fn is_unwrap_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + if let ExprKind::MethodCall(path, receiver, ..) = expr.kind + && path.ident.name == sym::unwrap + { + is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(receiver).peel_refs(), sym::Result) + } else { + false + } +} + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, receiver: &Expr<'_>) { + if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(receiver).peel_refs(), sym::RwLock) + && let Node::Expr(unwrap_call_expr) = cx.tcx.hir().get_parent(expr.hir_id) + && is_unwrap_call(cx, unwrap_call_expr) + && let parent = cx.tcx.hir().get_parent(unwrap_call_expr.hir_id) + && let Node::Local(local) = parent + && let Some(mir) = enclosing_mir(cx.tcx, expr.hir_id) + && let Some((local, _)) = mir.local_decls.iter_enumerated().find(|(_, decl)| { + local.span.contains(decl.source_info.span) + }) + && let Some(usages) = visit_local_usage(&[local], mir, Location { + block: START_BLOCK, + statement_index: 0, + }) + && let [usage] = usages.as_slice() + { + let writer_never_mutated = usage.local_consume_or_mutate_locs.is_empty(); + + if writer_never_mutated { + span_lint_and_sugg( + cx, + READONLY_WRITE_LOCK, + expr.span, + "this write lock is used only for reading", + "consider using a read lock instead", + format!("{}.read()", snippet(cx, receiver.span, "")), + Applicability::MaybeIncorrect // write lock might be intentional for enforcing exclusiveness + ); + } + } +} diff --git a/tests/ui/readonly_write_lock.rs b/tests/ui/readonly_write_lock.rs new file mode 100644 index 00000000000..656b45787e8 --- /dev/null +++ b/tests/ui/readonly_write_lock.rs @@ -0,0 +1,42 @@ +#![warn(clippy::readonly_write_lock)] + +use std::sync::RwLock; + +fn mutate_i32(x: &mut i32) { + *x += 1; +} + +fn accept_i32(_: i32) {} + +fn main() { + let lock = RwLock::new(42); + let lock2 = RwLock::new(1234); + + { + let writer = lock.write().unwrap(); + dbg!(&writer); + } + + { + let writer = lock.write().unwrap(); + accept_i32(*writer); + } + + { + let mut writer = lock.write().unwrap(); + mutate_i32(&mut writer); + dbg!(&writer); + } + + { + let mut writer = lock.write().unwrap(); + *writer += 1; + } + + { + let mut writer1 = lock.write().unwrap(); + let mut writer2 = lock2.write().unwrap(); + *writer2 += 1; + *writer1 = *writer2; + } +} diff --git a/tests/ui/readonly_write_lock.stderr b/tests/ui/readonly_write_lock.stderr new file mode 100644 index 00000000000..e3d8fce7b2c --- /dev/null +++ b/tests/ui/readonly_write_lock.stderr @@ -0,0 +1,16 @@ +error: this write lock is used only for reading + --> $DIR/readonly_write_lock.rs:16:22 + | +LL | let writer = lock.write().unwrap(); + | ^^^^^^^^^^^^ help: consider using a read lock instead: `lock.read()` + | + = note: `-D clippy::readonly-write-lock` implied by `-D warnings` + +error: this write lock is used only for reading + --> $DIR/readonly_write_lock.rs:21:22 + | +LL | let writer = lock.write().unwrap(); + | ^^^^^^^^^^^^ help: consider using a read lock instead: `lock.read()` + +error: aborting due to 2 previous errors +