diff --git a/CHANGELOG.md b/CHANGELOG.md index c4651cee057f..723a7dfda358 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5739,6 +5739,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 5d1eadfc7a4d..9abd414b01ee 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -717,6 +717,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 70292d3440ed..864a45778a8f 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -350,6 +350,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; @@ -1124,6 +1125,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(|_| Box::new(assigning_clones::AssigningClones)); store.register_late_pass(|_| Box::new(zero_repeat_side_effects::ZeroRepeatSideEffects)); store.register_late_pass(|_| Box::new(manual_unwrap_or_default::ManualUnwrapOrDefault)); + 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 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 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 +