diff --git a/CHANGELOG.md b/CHANGELOG.md index 964f339b2..a076997ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4082,6 +4082,7 @@ Released 2018-09-13 [`suspicious_op_assign_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_op_assign_impl [`suspicious_operation_groupings`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_operation_groupings [`suspicious_splitn`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_splitn +[`suspicious_to_owned`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_to_owned [`suspicious_unary_op_formatting`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_unary_op_formatting [`swap_ptr_to_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#swap_ptr_to_ref [`tabs_in_doc_comments`]: https://rust-lang.github.io/rust-clippy/master/index.html#tabs_in_doc_comments diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 61bb5d133..64982851b 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -207,6 +207,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(methods::STRING_EXTEND_CHARS), LintId::of(methods::SUSPICIOUS_MAP), LintId::of(methods::SUSPICIOUS_SPLITN), + LintId::of(methods::SUSPICIOUS_TO_OWNED), LintId::of(methods::UNINIT_ASSUMED_INIT), LintId::of(methods::UNIT_HASH), LintId::of(methods::UNNECESSARY_FILTER_MAP), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 0ec2884cb..fbf77fb73 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -358,6 +358,7 @@ store.register_lints(&[ methods::STRING_EXTEND_CHARS, methods::SUSPICIOUS_MAP, methods::SUSPICIOUS_SPLITN, + methods::SUSPICIOUS_TO_OWNED, methods::UNINIT_ASSUMED_INIT, methods::UNIT_HASH, methods::UNNECESSARY_FILTER_MAP, diff --git a/clippy_lints/src/lib.register_suspicious.rs b/clippy_lints/src/lib.register_suspicious.rs index 6f480c52c..aa4e571db 100644 --- a/clippy_lints/src/lib.register_suspicious.rs +++ b/clippy_lints/src/lib.register_suspicious.rs @@ -24,6 +24,7 @@ store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec! LintId::of(loops::MUT_RANGE_BOUND), LintId::of(methods::NO_EFFECT_REPLACE), LintId::of(methods::SUSPICIOUS_MAP), + LintId::of(methods::SUSPICIOUS_TO_OWNED), LintId::of(multi_assignments::MULTI_ASSIGNMENTS), LintId::of(mut_key::MUTABLE_KEY_TYPE), LintId::of(octal_escapes::OCTAL_ESCAPES), diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 0ec35ddc1..a0d190a58 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -78,6 +78,7 @@ mod str_splitn; mod string_extend_chars; mod suspicious_map; mod suspicious_splitn; +mod suspicious_to_owned; mod uninit_assumed_init; mod unit_hash; mod unnecessary_filter_map; @@ -2053,6 +2054,55 @@ declare_clippy_lint! { "replace `.iter().count()` with `.len()`" } +declare_clippy_lint! { + /// ### What it does + /// Checks for the usage of `_.to_owned()`, on a `Cow<'_, _>`. + /// + /// ### Why is this bad? + /// Calling `to_owned()` on a `Cow` creates a clone of the `Cow` + /// itself, without taking ownership of the `Cow` contents (i.e. + /// it's equivalent to calling `Cow::clone`). + /// The similarly named `into_owned` method, on the other hand, + /// clones the `Cow` contents, effectively turning any `Cow::Borrowed` + /// into a `Cow::Owned`. + /// + /// Given the potential ambiguity, consider replacing `to_owned` + /// with `clone` for better readability or, if getting a `Cow::Owned` + /// was the original intent, using `into_owned` instead. + /// + /// ### Example + /// ```rust + /// # use std::borrow::Cow; + /// let s = "Hello world!"; + /// let cow = Cow::Borrowed(s); + /// + /// let data = cow.to_owned(); + /// assert!(matches!(data, Cow::Borrowed(_))) + /// ``` + /// Use instead: + /// ```rust + /// # use std::borrow::Cow; + /// let s = "Hello world!"; + /// let cow = Cow::Borrowed(s); + /// + /// let data = cow.clone(); + /// assert!(matches!(data, Cow::Borrowed(_))) + /// ``` + /// or + /// ```rust + /// # use std::borrow::Cow; + /// let s = "Hello world!"; + /// let cow = Cow::Borrowed(s); + /// + /// let data = cow.into_owned(); + /// assert!(matches!(data, String)) + /// ``` + #[clippy::version = "1.65.0"] + pub SUSPICIOUS_TO_OWNED, + suspicious, + "calls to `to_owned` on a `Cow<'_, _>` might not do what they are expected" +} + declare_clippy_lint! { /// ### What it does /// Checks for calls to [`splitn`] @@ -3075,6 +3125,7 @@ impl_lint_pass!(Methods => [ FROM_ITER_INSTEAD_OF_COLLECT, INSPECT_FOR_EACH, IMPLICIT_CLONE, + SUSPICIOUS_TO_OWNED, SUSPICIOUS_SPLITN, MANUAL_STR_REPEAT, EXTEND_WITH_DRAIN, @@ -3553,7 +3604,12 @@ impl Methods { } unnecessary_lazy_eval::check(cx, expr, recv, arg, "then_some"); }, - ("to_os_string" | "to_owned" | "to_path_buf" | "to_vec", []) => { + ("to_owned", []) => { + if !suspicious_to_owned::check(cx, expr, recv) { + implicit_clone::check(cx, name, expr, recv); + } + }, + ("to_os_string" | "to_path_buf" | "to_vec", []) => { implicit_clone::check(cx, name, expr, recv); }, ("unwrap", []) => { diff --git a/clippy_lints/src/methods/suspicious_to_owned.rs b/clippy_lints/src/methods/suspicious_to_owned.rs new file mode 100644 index 000000000..6b306fbf0 --- /dev/null +++ b/clippy_lints/src/methods/suspicious_to_owned.rs @@ -0,0 +1,36 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::is_diag_trait_item; +use clippy_utils::source::snippet_with_context; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir as hir; +use rustc_lint::LateContext; +use rustc_middle::ty; +use rustc_span::sym; + +use super::SUSPICIOUS_TO_OWNED; + +pub fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>) -> bool { + if_chain! { + if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id); + if is_diag_trait_item(cx, method_def_id, sym::ToOwned); + let input_type = cx.typeck_results().expr_ty(expr); + if let ty::Adt(adt, _) = cx.typeck_results().expr_ty(expr).kind(); + if cx.tcx.is_diagnostic_item(sym::Cow, adt.did()); + then { + let mut app = Applicability::MaybeIncorrect; + let recv_snip = snippet_with_context(cx, recv.span, expr.span.ctxt(), "..", &mut app).0; + span_lint_and_sugg( + cx, + SUSPICIOUS_TO_OWNED, + expr.span, + &format!("this `to_owned` call clones the {0} itself and does not cause the {0} contents to become owned", input_type), + "consider using, depending on intent", + format!("{0}.clone()` or `{0}.into_owned()", recv_snip), + app, + ); + return true; + } + } + false +} diff --git a/tests/compile-test.rs b/tests/compile-test.rs index 5431f2ebc..ba6186e59 100644 --- a/tests/compile-test.rs +++ b/tests/compile-test.rs @@ -393,6 +393,7 @@ const RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS: &[&str] = &[ "search_is_some.rs", "single_component_path_imports_nested_first.rs", "string_add.rs", + "suspicious_to_owned.rs", "toplevel_ref_arg_non_rustfix.rs", "unit_arg.rs", "unnecessary_clone.rs", diff --git a/tests/ui/suspicious_to_owned.rs b/tests/ui/suspicious_to_owned.rs new file mode 100644 index 000000000..a90e78a0e --- /dev/null +++ b/tests/ui/suspicious_to_owned.rs @@ -0,0 +1,62 @@ +#![warn(clippy::suspicious_to_owned)] +#![warn(clippy::implicit_clone)] +#![allow(clippy::redundant_clone)] +use std::borrow::Cow; +use std::ffi::CStr; + +fn main() { + let moo = "Moooo"; + let c_moo = b"Moooo\0"; + let c_moo_ptr = c_moo.as_ptr() as *const i8; + let moos = ['M', 'o', 'o']; + let moos_vec = moos.to_vec(); + + // we expect this to be linted + let cow = Cow::Borrowed(moo); + let _ = cow.to_owned(); + // we expect no lints for this + let cow = Cow::Borrowed(moo); + let _ = cow.into_owned(); + // we expect no lints for this + let cow = Cow::Borrowed(moo); + let _ = cow.clone(); + + // we expect this to be linted + let cow = Cow::Borrowed(&moos); + let _ = cow.to_owned(); + // we expect no lints for this + let cow = Cow::Borrowed(&moos); + let _ = cow.into_owned(); + // we expect no lints for this + let cow = Cow::Borrowed(&moos); + let _ = cow.clone(); + + // we expect this to be linted + let cow = Cow::Borrowed(&moos_vec); + let _ = cow.to_owned(); + // we expect no lints for this + let cow = Cow::Borrowed(&moos_vec); + let _ = cow.into_owned(); + // we expect no lints for this + let cow = Cow::Borrowed(&moos_vec); + let _ = cow.clone(); + + // we expect this to be linted + let cow = unsafe { CStr::from_ptr(c_moo_ptr) }.to_string_lossy(); + let _ = cow.to_owned(); + // we expect no lints for this + let cow = unsafe { CStr::from_ptr(c_moo_ptr) }.to_string_lossy(); + let _ = cow.into_owned(); + // we expect no lints for this + let cow = unsafe { CStr::from_ptr(c_moo_ptr) }.to_string_lossy(); + let _ = cow.clone(); + + // we expect no lints for these + let _ = moo.to_owned(); + let _ = c_moo.to_owned(); + let _ = moos.to_owned(); + + // we expect implicit_clone lints for these + let _ = String::from(moo).to_owned(); + let _ = moos_vec.to_owned(); +} diff --git a/tests/ui/suspicious_to_owned.stderr b/tests/ui/suspicious_to_owned.stderr new file mode 100644 index 000000000..92e1024bf --- /dev/null +++ b/tests/ui/suspicious_to_owned.stderr @@ -0,0 +1,42 @@ +error: this `to_owned` call clones the std::borrow::Cow itself and does not cause the std::borrow::Cow contents to become owned + --> $DIR/suspicious_to_owned.rs:16:13 + | +LL | let _ = cow.to_owned(); + | ^^^^^^^^^^^^^^ help: consider using, depending on intent: `cow.clone()` or `cow.into_owned()` + | + = note: `-D clippy::suspicious-to-owned` implied by `-D warnings` + +error: this `to_owned` call clones the std::borrow::Cow<[char; 3]> itself and does not cause the std::borrow::Cow<[char; 3]> contents to become owned + --> $DIR/suspicious_to_owned.rs:26:13 + | +LL | let _ = cow.to_owned(); + | ^^^^^^^^^^^^^^ help: consider using, depending on intent: `cow.clone()` or `cow.into_owned()` + +error: this `to_owned` call clones the std::borrow::Cow> itself and does not cause the std::borrow::Cow> contents to become owned + --> $DIR/suspicious_to_owned.rs:36:13 + | +LL | let _ = cow.to_owned(); + | ^^^^^^^^^^^^^^ help: consider using, depending on intent: `cow.clone()` or `cow.into_owned()` + +error: this `to_owned` call clones the std::borrow::Cow itself and does not cause the std::borrow::Cow contents to become owned + --> $DIR/suspicious_to_owned.rs:46:13 + | +LL | let _ = cow.to_owned(); + | ^^^^^^^^^^^^^^ help: consider using, depending on intent: `cow.clone()` or `cow.into_owned()` + +error: implicitly cloning a `String` by calling `to_owned` on its dereferenced type + --> $DIR/suspicious_to_owned.rs:60:13 + | +LL | let _ = String::from(moo).to_owned(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `String::from(moo).clone()` + | + = note: `-D clippy::implicit-clone` implied by `-D warnings` + +error: implicitly cloning a `Vec` by calling `to_owned` on its dereferenced type + --> $DIR/suspicious_to_owned.rs:61:13 + | +LL | let _ = moos_vec.to_owned(); + | ^^^^^^^^^^^^^^^^^^^ help: consider using: `moos_vec.clone()` + +error: aborting due to 6 previous errors +