From a6f952b2834c671e37892cce8a8840b323c22a89 Mon Sep 17 00:00:00 2001 From: Jacherr Date: Wed, 13 Mar 2024 00:35:39 +0000 Subject: [PATCH] use visitors to support full block --- clippy_lints/src/manual_strip.rs | 4 +- clippy_lints/src/unnecessary_indexing.rs | 206 +++++++++--------- .../if_let_slice_binding.fixed | 1 + .../if_let_slice_binding.rs | 1 + .../if_let_slice_binding.stderr | 20 +- tests/ui/unnecessary_indexing.rs | 25 +++ tests/ui/unnecessary_indexing.stderr | 80 ++++++- 7 files changed, 214 insertions(+), 123 deletions(-) diff --git a/clippy_lints/src/manual_strip.rs b/clippy_lints/src/manual_strip.rs index bcd024360024..e7da7efaf08d 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,7 +105,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualStrip { span_lint_and_then( cx, MANUAL_STRIP, - strippings[0], + *first_stripping, &format!("stripping a {kind_word} manually"), |diag| { diag.span_note(test_span, format!("the {kind_word} was tested here")); diff --git a/clippy_lints/src/unnecessary_indexing.rs b/clippy_lints/src/unnecessary_indexing.rs index d3fbc81c3e9b..baff3f55cc82 100644 --- a/clippy_lints/src/unnecessary_indexing.rs +++ b/clippy_lints/src/unnecessary_indexing.rs @@ -1,13 +1,16 @@ +use std::ops::ControlFlow; + use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::eq_expr_value; use clippy_utils::source::snippet; use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::visitors::for_each_expr; use rustc_ast::LitKind; use rustc_errors::Applicability; -use rustc_hir::{ExprKind, Local, StmtKind, UnOp}; +use rustc_hir::{Expr, ExprKind, Local, Node, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; -use rustc_span::{sym, Span}; +use rustc_span::sym; declare_clippy_lint! { /// ### What it does @@ -53,111 +56,106 @@ impl LateLintPass<'_> for UnnecessaryIndexing { && (expr_ty.is_array_slice() || expr_ty.is_array() || is_type_diagnostic_item(cx, expr_ty, sym::Vec)) && let ExprKind::Block(block, _) = if_expr.then.kind { - // checked if conditional is calling `is_empty` on a sequence, now check if the first - // statement inside the 'if' statement does unchecked get of first element + // the receiver for the index operation + let mut index_receiver: Option<&Expr<'_>> = None; + // first local in the block - used as pattern for `Some(pat)` + let mut first_local: Option<&Local<'_>> = None; + // any other locals to be aware of, these are set to the value of `pat` + let mut extra_locals: Vec<&Local<'_>> = vec![]; + // any other index expressions to replace with `pat` (or "element" if no local exists) + let mut extra_exprs: Vec<&Expr<'_>> = vec![]; - // if calling a function on the indexing - local - if let Some(first) = block.stmts.first() - && let StmtKind::Local(local) = first.kind - && let Some(init) = local.init - && let ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args, _) = init.kind - { - for arg_expr in args { - if check_arg_stmt(cx, expr, arg_expr, conditional_receiver, if_expr.cond.span) { - break; - } - } - // if calling a function on the indexing - statement or expression - } else if let Some(first) = block.stmts.first() - && let StmtKind::Expr(stmt_expr) | StmtKind::Semi(stmt_expr) = first.kind - && let ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args, _) = stmt_expr.kind - { - for arg_expr in args { - if check_arg_stmt(cx, expr, arg_expr, conditional_receiver, if_expr.cond.span) { - break; - } - } - // if calling on a local which is not in itself a call or methodcall - } else if let Some(first) = block.stmts.first() - && let StmtKind::Local(local) = first.kind - && let Some(_) = local.init - { - check_local_stmt(cx, expr, local, conditional_receiver, if_expr.cond.span); - } - } - } -} + for_each_expr(block.stmts, |x| { + if let ExprKind::Index(receiver, index, _) = x.kind + && let ExprKind::Lit(lit) = index.kind + && let LitKind::Int(val, _) = lit.node + && eq_expr_value(cx, receiver, conditional_receiver) + && val.0 == 0 + { + index_receiver = Some(receiver); + if let Node::Local(local) = cx.tcx.parent_hir_node(x.hir_id) { + if first_local.is_none() { + first_local = Some(local); + } else { + extra_locals.push(local); + }; + } else { + extra_exprs.push(x); + }; + }; -fn check_local_stmt( - cx: &LateContext<'_>, - expr: &'_ rustc_hir::Expr<'_>, - local: &Local<'_>, - conditional_receiver: &'_ rustc_hir::Expr<'_>, - cond_span: Span, -) { - if let ExprKind::Index(receiver, index, _) = local.init.unwrap().kind - && let ExprKind::Lit(lit) = index.kind - && let LitKind::Int(val, _) = lit.node - && eq_expr_value(cx, receiver, conditional_receiver) - && val.0 == 0 - { - span_lint_and_then( - cx, - UNNECESSARY_INDEXING, - expr.span, - "condition can be simplified with if..let syntax", - |x| { - x.span_suggestion( - cond_span, - "consider using if..let syntax (variable may need to be dereferenced)", - format!( - "let Some({}) = {}.first()", - snippet(cx, local.pat.span, ".."), - snippet(cx, receiver.span, "..") - ), - Applicability::Unspecified, - ); - x.span_suggestion(local.span, "remove this line", "", Applicability::MachineApplicable); - }, - ); - } -} + ControlFlow::Continue::<()>(()) + }); -fn check_arg_stmt( - cx: &LateContext<'_>, - expr: &'_ rustc_hir::Expr<'_>, - arg_expr: &'_ rustc_hir::Expr<'_>, - conditional_receiver: &'_ rustc_hir::Expr<'_>, - cond_span: Span, -) -> bool { - if let ExprKind::Index(receiver, index, _) = arg_expr.kind - && let ExprKind::Lit(lit) = index.kind - && let LitKind::Int(val, _) = lit.node - && eq_expr_value(cx, receiver, conditional_receiver) - && val.0 == 0 - { - span_lint_and_then( - cx, - UNNECESSARY_INDEXING, - expr.span, - "condition can be simplified with if..let syntax", - |x| { - x.multipart_suggestion( - "consider using if..let syntax (variable may need to be dereferenced)", - vec![ - ( - cond_span, - format!("let Some(element) = {}.first()", snippet(cx, receiver.span, "..")), - ), - (arg_expr.span, "element".to_owned()), - ], - Applicability::Unspecified, - ); - }, - ); + if let Some(receiver) = index_receiver { + span_lint_and_then( + cx, + UNNECESSARY_INDEXING, + expr.span, + "condition can be simplified with if..let syntax", + |x| { + if let Some(first_local) = first_local { + x.span_suggestion( + if_expr.cond.span, + "consider using if..let syntax (variable may need to be dereferenced)", + format!( + "let Some({}) = {}.first()", + snippet(cx, first_local.pat.span, ".."), + snippet(cx, receiver.span, "..") + ), + Applicability::Unspecified, + ); + x.span_suggestion( + first_local.span, + "remove this line", + "", + Applicability::MachineApplicable, + ); + if !extra_locals.is_empty() { + let extra_local_suggestions = extra_locals + .iter() + .map(|x| { + ( + x.init.unwrap().span, + snippet(cx, first_local.pat.span, "..").to_string(), + ) + }) + .collect::>(); - return true; - } + x.multipart_suggestion( + "initialize this variable to be the `Some` variant (may need dereferencing)", + extra_local_suggestions, + Applicability::Unspecified, + ); + } + if !extra_exprs.is_empty() { + let index_accesses = extra_exprs + .iter() + .map(|x| (x.span, snippet(cx, first_local.pat.span, "..").to_string())) + .collect::>(); - false + x.multipart_suggestion( + "set this index to be the `Some` variant (may need dereferencing)", + index_accesses, + Applicability::Unspecified, + ); + } + } else { + let mut index_accesses = vec![( + if_expr.cond.span, + format!("let Some(element) = {}.first()", snippet(cx, receiver.span, "..")), + )]; + index_accesses.extend(extra_exprs.iter().map(|x| (x.span, "element".to_owned()))); + + x.multipart_suggestion( + "consider using if..let syntax (variable may need to be dereferenced)", + index_accesses, + Applicability::Unspecified, + ); + } + }, + ); + } + } + } } diff --git a/tests/ui/index_refutable_slice/if_let_slice_binding.fixed b/tests/ui/index_refutable_slice/if_let_slice_binding.fixed 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 index ef1da0e9e403..bc7d5c369327 100644 --- a/tests/ui/unnecessary_indexing.rs +++ b/tests/ui/unnecessary_indexing.rs @@ -1,5 +1,6 @@ //@no-rustfix #![allow(unused)] +#![allow(dropping_copy_types)] #![warn(clippy::unnecessary_indexing)] fn c(x: i32) -> i32 { @@ -52,6 +53,30 @@ fn main() { let b = a[0]; } + // lint when access is not first line + let a: &[i32] = &[1]; + if !a.is_empty() { + dbg!(a); + let b = a[0]; + } + + // lint on multiple accesses/locals + let a: &[i32] = &[1]; + if !a.is_empty() { + dbg!(a); + let b = a[0]; + let c = a[0]; + drop(a[0]); + } + + // lint on multiple accesses + let a: &[i32] = &[1]; + if !a.is_empty() { + dbg!(a); + drop(a[0]); + drop(a[0]); + } + // dont lint when not accessing [0] let a: &[i32] = &[1, 2]; if !a.is_empty() { diff --git a/tests/ui/unnecessary_indexing.stderr b/tests/ui/unnecessary_indexing.stderr index 23bcd0abb585..0397873552c1 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:21:5 + --> tests/ui/unnecessary_indexing.rs:22: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:27:5 + --> tests/ui/unnecessary_indexing.rs:28: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:33:5 + --> tests/ui/unnecessary_indexing.rs:34: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:39:5 + --> tests/ui/unnecessary_indexing.rs:40: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:45:5 + --> tests/ui/unnecessary_indexing.rs:46: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:51:5 + --> tests/ui/unnecessary_indexing.rs:52:5 | LL | / if !a.is_empty() { LL | | let b = a[0]; @@ -92,5 +92,71 @@ LL - let b = a[0]; LL + | -error: aborting due to 6 previous errors +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