Improve let_underscore_lock

- lint if the lock was in a nested pattern
- lint if the lock is inside a `Result<Lock, _>`

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
 

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,46 +105,65 @@ 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 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())),
  };
  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 },
  );
  }
- }
+ });
  }
 }

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)>,
 }
 
 impl AddToDiagnostic for NonBindingLetSub {
@@ -945,14 +944,18 @@ impl AddToDiagnostic for NonBindingLetSub {
  "_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,
- );
+ 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);
+ }
  }
 }
 

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.
 }

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
 }

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,68 @@ 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 immediately dropping the value using `drop(..)` after the `let` statement
+help: consider binding to an unused variable to avoid immediately dropping the value
+ |
+LL | (_unused , _) = (data.lock(), 1);
+ | ~~~~~~~
+
+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 immediately dropping the value using `drop(..)` after the `let` statement
+help: consider binding to an unused variable to avoid immediately dropping the value
+ |
+LL | (_b, Struct { a: _unused }) = (0, Struct { a: data.lock() });
+ | ~~~~~~~
+
+error: aborting due to 6 previous errors