Be more restrictive and use more clippy utils on lint manual_pattern_char_comparison.

AurelienFT · AurelienFT · commit db89f1d6eeef · 2024-05-28T11:29:05.000+02:00
Move `str_literal_to_char_literal` util function to source.rs
diff --git a/clippy_lints/src/methods/single_char_insert_string.rs b/clippy_lints/src/methods/single_char_insert_string.rs
@@ -1,6 +1,5 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::source::snippet_with_applicability;
-use clippy_utils::str_utils::get_hint_if_single_char_arg;
+use clippy_utils::source::{snippet_with_applicability, str_literal_to_char_literal};
 use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_lint::LateContext;
@@ -10,7 +9,7 @@ use super::SINGLE_CHAR_ADD_STR;
 /// lint for length-1 `str`s as argument for `insert_str`
 pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, receiver: &hir::Expr<'_>, args: &[hir::Expr<'_>]) {
     let mut applicability = Applicability::MachineApplicable;
-    if let Some(extension_string) = get_hint_if_single_char_arg(cx, &args[1], &mut applicability, false) {
+    if let Some(extension_string) = str_literal_to_char_literal(cx, &args[1], &mut applicability, false) {
         let base_string_snippet =
             snippet_with_applicability(cx, receiver.span.source_callsite(), "_", &mut applicability);
         let pos_arg = snippet_with_applicability(cx, args[0].span, "..", &mut applicability);
diff --git a/clippy_lints/src/methods/single_char_push_string.rs b/clippy_lints/src/methods/single_char_push_string.rs
@@ -1,6 +1,5 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::source::snippet_with_applicability;
-use clippy_utils::str_utils::get_hint_if_single_char_arg;
+use clippy_utils::source::{snippet_with_applicability, str_literal_to_char_literal};
 use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_lint::LateContext;
@@ -10,7 +9,7 @@ use super::SINGLE_CHAR_ADD_STR;
 /// lint for length-1 `str`s as argument for `push_str`
 pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, receiver: &hir::Expr<'_>, args: &[hir::Expr<'_>]) {
     let mut applicability = Applicability::MachineApplicable;
-    if let Some(extension_string) = get_hint_if_single_char_arg(cx, &args[0], &mut applicability, false) {
+    if let Some(extension_string) = str_literal_to_char_literal(cx, &args[0], &mut applicability, false) {
         let base_string_snippet =
             snippet_with_applicability(cx, receiver.span.source_callsite(), "..", &mut applicability);
         let sugg = format!("{base_string_snippet}.push({extension_string})");
diff --git a/clippy_lints/src/string_patterns.rs b/clippy_lints/src/string_patterns.rs
@@ -2,15 +2,17 @@ use std::cmp::Ordering;
 use std::ops::ControlFlow;
 
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::source::snippet;
-use clippy_utils::str_utils::get_hint_if_single_char_arg;
+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::visitors::{for_each_expr, Descend};
 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;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -93,95 +95,110 @@ const PATTERN_METHODS: [(&str, usize); 22] = [
     ("replacen", 0),
 ];
 
-fn check_single_char_pattern_lint(cx: &LateContext<'_>, arg: &Expr<'_>, applicability: &mut Applicability) {
-    if let Some(hint) = get_hint_if_single_char_arg(cx, arg, applicability, true) {
+fn check_single_char_pattern_lint(cx: &LateContext<'_>, arg: &Expr<'_>) {
+    let mut applicability = Applicability::MachineApplicable;
+    if let Some(hint) = str_literal_to_char_literal(cx, arg, &mut applicability, true) {
         span_lint_and_sugg(
             cx,
             SINGLE_CHAR_PATTERN,
             arg.span,
             "single-character string constant used as pattern",
             "consider using a `char`",
             hint,
-            *applicability,
+            applicability,
         );
     }
 }
 
-fn check_manual_pattern_char_comparison(cx: &LateContext<'_>, arg: &Expr<'_>, applicability: Applicability) {
-    if let ExprKind::Closure(closure) = arg.kind
+fn get_char_value(expr: &Expr<'_>) -> Option<String> {
+    match expr.kind {
+        ExprKind::Lit(lit) => match lit.node {
+            LitKind::Char(c) => Some(format!("'{}'", c.escape_default())),
+            _ => None,
+        },
+        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())
+                } else {
+                    None
+                }
+            } else {
+                None
+            }
+        },
+        _ => None,
+    }
+}
+
+fn check_manual_pattern_char_comparison(cx: &LateContext<'_>, method_arg: &Expr<'_>) {
+    let applicability = Applicability::MachineApplicable;
+    if let ExprKind::Closure(closure) = method_arg.kind
         && let body = cx.tcx.hir().body(closure.body)
-        && let Some(param_name) = body.params[0].pat.simple_ident()
+        && let PatKind::Binding(_, binding, ..) = body.params[0].pat.kind
     {
-        let ex = body.value;
         let mut set_chars: Vec<String> = 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.
-        for_each_expr(ex, |sub_expr| -> ControlFlow<(), Descend> {
+        if for_each_expr(body.value, |sub_expr| -> ControlFlow<(), Descend> {
             match sub_expr.kind {
-                ExprKind::Binary(op, left, right) => {
-                    // Avoid matching code that contains complex comparisons such as functions calls etc..
-                    // (ex: c.is_whitespace())
-                    if matches!(
-                        left.kind,
-                        ExprKind::Binary(_, _, _) | ExprKind::Match(_, _, _) | ExprKind::Lit(_) | ExprKind::Path(_)
-                    ) && matches!(
-                        right.kind,
-                        ExprKind::Binary(_, _, _) | ExprKind::Match(_, _, _) | ExprKind::Lit(_) | ExprKind::Path(_)
-                    ) {
-                        if matches!(op.node, BinOpKind::Eq | BinOpKind::Or) {
-                            ControlFlow::Continue(Descend::Yes)
-                        } else {
-                            ControlFlow::Continue(Descend::No)
-                        }
+                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)
+                    {
+                        set_chars.push(c);
+                        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)
+                    {
+                        set_chars.push(c);
+                        ControlFlow::Continue(Descend::No)
                     } else {
                         ControlFlow::Break(())
                     }
                 },
-                ExprKind::Match(match_value, arms, _) => {
-                    if snippet(cx, match_value.span, "") != param_name.name.as_str() || arms.len() != 2 {
+                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 let PatKind::Or(pats) = arms[0].pat.kind {
-                        for pat in pats {
-                            if let PatKind::Lit(expr) = pat.kind
-                                && let ExprKind::Lit(lit) = expr.kind
-                                && let LitKind::Char(c) = lit.node
-                            {
-                                set_chars.push(format!("'{}'", c.escape_default()));
-                            }
-                        }
-                    } else if let PatKind::Lit(expr) = arms[0].pat.kind
-                        && let ExprKind::Lit(lit) = expr.kind
-                        && let LitKind::Char(c) = lit.node
-                    {
-                        set_chars.push(format!("'{}'", c.escape_default()));
+                    if arm.guard.is_some() {
+                        return ControlFlow::Break(());
                     }
-                    ControlFlow::Continue(Descend::No)
-                },
-                ExprKind::Lit(lit) => {
-                    if let LitKind::Char(c) = lit.node {
-                        set_chars.push(format!("'{}'", c.escape_default()));
+                    if !path_to_local_id(match_value, binding) {
+                        return ControlFlow::Break(());
                     }
-                    ControlFlow::Continue(Descend::No)
-                },
-                ExprKind::Path(QPath::Resolved(_, path)) => {
-                    if path.segments.len() == 1 {
-                        let segment = &path.segments[0];
-                        if segment.ident != param_name {
-                            set_chars.push(segment.ident.to_string());
-                        }
+                    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()));
+                            }
+                            true
+                        },
+                        PatKind::Or(_) => true,
+                        _ => false,
+                    }) {
+                        ControlFlow::Continue(Descend::No)
+                    } else {
+                        ControlFlow::Break(())
                     }
-                    ControlFlow::Continue(Descend::No)
                 },
-                _ => ControlFlow::Continue(Descend::No),
+                _ => ControlFlow::Break(()),
             }
-        });
+        })
+        .is_some()
+        {
+            return;
+        }
         match set_chars.len().cmp(&1) {
             Ordering::Equal => span_lint_and_sugg(
                 cx,
                 MANUAL_PATTERN_CHAR_COMPARISON,
-                arg.span,
+                method_arg.span,
                 "this manual char comparison can be written more succinctly",
                 "consider using a `char`",
                 set_chars[0].clone(),
@@ -190,7 +207,7 @@ fn check_manual_pattern_char_comparison(cx: &LateContext<'_>, arg: &Expr<'_>, ap
             Ordering::Greater => span_lint_and_sugg(
                 cx,
                 MANUAL_PATTERN_CHAR_COMPARISON,
-                arg.span,
+                method_arg.span,
                 "this manual char comparison can be written more succinctly",
                 "consider using an array of `char`",
                 format!("[{}]", set_chars.join(", ")),
@@ -214,10 +231,9 @@ impl<'tcx> LateLintPass<'tcx> for StringPatterns {
             && args.len() > pos
         {
             let arg = &args[pos];
-            let mut applicability = Applicability::MachineApplicable;
-            check_single_char_pattern_lint(cx, arg, &mut applicability);
+            check_single_char_pattern_lint(cx, arg);
 
-            check_manual_pattern_char_comparison(cx, arg, applicability);
+            check_manual_pattern_char_comparison(cx, arg);
         }
     }
 }
diff --git a/clippy_utils/src/source.rs b/clippy_utils/src/source.rs
@@ -2,6 +2,7 @@
 
 #![allow(clippy::module_name_repetitions)]
 
+use rustc_ast::{LitKind, StrStyle};
 use rustc_data_structures::sync::Lrc;
 use rustc_errors::Applicability;
 use rustc_hir::{BlockCheckMode, Expr, ExprKind, UnsafeSource};
@@ -500,6 +501,50 @@ pub fn expand_past_previous_comma(cx: &LateContext<'_>, span: Span) -> Span {
     extended.with_lo(extended.lo() - BytePos(1))
 }
 
+/// Converts `expr` to a `char` literal if it's a `str` literal containing a single
+/// character (or a single byte with `ascii_only`)
+pub fn str_literal_to_char_literal(
+    cx: &LateContext<'_>,
+    expr: &Expr<'_>,
+    applicability: &mut Applicability,
+    ascii_only: bool,
+) -> Option<String> {
+    if let ExprKind::Lit(lit) = &expr.kind
+        && let LitKind::Str(r, style) = lit.node
+        && let string = r.as_str()
+        && let len = if ascii_only {
+            string.len()
+        } else {
+            string.chars().count()
+        }
+        && len == 1
+    {
+        let snip = snippet_with_applicability(cx, expr.span, string, applicability);
+        let ch = if let StrStyle::Raw(nhash) = style {
+            let nhash = nhash as usize;
+            // for raw string: r##"a"##
+            &snip[(nhash + 2)..(snip.len() - 1 - nhash)]
+        } else {
+            // for regular string: "a"
+            &snip[1..(snip.len() - 1)]
+        };
+
+        let hint = format!(
+            "'{}'",
+            match ch {
+                "'" => "\\'",
+                r"\" => "\\\\",
+                "\\\"" => "\"", // no need to escape `"` in `'"'`
+                _ => ch,
+            }
+        );
+
+        Some(hint)
+    } else {
+        None
+    }
+}
+
 #[cfg(test)]
 mod test {
     use super::{reindent_multiline, without_block_comments};
diff --git a/clippy_utils/src/str_utils.rs b/clippy_utils/src/str_utils.rs
@@ -1,10 +1,3 @@
-use rustc_ast::{LitKind, StrStyle};
-use rustc_errors::Applicability;
-use rustc_hir::{Expr, ExprKind};
-use rustc_lint::LateContext;
-
-use crate::source::snippet_with_applicability;
-
 /// Dealing with sting indices can be hard, this struct ensures that both the
 /// character and byte index are provided for correct indexing.
 #[derive(Debug, Default, PartialEq, Eq)]
@@ -296,49 +289,6 @@ pub fn to_camel_case(item_name: &str) -> String {
     s
 }
 
-/// Return an hint if the `arg` is a string with a single character
-pub fn get_hint_if_single_char_arg(
-    cx: &LateContext<'_>,
-    arg: &Expr<'_>,
-    applicability: &mut Applicability,
-    ascii_only: bool,
-) -> Option<String> {
-    if let ExprKind::Lit(lit) = &arg.kind
-        && let LitKind::Str(r, style) = lit.node
-        && let string = r.as_str()
-        && let len = if ascii_only {
-            string.len()
-        } else {
-            string.chars().count()
-        }
-        && len == 1
-    {
-        let snip = snippet_with_applicability(cx, arg.span, string, applicability);
-        let ch = if let StrStyle::Raw(nhash) = style {
-            let nhash = nhash as usize;
-            // for raw string: r##"a"##
-            &snip[(nhash + 2)..(snip.len() - 1 - nhash)]
-        } else {
-            // for regular string: "a"
-            &snip[1..(snip.len() - 1)]
-        };
-
-        let hint = format!(
-            "'{}'",
-            match ch {
-                "'" => "\\'",
-                r"\" => "\\\\",
-                "\\\"" => "\"", // no need to escape `"` in `'"'`
-                _ => ch,
-            }
-        );
-
-        Some(hint)
-    } else {
-        None
-    }
-}
-
 #[cfg(test)]
 mod test {
     use super::*;
diff --git a/tests/ui/manual_pattern_char_comparison.fixed b/tests/ui/manual_pattern_char_comparison.fixed
@@ -21,4 +21,22 @@ fn main() {
 
     let not_str = NotStr;
     not_str.find(|c: char| c == 'X');
+
+    "".find(|c| c == 'a' || c > 'z');
+
+    let x = true;
+    "".find(|c| c == 'a' || x || c == 'b');
+
+    let d = 'd';
+    "".find(|c| c == 'a' || d == 'b');
+
+    "".find(|c| match c {
+        'a' | 'b' => true,
+        _ => c.is_ascii(),
+    });
+
+    "".find(|c| matches!(c, 'a' | 'b' if false));
+
+    "".find(|c| matches!(c, 'a' | '1'..'4'));
+    "".find(|c| c == 'a' || matches!(c, '1'..'4'));
 }
diff --git a/tests/ui/manual_pattern_char_comparison.rs b/tests/ui/manual_pattern_char_comparison.rs
