use visitors to support full block

Jacherr · Jacherr · commit a6f952b2834c · 2024-03-13T00:35:39.000Z
diff --git a/clippy_lints/src/manual_strip.rs b/clippy_lints/src/manual_strip.rs
@@ -95,7 +95,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualStrip {
             }
 
             let strippings = find_stripping(cx, strip_kind, target_res, pattern, then);
-            if !strippings.is_empty() {
+            if let Some(first_stripping) = strippings.first() {
                 let kind_word = match strip_kind {
                     StripKind::Prefix => "prefix",
                     StripKind::Suffix => "suffix",
@@ -105,7 +105,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualStrip {
                 span_lint_and_then(
                     cx,
                     MANUAL_STRIP,
-                    strippings[0],
+                    *first_stripping,
                     &format!("stripping a {kind_word} manually"),
                     |diag| {
                         diag.span_note(test_span, format!("the {kind_word} was tested here"));
diff --git a/clippy_lints/src/unnecessary_indexing.rs b/clippy_lints/src/unnecessary_indexing.rs
@@ -1,13 +1,16 @@
+use std::ops::ControlFlow;
+
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::eq_expr_value;
 use clippy_utils::source::snippet;
 use clippy_utils::ty::is_type_diagnostic_item;
+use clippy_utils::visitors::for_each_expr;
 use rustc_ast::LitKind;
 use rustc_errors::Applicability;
-use rustc_hir::{ExprKind, Local, StmtKind, UnOp};
+use rustc_hir::{Expr, ExprKind, Local, Node, UnOp};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::declare_lint_pass;
-use rustc_span::{sym, Span};
+use rustc_span::sym;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -53,111 +56,106 @@ impl LateLintPass<'_> for UnnecessaryIndexing {
             && (expr_ty.is_array_slice() || expr_ty.is_array() || is_type_diagnostic_item(cx, expr_ty, sym::Vec))
             && let ExprKind::Block(block, _) = if_expr.then.kind
         {
-            // checked if conditional is calling `is_empty` on a sequence, now check if the first
-            // statement inside the 'if' statement does unchecked get of first element
+            // the receiver for the index operation
+            let mut index_receiver: Option<&Expr<'_>> = None;
+            // first local in the block - used as pattern for `Some(pat)`
+            let mut first_local: Option<&Local<'_>> = None;
+            // any other locals to be aware of, these are set to the value of `pat`
+            let mut extra_locals: Vec<&Local<'_>> = vec![];
+            // any other index expressions to replace with `pat` (or "element" if no local exists)
+            let mut extra_exprs: Vec<&Expr<'_>> = vec![];
 
-            // if calling a function on the indexing - local
-            if let Some(first) = block.stmts.first()
-                && let StmtKind::Local(local) = first.kind
-                && let Some(init) = local.init
-                && let ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args, _) = init.kind
-            {
-                for arg_expr in args {
-                    if check_arg_stmt(cx, expr, arg_expr, conditional_receiver, if_expr.cond.span) {
-                        break;
-                    }
-                }
-            // if calling a function on the indexing - statement or expression
-            } else if let Some(first) = block.stmts.first()
-                && let StmtKind::Expr(stmt_expr) | StmtKind::Semi(stmt_expr) = first.kind
-                && let ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args, _) = stmt_expr.kind
-            {
-                for arg_expr in args {
-                    if check_arg_stmt(cx, expr, arg_expr, conditional_receiver, if_expr.cond.span) {
-                        break;
-                    }
-                }
-            // if calling on a local which is not in itself a call or methodcall
-            } else if let Some(first) = block.stmts.first()
-                && let StmtKind::Local(local) = first.kind
-                && let Some(_) = local.init
-            {
-                check_local_stmt(cx, expr, local, conditional_receiver, if_expr.cond.span);
-            }
-        }
-    }
-}
+            for_each_expr(block.stmts, |x| {
+                if let ExprKind::Index(receiver, index, _) = x.kind
+                    && let ExprKind::Lit(lit) = index.kind
+                    && let LitKind::Int(val, _) = lit.node
+                    && eq_expr_value(cx, receiver, conditional_receiver)
+                    && val.0 == 0
+                {
+                    index_receiver = Some(receiver);
+                    if let Node::Local(local) = cx.tcx.parent_hir_node(x.hir_id) {
+                        if first_local.is_none() {
+                            first_local = Some(local);
+                        } else {
+                            extra_locals.push(local);
+                        };
+                    } else {
+                        extra_exprs.push(x);
+                    };
+                };
 
-fn check_local_stmt(
-    cx: &LateContext<'_>,
-    expr: &'_ rustc_hir::Expr<'_>,
-    local: &Local<'_>,
-    conditional_receiver: &'_ rustc_hir::Expr<'_>,
-    cond_span: Span,
-) {
-    if let ExprKind::Index(receiver, index, _) = local.init.unwrap().kind
-        && let ExprKind::Lit(lit) = index.kind
-        && let LitKind::Int(val, _) = lit.node
-        && eq_expr_value(cx, receiver, conditional_receiver)
-        && val.0 == 0
-    {
-        span_lint_and_then(
-            cx,
-            UNNECESSARY_INDEXING,
-            expr.span,
-            "condition can be simplified with if..let syntax",
-            |x| {
-                x.span_suggestion(
-                    cond_span,
-                    "consider using if..let syntax (variable may need to be dereferenced)",
-                    format!(
-                        "let Some({}) = {}.first()",
-                        snippet(cx, local.pat.span, ".."),
-                        snippet(cx, receiver.span, "..")
-                    ),
-                    Applicability::Unspecified,
-                );
-                x.span_suggestion(local.span, "remove this line", "", Applicability::MachineApplicable);
-            },
-        );
-    }
-}
+                ControlFlow::Continue::<()>(())
+            });
 
-fn check_arg_stmt(
-    cx: &LateContext<'_>,
-    expr: &'_ rustc_hir::Expr<'_>,
-    arg_expr: &'_ rustc_hir::Expr<'_>,
-    conditional_receiver: &'_ rustc_hir::Expr<'_>,
-    cond_span: Span,
-) -> bool {
-    if let ExprKind::Index(receiver, index, _) = arg_expr.kind
-        && let ExprKind::Lit(lit) = index.kind
-        && let LitKind::Int(val, _) = lit.node
-        && eq_expr_value(cx, receiver, conditional_receiver)
-        && val.0 == 0
-    {
-        span_lint_and_then(
-            cx,
-            UNNECESSARY_INDEXING,
-            expr.span,
-            "condition can be simplified with if..let syntax",
-            |x| {
-                x.multipart_suggestion(
-                    "consider using if..let syntax (variable may need to be dereferenced)",
-                    vec![
-                        (
-                            cond_span,
-                            format!("let Some(element) = {}.first()", snippet(cx, receiver.span, "..")),
-                        ),
-                        (arg_expr.span, "element".to_owned()),
-                    ],
-                    Applicability::Unspecified,
-                );
-            },
-        );
+            if let Some(receiver) = index_receiver {
+                span_lint_and_then(
+                    cx,
+                    UNNECESSARY_INDEXING,
+                    expr.span,
+                    "condition can be simplified with if..let syntax",
+                    |x| {
+                        if let Some(first_local) = first_local {
+                            x.span_suggestion(
+                                if_expr.cond.span,
+                                "consider using if..let syntax (variable may need to be dereferenced)",
+                                format!(
+                                    "let Some({}) = {}.first()",
+                                    snippet(cx, first_local.pat.span, ".."),
+                                    snippet(cx, receiver.span, "..")
+                                ),
+                                Applicability::Unspecified,
+                            );
+                            x.span_suggestion(
+                                first_local.span,
+                                "remove this line",
+                                "",
+                                Applicability::MachineApplicable,
+                            );
+                            if !extra_locals.is_empty() {
+                                let extra_local_suggestions = extra_locals
+                                    .iter()
+                                    .map(|x| {
+                                        (
+                                            x.init.unwrap().span,
+                                            snippet(cx, first_local.pat.span, "..").to_string(),
+                                        )
+                                    })
+                                    .collect::<Vec<_>>();
 
-        return true;
-    }
+                                x.multipart_suggestion(
+                                    "initialize this variable to be the `Some` variant (may need dereferencing)",
+                                    extra_local_suggestions,
+                                    Applicability::Unspecified,
+                                );
+                            }
+                            if !extra_exprs.is_empty() {
+                                let index_accesses = extra_exprs
+                                    .iter()
+                                    .map(|x| (x.span, snippet(cx, first_local.pat.span, "..").to_string()))
+                                    .collect::<Vec<_>>();
 
-    false
+                                x.multipart_suggestion(
+                                    "set this index to be the `Some` variant (may need dereferencing)",
+                                    index_accesses,
+                                    Applicability::Unspecified,
+                                );
+                            }
+                        } else {
+                            let mut index_accesses = vec![(
+                                if_expr.cond.span,
+                                format!("let Some(element) = {}.first()", snippet(cx, receiver.span, "..")),
+                            )];
+                            index_accesses.extend(extra_exprs.iter().map(|x| (x.span, "element".to_owned())));
+
+                            x.multipart_suggestion(
+                                "consider using if..let syntax (variable may need to be dereferenced)",
+                                index_accesses,
+                                Applicability::Unspecified,
+                            );
+                        }
+                    },
+                );
+            }
+        }
+    }
 }
diff --git a/tests/ui/index_refutable_slice/if_let_slice_binding.fixed b/tests/ui/index_refutable_slice/if_let_slice_binding.fixed
@@ -1,5 +1,6 @@
 #![deny(clippy::index_refutable_slice)]
 #![allow(clippy::uninlined_format_args)]
+#![allow(clippy::unnecessary_indexing)]
 
 enum SomeEnum<T> {
     One(T),
diff --git a/tests/ui/index_refutable_slice/if_let_slice_binding.rs b/tests/ui/index_refutable_slice/if_let_slice_binding.rs
@@ -1,5 +1,6 @@
 #![deny(clippy::index_refutable_slice)]
 #![allow(clippy::uninlined_format_args)]
+#![allow(clippy::unnecessary_indexing)]
 
 enum SomeEnum<T> {
     One(T),
diff --git a/tests/ui/index_refutable_slice/if_let_slice_binding.stderr b/tests/ui/index_refutable_slice/if_let_slice_binding.stderr
@@ -1,5 +1,5 @@
 error: this binding can be a slice pattern to avoid indexing
-  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:14:17
+  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:15:17
    |
 LL |     if let Some(slice) = slice {
    |                 ^^^^^
@@ -19,7 +19,7 @@ LL |         println!("{}", slice_0);
    |                        ~~~~~~~
 
 error: this binding can be a slice pattern to avoid indexing
-  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:21:17
+  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:22:17
    |
 LL |     if let Some(slice) = slice {
    |                 ^^^^^
@@ -34,7 +34,7 @@ LL |         println!("{}", slice_0);
    |                        ~~~~~~~
 
 error: this binding can be a slice pattern to avoid indexing
-  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:28:17
+  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:29:17
    |
 LL |     if let Some(slice) = slice {
    |                 ^^^^^
@@ -50,7 +50,7 @@ LL ~         println!("{}", slice_0);
    |
 
 error: this binding can be a slice pattern to avoid indexing
-  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:36:26
+  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:37:26
    |
 LL |     if let SomeEnum::One(slice) | SomeEnum::Three(slice) = slice_wrapped {
    |                          ^^^^^
@@ -65,7 +65,7 @@ LL |         println!("{}", slice_0);
    |                        ~~~~~~~
 
 error: this binding can be a slice pattern to avoid indexing
-  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:44:29
+  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:45:29
    |
 LL |     if let (SomeEnum::Three(a), Some(b)) = (a_wrapped, b_wrapped) {
    |                             ^
@@ -80,7 +80,7 @@ LL |         println!("{} -> {}", a_2, b[1]);
    |                              ~~~
 
 error: this binding can be a slice pattern to avoid indexing
-  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:44:38
+  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:45:38
    |
 LL |     if let (SomeEnum::Three(a), Some(b)) = (a_wrapped, b_wrapped) {
    |                                      ^
@@ -95,7 +95,7 @@ LL |         println!("{} -> {}", a[2], b_1);
    |                                    ~~~
 
 error: this binding can be a slice pattern to avoid indexing
-  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:53:21
+  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:54:21
    |
 LL |     if let Some(ref slice) = slice {
    |                     ^^^^^
@@ -110,7 +110,7 @@ LL |         println!("{:?}", slice_1);
    |                          ~~~~~~~
 
 error: this binding can be a slice pattern to avoid indexing
-  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:62:17
+  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:63:17
    |
 LL |     if let Some(slice) = &slice {
    |                 ^^^^^
@@ -125,7 +125,7 @@ LL |         println!("{:?}", slice_0);
    |                          ~~~~~~~
 
 error: this binding can be a slice pattern to avoid indexing
-  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:132:17
+  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:133:17
    |
 LL |     if let Some(slice) = wrap.inner {
    |                 ^^^^^
@@ -140,7 +140,7 @@ LL |             println!("This is awesome! {}", slice_0);
    |                                             ~~~~~~~
 
 error: this binding can be a slice pattern to avoid indexing
-  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:140:17
+  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:141:17
    |
 LL |     if let Some(slice) = wrap.inner {
    |                 ^^^^^
diff --git a/tests/ui/unnecessary_indexing.rs b/tests/ui/unnecessary_indexing.rs
@@ -1,5 +1,6 @@
 //@no-rustfix
 #![allow(unused)]
+#![allow(dropping_copy_types)]
 #![warn(clippy::unnecessary_indexing)]
 
 fn c(x: i32) -> i32 {
@@ -52,6 +53,30 @@ fn main() {
         let b = a[0];
     }
 
+    // lint when access is not first line
+    let a: &[i32] = &[1];
+    if !a.is_empty() {
+        dbg!(a);
+        let b = a[0];
+    }
+
+    // lint on multiple accesses/locals
+    let a: &[i32] = &[1];
+    if !a.is_empty() {
+        dbg!(a);
+        let b = a[0];
+        let c = a[0];
+        drop(a[0]);
+    }
+
+    // lint on multiple accesses
+    let a: &[i32] = &[1];
+    if !a.is_empty() {
+        dbg!(a);
+        drop(a[0]);
+        drop(a[0]);
+    }
+
     // dont lint when not accessing [0]
     let a: &[i32] = &[1, 2];
     if !a.is_empty() {
diff --git a/tests/ui/unnecessary_indexing.stderr b/tests/ui/unnecessary_indexing.stderr
