Auto merge of rust-lang#12886 - GuillaumeGomez:fix-needless_character…

…_iteration, r=blyxyas Fix false positive for `needless_character_iteration` lint Fixes rust-lang#12879. changelog: Fix false positive for `needless_character_iteration` lint
flip1995 · Jun 5, 2024 · 10d1f32 · 10d1f32
2 parents 46e3304 + 158b658
commit 10d1f32
Show file tree

Hide file tree

Showing 5 changed files with 43 additions and 14 deletions.
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
@@ -4485,7 +4485,7 @@ impl Methods {
                 },
                 ("all", [arg]) => {
                     unused_enumerate_index::check(cx, expr, recv, arg);
-                    needless_character_iteration::check(cx, expr, recv, arg);
+                    needless_character_iteration::check(cx, expr, recv, arg, true);
                     if let Some(("cloned", recv2, [], _, _)) = method_call(recv) {
                         iter_overeager_cloned::check(
                             cx,
@@ -4506,6 +4506,7 @@ impl Methods {
                 },
                 ("any", [arg]) => {
                     unused_enumerate_index::check(cx, expr, recv, arg);
+                    needless_character_iteration::check(cx, expr, recv, arg, false);
                     match method_call(recv) {
                         Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(
                             cx,

diff --git a/clippy_lints/src/methods/needless_character_iteration.rs b/clippy_lints/src/methods/needless_character_iteration.rs
@@ -24,10 +24,14 @@ fn handle_expr(
     span: Span,
     before_chars: Span,
     revert: bool,
+    is_all: bool,
 ) {
     match expr.kind {
         ExprKind::MethodCall(method, receiver, [], _) => {
-            if method.ident.name.as_str() == "is_ascii"
+            // If we have `!is_ascii`, then only `.any()` should warn. And if the condition is
+            // `is_ascii`, then only `.all()` should warn.
+            if revert != is_all
+                && 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
@@ -55,12 +59,23 @@ fn handle_expr(
                 && 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);
+                handle_expr(
+                    cx,
+                    block_expr,
+                    last_chain_binding_id,
+                    span,
+                    before_chars,
+                    revert,
+                    is_all,
+                );
             }
         },
-        ExprKind::Unary(UnOp::Not, expr) => handle_expr(cx, expr, first_param, span, before_chars, !revert),
+        ExprKind::Unary(UnOp::Not, expr) => handle_expr(cx, expr, first_param, span, before_chars, !revert, is_all),
         ExprKind::Call(fn_path, [arg]) => {
-            if let ExprKind::Path(path) = fn_path.kind
+            // If we have `!is_ascii`, then only `.any()` should warn. And if the condition is
+            // `is_ascii`, then only `.all()` should warn.
+            if revert != is_all
+                && 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)
@@ -81,7 +96,7 @@ fn handle_expr(
     }
 }
 
-pub(super) fn check(cx: &LateContext<'_>, call_expr: &Expr<'_>, recv: &Expr<'_>, closure_arg: &Expr<'_>) {
+pub(super) fn check(cx: &LateContext<'_>, call_expr: &Expr<'_>, recv: &Expr<'_>, closure_arg: &Expr<'_>, is_all: bool) {
     if let ExprKind::Closure(&Closure { body, .. }) = closure_arg.kind
         && let body = cx.tcx.hir().body(body)
         && let Some(first_param) = body.params.first()
@@ -103,6 +118,7 @@ pub(super) fn check(cx: &LateContext<'_>, call_expr: &Expr<'_>, recv: &Expr<'_>,
             recv.span.with_hi(call_expr.span.hi()),
             recv.span.with_hi(expr_start.hi()),
             false,
+            is_all,
         );
     }
 }
diff --git a/tests/ui/needless_character_iteration.fixed b/tests/ui/needless_character_iteration.fixed
@@ -48,4 +48,10 @@ fn main() {
 
     // Should not lint!
     "foo".chars().map(|c| c).all(|c| !char::is_ascii(&c));
+
+    // Should not lint!
+    "foo".chars().all(|c| !c.is_ascii());
+
+    // Should not lint!
+    "foo".chars().any(|c| c.is_ascii());
 }
diff --git a/tests/ui/needless_character_iteration.rs b/tests/ui/needless_character_iteration.rs
@@ -17,25 +17,25 @@ 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());
+    "foo".chars().any(|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));
+    "foo".chars().any(|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());
+    "foo".to_string().chars().any(|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| {
+    "foo".chars().any(|c| {
         //~^ ERROR: checking if a string is ascii using iterators
         let x = c;
         !x.is_ascii()
@@ -56,4 +56,10 @@ fn main() {
 
     // Should not lint!
     "foo".chars().map(|c| c).all(|c| !char::is_ascii(&c));
+
+    // Should not lint!
+    "foo".chars().all(|c| !c.is_ascii());
+
+    // Should not lint!
+    "foo".chars().any(|c| c.is_ascii());
 }
diff --git a/tests/ui/needless_character_iteration.stderr b/tests/ui/needless_character_iteration.stderr
@@ -10,7 +10,7 @@ LL |     "foo".chars().all(|c| c.is_ascii());
 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());
+LL |     "foo".chars().any(|c| !c.is_ascii());
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `!"foo".is_ascii()`
 
 error: checking if a string is ascii using iterators
@@ -22,7 +22,7 @@ LL |     "foo".chars().all(|c| char::is_ascii(&c));
 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));
+LL |     "foo".chars().any(|c| !char::is_ascii(&c));
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `!"foo".is_ascii()`
 
 error: checking if a string is ascii using iterators
@@ -34,7 +34,7 @@ LL |     s.chars().all(|c| c.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());
+LL |     "foo".to_string().chars().any(|c| !c.is_ascii());
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `!"foo".to_string().is_ascii()`
 
 error: checking if a string is ascii using iterators
@@ -50,7 +50,7 @@ LL | |     });
 error: checking if a string is ascii using iterators
   --> tests/ui/needless_character_iteration.rs:38:5
    |
-LL | /     "foo".chars().all(|c| {
+LL | /     "foo".chars().any(|c| {
 LL | |
 LL | |         let x = c;
 LL | |         !x.is_ascii()