new lint: unnecessary_indexing
#12464
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,220 @@ | ||||||
| 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" | ||||||
| && 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() | ||||||
|
There was a problem hiding this comment. If we use |
||||||
| { | ||||||
| diag.span_suggestion( | ||||||
|
There was a problem hiding this comment. We should push all the suggestions into one vec and use |
||||||
| if_expr.cond.span, | ||||||
| "consider using if..let syntax (variable may need to be dereferenced)", | ||||||
|
There was a problem hiding this comment. Let's be confident that our suggestion won't require dereferencing
Suggested change
|
||||||
| format!( | ||||||
| "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_borrow.is_empty() { | ||||||
| let mut index_accesses = r | ||||||
| .extra_exprs_borrow | ||||||
| .iter() | ||||||
| .map(|x| (x.span, snippet(cx, first_local.pat.span, "..").to_string())) | ||||||
| .collect::<Vec<_>>(); | ||||||
|
|
||||||
| 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_copy | ||||||
| .iter() | ||||||
| .map(|x| (x.span, snippet(cx, first_local.pat.span, "..").to_string())) | ||||||
| .collect::<Vec<_>>(); | ||||||
|
|
||||||
| diag.multipart_suggestion( | ||||||
| "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_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)", | ||||||
| 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_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 | ||||||
| /// 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_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 | ||||||
| && path_to_local_id(receiver, conditional_receiver_hid) | ||||||
| && val.0 == 0 | ||||||
| { | ||||||
| index_receiver = Some(receiver); | ||||||
|
There was a problem hiding this comment. 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
|
||||||
| 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_copy.push(x); | ||||||
| }; | ||||||
|
Comment on lines
+192
to
+198
There was a problem hiding this comment. It would also need a borrow when fn f(x: &[String]) {
if !x.is_empty() {
let _ = x[0].len();
}
} |
||||||
| } 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
There was a problem hiding this comment. 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 There was a problem hiding this comment. A step further would be to not break immediately but store a 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_borrow, | ||||||
| extra_exprs_copy, | ||||||
| }) | ||||||
| } | ||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.