diff --git a/clippy_lints/src/methods/collapsible_str_replace.rs b/clippy_lints/src/methods/collapsible_str_replace.rs index b4e97a3bea4..1760232041a 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 4257fce448d..05a4fd08a32 100644 --- a/tests/ui/collapsible_str_replace.rs +++ b/tests/ui/collapsible_str_replace.rs @@ -13,62 +13,20 @@ 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"); - 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"); println!("{replacement}"); + 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}"); - // 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 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"); - - // 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"); - 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"); + let replacement = misspelled + .replace('s', "l") + .replace('u', "l") + .replace('p', "l") + .replace('d', "l"); println!("{replacement}"); // FALLBACK CASES @@ -85,4 +43,45 @@ fn main() { let replacement = misspelled.replace(s, "l").replace(u, "l").replace(p, "l"); println!("{replacement}"); + + // NO LINT CASES + let replacement = misspelled.replace('s', "l"); + println!("{replacement}"); + + 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}"); + + let replacement = misspelled.replace(&get_filter(), "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}"); + + let replacement = misspelled.replace('s', "l").replace(&['u', 'p'], "l"); + println!("{replacement}"); + + 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, 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}"); }