diff --git a/CHANGELOG.md b/CHANGELOG.md index edd7bc250a75..6c4a0cc91aad 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index fefac7632d8e..65eda4299899 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/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), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index db475950086d..e64a473d068e 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/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, diff --git a/clippy_lints/src/lib.register_perf.rs b/clippy_lints/src/lib.register_perf.rs index e1b90acb93c2..531fc47f8fac 100644 --- a/clippy_lints/src/lib.register_perf.rs +++ b/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), diff --git a/clippy_lints/src/methods/collapsible_str_replace.rs b/clippy_lints/src/methods/collapsible_str_replace.rs new file mode 100644 index 000000000000..561033be5b6a --- /dev/null +++ b/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 = 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, + ); + } +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 1cfe8c4191ef..49d4900295fd 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/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().()` 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 diff --git a/tests/ui/collapsible_str_replace.fixed b/tests/ui/collapsible_str_replace.fixed new file mode 100644 index 000000000000..49fc9a9629e2 --- /dev/null +++ b/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()); +} diff --git a/tests/ui/collapsible_str_replace.rs b/tests/ui/collapsible_str_replace.rs new file mode 100644 index 000000000000..e3e25c4146ff --- /dev/null +++ b/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()); +} diff --git a/tests/ui/collapsible_str_replace.stderr b/tests/ui/collapsible_str_replace.stderr new file mode 100644 index 000000000000..8e3daf3b898a --- /dev/null +++ b/tests/ui/collapsible_str_replace.stderr @@ -0,0 +1,86 @@ +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:19:27 + | +LL | let _ = "hesuo worpd".replace('s', "l").replace('u', "l"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(['s', 'u'], "l")` + | + = note: `-D clippy::collapsible-str-replace` implied by `-D warnings` + +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:21:27 + | +LL | let _ = "hesuo worpd".replace('s', l).replace('u', l); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(['s', 'u'], l)` + +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:23:27 + | +LL | let _ = "hesuo worpd".replace('s', "l").replace('u', "l").replace('p', "l"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(['s', 'u', 'p'], "l")` + +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:26:10 + | +LL | .replace('s', "l") + | __________^ +LL | | .replace('u', "l") +LL | | .replace('p', "l") +LL | | .replace('d', "l"); + | |__________________________^ help: replace with: `replace(['s', 'u', 'p', 'd'], "l")` + +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:31:27 + | +LL | let _ = "hesuo world".replace(s, "l").replace('u', "l"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace([s, 'u'], "l")` + +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:33:27 + | +LL | let _ = "hesuo worpd".replace(s, "l").replace('u', "l").replace('p', "l"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace([s, 'u', 'p'], "l")` + +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:35:27 + | +LL | let _ = "hesuo worpd".replace(s, "l").replace(u, "l").replace('p', "l"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace([s, u, 'p'], "l")` + +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:37:27 + | +LL | let _ = "hesuo worpd".replace(s, "l").replace(u, "l").replace(p, "l"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace([s, u, p], "l")` + +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:39:27 + | +LL | let _ = "hesuo worlp".replace('s', "l").replace('u', "l").replace('p', "d"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(['s', 'u'], "l")` + +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:41:45 + | +LL | let _ = "hesuo worpd".replace('s', "x").replace('u', "l").replace('p', "l"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(['u', 'p'], "l")` + +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:44:47 + | +LL | let _ = "hesudo worpd".replace("su", "l").replace('d', "l").replace('p', "l"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(['d', 'p'], "l")` + +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:46:28 + | +LL | let _ = "hesudo worpd".replace(d, "l").replace('p', "l").replace("su", "l"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace([d, 'p'], "l")` + +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:48:27 + | +LL | let _ = "hesuo world".replace(get_filter(), "l").replace('s', "l"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace([get_filter(), 's'], "l")` + +error: aborting due to 13 previous errors +