Add needless_character_iteration lint

This commit is contained in:
Guillaume Gomez 2024-05-17 18:05:46 +02:00
parent caad063933
commit 566dfd9008
9 changed files with 330 additions and 18 deletions

View file

@ -5567,6 +5567,7 @@ Released 2018-09-13
[`needless_borrow`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
[`needless_borrowed_reference`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrowed_reference
[`needless_borrows_for_generic_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
[`needless_character_iteration`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_character_iteration
[`needless_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_collect
[`needless_continue`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_continue
[`needless_doctest_main`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_doctest_main

View file

@ -418,6 +418,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::methods::MAP_UNWRAP_OR_INFO,
crate::methods::MUT_MUTEX_LOCK_INFO,
crate::methods::NAIVE_BYTECOUNT_INFO,
crate::methods::NEEDLESS_CHARACTER_ITERATION_INFO,
crate::methods::NEEDLESS_COLLECT_INFO,
crate::methods::NEEDLESS_OPTION_AS_DEREF_INFO,
crate::methods::NEEDLESS_OPTION_TAKE_INFO,

View file

@ -66,6 +66,7 @@ mod map_flatten;
mod map_identity;
mod map_unwrap_or;
mod mut_mutex_lock;
mod needless_character_iteration;
mod needless_collect;
mod needless_option_as_deref;
mod needless_option_take;
@ -4089,6 +4090,27 @@ declare_clippy_lint! {
"is_empty() called on strings known at compile time"
}
declare_clippy_lint! {
/// ### What it does
/// Checks if an iterator is used to check if a string is ascii.
///
/// ### Why is this bad?
/// The `str` type already implements the `is_ascii` method.
///
/// ### Example
/// ```no_run
/// "foo".chars().all(|c| c.is_ascii());
/// ```
/// Use instead:
/// ```no_run
/// "foo".is_ascii();
/// ```
#[clippy::version = "1.80.0"]
pub NEEDLESS_CHARACTER_ITERATION,
suspicious,
"is_ascii() called on a char iterator"
}
pub struct Methods {
avoid_breaking_exported_api: bool,
msrv: Msrv,
@ -4254,6 +4276,7 @@ impl_lint_pass!(Methods => [
UNNECESSARY_RESULT_MAP_OR_ELSE,
MANUAL_C_STR_LITERALS,
UNNECESSARY_GET_THEN_CHECK,
NEEDLESS_CHARACTER_ITERATION,
]);
/// Extracts a method call name, args, and `Span` of the method name.
@ -4461,6 +4484,7 @@ impl Methods {
},
("all", [arg]) => {
unused_enumerate_index::check(cx, expr, recv, arg);
needless_character_iteration::check(cx, expr, recv, arg);
if let Some(("cloned", recv2, [], _, _)) = method_call(recv) {
iter_overeager_cloned::check(
cx,

View file

@ -0,0 +1,108 @@
use rustc_errors::Applicability;
use rustc_hir::{Closure, Expr, ExprKind, HirId, StmtKind, UnOp};
use rustc_lint::LateContext;
use rustc_middle::ty;
use rustc_span::Span;
use super::utils::get_last_chain_binding_hir_id;
use super::NEEDLESS_CHARACTER_ITERATION;
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::snippet_opt;
use clippy_utils::{match_def_path, path_to_local_id, peel_blocks};
fn peels_expr_ref<'a, 'tcx>(mut expr: &'a Expr<'tcx>) -> &'a Expr<'tcx> {
while let ExprKind::AddrOf(_, _, e) = expr.kind {
expr = e;
}
expr
}
fn handle_expr(
cx: &LateContext<'_>,
expr: &Expr<'_>,
first_param: HirId,
span: Span,
before_chars: Span,
revert: bool,
) {
match expr.kind {
ExprKind::MethodCall(method, receiver, [], _) => {
if method.ident.name.as_str() == "is_ascii"
&& path_to_local_id(receiver, first_param)
&& let char_arg_ty = cx.typeck_results().expr_ty_adjusted(receiver).peel_refs()
&& *char_arg_ty.kind() == ty::Char
&& let Some(snippet) = snippet_opt(cx, before_chars)
{
span_lint_and_sugg(
cx,
NEEDLESS_CHARACTER_ITERATION,
span,
"checking if a string is ascii using iterators",
"try",
format!("{}{snippet}.is_ascii()", if revert { "!" } else { "" }),
Applicability::MachineApplicable,
);
}
},
ExprKind::Block(block, _) => {
if block.stmts.iter().any(|stmt| !matches!(stmt.kind, StmtKind::Let(_))) {
// If there is something else than let bindings, then better not emit the lint.
return;
}
if let Some(block_expr) = block.expr
// First we ensure that this is a "binding chain" (each statement is a binding
// of the previous one) and that it is a binding of the closure argument.
&& let Some(last_chain_binding_id) =
get_last_chain_binding_hir_id(first_param, block.stmts)
{
handle_expr(cx, block_expr, last_chain_binding_id, span, before_chars, revert);
}
},
ExprKind::Unary(UnOp::Not, expr) => handle_expr(cx, expr, first_param, span, before_chars, !revert),
ExprKind::Call(fn_path, [arg]) => {
if let ExprKind::Path(path) = fn_path.kind
&& let Some(fn_def_id) = cx.qpath_res(&path, fn_path.hir_id).opt_def_id()
&& match_def_path(cx, fn_def_id, &["core", "char", "methods", "<impl char>", "is_ascii"])
&& path_to_local_id(peels_expr_ref(arg), first_param)
&& let Some(snippet) = snippet_opt(cx, before_chars)
{
span_lint_and_sugg(
cx,
NEEDLESS_CHARACTER_ITERATION,
span,
"checking if a string is ascii using iterators",
"try",
format!("{}{snippet}.is_ascii()", if revert { "!" } else { "" }),
Applicability::MachineApplicable,
);
}
},
_ => {},
}
}
pub(super) fn check(cx: &LateContext<'_>, call_expr: &Expr<'_>, recv: &Expr<'_>, closure_arg: &Expr<'_>) {
if let ExprKind::Closure(&Closure { body, .. }) = closure_arg.kind
&& let body = cx.tcx.hir().body(body)
&& let Some(first_param) = body.params.first()
&& let ExprKind::MethodCall(method, mut recv, [], _) = recv.kind
&& method.ident.name.as_str() == "chars"
&& let str_ty = cx.typeck_results().expr_ty_adjusted(recv).peel_refs()
&& *str_ty.kind() == ty::Str
{
let expr_start = recv.span;
while let ExprKind::MethodCall(_, new_recv, _, _) = recv.kind {
recv = new_recv;
}
let body_expr = peel_blocks(body.value);
handle_expr(
cx,
body_expr,
first_param.pat.hir_id,
recv.span.with_hi(call_expr.span.hi()),
recv.span.with_hi(expr_start.hi()),
false,
);
}
}

View file

@ -4,10 +4,11 @@ use clippy_utils::source::snippet;
use clippy_utils::ty::is_type_diagnostic_item;
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_hir::{Closure, Expr, ExprKind, HirId, QPath, Stmt, StmtKind};
use rustc_hir::{Closure, Expr, ExprKind, HirId, QPath};
use rustc_lint::LateContext;
use rustc_span::symbol::sym;
use super::utils::get_last_chain_binding_hir_id;
use super::UNNECESSARY_RESULT_MAP_OR_ELSE;
fn emit_lint(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_arg: &Expr<'_>) {
@ -25,22 +26,6 @@ fn emit_lint(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_arg: &E
);
}
fn get_last_chain_binding_hir_id(mut hir_id: HirId, statements: &[Stmt<'_>]) -> Option<HirId> {
for stmt in statements {
if let StmtKind::Let(local) = stmt.kind
&& let Some(init) = local.init
&& let ExprKind::Path(QPath::Resolved(_, path)) = init.kind
&& let hir::def::Res::Local(local_hir_id) = path.res
&& local_hir_id == hir_id
{
hir_id = local.pat.hir_id;
} else {
return None;
}
}
Some(hir_id)
}
fn handle_qpath(
cx: &LateContext<'_>,
expr: &Expr<'_>,

View file

@ -4,7 +4,7 @@ use clippy_utils::{get_parent_expr, path_to_local_id, usage};
use rustc_ast::ast;
use rustc_errors::Applicability;
use rustc_hir::intravisit::{walk_expr, Visitor};
use rustc_hir::{BorrowKind, Expr, ExprKind, HirId, Mutability, Pat};
use rustc_hir::{BorrowKind, Expr, ExprKind, HirId, Mutability, Pat, QPath, Stmt, StmtKind};
use rustc_lint::LateContext;
use rustc_middle::hir::nested_filter;
use rustc_middle::ty::{self, Ty};
@ -171,3 +171,19 @@ impl<'cx, 'tcx> CloneOrCopyVisitor<'cx, 'tcx> {
.any(|hir_id| path_to_local_id(expr, *hir_id))
}
}
pub(super) fn get_last_chain_binding_hir_id(mut hir_id: HirId, statements: &[Stmt<'_>]) -> Option<HirId> {
for stmt in statements {
if let StmtKind::Let(local) = stmt.kind
&& let Some(init) = local.init
&& let ExprKind::Path(QPath::Resolved(_, path)) = init.kind
&& let rustc_hir::def::Res::Local(local_hir_id) = path.res
&& local_hir_id == hir_id
{
hir_id = local.pat.hir_id;
} else {
return None;
}
}
Some(hir_id)
}

View file

@ -0,0 +1,51 @@
#![warn(clippy::needless_character_iteration)]
#![allow(clippy::map_identity, clippy::unnecessary_operation)]
#[derive(Default)]
struct S {
field: &'static str,
}
impl S {
fn field(&self) -> &str {
self.field
}
}
fn magic(_: char) {}
fn main() {
"foo".is_ascii();
//~^ ERROR: checking if a string is ascii using iterators
!"foo".is_ascii();
//~^ ERROR: checking if a string is ascii using iterators
"foo".is_ascii();
//~^ ERROR: checking if a string is ascii using iterators
!"foo".is_ascii();
//~^ ERROR: checking if a string is ascii using iterators
let s = String::new();
s.is_ascii();
//~^ ERROR: checking if a string is ascii using iterators
!"foo".to_string().is_ascii();
//~^ ERROR: checking if a string is ascii using iterators
"foo".is_ascii();
!"foo".is_ascii();
S::default().field().is_ascii();
//~^ ERROR: checking if a string is ascii using iterators
// Should not lint!
"foo".chars().all(|c| {
let x = c;
magic(x);
x.is_ascii()
});
// Should not lint!
"foo".chars().all(|c| c.is_ascii() && c.is_alphabetic());
// Should not lint!
"foo".chars().map(|c| c).all(|c| !char::is_ascii(&c));
}

View file

@ -0,0 +1,59 @@
#![warn(clippy::needless_character_iteration)]
#![allow(clippy::map_identity, clippy::unnecessary_operation)]
#[derive(Default)]
struct S {
field: &'static str,
}
impl S {
fn field(&self) -> &str {
self.field
}
}
fn magic(_: char) {}
fn main() {
"foo".chars().all(|c| c.is_ascii());
//~^ ERROR: checking if a string is ascii using iterators
"foo".chars().all(|c| !c.is_ascii());
//~^ ERROR: checking if a string is ascii using iterators
"foo".chars().all(|c| char::is_ascii(&c));
//~^ ERROR: checking if a string is ascii using iterators
"foo".chars().all(|c| !char::is_ascii(&c));
//~^ ERROR: checking if a string is ascii using iterators
let s = String::new();
s.chars().all(|c| c.is_ascii());
//~^ ERROR: checking if a string is ascii using iterators
"foo".to_string().chars().all(|c| !c.is_ascii());
//~^ ERROR: checking if a string is ascii using iterators
"foo".chars().all(|c| {
//~^ ERROR: checking if a string is ascii using iterators
let x = c;
x.is_ascii()
});
"foo".chars().all(|c| {
//~^ ERROR: checking if a string is ascii using iterators
let x = c;
!x.is_ascii()
});
S::default().field().chars().all(|x| x.is_ascii());
//~^ ERROR: checking if a string is ascii using iterators
// Should not lint!
"foo".chars().all(|c| {
let x = c;
magic(x);
x.is_ascii()
});
// Should not lint!
"foo".chars().all(|c| c.is_ascii() && c.is_alphabetic());
// Should not lint!
"foo".chars().map(|c| c).all(|c| !char::is_ascii(&c));
}

View file

@ -0,0 +1,67 @@
error: checking if a string is ascii using iterators
--> tests/ui/needless_character_iteration.rs:18:5
|
LL | "foo".chars().all(|c| c.is_ascii());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `"foo".is_ascii()`
|
= note: `-D clippy::needless-character-iteration` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::needless_character_iteration)]`
error: checking if a string is ascii using iterators
--> tests/ui/needless_character_iteration.rs:20:5
|
LL | "foo".chars().all(|c| !c.is_ascii());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `!"foo".is_ascii()`
error: checking if a string is ascii using iterators
--> tests/ui/needless_character_iteration.rs:22:5
|
LL | "foo".chars().all(|c| char::is_ascii(&c));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `"foo".is_ascii()`
error: checking if a string is ascii using iterators
--> tests/ui/needless_character_iteration.rs:24:5
|
LL | "foo".chars().all(|c| !char::is_ascii(&c));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `!"foo".is_ascii()`
error: checking if a string is ascii using iterators
--> tests/ui/needless_character_iteration.rs:28:5
|
LL | s.chars().all(|c| c.is_ascii());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `s.is_ascii()`
error: checking if a string is ascii using iterators
--> tests/ui/needless_character_iteration.rs:30:5
|
LL | "foo".to_string().chars().all(|c| !c.is_ascii());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `!"foo".to_string().is_ascii()`
error: checking if a string is ascii using iterators
--> tests/ui/needless_character_iteration.rs:33:5
|
LL | / "foo".chars().all(|c| {
LL | |
LL | | let x = c;
LL | | x.is_ascii()
LL | | });
| |______^ help: try: `"foo".is_ascii()`
error: checking if a string is ascii using iterators
--> tests/ui/needless_character_iteration.rs:38:5
|
LL | / "foo".chars().all(|c| {
LL | |
LL | | let x = c;
LL | | !x.is_ascii()
LL | | });
| |______^ help: try: `!"foo".is_ascii()`
error: checking if a string is ascii using iterators
--> tests/ui/needless_character_iteration.rs:44:5
|
LL | S::default().field().chars().all(|x| x.is_ascii());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `S::default().field().is_ascii()`
error: aborting due to 9 previous errors