Implemented suspicious_to_owned lint to check if `to_owned` is called on a `Cow`.

This is done because `to_owned` is very similarly named to `into_owned`, but the
effect of calling those two methods is completely different. This creates
confusion (stemming from the ambiguity of the 'owned' term in the context of
`Cow`s) and might not be what the writer intended.

9 files changed, 202 insertions, 1 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 964f339b26c..a076997eebd 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 61bb5d1337c..64982851b56 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 0ec2884cb6b..fbf77fb734f 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 6f480c52cc9..aa4e571dbe2 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 0ec35ddc1db..a0d190a58af 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;
@@ -2055,6 +2056,55 @@ declare_clippy_lint! {
 
 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`]
     /// (https://doc.rust-lang.org/std/primitive.str.html#method.splitn) and
     /// related functions with either zero or one splits.
@@ -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 00000000000..6b306fbf008
--- /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 5431f2ebc0a..ba6186e599e 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 00000000000..a90e78a0ea0
--- /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 00000000000..92e1024bf1f
--- /dev/null
+++ b/tests/ui/suspicious_to_owned.stderr
@@ -0,0 +1,42 @@
+error: this `to_owned` call clones the std::borrow::Cow<str> itself and does not cause the std::borrow::Cow<str> 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<std::vec::Vec<char>> itself and does not cause the std::borrow::Cow<std::vec::Vec<char>> 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<str> itself and does not cause the std::borrow::Cow<str> 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
+