Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new lint: unnecessary_indexing #12464

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

new lint: unnecessary_indexing #12464

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d3e174a
new lint: `unnecessary_indexing`
Jacherr Mar 11, 2024
bc4ce99
do not lint after mutable reference is taken
Jacherr Mar 14, 2024
cfa97d5
check path to local on conditional receiver, check mutable borrow aft…
Jacherr May 29, 2024
e8efc1a
fix autoborrow/mutability checks
Jacherr May 30, 2024
b54aaa3
remove unneded `extra_locals`
Jacherr Jun 26, 2024
672eba5
inline if ststements; check locality earlier
Jacherr Jun 27, 2024
299bcfd
remove unnecessary impl on IndexCheckResult
Jun 27, 2024
4109424
check for borrows in if block and change inner `Some` based on this
Jun 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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`
}

Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/manual_strip.rs
View file
Open in desktop
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
199 changes: 199 additions & 0 deletions clippy_lints/src/unnecessary_indexing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
use std::ops::ControlFlow;

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::{path_to_local, path_to_local_id};
use rustc_ast::{LitKind, Mutability};
use rustc_errors::Applicability;
use rustc_hir::{Block, Expr, ExprKind, HirId, LetStmt, Node, UnOp};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::adjustment::{Adjust, AutoBorrow, AutoBorrowMutability};
use rustc_session::declare_lint_pass;
use rustc_span::sym;

declare_clippy_lint! {
/// ### What it does
/// Checks for the use of `seq.is_empty()` in an if-conditional where `seq` is a slice, array, or Vec,
/// and in which the first element of the sequence is indexed.
///
/// ### Why is this bad?
/// This code is unnecessarily complicated and can instead be simplified to the use of an
/// if..let expression which accesses the first element of the sequence.
///
/// ### Example
/// ```no_run
/// let a: &[i32] = &[1];
/// if !a.is_empty() {
/// let b = a[0];
/// }
/// ```
/// Use instead:
/// ```no_run
/// let a: &[i32] = &[1];
/// if let Some(b) = a.first() {
///
/// }
/// ```
#[clippy::version = "1.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<'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
// check for call of is_empty
&& let ExprKind::MethodCall(method, conditional_receiver, _, _) = unary_inner.kind
&& method.ident.as_str() == "is_empty"
Alexendoo marked this conversation as resolved.
Show resolved Hide resolved
&& 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
&& expr_ty.ref_mutability() != Some(Mutability::Mut)
&& 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
&& first_local.pat.simple_ident().is_some()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use if let Some(ident) = first_local.pat.simple_ident() we can use ident.name later instead of getting a snippet from the pat's span

{
diag.span_suggestion(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should push all the suggestions into one vec and use multipart_suggestion at the end for all the changes, multiple span suggestions are presented as alternatives rather than something to combine in e.g. rust-analyzer

if_expr.cond.span,
"consider using if..let syntax (variable may need to be dereferenced)",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's be confident that our suggestion won't require dereferencing

Suggested change
"consider using if..let syntax (variable may need to be dereferenced)",
"consider using `if let` instead of indexing",

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

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

diag.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(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, 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>>,
// any other index expressions to replace with `pat` (or "element" if no local exists)
extra_exprs: Vec<&'a Expr<'a>>,
}

/// Checks the block for any indexing of the conditional receiver. Returns `None` if the block
/// contains code that invalidates the lint, e.g., the receiver is accessed via a mutable reference.
fn process_indexing<'a>(
cx: &LateContext<'a>,
block: &'a Block<'a>,
conditional_receiver_hid: HirId,
) -> Option<IndexCheckResult<'a>> {
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(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
&& path_to_local_id(receiver, conditional_receiver_hid)
&& val.0 == 0
{
index_receiver = Some(receiver);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a callback to #12464 (comment) we could make this a bool and check it before returning to ditch the field

if let Node::LetStmt(local) = cx.tcx.parent_hir_node(x.hir_id) {
if first_local.is_none() {
first_local = Some(local);
Jacherr marked this conversation as resolved.
Show resolved Hide resolved
} else {
extra_exprs.push(x);
};
} 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, _) = x.kind {
return ControlFlow::Break(());
};
Comment on lines +199 to +209
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will false negative for any code that mutates unrelated things

if !x.is_empty() {
    unrelated.push(x[0]);
}

Something like

} else if path_to_local_id(x, conditional_receiver_hid) {
    match cx.typeck_results().expr_ty_adjusted(x).kind() {
        RawPtr(_, Mutability::Mut) | Ref(_, _, Mutability::Mut) => return ControlFlow::Break(());
        Adt(..) => {
            if /* parent expr is not & */ {
                return ControlFlow::Break(())
            }
        }
        _ => {}
    }
}

would narrow it down to only things that mutate our input, it should also let us remove this check and lint when the type is &mut [T]/&mut Vec<T> (assuming it works)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A step further would be to not break immediately but store a bool that we've seen a mutation and break in the if let ExprKind::Index(receiver, index, _) if it's true

This would be since it doesn't matter if the code is mutating the input as long as it comes after all the indexes we're modifying:

// ok
if !input.is_empty() {
    input[0];
    input.push(1);
}
// not ok
if !input.is_empty() {
    input.push(1);
    input[0];
}


ControlFlow::Continue::<()>(())
});

res.is_none().then_some(IndexCheckResult {
index_receiver,
first_local,
extra_exprs,
})
}
1 change: 1 addition & 0 deletions tests/ui/index_refutable_slice/if_let_slice_binding.fixed
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
Loading