From a4413f75bf829549fdc0f01a834988d1128d6ba9 Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Tue, 2 Aug 2022 22:37:40 +0200 Subject: [PATCH 1/7] Register new lint collapsible_str_replace to methods --- CHANGELOG.md | 1 + clippy_lints/src/lib.register_all.rs | 1 + clippy_lints/src/lib.register_lints.rs | 1 + clippy_lints/src/lib.register_perf.rs | 1 + .../src/methods/collapsible_str_replace.rs | 6 ++ clippy_lints/src/methods/mod.rs | 29 ++++++++++ tests/ui/collapsible_str_replace.rs | 55 +++++++++++++++++++ 7 files changed, 94 insertions(+) create mode 100644 clippy_lints/src/methods/collapsible_str_replace.rs create mode 100644 tests/ui/collapsible_str_replace.rs 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..e5547b504713 --- /dev/null +++ b/clippy_lints/src/methods/collapsible_str_replace.rs @@ -0,0 +1,6 @@ +use rustc_hir as hir; +use rustc_hir::*; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>) {} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 1cfe8c4191ef..8f7cb105c220 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(|c| matches!(c, '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, @@ -3491,6 +3519,7 @@ impl Methods { ("sort_unstable_by", [arg]) => { unnecessary_sort_by::check(cx, expr, recv, arg, true); }, + ("replace", [_, _]) => collapsible_str_replace::check(cx, expr, recv, args), ("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); diff --git a/tests/ui/collapsible_str_replace.rs b/tests/ui/collapsible_str_replace.rs new file mode 100644 index 000000000000..c91809bc8409 --- /dev/null +++ b/tests/ui/collapsible_str_replace.rs @@ -0,0 +1,55 @@ +#![warn(clippy::collapsible_str_replace)] + +fn main() { + let misspelled = "hesuo worpd"; + + let p = 'p'; + let s = 's'; + let u = 'u'; + + // If the first argument to a single `str::replace` call is a slice and none of the chars + // are variables, recommend `collapsible_str_replace` + let replacement = misspelled.replace(&['s', 'u', 'p'], "l"); + println!("{replacement}"); + + // The first iteration of `collapsible_str_replace` will not create lint if the first argument to + // a single `str::replace` call is a slice and one or more of its chars are variables + let replacement = misspelled.replace(&['s', u, 'p'], "l"); + println!("{replacement}"); + + let replacement = misspelled.replace(&[s, u, 'p'], "l"); + println!("{replacement}"); + + let replacement = misspelled.replace(&[s, u, p], "l"); + println!("{replacement}"); + + // If there is a single call to `str::replace` and the first argument is a char or a variable, don't + // not recommend `collapsible_str_replace` + let replacement = misspelled.replace('s', "l"); + println!("{replacement}"); + + let replacement = misspelled.replace(s, "l"); + println!("{replacement}"); + + // If there are consecutive calls to `str::replace` and none of the chars are variables, + // recommend `collapsible_str_replace` + let replacement = misspelled.replace('s', "l").replace('u', "l"); + println!("{replacement}"); + + let replacement = misspelled.replace('s', "l").replace('u', "l").replace('p', "l"); + println!("{replacement}"); + + // If there are consecutive calls to `str::replace` and all or any chars are variables, + // recommend the fallback `misspelled.replace(&[s, u, p], "l")` + let replacement = misspelled.replace(s, "l").replace('u', "l"); + println!("{replacement}"); + + let replacement = misspelled.replace(s, "l").replace('u', "l").replace('p', "l"); + println!("{replacement}"); + + let replacement = misspelled.replace(s, "l").replace(u, "l").replace('p', "l"); + println!("{replacement}"); + + let replacement = misspelled.replace(s, "l").replace(u, "l").replace(p, "l"); + println!("{replacement}"); +} From 89698b9613179a1c025ce61fc17334a3ff315598 Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Thu, 4 Aug 2022 23:46:41 +0200 Subject: [PATCH 2/7] Extend and improve initial test cases for collapsible_str_replace --- tests/ui/collapsible_str_replace.rs | 52 +++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 13 deletions(-) diff --git a/tests/ui/collapsible_str_replace.rs b/tests/ui/collapsible_str_replace.rs index c91809bc8409..0f4bdf094808 100644 --- a/tests/ui/collapsible_str_replace.rs +++ b/tests/ui/collapsible_str_replace.rs @@ -1,5 +1,9 @@ #![warn(clippy::collapsible_str_replace)] +fn get_filter() -> &'static str { + "u" +} + fn main() { let misspelled = "hesuo worpd"; @@ -7,38 +11,60 @@ fn main() { let s = 's'; let u = 'u'; + // LINT CASES // If the first argument to a single `str::replace` call is a slice and none of the chars // are variables, recommend `collapsible_str_replace` let replacement = misspelled.replace(&['s', 'u', 'p'], "l"); println!("{replacement}"); - // The first iteration of `collapsible_str_replace` will not create lint if the first argument to - // a single `str::replace` call is a slice and one or more of its chars are variables - let replacement = misspelled.replace(&['s', u, 'p'], "l"); - println!("{replacement}"); - - let replacement = misspelled.replace(&[s, u, 'p'], "l"); + // If there are consecutive calls to `str::replace` and none of the chars are variables, + // recommend `collapsible_str_replace` + let replacement = misspelled.replace('s', "l").replace('u', "l"); println!("{replacement}"); - let replacement = misspelled.replace(&[s, u, p], "l"); + let replacement = misspelled.replace('s', "l").replace('u', "l").replace('p', "l"); println!("{replacement}"); - // If there is a single call to `str::replace` and the first argument is a char or a variable, don't - // not recommend `collapsible_str_replace` + // NO LINT CASES + // If there is a single call to `str::replace` and the first argument is a char or a variable, + // do not recommend `collapsible_str_replace` let replacement = misspelled.replace('s', "l"); println!("{replacement}"); let replacement = misspelled.replace(s, "l"); println!("{replacement}"); - // If there are consecutive calls to `str::replace` and none of the chars are variables, - // recommend `collapsible_str_replace` - let replacement = misspelled.replace('s', "l").replace('u', "l"); + // If the `from` argument is of kind other than a slice or a char, do not lint + let replacement = misspelled.replace(&get_filter(), "l"); + + // NO LINT TIL IMPROVEMENT + // If multiple `str::replace` calls contain slices and none of the chars are variables, + // the first iteration does not recommend `collapsible_str_replace` + let replacement = misspelled.replace(&['s', 'u', 'p'], "l").replace(&['s', 'p'], "l"); println!("{replacement}"); - let replacement = misspelled.replace('s', "l").replace('u', "l").replace('p', "l"); + // If a mixture of `str::replace` calls with slice and char arguments are used for `from` arg, + // the first iteration does not recommend `collapsible_str_replace` + let replacement = misspelled.replace(&['s', 'u'], "l").replace('p', "l"); + println!("replacement"); + + let replacement = misspelled.replace('p', "l").replace(&['s', 'u'], "l"); + + // The first iteration of `collapsible_str_replace` will not create lint if the first argument to + // a single `str::replace` call is a slice and one or more of its chars are variables + let replacement = misspelled.replace(&['s', u, 'p'], "l"); + println!("{replacement}"); + + let replacement = misspelled.replace(&[s, u, 'p'], "l"); + println!("{replacement}"); + + let replacement = misspelled.replace(&[s, u, p], "l"); + println!("{replacement}"); + + let replacement = misspelled.replace(&[s, u], "l").replace(&[u, p], "l"); println!("{replacement}"); + // FALLBACK CASES // If there are consecutive calls to `str::replace` and all or any chars are variables, // recommend the fallback `misspelled.replace(&[s, u, p], "l")` let replacement = misspelled.replace(s, "l").replace('u', "l"); From 6e866875294be38b02c1deaf0b1cba181a65109b Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Fri, 5 Aug 2022 21:08:43 +0200 Subject: [PATCH 3/7] Handle replace calls with char slices --- .../src/methods/collapsible_str_replace.rs | 154 +++++++++++++++++- clippy_lints/src/methods/mod.rs | 2 +- tests/ui/collapsible_str_replace.rs | 27 +-- 3 files changed, 167 insertions(+), 16 deletions(-) diff --git a/clippy_lints/src/methods/collapsible_str_replace.rs b/clippy_lints/src/methods/collapsible_str_replace.rs index e5547b504713..e2477fb06bd6 100644 --- a/clippy_lints/src/methods/collapsible_str_replace.rs +++ b/clippy_lints/src/methods/collapsible_str_replace.rs @@ -1,6 +1,154 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +// use clippy_utils::source::snippet_with_context; +use clippy_utils::visitors::for_each_expr; +use core::ops::ControlFlow; +use if_chain::if_chain; +use rustc_ast::ast::LitKind; +use rustc_errors::Applicability; use rustc_hir as hir; use rustc_hir::*; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_lint::LateContext; +use rustc_middle::ty; +use rustc_span::source_map::Spanned; +use std::unreachable; +// use rustc_span::Span; -pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>) {} +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>, + args: &'tcx [hir::Expr<'tcx>], +) { + match (name, args) { + ("replace", [from, to]) => { + // Check for `str::replace` calls with char slice for linting + let original_recv = find_original_recv(recv); + let original_recv_ty = cx.typeck_results().expr_ty(original_recv).peel_refs(); + if_chain! { + // Check the receiver of the method call is `str` type + if matches!(original_recv_ty.kind(), ty::Str); + let from_ty = cx.typeck_results().expr_ty(from).peel_refs(); + if let ty::Array(array_ty, _) = from_ty.kind(); + if matches!(array_ty.kind(), ty::Char); + then { + check_replace_call_with_char_slice(cx, from, to); + } + } + + match method_call(recv) { + // Check if there's an earlier `str::replace` call + Some(("replace", [prev_recv, prev_from, prev_to], prev_span)) => { + println!("Consecutive replace calls"); + // Check that the original receiver is of `ty::Str` type + // Check that all the `from` args are char literals + // Check that all the `to` args are the same variable or has the same &str value + // If so, then lint + }, + _ => {}, + } + }, + _ => {}, + } +} + +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 +} + +fn check_replace_call_with_char_slice<'tcx>( + cx: &LateContext<'tcx>, + from_arg: &'tcx hir::Expr<'tcx>, + to_arg: &'tcx hir::Expr<'tcx>, +) { + let mut has_no_var = true; + let mut char_list: Vec = Vec::new(); + // Go through the `from_arg` to collect all char literals + let _: Option<()> = for_each_expr(from_arg, |e| { + if let ExprKind::Lit(Spanned { + node: LitKind::Char(val), + .. + }) = e.kind + { + char_list.push(val); + ControlFlow::Continue(()) + } else if let ExprKind::Path(..) = e.kind { + // If a variable is found in the char slice, no lint for first version of this lint + has_no_var = false; + ControlFlow::BREAK + } else { + ControlFlow::Continue(()) + } + }); + + if has_no_var { + let to_arg_repr = match to_arg.kind { + ExprKind::Lit(Spanned { + node: LitKind::Str(to_arg_val, _), + .. + }) => { + let repr = to_arg_val.as_str(); + let double_quote = "\""; + 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(); + + path_segment_ident_names.join("::") + }, + _ => unreachable!(), + }; + + let app = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + COLLAPSIBLE_STR_REPLACE, + from_arg.span, + "used slice of chars in `str::replace` call", + "replace with", + format!( + "replace(|c| matches!(c, {}), {})", + format_slice_of_chars_for_sugg(&char_list), + to_arg_repr, + ), + app, + ); + } +} + +fn format_slice_of_chars_for_sugg(chars: &Vec) -> String { + let single_quoted_chars: Vec = chars + .iter() + .map(|c| "'".to_owned() + &c.to_string() + &"'".to_owned()) + .collect(); + single_quoted_chars.join(" | ") +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 8f7cb105c220..ec5d5402b0ea 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3519,7 +3519,7 @@ impl Methods { ("sort_unstable_by", [arg]) => { unnecessary_sort_by::check(cx, expr, recv, arg, true); }, - ("replace", [_, _]) => collapsible_str_replace::check(cx, expr, recv, args), + ("replace", [_, _]) => collapsible_str_replace::check(cx, expr, name, recv, args), ("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); diff --git a/tests/ui/collapsible_str_replace.rs b/tests/ui/collapsible_str_replace.rs index 0f4bdf094808..943fb4473bbd 100644 --- a/tests/ui/collapsible_str_replace.rs +++ b/tests/ui/collapsible_str_replace.rs @@ -10,6 +10,7 @@ fn main() { let p = 'p'; let s = 's'; let u = 'u'; + let l = "l"; // LINT CASES // If the first argument to a single `str::replace` call is a slice and none of the chars @@ -17,6 +18,20 @@ fn main() { let replacement = misspelled.replace(&['s', 'u', 'p'], "l"); println!("{replacement}"); + let replacement = misspelled.replace(&['s', 'u', 'p'], l); + println!("{replacement}"); + + // If multiple `str::replace` calls contain slices and none of the chars are variables, + // recommend `collapsible_str_replace` + let replacement = misspelled.replace(&['s', 'u'], "l").replace(&['u', 'p'], "l"); + println!("{replacement}"); + + let replacement = misspelled.replace('s', "l").replace(&['u', 'p'], "l"); + println!("{replacement}"); + + let replacement = misspelled.replace(&['s', 'u'], "l").replace('p', "l"); + println!("replacement"); + // If there are consecutive calls to `str::replace` and none of the chars are variables, // recommend `collapsible_str_replace` let replacement = misspelled.replace('s', "l").replace('u', "l"); @@ -38,18 +53,6 @@ fn main() { let replacement = misspelled.replace(&get_filter(), "l"); // NO LINT TIL IMPROVEMENT - // If multiple `str::replace` calls contain slices and none of the chars are variables, - // the first iteration does not recommend `collapsible_str_replace` - let replacement = misspelled.replace(&['s', 'u', 'p'], "l").replace(&['s', 'p'], "l"); - println!("{replacement}"); - - // If a mixture of `str::replace` calls with slice and char arguments are used for `from` arg, - // the first iteration does not recommend `collapsible_str_replace` - let replacement = misspelled.replace(&['s', 'u'], "l").replace('p', "l"); - println!("replacement"); - - let replacement = misspelled.replace('p', "l").replace(&['s', 'u'], "l"); - // The first iteration of `collapsible_str_replace` will not create lint if the first argument to // a single `str::replace` call is a slice and one or more of its chars are variables let replacement = misspelled.replace(&['s', u, 'p'], "l"); From a9bd0bd3214f04e5f3df9cc5cd9f6ecd375e6242 Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Sun, 7 Aug 2022 14:08:09 +0200 Subject: [PATCH 4/7] Handle repeated str::replace calls with single char kind to str --- .../src/methods/collapsible_str_replace.rs | 337 +++++++++++++----- clippy_lints/src/methods/mod.rs | 8 +- tests/ui/collapsible_str_replace.rs | 4 + 3 files changed, 258 insertions(+), 91 deletions(-) diff --git a/clippy_lints/src/methods/collapsible_str_replace.rs b/clippy_lints/src/methods/collapsible_str_replace.rs index e2477fb06bd6..b4e97a3bea4c 100644 --- a/clippy_lints/src/methods/collapsible_str_replace.rs +++ b/clippy_lints/src/methods/collapsible_str_replace.rs @@ -1,17 +1,16 @@ use clippy_utils::diagnostics::span_lint_and_sugg; -// use clippy_utils::source::snippet_with_context; +use clippy_utils::get_parent_expr; use clippy_utils::visitors::for_each_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::*; use rustc_lint::LateContext; use rustc_middle::ty; use rustc_span::source_map::Spanned; -use std::unreachable; -// use rustc_span::Span; use super::method_call; use super::COLLAPSIBLE_STR_REPLACE; @@ -25,28 +24,46 @@ pub(super) fn check<'tcx>( ) { match (name, args) { ("replace", [from, to]) => { - // Check for `str::replace` calls with char slice for linting + // 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 = cx.typeck_results().expr_ty(original_recv).peel_refs(); + 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! { - // Check the receiver of the method call is `str` type - if matches!(original_recv_ty.kind(), ty::Str); - let from_ty = cx.typeck_results().expr_ty(from).peel_refs(); - if let ty::Array(array_ty, _) = from_ty.kind(); + // Check for `str::replace` calls with char slice for linting + if original_recv_is_str_kind; + let from_ty_kind = cx.typeck_results().expr_ty(from).peel_refs().kind(); + if let ty::Array(array_ty, _) = from_ty_kind; if matches!(array_ty.kind(), ty::Char); then { check_replace_call_with_char_slice(cx, from, to); + return; + } + } + + if_chain! { + if original_recv_is_str_kind; + if let Some(parent) = get_parent_expr(cx, expr); + if let Some((name, [..], _)) = method_call(parent); + + then { + match name { + "replace" => return, + _ => { + check_consecutive_replace_calls(cx, expr); + return; + }, + } } } match method_call(recv) { // Check if there's an earlier `str::replace` call - Some(("replace", [prev_recv, prev_from, prev_to], prev_span)) => { - println!("Consecutive replace calls"); - // Check that the original receiver is of `ty::Str` type - // Check that all the `from` args are char literals - // Check that all the `to` args are the same variable or has the same &str value - // If so, then lint + Some(("replace", [_, _, _], _)) => { + if original_recv_is_str_kind { + check_consecutive_replace_calls(cx, expr); + return; + } }, _ => {}, } @@ -55,100 +72,246 @@ pub(super) fn check<'tcx>( } } -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 -} - +/// Check a `str::replace` call that contains a char slice as `from` argument for +/// `collapsible_str_replace` lint. fn check_replace_call_with_char_slice<'tcx>( cx: &LateContext<'tcx>, from_arg: &'tcx hir::Expr<'tcx>, to_arg: &'tcx hir::Expr<'tcx>, ) { - let mut has_no_var = true; - let mut char_list: Vec = Vec::new(); + let mut char_slice_has_no_variables = true; + let mut chars: Vec = Vec::new(); + // Go through the `from_arg` to collect all char literals let _: Option<()> = for_each_expr(from_arg, |e| { if let ExprKind::Lit(Spanned { - node: LitKind::Char(val), - .. + node: LitKind::Char(_), .. }) = e.kind { - char_list.push(val); + chars.push(get_replace_call_char_arg_repr(e).unwrap()); ControlFlow::Continue(()) } else if let ExprKind::Path(..) = e.kind { // If a variable is found in the char slice, no lint for first version of this lint - has_no_var = false; + char_slice_has_no_variables = false; ControlFlow::BREAK } else { ControlFlow::Continue(()) } }); - if has_no_var { - let to_arg_repr = match to_arg.kind { - ExprKind::Lit(Spanned { - node: LitKind::Str(to_arg_val, _), - .. - }) => { - let repr = to_arg_val.as_str(); - let double_quote = "\""; - double_quote.to_owned() + repr + double_quote - }, - ExprKind::Path(QPath::Resolved( - _, - Path { - segments: path_segments, - .. + if char_slice_has_no_variables { + if let Some(to_arg_repr) = get_replace_call_char_arg_repr(to_arg) { + let app = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + COLLAPSIBLE_STR_REPLACE, + from_arg.span, + "used slice of chars in `str::replace` call", + "replace with", + format!("replace(|c| matches!(c, {}), {})", chars.join(" | "), to_arg_repr,), + app, + ); + } + } +} + +/// 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 { + if replace_call_from_args_are_only_lit_chars(&from_args) { + let from_arg_reprs: Vec = 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, + "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 = 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, + "used consecutive `str::replace` call", + "replace with", + format!( + "replace(&[{}], {})", + from_arg_reprs.join(" , "), + to_arg, + ), + app, + ); + } + } + } +} + +/// 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: &Vec<&'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>( + cx: &LateContext<'tcx>, + expr: &'tcx hir::Expr<'tcx>, +) -> Option>> { + let mut all_from_args_are_chars = true; + let mut from_args = Vec::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(()) }, - )) => { - // 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(); - - path_segment_ident_names.join("::") + _ => ControlFlow::BREAK, + } + } else { + ControlFlow::Continue(()) + } + }); + + if all_from_args_are_chars { + return Some(from_args); + } else { + return 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 { + 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, + } + } else { + ControlFlow::Continue(()) + } + }); + + // let mut to_arg_repr_set = FxHashSet::default(); + let mut to_arg_reprs = Vec::new(); + for &to_arg in to_args.iter() { + 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 = FxHashSet::from_iter(to_arg_reprs.iter().cloned()); + // 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() +} + +/// 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 { + 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, + .. }, - _ => unreachable!(), - }; - - let app = Applicability::MachineApplicable; - span_lint_and_sugg( - cx, - COLLAPSIBLE_STR_REPLACE, - from_arg.span, - "used slice of chars in `str::replace` call", - "replace with", - format!( - "replace(|c| matches!(c, {}), {})", - format_slice_of_chars_for_sugg(&char_list), - to_arg_repr, - ), - app, - ); + )) => { + // 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, } } -fn format_slice_of_chars_for_sugg(chars: &Vec) -> String { - let single_quoted_chars: Vec = chars - .iter() - .map(|c| "'".to_owned() + &c.to_string() + &"'".to_owned()) - .collect(); - single_quoted_chars.join(" | ") +/// 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 } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index ec5d5402b0ea..f93586936239 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3519,7 +3519,10 @@ impl Methods { ("sort_unstable_by", [arg]) => { unnecessary_sort_by::check(cx, expr, recv, arg, true); }, - ("replace", [_, _]) => collapsible_str_replace::check(cx, expr, name, recv, args), + ("replace" | "replacen", [arg1, arg2] | [arg1, arg2, _]) => { + no_effect_replace::check(cx, expr, arg1, arg2); + collapsible_str_replace::check(cx, expr, name, recv, args); + }, ("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); @@ -3585,9 +3588,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.rs b/tests/ui/collapsible_str_replace.rs index 943fb4473bbd..4257fce448d4 100644 --- a/tests/ui/collapsible_str_replace.rs +++ b/tests/ui/collapsible_str_replace.rs @@ -49,6 +49,10 @@ fn main() { let replacement = misspelled.replace(s, "l"); println!("{replacement}"); + // If the consecutive `str::replace` calls have different `to` arguments, do not lint + let replacement = misspelled.replace('s', "l").replace('u', "p"); + println!("{replacement}"); + // If the `from` argument is of kind other than a slice or a char, do not lint let replacement = misspelled.replace(&get_filter(), "l"); From c989746ccfae22dd0f7f6d1e3d4f984ac8889e9f Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Mon, 8 Aug 2022 22:02:26 +0200 Subject: [PATCH 5/7] Remove checks on char slice; improve lint suggestion --- .../src/methods/collapsible_str_replace.rs | 89 ++++++------------- tests/ui/collapsible_str_replace.rs | 61 +++++++------ 2 files changed, 59 insertions(+), 91 deletions(-) diff --git a/clippy_lints/src/methods/collapsible_str_replace.rs b/clippy_lints/src/methods/collapsible_str_replace.rs index b4e97a3bea4c..1760232041ad 100644 --- a/clippy_lints/src/methods/collapsible_str_replace.rs +++ b/clippy_lints/src/methods/collapsible_str_replace.rs @@ -11,6 +11,7 @@ use rustc_hir::*; use rustc_lint::LateContext; use rustc_middle::ty; use rustc_span::source_map::Spanned; +use rustc_span::Span; use super::method_call; use super::COLLAPSIBLE_STR_REPLACE; @@ -23,31 +24,20 @@ pub(super) fn check<'tcx>( args: &'tcx [hir::Expr<'tcx>], ) { match (name, args) { - ("replace", [from, to]) => { + ("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! { - // Check for `str::replace` calls with char slice for linting - if original_recv_is_str_kind; - let from_ty_kind = cx.typeck_results().expr_ty(from).peel_refs().kind(); - if let ty::Array(array_ty, _) = from_ty_kind; - if matches!(array_ty.kind(), ty::Char); - then { - check_replace_call_with_char_slice(cx, from, to); - return; - } - } - 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 let Some((name, ..)) = method_call(parent); then { match name { + // If the parent node is a `str::replace` call, we've already handled the lint, don't lint again "replace" => return, _ => { check_consecutive_replace_calls(cx, expr); @@ -59,7 +49,7 @@ pub(super) fn check<'tcx>( match method_call(recv) { // Check if there's an earlier `str::replace` call - Some(("replace", [_, _, _], _)) => { + Some(("replace", ..)) => { if original_recv_is_str_kind { check_consecutive_replace_calls(cx, expr); return; @@ -72,55 +62,14 @@ pub(super) fn check<'tcx>( } } -/// Check a `str::replace` call that contains a char slice as `from` argument for -/// `collapsible_str_replace` lint. -fn check_replace_call_with_char_slice<'tcx>( - cx: &LateContext<'tcx>, - from_arg: &'tcx hir::Expr<'tcx>, - to_arg: &'tcx hir::Expr<'tcx>, -) { - let mut char_slice_has_no_variables = true; - let mut chars: Vec = Vec::new(); - - // Go through the `from_arg` to collect all char literals - let _: Option<()> = for_each_expr(from_arg, |e| { - if let ExprKind::Lit(Spanned { - node: LitKind::Char(_), .. - }) = e.kind - { - chars.push(get_replace_call_char_arg_repr(e).unwrap()); - ControlFlow::Continue(()) - } else if let ExprKind::Path(..) = e.kind { - // If a variable is found in the char slice, no lint for first version of this lint - char_slice_has_no_variables = false; - ControlFlow::BREAK - } else { - ControlFlow::Continue(()) - } - }); - - if char_slice_has_no_variables { - if let Some(to_arg_repr) = get_replace_call_char_arg_repr(to_arg) { - let app = Applicability::MachineApplicable; - span_lint_and_sugg( - cx, - COLLAPSIBLE_STR_REPLACE, - from_arg.span, - "used slice of chars in `str::replace` call", - "replace with", - format!("replace(|c| matches!(c, {}), {})", chars.join(" | "), to_arg_repr,), - app, - ); - } - } -} - /// 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 = from_args.iter().map(|from_arg| { get_replace_call_char_arg_repr(from_arg).unwrap() @@ -130,7 +79,7 @@ fn check_consecutive_replace_calls<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir span_lint_and_sugg( cx, COLLAPSIBLE_STR_REPLACE, - expr.span, + expr.span.with_lo(earliest_replace_call_span.lo()), "used consecutive `str::replace` call", "replace with", format!( @@ -150,7 +99,7 @@ fn check_consecutive_replace_calls<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir span_lint_and_sugg( cx, COLLAPSIBLE_STR_REPLACE, - expr.span, + expr.span.with_lo(earliest_replace_call_span.lo()), "used consecutive `str::replace` call", "replace with", format!( @@ -295,6 +244,26 @@ fn get_replace_call_char_arg_repr<'tcx>(arg: &'tcx hir::Expr<'tcx>) -> Option(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; diff --git a/tests/ui/collapsible_str_replace.rs b/tests/ui/collapsible_str_replace.rs index 4257fce448d4..05a4fd08a327 100644 --- a/tests/ui/collapsible_str_replace.rs +++ b/tests/ui/collapsible_str_replace.rs @@ -13,36 +13,38 @@ fn main() { let l = "l"; // LINT CASES - // If the first argument to a single `str::replace` call is a slice and none of the chars - // are variables, recommend `collapsible_str_replace` - let replacement = misspelled.replace(&['s', 'u', 'p'], "l"); + let replacement = misspelled.replace('s', "l").replace('u', "l"); println!("{replacement}"); - let replacement = misspelled.replace(&['s', 'u', 'p'], l); + let replacement = misspelled.replace('s', l).replace('u', l); println!("{replacement}"); - // If multiple `str::replace` calls contain slices and none of the chars are variables, - // recommend `collapsible_str_replace` - let replacement = misspelled.replace(&['s', 'u'], "l").replace(&['u', 'p'], "l"); + let replacement = misspelled.replace('s', "l").replace('u', "l").replace('p', "l"); println!("{replacement}"); - let replacement = misspelled.replace('s', "l").replace(&['u', 'p'], "l"); + let replacement = misspelled + .replace('s', "l") + .replace('u', "l") + .replace('p', "l") + .replace('d', "l"); println!("{replacement}"); - let replacement = misspelled.replace(&['s', 'u'], "l").replace('p', "l"); - println!("replacement"); + // FALLBACK CASES + // If there are consecutive calls to `str::replace` and all or any chars are variables, + // recommend the fallback `misspelled.replace(&[s, u, p], "l")` + let replacement = misspelled.replace(s, "l").replace('u', "l"); + println!("{replacement}"); - // If there are consecutive calls to `str::replace` and none of the chars are variables, - // recommend `collapsible_str_replace` - let replacement = misspelled.replace('s', "l").replace('u', "l"); + let replacement = misspelled.replace(s, "l").replace('u', "l").replace('p', "l"); println!("{replacement}"); - let replacement = misspelled.replace('s', "l").replace('u', "l").replace('p', "l"); + let replacement = misspelled.replace(s, "l").replace(u, "l").replace('p', "l"); + println!("{replacement}"); + + let replacement = misspelled.replace(s, "l").replace(u, "l").replace(p, "l"); println!("{replacement}"); // NO LINT CASES - // If there is a single call to `str::replace` and the first argument is a char or a variable, - // do not recommend `collapsible_str_replace` let replacement = misspelled.replace('s', "l"); println!("{replacement}"); @@ -53,36 +55,33 @@ fn main() { let replacement = misspelled.replace('s', "l").replace('u', "p"); println!("{replacement}"); - // If the `from` argument is of kind other than a slice or a char, do not lint let replacement = misspelled.replace(&get_filter(), "l"); + println!("{replacement}"); - // NO LINT TIL IMPROVEMENT - // The first iteration of `collapsible_str_replace` will not create lint if the first argument to - // a single `str::replace` call is a slice and one or more of its chars are variables - let replacement = misspelled.replace(&['s', u, 'p'], "l"); + let replacement = misspelled.replace(&['s', 'u', 'p'], "l"); println!("{replacement}"); - let replacement = misspelled.replace(&[s, u, 'p'], "l"); + let replacement = misspelled.replace(&['s', 'u', 'p'], l); println!("{replacement}"); - let replacement = misspelled.replace(&[s, u, p], "l"); + let replacement = misspelled.replace(&['s', 'u'], "l").replace(&['u', 'p'], "l"); println!("{replacement}"); - let replacement = misspelled.replace(&[s, u], "l").replace(&[u, p], "l"); + let replacement = misspelled.replace('s', "l").replace(&['u', 'p'], "l"); println!("{replacement}"); - // FALLBACK CASES - // If there are consecutive calls to `str::replace` and all or any chars are variables, - // recommend the fallback `misspelled.replace(&[s, u, p], "l")` - let replacement = misspelled.replace(s, "l").replace('u', "l"); + let replacement = misspelled.replace(&['s', 'u'], "l").replace('p', "l"); + println!("replacement"); + + let replacement = misspelled.replace(&['s', u, 'p'], "l"); println!("{replacement}"); - let replacement = misspelled.replace(s, "l").replace('u', "l").replace('p', "l"); + let replacement = misspelled.replace(&[s, u, 'p'], "l"); println!("{replacement}"); - let replacement = misspelled.replace(s, "l").replace(u, "l").replace('p', "l"); + let replacement = misspelled.replace(&[s, u, p], "l"); println!("{replacement}"); - let replacement = misspelled.replace(s, "l").replace(u, "l").replace(p, "l"); + let replacement = misspelled.replace(&[s, u], "l").replace(&[u, p], "l"); println!("{replacement}"); } From fb30b64f6379eb22880d70bfebce66ceccc75269 Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Mon, 8 Aug 2022 22:31:53 +0200 Subject: [PATCH 6/7] Adjust test cases; run cargo dev bless --- .../src/methods/collapsible_str_replace.rs | 65 ++++++++----------- clippy_lints/src/methods/mod.rs | 6 +- tests/ui/collapsible_str_replace.fixed | 65 +++++++++++++++++++ tests/ui/collapsible_str_replace.rs | 65 +++++++------------ tests/ui/collapsible_str_replace.stderr | 56 ++++++++++++++++ 5 files changed, 174 insertions(+), 83 deletions(-) create mode 100644 tests/ui/collapsible_str_replace.fixed create mode 100644 tests/ui/collapsible_str_replace.stderr diff --git a/clippy_lints/src/methods/collapsible_str_replace.rs b/clippy_lints/src/methods/collapsible_str_replace.rs index 1760232041ad..4c6288e798cd 100644 --- a/clippy_lints/src/methods/collapsible_str_replace.rs +++ b/clippy_lints/src/methods/collapsible_str_replace.rs @@ -1,3 +1,5 @@ +// run-rustfix + use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::get_parent_expr; use clippy_utils::visitors::for_each_expr; @@ -7,7 +9,7 @@ use rustc_ast::ast::LitKind; use rustc_data_structures::fx::FxHashSet; use rustc_errors::Applicability; use rustc_hir as hir; -use rustc_hir::*; +use rustc_hir::{ExprKind, Path, QPath}; use rustc_lint::LateContext; use rustc_middle::ty; use rustc_span::source_map::Spanned; @@ -21,44 +23,31 @@ pub(super) fn check<'tcx>( expr: &'tcx hir::Expr<'tcx>, name: &str, recv: &'tcx hir::Expr<'tcx>, - args: &'tcx [hir::Expr<'tcx>], ) { - match (name, args) { - ("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 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_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 { - match name { - // If the parent node is a `str::replace` call, we've already handled the lint, don't lint again - "replace" => return, - _ => { - check_consecutive_replace_calls(cx, expr); - return; - }, - } - } + then { + // If the parent node is a `str::replace` call, we've already handled the lint, don't lint again + return; } + } - match method_call(recv) { - // Check if there's an earlier `str::replace` call - Some(("replace", ..)) => { - if original_recv_is_str_kind { - check_consecutive_replace_calls(cx, expr); - 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); } - }, - _ => {}, + } } } @@ -116,7 +105,7 @@ fn check_consecutive_replace_calls<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir /// 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: &Vec<&'tcx hir::Expr<'tcx>>) -> bool { +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() { @@ -159,9 +148,9 @@ fn get_replace_call_from_args_if_all_char_ty<'tcx>( if all_from_args_are_chars { return Some(from_args); - } else { - return None; } + + None } /// Return a unique String representation of the `to` argument used in a chain of `str::replace` @@ -186,13 +175,13 @@ fn get_replace_call_unique_to_arg_repr<'tcx>(expr: &'tcx hir::Expr<'tcx>) -> Opt // let mut to_arg_repr_set = FxHashSet::default(); let mut to_arg_reprs = Vec::new(); - for &to_arg in to_args.iter() { + 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 = FxHashSet::from_iter(to_arg_reprs.iter().cloned()); + let to_arg_repr_set = to_arg_reprs.iter().cloned().collect::>(); // Check if the set of `to` argument representations has more than one unique value if to_arg_repr_set.len() != 1 { return None; diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index f93586936239..8b2fa8e94577 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -152,11 +152,11 @@ declare_clippy_lint! { /// let hello = "hesuo worpd" /// .replace('s', "l") /// .replace("u", "l") - /// .replace('p', "l") + /// .replace('p', "l"); /// ``` /// Use instead: /// ```rust - /// let hello = "hesuo worpd".replace(|c| matches!(c, 's' | 'u' | 'p'), "l") + /// let hello = "hesuo worpd".replace(|c| matches!(c, 's' | 'u' | 'p'), "l"); /// ``` #[clippy::version = "1.64.0"] pub COLLAPSIBLE_STR_REPLACE, @@ -3521,7 +3521,7 @@ impl Methods { }, ("replace" | "replacen", [arg1, arg2] | [arg1, arg2, _]) => { no_effect_replace::check(cx, expr, arg1, arg2); - collapsible_str_replace::check(cx, expr, name, recv, args); + 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) { diff --git a/tests/ui/collapsible_str_replace.fixed b/tests/ui/collapsible_str_replace.fixed new file mode 100644 index 000000000000..0bf857d9837e --- /dev/null +++ b/tests/ui/collapsible_str_replace.fixed @@ -0,0 +1,65 @@ +// run-rustfix + +#![warn(clippy::collapsible_str_replace)] + +fn get_filter() -> &'static str { + "u" +} + +fn main() { + let misspelled = "hesuo worpd"; + + let p = 'p'; + let s = 's'; + let u = 'u'; + let l = "l"; + + // LINT CASES + let _ = misspelled.replace(|c| matches!(c, 'u' | 's'), "l"); + + let _ = misspelled.replace(|c| matches!(c, 'u' | 's'), l); + + let _ = misspelled.replace(|c| matches!(c, 'p' | 'u' | 's'), "l"); + + let _ = misspelled + .replace(|c| matches!(c, 'd' | 'p' | 'u' | 's'), "l"); + + // FALLBACK CASES + // If there are consecutive calls to `str::replace` and all or any chars are variables, + // recommend the fallback `misspelled.replace(&[s, u, p], "l")` + let _ = misspelled.replace(&['u' , s], "l"); + + let _ = misspelled.replace(&['p' , 'u' , s], "l"); + + let _ = misspelled.replace(&['p' , u , s], "l"); + + let _ = misspelled.replace(&[p , u , s], "l"); + + // NO LINT CASES + let _ = misspelled.replace('s', "l"); + + let _ = misspelled.replace(s, "l"); + + // If the consecutive `str::replace` calls have different `to` arguments, do not lint + let _ = misspelled.replace('s', "l").replace('u', "p"); + + let _ = misspelled.replace(&get_filter(), "l"); + + let _ = misspelled.replace(&['s', 'u', 'p'], "l"); + + let _ = misspelled.replace(&['s', 'u', 'p'], l); + + let _ = misspelled.replace(&['s', 'u'], "l").replace(&['u', 'p'], "l"); + + let _ = misspelled.replace('s', "l").replace(&['u', 'p'], "l"); + + let _ = misspelled.replace(&['s', 'u'], "l").replace('p', "l"); + + let _ = misspelled.replace(&['s', u, 'p'], "l"); + + let _ = misspelled.replace(&[s, u, 'p'], "l"); + + let _ = misspelled.replace(&[s, u, p], "l"); + + let _ = misspelled.replace(&[s, u], "l").replace(&[u, p], "l"); +} diff --git a/tests/ui/collapsible_str_replace.rs b/tests/ui/collapsible_str_replace.rs index 05a4fd08a327..45d9fd87e5e4 100644 --- a/tests/ui/collapsible_str_replace.rs +++ b/tests/ui/collapsible_str_replace.rs @@ -1,3 +1,5 @@ +// run-rustfix + #![warn(clippy::collapsible_str_replace)] fn get_filter() -> &'static str { @@ -13,75 +15,54 @@ fn main() { let l = "l"; // LINT CASES - let replacement = misspelled.replace('s', "l").replace('u', "l"); - println!("{replacement}"); + let _ = misspelled.replace('s', "l").replace('u', "l"); - let replacement = misspelled.replace('s', l).replace('u', l); - println!("{replacement}"); + let _ = misspelled.replace('s', l).replace('u', l); - let replacement = misspelled.replace('s', "l").replace('u', "l").replace('p', "l"); - println!("{replacement}"); + let _ = misspelled.replace('s', "l").replace('u', "l").replace('p', "l"); - let replacement = misspelled + let _ = misspelled .replace('s', "l") .replace('u', "l") .replace('p', "l") .replace('d', "l"); - println!("{replacement}"); // FALLBACK CASES // If there are consecutive calls to `str::replace` and all or any chars are variables, // recommend the fallback `misspelled.replace(&[s, u, p], "l")` - let replacement = misspelled.replace(s, "l").replace('u', "l"); - println!("{replacement}"); + let _ = misspelled.replace(s, "l").replace('u', "l"); - let replacement = misspelled.replace(s, "l").replace('u', "l").replace('p', "l"); - println!("{replacement}"); + let _ = misspelled.replace(s, "l").replace('u', "l").replace('p', "l"); - let replacement = misspelled.replace(s, "l").replace(u, "l").replace('p', "l"); - println!("{replacement}"); + let _ = misspelled.replace(s, "l").replace(u, "l").replace('p', "l"); - let replacement = misspelled.replace(s, "l").replace(u, "l").replace(p, "l"); - println!("{replacement}"); + let _ = misspelled.replace(s, "l").replace(u, "l").replace(p, "l"); // NO LINT CASES - let replacement = misspelled.replace('s', "l"); - println!("{replacement}"); + let _ = misspelled.replace('s', "l"); - let replacement = misspelled.replace(s, "l"); - println!("{replacement}"); + let _ = misspelled.replace(s, "l"); // If the consecutive `str::replace` calls have different `to` arguments, do not lint - let replacement = misspelled.replace('s', "l").replace('u', "p"); - println!("{replacement}"); + let _ = misspelled.replace('s', "l").replace('u', "p"); - let replacement = misspelled.replace(&get_filter(), "l"); - println!("{replacement}"); + let _ = misspelled.replace(&get_filter(), "l"); - let replacement = misspelled.replace(&['s', 'u', 'p'], "l"); - println!("{replacement}"); + let _ = misspelled.replace(&['s', 'u', 'p'], "l"); - let replacement = misspelled.replace(&['s', 'u', 'p'], l); - println!("{replacement}"); + let _ = misspelled.replace(&['s', 'u', 'p'], l); - let replacement = misspelled.replace(&['s', 'u'], "l").replace(&['u', 'p'], "l"); - println!("{replacement}"); + let _ = misspelled.replace(&['s', 'u'], "l").replace(&['u', 'p'], "l"); - let replacement = misspelled.replace('s', "l").replace(&['u', 'p'], "l"); - println!("{replacement}"); + let _ = misspelled.replace('s', "l").replace(&['u', 'p'], "l"); - let replacement = misspelled.replace(&['s', 'u'], "l").replace('p', "l"); - println!("replacement"); + let _ = misspelled.replace(&['s', 'u'], "l").replace('p', "l"); - let replacement = misspelled.replace(&['s', u, 'p'], "l"); - println!("{replacement}"); + let _ = misspelled.replace(&['s', u, 'p'], "l"); - let replacement = misspelled.replace(&[s, u, 'p'], "l"); - println!("{replacement}"); + let _ = misspelled.replace(&[s, u, 'p'], "l"); - let replacement = misspelled.replace(&[s, u, p], "l"); - println!("{replacement}"); + let _ = misspelled.replace(&[s, u, p], "l"); - let replacement = misspelled.replace(&[s, u], "l").replace(&[u, p], "l"); - println!("{replacement}"); + let _ = misspelled.replace(&[s, u], "l").replace(&[u, p], "l"); } diff --git a/tests/ui/collapsible_str_replace.stderr b/tests/ui/collapsible_str_replace.stderr new file mode 100644 index 000000000000..372fe1da4480 --- /dev/null +++ b/tests/ui/collapsible_str_replace.stderr @@ -0,0 +1,56 @@ +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:18:24 + | +LL | let _ = misspelled.replace('s', "l").replace('u', "l"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(|c| matches!(c, 'u' | 's'), "l")` + | + = note: `-D clippy::collapsible-str-replace` implied by `-D warnings` + +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:20:24 + | +LL | let _ = misspelled.replace('s', l).replace('u', l); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(|c| matches!(c, 'u' | 's'), l)` + +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:22:24 + | +LL | let _ = misspelled.replace('s', "l").replace('u', "l").replace('p', "l"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(|c| matches!(c, 'p' | 'u' | 's'), "l")` + +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:25:10 + | +LL | .replace('s', "l") + | __________^ +LL | | .replace('u', "l") +LL | | .replace('p', "l") +LL | | .replace('d', "l"); + | |__________________________^ help: replace with: `replace(|c| matches!(c, 'd' | 'p' | 'u' | 's'), "l")` + +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:33:24 + | +LL | let _ = misspelled.replace(s, "l").replace('u', "l"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(&['u' , s], "l")` + +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:35:24 + | +LL | let _ = misspelled.replace(s, "l").replace('u', "l").replace('p', "l"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(&['p' , 'u' , s], "l")` + +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:37:24 + | +LL | let _ = misspelled.replace(s, "l").replace(u, "l").replace('p', "l"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(&['p' , u , s], "l")` + +error: used consecutive `str::replace` call + --> $DIR/collapsible_str_replace.rs:39:24 + | +LL | let _ = misspelled.replace(s, "l").replace(u, "l").replace(p, "l"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(&[p , u , s], "l")` + +error: aborting due to 8 previous errors + From b070b4045f348f9222008270a435bda17b048c15 Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Sun, 14 Aug 2022 18:33:55 +0200 Subject: [PATCH 7/7] Simplify lint logic and address code review comments --- .../src/methods/collapsible_str_replace.rs | 301 ++++-------------- clippy_lints/src/methods/mod.rs | 14 +- tests/ui/collapsible_str_replace.fixed | 70 ++-- tests/ui/collapsible_str_replace.rs | 68 ++-- tests/ui/collapsible_str_replace.stderr | 78 +++-- 5 files changed, 201 insertions(+), 330 deletions(-) diff --git a/clippy_lints/src/methods/collapsible_str_replace.rs b/clippy_lints/src/methods/collapsible_str_replace.rs index 4c6288e798cd..561033be5b6a 100644 --- a/clippy_lints/src/methods/collapsible_str_replace.rs +++ b/clippy_lints/src/methods/collapsible_str_replace.rs @@ -1,19 +1,12 @@ -// 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; @@ -21,255 +14,83 @@ 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 = 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 = 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>> { - 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 { - 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::>(); - // 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 { - 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 = 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 -} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 8b2fa8e94577..49d4900295fd 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/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); diff --git a/tests/ui/collapsible_str_replace.fixed b/tests/ui/collapsible_str_replace.fixed index 0bf857d9837e..49fc9a9629e2 100644 --- a/tests/ui/collapsible_str_replace.fixed +++ b/tests/ui/collapsible_str_replace.fixed @@ -2,64 +2,72 @@ #![warn(clippy::collapsible_str_replace)] -fn get_filter() -> &'static str { - "u" +fn get_filter() -> char { + 'u' } fn main() { - let misspelled = "hesuo worpd"; - + let d = 'd'; let p = 'p'; let s = 's'; let u = 'u'; let l = "l"; + let mut iter = ["l", "z"].iter(); + // LINT CASES - let _ = misspelled.replace(|c| matches!(c, 'u' | 's'), "l"); + let _ = "hesuo worpd".replace(['s', 'u'], "l"); - let _ = misspelled.replace(|c| matches!(c, 'u' | 's'), l); + let _ = "hesuo worpd".replace(['s', 'u'], l); - let _ = misspelled.replace(|c| matches!(c, 'p' | 'u' | 's'), "l"); + let _ = "hesuo worpd".replace(['s', 'u', 'p'], "l"); - let _ = misspelled - .replace(|c| matches!(c, 'd' | 'p' | 'u' | 's'), "l"); + let _ = "hesuo worpd" + .replace(['s', 'u', 'p', 'd'], "l"); - // FALLBACK CASES - // If there are consecutive calls to `str::replace` and all or any chars are variables, - // recommend the fallback `misspelled.replace(&[s, u, p], "l")` - let _ = misspelled.replace(&['u' , s], "l"); + let _ = "hesuo world".replace([s, 'u'], "l"); - let _ = misspelled.replace(&['p' , 'u' , s], "l"); + let _ = "hesuo worpd".replace([s, 'u', 'p'], "l"); - let _ = misspelled.replace(&['p' , u , s], "l"); + let _ = "hesuo worpd".replace([s, u, 'p'], "l"); - let _ = misspelled.replace(&[p , u , s], "l"); + let _ = "hesuo worpd".replace([s, u, p], "l"); - // NO LINT CASES - let _ = misspelled.replace('s', "l"); + let _ = "hesuo worlp".replace(['s', 'u'], "l").replace('p', "d"); + + let _ = "hesuo worpd".replace('s', "x").replace(['u', 'p'], "l"); - let _ = misspelled.replace(s, "l"); + // Note: Future iterations could lint `replace(|c| matches!(c, "su" | 'd' | 'p'), "l")` + let _ = "hesudo worpd".replace("su", "l").replace(['d', 'p'], "l"); - // If the consecutive `str::replace` calls have different `to` arguments, do not lint - let _ = misspelled.replace('s', "l").replace('u', "p"); + 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 _ = misspelled.replace(&get_filter(), "l"); + let _ = "hesuo worpd".replace('s', "l").replace('p', l); - let _ = misspelled.replace(&['s', 'u', 'p'], "l"); + let _ = "hesudo worpd".replace('d', "l").replace("su", "l").replace('p', "l"); - let _ = misspelled.replace(&['s', 'u', '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 _ = misspelled.replace(&['s', 'u'], "l").replace(&['u', 'p'], "l"); + let _ = "hesuo worpd".replace(['s', 'u'], "l").replace(['u', 'p'], "l"); - let _ = misspelled.replace('s', "l").replace(&['u', 'p'], "l"); + let _ = "hesuo worpd".replace('s', "l").replace(['u', 'p'], "l"); - let _ = misspelled.replace(&['s', 'u'], "l").replace('p', "l"); + let _ = "hesuo worpd".replace(['s', 'u', 'p'], "l").replace('r', "l"); - let _ = misspelled.replace(&['s', u, 'p'], "l"); + let _ = "hesuo worpd".replace(['s', 'u', 'p'], l).replace('r', l); - let _ = misspelled.replace(&[s, u, 'p'], "l"); + let _ = "hesuo worpd".replace(['s', u, 'p'], "l").replace('r', "l"); - let _ = misspelled.replace(&[s, u, p], "l"); + let _ = "hesuo worpd".replace([s, u], "l").replace(p, "l"); - let _ = misspelled.replace(&[s, u], "l").replace(&[u, 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 index 45d9fd87e5e4..e3e25c4146ff 100644 --- a/tests/ui/collapsible_str_replace.rs +++ b/tests/ui/collapsible_str_replace.rs @@ -2,67 +2,75 @@ #![warn(clippy::collapsible_str_replace)] -fn get_filter() -> &'static str { - "u" +fn get_filter() -> char { + 'u' } fn main() { - let misspelled = "hesuo worpd"; - + let d = 'd'; let p = 'p'; let s = 's'; let u = 'u'; let l = "l"; + let mut iter = ["l", "z"].iter(); + // LINT CASES - let _ = misspelled.replace('s', "l").replace('u', "l"); + let _ = "hesuo worpd".replace('s', "l").replace('u', "l"); - let _ = misspelled.replace('s', l).replace('u', l); + let _ = "hesuo worpd".replace('s', l).replace('u', l); - let _ = misspelled.replace('s', "l").replace('u', "l").replace('p', "l"); + let _ = "hesuo worpd".replace('s', "l").replace('u', "l").replace('p', "l"); - let _ = misspelled + let _ = "hesuo worpd" .replace('s', "l") .replace('u', "l") .replace('p', "l") .replace('d', "l"); - // FALLBACK CASES - // If there are consecutive calls to `str::replace` and all or any chars are variables, - // recommend the fallback `misspelled.replace(&[s, u, p], "l")` - let _ = misspelled.replace(s, "l").replace('u', "l"); + let _ = "hesuo world".replace(s, "l").replace('u', "l"); - let _ = misspelled.replace(s, "l").replace('u', "l").replace('p', "l"); + let _ = "hesuo worpd".replace(s, "l").replace('u', "l").replace('p', "l"); - let _ = misspelled.replace(s, "l").replace(u, "l").replace('p', "l"); + let _ = "hesuo worpd".replace(s, "l").replace(u, "l").replace('p', "l"); - let _ = misspelled.replace(s, "l").replace(u, "l").replace(p, "l"); + let _ = "hesuo worpd".replace(s, "l").replace(u, "l").replace(p, "l"); - // NO LINT CASES - let _ = misspelled.replace('s', "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"); - let _ = misspelled.replace(s, "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"); - // If the consecutive `str::replace` calls have different `to` arguments, do not lint - let _ = misspelled.replace('s', "l").replace('u', "p"); + 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 _ = misspelled.replace(&get_filter(), "l"); + let _ = "hesuo worpd".replace('s', "l").replace('p', l); - let _ = misspelled.replace(&['s', 'u', 'p'], "l"); + let _ = "hesudo worpd".replace('d', "l").replace("su", "l").replace('p', "l"); - let _ = misspelled.replace(&['s', 'u', '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 _ = misspelled.replace(&['s', 'u'], "l").replace(&['u', 'p'], "l"); + let _ = "hesuo worpd".replace(['s', 'u'], "l").replace(['u', 'p'], "l"); - let _ = misspelled.replace('s', "l").replace(&['u', 'p'], "l"); + let _ = "hesuo worpd".replace('s', "l").replace(['u', 'p'], "l"); - let _ = misspelled.replace(&['s', 'u'], "l").replace('p', "l"); + let _ = "hesuo worpd".replace(['s', 'u', 'p'], "l").replace('r', "l"); - let _ = misspelled.replace(&['s', u, 'p'], "l"); + let _ = "hesuo worpd".replace(['s', 'u', 'p'], l).replace('r', l); - let _ = misspelled.replace(&[s, u, 'p'], "l"); + let _ = "hesuo worpd".replace(['s', u, 'p'], "l").replace('r', "l"); - let _ = misspelled.replace(&[s, u, p], "l"); + let _ = "hesuo worpd".replace([s, u], "l").replace(p, "l"); - let _ = misspelled.replace(&[s, u], "l").replace(&[u, 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 index 372fe1da4480..8e3daf3b898a 100644 --- a/tests/ui/collapsible_str_replace.stderr +++ b/tests/ui/collapsible_str_replace.stderr @@ -1,56 +1,86 @@ error: used consecutive `str::replace` call - --> $DIR/collapsible_str_replace.rs:18:24 + --> $DIR/collapsible_str_replace.rs:19:27 | -LL | let _ = misspelled.replace('s', "l").replace('u', "l"); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(|c| matches!(c, 'u' | 's'), "l")` +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:20:24 + --> $DIR/collapsible_str_replace.rs:21:27 | -LL | let _ = misspelled.replace('s', l).replace('u', l); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(|c| matches!(c, 'u' | 's'), l)` +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:22:24 + --> $DIR/collapsible_str_replace.rs:23:27 | -LL | let _ = misspelled.replace('s', "l").replace('u', "l").replace('p', "l"); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(|c| matches!(c, 'p' | 'u' | 's'), "l")` +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:25:10 + --> $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(|c| matches!(c, 'd' | 'p' | 'u' | 's'), "l")` + | |__________________________^ help: replace with: `replace(['s', 'u', 'p', 'd'], "l")` error: used consecutive `str::replace` call - --> $DIR/collapsible_str_replace.rs:33:24 + --> $DIR/collapsible_str_replace.rs:31:27 | -LL | let _ = misspelled.replace(s, "l").replace('u', "l"); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(&['u' , s], "l")` +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:35:24 + --> $DIR/collapsible_str_replace.rs:33:27 | -LL | let _ = misspelled.replace(s, "l").replace('u', "l").replace('p', "l"); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(&['p' , 'u' , s], "l")` +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:24 + --> $DIR/collapsible_str_replace.rs:35:27 | -LL | let _ = misspelled.replace(s, "l").replace(u, "l").replace('p', "l"); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(&['p' , u , s], "l")` +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:24 + --> $DIR/collapsible_str_replace.rs:37:27 | -LL | let _ = misspelled.replace(s, "l").replace(u, "l").replace(p, "l"); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace(&[p , u , s], "l")` +LL | let _ = "hesuo worpd".replace(s, "l").replace(u, "l").replace(p, "l"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `replace([s, u, p], "l")` -error: aborting due to 8 previous errors +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