Lint collapsible_str_replace

CHANGELOG.md

@@ -3642,6 +3642,7 @@ Released 2018-09-13
 [`collapsible_else_if`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_else_if
 [`collapsible_if`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if
 [`collapsible_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_match
+[`collapsible_str_replace`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_str_replace
 [`comparison_chain`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_chain
 [`comparison_to_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_to_empty
 [`const_static_lifetime`]: https://rust-lang.github.io/rust-clippy/master/index.html#const_static_lifetime

clippy_lints/src/lib.register_all.rs

@@ -154,6 +154,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(methods::CHARS_NEXT_CMP),
     LintId::of(methods::CLONE_DOUBLE_REF),
     LintId::of(methods::CLONE_ON_COPY),
+    LintId::of(methods::COLLAPSIBLE_STR_REPLACE),
     LintId::of(methods::ERR_EXPECT),
     LintId::of(methods::EXPECT_FUN_CALL),
     LintId::of(methods::EXTEND_WITH_DRAIN),

clippy_lints/src/lib.register_lints.rs

@@ -287,6 +287,7 @@ store.register_lints(&[
     methods::CLONE_DOUBLE_REF,
     methods::CLONE_ON_COPY,
     methods::CLONE_ON_REF_PTR,
+    methods::COLLAPSIBLE_STR_REPLACE,
     methods::ERR_EXPECT,
     methods::EXPECT_FUN_CALL,
     methods::EXPECT_USED,

clippy_lints/src/lib.register_perf.rs

@@ -13,6 +13,7 @@ store.register_group(true, "clippy::perf", Some("clippy_perf"), vec![
     LintId::of(loops::MISSING_SPIN_LOOP),
     LintId::of(loops::NEEDLESS_COLLECT),
     LintId::of(manual_retain::MANUAL_RETAIN),
+    LintId::of(methods::COLLAPSIBLE_STR_REPLACE),
     LintId::of(methods::EXPECT_FUN_CALL),
     LintId::of(methods::EXTEND_WITH_DRAIN),
     LintId::of(methods::ITER_NTH),

clippy_lints/src/methods/collapsible_str_replace.rs

@@ -0,0 +1,96 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+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 rustc_errors::Applicability;
+use rustc_hir as hir;
+use rustc_lint::LateContext;
+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>,
+    from: &'tcx hir::Expr<'tcx>,
+    to: &'tcx hir::Expr<'tcx>,
+) {
+    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;
+        }
+
+        check_consecutive_replace_calls(cx, expr, &replace_methods, to);
+    }
+}
+
+struct ReplaceMethods<'tcx> {
+    methods: VecDeque<&'tcx hir::Expr<'tcx>>,
+    from_args: VecDeque<&'tcx hir::Expr<'tcx>>,
+}
+
+fn collect_replace_calls<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx hir::Expr<'tcx>,
+    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(("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(())
+        }
+    });
+
+    ReplaceMethods { methods, from_args }
+}
+
+/// 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,
+        );
+    }
+}

clippy_lints/src/methods/mod.rs

@@ -12,6 +12,7 @@ mod chars_next_cmp_with_unwrap;
 mod clone_on_copy;
 mod clone_on_ref_ptr;
 mod cloned_instead_of_copied;
+mod collapsible_str_replace;
 mod err_expect;
 mod expect_fun_call;
 mod expect_used;
@@ -137,6 +138,32 @@ declare_clippy_lint! {
     "used `cloned` where `copied` could be used instead"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for consecutive calls to `str::replace` (2 or more)
+    /// that can be collapsed into a single call.
+    ///
+    /// ### Why is this bad?
+    /// Consecutive `str::replace` calls scan the string multiple times
+    /// with repetitive code.
+    ///
+    /// ### Example
+    /// ```rust
+    /// let hello = "hesuo worpd"
+    ///     .replace('s', "l")
+    ///     .replace("u", "l")
+    ///     .replace('p', "l");
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// let hello = "hesuo worpd".replace(&['s', 'u', 'p'], "l");
+    /// ```
+    #[clippy::version = "1.64.0"]
+    pub COLLAPSIBLE_STR_REPLACE,
+    perf,
+    "collapse consecutive calls to str::replace (2 or more) into a single call"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for usage of `_.cloned().<func>()` where call to `.cloned()` can be postponed.
@@ -3001,6 +3028,7 @@ impl_lint_pass!(Methods => [
     CLONE_ON_COPY,
     CLONE_ON_REF_PTR,
     CLONE_DOUBLE_REF,
+    COLLAPSIBLE_STR_REPLACE,
     ITER_OVEREAGER_CLONED,
     CLONED_INSTEAD_OF_COPIED,
     FLAT_MAP_OPTION,
@@ -3479,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);
                 },
@@ -3556,9 +3592,6 @@ impl Methods {
                         unnecessary_lazy_eval::check(cx, expr, recv, u_arg, "unwrap_or");
                     },
                 },
-                ("replace" | "replacen", [arg1, arg2] | [arg1, arg2, _]) => {
-                    no_effect_replace::check(cx, expr, arg1, arg2);
-                },
                 ("zip", [arg]) => {
                     if let ExprKind::MethodCall(name, [iter_recv], _) = recv.kind
                         && name.ident.name == sym::iter

tests/ui/collapsible_str_replace.fixed

@@ -0,0 +1,73 @@
+// run-rustfix
+
+#![warn(clippy::collapsible_str_replace)]
+
+fn get_filter() -> char {
+    'u'
+}
+
+fn main() {
+    let d = 'd';
+    let p = 'p';
+    let s = 's';
+    let u = 'u';
+    let l = "l";
+
+    let mut iter = ["l", "z"].iter();
+
+    // LINT CASES
+    let _ = "hesuo worpd".replace(['s', 'u'], "l");
+
+    let _ = "hesuo worpd".replace(['s', 'u'], l);
+
+    let _ = "hesuo worpd".replace(['s', 'u', 'p'], "l");
+
+    let _ = "hesuo worpd"
+        .replace(['s', 'u', 'p', 'd'], "l");
+
+    let _ = "hesuo world".replace([s, 'u'], "l");
+
+    let _ = "hesuo worpd".replace([s, 'u', 'p'], "l");
+
+    let _ = "hesuo worpd".replace([s, u, 'p'], "l");
+
+    let _ = "hesuo worpd".replace([s, u, p], "l");
+
+    let _ = "hesuo worlp".replace(['s', 'u'], "l").replace('p', "d");
+
+    let _ = "hesuo worpd".replace('s', "x").replace(['u', 'p'], "l");
+
+    // Note: Future iterations could lint `replace(|c| matches!(c, "su" | 'd' | 'p'), "l")`
+    let _ = "hesudo worpd".replace("su", "l").replace(['d', 'p'], "l");
+
+    let _ = "hesudo worpd".replace([d, 'p'], "l").replace("su", "l");
+
+    let _ = "hesuo world".replace([get_filter(), 's'], "l");
+
+    // NO LINT CASES
+    let _ = "hesuo world".replace('s', "l").replace('u', "p");
+
+    let _ = "hesuo worpd".replace('s', "l").replace('p', l);
+
+    let _ = "hesudo worpd".replace('d', "l").replace("su", "l").replace('p', "l");
+
+    // Note: Future iterations of `collapsible_str_replace` might lint this and combine to `[s, u, p]`
+    let _ = "hesuo worpd".replace([s, u], "l").replace([u, p], "l");
+
+    let _ = "hesuo worpd".replace(['s', 'u'], "l").replace(['u', 'p'], "l");
+
+    let _ = "hesuo worpd".replace('s', "l").replace(['u', 'p'], "l");
+
+    let _ = "hesuo worpd".replace(['s', 'u', 'p'], "l").replace('r', "l");
+
+    let _ = "hesuo worpd".replace(['s', 'u', 'p'], l).replace('r', l);
+
+    let _ = "hesuo worpd".replace(['s', u, 'p'], "l").replace('r', "l");
+
+    let _ = "hesuo worpd".replace([s, u], "l").replace(p, "l");
+
+    // Regression test
+    let _ = "hesuo worpd"
+        .replace('u', iter.next().unwrap())
+        .replace('s', iter.next().unwrap());
+}

tests/ui/collapsible_str_replace.rs

@@ -0,0 +1,76 @@
+// run-rustfix
+
+#![warn(clippy::collapsible_str_replace)]
+
+fn get_filter() -> char {
+    'u'
+}
+
+fn main() {
+    let d = 'd';
+    let p = 'p';
+    let s = 's';
+    let u = 'u';
+    let l = "l";
+
+    let mut iter = ["l", "z"].iter();
+
+    // LINT CASES
+    let _ = "hesuo worpd".replace('s', "l").replace('u', "l");
+
+    let _ = "hesuo worpd".replace('s', l).replace('u', l);
+
+    let _ = "hesuo worpd".replace('s', "l").replace('u', "l").replace('p', "l");
+
+    let _ = "hesuo worpd"
+        .replace('s', "l")
+        .replace('u', "l")
+        .replace('p', "l")
+        .replace('d', "l");
+
+    let _ = "hesuo world".replace(s, "l").replace('u', "l");
+
+    let _ = "hesuo worpd".replace(s, "l").replace('u', "l").replace('p', "l");
+
+    let _ = "hesuo worpd".replace(s, "l").replace(u, "l").replace('p', "l");
+
+    let _ = "hesuo worpd".replace(s, "l").replace(u, "l").replace(p, "l");
+
+    let _ = "hesuo worlp".replace('s', "l").replace('u', "l").replace('p', "d");
+
+    let _ = "hesuo worpd".replace('s', "x").replace('u', "l").replace('p', "l");
+
+    // Note: Future iterations could lint `replace(|c| matches!(c, "su" | 'd' | 'p'), "l")`
+    let _ = "hesudo worpd".replace("su", "l").replace('d', "l").replace('p', "l");
+
+    let _ = "hesudo worpd".replace(d, "l").replace('p', "l").replace("su", "l");
+
+    let _ = "hesuo world".replace(get_filter(), "l").replace('s', "l");
+
+    // NO LINT CASES
+    let _ = "hesuo world".replace('s', "l").replace('u', "p");
+
+    let _ = "hesuo worpd".replace('s', "l").replace('p', l);
+
+    let _ = "hesudo worpd".replace('d', "l").replace("su", "l").replace('p', "l");
+
+    // Note: Future iterations of `collapsible_str_replace` might lint this and combine to `[s, u, p]`
+    let _ = "hesuo worpd".replace([s, u], "l").replace([u, p], "l");
+
+    let _ = "hesuo worpd".replace(['s', 'u'], "l").replace(['u', 'p'], "l");
+
+    let _ = "hesuo worpd".replace('s', "l").replace(['u', 'p'], "l");
+
+    let _ = "hesuo worpd".replace(['s', 'u', 'p'], "l").replace('r', "l");
+
+    let _ = "hesuo worpd".replace(['s', 'u', 'p'], l).replace('r', l);
+
+    let _ = "hesuo worpd".replace(['s', u, 'p'], "l").replace('r', "l");
+
+    let _ = "hesuo worpd".replace([s, u], "l").replace(p, "l");
+
+    // Regression test
+    let _ = "hesuo worpd"
+        .replace('u', iter.next().unwrap())
+        .replace('s', iter.next().unwrap());
+}