In string_patterns, use span instead of char to reduce allocation and complexity and some nits changes.

AurelienFT · AurelienFT · commit 06aae81d7f9d · 2024-06-05T11:50:55.000+02:00
diff --git a/clippy_lints/src/string_patterns.rs b/clippy_lints/src/string_patterns.rs
@@ -1,18 +1,19 @@
 use std::cmp::Ordering;
 use std::ops::ControlFlow;
 
-use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
 use clippy_utils::macros::matching_root_macro_call;
 use clippy_utils::path_to_local_id;
-use clippy_utils::source::str_literal_to_char_literal;
+use clippy_utils::source::{snippet_opt, str_literal_to_char_literal};
 use clippy_utils::visitors::{for_each_expr, Descend};
+use itertools::Itertools;
 use rustc_ast::{BinOpKind, LitKind};
 use rustc_errors::Applicability;
 use rustc_hir::{Expr, ExprKind, PatKind, QPath};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty;
 use rustc_session::declare_lint_pass;
-use rustc_span::sym;
+use rustc_span::{sym, Span};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -110,17 +111,17 @@ fn check_single_char_pattern_lint(cx: &LateContext<'_>, arg: &Expr<'_>) {
     }
 }
 
-fn get_char_value(expr: &Expr<'_>) -> Option<String> {
+fn get_char_span<'tcx>(cx: &'_ LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<Span> {
+    if !cx.typeck_results().expr_ty_adjusted(expr).is_char() || expr.span.from_expansion() {
+        return None;
+    }
     match expr.kind {
-        ExprKind::Lit(lit) => match lit.node {
-            LitKind::Char(c) => Some(format!("'{}'", c.escape_default())),
-            _ => None,
-        },
+        ExprKind::Lit(lit) if let LitKind::Char(_) = lit.node => Some(lit.span),
         ExprKind::Path(QPath::Resolved(_, path)) => {
             if path.segments.len() == 1 {
                 let segment = &path.segments[0];
                 if segment.args.is_none() {
-                    Some(segment.ident.name.to_string())
+                    Some(segment.ident.span)
                 } else {
                     None
                 }
@@ -138,44 +139,39 @@ fn check_manual_pattern_char_comparison(cx: &LateContext<'_>, method_arg: &Expr<
         && let body = cx.tcx.hir().body(closure.body)
         && let PatKind::Binding(_, binding, ..) = body.params[0].pat.kind
     {
-        let mut set_chars: Vec<String> = Vec::new();
+        let mut set_char_spans: Vec<Span> = Vec::new();
 
         // We want to retrieve all the comparisons done.
         // They are ordered in a nested way and so we need to traverse the AST to collect them all.
         if for_each_expr(body.value, |sub_expr| -> ControlFlow<(), Descend> {
             match sub_expr.kind {
                 ExprKind::Binary(op, left, right) if op.node == BinOpKind::Eq => {
                     if path_to_local_id(left, binding)
-                        && cx.typeck_results().expr_ty_adjusted(right).kind() == &ty::Char
-                        && let Some(c) = get_char_value(right)
+                        && let Some(span) = get_char_span(cx, right)
                     {
-                        set_chars.push(c);
+                        set_char_spans.push(span);
                         ControlFlow::Continue(Descend::No)
                     } else if path_to_local_id(right, binding)
-                        && cx.typeck_results().expr_ty_adjusted(left).kind() == &ty::Char
-                        && let Some(c) = get_char_value(left)
+                        && let Some(span) = get_char_span(cx, left)
                     {
-                        set_chars.push(c);
+                        set_char_spans.push(span);
                         ControlFlow::Continue(Descend::No)
                     } else {
                         ControlFlow::Break(())
                     }
                 },
                 ExprKind::Binary(op, _, _) if op.node == BinOpKind::Or => ControlFlow::Continue(Descend::Yes),
                 ExprKind::Match(match_value, [arm, _], _) => {
-                    if matching_root_macro_call(cx, sub_expr.span, sym::matches_macro).is_none() {
-                        return ControlFlow::Break(());
-                    }
-                    if arm.guard.is_some() {
-                        return ControlFlow::Break(());
-                    }
-                    if !path_to_local_id(match_value, binding) {
+                    if matching_root_macro_call(cx, sub_expr.span, sym::matches_macro).is_none()
+                        || arm.guard.is_some()
+                        || !path_to_local_id(match_value, binding)
+                    {
                         return ControlFlow::Break(());
                     }
                     if arm.pat.walk_short(|pat| match pat.kind {
                         PatKind::Lit(expr) if let ExprKind::Lit(lit) = expr.kind => {
-                            if let LitKind::Char(c) = lit.node {
-                                set_chars.push(format!("'{}'", c.escape_default()));
+                            if let LitKind::Char(_) = lit.node {
+                                set_char_spans.push(lit.span);
                             }
                             true
                         },
@@ -194,27 +190,37 @@ fn check_manual_pattern_char_comparison(cx: &LateContext<'_>, method_arg: &Expr<
         {
             return;
         }
-        match set_chars.len().cmp(&1) {
-            Ordering::Equal => span_lint_and_sugg(
-                cx,
-                MANUAL_PATTERN_CHAR_COMPARISON,
-                method_arg.span,
-                "this manual char comparison can be written more succinctly",
-                "consider using a `char`",
-                set_chars[0].clone(),
-                applicability,
-            ),
-            Ordering::Greater => span_lint_and_sugg(
-                cx,
-                MANUAL_PATTERN_CHAR_COMPARISON,
-                method_arg.span,
-                "this manual char comparison can be written more succinctly",
-                "consider using an array of `char`",
-                format!("[{}]", set_chars.join(", ")),
-                applicability,
-            ),
-            Ordering::Less => {},
-        }
+        span_lint_and_then(
+            cx,
+            MANUAL_PATTERN_CHAR_COMPARISON,
+            method_arg.span,
+            "this manual char comparison can be written more succinctly",
+            |diag| match set_char_spans.len().cmp(&1) {
+                Ordering::Equal => {
+                    diag.span_suggestion(
+                        method_arg.span,
+                        "consider using a `char`",
+                        snippet_opt(cx, set_char_spans[0]).unwrap(),
+                        applicability,
+                    );
+                },
+                Ordering::Greater => {
+                    diag.span_suggestion(
+                        method_arg.span,
+                        "consider using an array of `char`",
+                        format!(
+                            "[{}]",
+                            set_char_spans
+                                .into_iter()
+                                .map(|span| snippet_opt(cx, span).unwrap())
+                                .join(", ")
+                        ),
+                        applicability,
+                    );
+                },
+                Ordering::Less => {},
+            },
+        )
     }
 }
 
@@ -228,9 +234,8 @@ impl<'tcx> LateLintPass<'tcx> for StringPatterns {
             && let Some(&(_, pos)) = PATTERN_METHODS
                 .iter()
                 .find(|(array_method_name, _)| *array_method_name == method_name)
-            && args.len() > pos
+            && let Some(arg) = args.get(pos)
         {
-            let arg = &args[pos];
             check_single_char_pattern_lint(cx, arg);
 
             check_manual_pattern_char_comparison(cx, arg);
diff --git a/tests/ui/manual_pattern_char_comparison.fixed b/tests/ui/manual_pattern_char_comparison.fixed
@@ -18,6 +18,7 @@ fn main() {
     sentence.split(['\n', 'X', 'Y']);
     sentence.splitn(3, 'X');
     sentence.splitn(3, ['X', 'W']);
+    sentence.find('🎈');
 
     let not_str = NotStr;
     not_str.find(|c: char| c == 'X');
@@ -39,4 +40,10 @@ fn main() {
 
     "".find(|c| matches!(c, 'a' | '1'..'4'));
     "".find(|c| c == 'a' || matches!(c, '1'..'4'));
+    macro_rules! m {
+        ($e:expr) => {
+            $e == '?'
+        };
+    }
+    "".find(|c| m!(c));
 }
diff --git a/tests/ui/manual_pattern_char_comparison.rs b/tests/ui/manual_pattern_char_comparison.rs
@@ -18,6 +18,7 @@ fn main() {
     sentence.split(|c: char| matches!(c, '\n' | 'X' | 'Y'));
     sentence.splitn(3, |c: char| matches!(c, 'X'));
     sentence.splitn(3, |c: char| matches!(c, 'X' | 'W'));
+    sentence.find(|c| c == '🎈');
 
     let not_str = NotStr;
     not_str.find(|c: char| c == 'X');
@@ -39,4 +40,10 @@ fn main() {
 
     "".find(|c| matches!(c, 'a' | '1'..'4'));
     "".find(|c| c == 'a' || matches!(c, '1'..'4'));
+    macro_rules! m {
+        ($e:expr) => {
+            $e == '?'
+        };
+    }
+    "".find(|c| m!(c));
 }
diff --git a/tests/ui/manual_pattern_char_comparison.stderr b/tests/ui/manual_pattern_char_comparison.stderr
@@ -49,5 +49,11 @@ error: this manual char comparison can be written more succinctly
 LL |     sentence.splitn(3, |c: char| matches!(c, 'X' | 'W'));
    |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using an array of `char`: `['X', 'W']`
 
-error: aborting due to 8 previous errors
+error: this manual char comparison can be written more succinctly
+  --> tests/ui/manual_pattern_char_comparison.rs:21:19
+   |
+LL |     sentence.find(|c| c == '🎈');
+   |                   ^^^^^^^^^^^^^ help: consider using a `char`: `'🎈'`
+
+error: aborting due to 9 previous errors
 
