Skip to content

Commit b983667

Browse files
rust-timerrust-timer
authored
Unrolled build for rust-lang#119710
Rollup merge of rust-lang#119710 - Nilstrieb:let-_-=-oops, r=TaKO8Ki Improve `let_underscore_lock` - lint if the lock was in a nested pattern - lint if the lock is inside a `Result<Lock, _>` addresses rust-lang#119704 (comment)
2 parents 6fff796 + a04ac49 commit b983667

File tree

7 files changed

+154
-42
lines changed

7 files changed

+154
-42
lines changed

‎compiler/rustc_lint/messages.ftl

+3
Original file line numberDiff line numberDiff line change
@@ -345,6 +345,9 @@ lint_multiple_supertrait_upcastable = `{$ident}` is object-safe and has multiple
345345
lint_node_source = `forbid` level set here
346346
.note = {$reason}
347347
348+
lint_non_binding_let_multi_drop_fn =
349+
consider immediately dropping the value using `drop(..)` after the `let` statement
350+
348351
lint_non_binding_let_multi_suggestion =
349352
consider immediately dropping the value
350353

‎compiler/rustc_lint/src/let_underscore.rs

+39-20
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use crate::{
55
use rustc_errors::MultiSpan;
66
use rustc_hir as hir;
77
use rustc_middle::ty;
8-
use rustc_span::Symbol;
8+
use rustc_span::{sym, Symbol};
99

1010
declare_lint! {
1111
/// The `let_underscore_drop` lint checks for statements which don't bind
@@ -105,51 +105,70 @@ const SYNC_GUARD_SYMBOLS: [Symbol; 3] = [
105105

106106
impl<'tcx> LateLintPass<'tcx> for LetUnderscore {
107107
fn check_local(&mut self, cx: &LateContext<'_>, local: &hir::Local<'_>) {
108-
if !matches!(local.pat.kind, hir::PatKind::Wild) {
109-
return;
110-
}
111-
112108
if matches!(local.source, rustc_hir::LocalSource::AsyncFn) {
113109
return;
114110
}
115-
if let Some(init) = local.init {
116-
let init_ty = cx.typeck_results().expr_ty(init);
111+
112+
let mut top_level = true;
113+
114+
// We recursively walk through all patterns, so that we can catch cases where the lock is nested in a pattern.
115+
// For the basic `let_underscore_drop` lint, we only look at the top level, since there are many legitimate reasons
116+
// to bind a sub-pattern to an `_`, if we're only interested in the rest.
117+
// But with locks, we prefer having the chance of "false positives" over missing cases, since the effects can be
118+
// quite catastrophic.
119+
local.pat.walk_always(|pat| {
120+
let is_top_level = top_level;
121+
top_level = false;
122+
123+
if !matches!(pat.kind, hir::PatKind::Wild) {
124+
return;
125+
}
126+
127+
let ty = cx.typeck_results().pat_ty(pat);
128+
117129
// If the type has a trivial Drop implementation, then it doesn't
118130
// matter that we drop the value immediately.
119-
if !init_ty.needs_drop(cx.tcx, cx.param_env) {
131+
if !ty.needs_drop(cx.tcx, cx.param_env) {
120132
return;
121133
}
122-
let is_sync_lock = match init_ty.kind() {
134+
// Lint for patterns like `mutex.lock()`, which returns `Result<MutexGuard, _>` as well.
135+
let potential_lock_type = match ty.kind() {
136+
ty::Adt(adt, args) if cx.tcx.is_diagnostic_item(sym::Result, adt.did()) => {
137+
args.type_at(0)
138+
}
139+
_ => ty,
140+
};
141+
let is_sync_lock = match potential_lock_type.kind() {
123142
ty::Adt(adt, _) => SYNC_GUARD_SYMBOLS
124143
.iter()
125144
.any(|guard_symbol| cx.tcx.is_diagnostic_item(*guard_symbol, adt.did())),
126145
_ => false,
127146
};
128147

148+
let can_use_init = is_top_level.then_some(local.init).flatten();
149+
129150
let sub = NonBindingLetSub {
130-
suggestion: local.pat.span,
131-
multi_suggestion_start: local.span.until(init.span),
132-
multi_suggestion_end: init.span.shrink_to_hi(),
151+
suggestion: pat.span,
152+
// We can't suggest `drop()` when we're on the top level.
153+
drop_fn_start_end: can_use_init
154+
.map(|init| (local.span.until(init.span), init.span.shrink_to_hi())),
133155
is_assign_desugar: matches!(local.source, rustc_hir::LocalSource::AssignDesugar(_)),
134156
};
135157
if is_sync_lock {
136-
let mut span = MultiSpan::from_spans(vec![local.pat.span, init.span]);
158+
let mut span = MultiSpan::from_span(pat.span);
137159
span.push_span_label(
138-
local.pat.span,
160+
pat.span,
139161
"this lock is not assigned to a binding and is immediately dropped".to_string(),
140162
);
141-
span.push_span_label(
142-
init.span,
143-
"this binding will immediately drop the value assigned to it".to_string(),
144-
);
145163
cx.emit_spanned_lint(LET_UNDERSCORE_LOCK, span, NonBindingLet::SyncLock { sub });
146-
} else {
164+
// Only emit let_underscore_drop for top-level `_` patterns.
165+
} else if can_use_init.is_some() {
147166
cx.emit_spanned_lint(
148167
LET_UNDERSCORE_DROP,
149168
local.span,
150169
NonBindingLet::DropType { sub },
151170
);
152171
}
153-
}
172+
});
154173
}
155174
}

‎compiler/rustc_lint/src/lints.rs

+26-17
Original file line numberDiff line numberDiff line change
@@ -930,8 +930,7 @@ pub enum NonBindingLet {
930930

931931
pub struct NonBindingLetSub {
932932
pub suggestion: Span,
933-
pub multi_suggestion_start: Span,
934-
pub multi_suggestion_end: Span,
933+
pub drop_fn_start_end: Option<(Span, Span)>,
935934
pub is_assign_desugar: bool,
936935
}
937936

@@ -940,21 +939,31 @@ impl AddToDiagnostic for NonBindingLetSub {
940939
where
941940
F: Fn(&mut Diagnostic, SubdiagnosticMessage) -> SubdiagnosticMessage,
942941
{
943-
let prefix = if self.is_assign_desugar { "let " } else { "" };
944-
diag.span_suggestion_verbose(
945-
self.suggestion,
946-
fluent::lint_non_binding_let_suggestion,
947-
format!("{prefix}_unused"),
948-
Applicability::MachineApplicable,
949-
);
950-
diag.multipart_suggestion(
951-
fluent::lint_non_binding_let_multi_suggestion,
952-
vec![
953-
(self.multi_suggestion_start, "drop(".to_string()),
954-
(self.multi_suggestion_end, ")".to_string()),
955-
],
956-
Applicability::MachineApplicable,
957-
);
942+
let can_suggest_binding = self.drop_fn_start_end.is_some() || !self.is_assign_desugar;
943+
944+
if can_suggest_binding {
945+
let prefix = if self.is_assign_desugar { "let " } else { "" };
946+
diag.span_suggestion_verbose(
947+
self.suggestion,
948+
fluent::lint_non_binding_let_suggestion,
949+
format!("{prefix}_unused"),
950+
Applicability::MachineApplicable,
951+
);
952+
} else {
953+
diag.span_help(self.suggestion, fluent::lint_non_binding_let_suggestion);
954+
}
955+
if let Some(drop_fn_start_end) = self.drop_fn_start_end {
956+
diag.multipart_suggestion(
957+
fluent::lint_non_binding_let_multi_suggestion,
958+
vec![
959+
(drop_fn_start_end.0, "drop(".to_string()),
960+
(drop_fn_start_end.1, ")".to_string()),
961+
],
962+
Applicability::MachineApplicable,
963+
);
964+
} else {
965+
diag.help(fluent::lint_non_binding_let_multi_drop_fn);
966+
}
958967
}
959968
}
960969

‎src/tools/clippy/tests/ui/let_underscore_lock.rs

+1
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ fn main() {
2626
let _ = p_rw;
2727
}
2828

29+
#[allow(let_underscore_lock)]
2930
fn uplifted() {
3031
// shouldn't lint std locks as they were uplifted as rustc's `let_underscore_lock`
3132

‎tests/ui/lint/let_underscore/let_underscore_drop.rs

+2
Original file line numberDiff line numberDiff line change
@@ -11,4 +11,6 @@ impl Drop for NontrivialDrop {
1111

1212
fn main() {
1313
let _ = NontrivialDrop; //~WARNING non-binding let on a type that implements `Drop`
14+
15+
let (_, _) = (NontrivialDrop, NontrivialDrop); // This should be ignored.
1416
}

‎tests/ui/lint/let_underscore/let_underscore_lock.rs

+15
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,22 @@
11
// check-fail
22
use std::sync::{Arc, Mutex};
33

4+
struct Struct<T> {
5+
a: T,
6+
}
7+
48
fn main() {
59
let data = Arc::new(Mutex::new(0));
610
let _ = data.lock().unwrap(); //~ERROR non-binding let on a synchronization lock
11+
12+
let _ = data.lock(); //~ERROR non-binding let on a synchronization lock
13+
14+
let (_, _) = (data.lock(), 1); //~ERROR non-binding let on a synchronization lock
15+
16+
let (_a, Struct { a: _ }) = (0, Struct { a: data.lock() }); //~ERROR non-binding let on a synchronization lock
17+
18+
(_ , _) = (data.lock(), 1); //~ERROR non-binding let on a synchronization lock
19+
20+
let _b;
21+
(_b, Struct { a: _ }) = (0, Struct { a: data.lock() }); //~ERROR non-binding let on a synchronization lock
722
}

‎tests/ui/lint/let_underscore/let_underscore_lock.stderr

+68-5
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,8 @@
11
error: non-binding let on a synchronization lock
2-
--> $DIR/let_underscore_lock.rs:6:9
2+
--> $DIR/let_underscore_lock.rs:10:9
33
|
44
LL | let _ = data.lock().unwrap();
5-
| ^ ^^^^^^^^^^^^^^^^^^^^ this binding will immediately drop the value assigned to it
6-
| |
7-
| this lock is not assigned to a binding and is immediately dropped
5+
| ^ this lock is not assigned to a binding and is immediately dropped
86
|
97
= note: `#[deny(let_underscore_lock)]` on by default
108
help: consider binding to an unused variable to avoid immediately dropping the value
@@ -16,5 +14,70 @@ help: consider immediately dropping the value
1614
LL | drop(data.lock().unwrap());
1715
| ~~~~~ +
1816

19-
error: aborting due to 1 previous error
17+
error: non-binding let on a synchronization lock
18+
--> $DIR/let_underscore_lock.rs:12:9
19+
|
20+
LL | let _ = data.lock();
21+
| ^ this lock is not assigned to a binding and is immediately dropped
22+
|
23+
help: consider binding to an unused variable to avoid immediately dropping the value
24+
|
25+
LL | let _unused = data.lock();
26+
| ~~~~~~~
27+
help: consider immediately dropping the value
28+
|
29+
LL | drop(data.lock());
30+
| ~~~~~ +
31+
32+
error: non-binding let on a synchronization lock
33+
--> $DIR/let_underscore_lock.rs:14:10
34+
|
35+
LL | let (_, _) = (data.lock(), 1);
36+
| ^ this lock is not assigned to a binding and is immediately dropped
37+
|
38+
= help: consider immediately dropping the value using `drop(..)` after the `let` statement
39+
help: consider binding to an unused variable to avoid immediately dropping the value
40+
|
41+
LL | let (_unused, _) = (data.lock(), 1);
42+
| ~~~~~~~
43+
44+
error: non-binding let on a synchronization lock
45+
--> $DIR/let_underscore_lock.rs:16:26
46+
|
47+
LL | let (_a, Struct { a: _ }) = (0, Struct { a: data.lock() });
48+
| ^ this lock is not assigned to a binding and is immediately dropped
49+
|
50+
= help: consider immediately dropping the value using `drop(..)` after the `let` statement
51+
help: consider binding to an unused variable to avoid immediately dropping the value
52+
|
53+
LL | let (_a, Struct { a: _unused }) = (0, Struct { a: data.lock() });
54+
| ~~~~~~~
55+
56+
error: non-binding let on a synchronization lock
57+
--> $DIR/let_underscore_lock.rs:18:6
58+
|
59+
LL | (_ , _) = (data.lock(), 1);
60+
| ^ this lock is not assigned to a binding and is immediately dropped
61+
|
62+
help: consider binding to an unused variable to avoid immediately dropping the value
63+
--> $DIR/let_underscore_lock.rs:18:6
64+
|
65+
LL | (_ , _) = (data.lock(), 1);
66+
| ^
67+
= help: consider immediately dropping the value using `drop(..)` after the `let` statement
68+
69+
error: non-binding let on a synchronization lock
70+
--> $DIR/let_underscore_lock.rs:21:22
71+
|
72+
LL | (_b, Struct { a: _ }) = (0, Struct { a: data.lock() });
73+
| ^ this lock is not assigned to a binding and is immediately dropped
74+
|
75+
help: consider binding to an unused variable to avoid immediately dropping the value
76+
--> $DIR/let_underscore_lock.rs:21:22
77+
|
78+
LL | (_b, Struct { a: _ }) = (0, Struct { a: data.lock() });
79+
| ^
80+
= help: consider immediately dropping the value using `drop(..)` after the `let` statement
81+
82+
error: aborting due to 6 previous errors
2083

0 commit comments

Comments
 (0)