Auto merge of rust-lang#11210 - y21:readonly_write_lock, r=giraffate

new lint: [`readonly_write_lock`]

Closes rust-lang#8555

A new lint that catches `RwLock::write` calls to acquire a write lock only to read from it and not actually do any writes (mutations).

changelog: new lint: [`readonly_write_lock`]

Author: bors

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

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,

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<i32>) {
+    ///     let num = lock.write().unwrap();
+    ///     assert_eq!(*num, 0);
+    /// }
+    /// ```
+    ///
+    /// Use instead:
+    /// ```rust
+    /// use std::sync::RwLock;
+    /// fn assert_is_zero(lock: &RwLock<i32>) {
+    ///     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);
+                }
                 _ => {},
             }
         }

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, "<receiver>")),
+                Applicability::MaybeIncorrect // write lock might be intentional for enforcing exclusiveness
+            );
+        }
+    }
+}

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;
+    }
+}

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
+