Skip to content

Commit

Permalink
Add needless_character_iteration lint
Browse files Browse the repository at this point in the history
  • Loading branch information
GuillaumeGomez committed May 27, 2024
1 parent caad063 commit 566dfd9
Show file tree
Hide file tree
Showing 9 changed files with 330 additions and 18 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
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 @@ -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,
Expand Down
24 changes: 24 additions & 0 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down
108 changes: 108 additions & 0 deletions clippy_lints/src/methods/needless_character_iteration.rs
Original file line number Diff line number Diff line change
@@ -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,
);
}
}
19 changes: 2 additions & 17 deletions clippy_lints/src/methods/unnecessary_result_map_or_else.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<'_>) {
Expand All @@ -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<'_>,
Expand Down
18 changes: 17 additions & 1 deletion clippy_lints/src/methods/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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)
}
51 changes: 51 additions & 0 deletions tests/ui/needless_character_iteration.fixed
Original file line number Diff line number Diff line change
@@ -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));
}
59 changes: 59 additions & 0 deletions tests/ui/needless_character_iteration.rs
Original file line number Diff line number Diff line change
@@ -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));
}
Loading

0 comments on commit 566dfd9

Please sign in to comment.