Skip to content

Commit c3d756f

Browse files
max-niedermanmax-niederman
committed
new lint: redundant_local
fix dogfood lints in `redundant_local` keep `redundant_local` from running in proc macros rewrite `redundant_local` as late pass
1 parent 384cf37 commit c3d756f

13 files changed

+383
-43
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5106,6 +5106,7 @@ Released 2018-09-13
51065106
[`redundant_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_else
51075107
[`redundant_feature_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_feature_names
51085108
[`redundant_field_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_field_names
5109+
[`redundant_local`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_local
51095110
[`redundant_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pattern
51105111
[`redundant_pattern_matching`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pattern_matching
51115112
[`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
@@ -540,6 +540,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
540540
crate::redundant_closure_call::REDUNDANT_CLOSURE_CALL_INFO,
541541
crate::redundant_else::REDUNDANT_ELSE_INFO,
542542
crate::redundant_field_names::REDUNDANT_FIELD_NAMES_INFO,
543+
crate::redundant_local::REDUNDANT_LOCAL_INFO,
543544
crate::redundant_pub_crate::REDUNDANT_PUB_CRATE_INFO,
544545
crate::redundant_slicing::DEREF_BY_SLICING_INFO,
545546
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
@@ -265,6 +265,7 @@ mod redundant_clone;
265265
mod redundant_closure_call;
266266
mod redundant_else;
267267
mod redundant_field_names;
268+
mod redundant_local;
268269
mod redundant_pub_crate;
269270
mod redundant_slicing;
270271
mod redundant_static_lifetimes;
@@ -1021,6 +1022,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
10211022
store.register_late_pass(|_| Box::new(endian_bytes::EndianBytes));
10221023
store.register_late_pass(|_| Box::new(redundant_type_annotations::RedundantTypeAnnotations));
10231024
store.register_late_pass(|_| Box::new(arc_with_non_send_sync::ArcWithNonSendSync));
1025+
store.register_late_pass(|_| Box::new(redundant_local::RedundantLocal));
10241026
// add lints here, do not remove this comment, it's used in `new_lint`
10251027
}
10261028

‎clippy_lints/src/redundant_local.rs

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

‎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_local
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_local
910
)]
1011

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

‎tests/ui/option_if_let_else.stderr

Lines changed: 21 additions & 21 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,7 +266,7 @@ 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)`

0 commit comments

Comments
 (0)