From a04ac4952ce0fd9ac7a61da2eef1e2d8e4f9098d Mon Sep 17 00:00:00 2001 From: Nilstrieb <48135649+Nilstrieb@users.noreply.github.com> Date: Sun, 7 Jan 2024 20:37:51 +0100 Subject: [PATCH] Improve `let_underscore_lock` - lint if the lock was in a nested pattern - lint if the lock is inside a `Result` --- compiler/rustc_lint/messages.ftl | 3 + compiler/rustc_lint/src/let_underscore.rs | 63 ++++++++++------ compiler/rustc_lint/src/lints.rs | 43 ++++++----- .../clippy/tests/ui/let_underscore_lock.rs | 1 + .../let_underscore/let_underscore_drop.rs | 2 + .../let_underscore/let_underscore_lock.rs | 15 ++++ .../let_underscore/let_underscore_lock.stderr | 73 +++++++++++++++++-- 7 files changed, 156 insertions(+), 44 deletions(-) diff --git a/compiler/rustc_lint/messages.ftl b/compiler/rustc_lint/messages.ftl index b6fa2f1f221..7d4afc5bad7 100644 --- 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 index bdace8e01f6..0c9dff2b5d9 100644 --- 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); - // 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) { + + 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 is_sync_lock = match init_ty.kind() { + + 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 !ty.needs_drop(cx.tcx, cx.param_env) { + return; + } + // Lint for patterns like `mutex.lock()`, which returns `Result` 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 index f370c4392b3..46e75ef975e 100644 --- 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 index ccac73be79e..28d8dd49831 100644 --- 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 index f298871f122..a31b18ed594 100644 --- 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 index 7423862cdf0..df7e60e3940 100644 --- 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 { + 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 index f88a1df55e0..fb8b9ec2203 100644 --- 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