diff --git a/clippy_lints/src/methods/implicit_clone.rs b/clippy_lints/src/methods/implicit_clone.rs index 81c42de14..ff7904ce5 100644 --- a/clippy_lints/src/methods/implicit_clone.rs +++ b/clippy_lints/src/methods/implicit_clone.rs @@ -12,15 +12,7 @@ use super::IMPLICIT_CLONE; pub fn check(cx: &LateContext<'_>, method_name: &str, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>, span: Span) { if_chain! { if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id); - if 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_vec" => cx.tcx.impl_of_method(method_def_id) - .map(|impl_did| Some(impl_did) == cx.tcx.lang_items().slice_alloc_impl()) - == Some(true), - _ => false, - }; + if is_clone_like(cx, method_name, method_def_id); let return_type = cx.typeck_results().expr_ty(expr); let input_type = cx.typeck_results().expr_ty(recv).peel_refs(); if let Some(ty_name) = input_type.ty_adt_def().map(|adt_def| cx.tcx.item_name(adt_def.did)); @@ -38,3 +30,19 @@ 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. +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_vec" => { + cx.tcx + .impl_of_method(method_def_id) + .map(|impl_did| Some(impl_did) == cx.tcx.lang_items().slice_alloc_impl()) + == Some(true) + }, + _ => false, + } +} diff --git a/clippy_lints/src/methods/unnecessary_to_owned.rs b/clippy_lints/src/methods/unnecessary_to_owned.rs index 307de7bb6..b380f734b 100644 --- a/clippy_lints/src/methods/unnecessary_to_owned.rs +++ b/clippy_lints/src/methods/unnecessary_to_owned.rs @@ -1,7 +1,8 @@ +use super::implicit_clone::is_clone_like; use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet_opt; use clippy_utils::ty::{implements_trait, is_copy, peel_mid_ty_refs}; -use clippy_utils::{get_parent_expr, match_def_path, paths}; +use clippy_utils::{get_parent_expr, is_diag_item_method, is_diag_trait_item}; use rustc_errors::Applicability; use rustc_hir::{def_id::DefId, BorrowKind, Expr, ExprKind}; use rustc_lint::LateContext; @@ -14,28 +15,17 @@ use std::cmp::max; use super::UNNECESSARY_TO_OWNED; -const TO_OWNED_LIKE_PATHS: &[&[&str]] = &[ - &paths::COW_INTO_OWNED, - &paths::OS_STR_TO_OS_STRING, - &paths::PATH_TO_PATH_BUF, - &paths::SLICE_TO_VEC, - &paths::TO_OWNED_METHOD, - &paths::TO_STRING_METHOD, -]; - pub fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, method_name: Symbol, args: &'tcx [Expr<'tcx>]) { if_chain! { if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id); - if TO_OWNED_LIKE_PATHS - .iter() - .any(|path| match_def_path(cx, method_def_id, path)); + if is_to_owned_like(cx, method_name, method_def_id); if let [receiver] = args; then { // At this point, we know the call is of a `to_owned`-like function. The functions // `check_addr_of_expr` and `check_call_arg` determine whether the call is unnecessary // based on its context, that is, whether it is a referent in an `AddrOf` expression or // an argument in a function call. - if check_addr_of_expr(cx, expr, method_name, receiver) { + if check_addr_of_expr(cx, expr, method_name, method_def_id, receiver) { return; } check_call_arg(cx, expr, method_name, receiver); @@ -45,10 +35,12 @@ pub fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, method_name: Symbol /// Checks whether `expr` is a referent in an `AddrOf` expression and, if so, determines whether its /// call of a `to_owned`-like function is unnecessary. +#[allow(clippy::too_many_lines)] fn check_addr_of_expr( cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, method_name: Symbol, + method_def_id: DefId, receiver: &'tcx Expr<'tcx>, ) -> bool { if_chain! { @@ -100,14 +92,17 @@ fn check_addr_of_expr( ] => Some(target_ty), _ => None, }; + let receiver_ty = cx.typeck_results().expr_ty(receiver); + // Only flag cases where the receiver is copyable or the method is `Cow::into_owned`. This + // restriction is to ensure there is not overlap between `redundant_clone` and this lint. + if is_copy(cx, receiver_ty) || is_cow_into_owned(cx, method_name, method_def_id); + if let Some(receiver_snippet) = snippet_opt(cx, receiver.span); then { let (target_ty, n_target_refs) = peel_mid_ty_refs(target_ty); - let receiver_ty = cx.typeck_results().expr_ty(receiver); let (receiver_ty, n_receiver_refs) = peel_mid_ty_refs(receiver_ty); if_chain! { if receiver_ty == target_ty; if n_target_refs >= n_receiver_refs; - if let Some(receiver_snippet) = snippet_opt(cx, receiver.span); then { span_lint_and_sugg( cx, @@ -122,21 +117,32 @@ fn check_addr_of_expr( } } if implements_deref_trait(cx, receiver_ty, target_ty) { - span_lint_and_sugg( - cx, - UNNECESSARY_TO_OWNED, - expr.span.with_lo(receiver.span.hi()), - &format!("unnecessary use of `{}`", method_name), - "remove this", - String::new(), - Applicability::MachineApplicable, - ); + if n_receiver_refs > 0 { + span_lint_and_sugg( + cx, + UNNECESSARY_TO_OWNED, + parent.span, + &format!("unnecessary use of `{}`", method_name), + "use", + receiver_snippet, + Applicability::MachineApplicable, + ); + } else { + span_lint_and_sugg( + cx, + UNNECESSARY_TO_OWNED, + expr.span.with_lo(receiver.span.hi()), + &format!("unnecessary use of `{}`", method_name), + "remove this", + String::new(), + Applicability::MachineApplicable, + ); + } return true; } if_chain! { if let Some(as_ref_trait_id) = cx.tcx.get_diagnostic_item(sym::AsRef); if implements_trait(cx, receiver_ty, as_ref_trait_id, &[GenericArg::from(target_ty)]); - if let Some(receiver_snippet) = snippet_opt(cx, receiver.span); then { span_lint_and_sugg( cx, @@ -326,3 +332,21 @@ fn implements_deref_trait(cx: &LateContext<'tcx>, ty: Ty<'tcx>, deref_target_ty: } } } + +/// Returns true if the named method can be used to convert the receiver to its "owned" +/// representation. +fn is_to_owned_like(cx: &LateContext<'_>, 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_to_string(cx, method_name, method_def_id) +} + +/// Returns true if the named method is `Cow::into_owned`. +fn is_cow_into_owned(cx: &LateContext<'_>, method_name: Symbol, method_def_id: DefId) -> bool { + method_name.as_str() == "into_owned" && is_diag_item_method(cx, method_def_id, sym::Cow) +} + +/// Returns true if the named method is `ToString::to_string`. +fn is_to_string(cx: &LateContext<'_>, method_name: Symbol, method_def_id: DefId) -> bool { + method_name.as_str() == "to_string" && is_diag_trait_item(cx, method_def_id, sym::ToString) +} diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index 634a452d6..6171823ab 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -36,7 +36,6 @@ pub const CLONE_TRAIT_METHOD: [&str; 4] = ["core", "clone", "Clone", "clone"]; pub const CMP_MAX: [&str; 3] = ["core", "cmp", "max"]; pub const CMP_MIN: [&str; 3] = ["core", "cmp", "min"]; pub const COW: [&str; 3] = ["alloc", "borrow", "Cow"]; -pub const COW_INTO_OWNED: [&str; 4] = ["alloc", "borrow", "Cow", "into_owned"]; pub const CSTRING_AS_C_STR: [&str; 5] = ["std", "ffi", "c_str", "CString", "as_c_str"]; pub const DEFAULT_TRAIT_METHOD: [&str; 4] = ["core", "default", "Default", "default"]; pub const DEREF_MUT_TRAIT_METHOD: [&str; 5] = ["core", "ops", "deref", "DerefMut", "deref_mut"]; @@ -171,7 +170,6 @@ pub const SLICE_FROM_RAW_PARTS: [&str; 4] = ["core", "slice", "raw", "from_raw_p pub const SLICE_FROM_RAW_PARTS_MUT: [&str; 4] = ["core", "slice", "raw", "from_raw_parts_mut"]; pub const SLICE_INTO_VEC: [&str; 4] = ["alloc", "slice", "", "into_vec"]; pub const SLICE_ITER: [&str; 4] = ["core", "slice", "iter", "Iter"]; -pub const SLICE_TO_VEC: [&str; 4] = ["alloc", "slice", "", "to_vec"]; pub const STDERR: [&str; 4] = ["std", "io", "stdio", "stderr"]; pub const STDOUT: [&str; 4] = ["std", "io", "stdio", "stdout"]; pub const CONVERT_IDENTITY: [&str; 3] = ["core", "convert", "identity"]; diff --git a/tests/ui/unnecessary_to_owned.fixed b/tests/ui/unnecessary_to_owned.fixed index f1a5dc91a..bd9ef1fff 100644 --- a/tests/ui/unnecessary_to_owned.fixed +++ b/tests/ui/unnecessary_to_owned.fixed @@ -1,15 +1,14 @@ // run-rustfix #![allow(clippy::ptr_arg)] -// Some of the expressions that `redundant_clone` flags overlap with ours. Enabling it interferes -// with `rustfix`. -#![allow(clippy::redundant_clone)] -// `needless_borrow` is for checking the fixed code. +// `needless_borrow` is to ensure there are no needles borrows in the fixed code. #![warn(clippy::needless_borrow)] +// `redundant_clone` is to ensure there is no overlap between that lint and this one. +#![warn(clippy::redundant_clone)] #![warn(clippy::unnecessary_to_owned)] use std::borrow::Cow; -use std::ffi::{CStr, OsStr}; +use std::ffi::{CStr, CString, OsStr, OsString}; use std::ops::Deref; #[derive(Clone)] @@ -51,6 +50,7 @@ fn main() { let array_ref = &["x"]; let slice = &["x"][..]; let x = X(String::from("x")); + let x_ref = &x; require_c_str(&Cow::from(c_str)); require_c_str(c_str); @@ -66,17 +66,17 @@ fn main() { require_str(s); require_str(&Cow::from(s)); require_str(s); - require_str(x.as_ref()); + require_str(x_ref.as_ref()); require_slice(slice); require_slice(&Cow::from(slice)); require_slice(array.as_ref()); require_slice(array_ref.as_ref()); require_slice(slice); - require_slice(&x); + require_slice(x_ref); require_x(&Cow::::Owned(x.clone())); - require_x(&x); + require_x(x_ref); require_deref_c_str(c_str); require_deref_os_str(os_str); @@ -118,16 +118,23 @@ fn main() { require_as_ref_slice_str(array_ref, s); require_as_ref_slice_str(slice, s); - let _ = x.join(&x); + let _ = x.join(x_ref); // negative tests require_string(&s.to_string()); require_string(&Cow::from(s).into_owned()); require_string(&s.to_owned()); - require_string(&x.to_string()); + require_string(&x_ref.to_string()); // `X` isn't copy. + require_slice(&x.to_owned()); require_deref_slice(x.to_owned()); + + // The following should be flagged by `redundant_clone`, but not by this lint. + require_c_str(&CString::from_vec_with_nul(vec![0]).unwrap()); + require_os_str(&OsString::from("x")); + require_path(&std::path::PathBuf::from("x")); + require_str(&String::from("x")); } fn require_c_str(_: &CStr) {} diff --git a/tests/ui/unnecessary_to_owned.rs b/tests/ui/unnecessary_to_owned.rs index 081ff635b..1bf30e228 100644 --- a/tests/ui/unnecessary_to_owned.rs +++ b/tests/ui/unnecessary_to_owned.rs @@ -1,15 +1,14 @@ // run-rustfix #![allow(clippy::ptr_arg)] -// Some of the expressions that `redundant_clone` flags overlap with ours. Enabling it interferes -// with `rustfix`. -#![allow(clippy::redundant_clone)] -// `needless_borrow` is for checking the fixed code. +// `needless_borrow` is to ensure there are no needles borrows in the fixed code. #![warn(clippy::needless_borrow)] +// `redundant_clone` is to ensure there is no overlap between that lint and this one. +#![warn(clippy::redundant_clone)] #![warn(clippy::unnecessary_to_owned)] use std::borrow::Cow; -use std::ffi::{CStr, OsStr}; +use std::ffi::{CStr, CString, OsStr, OsString}; use std::ops::Deref; #[derive(Clone)] @@ -51,6 +50,7 @@ fn main() { let array_ref = &["x"]; let slice = &["x"][..]; let x = X(String::from("x")); + let x_ref = &x; require_c_str(&Cow::from(c_str).into_owned()); require_c_str(&c_str.to_owned()); @@ -66,17 +66,17 @@ fn main() { require_str(&s.to_string()); require_str(&Cow::from(s).into_owned()); require_str(&s.to_owned()); - require_str(&x.to_string()); + require_str(&x_ref.to_string()); require_slice(&slice.to_vec()); require_slice(&Cow::from(slice).into_owned()); require_slice(&array.to_owned()); require_slice(&array_ref.to_owned()); require_slice(&slice.to_owned()); - require_slice(&x.to_owned()); + require_slice(&x_ref.to_owned()); require_x(&Cow::::Owned(x.clone()).into_owned()); - require_x(&x.to_owned()); + require_x(&x_ref.to_owned()); require_deref_c_str(c_str.to_owned()); require_deref_os_str(os_str.to_owned()); @@ -118,16 +118,23 @@ fn main() { require_as_ref_slice_str(array_ref.to_owned(), s.to_owned()); require_as_ref_slice_str(slice.to_owned(), s.to_owned()); - let _ = x.join(&x.to_string()); + let _ = x.join(&x_ref.to_string()); // negative tests require_string(&s.to_string()); require_string(&Cow::from(s).into_owned()); require_string(&s.to_owned()); - require_string(&x.to_string()); + require_string(&x_ref.to_string()); // `X` isn't copy. + require_slice(&x.to_owned()); require_deref_slice(x.to_owned()); + + // The following should be flagged by `redundant_clone`, but not by this lint. + require_c_str(&CString::from_vec_with_nul(vec![0]).unwrap().to_owned()); + require_os_str(&OsString::from("x").to_os_string()); + require_path(&std::path::PathBuf::from("x").to_path_buf()); + require_str(&String::from("x").to_string()); } fn require_c_str(_: &CStr) {} diff --git a/tests/ui/unnecessary_to_owned.stderr b/tests/ui/unnecessary_to_owned.stderr index b15dd4c7e..83cc53ef3 100644 --- a/tests/ui/unnecessary_to_owned.stderr +++ b/tests/ui/unnecessary_to_owned.stderr @@ -1,3 +1,52 @@ +error: redundant clone + --> $DIR/unnecessary_to_owned.rs:134:64 + | +LL | require_c_str(&CString::from_vec_with_nul(vec![0]).unwrap().to_owned()); + | ^^^^^^^^^^^ help: remove this + | + = note: `-D clippy::redundant-clone` implied by `-D warnings` +note: this value is dropped without further use + --> $DIR/unnecessary_to_owned.rs:134:20 + | +LL | require_c_str(&CString::from_vec_with_nul(vec![0]).unwrap().to_owned()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: redundant clone + --> $DIR/unnecessary_to_owned.rs:135:40 + | +LL | require_os_str(&OsString::from("x").to_os_string()); + | ^^^^^^^^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/unnecessary_to_owned.rs:135:21 + | +LL | require_os_str(&OsString::from("x").to_os_string()); + | ^^^^^^^^^^^^^^^^^^^ + +error: redundant clone + --> $DIR/unnecessary_to_owned.rs:136:48 + | +LL | require_path(&std::path::PathBuf::from("x").to_path_buf()); + | ^^^^^^^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/unnecessary_to_owned.rs:136:19 + | +LL | require_path(&std::path::PathBuf::from("x").to_path_buf()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: redundant clone + --> $DIR/unnecessary_to_owned.rs:137:35 + | +LL | require_str(&String::from("x").to_string()); + | ^^^^^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/unnecessary_to_owned.rs:137:18 + | +LL | require_str(&String::from("x").to_string()); + | ^^^^^^^^^^^^^^^^^ + error: unnecessary use of `into_owned` --> $DIR/unnecessary_to_owned.rs:55:36 | @@ -69,8 +118,8 @@ LL | require_str(&s.to_owned()); error: unnecessary use of `to_string` --> $DIR/unnecessary_to_owned.rs:69:17 | -LL | require_str(&x.to_string()); - | ^^^^^^^^^^^^^^ help: use: `x.as_ref()` +LL | require_str(&x_ref.to_string()); + | ^^^^^^^^^^^^^^^^^^ help: use: `x_ref.as_ref()` error: unnecessary use of `to_vec` --> $DIR/unnecessary_to_owned.rs:71:19 @@ -103,10 +152,10 @@ LL | require_slice(&slice.to_owned()); | ^^^^^^^^^^^^^^^^^ help: use: `slice` error: unnecessary use of `to_owned` - --> $DIR/unnecessary_to_owned.rs:76:21 + --> $DIR/unnecessary_to_owned.rs:76:19 | -LL | require_slice(&x.to_owned()); - | ^^^^^^^^^^^ help: remove this +LL | require_slice(&x_ref.to_owned()); + | ^^^^^^^^^^^^^^^^^ help: use: `x_ref` error: unnecessary use of `into_owned` --> $DIR/unnecessary_to_owned.rs:78:42 @@ -117,8 +166,8 @@ LL | require_x(&Cow::::Owned(x.clone()).into_owned()); error: unnecessary use of `to_owned` --> $DIR/unnecessary_to_owned.rs:79:15 | -LL | require_x(&x.to_owned()); - | ^^^^^^^^^^^^^ help: use: `&x` +LL | require_x(&x_ref.to_owned()); + | ^^^^^^^^^^^^^^^^^ help: use: `x_ref` error: unnecessary use of `to_owned` --> $DIR/unnecessary_to_owned.rs:81:25 @@ -375,8 +424,8 @@ LL | require_as_ref_slice_str(slice.to_owned(), s.to_owned()); error: unnecessary use of `to_string` --> $DIR/unnecessary_to_owned.rs:121:20 | -LL | let _ = x.join(&x.to_string()); - | ^^^^^^^^^^^^^^ help: use: `&x` +LL | let _ = x.join(&x_ref.to_string()); + | ^^^^^^^^^^^^^^^^^^ help: use: `x_ref` -error: aborting due to 63 previous errors +error: aborting due to 67 previous errors