Skip to content

Commit

Permalink
Improve let_underscore_lock
Browse files Browse the repository at this point in the history
- lint if the lock was in a nested pattern
- lint if the lock is inside a `Result<Lock, _>`
  • Loading branch information
Noratrieb committed Jan 7, 2024
1 parent 5cb2e7d commit 478b5b4
Show file tree
Hide file tree
Showing 6 changed files with 137 additions and 35 deletions.
3 changes: 3 additions & 0 deletions compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
57 changes: 38 additions & 19 deletions compiler/rustc_lint/src/let_underscore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 },
);
}
}
});
}
}
23 changes: 13 additions & 10 deletions compiler/rustc_lint/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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);
}
}
}

Expand Down
2 changes: 2 additions & 0 deletions tests/ui/lint/let_underscore/let_underscore_drop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
}
16 changes: 15 additions & 1 deletion tests/ui/lint/let_underscore/let_underscore_lock.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,21 @@
// 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
}
71 changes: 66 additions & 5 deletions tests/ui/lint/let_underscore/let_underscore_lock.stderr
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
error: non-binding let on a synchronization lock
--> $DIR/let_underscore_lock.rs:6:9
--> $DIR/let_underscore_lock.rs:9: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
Expand All @@ -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:11: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:13: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:15: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:17: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:20: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

0 comments on commit 478b5b4

Please sign in to comment.