diff --git a/.github/driver.sh b/.github/driver.sh index 5a81b4112918..2874aaf2110c 100755 --- a/.github/driver.sh +++ b/.github/driver.sh @@ -47,9 +47,9 @@ unset CARGO_MANIFEST_DIR # Run a lint and make sure it produces the expected output. It's also expected to exit with code 1 # FIXME: How to match the clippy invocation in compile-test.rs? -./target/debug/clippy-driver -Dwarnings -Aunused -Zui-testing --emit metadata --crate-type bin tests/ui/string_to_string.rs 2>string_to_string.stderr && exit 1 -sed -e "/= help: for/d" string_to_string.stderr > normalized.stderr -diff -u normalized.stderr tests/ui/string_to_string.stderr +./target/debug/clippy-driver -Dwarnings -Aunused -Zui-testing --emit metadata --crate-type bin tests/ui/char_lit_as_u8.rs 2>char_lit_as_u8.stderr && exit 1 +sed -e "/= help: for/d" char_lit_as_u8.stderr > normalized.stderr +diff -u normalized.stderr tests/ui/char_lit_as_u8.stderr # make sure "clippy-driver --rustc --arg" and "rustc --arg" behave the same SYSROOT=$(rustc --print sysroot) diff --git a/clippy_dev/src/update_lints.rs b/clippy_dev/src/update_lints.rs index 0be24f322d2d..9dfebf04e4c0 100644 --- a/clippy_dev/src/update_lints.rs +++ b/clippy_dev/src/update_lints.rs @@ -546,7 +546,7 @@ impl Lint { /// Returns the lints in a `HashMap`, grouped by the different lint groups #[must_use] fn by_lint_group(lints: impl Iterator) -> HashMap> { - lints.map(|lint| (lint.group.to_string(), lint)).into_group_map() + lints.map(|lint| (lint.group.clone(), lint)).into_group_map() } } diff --git a/clippy_lints/src/blocks_in_conditions.rs b/clippy_lints/src/blocks_in_conditions.rs index 011962846cb3..10fe1a4174f8 100644 --- a/clippy_lints/src/blocks_in_conditions.rs +++ b/clippy_lints/src/blocks_in_conditions.rs @@ -100,8 +100,7 @@ impl<'tcx> LateLintPass<'tcx> for BlocksInConditions { cond.span, BRACED_EXPR_MESSAGE, "try", - snippet_block_with_applicability(cx, ex.span, "..", Some(expr.span), &mut applicability) - .to_string(), + snippet_block_with_applicability(cx, ex.span, "..", Some(expr.span), &mut applicability), applicability, ); } diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 42fbe6438d4e..5f24ac4f9b97 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -245,9 +245,9 @@ fn lint_branches_sharing_code<'tcx>( let cond_snippet = reindent_multiline(&snippet(cx, cond_span, "_"), false, None); let cond_indent = indent_of(cx, cond_span); let moved_snippet = reindent_multiline(&snippet(cx, span, "_"), true, None); - let suggestion = moved_snippet.to_string() + "\n" + &cond_snippet + "{"; + let suggestion = moved_snippet + "\n" + &cond_snippet + "{"; let suggestion = reindent_multiline(&suggestion, true, cond_indent); - (replace_span, suggestion.to_string()) + (replace_span, suggestion) }); let end_suggestion = res.end_span(last_block, sm).map(|span| { let moved_snipped = reindent_multiline(&snippet(cx, span, "_"), true, None); @@ -263,7 +263,7 @@ fn lint_branches_sharing_code<'tcx>( .then_some(range.start - 4..range.end) }) .map_or(span, |range| range.with_ctxt(span.ctxt())); - (span, suggestion.to_string()) + (span, suggestion.clone()) }); let (span, msg, end_span) = match (&start_suggestion, &end_suggestion) { diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 3e9469256e51..276e03b046ef 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -709,7 +709,6 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::strings::STRING_FROM_UTF8_AS_BYTES_INFO, crate::strings::STRING_LIT_AS_BYTES_INFO, crate::strings::STRING_SLICE_INFO, - crate::strings::STRING_TO_STRING_INFO, crate::strings::STR_TO_STRING_INFO, crate::strings::TRIM_SPLIT_WHITESPACE_INFO, crate::strlen_on_c_strings::STRLEN_ON_C_STRINGS_INFO, diff --git a/clippy_lints/src/deprecated_lints.rs b/clippy_lints/src/deprecated_lints.rs index b60c11d79d48..44a35d21d39a 100644 --- a/clippy_lints/src/deprecated_lints.rs +++ b/clippy_lints/src/deprecated_lints.rs @@ -44,6 +44,8 @@ declare_with_version! { DEPRECATED(DEPRECATED_VERSION): &[(&str, &str)] = &[ ("clippy::option_map_or_err_ok", "`clippy::manual_ok_or` covers this case"), #[clippy::version = "1.86.0"] ("clippy::match_on_vec_items", "`clippy::indexing_slicing` covers indexing and slicing on `Vec<_>`"), + #[clippy::version = "1.87.0"] + ("clippy::string_to_string", "`clippy:implicit_clone` and `clippy:map_clone` cover those cases"), // end deprecated lints. used by `cargo dev deprecate_lint` ]} diff --git a/clippy_lints/src/if_not_else.rs b/clippy_lints/src/if_not_else.rs index 45f9aa0a53e4..5e7eb26a4f90 100644 --- a/clippy_lints/src/if_not_else.rs +++ b/clippy_lints/src/if_not_else.rs @@ -89,7 +89,7 @@ impl LateLintPass<'_> for IfNotElse { e.span, msg, "try", - make_sugg(cx, &cond.kind, cond_inner.span, els.span, "..", Some(e.span)).to_string(), + make_sugg(cx, &cond.kind, cond_inner.span, els.span, "..", Some(e.span)), Applicability::MachineApplicable, ), _ => span_lint_and_help(cx, IF_NOT_ELSE, e.span, msg, None, help), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 9e1df7881ce6..8743a6233bb1 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -823,7 +823,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_early_pass(|| Box::new(asm_syntax::InlineAsmX86IntelSyntax)); store.register_late_pass(|_| Box::new(empty_drop::EmptyDrop)); store.register_late_pass(|_| Box::new(strings::StrToString)); - store.register_late_pass(|_| Box::new(strings::StringToString)); store.register_late_pass(|_| Box::new(zero_sized_map_values::ZeroSizedMapValues)); store.register_late_pass(|_| Box::::default()); store.register_late_pass(|_| Box::new(redundant_slicing::RedundantSlicing)); diff --git a/clippy_lints/src/manual_async_fn.rs b/clippy_lints/src/manual_async_fn.rs index abd1ac954cda..ba1ad599e116 100644 --- a/clippy_lints/src/manual_async_fn.rs +++ b/clippy_lints/src/manual_async_fn.rs @@ -83,7 +83,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualAsyncFn { format!("{} async {}", vis_snip, &header_snip[vis_snip.len() + 1..ret_pos]) }; - let body_snip = snippet_block(cx, closure_body.value.span, "..", Some(block.span)).to_string(); + let body_snip = snippet_block(cx, closure_body.value.span, "..", Some(block.span)); diag.multipart_suggestion( "make the function `async` and return the output of the future directly", diff --git a/clippy_lints/src/matches/match_single_binding.rs b/clippy_lints/src/matches/match_single_binding.rs index bca41956f1ca..922381c1c219 100644 --- a/clippy_lints/src/matches/match_single_binding.rs +++ b/clippy_lints/src/matches/match_single_binding.rs @@ -32,8 +32,7 @@ pub(crate) fn check<'a>(cx: &LateContext<'a>, ex: &Expr<'a>, arms: &[Arm<'_>], e Some(expr.span), &mut app, ) - .0 - .to_string(); + .0; // Do we need to add ';' to suggestion ? if let Node::Stmt(stmt) = cx.tcx.parent_hir_node(expr.hir_id) diff --git a/clippy_lints/src/matches/single_match.rs b/clippy_lints/src/matches/single_match.rs index 735ba63eb771..e520b3d91a3b 100644 --- a/clippy_lints/src/matches/single_match.rs +++ b/clippy_lints/src/matches/single_match.rs @@ -112,9 +112,7 @@ fn report_single_pattern( let (sugg, help) = if is_unit_expr(arm.body) { (String::new(), "`match` expression can be removed") } else { - let mut sugg = snippet_block_with_context(cx, arm.body.span, ctxt, "..", Some(expr.span), &mut app) - .0 - .to_string(); + let mut sugg = snippet_block_with_context(cx, arm.body.span, ctxt, "..", Some(expr.span), &mut app).0; if let Node::Stmt(stmt) = cx.tcx.parent_hir_node(expr.hir_id) && let StmtKind::Expr(_) = stmt.kind && match arm.body.kind { @@ -127,7 +125,7 @@ fn report_single_pattern( (sugg, "try") }; span_lint_and_then(cx, lint, expr.span, msg, |diag| { - diag.span_suggestion(expr.span, help, sugg.to_string(), app); + diag.span_suggestion(expr.span, help, sugg, app); note(diag); }); return; @@ -183,7 +181,7 @@ fn report_single_pattern( }; span_lint_and_then(cx, lint, expr.span, msg, |diag| { - diag.span_suggestion(expr.span, "try", sugg.to_string(), app); + diag.span_suggestion(expr.span, "try", sugg, app); note(diag); }); } diff --git a/clippy_lints/src/methods/implicit_clone.rs b/clippy_lints/src/methods/implicit_clone.rs index 519091406ccf..a87aa140fea1 100644 --- a/clippy_lints/src/methods/implicit_clone.rs +++ b/clippy_lints/src/methods/implicit_clone.rs @@ -1,11 +1,12 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet_with_context; use clippy_utils::ty::implements_trait; -use clippy_utils::{is_diag_item_method, is_diag_trait_item, peel_middle_ty_refs}; +use clippy_utils::{get_parent_expr, is_diag_item_method, is_diag_trait_item, peel_middle_ty_refs}; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_lint::LateContext; use rustc_span::sym; +use std::ops::ControlFlow; use super::IMPLICIT_CLONE; @@ -20,6 +21,7 @@ pub fn check(cx: &LateContext<'_>, method_name: &str, expr: &hir::Expr<'_>, recv && return_type == input_type && let Some(clone_trait) = cx.tcx.lang_items().clone_trait() && implements_trait(cx, return_type, clone_trait, &[]) + && is_called_from_map_like(cx, expr).is_none() { let mut app = Applicability::MachineApplicable; let recv_snip = snippet_with_context(cx, recv.span, expr.span.ctxt(), "..", &mut app).0; @@ -40,14 +42,12 @@ pub fn check(cx: &LateContext<'_>, method_name: &str, expr: &hir::Expr<'_>, recv } /// Returns true if the named method can be used to clone the receiver. -/// Note that `to_string` is not flagged by `implicit_clone`. So other lints that call -/// `is_clone_like` and that do flag `to_string` must handle it separately. See, e.g., -/// `is_to_owned_like` in `unnecessary_to_owned.rs`. pub fn is_clone_like(cx: &LateContext<'_>, method_name: &str, method_def_id: hir::def_id::DefId) -> bool { match method_name { "to_os_string" => is_diag_item_method(cx, method_def_id, sym::OsStr), "to_owned" => is_diag_trait_item(cx, method_def_id, sym::ToOwned), "to_path_buf" => is_diag_item_method(cx, method_def_id, sym::Path), + "to_string" => is_diag_trait_item(cx, method_def_id, sym::ToString), "to_vec" => cx .tcx .impl_of_method(method_def_id) @@ -58,3 +58,38 @@ pub fn is_clone_like(cx: &LateContext<'_>, method_name: &str, method_def_id: hir _ => false, } } + +fn is_called_from_map_like(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option { + // Look for a closure as parent of `expr`, discarding simple blocks + let parent_closure = cx + .tcx + .hir_parent_iter(expr.hir_id) + .try_fold(expr.hir_id, |child_hir_id, (_, node)| match node { + // Check that the child expression is the only expression in the block + hir::Node::Block(block) if block.stmts.is_empty() && block.expr.map(|e| e.hir_id) == Some(child_hir_id) => { + ControlFlow::Continue(block.hir_id) + }, + hir::Node::Expr(expr) if matches!(expr.kind, hir::ExprKind::Block(..)) => { + ControlFlow::Continue(expr.hir_id) + }, + hir::Node::Expr(expr) if matches!(expr.kind, hir::ExprKind::Closure(_)) => ControlFlow::Break(Some(expr)), + _ => ControlFlow::Break(None), + }) + .break_value()?; + is_parent_map_like(cx, parent_closure?) +} + +fn is_parent_map_like(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option { + if let Some(parent_expr) = get_parent_expr(cx, expr) + && let hir::ExprKind::MethodCall(name, _, _, parent_span) = parent_expr.kind + && name.ident.name == sym::map + && let Some(caller_def_id) = cx.typeck_results().type_dependent_def_id(parent_expr.hir_id) + && (is_diag_item_method(cx, caller_def_id, sym::Result) + || is_diag_item_method(cx, caller_def_id, sym::Option) + || is_diag_trait_item(cx, caller_def_id, sym::Iterator)) + { + Some(parent_span) + } else { + None + } +} diff --git a/clippy_lints/src/methods/map_clone.rs b/clippy_lints/src/methods/map_clone.rs index 333a33f7527d..214be8f440f1 100644 --- a/clippy_lints/src/methods/map_clone.rs +++ b/clippy_lints/src/methods/map_clone.rs @@ -1,3 +1,4 @@ +use super::implicit_clone::is_clone_like; use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::msrvs::{self, Msrv}; use clippy_utils::source::snippet_with_applicability; @@ -8,8 +9,8 @@ use rustc_hir::def_id::DefId; use rustc_hir::{self as hir, LangItem}; use rustc_lint::LateContext; use rustc_middle::mir::Mutability; -use rustc_middle::ty; use rustc_middle::ty::adjustment::Adjust; +use rustc_middle::ty::{self, Ty}; use rustc_span::symbol::Ident; use rustc_span::{Span, sym}; @@ -67,22 +68,29 @@ pub(super) fn check(cx: &LateContext<'_>, e: &hir::Expr<'_>, recv: &hir::Expr<'_ } }, hir::ExprKind::MethodCall(method, obj, [], _) => { - if ident_eq(name, obj) && method.ident.name == sym::clone + if ident_eq(name, obj) && let Some(fn_id) = cx.typeck_results().type_dependent_def_id(closure_expr.hir_id) - && let Some(trait_id) = cx.tcx.trait_of_item(fn_id) - && cx.tcx.lang_items().clone_trait() == Some(trait_id) + && (is_clone(cx, method.ident.name, fn_id) + || is_clone_like(cx, method.ident.as_str(), fn_id)) // no autoderefs && !cx.typeck_results().expr_adjustments(obj).iter() .any(|a| matches!(a.kind, Adjust::Deref(Some(..)))) + && let obj_ty = cx.typeck_results().expr_ty_adjusted(obj) + && let ty::Ref(_, ty, Mutability::Not) = obj_ty.kind() + // Verify that the method call's output type is the same as its input type. This is to + // avoid cases like `to_string` being called on a `&str`, or `to_vec` being called on a + // slice. + && *ty == cx.typeck_results().expr_ty_adjusted(closure_expr) { - let obj_ty = cx.typeck_results().expr_ty(obj); - if let ty::Ref(_, ty, mutability) = obj_ty.kind() { - if matches!(mutability, Mutability::Not) { - let copy = is_copy(cx, *ty); - lint_explicit_closure(cx, e.span, recv.span, copy, msrv); - } + let obj_ty_unadjusted = cx.typeck_results().expr_ty(obj); + if obj_ty == obj_ty_unadjusted { + let copy = is_copy(cx, *ty); + lint_explicit_closure(cx, e.span, recv.span, copy, msrv); } else { - lint_needless_cloning(cx, e.span, recv.span); + // To avoid issue #6299, ensure `obj` is not a mutable reference. + if !matches!(obj_ty_unadjusted.kind(), ty::Ref(_, _, Mutability::Mut)) { + lint_needless_cloning(cx, e.span, recv.span); + } } } }, @@ -105,6 +113,17 @@ pub(super) fn check(cx: &LateContext<'_>, e: &hir::Expr<'_>, recv: &hir::Expr<'_ } } +fn is_clone(cx: &LateContext<'_>, method: rustc_span::Symbol, fn_id: DefId) -> bool { + if method == sym::clone + && let Some(trait_id) = cx.tcx.trait_of_item(fn_id) + && cx.tcx.lang_items().clone_trait() == Some(trait_id) + { + true + } else { + false + } +} + fn handle_path( cx: &LateContext<'_>, arg: &hir::Expr<'_>, @@ -112,22 +131,47 @@ fn handle_path( e: &hir::Expr<'_>, recv: &hir::Expr<'_>, ) { + let method = || match qpath { + hir::QPath::TypeRelative(_, method) => Some(method), + _ => None, + }; if let Some(path_def_id) = cx.qpath_res(qpath, arg.hir_id).opt_def_id() - && cx.tcx.lang_items().get(LangItem::CloneFn) == Some(path_def_id) + && (cx.tcx.lang_items().get(LangItem::CloneFn) == Some(path_def_id) + || method().is_some_and(|method| is_clone_like(cx, method.ident.name.as_str(), path_def_id))) // The `copied` and `cloned` methods are only available on `&T` and `&mut T` in `Option` // and `Result`. - && let ty::Adt(_, args) = cx.typeck_results().expr_ty(recv).kind() - && let args = args.as_slice() - && let Some(ty) = args.iter().find_map(|generic_arg| generic_arg.as_type()) - && let ty::Ref(_, ty, Mutability::Not) = ty.kind() - && let ty::FnDef(_, lst) = cx.typeck_results().expr_ty(arg).kind() - && lst.iter().all(|l| l.as_type() == Some(*ty)) - && !should_call_clone_as_function(cx, *ty) + // Previously, `handle_path` would make this determination by checking whether `recv`'s type + // was instantiated with a reference. However, that approach can produce false negatives. + // Consider `std::slice::Iter`, for example. Even if it is instantiated with a non-reference + // type, its `map` method will expect a function that operates on references. + && let Some(ty) = clone_like_referent(cx, arg) + && !should_call_clone_as_function(cx, ty) { lint_path(cx, e.span, recv.span, is_copy(cx, ty.peel_refs())); } } +/// Determine whether `expr` is function whose input type is `&T` and whose output type is `T`. If +/// so, return `T`. +fn clone_like_referent<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>) -> Option> { + let ty = cx.typeck_results().expr_ty_adjusted(expr); + if let ty::FnDef(def_id, generic_args) = ty.kind() + && let tys = cx + .tcx + .fn_sig(def_id) + .instantiate(cx.tcx, generic_args) + .skip_binder() + .inputs_and_output + && let [input_ty, output_ty] = tys.as_slice() + && let ty::Ref(_, referent_ty, Mutability::Not) = input_ty.kind() + && referent_ty == output_ty + { + Some(*referent_ty) + } else { + None + } +} + fn ident_eq(name: Ident, path: &hir::Expr<'_>) -> bool { if let hir::ExprKind::Path(hir::QPath::Resolved(None, path)) = path.kind { path.segments.len() == 1 && path.segments[0].ident == name diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 7e2ce157c737..9f0197c0414f 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -5368,7 +5368,7 @@ impl Methods { implicit_clone::check(cx, name, expr, recv); } }, - ("to_os_string" | "to_path_buf" | "to_vec", []) => { + ("to_os_string" | "to_path_buf" | "to_string" | "to_vec", []) => { implicit_clone::check(cx, name, expr, recv); }, ("type_id", []) => { diff --git a/clippy_lints/src/methods/unnecessary_sort_by.rs b/clippy_lints/src/methods/unnecessary_sort_by.rs index 235fdac29433..437f353746f5 100644 --- a/clippy_lints/src/methods/unnecessary_sort_by.rs +++ b/clippy_lints/src/methods/unnecessary_sort_by.rs @@ -216,7 +216,7 @@ pub(super) fn check<'tcx>( { format!("{}::cmp::Reverse({})", std_or_core, trigger.closure_body) } else { - trigger.closure_body.to_string() + trigger.closure_body }, ), if trigger.reverse { diff --git a/clippy_lints/src/methods/unnecessary_to_owned.rs b/clippy_lints/src/methods/unnecessary_to_owned.rs index 62ba3012643c..46bedd4b44ce 100644 --- a/clippy_lints/src/methods/unnecessary_to_owned.rs +++ b/clippy_lints/src/methods/unnecessary_to_owned.rs @@ -621,8 +621,8 @@ fn is_cloned_or_copied(cx: &LateContext<'_>, method_name: Symbol, method_def_id: /// Returns true if the named method can be used to convert the receiver to its "owned" /// representation. fn is_to_owned_like<'a>(cx: &LateContext<'a>, call_expr: &Expr<'a>, method_name: Symbol, method_def_id: DefId) -> bool { - is_clone_like(cx, method_name.as_str(), method_def_id) - || is_cow_into_owned(cx, method_name, method_def_id) + is_cow_into_owned(cx, method_name, method_def_id) + || (method_name != sym::to_string && is_clone_like(cx, method_name.as_str(), method_def_id)) || is_to_string_on_string_like(cx, call_expr, method_name, method_def_id) } diff --git a/clippy_lints/src/redundant_else.rs b/clippy_lints/src/redundant_else.rs index a3be16ed858e..79353dc9247e 100644 --- a/clippy_lints/src/redundant_else.rs +++ b/clippy_lints/src/redundant_else.rs @@ -97,7 +97,7 @@ impl EarlyLintPass for RedundantElse { els.span.with_lo(then.span.hi()), "redundant else block", "remove the `else` block and move the contents out", - make_sugg(cx, els.span, "..", Some(expr.span)).to_string(), + make_sugg(cx, els.span, "..", Some(expr.span)), app, ); } diff --git a/clippy_lints/src/strings.rs b/clippy_lints/src/strings.rs index dd819510f84c..c7e9a53dea05 100644 --- a/clippy_lints/src/strings.rs +++ b/clippy_lints/src/strings.rs @@ -14,8 +14,6 @@ use rustc_session::declare_lint_pass; use rustc_span::source_map::Spanned; use rustc_span::sym; -use std::ops::ControlFlow; - declare_clippy_lint! { /// ### What it does /// Checks for string appends of the form `x = x + y` (without @@ -412,125 +410,6 @@ impl<'tcx> LateLintPass<'tcx> for StrToString { } } -declare_clippy_lint! { - /// ### What it does - /// This lint checks for `.to_string()` method calls on values of type `String`. - /// - /// ### Why restrict this? - /// The `to_string` method is also used on other types to convert them to a string. - /// When called on a `String` it only clones the `String`, which can be more specifically - /// expressed with `.clone()`. - /// - /// ### Example - /// ```no_run - /// let msg = String::from("Hello World"); - /// let _ = msg.to_string(); - /// ``` - /// Use instead: - /// ```no_run - /// let msg = String::from("Hello World"); - /// let _ = msg.clone(); - /// ``` - #[clippy::version = "pre 1.29.0"] - pub STRING_TO_STRING, - restriction, - "using `to_string()` on a `String`, which should be `clone()`" -} - -declare_lint_pass!(StringToString => [STRING_TO_STRING]); - -fn is_parent_map_like(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option { - if let Some(parent_expr) = get_parent_expr(cx, expr) - && let ExprKind::MethodCall(name, _, _, parent_span) = parent_expr.kind - && name.ident.name == sym::map - && let Some(caller_def_id) = cx.typeck_results().type_dependent_def_id(parent_expr.hir_id) - && (clippy_utils::is_diag_item_method(cx, caller_def_id, sym::Result) - || clippy_utils::is_diag_item_method(cx, caller_def_id, sym::Option) - || clippy_utils::is_diag_trait_item(cx, caller_def_id, sym::Iterator)) - { - Some(parent_span) - } else { - None - } -} - -fn is_called_from_map_like(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option { - // Look for a closure as parent of `expr`, discarding simple blocks - let parent_closure = cx - .tcx - .hir_parent_iter(expr.hir_id) - .try_fold(expr.hir_id, |child_hir_id, (_, node)| match node { - // Check that the child expression is the only expression in the block - Node::Block(block) if block.stmts.is_empty() && block.expr.map(|e| e.hir_id) == Some(child_hir_id) => { - ControlFlow::Continue(block.hir_id) - }, - Node::Expr(expr) if matches!(expr.kind, ExprKind::Block(..)) => ControlFlow::Continue(expr.hir_id), - Node::Expr(expr) if matches!(expr.kind, ExprKind::Closure(_)) => ControlFlow::Break(Some(expr)), - _ => ControlFlow::Break(None), - }) - .break_value()?; - is_parent_map_like(cx, parent_closure?) -} - -fn suggest_cloned_string_to_string(cx: &LateContext<'_>, span: rustc_span::Span) { - span_lint_and_sugg( - cx, - STRING_TO_STRING, - span, - "`to_string()` called on a `String`", - "try", - "cloned()".to_string(), - Applicability::MachineApplicable, - ); -} - -impl<'tcx> LateLintPass<'tcx> for StringToString { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'_>) { - if expr.span.from_expansion() { - return; - } - - match &expr.kind { - ExprKind::MethodCall(path, self_arg, [], _) => { - if path.ident.name == sym::to_string - && let ty = cx.typeck_results().expr_ty(self_arg) - && is_type_lang_item(cx, ty.peel_refs(), LangItem::String) - { - if let Some(parent_span) = is_called_from_map_like(cx, expr) { - suggest_cloned_string_to_string(cx, parent_span); - } else { - #[expect(clippy::collapsible_span_lint_calls, reason = "rust-clippy#7797")] - span_lint_and_then( - cx, - STRING_TO_STRING, - expr.span, - "`to_string()` called on a `String`", - |diag| { - diag.help("consider using `.clone()`"); - }, - ); - } - } - }, - ExprKind::Path(QPath::TypeRelative(ty, segment)) => { - if segment.ident.name == sym::to_string - && let rustc_hir::TyKind::Path(QPath::Resolved(_, path)) = ty.peel_refs().kind - && let rustc_hir::def::Res::Def(_, def_id) = path.res - && cx - .tcx - .lang_items() - .get(LangItem::String) - .is_some_and(|lang_id| lang_id == def_id) - && let Some(parent_span) = is_parent_map_like(cx, expr) - { - suggest_cloned_string_to_string(cx, parent_span); - } - }, - _ => {}, - } - } -} - declare_clippy_lint! { /// ### What it does /// Warns about calling `str::trim` (or variants) before `str::split_whitespace`. diff --git a/lintcheck/src/input.rs b/lintcheck/src/input.rs index 83eb0a577d6b..60182fc2293a 100644 --- a/lintcheck/src/input.rs +++ b/lintcheck/src/input.rs @@ -117,7 +117,7 @@ pub fn read_crates(toml_path: &Path) -> (Vec, RecursiveOptions) crate_sources.push(CrateWithSource { name: tk.name.clone(), source: CrateSource::CratesIo { - version: version.to_string(), + version: version.clone(), }, file_link: tk.file_link(DEFAULT_DOCS_LINK), options: tk.options.clone(), diff --git a/lintcheck/src/output.rs b/lintcheck/src/output.rs index dcc1ec339ef9..e38e21015702 100644 --- a/lintcheck/src/output.rs +++ b/lintcheck/src/output.rs @@ -220,7 +220,7 @@ fn print_stats(old_stats: HashMap, new_stats: HashMap<&String, us let same_in_both_hashmaps = old_stats .iter() .filter(|(old_key, old_val)| new_stats.get::<&String>(old_key) == Some(old_val)) - .map(|(k, v)| (k.to_string(), *v)) + .map(|(k, v)| (k.clone(), *v)) .collect::>(); let mut old_stats_deduped = old_stats; diff --git a/tests/ui/deprecated.rs b/tests/ui/deprecated.rs index 2787f6406fe3..37324f16dcc8 100644 --- a/tests/ui/deprecated.rs +++ b/tests/ui/deprecated.rs @@ -17,5 +17,6 @@ #![warn(clippy::wrong_pub_self_convention)] //~ ERROR: lint `clippy::wrong_pub_self_convention` #![warn(clippy::option_map_or_err_ok)] //~ ERROR: lint `clippy::option_map_or_err_ok` #![warn(clippy::match_on_vec_items)] //~ ERROR: lint `clippy::match_on_vec_items` +#![warn(clippy::string_to_string)] //~ ERROR: lint `clippy::string_to_string` fn main() {} diff --git a/tests/ui/deprecated.stderr b/tests/ui/deprecated.stderr index 604732405c37..41c4577a7ba3 100644 --- a/tests/ui/deprecated.stderr +++ b/tests/ui/deprecated.stderr @@ -91,5 +91,11 @@ error: lint `clippy::match_on_vec_items` has been removed: `clippy::indexing_sli LL | #![warn(clippy::match_on_vec_items)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 15 previous errors +error: lint `clippy::string_to_string` has been removed: `clippy:implicit_clone` and `clippy:map_clone` cover those cases + --> tests/ui/deprecated.rs:20:9 + | +LL | #![warn(clippy::string_to_string)] + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 16 previous errors diff --git a/tests/ui/implicit_clone.fixed b/tests/ui/implicit_clone.fixed index d60d1cb0ec04..267514c5f3d3 100644 --- a/tests/ui/implicit_clone.fixed +++ b/tests/ui/implicit_clone.fixed @@ -135,4 +135,10 @@ fn main() { } let no_clone = &NoClone; let _ = no_clone.to_owned(); + + let s = String::from("foo"); + let _ = s.clone(); + //~^ implicit_clone + let _ = s.clone(); + //~^ implicit_clone } diff --git a/tests/ui/implicit_clone.rs b/tests/ui/implicit_clone.rs index b96828f28c82..fba954026e7f 100644 --- a/tests/ui/implicit_clone.rs +++ b/tests/ui/implicit_clone.rs @@ -135,4 +135,10 @@ fn main() { } let no_clone = &NoClone; let _ = no_clone.to_owned(); + + let s = String::from("foo"); + let _ = s.to_owned(); + //~^ implicit_clone + let _ = s.to_string(); + //~^ implicit_clone } diff --git a/tests/ui/implicit_clone.stderr b/tests/ui/implicit_clone.stderr index 1eb6ff1fe429..4cca9b0d0c07 100644 --- a/tests/ui/implicit_clone.stderr +++ b/tests/ui/implicit_clone.stderr @@ -67,5 +67,17 @@ error: implicitly cloning a `PathBuf` by calling `to_path_buf` on its dereferenc LL | let _ = pathbuf_ref.to_path_buf(); | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `(**pathbuf_ref).clone()` -error: aborting due to 11 previous errors +error: implicitly cloning a `String` by calling `to_owned` on its dereferenced type + --> tests/ui/implicit_clone.rs:140:13 + | +LL | let _ = s.to_owned(); + | ^^^^^^^^^^^^ help: consider using: `s.clone()` + +error: implicitly cloning a `String` by calling `to_string` on its dereferenced type + --> tests/ui/implicit_clone.rs:142:13 + | +LL | let _ = s.to_string(); + | ^^^^^^^^^^^^^ help: consider using: `s.clone()` + +error: aborting due to 13 previous errors diff --git a/tests/ui/map_clone.fixed b/tests/ui/map_clone.fixed index 8db4b21be684..3862177fd057 100644 --- a/tests/ui/map_clone.fixed +++ b/tests/ui/map_clone.fixed @@ -158,4 +158,20 @@ fn main() { let y = Some(&x); let _z = y.map(RcWeak::clone); } + + let variable1 = String::new(); + let v = &variable1; + let variable2 = Some(v); + let _ = variable2.cloned(); + //~^ map_clone + let _ = variable2.cloned(); + //~^ map_clone + #[rustfmt::skip] + let _ = variable2.cloned(); + //~^ map_clone + + let _ = vec![String::new()].iter().cloned().collect::>(); + //~^ map_clone + let _ = vec![String::new()].iter().cloned().collect::>(); + //~^ map_clone } diff --git a/tests/ui/map_clone.rs b/tests/ui/map_clone.rs index a726e3561634..4e10d4cda1d3 100644 --- a/tests/ui/map_clone.rs +++ b/tests/ui/map_clone.rs @@ -158,4 +158,20 @@ fn main() { let y = Some(&x); let _z = y.map(RcWeak::clone); } + + let variable1 = String::new(); + let v = &variable1; + let variable2 = Some(v); + let _ = variable2.map(String::to_string); + //~^ map_clone + let _ = variable2.map(|x| x.to_string()); + //~^ map_clone + #[rustfmt::skip] + let _ = variable2.map(|x| { x.to_string() }); + //~^ map_clone + + let _ = vec![String::new()].iter().map(String::to_string).collect::>(); + //~^ map_clone + let _ = vec![String::new()].iter().map(|x| x.to_string()).collect::>(); + //~^ map_clone } diff --git a/tests/ui/map_clone.stderr b/tests/ui/map_clone.stderr index 8bf54e4e8aee..e44388ba78d0 100644 --- a/tests/ui/map_clone.stderr +++ b/tests/ui/map_clone.stderr @@ -91,5 +91,35 @@ error: you are explicitly cloning with `.map()` LL | let y = x.map(|x| Clone::clone(x)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `x.copied()` -error: aborting due to 15 previous errors +error: you are explicitly cloning with `.map()` + --> tests/ui/map_clone.rs:165:13 + | +LL | let _ = variable2.map(String::to_string); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `variable2.cloned()` + +error: you are using an explicit closure for cloning elements + --> tests/ui/map_clone.rs:167:13 + | +LL | let _ = variable2.map(|x| x.to_string()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `variable2.cloned()` + +error: you are using an explicit closure for cloning elements + --> tests/ui/map_clone.rs:170:13 + | +LL | let _ = variable2.map(|x| { x.to_string() }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `variable2.cloned()` + +error: you are explicitly cloning with `.map()` + --> tests/ui/map_clone.rs:173:13 + | +LL | let _ = vec![String::new()].iter().map(String::to_string).collect::>(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `vec![String::new()].iter().cloned()` + +error: you are using an explicit closure for cloning elements + --> tests/ui/map_clone.rs:175:13 + | +LL | let _ = vec![String::new()].iter().map(|x| x.to_string()).collect::>(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `vec![String::new()].iter().cloned()` + +error: aborting due to 20 previous errors diff --git a/tests/ui/string_to_string.rs b/tests/ui/string_to_string.rs deleted file mode 100644 index 7c5bd8a897ba..000000000000 --- a/tests/ui/string_to_string.rs +++ /dev/null @@ -1,21 +0,0 @@ -#![warn(clippy::string_to_string)] -#![allow(clippy::redundant_clone, clippy::unnecessary_literal_unwrap)] - -fn main() { - let mut message = String::from("Hello"); - let mut v = message.to_string(); - //~^ string_to_string - - let variable1 = String::new(); - let v = &variable1; - let variable2 = Some(v); - let _ = variable2.map(|x| { - println!(); - x.to_string() - }); - //~^^ string_to_string - - let x = Some(String::new()); - let _ = x.unwrap_or_else(|| v.to_string()); - //~^ string_to_string -} diff --git a/tests/ui/string_to_string.stderr b/tests/ui/string_to_string.stderr deleted file mode 100644 index 99eea06f18eb..000000000000 --- a/tests/ui/string_to_string.stderr +++ /dev/null @@ -1,28 +0,0 @@ -error: `to_string()` called on a `String` - --> tests/ui/string_to_string.rs:6:17 - | -LL | let mut v = message.to_string(); - | ^^^^^^^^^^^^^^^^^^^ - | - = help: consider using `.clone()` - = note: `-D clippy::string-to-string` implied by `-D warnings` - = help: to override `-D warnings` add `#[allow(clippy::string_to_string)]` - -error: `to_string()` called on a `String` - --> tests/ui/string_to_string.rs:14:9 - | -LL | x.to_string() - | ^^^^^^^^^^^^^ - | - = help: consider using `.clone()` - -error: `to_string()` called on a `String` - --> tests/ui/string_to_string.rs:19:33 - | -LL | let _ = x.unwrap_or_else(|| v.to_string()); - | ^^^^^^^^^^^^^ - | - = help: consider using `.clone()` - -error: aborting due to 3 previous errors - diff --git a/tests/ui/string_to_string_in_map.fixed b/tests/ui/string_to_string_in_map.fixed deleted file mode 100644 index efc085539f15..000000000000 --- a/tests/ui/string_to_string_in_map.fixed +++ /dev/null @@ -1,20 +0,0 @@ -#![deny(clippy::string_to_string)] -#![allow(clippy::unnecessary_literal_unwrap, clippy::useless_vec, clippy::iter_cloned_collect)] - -fn main() { - let variable1 = String::new(); - let v = &variable1; - let variable2 = Some(v); - let _ = variable2.cloned(); - //~^ string_to_string - let _ = variable2.cloned(); - //~^ string_to_string - #[rustfmt::skip] - let _ = variable2.cloned(); - //~^ string_to_string - - let _ = vec![String::new()].iter().cloned().collect::>(); - //~^ string_to_string - let _ = vec![String::new()].iter().cloned().collect::>(); - //~^ string_to_string -} diff --git a/tests/ui/string_to_string_in_map.rs b/tests/ui/string_to_string_in_map.rs deleted file mode 100644 index 5bf1d7ba5a2e..000000000000 --- a/tests/ui/string_to_string_in_map.rs +++ /dev/null @@ -1,20 +0,0 @@ -#![deny(clippy::string_to_string)] -#![allow(clippy::unnecessary_literal_unwrap, clippy::useless_vec, clippy::iter_cloned_collect)] - -fn main() { - let variable1 = String::new(); - let v = &variable1; - let variable2 = Some(v); - let _ = variable2.map(String::to_string); - //~^ string_to_string - let _ = variable2.map(|x| x.to_string()); - //~^ string_to_string - #[rustfmt::skip] - let _ = variable2.map(|x| { x.to_string() }); - //~^ string_to_string - - let _ = vec![String::new()].iter().map(String::to_string).collect::>(); - //~^ string_to_string - let _ = vec![String::new()].iter().map(|x| x.to_string()).collect::>(); - //~^ string_to_string -} diff --git a/tests/ui/string_to_string_in_map.stderr b/tests/ui/string_to_string_in_map.stderr deleted file mode 100644 index 35aeed656eed..000000000000 --- a/tests/ui/string_to_string_in_map.stderr +++ /dev/null @@ -1,38 +0,0 @@ -error: `to_string()` called on a `String` - --> tests/ui/string_to_string_in_map.rs:8:23 - | -LL | let _ = variable2.map(String::to_string); - | ^^^^^^^^^^^^^^^^^^^^^^ help: try: `cloned()` - | -note: the lint level is defined here - --> tests/ui/string_to_string_in_map.rs:1:9 - | -LL | #![deny(clippy::string_to_string)] - | ^^^^^^^^^^^^^^^^^^^^^^^^ - -error: `to_string()` called on a `String` - --> tests/ui/string_to_string_in_map.rs:10:23 - | -LL | let _ = variable2.map(|x| x.to_string()); - | ^^^^^^^^^^^^^^^^^^^^^^ help: try: `cloned()` - -error: `to_string()` called on a `String` - --> tests/ui/string_to_string_in_map.rs:13:23 - | -LL | let _ = variable2.map(|x| { x.to_string() }); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `cloned()` - -error: `to_string()` called on a `String` - --> tests/ui/string_to_string_in_map.rs:16:40 - | -LL | let _ = vec![String::new()].iter().map(String::to_string).collect::>(); - | ^^^^^^^^^^^^^^^^^^^^^^ help: try: `cloned()` - -error: `to_string()` called on a `String` - --> tests/ui/string_to_string_in_map.rs:18:40 - | -LL | let _ = vec![String::new()].iter().map(|x| x.to_string()).collect::>(); - | ^^^^^^^^^^^^^^^^^^^^^^ help: try: `cloned()` - -error: aborting due to 5 previous errors -