Rollup merge of #97739 - a2aaron:let_underscore, r=estebank

Uplift the `let_underscore` lints from clippy into rustc.

This PR resolves #97241.

This PR adds three lints from clippy--`let_underscore_drop`, `let_underscore_lock`, and `let_underscore_must_use`, which are meant to capture likely-incorrect uses of `let _ = ...` bindings (in particular, doing this on a type with a non-trivial `Drop` causes the `Drop` to occur immediately, instead of at the end of the scope. For a type like `MutexGuard`, this effectively releases the lock immediately, which is almost certainly the wrong behavior)

In porting the lints from clippy I had to copy over a bunch of utility functions from `clippy_util` that these lints also relied upon. Is that the right approach?

Note that I've set the `must_use` and `drop` lints to Allow by default and set `lock` to Deny by default (this matches the same settings that clippy has). In talking with `@estebank` he informed me to do a Crater run (I am not sure what type of Crater run to request here--I think it's just "check only"?)

On the linked issue, there's some discussion about using `must_use` and `Drop` together as a heuristic for when to warn--I did not implement this yet.

r? `@estebank`

compiler/rustc_lint/src/let_underscore.rs

@@ -0,0 +1,175 @@
+use crate::{LateContext, LateLintPass, LintContext};
+use rustc_errors::{Applicability, LintDiagnosticBuilder, MultiSpan};
+use rustc_hir as hir;
+use rustc_middle::ty;
+use rustc_span::Symbol;
+
+declare_lint! {
+    /// The `let_underscore_drop` lint checks for statements which don't bind
+    /// an expression which has a non-trivial Drop implementation to anything,
+    /// causing the expression to be dropped immediately instead of at end of
+    /// scope.
+    ///
+    /// ### Example
+    /// ```
+    /// struct SomeStruct;
+    /// impl Drop for SomeStruct {
+    ///     fn drop(&mut self) {
+    ///         println!("Dropping SomeStruct");
+    ///     }
+    /// }
+    ///
+    /// fn main() {
+    ///    #[warn(let_underscore_drop)]
+    ///     // SomeStuct is dropped immediately instead of at end of scope,
+    ///     // so "Dropping SomeStruct" is printed before "end of main".
+    ///     // The order of prints would be reversed if SomeStruct was bound to
+    ///     // a name (such as "_foo").
+    ///     let _ = SomeStruct;
+    ///     println!("end of main");
+    /// }
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// Statements which assign an expression to an underscore causes the
+    /// expression to immediately drop instead of extending the expression's
+    /// lifetime to the end of the scope. This is usually unintended,
+    /// especially for types like `MutexGuard`, which are typically used to
+    /// lock a mutex for the duration of an entire scope.
+    ///
+    /// If you want to extend the expression's lifetime to the end of the scope,
+    /// assign an underscore-prefixed name (such as `_foo`) to the expression.
+    /// If you do actually want to drop the expression immediately, then
+    /// calling `std::mem::drop` on the expression is clearer and helps convey
+    /// intent.
+    pub LET_UNDERSCORE_DROP,
+    Allow,
+    "non-binding let on a type that implements `Drop`"
+}
+
+declare_lint! {
+    /// The `let_underscore_lock` lint checks for statements which don't bind
+    /// a mutex to anything, causing the lock to be released immediately instead
+    /// of at end of scope, which is typically incorrect.
+    ///
+    /// ### Example
+    /// ```compile_fail
+    /// use std::sync::{Arc, Mutex};
+    /// use std::thread;
+    /// let data = Arc::new(Mutex::new(0));
+    ///
+    /// thread::spawn(move || {
+    ///     // The lock is immediately released instead of at the end of the
+    ///     // scope, which is probably not intended.
+    ///     let _ = data.lock().unwrap();
+    ///     println!("doing some work");
+    ///     let mut lock = data.lock().unwrap();
+    ///     *lock += 1;
+    /// });
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// Statements which assign an expression to an underscore causes the
+    /// expression to immediately drop instead of extending the expression's
+    /// lifetime to the end of the scope. This is usually unintended,
+    /// especially for types like `MutexGuard`, which are typically used to
+    /// lock a mutex for the duration of an entire scope.
+    ///
+    /// If you want to extend the expression's lifetime to the end of the scope,
+    /// assign an underscore-prefixed name (such as `_foo`) to the expression.
+    /// If you do actually want to drop the expression immediately, then
+    /// calling `std::mem::drop` on the expression is clearer and helps convey
+    /// intent.
+    pub LET_UNDERSCORE_LOCK,
+    Deny,
+    "non-binding let on a synchronization lock"
+}
+
+declare_lint_pass!(LetUnderscore => [LET_UNDERSCORE_DROP, LET_UNDERSCORE_LOCK]);
+
+const SYNC_GUARD_SYMBOLS: [Symbol; 3] = [
+    rustc_span::sym::MutexGuard,
+    rustc_span::sym::RwLockReadGuard,
+    rustc_span::sym::RwLockWriteGuard,
+];
+
+impl<'tcx> LateLintPass<'tcx> for LetUnderscore {
+    fn check_local(&mut self, cx: &LateContext<'_>, local: &hir::Local<'_>) {
+        if !matches!(local.pat.kind, hir::PatKind::Wild) {
+            return;
+        }
+        if let Some(init) = local.init {
+            let init_ty = cx.typeck_results().expr_ty(init);
+            // If the type has a trivial Drop implementation, then it doesn't
+            // matter that we drop the value immediately.
+            if !init_ty.needs_drop(cx.tcx, cx.param_env) {
+                return;
+            }
+            let is_sync_lock = match init_ty.kind() {
+                ty::Adt(adt, _) => SYNC_GUARD_SYMBOLS
+                    .iter()
+                    .any(|guard_symbol| cx.tcx.is_diagnostic_item(*guard_symbol, adt.did())),
+                _ => false,
+            };
+
+            if is_sync_lock {
+                let mut span = MultiSpan::from_spans(vec![local.pat.span, init.span]);
+                span.push_span_label(
+                    local.pat.span,
+                    "this lock is not assigned to a binding and is immediately dropped".to_string(),
+                );
+                span.push_span_label(
+                    init.span,
+                    "this binding will immediately drop the value assigned to it".to_string(),
+                );
+                cx.struct_span_lint(LET_UNDERSCORE_LOCK, span, |lint| {
+                    build_and_emit_lint(
+                        lint,
+                        local,
+                        init.span,
+                        "non-binding let on a synchronization lock",
+                    )
+                })
+            } else {
+                cx.struct_span_lint(LET_UNDERSCORE_DROP, local.span, |lint| {
+                    build_and_emit_lint(
+                        lint,
+                        local,
+                        init.span,
+                        "non-binding let on a type that implements `Drop`",
+                    );
+                })
+            }
+        }
+    }
+}
+
+fn build_and_emit_lint(
+    lint: LintDiagnosticBuilder<'_, ()>,
+    local: &hir::Local<'_>,
+    init_span: rustc_span::Span,
+    msg: &str,
+) {
+    lint.build(msg)
+        .span_suggestion_verbose(
+            local.pat.span,
+            "consider binding to an unused variable to avoid immediately dropping the value",
+            "_unused",
+            Applicability::MachineApplicable,
+        )
+        .multipart_suggestion(
+            "consider immediately dropping the value",
+            vec![
+                (local.span.until(init_span), "drop(".to_string()),
+                (init_span.shrink_to_hi(), ")".to_string()),
+            ],
+            Applicability::MachineApplicable,
+        )
+        .emit();
+}

compiler/rustc_lint/src/lib.rs

@@ -55,6 +55,7 @@ mod expect;
 pub mod hidden_unicode_codepoints;
 mod internal;
 mod late;
+mod let_underscore;
 mod levels;
 mod methods;
 mod non_ascii_idents;
@@ -86,6 +87,7 @@ use builtin::*;
 use enum_intrinsics_non_enums::EnumIntrinsicsNonEnums;
 use hidden_unicode_codepoints::*;
 use internal::*;
+use let_underscore::*;
 use methods::*;
 use non_ascii_idents::*;
 use non_fmt_panic::NonPanicFmt;
@@ -189,6 +191,7 @@ macro_rules! late_lint_mod_passes {
                 VariantSizeDifferences: VariantSizeDifferences,
                 BoxPointers: BoxPointers,
                 PathStatements: PathStatements,
+                LetUnderscore: LetUnderscore,
                 // Depends on referenced function signatures in expressions
                 UnusedResults: UnusedResults,
                 NonUpperCaseGlobals: NonUpperCaseGlobals,
@@ -315,6 +318,8 @@ fn register_builtins(store: &mut LintStore, no_interleave_lints: bool) {
         REDUNDANT_SEMICOLONS
     );
 
+    add_lint_group!("let_underscore", LET_UNDERSCORE_DROP, LET_UNDERSCORE_LOCK);
+
     add_lint_group!(
         "rust_2018_idioms",
         BARE_TRAIT_OBJECTS,

compiler/rustc_span/src/symbol.rs

@@ -223,6 +223,7 @@ symbols! {
         LinkedList,
         LintPass,
         Mutex,
+        MutexGuard,
         N,
         NonZeroI128,
         NonZeroI16,
@@ -271,6 +272,8 @@ symbols! {
         Rust,
         RustcDecodable,
         RustcEncodable,
+        RwLockReadGuard,
+        RwLockWriteGuard,
         Send,
         SeqCst,
         SessionDiagnostic,

library/std/src/sync/mutex.rs

@@ -192,6 +192,7 @@ unsafe impl<T: ?Sized + Send> Sync for Mutex<T> {}
                       and cause Futures to not implement `Send`"]
 #[stable(feature = "rust1", since = "1.0.0")]
 #[clippy::has_significant_drop]
+#[cfg_attr(not(test), rustc_diagnostic_item = "MutexGuard")]
 pub struct MutexGuard<'a, T: ?Sized + 'a> {
     lock: &'a Mutex<T>,
     poison: poison::Guard,

library/std/src/sync/rwlock.rs

@@ -101,6 +101,7 @@ unsafe impl<T: ?Sized + Send + Sync> Sync for RwLock<T> {}
                       and cause Futures to not implement `Send`"]
 #[stable(feature = "rust1", since = "1.0.0")]
 #[clippy::has_significant_drop]
+#[cfg_attr(not(test), rustc_diagnostic_item = "RwLockReadGuard")]
 pub struct RwLockReadGuard<'a, T: ?Sized + 'a> {
     // NB: we use a pointer instead of `&'a T` to avoid `noalias` violations, because a
     // `Ref` argument doesn't hold immutability for its whole scope, only until it drops.
@@ -130,6 +131,7 @@ unsafe impl<T: ?Sized + Sync> Sync for RwLockReadGuard<'_, T> {}
                       and cause Future's to not implement `Send`"]
 #[stable(feature = "rust1", since = "1.0.0")]
 #[clippy::has_significant_drop]
+#[cfg_attr(not(test), rustc_diagnostic_item = "RwLockWriteGuard")]
 pub struct RwLockWriteGuard<'a, T: ?Sized + 'a> {
     lock: &'a RwLock<T>,
     poison: poison::Guard,

src/test/ui/lint/let_underscore/let_underscore_drop.rs

@@ -0,0 +1,14 @@
+// check-pass
+#![warn(let_underscore_drop)]
+
+struct NontrivialDrop;
+
+impl Drop for NontrivialDrop {
+    fn drop(&mut self) {
+        println!("Dropping!");
+    }
+}
+
+fn main() {
+    let _ = NontrivialDrop; //~WARNING non-binding let on a type that implements `Drop`
+}

src/test/ui/lint/let_underscore/let_underscore_drop.stderr

@@ -0,0 +1,22 @@
+warning: non-binding let on a type that implements `Drop`
+  --> $DIR/let_underscore_drop.rs:13:5
+   |
+LL |     let _ = NontrivialDrop;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+note: the lint level is defined here
+  --> $DIR/let_underscore_drop.rs:2:9
+   |
+LL | #![warn(let_underscore_drop)]
+   |         ^^^^^^^^^^^^^^^^^^^
+help: consider binding to an unused variable to avoid immediately dropping the value
+   |
+LL |     let _unused = NontrivialDrop;
+   |         ~~~~~~~
+help: consider immediately dropping the value
+   |
+LL |     drop(NontrivialDrop);
+   |     ~~~~~              +
+
+warning: 1 warning emitted
+

src/test/ui/lint/let_underscore/let_underscore_lock.rs

@@ -0,0 +1,7 @@
+// check-fail
+use std::sync::{Arc, Mutex};
+
+fn main() {
+    let data = Arc::new(Mutex::new(0));
+    let _ = data.lock().unwrap(); //~ERROR non-binding let on a synchronization lock
+}

src/test/ui/lint/let_underscore/let_underscore_lock.stderr

@@ -0,0 +1,20 @@
+error: non-binding let on a synchronization lock
+  --> $DIR/let_underscore_lock.rs:6:9
+   |
+LL |     let _ = data.lock().unwrap();
+   |         ^   ^^^^^^^^^^^^^^^^^^^^ this binding will immediately drop the value assigned to it
+   |         |
+   |         this lock is not assigned to a binding and is immediately dropped
+   |
+   = note: `#[deny(let_underscore_lock)]` on by default
+help: consider binding to an unused variable to avoid immediately dropping the value
+   |
+LL |     let _unused = data.lock().unwrap();
+   |         ~~~~~~~
+help: consider immediately dropping the value
+   |
+LL |     drop(data.lock().unwrap());
+   |     ~~~~~                    +
+
+error: aborting due to previous error
+

src/tools/lint-docs/src/groups.rs

@@ -8,6 +8,7 @@ use std::process::Command;
 /// Descriptions of rustc lint groups.
 static GROUP_DESCRIPTIONS: &[(&str, &str)] = &[
     ("unused", "Lints that detect things being declared but not used, or excess syntax"),
+    ("let-underscore", "Lints that detect wildcard let bindings that are likely to be invalid"),
     ("rustdoc", "Rustdoc-specific lints"),
     ("rust-2018-idioms", "Lints to nudge you toward idiomatic features of Rust 2018"),
     ("nonstandard-style", "Violation of standard naming conventions"),