diff --git a/CHANGELOG.md b/CHANGELOG.md index 37d46d349667..4ff6b9d38a46 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 375d179681da..7225a971582a 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/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, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 41702ae9fb97..be1a41926c32 100644 --- a/clippy_lints/src/lib.rs +++ b/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` } diff --git a/clippy_lints/src/unnecessary_indexing.rs b/clippy_lints/src/unnecessary_indexing.rs new file mode 100644 index 000000000000..8c06f3b68f36 --- /dev/null +++ b/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> { + let mut index_receiver_span: Option = 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, + }) +} diff --git a/tests/ui/explicit_write_in_test.stderr b/tests/ui/explicit_write_in_test.stderr deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/tests/ui/index_refutable_slice/if_let_slice_binding.fixed b/tests/ui/index_refutable_slice/if_let_slice_binding.fixed index dc7e09bbdc7d..cb704fc8d137 100644 --- 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,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 { 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 index f39ace101b45..c174aad8520c 100644 --- 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,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 { One(T), diff --git a/tests/ui/unnecessary_indexing.fixed b/tests/ui/unnecessary_indexing.fixed new file mode 100644 index 000000000000..58d136ebd987 --- /dev/null +++ b/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 = 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 = 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 = 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; + } +} diff --git a/tests/ui/unnecessary_indexing.rs b/tests/ui/unnecessary_indexing.rs new file mode 100644 index 000000000000..b4cf56f9d964 --- /dev/null +++ b/tests/ui/unnecessary_indexing.rs @@ -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 = vec![1]; + if !a.is_empty() { + //~^ ERROR: condition can be simplified with `if..let` syntax + let b = c(a[0]); + } + + // lint on vecs with a method call + let a: Vec = vec![1]; + if !a.is_empty() { + //~^ ERROR: condition can be simplified with `if..let` syntax + let b = Struct::a(a[0]); + } + + // lint on arrays with a call + let a: &[i32] = &[1]; + if !a.is_empty() { + //~^ ERROR: condition can be simplified with `if..let` syntax + let b = c(a[0]); + } + + // lint on arrays with a method call + let a: &[i32] = &[1]; + if !a.is_empty() { + //~^ ERROR: condition can be simplified with `if..let` syntax + let b = Struct::a(a[0]); + } + + // lint on vecs with a local access + let a: Vec = vec![1]; + if !a.is_empty() { + //~^ ERROR: condition can be simplified with `if..let` syntax + let b = a[0]; + } + + // lint on arrays with a local access + let a: &[i32] = &[1]; + if !a.is_empty() { + //~^ ERROR: condition can be simplified with `if..let` syntax + let b = a[0]; + } + + // lint when access is not first line + let a: &[i32] = &[1]; + if !a.is_empty() { + //~^ ERROR: condition can be simplified with `if..let` syntax + dbg!(a); + let b = a[0]; + } + + // lint on multiple accesses/locals + let a: &[i32] = &[1]; + if !a.is_empty() { + //~^ ERROR: condition can be simplified with `if..let` syntax + 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() { + //~^ ERROR: condition can be simplified with `if..let` syntax + dbg!(a); + drop(a[0]); + drop(a[0]); + } + + let _first = if !a.is_empty() { + //~^ ERROR: condition can be simplified with `if..let` syntax + a[0] + } 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; + } +} diff --git a/tests/ui/unnecessary_indexing.stderr b/tests/ui/unnecessary_indexing.stderr new file mode 100644 index 000000000000..aba4067a3ef2 --- /dev/null +++ b/tests/ui/unnecessary_indexing.stderr @@ -0,0 +1,179 @@ +error: condition can be simplified with `if..let` syntax + --> tests/ui/unnecessary_indexing.rs:28:5 + | +LL | / if !a.is_empty() { +LL | | +LL | | let b = c(a[0]); +LL | | } + | |_____^ + | + = note: `-D clippy::unnecessary-indexing` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unnecessary_indexing)]` +help: consider using `if..let` syntax instead of indexing + | +LL ~ if let Some(&element) = a.first() { +LL | +LL ~ let b = c(element); + | + +error: condition can be simplified with `if..let` syntax + --> tests/ui/unnecessary_indexing.rs:35:5 + | +LL | / if !a.is_empty() { +LL | | +LL | | let b = Struct::a(a[0]); +LL | | } + | |_____^ + | +help: consider using `if..let` syntax instead of indexing + | +LL ~ if let Some(&element) = a.first() { +LL | +LL ~ let b = Struct::a(element); + | + +error: condition can be simplified with `if..let` syntax + --> tests/ui/unnecessary_indexing.rs:42:5 + | +LL | / if !a.is_empty() { +LL | | +LL | | let b = c(a[0]); +LL | | } + | |_____^ + | +help: consider using `if..let` syntax instead of indexing + | +LL ~ if let Some(&element) = a.first() { +LL | +LL ~ let b = c(element); + | + +error: condition can be simplified with `if..let` syntax + --> tests/ui/unnecessary_indexing.rs:49:5 + | +LL | / if !a.is_empty() { +LL | | +LL | | let b = Struct::a(a[0]); +LL | | } + | |_____^ + | +help: consider using `if..let` syntax instead of indexing + | +LL ~ if let Some(&element) = a.first() { +LL | +LL ~ let b = Struct::a(element); + | + +error: condition can be simplified with `if..let` syntax + --> tests/ui/unnecessary_indexing.rs:56:5 + | +LL | / if !a.is_empty() { +LL | | +LL | | let b = a[0]; +LL | | } + | |_____^ + | +help: consider using `if..let` syntax instead of indexing + | +LL ~ if let Some(&b) = a.first() { +LL | +LL ~ + | + +error: condition can be simplified with `if..let` syntax + --> tests/ui/unnecessary_indexing.rs:63:5 + | +LL | / if !a.is_empty() { +LL | | +LL | | let b = a[0]; +LL | | } + | |_____^ + | +help: consider using `if..let` syntax instead of indexing + | +LL ~ if let Some(&b) = a.first() { +LL | +LL ~ + | + +error: condition can be simplified with `if..let` syntax + --> tests/ui/unnecessary_indexing.rs:70:5 + | +LL | / if !a.is_empty() { +LL | | +LL | | dbg!(a); +LL | | let b = a[0]; +LL | | } + | |_____^ + | +help: consider using `if..let` syntax instead of indexing + | +LL ~ if let Some(&b) = a.first() { +LL | +LL | dbg!(a); +LL ~ + | + +error: condition can be simplified with `if..let` syntax + --> tests/ui/unnecessary_indexing.rs:78:5 + | +LL | / if !a.is_empty() { +LL | | +LL | | dbg!(a); +LL | | let b = &a[0]; +LL | | let c = a[0]; +LL | | drop(a[0]); +LL | | } + | |_____^ + | +help: consider using `if..let` syntax instead of indexing, and replacing indexing expression(s) with the value in `Some` variant + | +LL ~ if let Some(c) = a.first() { +LL | +LL | dbg!(a); +LL ~ let b = c; +LL ~ +LL ~ drop(c); + | + +error: condition can be simplified with `if..let` syntax + --> tests/ui/unnecessary_indexing.rs:88:5 + | +LL | / if !a.is_empty() { +LL | | +LL | | dbg!(a); +LL | | drop(a[0]); +LL | | drop(a[0]); +LL | | } + | |_____^ + | +help: consider using `if..let` syntax instead of indexing + | +LL ~ if let Some(&element) = a.first() { +LL | +LL | dbg!(a); +LL ~ drop(element); +LL ~ drop(element); + | + +error: condition can be simplified with `if..let` syntax + --> tests/ui/unnecessary_indexing.rs:95:18 + | +LL | let _first = if !a.is_empty() { + | __________________^ +LL | | +LL | | a[0] +LL | | } else { +LL | | 1 +LL | | }; + | |_____^ + | +help: consider using `if..let` syntax instead of indexing + | +LL ~ let _first = if let Some(&element) = a.first() { +LL | +LL ~ element + | + +error: aborting due to 10 previous errors +