Skip to content

Commit

Permalink
new lint: unnecessary_indexing
Browse files Browse the repository at this point in the history
  • Loading branch information
Jacherr committed Mar 13, 2024
1 parent 99e8000 commit ff31e49
Show file tree
Hide file tree
Showing 10 changed files with 440 additions and 12 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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`
}

Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/manual_strip.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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"));
Expand Down
161 changes: 161 additions & 0 deletions clippy_lints/src/unnecessary_indexing.rs
Original file line number Diff line number Diff line change
@@ -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::<Vec<_>>();

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::<Vec<_>>();

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,
);
}
},
);
}
}
}
}
1 change: 1 addition & 0 deletions tests/ui/index_refutable_slice/if_let_slice_binding.fixed
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#![deny(clippy::index_refutable_slice)]
#![allow(clippy::uninlined_format_args)]
#![allow(clippy::unnecessary_indexing)]

enum SomeEnum<T> {
One(T),
Expand Down
1 change: 1 addition & 0 deletions tests/ui/index_refutable_slice/if_let_slice_binding.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#![deny(clippy::index_refutable_slice)]
#![allow(clippy::uninlined_format_args)]
#![allow(clippy::unnecessary_indexing)]

enum SomeEnum<T> {
One(T),
Expand Down
20 changes: 10 additions & 10 deletions tests/ui/index_refutable_slice/if_let_slice_binding.stderr
Original file line number Diff line number Diff line change
@@ -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 {
| ^^^^^
Expand All @@ -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 {
| ^^^^^
Expand All @@ -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 {
| ^^^^^
Expand All @@ -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 {
| ^^^^^
Expand All @@ -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) {
| ^
Expand All @@ -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) {
| ^
Expand All @@ -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 {
| ^^^^^
Expand All @@ -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 {
| ^^^^^
Expand All @@ -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 {
| ^^^^^
Expand All @@ -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 {
| ^^^^^
Expand Down
Loading

0 comments on commit ff31e49

Please sign in to comment.