new lint: unnecessary_indexing

CHANGELOG.md

@@ -6888,6 +6888,7 @@ Released 2018-09-13
 [`unnecessary_first_then_check`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_first_then_check
 [`unnecessary_fold`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_fold
 [`unnecessary_get_then_check`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_get_then_check
+[`unnecessary_indexing`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_indexing
 [`unnecessary_join`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_join
 [`unnecessary_lazy_evaluations`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_lazy_evaluations
 [`unnecessary_literal_bound`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_literal_bound

clippy_lints/src/declared_lints.rs

@@ -751,6 +751,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::unit_types::UNIT_ARG_INFO,
     crate::unit_types::UNIT_CMP_INFO,
     crate::unnecessary_box_returns::UNNECESSARY_BOX_RETURNS_INFO,
+    crate::unnecessary_indexing::UNNECESSARY_INDEXING_INFO,
     crate::unnecessary_literal_bound::UNNECESSARY_LITERAL_BOUND_INFO,
     crate::unnecessary_map_on_constructor::UNNECESSARY_MAP_ON_CONSTRUCTOR_INFO,
     crate::unnecessary_mut_passed::UNNECESSARY_MUT_PASSED_INFO,

clippy_lints/src/lib.rs

@@ -372,6 +372,7 @@ mod uninit_vec;
 mod unit_return_expecting_ord;
 mod unit_types;
 mod unnecessary_box_returns;
+mod unnecessary_indexing;
 mod unnecessary_literal_bound;
 mod unnecessary_map_on_constructor;
 mod unnecessary_mut_passed;
@@ -834,5 +835,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
     store.register_late_pass(|_| Box::new(toplevel_ref_arg::ToplevelRefArg));
     store.register_late_pass(|_| Box::new(volatile_composites::VolatileComposites));
     store.register_late_pass(|_| Box::new(replace_box::ReplaceBox));
+    store.register_late_pass(|_| Box::new(unnecessary_indexing::UnnecessaryIndexing));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }

clippy_lints/src/unnecessary_indexing.rs

@@ -0,0 +1,188 @@
+use std::ops::ControlFlow;
+
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::res::{MaybeDef, MaybeResPath};
+use clippy_utils::source::snippet;
+use clippy_utils::visitors::for_each_expr;
+use rustc_ast::{LitKind, Mutability};
+use rustc_errors::Applicability;
+use rustc_hir::{Block, Expr, ExprKind, HirId, LetStmt, Node, UnOp};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::adjustment::{Adjust, AutoBorrow, AutoBorrowMutability};
+use rustc_session::declare_lint_pass;
+use rustc_span::{Span, sym};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for the use of `seq.is_empty()` in an if-conditional where `seq` is a slice, array, or Vec,
+    /// and in which the first element of the sequence is indexed.
+    ///
+    /// ### Why is this bad?
+    /// This code is unnecessarily complicated and can instead be simplified to the use of an
+    /// if..let expression which accesses the first element of the sequence.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// let a: &[i32] = &[1];
+    /// if !a.is_empty() {
+    ///    let b = a[0];
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// let a: &[i32] = &[1];
+    /// if let Some(b) = a.first() {
+    ///
+    /// }
+    /// ```
+    #[clippy::version = "1.86.0"]
+    pub UNNECESSARY_INDEXING,
+    complexity,
+    "unnecessary use of `seq.is_empty()` in a conditional when if..let is more appropriate"
+}
+
+declare_lint_pass!(UnnecessaryIndexing => [UNNECESSARY_INDEXING]);
+
+impl<'tcx> LateLintPass<'tcx> for UnnecessaryIndexing {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'_ Expr<'tcx>) {
+        if let Some(if_expr) = clippy_utils::higher::If::hir(expr)
+            && !if_expr.cond.span.from_expansion()
+            // check for negation
+            && let ExprKind::Unary(UnOp::Not, unary_inner) = if_expr.cond.kind
+            // check for call of is_empty
+            && let ExprKind::MethodCall(method, conditional_receiver, _, _) = unary_inner.kind
+            && method.ident.as_str() == "is_empty"
+            && let expr_ty = cx.typeck_results().expr_ty(conditional_receiver)
+            && let peeled = expr_ty.peel_refs()
+            && (peeled.is_slice() || peeled.is_array() || peeled.is_diag_item(cx, sym::Vec))
+            && let ExprKind::Block(block, _) = if_expr.then.kind
+            // do not lint if conditional receiver is mutable reference
+            && expr_ty.ref_mutability() != Some(Mutability::Mut)
+            && let Some(con_path) = conditional_receiver.res_local_id()
+            && let Some(r) = process_indexing(cx, block, con_path)
+        {
+            span_lint_and_then(
+                cx,
+                UNNECESSARY_INDEXING,
+                expr.span,
+                "condition can be simplified with `if..let` syntax",
+                |diag| {
+                    let receiver = snippet(cx, r.index_receiver_span, "..");
+                    let mut suggestions: Vec<(Span, String)> = vec![];
+                    let mut message = "consider using `if..let` syntax instead of indexing".to_string();
+                    if let Some(first_local) = r.first_local
+                        && let Some(name) = first_local.pat.simple_ident().map(|ident| ident.name)
+                    {
+                        suggestions.push((
+                            if_expr.cond.span,
+                            format!(
+                                "let Some({}{name}) = {receiver}.first()",
+                                // if we arent borrowing anything then we can pass a reference here for correctness
+                                if r.extra_exprs_borrow.is_empty() { "&" } else { "" },
+                            ),
+                        ));
+                        suggestions.push((first_local.span, String::new()));
+
+                        if !r.extra_exprs_borrow.is_empty() {
+                            suggestions.extend(
+                                r.extra_exprs_borrow
+                                    .iter()
+                                    .chain(r.extra_exprs_copy.iter())
+                                    .map(|x| (x.span, name.to_string())),
+                            );
+
+                            message.push_str(", and replacing indexing expression(s) with the value in `Some` variant");
+                        } else if !r.extra_exprs_copy.is_empty() {
+                            suggestions.extend(r.extra_exprs_copy.iter().map(|x| (x.span, name.to_string())));
+                        }
+                    } else if r.extra_exprs_borrow.is_empty() {
+                        suggestions.push((if_expr.cond.span, format!("let Some(&element) = {receiver}.first()")));
+                        suggestions.extend(r.extra_exprs_copy.iter().map(|x| (x.span, "element".to_owned())));
+                    } else {
+                        suggestions.push((if_expr.cond.span, format!("let Some(element) = {receiver}.first()")));
+                        suggestions.extend(
+                            r.extra_exprs_borrow
+                                .iter()
+                                .chain(r.extra_exprs_copy.iter())
+                                .map(|x| (x.span, "element".to_owned())),
+                        );
+                    }
+
+                    diag.multipart_suggestion(message, suggestions, Applicability::MaybeIncorrect);
+                },
+            );
+        }
+    }
+}
+
+struct IndexCheckResult<'a> {
+    // span of the receiver for the index operation, only Some in the event the indexing is via a direct primitive
+    index_receiver_span: Span,
+    // first local in the block - used as pattern for `Some(pat)`
+    first_local: Option<&'a LetStmt<'a>>,
+    // any other index expressions to replace with `pat` (or "element" if no local exists)
+    extra_exprs_borrow: Vec<&'a Expr<'a>>,
+    // copied extra index expressions: if we only have these and no borrows we can provide a correct suggestion of `let
+    // Some(&a) = ...`
+    extra_exprs_copy: Vec<&'a Expr<'a>>,
+}
+
+/// Checks the block for any indexing of the conditional receiver. Returns `None` if the block
+/// contains code that invalidates the lint, e.g., the receiver is accessed via a mutable reference.
+fn process_indexing<'a>(
+    cx: &LateContext<'a>,
+    block: &'a Block<'a>,
+    conditional_receiver_hid: HirId,
+) -> Option<IndexCheckResult<'a>> {
+    let mut index_receiver_span: Option<Span> = None;
+    let mut first_local: Option<&LetStmt<'_>> = None;
+    let mut extra_exprs_borrow: Vec<&Expr<'_>> = vec![];
+    let mut extra_exprs_copy: Vec<&Expr<'_>> = vec![];
+
+    // if res == Some(()), then mutation occurred
+    // & therefore we should not lint on this
+    let res = for_each_expr(cx, block, |x| {
+        let adjustments = cx.typeck_results().expr_adjustments(x);
+        if let ExprKind::Index(receiver, index, _) = x.kind
+            && let ExprKind::Lit(lit) = index.kind
+            && let LitKind::Int(val, _) = lit.node
+            && receiver.res_local_id() == Some(conditional_receiver_hid)
+            && val.0 == 0
+        {
+            index_receiver_span = Some(receiver.span);
+            if let Node::LetStmt(local) = cx.tcx.parent_hir_node(x.hir_id) {
+                if first_local.is_none() {
+                    first_local = Some(local);
+                    return ControlFlow::Continue::<()>(());
+                }
+            }
+
+            if let Node::Expr(x) = cx.tcx.parent_hir_node(x.hir_id)
+                && let ExprKind::AddrOf(_, _, _) = x.kind
+            {
+                extra_exprs_borrow.push(x);
+            } else {
+                extra_exprs_copy.push(x);
+            }
+        } else if adjustments.iter().any(|adjustment| {
+            matches!(
+                adjustment.kind,
+                Adjust::Borrow(AutoBorrow::Ref(AutoBorrowMutability::Mut { .. }))
+            )
+        }) {
+            // do not lint on mutable auto borrows (https://github.com/rust-lang/rust-clippy/pull/12464#discussion_r1600352696)
+            return ControlFlow::Break(());
+        } else if let ExprKind::AddrOf(_, Mutability::Mut, _) = x.kind {
+            return ControlFlow::Break(());
+        }
+
+        ControlFlow::Continue::<()>(())
+    });
+
+    res.is_none().then_some(IndexCheckResult {
+        index_receiver_span: index_receiver_span?,
+        first_local,
+        extra_exprs_borrow,
+        extra_exprs_copy,
+    })
+}

tests/ui/index_refutable_slice/if_let_slice_binding.fixed

@@ -1,5 +1,5 @@
 #![deny(clippy::index_refutable_slice)]
-#![allow(clippy::needless_lifetimes, clippy::collapsible_if)]
+#![allow(clippy::needless_lifetimes, clippy::collapsible_if, clippy::unnecessary_indexing)]
 
 enum SomeEnum<T> {
     One(T),

tests/ui/index_refutable_slice/if_let_slice_binding.rs

@@ -1,5 +1,5 @@
 #![deny(clippy::index_refutable_slice)]
-#![allow(clippy::needless_lifetimes, clippy::collapsible_if)]
+#![allow(clippy::needless_lifetimes, clippy::collapsible_if, clippy::unnecessary_indexing)]
 
 enum SomeEnum<T> {
     One(T),

tests/ui/unnecessary_indexing.fixed

@@ -0,0 +1,157 @@
+#![allow(unused)]
+#![allow(dropping_copy_types)]
+#![allow(dropping_references)]
+#![warn(clippy::unnecessary_indexing)]
+
+macro_rules! not_empty {
+    ($seq:ident) => {
+        !$seq.is_empty()
+    };
+}
+
+fn c(x: i32) -> i32 {
+    println!("{x}");
+    10
+}
+
+struct Struct;
+impl Struct {
+    pub fn a(x: i32) -> i32 {
+        println!("{x}");
+        10
+    }
+}
+
+fn main() {
+    // lint on vecs with a call
+    let a: Vec<i32> = vec![1];
+    if let Some(&element) = a.first() {
+        //~^ ERROR: condition can be simplified with `if..let` syntax
+        let b = c(element);
+    }
+
+    // lint on vecs with a method call
+    let a: Vec<i32> = vec![1];
+    if let Some(&element) = a.first() {
+        //~^ ERROR: condition can be simplified with `if..let` syntax
+        let b = Struct::a(element);
+    }
+
+    // lint on arrays with a call
+    let a: &[i32] = &[1];
+    if let Some(&element) = a.first() {
+        //~^ ERROR: condition can be simplified with `if..let` syntax
+        let b = c(element);
+    }
+
+    // lint on arrays with a method call
+    let a: &[i32] = &[1];
+    if let Some(&element) = a.first() {
+        //~^ ERROR: condition can be simplified with `if..let` syntax
+        let b = Struct::a(element);
+    }
+
+    // lint on vecs with a local access
+    let a: Vec<i32> = vec![1];
+    if let Some(&b) = a.first() {
+        //~^ ERROR: condition can be simplified with `if..let` syntax
+
+    }
+
+    // lint on arrays with a local access
+    let a: &[i32] = &[1];
+    if let Some(&b) = a.first() {
+        //~^ ERROR: condition can be simplified with `if..let` syntax
+
+    }
+
+    // lint when access is not first line
+    let a: &[i32] = &[1];
+    if let Some(&b) = a.first() {
+        //~^ ERROR: condition can be simplified with `if..let` syntax
+        dbg!(a);
+
+    }
+
+    // lint on multiple accesses/locals
+    let a: &[i32] = &[1];
+    if let Some(c) = a.first() {
+        //~^ ERROR: condition can be simplified with `if..let` syntax
+        dbg!(a);
+        let b = c;
+
+        drop(c);
+    }
+
+    // lint on multiple accesses
+    let a: &[i32] = &[1];
+    if let Some(&element) = a.first() {
+        //~^ ERROR: condition can be simplified with `if..let` syntax
+        dbg!(a);
+        drop(element);
+        drop(element);
+    }
+
+    let _first = if let Some(&element) = a.first() {
+        //~^ ERROR: condition can be simplified with `if..let` syntax
+        element
+    } else {
+        1
+    };
+
+    // don't lint when the condition is from expansion
+    if not_empty!(a) {
+        let b = a[0];
+    }
+
+    // dont lint when not accessing [0]
+    let a: &[i32] = &[1, 2];
+    if !a.is_empty() {
+        let b = a[1];
+    }
+
+    // dont lint when access is dynamic
+    const T: usize = 0;
+
+    let a: &[i32] = &[1];
+    if !a.is_empty() {
+        let b = a[T];
+    }
+
+    // dont lint without unary
+    let a: &[i32] = &[1];
+    if a.is_empty() {
+        let b = a[0];
+    }
+
+    // dont lint if we have mutable reference
+    let mut a: &[i32] = &[1];
+    if !a.is_empty() {
+        drop(&mut a);
+        let b = a[0];
+    }
+
+    // dont lint if we have a mutable reference, even if the mutable reference occurs after what we are
+    // linting against
+    let mut a: &[i32] = &[1];
+    if !a.is_empty() {
+        let b = a[0];
+        drop(&mut a);
+    }
+
+    // dont lint on mutable auto borrow
+    let mut a = vec![1, 2, 3];
+    if !a.is_empty() {
+        a.push(1);
+        let b = a[0];
+        b;
+    }
+
+    // do not lint if conditional receiver is mutable reference
+    let a = &mut vec![1, 2, 3];
+    if !a.is_empty() {
+        let b = a[0];
+        a;
+        b;
+    }
+}