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.
This commit is contained in:
Marco Mastropaolo 2022-06-10 18:28:31 +02:00 committed by Marco Mastropaolo
parent 602bec26b0
commit de028e2fb9
9 changed files with 202 additions and 1 deletions

View file

@ -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),

View file

@ -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,

View file

@ -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),

View file

@ -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", []) => {

View file

@ -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
}