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.

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 to detect inefficient iter().any() #13817

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6020,6 +6020,7 @@ Released 2018-09-13
[`size_of_in_element_count`]: https://rust-lang.github.io/rust-clippy/master/index.html#size_of_in_element_count
[`size_of_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#size_of_ref
[`skip_while_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#skip_while_next
[`slice_iter_any`]: https://rust-lang.github.io/rust-clippy/master/index.html#slice_iter_any
[`slow_vector_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#slow_vector_initialization
[`stable_sort_primitive`]: https://rust-lang.github.io/rust-clippy/master/index.html#stable_sort_primitive
[`std_instead_of_alloc`]: https://rust-lang.github.io/rust-clippy/master/index.html#std_instead_of_alloc
Expand Down Expand Up @@ -6115,6 +6116,7 @@ Released 2018-09-13
[`unnecessary_first_then_check`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_first_then_check
[`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_iter_any`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_iter_any
[`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_bound`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_literal_bound
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
crate::methods::SHOULD_IMPLEMENT_TRAIT_INFO,
crate::methods::SINGLE_CHAR_ADD_STR_INFO,
crate::methods::SKIP_WHILE_NEXT_INFO,
crate::methods::SLICE_ITER_ANY_INFO,
crate::methods::STABLE_SORT_PRIMITIVE_INFO,
crate::methods::STRING_EXTEND_CHARS_INFO,
crate::methods::STRING_LIT_CHARS_ANY_INFO,
Expand All @@ -482,6 +483,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
crate::methods::UNNECESSARY_FIRST_THEN_CHECK_INFO,
crate::methods::UNNECESSARY_FOLD_INFO,
crate::methods::UNNECESSARY_GET_THEN_CHECK_INFO,
crate::methods::UNNECESSARY_ITER_ANY_INFO,
crate::methods::UNNECESSARY_JOIN_INFO,
crate::methods::UNNECESSARY_LAZY_EVALUATIONS_INFO,
crate::methods::UNNECESSARY_LITERAL_UNWRAP_INFO,
Expand Down
60 changes: 60 additions & 0 deletions clippy_lints/src/methods/iter_any.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
use clippy_utils::diagnostics::span_lint;
use rustc_hir::{Expr, ExprKind};
use rustc_lint::LateContext;
use rustc_middle::ty::{self};

use super::{SLICE_ITER_ANY, UNNECESSARY_ITER_ANY, method_call};

pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>) {
if !expr.span.from_expansion()
// any()
&& let Some((name, recv, args, _, _)) = method_call(expr)
&& name == "any"
// check if the inner closure is a equality check
&& args.len() == 1
&& let ExprKind::Closure(closure) = args[0].kind
&& let body = cx.tcx.hir().body(closure.body)
&& let ExprKind::Binary(op, _, _) = body.value.kind
&& op.node == rustc_ast::ast::BinOpKind::Eq
// iter()
&& let Some((name, recv, _, _, _)) = method_call(recv)
&& name == "iter"
{
let ref_type = cx.typeck_results().expr_ty(recv);
Copy link
Member

Choose a reason for hiding this comment

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

issue: Probably should be expr_ty_adjusted to handle autoderef. Add a test that ensures this works on a vector vec.iter().any(...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I changed to use expr_ty_adjusted and added a test for this in e964cbc.


match ref_type.kind() {
ty::Ref(_, inner_type, _) if inner_type.is_slice() => {
// check if the receiver is a u8/i8 slice
if let ty::Slice(slice_type) = inner_type.kind()
&& (slice_type.to_string() == "u8" || slice_type.to_string() == "i8")
Copy link
Contributor Author

@lapla-cogito lapla-cogito Dec 14, 2024

Choose a reason for hiding this comment

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

In my environment, I could only see the speedup for the u8 and i8 slices (about 5~7x) while @samueltardieu says he has been able to confirm this with other types of slices, so the changes in this PR are only for these types.
At least the speedups for the u8 and i8 slices are correct for reasons that come from the Rust implementation, I think.

{
span_lint(
cx,
SLICE_ITER_ANY,
expr.span,
"using `contains()` instead of `iter().any()` on u8/i8 slices is more efficient",
);
} else if let ty::Slice(slice_type) = inner_type.kind()
&& slice_type.is_numeric()
{
span_lint(
cx,
UNNECESSARY_ITER_ANY,
expr.span,
"using `contains()` instead of `iter().any()` is more readable",
);
}
},
// if it's an array that uses `iter().any()` and its closure is an equality check, suggest using
// `contains()` (currently only for numeric arrays because of the difficulty in determining whether
// `contains()` can be replaced by `contains()` for arrays of general types)
lapla-cogito marked this conversation as resolved.
Show resolved Hide resolved
ty::Array(array_type, _) if array_type.is_numeric() => span_lint(
cx,
UNNECESSARY_ITER_ANY,
expr.span,
"using `contains()` instead of `iter().any()` is more readable",
),
_ => {},
}
}
}
52 changes: 52 additions & 0 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ mod inspect_for_each;
mod into_iter_on_ref;
mod is_digit_ascii_radix;
mod is_empty;
mod iter_any;
mod iter_cloned_collect;
mod iter_count;
mod iter_filter;
Expand Down Expand Up @@ -4284,6 +4285,54 @@ declare_clippy_lint! {
"map of a trivial closure (not dependent on parameter) over a range"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for usage of `iter().any()` on slices of `u8`/`i8` when it can be replaced with `contains()` and suggests doing so.
///
/// ### Why is this bad?
/// `contains()` on slices of `u8`/`i8` is faster than `iter().any()` in such cases.
///
/// ### Example
/// ```no_run
/// fn foo(values: &[u8]) -> bool {
/// values.iter().any(|&v| v == 10)
/// }
/// ```
/// Use instead:
/// ```no_run
/// fn foo(values: &[u8]) -> bool {
/// values.contains(&10)
/// }
/// ```
#[clippy::version = "1.85.0"]
pub SLICE_ITER_ANY,
perf,
"using `contains()` instead of `iter().any()` on `u8`/`i8` slices is more fast"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for usage of `iter().any()` when it can be replaced with `contains()` and suggests doing so.
///
/// ### Why is this bad?
/// It makes the code less readable.
///
/// ### Example
/// ```no_run
/// let values = &[1, 2, 3];
/// let _ = values.iter().any(|&v| v == 2);
/// ```
/// Use instead:
/// ```no_run
/// let values = &[1, 2, 3];
/// let _ = values.contains(&2);
/// ```
#[clippy::version = "1.85.0"]
pub UNNECESSARY_ITER_ANY,
style,
"using `contains()` instead of `iter().any()` is more readable"
}

pub struct Methods {
avoid_breaking_exported_api: bool,
msrv: Msrv,
Expand Down Expand Up @@ -4449,6 +4498,8 @@ impl_lint_pass!(Methods => [
MAP_ALL_ANY_IDENTITY,
MAP_WITH_UNUSED_ARGUMENT_OVER_RANGES,
UNNECESSARY_MAP_OR,
SLICE_ITER_ANY,
UNNECESSARY_ITER_ANY,
]);

/// Extracts a method call name, args, and `Span` of the method name.
Expand Down Expand Up @@ -4683,6 +4734,7 @@ impl Methods {
("any", [arg]) => {
unused_enumerate_index::check(cx, expr, recv, arg);
needless_character_iteration::check(cx, expr, recv, arg, false);
iter_any::check(cx, expr);
match method_call(recv) {
Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(
cx,
Expand Down
36 changes: 36 additions & 0 deletions tests/ui/iter_any.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#![warn(clippy::slice_iter_any)]

fn main() {
let vec: Vec<u8> = vec![1, 2, 3, 4, 5, 6];
let values = &vec[..];
let _ = values.iter().any(|&v| v == 4);
//~^ ERROR: using `contains()` instead of `iter().any()` on u8/i8 slices is more efficient

let vec: Vec<u8> = vec![1, 2, 3, 4, 5, 6];
let values = &vec[..];
let _ = values.contains(&4);
// no error, because it uses `contains()`

let vec: Vec<u32> = vec![1, 2, 3, 4, 5, 6];
let values = &vec[..];
let _ = values.iter().any(|&v| v == 4);
//~^ ERROR: using `contains()` instead of `iter().any()` is more readable

let values: [u8; 6] = [3, 14, 15, 92, 6, 5];
let _ = values.iter().any(|&v| v == 10);
//~^ ERROR: using `contains()` instead of `iter().any()` is more readable

let values: [u8; 6] = [3, 14, 15, 92, 6, 5];
let _ = values.iter().any(|&v| v > 10);
// no error, because this `any()` is not for equality
}

fn foo(values: &[u8]) -> bool {
values.iter().any(|&v| v == 10)
//~^ ERROR: using `contains()` instead of `iter().any()` on u8/i8 slices is more efficient
}

fn bar(values: [u8; 3]) -> bool {
values.iter().any(|&v| v == 10)
//~^ ERROR: using `contains()` instead of `iter().any()` is more readable
}
38 changes: 38 additions & 0 deletions tests/ui/iter_any.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
error: using `contains()` instead of `iter().any()` on u8/i8 slices is more efficient
--> tests/ui/iter_any.rs:6:13
|
LL | let _ = values.iter().any(|&v| v == 4);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `-D clippy::slice-iter-any` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::slice_iter_any)]`

error: using `contains()` instead of `iter().any()` is more readable
--> tests/ui/iter_any.rs:16:13
|
LL | let _ = values.iter().any(|&v| v == 4);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `-D clippy::unnecessary-iter-any` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::unnecessary_iter_any)]`

error: using `contains()` instead of `iter().any()` is more readable
--> tests/ui/iter_any.rs:20:13
|
LL | let _ = values.iter().any(|&v| v == 10);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: using `contains()` instead of `iter().any()` on u8/i8 slices is more efficient
--> tests/ui/iter_any.rs:29:5
|
LL | values.iter().any(|&v| v == 10)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: using `contains()` instead of `iter().any()` is more readable
--> tests/ui/iter_any.rs:34:5
|
LL | values.iter().any(|&v| v == 10)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 5 previous errors

7 changes: 6 additions & 1 deletion tests/ui/needless_collect.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@
use std::collections::{BTreeMap, BTreeSet, BinaryHeap, HashMap, HashSet, LinkedList};

#[warn(clippy::needless_collect)]
#[allow(unused_variables, clippy::iter_cloned_collect, clippy::iter_next_slice)]
#[allow(
unused_variables,
clippy::unnecessary_iter_any,
clippy::iter_cloned_collect,
clippy::iter_next_slice
)]
fn main() {
let sample = [1; 5];
let len = sample.iter().count();
Expand Down
7 changes: 6 additions & 1 deletion tests/ui/needless_collect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@
use std::collections::{BTreeMap, BTreeSet, BinaryHeap, HashMap, HashSet, LinkedList};

#[warn(clippy::needless_collect)]
#[allow(unused_variables, clippy::iter_cloned_collect, clippy::iter_next_slice)]
#[allow(
unused_variables,
clippy::unnecessary_iter_any,
clippy::iter_cloned_collect,
clippy::iter_next_slice
)]
fn main() {
let sample = [1; 5];
let len = sample.iter().collect::<Vec<_>>().len();
Expand Down
Loading
Loading