Auto merge of #12815 - GuillaumeGomez:add-needless_character_iteratio…

…n, r=xFrednet

Add `needless_character_iteration` lint

Fixes #4817.

r? `@xFrednet`

changelog: Add `needless_character_iteration` lint

CHANGELOG.md

@@ -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

clippy_lints/src/declared_lints.rs

@@ -419,6 +419,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,

clippy_lints/src/methods/mod.rs

@@ -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;
@@ -4090,6 +4091,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,
@@ -4255,6 +4277,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.
@@ -4462,6 +4485,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,

clippy_lints/src/methods/needless_character_iteration.rs

@@ -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,
+        );
+    }
+}

clippy_lints/src/methods/unnecessary_result_map_or_else.rs

@@ -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<'_>,

clippy_lints/src/methods/utils.rs

@@ -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};
@@ -175,3 +175,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)
+}

tests/ui/needless_character_iteration.fixed

@@ -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));
+}

tests/ui/needless_character_iteration.rs

@@ -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));
+}