Rollup merge of #119710 - Nilstrieb:let-_-=-oops, r=TaKO8Ki

matthiaskrgr · web-flow · commit 72dddeaeb78f · 2024-01-22T07:56:41.000+01:00
Improve `let_underscore_lock` - lint if the lock was in a nested pattern - lint if the lock is inside a `Result<Lock, _>` addresses #119704 (comment)
diff --git a/compiler/rustc_lint/messages.ftl b/compiler/rustc_lint/messages.ftl
@@ -345,6 +345,9 @@ lint_multiple_supertrait_upcastable = `{$ident}` is object-safe and has multiple
 lint_node_source = `forbid` level set here
     .note = {$reason}
 
+lint_non_binding_let_multi_drop_fn =
+    consider immediately dropping the value using `drop(..)` after the `let` statement
+
 lint_non_binding_let_multi_suggestion =
     consider immediately dropping the value
 
diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs
@@ -5,7 +5,7 @@ use crate::{
 use rustc_errors::MultiSpan;
 use rustc_hir as hir;
 use rustc_middle::ty;
-use rustc_span::Symbol;
+use rustc_span::{sym, Symbol};
 
 declare_lint! {
     /// The `let_underscore_drop` lint checks for statements which don't bind
@@ -105,51 +105,70 @@ const SYNC_GUARD_SYMBOLS: [Symbol; 3] = [
 
 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 matches!(local.source, rustc_hir::LocalSource::AsyncFn) {
             return;
         }
-        if let Some(init) = local.init {
-            let init_ty = cx.typeck_results().expr_ty(init);
+
+        let mut top_level = true;
+
+        // We recursively walk through all patterns, so that we can catch cases where the lock is nested in a pattern.
+        // For the basic `let_underscore_drop` lint, we only look at the top level, since there are many legitimate reasons
+        // to bind a sub-pattern to an `_`, if we're only interested in the rest.
+        // But with locks, we prefer having the chance of "false positives" over missing cases, since the effects can be
+        // quite catastrophic.
+        local.pat.walk_always(|pat| {
+            let is_top_level = top_level;
+            top_level = false;
+
+            if !matches!(pat.kind, hir::PatKind::Wild) {
+                return;
+            }
+
+            let ty = cx.typeck_results().pat_ty(pat);
+
             // 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) {
+            if !ty.needs_drop(cx.tcx, cx.param_env) {
                 return;
             }
-            let is_sync_lock = match init_ty.kind() {
+            // Lint for patterns like `mutex.lock()`, which returns `Result<MutexGuard, _>` as well.
+            let potential_lock_type = match ty.kind() {
+                ty::Adt(adt, args) if cx.tcx.is_diagnostic_item(sym::Result, adt.did()) => {
+                    args.type_at(0)
+                }
+                _ => ty,
+            };
+            let is_sync_lock = match potential_lock_type.kind() {
                 ty::Adt(adt, _) => SYNC_GUARD_SYMBOLS
                     .iter()
                     .any(|guard_symbol| cx.tcx.is_diagnostic_item(*guard_symbol, adt.did())),
                 _ => false,
             };
 
+            let can_use_init = is_top_level.then_some(local.init).flatten();
+
             let sub = NonBindingLetSub {
-                suggestion: local.pat.span,
-                multi_suggestion_start: local.span.until(init.span),
-                multi_suggestion_end: init.span.shrink_to_hi(),
+                suggestion: pat.span,
+                // We can't suggest `drop()` when we're on the top level.
+                drop_fn_start_end: can_use_init
+                    .map(|init| (local.span.until(init.span), init.span.shrink_to_hi())),
                 is_assign_desugar: matches!(local.source, rustc_hir::LocalSource::AssignDesugar(_)),
             };
             if is_sync_lock {
-                let mut span = MultiSpan::from_spans(vec![local.pat.span, init.span]);
+                let mut span = MultiSpan::from_span(pat.span);
                 span.push_span_label(
-                    local.pat.span,
+                    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.emit_spanned_lint(LET_UNDERSCORE_LOCK, span, NonBindingLet::SyncLock { sub });
-            } else {
+            // Only emit let_underscore_drop for top-level `_` patterns.
+            } else if can_use_init.is_some() {
                 cx.emit_spanned_lint(
                     LET_UNDERSCORE_DROP,
                     local.span,
                     NonBindingLet::DropType { sub },
                 );
             }
-        }
+        });
     }
 }
diff --git a/compiler/rustc_lint/src/lints.rs b/compiler/rustc_lint/src/lints.rs
@@ -930,8 +930,7 @@ pub enum NonBindingLet {
 
 pub struct NonBindingLetSub {
     pub suggestion: Span,
-    pub multi_suggestion_start: Span,
-    pub multi_suggestion_end: Span,
+    pub drop_fn_start_end: Option<(Span, Span)>,
     pub is_assign_desugar: bool,
 }
 
@@ -940,21 +939,31 @@ impl AddToDiagnostic for NonBindingLetSub {
     where
         F: Fn(&mut Diagnostic, SubdiagnosticMessage) -> SubdiagnosticMessage,
     {
-        let prefix = if self.is_assign_desugar { "let " } else { "" };
-        diag.span_suggestion_verbose(
-            self.suggestion,
-            fluent::lint_non_binding_let_suggestion,
-            format!("{prefix}_unused"),
-            Applicability::MachineApplicable,
-        );
-        diag.multipart_suggestion(
-            fluent::lint_non_binding_let_multi_suggestion,
-            vec![
-                (self.multi_suggestion_start, "drop(".to_string()),
-                (self.multi_suggestion_end, ")".to_string()),
-            ],
-            Applicability::MachineApplicable,
-        );
+        let can_suggest_binding = self.drop_fn_start_end.is_some() || !self.is_assign_desugar;
+
+        if can_suggest_binding {
+            let prefix = if self.is_assign_desugar { "let " } else { "" };
+            diag.span_suggestion_verbose(
+                self.suggestion,
+                fluent::lint_non_binding_let_suggestion,
+                format!("{prefix}_unused"),
+                Applicability::MachineApplicable,
+            );
+        } else {
+            diag.span_help(self.suggestion, fluent::lint_non_binding_let_suggestion);
+        }
+        if let Some(drop_fn_start_end) = self.drop_fn_start_end {
+            diag.multipart_suggestion(
+                fluent::lint_non_binding_let_multi_suggestion,
+                vec![
+                    (drop_fn_start_end.0, "drop(".to_string()),
+                    (drop_fn_start_end.1, ")".to_string()),
+                ],
+                Applicability::MachineApplicable,
+            );
+        } else {
+            diag.help(fluent::lint_non_binding_let_multi_drop_fn);
+        }
     }
 }
 
diff --git a/src/tools/clippy/tests/ui/let_underscore_lock.rs b/src/tools/clippy/tests/ui/let_underscore_lock.rs
@@ -26,6 +26,7 @@ fn main() {
     let _ = p_rw;
 }
 
+#[allow(let_underscore_lock)]
 fn uplifted() {
     // shouldn't lint std locks as they were uplifted as rustc's `let_underscore_lock`
 
diff --git a/tests/ui/lint/let_underscore/let_underscore_drop.rs b/tests/ui/lint/let_underscore/let_underscore_drop.rs
@@ -11,4 +11,6 @@ impl Drop for NontrivialDrop {
 
 fn main() {
     let _ = NontrivialDrop; //~WARNING non-binding let on a type that implements `Drop`
+
+    let (_, _) = (NontrivialDrop, NontrivialDrop); // This should be ignored.
 }
diff --git a/tests/ui/lint/let_underscore/let_underscore_lock.rs b/tests/ui/lint/let_underscore/let_underscore_lock.rs
@@ -1,7 +1,22 @@
 // check-fail
 use std::sync::{Arc, Mutex};
 
+struct Struct<T> {
+    a: T,
+}
+
 fn main() {
     let data = Arc::new(Mutex::new(0));
     let _ = data.lock().unwrap(); //~ERROR non-binding let on a synchronization lock
+
+    let _ = data.lock(); //~ERROR non-binding let on a synchronization lock
+
+    let (_, _) = (data.lock(), 1); //~ERROR non-binding let on a synchronization lock
+
+    let (_a, Struct { a: _ }) = (0, Struct { a: data.lock() }); //~ERROR non-binding let on a synchronization lock
+
+    (_ , _) = (data.lock(), 1); //~ERROR non-binding let on a synchronization lock
+
+    let _b;
+    (_b, Struct { a: _ }) = (0, Struct { a: data.lock() }); //~ERROR non-binding let on a synchronization lock
 }
diff --git a/tests/ui/lint/let_underscore/let_underscore_lock.stderr b/tests/ui/lint/let_underscore/let_underscore_lock.stderr
@@ -1,10 +1,8 @@
 error: non-binding let on a synchronization lock
-  --> $DIR/let_underscore_lock.rs:6:9
+  --> $DIR/let_underscore_lock.rs:10: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
+   |         ^ 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
@@ -16,5 +14,70 @@ help: consider immediately dropping the value
 LL |     drop(data.lock().unwrap());
    |     ~~~~~                    +
 
-error: aborting due to 1 previous error
+error: non-binding let on a synchronization lock
+  --> $DIR/let_underscore_lock.rs:12:9
+   |
+LL |     let _ = data.lock();
+   |         ^ this lock is not assigned to a binding and is immediately dropped
+   |
+help: consider binding to an unused variable to avoid immediately dropping the value
+   |
+LL |     let _unused = data.lock();
+   |         ~~~~~~~
+help: consider immediately dropping the value
+   |
+LL |     drop(data.lock());
+   |     ~~~~~           +
+
+error: non-binding let on a synchronization lock
+  --> $DIR/let_underscore_lock.rs:14:10
+   |
+LL |     let (_, _) = (data.lock(), 1);
+   |          ^ this lock is not assigned to a binding and is immediately dropped
+   |
+   = help: consider immediately dropping the value using `drop(..)` after the `let` statement
+help: consider binding to an unused variable to avoid immediately dropping the value
+   |
+LL |     let (_unused, _) = (data.lock(), 1);
+   |          ~~~~~~~
+
+error: non-binding let on a synchronization lock
+  --> $DIR/let_underscore_lock.rs:16:26
+   |
+LL |     let (_a, Struct { a: _ }) = (0, Struct { a: data.lock() });
+   |                          ^ this lock is not assigned to a binding and is immediately dropped
+   |
+   = help: consider immediately dropping the value using `drop(..)` after the `let` statement
+help: consider binding to an unused variable to avoid immediately dropping the value
+   |
+LL |     let (_a, Struct { a: _unused }) = (0, Struct { a: data.lock() });
+   |                          ~~~~~~~
+
+error: non-binding let on a synchronization lock
+  --> $DIR/let_underscore_lock.rs:18:6
+   |
+LL |     (_ , _) = (data.lock(), 1);
+   |      ^ this lock is not assigned to a binding and is immediately dropped
+   |
+help: consider binding to an unused variable to avoid immediately dropping the value
+  --> $DIR/let_underscore_lock.rs:18:6
+   |
+LL |     (_ , _) = (data.lock(), 1);
+   |      ^
+   = help: consider immediately dropping the value using `drop(..)` after the `let` statement
+
+error: non-binding let on a synchronization lock
+  --> $DIR/let_underscore_lock.rs:21:22
+   |
+LL |     (_b, Struct { a: _ }) = (0, Struct { a: data.lock() });
+   |                      ^ this lock is not assigned to a binding and is immediately dropped
+   |
+help: consider binding to an unused variable to avoid immediately dropping the value
+  --> $DIR/let_underscore_lock.rs:21:22
+   |
+LL |     (_b, Struct { a: _ }) = (0, Struct { a: data.lock() });
+   |                      ^
+   = help: consider immediately dropping the value using `drop(..)` after the `let` statement
+
+error: aborting due to 6 previous errors
 

Original file line number	Diff line number	Diff line change
`@@ -26,6 +26,7 @@ fn main() {`
`26`	`26`	`let _ = p_rw;`
`27`	`27`	`}`
`28`	`28`
	`29`	`+#[allow(let_underscore_lock)]`
`29`	`30`	`fn uplifted() {`
`30`	`31`	// shouldn't lint std locks as they were uplifted as rustc's `let_underscore_lock`
`31`	`32`
Original file line number	Diff line number	Diff line change
`@@ -11,4 +11,6 @@ impl Drop for NontrivialDrop {`
`11`	`11`
`12`	`12`	`fn main() {`
`13`	`13`	let _ = NontrivialDrop; //~WARNING non-binding let on a type that implements `Drop`
	`14`	`+`
	`15`	`+ let (_, _) = (NontrivialDrop, NontrivialDrop); // This should be ignored.`
`14`	`16`	`}`