Skip to content

Commit

Permalink
new lint: redundant_local
Browse files Browse the repository at this point in the history
fix dogfood lints in `redundant_local`

keep `redundant_local` from running in proc macros

rewrite `redundant_local` as late pass
  • Loading branch information
max-niederman committed Jun 9, 2023
1 parent 384cf37 commit c3d756f
Show file tree
Hide file tree
Showing 13 changed files with 383 additions and 43 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5106,6 +5106,7 @@ Released 2018-09-13
[`redundant_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_else
[`redundant_feature_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_feature_names
[`redundant_field_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_field_names
[`redundant_local`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_local
[`redundant_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pattern
[`redundant_pattern_matching`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pattern_matching
[`redundant_pub_crate`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pub_crate
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::redundant_closure_call::REDUNDANT_CLOSURE_CALL_INFO,
crate::redundant_else::REDUNDANT_ELSE_INFO,
crate::redundant_field_names::REDUNDANT_FIELD_NAMES_INFO,
crate::redundant_local::REDUNDANT_LOCAL_INFO,
crate::redundant_pub_crate::REDUNDANT_PUB_CRATE_INFO,
crate::redundant_slicing::DEREF_BY_SLICING_INFO,
crate::redundant_slicing::REDUNDANT_SLICING_INFO,
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,7 @@ mod redundant_clone;
mod redundant_closure_call;
mod redundant_else;
mod redundant_field_names;
mod redundant_local;
mod redundant_pub_crate;
mod redundant_slicing;
mod redundant_static_lifetimes;
Expand Down Expand Up @@ -1021,6 +1022,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(|_| Box::new(endian_bytes::EndianBytes));
store.register_late_pass(|_| Box::new(redundant_type_annotations::RedundantTypeAnnotations));
store.register_late_pass(|_| Box::new(arc_with_non_send_sync::ArcWithNonSendSync));
store.register_late_pass(|_| Box::new(redundant_local::RedundantLocal));
// add lints here, do not remove this comment, it's used in `new_lint`
}

Expand Down
110 changes: 110 additions & 0 deletions clippy_lints/src/redundant_local.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
use clippy_utils::{diagnostics::span_lint_and_help, is_from_proc_macro, ty::needs_ordered_drop};
use rustc_hir::{def::Res, BindingAnnotation, ByRef, Expr, ExprKind, HirId, Local, Node, Pat, PatKind, QPath};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_middle::lint::in_external_macro;
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::symbol::Ident;

declare_clippy_lint! {
/// ### What it does
/// Checks for redundant redefinitions of local bindings.
///
/// ### Why is this bad?
/// Redundant redefinitions of local bindings do not change behavior and are likely to be unintended.
///
/// Note that although these bindings do not affect your code's meaning, they _may_ affect `rustc`'s stack allocation.
///
/// ### Example
/// ```rust
/// let a = 0;
/// let a = a;
///
/// fn foo(b: i32) {
/// let b = b;
/// }
/// ```
/// Use instead:
/// ```rust
/// let a = 0;
/// // no redefinition with the same name
///
/// fn foo(b: i32) {
/// // no redefinition with the same name
/// }
/// ```
#[clippy::version = "1.72.0"]
pub REDUNDANT_LOCAL,
correctness,
"redundant redefinition of a local binding"
}
declare_lint_pass!(RedundantLocal => [REDUNDANT_LOCAL]);

impl<'tcx> LateLintPass<'tcx> for RedundantLocal {
fn check_local(&mut self, cx: &LateContext<'tcx>, local: &'tcx Local<'tcx>) {
if_chain! {
// the pattern is a single by-value binding
if let PatKind::Binding(BindingAnnotation(ByRef::No, mutability), _, ident, None) = local.pat.kind;
// the binding is not type-ascribed
if local.ty.is_none();
// the expression is a single-segment path
if let Some(expr) = local.init;
if let ExprKind::Path(qpath @ QPath::Resolved(None, path)) = expr.kind;
// the path is a single segment equal to the local's name
if path.segments.len() == 1;
if path.segments[0].ident == ident;
// resolve the path to its defining binding pattern
if let Res::Local(binding_id) = cx.qpath_res(&qpath, expr.hir_id);
if let Node::Pat(binding_pat) = cx.tcx.hir().get(binding_id);
// the previous binding has the same mutability
if find_binding(binding_pat, ident).unwrap().1 == mutability;
// the local does not affect the code's drop behavior
if !affects_drop_behavior(cx, binding_id, local.hir_id, expr);
// the local is user-controlled
if !in_external_macro(cx.sess(), local.span);
if !is_from_proc_macro(cx, expr);
then {
span_lint_and_help(
cx,
REDUNDANT_LOCAL,
vec![binding_pat.span, local.span],
"redundant redefinition of a binding",
None,
&format!("remove the redefinition of `{ident}`"),
);
}
}
}
}

/// Find the annotation of a binding introduced by a pattern, or `None` if it's not introduced.
fn find_binding(pat: &Pat<'_>, name: Ident) -> Option<BindingAnnotation> {
match pat.kind {
PatKind::Binding(annotation, _, ident, _) if ident == name => Some(annotation),
PatKind::Binding(_, _, _, Some(subpat)) => find_binding(subpat, name),
PatKind::Struct(_, fields, _) => fields.iter().find_map(|field| find_binding(field.pat, name)),
PatKind::TupleStruct(_, fields, _) | PatKind::Or(fields) | PatKind::Tuple(fields, _) => {
fields.iter().find_map(|field| find_binding(field, name))
},
PatKind::Slice(before, middle, after) => before
.iter()
.chain(middle)
.chain(after.iter())
.find_map(|field| find_binding(field, name)),
PatKind::Box(inner) | PatKind::Ref(inner, _) => find_binding(inner, name),
PatKind::Wild
| PatKind::Binding(_, _, _, None)
| PatKind::Path(_)
| PatKind::Lit(_)
| PatKind::Range(_, _, _) => None,
}
}

/// Check if a rebinding of a local affects the code's drop behavior.
fn affects_drop_behavior<'tcx>(cx: &LateContext<'tcx>, bind: HirId, rebind: HirId, rebind_expr: &Expr<'tcx>) -> bool {
let hir = cx.tcx.hir();

// the rebinding is in a different scope than the original binding
// and the type of the binding cares about drop order
hir.get_enclosing_scope(bind) != hir.get_enclosing_scope(rebind)
&& needs_ordered_drop(cx, cx.typeck_results().expr_ty(rebind_expr))
}
3 changes: 2 additions & 1 deletion tests/ui/option_if_let_else.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
clippy::redundant_closure,
clippy::ref_option_ref,
clippy::equatable_if_let,
clippy::let_unit_value
clippy::let_unit_value,
clippy::redundant_local
)]

fn bad1(string: Option<&str>) -> (bool, &str) {
Expand Down
3 changes: 2 additions & 1 deletion tests/ui/option_if_let_else.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
clippy::redundant_closure,
clippy::ref_option_ref,
clippy::equatable_if_let,
clippy::let_unit_value
clippy::let_unit_value,
clippy::redundant_local
)]

fn bad1(string: Option<&str>) -> (bool, &str) {
Expand Down
42 changes: 21 additions & 21 deletions tests/ui/option_if_let_else.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:12:5
--> $DIR/option_if_let_else.rs:13:5
|
LL | / if let Some(x) = string {
LL | | (true, x)
Expand All @@ -11,19 +11,19 @@ LL | | }
= note: `-D clippy::option-if-let-else` implied by `-D warnings`

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

0 comments on commit c3d756f

Please sign in to comment.