From d3e174a2e2d51448d7b2ce15668ece1dfc381a45 Mon Sep 17 00:00:00 2001 From: Jacherr Date: Mon, 11 Mar 2024 22:15:19 +0000 Subject: [PATCH 1/8] new lint: `unnecessary_indexing` --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/manual_strip.rs | 6 +- clippy_lints/src/unnecessary_indexing.rs | 161 +++++++++++++++++ .../if_let_slice_binding.fixed | 1 + .../if_let_slice_binding.rs | 1 + .../if_let_slice_binding.stderr | 20 +-- tests/ui/unnecessary_indexing.rs | 99 +++++++++++ tests/ui/unnecessary_indexing.stderr | 162 ++++++++++++++++++ 10 files changed, 441 insertions(+), 13 deletions(-) create mode 100644 clippy_lints/src/unnecessary_indexing.rs create mode 100644 tests/ui/unnecessary_indexing.rs create mode 100644 tests/ui/unnecessary_indexing.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index fe9fbd486c9c..23cd7521634b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5913,6 +5913,7 @@ Released 2018-09-13 [`unnecessary_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_find_map [`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_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_literal_unwrap diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 3b3d0db79dcc..9f3e21f7b250 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -728,6 +728,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::unit_types::UNIT_CMP_INFO, crate::unnamed_address::FN_ADDRESS_COMPARISONS_INFO, crate::unnecessary_box_returns::UNNECESSARY_BOX_RETURNS_INFO, + crate::unnecessary_indexing::UNNECESSARY_INDEXING_INFO, crate::unnecessary_map_on_constructor::UNNECESSARY_MAP_ON_CONSTRUCTOR_INFO, crate::unnecessary_owned_empty_strings::UNNECESSARY_OWNED_EMPTY_STRINGS_INFO, crate::unnecessary_self_imports::UNNECESSARY_SELF_IMPORTS_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 1bd6dfe24617..c5336e763488 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -355,6 +355,7 @@ mod unit_return_expecting_ord; mod unit_types; mod unnamed_address; mod unnecessary_box_returns; +mod unnecessary_indexing; mod unnecessary_map_on_constructor; mod unnecessary_owned_empty_strings; mod unnecessary_self_imports; @@ -1171,6 +1172,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { }); store.register_late_pass(move |_| Box::new(string_patterns::StringPatterns::new(msrv()))); store.register_early_pass(|| Box::new(field_scoped_visibility_modifiers::FieldScopedVisibilityModifiers)); + 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/manual_strip.rs b/clippy_lints/src/manual_strip.rs index 45af9f07718d..84ef3dfd4927 100644 --- 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,8 +105,8 @@ impl<'tcx> LateLintPass<'tcx> for ManualStrip { span_lint_and_then( cx, MANUAL_STRIP, - strippings[0], - format!("stripping a {kind_word} manually"), + *first_stripping, + &format!("stripping a {kind_word} manually"), |diag| { diag.span_note(test_span, format!("the {kind_word} was tested here")); multispan_sugg( diff --git a/clippy_lints/src/unnecessary_indexing.rs b/clippy_lints/src/unnecessary_indexing.rs new file mode 100644 index 000000000000..baff3f55cc82 --- /dev/null +++ b/clippy_lints/src/unnecessary_indexing.rs @@ -0,0 +1,161 @@ +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::{Expr, ExprKind, Local, Node, UnOp}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::declare_lint_pass; +use rustc_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 accessed 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.78.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 LateLintPass<'_> for UnnecessaryIndexing { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ rustc_hir::Expr<'_>) { + if let Some(if_expr) = clippy_utils::higher::If::hir(expr) + // check for negation + && let ExprKind::Unary(op, unary_inner) = if_expr.cond.kind + && UnOp::Not == op + // 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).peel_refs() + && (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 + { + // 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![]; + + 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); + }; + }; + + ControlFlow::Continue::<()>(()) + }); + + 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::>(); + + 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::>(); + + 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 index 13f0cbe9cc88..7e2cdac04cca 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,6 @@ #![deny(clippy::index_refutable_slice)] #![allow(clippy::uninlined_format_args)] +#![allow(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 d8d38c167fa5..9eded7329599 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,6 @@ #![deny(clippy::index_refutable_slice)] #![allow(clippy::uninlined_format_args)] +#![allow(clippy::unnecessary_indexing)] enum SomeEnum { 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 index 14e0f931f240..593be58d3f10 100644 --- 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 new file mode 100644 index 000000000000..bc7d5c369327 --- /dev/null +++ b/tests/ui/unnecessary_indexing.rs @@ -0,0 +1,99 @@ +//@no-rustfix +#![allow(unused)] +#![allow(dropping_copy_types)] +#![warn(clippy::unnecessary_indexing)] + +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() { + let b = c(a[0]); + } + + // lint on vecs with a method call + let a: Vec = vec![1]; + if !a.is_empty() { + let b = Struct::a(a[0]); + } + + // lint on arrays with a call + let a: &[i32] = &[1]; + if !a.is_empty() { + let b = c(a[0]); + } + + // lint on arrays with a method call + let a: &[i32] = &[1]; + if !a.is_empty() { + let b = Struct::a(a[0]); + } + + // lint on vecs with a local access + let a: Vec = vec![1]; + if !a.is_empty() { + let b = a[0]; + } + + // lint on arrays with a local access + let a: &[i32] = &[1]; + if !a.is_empty() { + 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() { + 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]; + } +} diff --git a/tests/ui/unnecessary_indexing.stderr b/tests/ui/unnecessary_indexing.stderr new file mode 100644 index 000000000000..0397873552c1 --- /dev/null +++ b/tests/ui/unnecessary_indexing.stderr @@ -0,0 +1,162 @@ +error: condition can be simplified with if..let syntax + --> tests/ui/unnecessary_indexing.rs:22:5 + | +LL | / if !a.is_empty() { +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 (variable may need to be dereferenced) + | +LL ~ if let Some(element) = a.first() { +LL ~ let b = c(element); + | + +error: condition can be simplified with if..let syntax + --> tests/ui/unnecessary_indexing.rs:28:5 + | +LL | / if !a.is_empty() { +LL | | let b = Struct::a(a[0]); +LL | | } + | |_____^ + | +help: consider using if..let syntax (variable may need to be dereferenced) + | +LL ~ if let Some(element) = a.first() { +LL ~ let b = Struct::a(element); + | + +error: condition can be simplified with if..let syntax + --> tests/ui/unnecessary_indexing.rs:34:5 + | +LL | / if !a.is_empty() { +LL | | let b = c(a[0]); +LL | | } + | |_____^ + | +help: consider using if..let syntax (variable may need to be dereferenced) + | +LL ~ if let Some(element) = a.first() { +LL ~ let b = c(element); + | + +error: condition can be simplified with if..let syntax + --> tests/ui/unnecessary_indexing.rs:40:5 + | +LL | / if !a.is_empty() { +LL | | let b = Struct::a(a[0]); +LL | | } + | |_____^ + | +help: consider using if..let syntax (variable may need to be dereferenced) + | +LL ~ if let Some(element) = a.first() { +LL ~ let b = Struct::a(element); + | + +error: condition can be simplified with if..let syntax + --> tests/ui/unnecessary_indexing.rs:46:5 + | +LL | / if !a.is_empty() { +LL | | let b = a[0]; +LL | | } + | |_____^ + | +help: consider using if..let syntax (variable may need to be dereferenced) + | +LL | if let Some(b) = a.first() { + | ~~~~~~~~~~~~~~~~~~~~~~~ +help: remove this line + | +LL - let b = a[0]; +LL + + | + +error: condition can be simplified with if..let syntax + --> tests/ui/unnecessary_indexing.rs:52:5 + | +LL | / if !a.is_empty() { +LL | | let b = a[0]; +LL | | } + | |_____^ + | +help: consider using if..let syntax (variable may need to be dereferenced) + | +LL | if let Some(b) = a.first() { + | ~~~~~~~~~~~~~~~~~~~~~~~ +help: remove this line + | +LL - let b = a[0]; +LL + + | + +error: condition can be simplified with if..let syntax + --> tests/ui/unnecessary_indexing.rs:58:5 + | +LL | / if !a.is_empty() { +LL | | dbg!(a); +LL | | let b = a[0]; +LL | | } + | |_____^ + | +help: consider using if..let syntax (variable may need to be dereferenced) + | +LL | if let Some(b) = a.first() { + | ~~~~~~~~~~~~~~~~~~~~~~~ +help: remove this line + | +LL - let b = a[0]; +LL + + | + +error: condition can be simplified with if..let syntax + --> tests/ui/unnecessary_indexing.rs:65:5 + | +LL | / if !a.is_empty() { +LL | | dbg!(a); +LL | | let b = a[0]; +LL | | let c = a[0]; +LL | | drop(a[0]); +LL | | } + | |_____^ + | +help: consider using if..let syntax (variable may need to be dereferenced) + | +LL | if let Some(b) = a.first() { + | ~~~~~~~~~~~~~~~~~~~~~~~ +help: remove this line + | +LL - let b = a[0]; +LL + + | +help: initialize this variable to be the `Some` variant (may need dereferencing) + | +LL | let c = b; + | ~ +help: set this index to be the `Some` variant (may need dereferencing) + | +LL | drop(b); + | ~ + +error: condition can be simplified with if..let syntax + --> tests/ui/unnecessary_indexing.rs:74:5 + | +LL | / if !a.is_empty() { +LL | | dbg!(a); +LL | | drop(a[0]); +LL | | drop(a[0]); +LL | | } + | |_____^ + | +help: consider using if..let syntax (variable may need to be dereferenced) + | +LL ~ if let Some(element) = a.first() { +LL | dbg!(a); +LL ~ drop(element); +LL ~ drop(element); + | + +error: aborting due to 9 previous errors + From bc4ce993fc61ec51a4095f6df46b4ddcf21a5026 Mon Sep 17 00:00:00 2001 From: Jacherr Date: Thu, 14 Mar 2024 17:39:30 +0000 Subject: [PATCH 2/8] do not lint after mutable reference is taken --- clippy_lints/src/unnecessary_indexing.rs | 16 +++++++--------- tests/ui/unnecessary_indexing.rs | 8 ++++++++ tests/ui/unnecessary_indexing.stderr | 18 +++++++++--------- 3 files changed, 24 insertions(+), 18 deletions(-) diff --git a/clippy_lints/src/unnecessary_indexing.rs b/clippy_lints/src/unnecessary_indexing.rs index baff3f55cc82..f2c82caa3179 100644 --- a/clippy_lints/src/unnecessary_indexing.rs +++ b/clippy_lints/src/unnecessary_indexing.rs @@ -5,7 +5,7 @@ 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_ast::{BorrowKind, LitKind, Mutability}; use rustc_errors::Applicability; use rustc_hir::{Expr, ExprKind, Local, Node, UnOp}; use rustc_lint::{LateContext, LateLintPass}; @@ -47,8 +47,7 @@ impl LateLintPass<'_> for UnnecessaryIndexing { fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ rustc_hir::Expr<'_>) { if let Some(if_expr) = clippy_utils::higher::If::hir(expr) // check for negation - && let ExprKind::Unary(op, unary_inner) = if_expr.cond.kind - && UnOp::Not == op + && 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" @@ -82,6 +81,10 @@ impl LateLintPass<'_> for UnnecessaryIndexing { } else { extra_exprs.push(x); }; + } else if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, val) = x.kind + && eq_expr_value(cx, conditional_receiver, val) + { + return ControlFlow::Break(()); }; ControlFlow::Continue::<()>(()) @@ -105,12 +108,7 @@ impl LateLintPass<'_> for UnnecessaryIndexing { ), Applicability::Unspecified, ); - x.span_suggestion( - first_local.span, - "remove this line", - "", - Applicability::MachineApplicable, - ); + x.span_suggestion(first_local.span, "remove this line", "", Applicability::Unspecified); if !extra_locals.is_empty() { let extra_local_suggestions = extra_locals .iter() diff --git a/tests/ui/unnecessary_indexing.rs b/tests/ui/unnecessary_indexing.rs index bc7d5c369327..116e8663bbca 100644 --- a/tests/ui/unnecessary_indexing.rs +++ b/tests/ui/unnecessary_indexing.rs @@ -1,6 +1,7 @@ //@no-rustfix #![allow(unused)] #![allow(dropping_copy_types)] +#![allow(dropping_references)] #![warn(clippy::unnecessary_indexing)] fn c(x: i32) -> i32 { @@ -96,4 +97,11 @@ fn main() { 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]; + } } diff --git a/tests/ui/unnecessary_indexing.stderr b/tests/ui/unnecessary_indexing.stderr index 0397873552c1..d346d8e81b0f 100644 --- a/tests/ui/unnecessary_indexing.stderr +++ b/tests/ui/unnecessary_indexing.stderr @@ -1,5 +1,5 @@ error: condition can be simplified with if..let syntax - --> tests/ui/unnecessary_indexing.rs:22:5 + --> tests/ui/unnecessary_indexing.rs:23:5 | LL | / if !a.is_empty() { LL | | let b = c(a[0]); @@ -15,7 +15,7 @@ LL ~ let b = c(element); | error: condition can be simplified with if..let syntax - --> tests/ui/unnecessary_indexing.rs:28:5 + --> tests/ui/unnecessary_indexing.rs:29:5 | LL | / if !a.is_empty() { LL | | let b = Struct::a(a[0]); @@ -29,7 +29,7 @@ LL ~ let b = Struct::a(element); | error: condition can be simplified with if..let syntax - --> tests/ui/unnecessary_indexing.rs:34:5 + --> tests/ui/unnecessary_indexing.rs:35:5 | LL | / if !a.is_empty() { LL | | let b = c(a[0]); @@ -43,7 +43,7 @@ LL ~ let b = c(element); | error: condition can be simplified with if..let syntax - --> tests/ui/unnecessary_indexing.rs:40:5 + --> tests/ui/unnecessary_indexing.rs:41:5 | LL | / if !a.is_empty() { LL | | let b = Struct::a(a[0]); @@ -57,7 +57,7 @@ LL ~ let b = Struct::a(element); | error: condition can be simplified with if..let syntax - --> tests/ui/unnecessary_indexing.rs:46:5 + --> tests/ui/unnecessary_indexing.rs:47:5 | LL | / if !a.is_empty() { LL | | let b = a[0]; @@ -75,7 +75,7 @@ LL + | error: condition can be simplified with if..let syntax - --> tests/ui/unnecessary_indexing.rs:52:5 + --> tests/ui/unnecessary_indexing.rs:53:5 | LL | / if !a.is_empty() { LL | | let b = a[0]; @@ -93,7 +93,7 @@ LL + | error: condition can be simplified with if..let syntax - --> tests/ui/unnecessary_indexing.rs:58:5 + --> tests/ui/unnecessary_indexing.rs:59:5 | LL | / if !a.is_empty() { LL | | dbg!(a); @@ -112,7 +112,7 @@ LL + | error: condition can be simplified with if..let syntax - --> tests/ui/unnecessary_indexing.rs:65:5 + --> tests/ui/unnecessary_indexing.rs:66:5 | LL | / if !a.is_empty() { LL | | dbg!(a); @@ -141,7 +141,7 @@ LL | drop(b); | ~ error: condition can be simplified with if..let syntax - --> tests/ui/unnecessary_indexing.rs:74:5 + --> tests/ui/unnecessary_indexing.rs:75:5 | LL | / if !a.is_empty() { LL | | dbg!(a); From cfa97d53fd08eb6ea8ff2587646ed4615bec54e2 Mon Sep 17 00:00:00 2001 From: Jacher Date: Wed, 29 May 2024 19:24:42 +0000 Subject: [PATCH 3/8] check path to local on conditional receiver, check mutable borrow after ref, do not lint on mutable auto borrow --- clippy_lints/src/manual_strip.rs | 2 +- clippy_lints/src/unnecessary_indexing.rs | 163 +++++++++++++++-------- tests/ui/unnecessary_indexing.rs | 8 ++ 3 files changed, 119 insertions(+), 54 deletions(-) diff --git a/clippy_lints/src/manual_strip.rs b/clippy_lints/src/manual_strip.rs index 84ef3dfd4927..0cf6908dbb4e 100644 --- a/clippy_lints/src/manual_strip.rs +++ b/clippy_lints/src/manual_strip.rs @@ -106,7 +106,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualStrip { cx, MANUAL_STRIP, *first_stripping, - &format!("stripping a {kind_word} manually"), + format!("stripping a {kind_word} manually"), |diag| { diag.span_note(test_span, format!("the {kind_word} was tested here")); multispan_sugg( diff --git a/clippy_lints/src/unnecessary_indexing.rs b/clippy_lints/src/unnecessary_indexing.rs index f2c82caa3179..fbdc0a45a7b0 100644 --- a/clippy_lints/src/unnecessary_indexing.rs +++ b/clippy_lints/src/unnecessary_indexing.rs @@ -1,14 +1,15 @@ 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 clippy_utils::{eq_expr_value, path_to_local, path_to_local_id}; use rustc_ast::{BorrowKind, LitKind, Mutability}; use rustc_errors::Applicability; -use rustc_hir::{Expr, ExprKind, Local, Node, UnOp}; +use rustc_hir::{Block, Expr, ExprKind, 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::sym; @@ -19,7 +20,7 @@ declare_clippy_lint! { /// /// ### Why is this bad? /// This code is unnecessarily complicated and can instead be simplified to the use of an - /// if..let expression which accessed the first element of the sequence. + /// if..let expression which accesses the first element of the sequence. /// /// ### Example /// ```no_run @@ -44,61 +45,40 @@ declare_clippy_lint! { declare_lint_pass!(UnnecessaryIndexing => [UNNECESSARY_INDEXING]); impl LateLintPass<'_> for UnnecessaryIndexing { - fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ rustc_hir::Expr<'_>) { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ Expr<'_>) { if let Some(if_expr) = clippy_utils::higher::If::hir(expr) // 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).peel_refs() - && (expr_ty.is_array_slice() || expr_ty.is_array() || is_type_diagnostic_item(cx, expr_ty, sym::Vec)) + && let typeck_results = cx.typeck_results() + // do not lint on mutable auto borrows (https://github.com/rust-lang/rust-clippy/pull/12464#discussion_r1600352696) + && let adjustments = typeck_results.expr_adjustments(conditional_receiver) + && !adjustments.iter().any(|adjustment| { + matches!(adjustment.kind, Adjust::Borrow(AutoBorrow::Ref(_, AutoBorrowMutability::Mut { + allow_two_phase_borrow: _ + }))) + }) + // do not lint if receiver is a mutable reference + && let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, _) = conditional_receiver.kind + && let expr_ty = typeck_results.expr_ty(conditional_receiver).peel_refs() + && (expr_ty.is_slice() || expr_ty.is_array() || is_type_diagnostic_item(cx, expr_ty, sym::Vec)) && let ExprKind::Block(block, _) = if_expr.then.kind { - // 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![]; + let result = process_indexing(cx, block, conditional_receiver); - 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); - }; - } else if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, val) = x.kind - && eq_expr_value(cx, conditional_receiver, val) - { - return ControlFlow::Break(()); - }; - - ControlFlow::Continue::<()>(()) - }); - - if let Some(receiver) = index_receiver { + if let Some(r) = result + && let Some(receiver) = r.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( + |diag| { + if let Some(first_local) = r.first_local { + diag.span_suggestion( if_expr.cond.span, "consider using if..let syntax (variable may need to be dereferenced)", format!( @@ -108,9 +88,10 @@ impl LateLintPass<'_> for UnnecessaryIndexing { ), Applicability::Unspecified, ); - x.span_suggestion(first_local.span, "remove this line", "", Applicability::Unspecified); - if !extra_locals.is_empty() { - let extra_local_suggestions = extra_locals + diag.span_suggestion(first_local.span, "remove this line", "", Applicability::Unspecified); + if !r.extra_locals.is_empty() { + let extra_local_suggestions = r + .extra_locals .iter() .map(|x| { ( @@ -120,19 +101,20 @@ impl LateLintPass<'_> for UnnecessaryIndexing { }) .collect::>(); - x.multipart_suggestion( + diag.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 + if !r.extra_exprs.is_empty() { + let index_accesses = r + .extra_exprs .iter() .map(|x| (x.span, snippet(cx, first_local.pat.span, "..").to_string())) .collect::>(); - x.multipart_suggestion( + diag.multipart_suggestion( "set this index to be the `Some` variant (may need dereferencing)", index_accesses, Applicability::Unspecified, @@ -143,9 +125,9 @@ impl LateLintPass<'_> for UnnecessaryIndexing { 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()))); + index_accesses.extend(r.extra_exprs.iter().map(|x| (x.span, "element".to_owned()))); - x.multipart_suggestion( + diag.multipart_suggestion( "consider using if..let syntax (variable may need to be dereferenced)", index_accesses, Applicability::Unspecified, @@ -157,3 +139,78 @@ impl LateLintPass<'_> for UnnecessaryIndexing { } } } + +struct IndexCheckResult<'a> { + // the receiver for the index operation + pub index_receiver: Option<&'a Expr<'a>>, + // first local in the block - used as pattern for `Some(pat)` + pub first_local: Option<&'a LetStmt<'a>>, + // any other locals to be aware of, these are set to the value of `pat` + pub extra_locals: Vec<&'a LetStmt<'a>>, + // any other index expressions to replace with `pat` (or "element" if no local exists) + pub extra_exprs: Vec<&'a Expr<'a>>, +} +impl<'a> IndexCheckResult<'a> { + pub fn new() -> Self { + IndexCheckResult { + index_receiver: None, + first_local: None, + extra_locals: vec![], + extra_exprs: vec![], + } + } +} + +/// 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: &'a LateContext<'_>, + block: &'a Block<'_>, + conditional_receiver: &'a Expr<'_>, +) -> Option> { + let mut result = IndexCheckResult::new(); + + let mut index_receiver: Option<&Expr<'_>> = None; + let mut first_local: Option<&LetStmt<'_>> = None; + let mut extra_locals: Vec<&LetStmt<'_>> = vec![]; + let mut extra_exprs: Vec<&Expr<'_>> = vec![]; + + // if res == Some(()), then mutation occurred + // & therefore we should not lint on this + let res = 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 + && let Some(con_path) = path_to_local(conditional_receiver) + && path_to_local_id(receiver, con_path) + && val.0 == 0 + { + index_receiver = Some(receiver); + if let Node::LetStmt(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); + }; + } else if let ExprKind::AddrOf(_, Mutability::Mut, val) = x.kind + && eq_expr_value(cx, conditional_receiver, val) + { + return ControlFlow::Break(()); + }; + + ControlFlow::Continue::<()>(()) + }); + + if res.is_none() { + result.extra_exprs = extra_exprs; + result.extra_locals = extra_locals; + result.first_local = first_local; + result.index_receiver = index_receiver; + Some(result) + } else { + None + } +} diff --git a/tests/ui/unnecessary_indexing.rs b/tests/ui/unnecessary_indexing.rs index 116e8663bbca..bdd6ab1bace5 100644 --- a/tests/ui/unnecessary_indexing.rs +++ b/tests/ui/unnecessary_indexing.rs @@ -104,4 +104,12 @@ fn main() { 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); + } } From e8efc1aa2d74402557ffbd8802a374f8cee0db6f Mon Sep 17 00:00:00 2001 From: Jacher Date: Thu, 30 May 2024 11:31:55 +0000 Subject: [PATCH 4/8] fix autoborrow/mutability checks --- clippy_lints/src/unnecessary_indexing.rs | 32 +++++++++++++++--------- tests/ui/unnecessary_indexing.rs | 16 ++++++++++++ 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/unnecessary_indexing.rs b/clippy_lints/src/unnecessary_indexing.rs index fbdc0a45a7b0..d73073d07063 100644 --- a/clippy_lints/src/unnecessary_indexing.rs +++ b/clippy_lints/src/unnecessary_indexing.rs @@ -5,11 +5,12 @@ use clippy_utils::source::snippet; use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::visitors::for_each_expr; use clippy_utils::{eq_expr_value, path_to_local, path_to_local_id}; -use rustc_ast::{BorrowKind, LitKind, Mutability}; +use rustc_ast::{LitKind, Mutability}; use rustc_errors::Applicability; use rustc_hir::{Block, Expr, ExprKind, LetStmt, Node, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::adjustment::{Adjust, AutoBorrow, AutoBorrowMutability}; +use rustc_middle::ty::{self}; use rustc_session::declare_lint_pass; use rustc_span::sym; @@ -53,19 +54,16 @@ impl LateLintPass<'_> for UnnecessaryIndexing { && let ExprKind::MethodCall(method, conditional_receiver, _, _) = unary_inner.kind && method.ident.as_str() == "is_empty" && let typeck_results = cx.typeck_results() - // do not lint on mutable auto borrows (https://github.com/rust-lang/rust-clippy/pull/12464#discussion_r1600352696) - && let adjustments = typeck_results.expr_adjustments(conditional_receiver) - && !adjustments.iter().any(|adjustment| { - matches!(adjustment.kind, Adjust::Borrow(AutoBorrow::Ref(_, AutoBorrowMutability::Mut { - allow_two_phase_borrow: _ - }))) - }) - // do not lint if receiver is a mutable reference - && let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, _) = conditional_receiver.kind - && let expr_ty = typeck_results.expr_ty(conditional_receiver).peel_refs() - && (expr_ty.is_slice() || expr_ty.is_array() || is_type_diagnostic_item(cx, expr_ty, sym::Vec)) + && let expr_ty = typeck_results.expr_ty(conditional_receiver) + && let peeled = expr_ty.peel_refs() + && (peeled.is_slice() || peeled.is_array() || is_type_diagnostic_item(cx, peeled, sym::Vec)) && let ExprKind::Block(block, _) = if_expr.then.kind { + // do not lint if conditional receiver is mutable reference + if let ty::Ref(_, _, Mutability::Mut) = expr_ty.kind() { + return; + } + let result = process_indexing(cx, block, conditional_receiver); if let Some(r) = result @@ -178,6 +176,8 @@ fn process_indexing<'a>( // if res == Some(()), then mutation occurred // & therefore we should not lint on this let res = for_each_expr(block.stmts, |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 @@ -195,6 +195,14 @@ fn process_indexing<'a>( } else { extra_exprs.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, val) = x.kind && eq_expr_value(cx, conditional_receiver, val) { diff --git a/tests/ui/unnecessary_indexing.rs b/tests/ui/unnecessary_indexing.rs index bdd6ab1bace5..ad082e7d8d88 100644 --- a/tests/ui/unnecessary_indexing.rs +++ b/tests/ui/unnecessary_indexing.rs @@ -112,4 +112,20 @@ fn main() { 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; + } } From b54aaa30d858c2c640ee1d950a1cfaefe66eed64 Mon Sep 17 00:00:00 2001 From: Jacher Date: Wed, 26 Jun 2024 23:38:36 +0000 Subject: [PATCH 5/8] remove unneded `extra_locals` --- clippy_lints/src/unnecessary_indexing.rs | 55 ++++++++++-------------- tests/ui/unnecessary_indexing.stderr | 9 ++-- 2 files changed, 26 insertions(+), 38 deletions(-) diff --git a/clippy_lints/src/unnecessary_indexing.rs b/clippy_lints/src/unnecessary_indexing.rs index d73073d07063..50a797acd15f 100644 --- a/clippy_lints/src/unnecessary_indexing.rs +++ b/clippy_lints/src/unnecessary_indexing.rs @@ -10,7 +10,6 @@ use rustc_errors::Applicability; use rustc_hir::{Block, Expr, ExprKind, LetStmt, Node, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::adjustment::{Adjust, AutoBorrow, AutoBorrowMutability}; -use rustc_middle::ty::{self}; use rustc_session::declare_lint_pass; use rustc_span::sym; @@ -53,20 +52,14 @@ impl LateLintPass<'_> for UnnecessaryIndexing { // check for call of is_empty && let ExprKind::MethodCall(method, conditional_receiver, _, _) = unary_inner.kind && method.ident.as_str() == "is_empty" - && let typeck_results = cx.typeck_results() - && let expr_ty = typeck_results.expr_ty(conditional_receiver) + && let expr_ty = cx.typeck_results().expr_ty(conditional_receiver) && let peeled = expr_ty.peel_refs() && (peeled.is_slice() || peeled.is_array() || is_type_diagnostic_item(cx, peeled, sym::Vec)) && let ExprKind::Block(block, _) = if_expr.then.kind - { // do not lint if conditional receiver is mutable reference - if let ty::Ref(_, _, Mutability::Mut) = expr_ty.kind() { - return; - } - - let result = process_indexing(cx, block, conditional_receiver); - - if let Some(r) = result + && expr_ty.ref_mutability() != Some(Mutability::Mut) + { + if let Some(r) = process_indexing(cx, block, conditional_receiver) && let Some(receiver) = r.index_receiver { span_lint_and_then( @@ -87,23 +80,26 @@ impl LateLintPass<'_> for UnnecessaryIndexing { Applicability::Unspecified, ); diag.span_suggestion(first_local.span, "remove this line", "", Applicability::Unspecified); - if !r.extra_locals.is_empty() { + if !r.extra_exprs.is_empty() { let extra_local_suggestions = r - .extra_locals + .extra_exprs .iter() - .map(|x| { - ( - x.init.unwrap().span, - snippet(cx, first_local.pat.span, "..").to_string(), - ) + .filter_map(|x| { + if let ExprKind::Let(l) = x.kind { + Some((l.init.span, snippet(cx, first_local.pat.span, "..").to_string())) + } else { + None + } }) .collect::>(); - diag.multipart_suggestion( - "initialize this variable to be the `Some` variant (may need dereferencing)", - extra_local_suggestions, - Applicability::Unspecified, - ); + if !extra_local_suggestions.is_empty() { + diag.multipart_suggestion( + "initialize this variable to be the `Some` variant (may need dereferencing)", + extra_local_suggestions, + Applicability::Unspecified, + ); + } } if !r.extra_exprs.is_empty() { let index_accesses = r @@ -140,20 +136,17 @@ impl LateLintPass<'_> for UnnecessaryIndexing { struct IndexCheckResult<'a> { // the receiver for the index operation - pub index_receiver: Option<&'a Expr<'a>>, + index_receiver: Option<&'a Expr<'a>>, // first local in the block - used as pattern for `Some(pat)` - pub first_local: Option<&'a LetStmt<'a>>, - // any other locals to be aware of, these are set to the value of `pat` - pub extra_locals: Vec<&'a LetStmt<'a>>, + first_local: Option<&'a LetStmt<'a>>, // any other index expressions to replace with `pat` (or "element" if no local exists) - pub extra_exprs: Vec<&'a Expr<'a>>, + extra_exprs: Vec<&'a Expr<'a>>, } impl<'a> IndexCheckResult<'a> { pub fn new() -> Self { IndexCheckResult { index_receiver: None, first_local: None, - extra_locals: vec![], extra_exprs: vec![], } } @@ -170,7 +163,6 @@ fn process_indexing<'a>( let mut index_receiver: Option<&Expr<'_>> = None; let mut first_local: Option<&LetStmt<'_>> = None; - let mut extra_locals: Vec<&LetStmt<'_>> = vec![]; let mut extra_exprs: Vec<&Expr<'_>> = vec![]; // if res == Some(()), then mutation occurred @@ -190,7 +182,7 @@ fn process_indexing<'a>( if first_local.is_none() { first_local = Some(local); } else { - extra_locals.push(local); + extra_exprs.push(x); }; } else { extra_exprs.push(x); @@ -214,7 +206,6 @@ fn process_indexing<'a>( if res.is_none() { result.extra_exprs = extra_exprs; - result.extra_locals = extra_locals; result.first_local = first_local; result.index_receiver = index_receiver; Some(result) diff --git a/tests/ui/unnecessary_indexing.stderr b/tests/ui/unnecessary_indexing.stderr index d346d8e81b0f..37365bcff9f7 100644 --- a/tests/ui/unnecessary_indexing.stderr +++ b/tests/ui/unnecessary_indexing.stderr @@ -131,14 +131,11 @@ help: remove this line LL - let b = a[0]; LL + | -help: initialize this variable to be the `Some` variant (may need dereferencing) - | -LL | let c = b; - | ~ help: set this index to be the `Some` variant (may need dereferencing) | -LL | drop(b); - | ~ +LL ~ let c = b; +LL ~ drop(b); + | error: condition can be simplified with if..let syntax --> tests/ui/unnecessary_indexing.rs:75:5 From 672eba5ec2a9865aa145e7a5bcc3189a045b6293 Mon Sep 17 00:00:00 2001 From: Jacher Date: Thu, 27 Jun 2024 09:27:54 +0000 Subject: [PATCH 6/8] inline if ststements; check locality earlier --- clippy_lints/src/unnecessary_indexing.rs | 161 +++++++++++------------ 1 file changed, 77 insertions(+), 84 deletions(-) diff --git a/clippy_lints/src/unnecessary_indexing.rs b/clippy_lints/src/unnecessary_indexing.rs index 50a797acd15f..dae2b42cb948 100644 --- a/clippy_lints/src/unnecessary_indexing.rs +++ b/clippy_lints/src/unnecessary_indexing.rs @@ -4,10 +4,10 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::snippet; use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::visitors::for_each_expr; -use clippy_utils::{eq_expr_value, path_to_local, path_to_local_id}; +use clippy_utils::{path_to_local, path_to_local_id}; use rustc_ast::{LitKind, Mutability}; use rustc_errors::Applicability; -use rustc_hir::{Block, Expr, ExprKind, LetStmt, Node, UnOp}; +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; @@ -44,8 +44,8 @@ declare_clippy_lint! { declare_lint_pass!(UnnecessaryIndexing => [UNNECESSARY_INDEXING]); -impl LateLintPass<'_> for UnnecessaryIndexing { - fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ Expr<'_>) { +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) // check for negation && let ExprKind::Unary(UnOp::Not, unary_inner) = if_expr.cond.kind @@ -58,84 +58,85 @@ impl LateLintPass<'_> for UnnecessaryIndexing { && let ExprKind::Block(block, _) = if_expr.then.kind // do not lint if conditional receiver is mutable reference && expr_ty.ref_mutability() != Some(Mutability::Mut) - { - if let Some(r) = process_indexing(cx, block, conditional_receiver) + && let Some(con_path) = path_to_local(conditional_receiver) + && let Some(r) = process_indexing(cx, block, con_path) && let Some(receiver) = r.index_receiver - { - span_lint_and_then( - cx, - UNNECESSARY_INDEXING, - expr.span, - "condition can be simplified with if..let syntax", - |diag| { - if let Some(first_local) = r.first_local { - diag.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, - ); - diag.span_suggestion(first_local.span, "remove this line", "", Applicability::Unspecified); - if !r.extra_exprs.is_empty() { - let extra_local_suggestions = r - .extra_exprs - .iter() - .filter_map(|x| { - if let ExprKind::Let(l) = x.kind { - Some((l.init.span, snippet(cx, first_local.pat.span, "..").to_string())) - } else { - None - } - }) - .collect::>(); - - if !extra_local_suggestions.is_empty() { - diag.multipart_suggestion( - "initialize this variable to be the `Some` variant (may need dereferencing)", - extra_local_suggestions, - Applicability::Unspecified, - ); - } - } - if !r.extra_exprs.is_empty() { - let index_accesses = r - .extra_exprs - .iter() - .map(|x| (x.span, snippet(cx, first_local.pat.span, "..").to_string())) - .collect::>(); + { + span_lint_and_then( + cx, + UNNECESSARY_INDEXING, + expr.span, + "condition can be simplified with if..let syntax", + |diag| { + if let Some(first_local) = r.first_local + && first_local.pat.simple_ident().is_some() + { + diag.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, + ); + diag.span_suggestion(first_local.span, "remove this line", "", Applicability::Unspecified); + if !r.extra_exprs.is_empty() { + let extra_local_suggestions = r + .extra_exprs + .iter() + .filter_map(|x| { + if let ExprKind::Let(l) = x.kind { + Some((l.init.span, snippet(cx, first_local.pat.span, "..").to_string())) + } else { + None + } + }) + .collect::>(); + if !extra_local_suggestions.is_empty() { diag.multipart_suggestion( - "set this index to be the `Some` variant (may need dereferencing)", - index_accesses, + "initialize this variable to be the `Some` variant (may need dereferencing)", + extra_local_suggestions, Applicability::Unspecified, ); } - } else { - let mut index_accesses = vec![( - if_expr.cond.span, - format!("let Some(element) = {}.first()", snippet(cx, receiver.span, "..")), - )]; - index_accesses.extend(r.extra_exprs.iter().map(|x| (x.span, "element".to_owned()))); + } + if !r.extra_exprs.is_empty() { + let index_accesses = r + .extra_exprs + .iter() + .map(|x| (x.span, snippet(cx, first_local.pat.span, "..").to_string())) + .collect::>(); diag.multipart_suggestion( - "consider using if..let syntax (variable may need to be dereferenced)", + "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(r.extra_exprs.iter().map(|x| (x.span, "element".to_owned()))); + + diag.multipart_suggestion( + "consider using if..let syntax (variable may need to be dereferenced)", + index_accesses, + Applicability::Unspecified, + ); + } + }, + ); } } } struct IndexCheckResult<'a> { - // the receiver for the index operation + // the receiver for the index operation, only Some in the event the indexing is via a direct primitive index_receiver: Option<&'a Expr<'a>>, // first local in the block - used as pattern for `Some(pat)` first_local: Option<&'a LetStmt<'a>>, @@ -155,26 +156,23 @@ impl<'a> IndexCheckResult<'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: &'a LateContext<'_>, - block: &'a Block<'_>, - conditional_receiver: &'a Expr<'_>, + cx: &LateContext<'a>, + block: &'a Block<'a>, + conditional_receiver_hid: HirId, ) -> Option> { - let mut result = IndexCheckResult::new(); - let mut index_receiver: Option<&Expr<'_>> = None; let mut first_local: Option<&LetStmt<'_>> = None; let mut extra_exprs: Vec<&Expr<'_>> = vec![]; // if res == Some(()), then mutation occurred // & therefore we should not lint on this - let res = for_each_expr(block.stmts, |x| { + let res = for_each_expr(cx, block.stmts, |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 - && let Some(con_path) = path_to_local(conditional_receiver) - && path_to_local_id(receiver, con_path) + && path_to_local_id(receiver, conditional_receiver_hid) && val.0 == 0 { index_receiver = Some(receiver); @@ -195,21 +193,16 @@ fn process_indexing<'a>( }) { // 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, val) = x.kind - && eq_expr_value(cx, conditional_receiver, val) - { + } else if let ExprKind::AddrOf(_, Mutability::Mut, _) = x.kind { return ControlFlow::Break(()); }; ControlFlow::Continue::<()>(()) }); - if res.is_none() { - result.extra_exprs = extra_exprs; - result.first_local = first_local; - result.index_receiver = index_receiver; - Some(result) - } else { - None - } + res.is_none().then_some(IndexCheckResult { + index_receiver, + first_local, + extra_exprs, + }) } From 299bcfdbdbde29284fc6b741a16f05df4497825f Mon Sep 17 00:00:00 2001 From: Jacherr Date: Thu, 27 Jun 2024 21:36:47 +0100 Subject: [PATCH 7/8] remove unnecessary impl on IndexCheckResult --- clippy_lints/src/unnecessary_indexing.rs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/clippy_lints/src/unnecessary_indexing.rs b/clippy_lints/src/unnecessary_indexing.rs index dae2b42cb948..302ccabf4a91 100644 --- a/clippy_lints/src/unnecessary_indexing.rs +++ b/clippy_lints/src/unnecessary_indexing.rs @@ -143,15 +143,6 @@ struct IndexCheckResult<'a> { // any other index expressions to replace with `pat` (or "element" if no local exists) extra_exprs: Vec<&'a Expr<'a>>, } -impl<'a> IndexCheckResult<'a> { - pub fn new() -> Self { - IndexCheckResult { - index_receiver: None, - first_local: None, - extra_exprs: vec![], - } - } -} /// 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. From 410942403cd499e418bda44decf88227b973cd9d Mon Sep 17 00:00:00 2001 From: Jacherr Date: Mon, 1 Jul 2024 00:09:40 +0100 Subject: [PATCH 8/8] check for borrows in if block and change inner `Some` based on this --- clippy_lints/src/unnecessary_indexing.rs | 81 +++++++++++++++--------- tests/ui/unnecessary_indexing.rs | 2 +- tests/ui/unnecessary_indexing.stderr | 45 ++++++------- 3 files changed, 75 insertions(+), 53 deletions(-) diff --git a/clippy_lints/src/unnecessary_indexing.rs b/clippy_lints/src/unnecessary_indexing.rs index 302ccabf4a91..b0ce552b6b0b 100644 --- a/clippy_lints/src/unnecessary_indexing.rs +++ b/clippy_lints/src/unnecessary_indexing.rs @@ -75,53 +75,65 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryIndexing { if_expr.cond.span, "consider using if..let syntax (variable may need to be dereferenced)", format!( - "let Some({}) = {}.first()", + "let Some({}{}) = {}.first()", + // if we arent borrowing anything then we can pass a reference here for correctness + if r.extra_exprs_borrow.is_empty() { "&" } else { "" }, snippet(cx, first_local.pat.span, ".."), snippet(cx, receiver.span, "..") ), Applicability::Unspecified, ); diag.span_suggestion(first_local.span, "remove this line", "", Applicability::Unspecified); - if !r.extra_exprs.is_empty() { - let extra_local_suggestions = r - .extra_exprs + if !r.extra_exprs_borrow.is_empty() { + let mut index_accesses = r + .extra_exprs_borrow .iter() - .filter_map(|x| { - if let ExprKind::Let(l) = x.kind { - Some((l.init.span, snippet(cx, first_local.pat.span, "..").to_string())) - } else { - None - } - }) + .map(|x| (x.span, snippet(cx, first_local.pat.span, "..").to_string())) .collect::>(); - if !extra_local_suggestions.is_empty() { - diag.multipart_suggestion( - "initialize this variable to be the `Some` variant (may need dereferencing)", - extra_local_suggestions, - Applicability::Unspecified, - ); - } - } - if !r.extra_exprs.is_empty() { + index_accesses.extend( + r.extra_exprs_copy + .iter() + .map(|x| (x.span, snippet(cx, first_local.pat.span, "..").to_string())), + ); + + diag.multipart_suggestion( + "set this indexing to be the `Some` variant (may need dereferencing)", + index_accesses, + Applicability::Unspecified, + ); + } else if !r.extra_exprs_copy.is_empty() { let index_accesses = r - .extra_exprs + .extra_exprs_copy .iter() .map(|x| (x.span, snippet(cx, first_local.pat.span, "..").to_string())) .collect::>(); diag.multipart_suggestion( - "set this index to be the `Some` variant (may need dereferencing)", + "set this indexing to be the `Some` variant", index_accesses, Applicability::Unspecified, ); } + } else if r.extra_exprs_borrow.is_empty() { + let mut index_accesses = vec![( + if_expr.cond.span, + format!("let Some(&element) = {}.first()", snippet(cx, receiver.span, "..")), + )]; + index_accesses.extend(r.extra_exprs_copy.iter().map(|x| (x.span, "element".to_owned()))); + + diag.multipart_suggestion( + "consider using if..let syntax", + 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(r.extra_exprs.iter().map(|x| (x.span, "element".to_owned()))); + index_accesses.extend(r.extra_exprs_borrow.iter().map(|x| (x.span, "element".to_owned()))); + index_accesses.extend(r.extra_exprs_copy.iter().map(|x| (x.span, "element".to_owned()))); diag.multipart_suggestion( "consider using if..let syntax (variable may need to be dereferenced)", @@ -141,7 +153,10 @@ struct IndexCheckResult<'a> { // 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: Vec<&'a Expr<'a>>, + 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 @@ -153,13 +168,13 @@ fn process_indexing<'a>( ) -> Option> { let mut index_receiver: Option<&Expr<'_>> = None; let mut first_local: Option<&LetStmt<'_>> = None; - let mut extra_exprs: Vec<&Expr<'_>> = vec![]; + 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.stmts, |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 @@ -170,11 +185,16 @@ fn process_indexing<'a>( if let Node::LetStmt(local) = cx.tcx.parent_hir_node(x.hir_id) { if first_local.is_none() { first_local = Some(local); - } else { - extra_exprs.push(x); + 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.push(x); + extra_exprs_copy.push(x); }; } else if adjustments.iter().any(|adjustment| { matches!( @@ -194,6 +214,7 @@ fn process_indexing<'a>( res.is_none().then_some(IndexCheckResult { index_receiver, first_local, - extra_exprs, + extra_exprs_borrow, + extra_exprs_copy, }) } diff --git a/tests/ui/unnecessary_indexing.rs b/tests/ui/unnecessary_indexing.rs index ad082e7d8d88..31d2b2d0433b 100644 --- a/tests/ui/unnecessary_indexing.rs +++ b/tests/ui/unnecessary_indexing.rs @@ -65,7 +65,7 @@ fn main() { let a: &[i32] = &[1]; if !a.is_empty() { dbg!(a); - let b = a[0]; + let b = &a[0]; let c = a[0]; drop(a[0]); } diff --git a/tests/ui/unnecessary_indexing.stderr b/tests/ui/unnecessary_indexing.stderr index 37365bcff9f7..15714a5c25e2 100644 --- a/tests/ui/unnecessary_indexing.stderr +++ b/tests/ui/unnecessary_indexing.stderr @@ -8,9 +8,9 @@ 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 (variable may need to be dereferenced) +help: consider using if..let syntax | -LL ~ if let Some(element) = a.first() { +LL ~ if let Some(&element) = a.first() { LL ~ let b = c(element); | @@ -22,9 +22,9 @@ LL | | let b = Struct::a(a[0]); LL | | } | |_____^ | -help: consider using if..let syntax (variable may need to be dereferenced) +help: consider using if..let syntax | -LL ~ if let Some(element) = a.first() { +LL ~ if let Some(&element) = a.first() { LL ~ let b = Struct::a(element); | @@ -36,9 +36,9 @@ LL | | let b = c(a[0]); LL | | } | |_____^ | -help: consider using if..let syntax (variable may need to be dereferenced) +help: consider using if..let syntax | -LL ~ if let Some(element) = a.first() { +LL ~ if let Some(&element) = a.first() { LL ~ let b = c(element); | @@ -50,9 +50,9 @@ LL | | let b = Struct::a(a[0]); LL | | } | |_____^ | -help: consider using if..let syntax (variable may need to be dereferenced) +help: consider using if..let syntax | -LL ~ if let Some(element) = a.first() { +LL ~ if let Some(&element) = a.first() { LL ~ let b = Struct::a(element); | @@ -66,8 +66,8 @@ LL | | } | help: consider using if..let syntax (variable may need to be dereferenced) | -LL | if let Some(b) = a.first() { - | ~~~~~~~~~~~~~~~~~~~~~~~ +LL | if let Some(&b) = a.first() { + | ~~~~~~~~~~~~~~~~~~~~~~~~ help: remove this line | LL - let b = a[0]; @@ -84,8 +84,8 @@ LL | | } | help: consider using if..let syntax (variable may need to be dereferenced) | -LL | if let Some(b) = a.first() { - | ~~~~~~~~~~~~~~~~~~~~~~~ +LL | if let Some(&b) = a.first() { + | ~~~~~~~~~~~~~~~~~~~~~~~~ help: remove this line | LL - let b = a[0]; @@ -103,8 +103,8 @@ LL | | } | help: consider using if..let syntax (variable may need to be dereferenced) | -LL | if let Some(b) = a.first() { - | ~~~~~~~~~~~~~~~~~~~~~~~ +LL | if let Some(&b) = a.first() { + | ~~~~~~~~~~~~~~~~~~~~~~~~ help: remove this line | LL - let b = a[0]; @@ -116,7 +116,7 @@ error: condition can be simplified with if..let syntax | LL | / if !a.is_empty() { LL | | dbg!(a); -LL | | let b = a[0]; +LL | | let b = &a[0]; LL | | let c = a[0]; LL | | drop(a[0]); LL | | } @@ -124,17 +124,18 @@ LL | | } | help: consider using if..let syntax (variable may need to be dereferenced) | -LL | if let Some(b) = a.first() { +LL | if let Some(c) = a.first() { | ~~~~~~~~~~~~~~~~~~~~~~~ help: remove this line | -LL - let b = a[0]; +LL - let c = a[0]; LL + | -help: set this index to be the `Some` variant (may need dereferencing) +help: set this indexing to be the `Some` variant (may need dereferencing) | -LL ~ let c = b; -LL ~ drop(b); +LL ~ let b = c; +LL | let c = a[0]; +LL ~ drop(c); | error: condition can be simplified with if..let syntax @@ -147,9 +148,9 @@ LL | | drop(a[0]); LL | | } | |_____^ | -help: consider using if..let syntax (variable may need to be dereferenced) +help: consider using if..let syntax | -LL ~ if let Some(element) = a.first() { +LL ~ if let Some(&element) = a.first() { LL | dbg!(a); LL ~ drop(element); LL ~ drop(element);