From 158b65889cb5e5be2b367c1acb349c966e70543f Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Mon, 3 Jun 2024 21:57:47 +0200 Subject: [PATCH] Fix false positive for `needless_character_iteration` lint --- clippy_lints/src/methods/mod.rs | 3 ++- .../methods/needless_character_iteration.rs | 26 +++++++++++++++---- tests/ui/needless_character_iteration.fixed | 6 +++++ tests/ui/needless_character_iteration.rs | 14 +++++++--- tests/ui/needless_character_iteration.stderr | 8 +++--- 5 files changed, 43 insertions(+), 14 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 1572f6fb601..6a535c11a98 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -4485,7 +4485,7 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { }, ("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 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { }, ("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 index f4467af4de8..e3d78207715 100644 --- 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", "", "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 index 5a5da592987..f0bf84a41d7 100644 --- 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 index f6320ff22b7..2805d2438b4 100644 --- a/tests/ui/needless_character_iteration.rs +++ b/tests/ui/needless_character_iteration.rs @@ -17,17 +17,17 @@ 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| { @@ -35,7 +35,7 @@ fn main() { 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 index 05055f75aa7..7966033555f 100644 --- 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()