Auto merge of #5101 - Areredify:let_underscore_lock, r=flip1995

add `let_underscore_lock` lint closes #1574 changelog: add `let_underscore_lock` lint I am not entirely sure about my docs/messages wording here, improvements are welcome
rust-lang · Jan 30, 2020 · c13268a · c13268a
2 parents f69835b + 63ab7a5
commit c13268a
Show file tree

Hide file tree

Showing 10 changed files with 139 additions and 18 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1153,6 +1153,7 @@ Released 2018-09-13
 [`len_without_is_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_without_is_empty
 [`len_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_zero
 [`let_and_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return
+[`let_underscore_lock`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_lock
 [`let_underscore_must_use`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_must_use
 [`let_unit_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_unit_value
 [`linkedlist`]: https://rust-lang.github.io/rust-clippy/master/index.html#linkedlist

diff --git a/README.md b/README.md
@@ -6,7 +6,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 350 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 351 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 

diff --git a/clippy_lints/src/let_underscore.rs b/clippy_lints/src/let_underscore.rs
@@ -4,7 +4,7 @@ use rustc_hir::*;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 
-use crate::utils::{is_must_use_func_call, is_must_use_ty, span_lint_and_help};
+use crate::utils::{is_must_use_func_call, is_must_use_ty, match_type, paths, span_lint_and_help};
 
 declare_clippy_lint! {
     /// **What it does:** Checks for `let _ = <expr>`
@@ -30,7 +30,40 @@ declare_clippy_lint! {
     "non-binding let on a `#[must_use]` expression"
 }
 
-declare_lint_pass!(LetUnderscore => [LET_UNDERSCORE_MUST_USE]);
+declare_clippy_lint! {
+    /// **What it does:** Checks for `let _ = sync_lock`
+    ///
+    /// **Why is this bad?** This statement immediately drops the lock instead of
+    /// extending it's lifetime to the end of the scope, which is often not intended.
+    /// To extend lock lifetime to the end of the scope, use an underscore-prefixed
+    /// name instead (i.e. _lock). If you want to explicitly drop the lock,
+    /// `std::mem::drop` conveys your intention better and is less error-prone.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// Bad:
+    /// ```rust,ignore
+    /// let _ = mutex.lock();
+    /// ```
+    ///
+    /// Good:
+    /// ```rust,ignore
+    /// let _lock = mutex.lock();
+    /// ```
+    pub LET_UNDERSCORE_LOCK,
+    correctness,
+    "non-binding let on a synchronization lock"
+}
+
+declare_lint_pass!(LetUnderscore => [LET_UNDERSCORE_MUST_USE, LET_UNDERSCORE_LOCK]);
+
+const SYNC_GUARD_PATHS: [&[&str]; 3] = [
+    &paths::MUTEX_GUARD,
+    &paths::RWLOCK_READ_GUARD,
+    &paths::RWLOCK_WRITE_GUARD,
+];
 
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LetUnderscore {
     fn check_stmt(&mut self, cx: &LateContext<'_, '_>, stmt: &Stmt<'_>) {
@@ -43,8 +76,18 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LetUnderscore {
             if let PatKind::Wild = local.pat.kind;
             if let Some(ref init) = local.init;
             then {
-                if is_must_use_ty(cx, cx.tables.expr_ty(init)) {
-                   span_lint_and_help(
+                let check_ty = |ty| SYNC_GUARD_PATHS.iter().any(|path| match_type(cx, ty, path));
+                if cx.tables.expr_ty(init).walk().any(check_ty) {
+                    span_lint_and_help(
+                        cx,
+                        LET_UNDERSCORE_LOCK,
+                        stmt.span,
+                        "non-binding let on a synchronization lock",
+                        "consider using an underscore-prefixed named \
+                            binding or dropping explicitly with `std::mem::drop`"
+                    )
+                } else if is_must_use_ty(cx, cx.tables.expr_ty(init)) {
+                    span_lint_and_help(
                         cx,
                         LET_UNDERSCORE_MUST_USE,
                         stmt.span,

diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -566,6 +566,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &len_zero::LEN_WITHOUT_IS_EMPTY,
         &len_zero::LEN_ZERO,
         &let_if_seq::USELESS_LET_IF_SEQ,
+        &let_underscore::LET_UNDERSCORE_LOCK,
         &let_underscore::LET_UNDERSCORE_MUST_USE,
         &lifetimes::EXTRA_UNUSED_LIFETIMES,
         &lifetimes::NEEDLESS_LIFETIMES,
@@ -1171,6 +1172,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&len_zero::LEN_WITHOUT_IS_EMPTY),
         LintId::of(&len_zero::LEN_ZERO),
         LintId::of(&let_if_seq::USELESS_LET_IF_SEQ),
+        LintId::of(&let_underscore::LET_UNDERSCORE_LOCK),
         LintId::of(&lifetimes::EXTRA_UNUSED_LIFETIMES),
         LintId::of(&lifetimes::NEEDLESS_LIFETIMES),
         LintId::of(&literal_representation::INCONSISTENT_DIGIT_GROUPING),
@@ -1556,6 +1558,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&infinite_iter::INFINITE_ITER),
         LintId::of(&inherent_to_string::INHERENT_TO_STRING_SHADOW_DISPLAY),
         LintId::of(&inline_fn_without_body::INLINE_FN_WITHOUT_BODY),
+        LintId::of(&let_underscore::LET_UNDERSCORE_LOCK),
         LintId::of(&literal_representation::MISTYPED_LITERAL_SUFFIXES),
         LintId::of(&loops::FOR_LOOP_OVER_OPTION),
         LintId::of(&loops::FOR_LOOP_OVER_RESULT),

diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs
@@ -58,6 +58,7 @@ pub const MEM_REPLACE: [&str; 3] = ["core", "mem", "replace"];
 pub const MEM_UNINITIALIZED: [&str; 3] = ["core", "mem", "uninitialized"];
 pub const MEM_ZEROED: [&str; 3] = ["core", "mem", "zeroed"];
 pub const MUTEX: [&str; 4] = ["std", "sync", "mutex", "Mutex"];
+pub const MUTEX_GUARD: [&str; 4] = ["std", "sync", "mutex", "MutexGuard"];
 pub const OPEN_OPTIONS: [&str; 3] = ["std", "fs", "OpenOptions"];
 pub const OPS_MODULE: [&str; 2] = ["core", "ops"];
 pub const OPTION: [&str; 3] = ["core", "option", "Option"];
@@ -100,6 +101,8 @@ pub const REPEAT: [&str; 3] = ["core", "iter", "repeat"];
 pub const RESULT: [&str; 3] = ["core", "result", "Result"];
 pub const RESULT_ERR: [&str; 4] = ["core", "result", "Result", "Err"];
 pub const RESULT_OK: [&str; 4] = ["core", "result", "Result", "Ok"];
+pub const RWLOCK_READ_GUARD: [&str; 4] = ["std", "sync", "rwlock", "RwLockReadGuard"];
+pub const RWLOCK_WRITE_GUARD: [&str; 4] = ["std", "sync", "rwlock", "RwLockWriteGuard"];
 pub const SERDE_DE_VISITOR: [&str; 3] = ["serde", "de", "Visitor"];
 pub const SLICE_INTO_VEC: [&str; 4] = ["alloc", "slice", "<impl [T]>", "into_vec"];
 pub const SLICE_ITER: [&str; 3] = ["core", "slice", "Iter"];

diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs
@@ -6,7 +6,7 @@ pub use lint::Lint;
 pub use lint::LINT_LEVELS;
 
 // begin lint list, do not remove this comment, it’s used in `update_lints`
-pub const ALL_LINTS: [Lint; 350] = [
+pub const ALL_LINTS: [Lint; 351] = [
     Lint {
         name: "absurd_extreme_comparisons",
         group: "correctness",
@@ -959,6 +959,13 @@ pub const ALL_LINTS: [Lint; 350] = [
         deprecation: None,
         module: "returns",
     },
+    Lint {
+        name: "let_underscore_lock",
+        group: "correctness",
+        desc: "non-binding let on a synchronization lock",
+        deprecation: None,
+        module: "let_underscore",
+    },
     Lint {
         name: "let_underscore_must_use",
         group: "restriction",

diff --git a/tests/ui/let_underscore_lock.rs b/tests/ui/let_underscore_lock.rs
@@ -0,0 +1,13 @@
+#![warn(clippy::let_underscore_lock)]
+
+fn main() {
+    let m = std::sync::Mutex::new(());
+    let rw = std::sync::RwLock::new(());
+
+    let _ = m.lock();
+    let _ = rw.read();
+    let _ = rw.write();
+    let _ = m.try_lock();
+    let _ = rw.try_read();
+    let _ = rw.try_write();
+}
diff --git a/tests/ui/let_underscore_lock.stderr b/tests/ui/let_underscore_lock.stderr
@@ -0,0 +1,51 @@
+error: non-binding let on a synchronization lock
+  --> $DIR/let_underscore_lock.rs:7:5
+   |
+LL |     let _ = m.lock();
+   |     ^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::let-underscore-lock` implied by `-D warnings`
+   = help: consider using an underscore-prefixed named binding or dropping explicitly with `std::mem::drop`
+
+error: non-binding let on a synchronization lock
+  --> $DIR/let_underscore_lock.rs:8:5
+   |
+LL |     let _ = rw.read();
+   |     ^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using an underscore-prefixed named binding or dropping explicitly with `std::mem::drop`
+
+error: non-binding let on a synchronization lock
+  --> $DIR/let_underscore_lock.rs:9:5
+   |
+LL |     let _ = rw.write();
+   |     ^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using an underscore-prefixed named binding or dropping explicitly with `std::mem::drop`
+
+error: non-binding let on a synchronization lock
+  --> $DIR/let_underscore_lock.rs:10:5
+   |
+LL |     let _ = m.try_lock();
+   |     ^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using an underscore-prefixed named binding or dropping explicitly with `std::mem::drop`
+
+error: non-binding let on a synchronization lock
+  --> $DIR/let_underscore_lock.rs:11:5
+   |
+LL |     let _ = rw.try_read();
+   |     ^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using an underscore-prefixed named binding or dropping explicitly with `std::mem::drop`
+
+error: non-binding let on a synchronization lock
+  --> $DIR/let_underscore_lock.rs:12:5
+   |
+LL |     let _ = rw.try_write();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using an underscore-prefixed named binding or dropping explicitly with `std::mem::drop`
+
+error: aborting due to 6 previous errors
+
diff --git a/tests/ui/let_underscore.rs → tests/ui/let_underscore_must_use.rs b/tests/ui/let_underscore.rs → tests/ui/let_underscore_must_use.rs
diff --git a/tests/ui/let_underscore.stderr → tests/ui/let_underscore_must_use.stderr b/tests/ui/let_underscore.stderr → tests/ui/let_underscore_must_use.stderr
@@ -1,5 +1,5 @@
 error: non-binding let on a result of a `#[must_use]` function
-  --> $DIR/let_underscore.rs:66:5
+  --> $DIR/let_underscore_must_use.rs:66:5
    |
 LL |     let _ = f();
    |     ^^^^^^^^^^^^
@@ -8,87 +8,87 @@ LL |     let _ = f();
    = help: consider explicitly using function result
 
 error: non-binding let on an expression with `#[must_use]` type
-  --> $DIR/let_underscore.rs:67:5
+  --> $DIR/let_underscore_must_use.rs:67:5
    |
 LL |     let _ = g();
    |     ^^^^^^^^^^^^
    |
    = help: consider explicitly using expression value
 
 error: non-binding let on a result of a `#[must_use]` function
-  --> $DIR/let_underscore.rs:69:5
+  --> $DIR/let_underscore_must_use.rs:69:5
    |
 LL |     let _ = l(0_u32);
    |     ^^^^^^^^^^^^^^^^^
    |
    = help: consider explicitly using function result
 
 error: non-binding let on a result of a `#[must_use]` function
-  --> $DIR/let_underscore.rs:73:5
+  --> $DIR/let_underscore_must_use.rs:73:5
    |
 LL |     let _ = s.f();
    |     ^^^^^^^^^^^^^^
    |
    = help: consider explicitly using function result
 
 error: non-binding let on an expression with `#[must_use]` type
-  --> $DIR/let_underscore.rs:74:5
+  --> $DIR/let_underscore_must_use.rs:74:5
    |
 LL |     let _ = s.g();
    |     ^^^^^^^^^^^^^^
    |
    = help: consider explicitly using expression value
 
 error: non-binding let on a result of a `#[must_use]` function
-  --> $DIR/let_underscore.rs:77:5
+  --> $DIR/let_underscore_must_use.rs:77:5
    |
 LL |     let _ = S::h();
    |     ^^^^^^^^^^^^^^^
    |
    = help: consider explicitly using function result
 
 error: non-binding let on an expression with `#[must_use]` type
-  --> $DIR/let_underscore.rs:78:5
+  --> $DIR/let_underscore_must_use.rs:78:5
    |
 LL |     let _ = S::p();
    |     ^^^^^^^^^^^^^^^
    |
    = help: consider explicitly using expression value
 
 error: non-binding let on a result of a `#[must_use]` function
-  --> $DIR/let_underscore.rs:80:5
+  --> $DIR/let_underscore_must_use.rs:80:5
    |
 LL |     let _ = S::a();
    |     ^^^^^^^^^^^^^^^
    |
    = help: consider explicitly using function result
 
 error: non-binding let on an expression with `#[must_use]` type
-  --> $DIR/let_underscore.rs:82:5
+  --> $DIR/let_underscore_must_use.rs:82:5
    |
 LL |     let _ = if true { Ok(()) } else { Err(()) };
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: consider explicitly using expression value
 
 error: non-binding let on a result of a `#[must_use]` function
-  --> $DIR/let_underscore.rs:86:5
+  --> $DIR/let_underscore_must_use.rs:86:5
    |
 LL |     let _ = a.is_ok();
    |     ^^^^^^^^^^^^^^^^^^
    |
    = help: consider explicitly using function result
 
 error: non-binding let on an expression with `#[must_use]` type
-  --> $DIR/let_underscore.rs:88:5
+  --> $DIR/let_underscore_must_use.rs:88:5
    |
 LL |     let _ = a.map(|_| ());
    |     ^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: consider explicitly using expression value
 
 error: non-binding let on an expression with `#[must_use]` type
-  --> $DIR/let_underscore.rs:90:5
+  --> $DIR/let_underscore_must_use.rs:90:5
    |
 LL |     let _ = a;
    |     ^^^^^^^^^^