Skip to content

Commit a44dcf8

Browse files
borsbors
committed
Auto merge of #10885 - max-niederman:redundant_local, r=Centri3
new lint: `redundant_locals` This lint checks for code like the following: ```rs let x = 1; let x = x; ``` It checks (afaik) all cases where a binding is shadowed by its own value in the same block, including function parameters. This has no effect and is almost certainly accidental, so it's in the `correctness` category like `self_assignment`. This also lays the groundwork for a more generalized version of #5102. changelog: new lint: [`redundant_local`]
2 parents 58df1e6 + 008ba2b commit a44dcf8

File tree

13 files changed

+403
-45
lines changed

13 files changed

+403
-45
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5190,6 +5190,7 @@ Released 2018-09-13
51905190
[`redundant_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_else
51915191
[`redundant_feature_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_feature_names
51925192
[`redundant_field_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_field_names
5193+
[`redundant_locals`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_locals
51935194
[`redundant_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pattern
51945195
[`redundant_pattern_matching`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pattern_matching
51955196
[`redundant_pub_crate`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pub_crate

‎clippy_lints/src/declared_lints.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -561,6 +561,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
561561
crate::redundant_closure_call::REDUNDANT_CLOSURE_CALL_INFO,
562562
crate::redundant_else::REDUNDANT_ELSE_INFO,
563563
crate::redundant_field_names::REDUNDANT_FIELD_NAMES_INFO,
564+
crate::redundant_locals::REDUNDANT_LOCALS_INFO,
564565
crate::redundant_pub_crate::REDUNDANT_PUB_CRATE_INFO,
565566
crate::redundant_slicing::DEREF_BY_SLICING_INFO,
566567
crate::redundant_slicing::REDUNDANT_SLICING_INFO,

‎clippy_lints/src/lib.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,7 @@ mod redundant_clone;
275275
mod redundant_closure_call;
276276
mod redundant_else;
277277
mod redundant_field_names;
278+
mod redundant_locals;
278279
mod redundant_pub_crate;
279280
mod redundant_slicing;
280281
mod redundant_static_lifetimes;
@@ -1091,6 +1092,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
10911092
absolute_paths_allowed_crates: absolute_paths_allowed_crates.clone(),
10921093
})
10931094
});
1095+
store.register_late_pass(|_| Box::new(redundant_locals::RedundantLocals));
10941096
// add lints here, do not remove this comment, it's used in `new_lint`
10951097
}
10961098

‎clippy_lints/src/redundant_locals.rs‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
use clippy_utils::diagnostics::span_lint_and_help;
2+
use clippy_utils::is_from_proc_macro;
3+
use clippy_utils::ty::needs_ordered_drop;
4+
use rustc_hir::def::Res;
5+
use rustc_hir::{BindingAnnotation, ByRef, Expr, ExprKind, HirId, Local, Node, Pat, PatKind, QPath};
6+
use rustc_lint::{LateContext, LateLintPass, LintContext};
7+
use rustc_middle::lint::in_external_macro;
8+
use rustc_session::{declare_lint_pass, declare_tool_lint};
9+
use rustc_span::symbol::Ident;
10+
11+
declare_clippy_lint! {
12+
/// ### What it does
13+
/// Checks for redundant redefinitions of local bindings.
14+
///
15+
/// ### Why is this bad?
16+
/// Redundant redefinitions of local bindings do not change behavior and are likely to be unintended.
17+
///
18+
/// Note that although these bindings do not affect your code's meaning, they _may_ affect `rustc`'s stack allocation.
19+
///
20+
/// ### Example
21+
/// ```rust
22+
/// let a = 0;
23+
/// let a = a;
24+
///
25+
/// fn foo(b: i32) {
26+
/// let b = b;
27+
/// }
28+
/// ```
29+
/// Use instead:
30+
/// ```rust
31+
/// let a = 0;
32+
/// // no redefinition with the same name
33+
///
34+
/// fn foo(b: i32) {
35+
/// // no redefinition with the same name
36+
/// }
37+
/// ```
38+
#[clippy::version = "1.72.0"]
39+
pub REDUNDANT_LOCALS,
40+
correctness,
41+
"redundant redefinition of a local binding"
42+
}
43+
declare_lint_pass!(RedundantLocals => [REDUNDANT_LOCALS]);
44+
45+
impl<'tcx> LateLintPass<'tcx> for RedundantLocals {
46+
fn check_local(&mut self, cx: &LateContext<'tcx>, local: &'tcx Local<'tcx>) {
47+
if_chain! {
48+
// the pattern is a single by-value binding
49+
if let PatKind::Binding(BindingAnnotation(ByRef::No, mutability), _, ident, None) = local.pat.kind;
50+
// the binding is not type-ascribed
51+
if local.ty.is_none();
52+
// the expression is a resolved path
53+
if let Some(expr) = local.init;
54+
if let ExprKind::Path(qpath @ QPath::Resolved(None, path)) = expr.kind;
55+
// the path is a single segment equal to the local's name
56+
if let [last_segment] = path.segments;
57+
if last_segment.ident == ident;
58+
// resolve the path to its defining binding pattern
59+
if let Res::Local(binding_id) = cx.qpath_res(&qpath, expr.hir_id);
60+
if let Node::Pat(binding_pat) = cx.tcx.hir().get(binding_id);
61+
// the previous binding has the same mutability
62+
if find_binding(binding_pat, ident).unwrap().1 == mutability;
63+
// the local does not affect the code's drop behavior
64+
if !affects_drop_behavior(cx, binding_id, local.hir_id, expr);
65+
// the local is user-controlled
66+
if !in_external_macro(cx.sess(), local.span);
67+
if !is_from_proc_macro(cx, expr);
68+
then {
69+
span_lint_and_help(
70+
cx,
71+
REDUNDANT_LOCALS,
72+
vec![binding_pat.span, local.span],
73+
"redundant redefinition of a binding",
74+
None,
75+
&format!("remove the redefinition of `{ident}`"),
76+
);
77+
}
78+
}
79+
}
80+
}
81+
82+
/// Find the annotation of a binding introduced by a pattern, or `None` if it's not introduced.
83+
fn find_binding(pat: &Pat<'_>, name: Ident) -> Option<BindingAnnotation> {
84+
let mut ret = None;
85+
86+
pat.each_binding_or_first(&mut |annotation, _, _, ident| {
87+
if ident == name {
88+
ret = Some(annotation);
89+
}
90+
});
91+
92+
ret
93+
}
94+
95+
/// Check if a rebinding of a local affects the code's drop behavior.
96+
fn affects_drop_behavior<'tcx>(cx: &LateContext<'tcx>, bind: HirId, rebind: HirId, rebind_expr: &Expr<'tcx>) -> bool {
97+
let hir = cx.tcx.hir();
98+
99+
// the rebinding is in a different scope than the original binding
100+
// and the type of the binding cares about drop order
101+
hir.get_enclosing_scope(bind) != hir.get_enclosing_scope(rebind)
102+
&& needs_ordered_drop(cx, cx.typeck_results().expr_ty(rebind_expr))
103+
}

‎tests/ui/option_if_let_else.fixed‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@
55
clippy::redundant_closure,
66
clippy::ref_option_ref,
77
clippy::equatable_if_let,
8-
clippy::let_unit_value
8+
clippy::let_unit_value,
9+
clippy::redundant_locals
910
)]
1011

1112
fn bad1(string: Option<&str>) -> (bool, &str) {

‎tests/ui/option_if_let_else.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@
55
clippy::redundant_closure,
66
clippy::ref_option_ref,
77
clippy::equatable_if_let,
8-
clippy::let_unit_value
8+
clippy::let_unit_value,
9+
clippy::redundant_locals
910
)]
1011

1112
fn bad1(string: Option<&str>) -> (bool, &str) {

‎tests/ui/option_if_let_else.stderr‎

Lines changed: 23 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: use Option::map_or instead of an if let/else
2-
--> $DIR/option_if_let_else.rs:12:5
2+
--> $DIR/option_if_let_else.rs:13:5
33
|
44
LL | / if let Some(x) = string {
55
LL | | (true, x)
@@ -11,19 +11,19 @@ LL | | }
1111
= note: `-D clippy::option-if-let-else` implied by `-D warnings`
1212

1313
error: use Option::map_or instead of an if let/else
14-
--> $DIR/option_if_let_else.rs:30:13
14+
--> $DIR/option_if_let_else.rs:31:13
1515
|
1616
LL | let _ = if let Some(s) = *string { s.len() } else { 0 };
1717
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `string.map_or(0, |s| s.len())`
1818

1919
error: use Option::map_or instead of an if let/else
20-
--> $DIR/option_if_let_else.rs:31:13
20+
--> $DIR/option_if_let_else.rs:32:13
2121
|
2222
LL | let _ = if let Some(s) = &num { s } else { &0 };
2323
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`
2424

2525
error: use Option::map_or instead of an if let/else
26-
--> $DIR/option_if_let_else.rs:32:13
26+
--> $DIR/option_if_let_else.rs:33:13
2727
|
2828
LL | let _ = if let Some(s) = &mut num {
2929
| _____________^
@@ -43,13 +43,13 @@ LL ~ });
4343
|
4444

4545
error: use Option::map_or instead of an if let/else
46-
--> $DIR/option_if_let_else.rs:38:13
46+
--> $DIR/option_if_let_else.rs:39:13
4747
|
4848
LL | let _ = if let Some(ref s) = num { s } else { &0 };
4949
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`
5050

5151
error: use Option::map_or instead of an if let/else
52-
--> $DIR/option_if_let_else.rs:39:13
52+
--> $DIR/option_if_let_else.rs:40:13
5353
|
5454
LL | let _ = if let Some(mut s) = num {
5555
| _____________^
@@ -69,7 +69,7 @@ LL ~ });
6969
|
7070

7171
error: use Option::map_or instead of an if let/else
72-
--> $DIR/option_if_let_else.rs:45:13
72+
--> $DIR/option_if_let_else.rs:46:13
7373
|
7474
LL | let _ = if let Some(ref mut s) = num {
7575
| _____________^
@@ -89,7 +89,7 @@ LL ~ });
8989
|
9090

9191
error: use Option::map_or instead of an if let/else
92-
--> $DIR/option_if_let_else.rs:54:5
92+
--> $DIR/option_if_let_else.rs:55:5
9393
|
9494
LL | / if let Some(x) = arg {
9595
LL | | let y = x * x;
@@ -108,7 +108,7 @@ LL + })
108108
|
109109

110110
error: use Option::map_or_else instead of an if let/else
111-
--> $DIR/option_if_let_else.rs:67:13
111+
--> $DIR/option_if_let_else.rs:68:13
112112
|
113113
LL | let _ = if let Some(x) = arg {
114114
| _____________^
@@ -120,7 +120,7 @@ LL | | };
120120
| |_____^ help: try: `arg.map_or_else(|| side_effect(), |x| x)`
121121

122122
error: use Option::map_or_else instead of an if let/else
123-
--> $DIR/option_if_let_else.rs:76:13
123+
--> $DIR/option_if_let_else.rs:77:13
124124
|
125125
LL | let _ = if let Some(x) = arg {
126126
| _____________^
@@ -143,7 +143,7 @@ LL ~ }, |x| x * x * x * x);
143143
|
144144

145145
error: use Option::map_or_else instead of an if let/else
146-
--> $DIR/option_if_let_else.rs:109:13
146+
--> $DIR/option_if_let_else.rs:110:13
147147
|
148148
LL | / if let Some(idx) = s.find('.') {
149149
LL | | vec![s[..idx].to_string(), s[idx..].to_string()]
@@ -153,7 +153,7 @@ LL | | }
153153
| |_____________^ help: try: `s.find('.').map_or_else(|| vec![s.to_string()], |idx| vec![s[..idx].to_string(), s[idx..].to_string()])`
154154

155155
error: use Option::map_or_else instead of an if let/else
156-
--> $DIR/option_if_let_else.rs:120:5
156+
--> $DIR/option_if_let_else.rs:121:5
157157
|
158158
LL | / if let Ok(binding) = variable {
159159
LL | | println!("Ok {binding}");
@@ -172,13 +172,13 @@ LL + })
172172
|
173173

174174
error: use Option::map_or instead of an if let/else
175-
--> $DIR/option_if_let_else.rs:142:13
175+
--> $DIR/option_if_let_else.rs:143:13
176176
|
177177
LL | let _ = if let Some(x) = optional { x + 2 } else { 5 };
178178
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)`
179179

180180
error: use Option::map_or instead of an if let/else
181-
--> $DIR/option_if_let_else.rs:152:13
181+
--> $DIR/option_if_let_else.rs:153:13
182182
|
183183
LL | let _ = if let Some(x) = Some(0) {
184184
| _____________^
@@ -200,13 +200,13 @@ LL ~ });
200200
|
201201

202202
error: use Option::map_or instead of an if let/else
203-
--> $DIR/option_if_let_else.rs:180:13
203+
--> $DIR/option_if_let_else.rs:181:13
204204
|
205205
LL | let _ = if let Some(x) = Some(0) { s.len() + x } else { s.len() };
206206
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(0).map_or(s.len(), |x| s.len() + x)`
207207

208208
error: use Option::map_or instead of an if let/else
209-
--> $DIR/option_if_let_else.rs:184:13
209+
--> $DIR/option_if_let_else.rs:185:13
210210
|
211211
LL | let _ = if let Some(x) = Some(0) {
212212
| _____________^
@@ -226,7 +226,7 @@ LL ~ });
226226
|
227227

228228
error: use Option::map_or instead of an if let/else
229-
--> $DIR/option_if_let_else.rs:223:13
229+
--> $DIR/option_if_let_else.rs:224:13
230230
|
231231
LL | let _ = match s {
232232
| _____________^
@@ -236,7 +236,7 @@ LL | | };
236236
| |_____^ help: try: `s.map_or(1, |string| string.len())`
237237

238238
error: use Option::map_or instead of an if let/else
239-
--> $DIR/option_if_let_else.rs:227:13
239+
--> $DIR/option_if_let_else.rs:228:13
240240
|
241241
LL | let _ = match Some(10) {
242242
| _____________^
@@ -246,7 +246,7 @@ LL | | };
246246
| |_____^ help: try: `Some(10).map_or(5, |a| a + 1)`
247247

248248
error: use Option::map_or instead of an if let/else
249-
--> $DIR/option_if_let_else.rs:233:13
249+
--> $DIR/option_if_let_else.rs:234:13
250250
|
251251
LL | let _ = match res {
252252
| _____________^
@@ -256,7 +256,7 @@ LL | | };
256256
| |_____^ help: try: `res.map_or(1, |a| a + 1)`
257257

258258
error: use Option::map_or instead of an if let/else
259-
--> $DIR/option_if_let_else.rs:237:13
259+
--> $DIR/option_if_let_else.rs:238:13
260260
|
261261
LL | let _ = match res {
262262
| _____________^
@@ -266,13 +266,13 @@ LL | | };
266266
| |_____^ help: try: `res.map_or(1, |a| a + 1)`
267267

268268
error: use Option::map_or instead of an if let/else
269-
--> $DIR/option_if_let_else.rs:241:13
269+
--> $DIR/option_if_let_else.rs:242:13
270270
|
271271
LL | let _ = if let Ok(a) = res { a + 1 } else { 5 };
272272
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `res.map_or(5, |a| a + 1)`
273273

274274
error: use Option::map_or instead of an if let/else
275-
--> $DIR/option_if_let_else.rs:258:9
275+
--> $DIR/option_if_let_else.rs:259:9
276276
|
277277
LL | / match initial {
278278
LL | | Some(value) => do_something(value),
@@ -281,7 +281,7 @@ LL | | }
281281
| |_________^ help: try: `initial.as_ref().map_or({}, |value| do_something(value))`
282282

283283
error: use Option::map_or instead of an if let/else
284-
--> $DIR/option_if_let_else.rs:265:9
284+
--> $DIR/option_if_let_else.rs:266:9
285285
|
286286
LL | / match initial {
287287
LL | | Some(value) => do_something2(value),

0 commit comments

Comments
 (0)