Simplify lint logic and address code review comments

Author: nahuakang

clippy_lints/src/methods/collapsible_str_replace.rs

@@ -1,275 +1,96 @@
-// run-rustfix
-
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::get_parent_expr;
+use clippy_utils::source::snippet;
 use clippy_utils::visitors::for_each_expr;
+use clippy_utils::{eq_expr_value, get_parent_expr};
 use core::ops::ControlFlow;
-use if_chain::if_chain;
-use rustc_ast::ast::LitKind;
-use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
-use rustc_hir::{ExprKind, Path, QPath};
 use rustc_lint::LateContext;
-use rustc_middle::ty;
-use rustc_span::source_map::Spanned;
-use rustc_span::Span;
+use std::collections::VecDeque;
 
 use super::method_call;
 use super::COLLAPSIBLE_STR_REPLACE;
 
 pub(super) fn check<'tcx>(
     cx: &LateContext<'tcx>,
     expr: &'tcx hir::Expr<'tcx>,
-    name: &str,
-    recv: &'tcx hir::Expr<'tcx>,
+    from: &'tcx hir::Expr<'tcx>,
+    to: &'tcx hir::Expr<'tcx>,
 ) {
-    if name == "replace" {
-        // The receiver of the method call must be `str` type to lint `collapsible_str_replace`
-        let original_recv = find_original_recv(recv);
-        let original_recv_ty_kind = cx.typeck_results().expr_ty(original_recv).peel_refs().kind();
-        let original_recv_is_str_kind = matches!(original_recv_ty_kind, ty::Str);
-
-        if_chain! {
-            if original_recv_is_str_kind;
-            if let Some(parent) = get_parent_expr(cx, expr);
-            if let Some((name, ..)) = method_call(parent);
-            if name == "replace";
-
-            then {
-                // If the parent node is a `str::replace` call, we've already handled the lint, don't lint again
-                return;
-            }
+    let replace_methods = collect_replace_calls(cx, expr, to);
+    if replace_methods.methods.len() > 1 {
+        let from_kind = cx.typeck_results().expr_ty(from).peel_refs().kind();
+        // If the parent node's `to` argument is the same as the `to` argument
+        // of the last replace call in the current chain, don't lint as it was already linted
+        if let Some(parent) = get_parent_expr(cx, expr)
+            && let Some(("replace", [_, current_from, current_to], _)) = method_call(parent)
+            && eq_expr_value(cx, to, current_to)
+            && from_kind == cx.typeck_results().expr_ty(current_from).peel_refs().kind()
+        {
+            return;
         }
 
-        if let Some(("replace", ..)) = method_call(recv) {
-            // Check if there's an earlier `str::replace` call
-            if original_recv_is_str_kind {
-                check_consecutive_replace_calls(cx, expr);
-            }
-        }
+        check_consecutive_replace_calls(cx, expr, &replace_methods, to);
     }
 }
 
-/// Check a chain of `str::replace` calls for `collapsible_str_replace` lint.
-fn check_consecutive_replace_calls<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'tcx>) {
-    if_chain! {
-        if let Some(from_args) = get_replace_call_from_args_if_all_char_ty(cx, expr);
-        if let Some(to_arg) = get_replace_call_unique_to_arg_repr(expr);
-        then {
-            let earliest_replace_call_span = get_earliest_replace_call_span(expr);
-
-            if replace_call_from_args_are_only_lit_chars(&from_args) {
-                let from_arg_reprs: Vec<String> = from_args.iter().map(|from_arg| {
-                    get_replace_call_char_arg_repr(from_arg).unwrap()
-                }).collect();
-                let app = Applicability::MachineApplicable;
-
-                span_lint_and_sugg(
-                    cx,
-                    COLLAPSIBLE_STR_REPLACE,
-                    expr.span.with_lo(earliest_replace_call_span.lo()),
-                    "used consecutive `str::replace` call",
-                    "replace with",
-                    format!(
-                        "replace(|c| matches!(c, {}), {})",
-                        from_arg_reprs.join(" | "),
-                        to_arg,
-                    ),
-                    app,
-                );
-            } else {
-                // Use fallback lint
-                let from_arg_reprs: Vec<String> = from_args.iter().map(|from_arg| {
-                    get_replace_call_char_arg_repr(from_arg).unwrap()
-                }).collect();
-                let app = Applicability::MachineApplicable;
-
-                span_lint_and_sugg(
-                    cx,
-                    COLLAPSIBLE_STR_REPLACE,
-                    expr.span.with_lo(earliest_replace_call_span.lo()),
-                    "used consecutive `str::replace` call",
-                    "replace with",
-                    format!(
-                        "replace(&[{}], {})",
-                        from_arg_reprs.join(" , "),
-                        to_arg,
-                    ),
-                    app,
-                );
-            }
-        }
-    }
+struct ReplaceMethods<'tcx> {
+    methods: VecDeque<&'tcx hir::Expr<'tcx>>,
+    from_args: VecDeque<&'tcx hir::Expr<'tcx>>,
 }
 
-/// Check if all the `from` arguments of a chain of consecutive calls to `str::replace`
-/// are all of `ExprKind::Lit` types. If any is not, return false.
-fn replace_call_from_args_are_only_lit_chars<'tcx>(from_args: &[&'tcx hir::Expr<'tcx>]) -> bool {
-    let mut only_lit_chars = true;
-
-    for from_arg in from_args.iter() {
-        match from_arg.kind {
-            ExprKind::Lit(..) => {},
-            _ => only_lit_chars = false,
-        }
-    }
-
-    only_lit_chars
-}
-
-/// Collect and return all of the `from` arguments of a chain of consecutive `str::replace` calls
-/// if these `from` arguments's expressions are of the `ty::Char` kind. Otherwise return `None`.
-fn get_replace_call_from_args_if_all_char_ty<'tcx>(
+fn collect_replace_calls<'tcx>(
     cx: &LateContext<'tcx>,
     expr: &'tcx hir::Expr<'tcx>,
-) -> Option<Vec<&'tcx hir::Expr<'tcx>>> {
-    let mut all_from_args_are_chars = true;
-    let mut from_args = Vec::new();
+    to_arg: &'tcx hir::Expr<'tcx>,
+) -> ReplaceMethods<'tcx> {
+    let mut methods = VecDeque::new();
+    let mut from_args = VecDeque::new();
 
     let _: Option<()> = for_each_expr(expr, |e| {
-        if let Some((name, [_, args @ ..], _)) = method_call(e) {
-            match (name, args) {
-                ("replace", [from, _]) => {
-                    let from_ty_kind = cx.typeck_results().expr_ty(from).peel_refs().kind();
-                    if matches!(from_ty_kind, ty::Char) {
-                        from_args.push(from);
-                    } else {
-                        all_from_args_are_chars = false;
-                    }
-                    ControlFlow::Continue(())
-                },
-                _ => ControlFlow::BREAK,
-            }
-        } else {
-            ControlFlow::Continue(())
-        }
-    });
-
-    if all_from_args_are_chars {
-        return Some(from_args);
-    }
-
-    None
-}
-
-/// Return a unique String representation of the `to` argument used in a chain of `str::replace`
-/// calls if each `str::replace` call's `to` argument is identical to the other `to` arguments in
-/// the chain. Otherwise, return `None`.
-fn get_replace_call_unique_to_arg_repr<'tcx>(expr: &'tcx hir::Expr<'tcx>) -> Option<String> {
-    let mut to_args = Vec::new();
-
-    let _: Option<()> = for_each_expr(expr, |e| {
-        if let Some((name, [_, args @ ..], _)) = method_call(e) {
-            match (name, args) {
-                ("replace", [_, to]) => {
-                    to_args.push(to);
-                    ControlFlow::Continue(())
-                },
-                _ => ControlFlow::BREAK,
+        if let Some(("replace", [_, from, to], _)) = method_call(e) {
+            if eq_expr_value(cx, to_arg, to) && cx.typeck_results().expr_ty(from).peel_refs().is_char() {
+                methods.push_front(e);
+                from_args.push_front(from);
+                ControlFlow::Continue(())
+            } else {
+                ControlFlow::BREAK
             }
         } else {
             ControlFlow::Continue(())
         }
     });
 
-    // let mut to_arg_repr_set = FxHashSet::default();
-    let mut to_arg_reprs = Vec::new();
-    for &to_arg in &to_args {
-        if let Some(to_arg_repr) = get_replace_call_char_arg_repr(to_arg) {
-            to_arg_reprs.push(to_arg_repr);
-        }
-    }
-
-    let to_arg_repr_set = to_arg_reprs.iter().cloned().collect::<FxHashSet<_>>();
-    // Check if the set of `to` argument representations has more than one unique value
-    if to_arg_repr_set.len() != 1 {
-        return None;
-    }
-
-    // Return the single representation value
-    to_arg_reprs.pop()
+    ReplaceMethods { methods, from_args }
 }
 
-/// Get the representation of an argument of a `str::replace` call either of the literal char value
-/// or variable name, i.e. the resolved path segments `ident`.
-/// Return:
-/// - the str literal with double quotes, e.g. "\"l\""
-/// - the char literal with single quotes, e.g. "'l'"
-/// - the variable as a String, e.g. "l"
-fn get_replace_call_char_arg_repr<'tcx>(arg: &'tcx hir::Expr<'tcx>) -> Option<String> {
-    match arg.kind {
-        ExprKind::Lit(Spanned {
-            node: LitKind::Str(to_arg_val, _),
-            ..
-        }) => {
-            let repr = to_arg_val.as_str();
-            let double_quote = "\"";
-            Some(double_quote.to_owned() + repr + double_quote)
-        },
-        ExprKind::Lit(Spanned {
-            node: LitKind::Char(to_arg_val),
-            ..
-        }) => {
-            let repr = to_arg_val.to_string();
-            let double_quote = "\'";
-            Some(double_quote.to_owned() + &repr + double_quote)
-        },
-        ExprKind::Path(QPath::Resolved(
-            _,
-            Path {
-                segments: path_segments,
-                ..
-            },
-        )) => {
-            // join the path_segments values by "::"
-            let path_segment_ident_names: Vec<&str> = path_segments
-                .iter()
-                .map(|path_seg| path_seg.ident.name.as_str())
-                .collect();
-            Some(path_segment_ident_names.join("::"))
-        },
-        _ => None,
+/// Check a chain of `str::replace` calls for `collapsible_str_replace` lint.
+fn check_consecutive_replace_calls<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx hir::Expr<'tcx>,
+    replace_methods: &ReplaceMethods<'tcx>,
+    to_arg: &'tcx hir::Expr<'tcx>,
+) {
+    let from_args = &replace_methods.from_args;
+    let from_arg_reprs: Vec<String> = from_args
+        .iter()
+        .map(|from_arg| snippet(cx, from_arg.span, "..").to_string())
+        .collect();
+    let app = Applicability::MachineApplicable;
+    let earliest_replace_call = replace_methods.methods.front().unwrap();
+    if let Some((_, [..], span_lo)) = method_call(earliest_replace_call) {
+        span_lint_and_sugg(
+            cx,
+            COLLAPSIBLE_STR_REPLACE,
+            expr.span.with_lo(span_lo.lo()),
+            "used consecutive `str::replace` call",
+            "replace with",
+            format!(
+                "replace([{}], {})",
+                from_arg_reprs.join(", "),
+                snippet(cx, to_arg.span, ".."),
+            ),
+            app,
+        );
     }
 }
-
-fn get_earliest_replace_call_span<'tcx>(expr: &'tcx hir::Expr<'tcx>) -> Span {
-    let mut earliest_replace_call_span = expr.span;
-
-    let _: Option<()> = for_each_expr(expr, |e| {
-        if let Some((name, [_, args @ ..], span)) = method_call(e) {
-            match (name, args) {
-                ("replace", [_, _]) => {
-                    earliest_replace_call_span = span;
-                    ControlFlow::Continue(())
-                },
-                _ => ControlFlow::BREAK,
-            }
-        } else {
-            ControlFlow::Continue(())
-        }
-    });
-
-    earliest_replace_call_span
-}
-
-/// Find the original receiver of a chain of `str::replace` method calls.
-fn find_original_recv<'tcx>(recv: &'tcx hir::Expr<'tcx>) -> &'tcx hir::Expr<'tcx> {
-    let mut original_recv = recv;
-
-    let _: Option<()> = for_each_expr(recv, |e| {
-        if let Some((name, [prev_recv, args @ ..], _)) = method_call(e) {
-            match (name, args) {
-                ("replace", [_, _]) => {
-                    original_recv = prev_recv;
-                    ControlFlow::Continue(())
-                },
-                _ => ControlFlow::BREAK,
-            }
-        } else {
-            ControlFlow::Continue(())
-        }
-    });
-
-    original_recv
-}

clippy_lints/src/methods/mod.rs

@@ -156,7 +156,7 @@ declare_clippy_lint! {
     /// ```
     /// Use instead:
     /// ```rust
-    /// let hello = "hesuo worpd".replace(|c| matches!(c, 's' | 'u' | 'p'), "l");
+    /// let hello = "hesuo worpd".replace(&['s', 'u', 'p'], "l");
     /// ```
     #[clippy::version = "1.64.0"]
     pub COLLAPSIBLE_STR_REPLACE,
@@ -3507,6 +3507,14 @@ impl Methods {
                 ("repeat", [arg]) => {
                     repeat_once::check(cx, expr, recv, arg);
                 },
+                (name @ ("replace" | "replacen"), [arg1, arg2] | [arg1, arg2, _]) => {
+                    no_effect_replace::check(cx, expr, arg1, arg2);
+
+                    // Check for repeated `str::replace` calls to perform `collapsible_str_replace` lint
+                    if name == "replace" && let Some(("replace", ..)) = method_call(recv) {
+                        collapsible_str_replace::check(cx, expr, arg1, arg2);
+                    }
+                },
                 ("resize", [count_arg, default_arg]) => {
                     vec_resize_to_zero::check(cx, expr, count_arg, default_arg, span);
                 },
@@ -3519,10 +3527,6 @@ impl Methods {
                 ("sort_unstable_by", [arg]) => {
                     unnecessary_sort_by::check(cx, expr, recv, arg, true);
                 },
-                ("replace" | "replacen", [arg1, arg2] | [arg1, arg2, _]) => {
-                    no_effect_replace::check(cx, expr, arg1, arg2);
-                    collapsible_str_replace::check(cx, expr, name, recv);
-                },
                 ("splitn" | "rsplitn", [count_arg, pat_arg]) => {
                     if let Some((Constant::Int(count), _)) = constant(cx, cx.typeck_results(), count_arg) {
                         suspicious_splitn::check(cx, name, expr, recv, count);