Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add owned_cow lint #13948

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5891,6 +5891,7 @@ Released 2018-09-13
[`out_of_bounds_indexing`]: https://rust-lang.github.io/rust-clippy/master/index.html#out_of_bounds_indexing
[`overflow_check_conditional`]: https://rust-lang.github.io/rust-clippy/master/index.html#overflow_check_conditional
[`overly_complex_bool_expr`]: https://rust-lang.github.io/rust-clippy/master/index.html#overly_complex_bool_expr
[`owned_cow`]: https://rust-lang.github.io/rust-clippy/master/index.html#owned_cow
[`panic`]: https://rust-lang.github.io/rust-clippy/master/index.html#panic
[`panic_in_result_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#panic_in_result_fn
[`panic_params`]: https://rust-lang.github.io/rust-clippy/master/index.html#panic_params
Expand Down
1 change: 1 addition & 0 deletions book/src/lint_configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,7 @@ Suppress lints whenever the suggested change would cause breakage for other crat
* [`linkedlist`](https://rust-lang.github.io/rust-clippy/master/index.html#linkedlist)
* [`needless_pass_by_ref_mut`](https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_ref_mut)
* [`option_option`](https://rust-lang.github.io/rust-clippy/master/index.html#option_option)
* [`owned_cow`](https://rust-lang.github.io/rust-clippy/master/index.html#owned_cow)
* [`rc_buffer`](https://rust-lang.github.io/rust-clippy/master/index.html#rc_buffer)
* [`rc_mutex`](https://rust-lang.github.io/rust-clippy/master/index.html#rc_mutex)
* [`redundant_allocation`](https://rust-lang.github.io/rust-clippy/master/index.html#redundant_allocation)
Expand Down
1 change: 1 addition & 0 deletions clippy_config/src/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,7 @@ define_Conf! {
linkedlist,
needless_pass_by_ref_mut,
option_option,
owned_cow,
rc_buffer,
rc_mutex,
redundant_allocation,
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -731,6 +731,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
crate::types::BOX_COLLECTION_INFO,
crate::types::LINKEDLIST_INFO,
crate::types::OPTION_OPTION_INFO,
crate::types::OWNED_COW_INFO,
crate::types::RC_BUFFER_INFO,
crate::types::RC_MUTEX_INFO,
crate::types::REDUNDANT_ALLOCATION_INFO,
Expand Down
61 changes: 60 additions & 1 deletion clippy_lints/src/types/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ mod borrowed_box;
mod box_collection;
mod linked_list;
mod option_option;
mod owned_cow;
mod rc_buffer;
mod rc_mutex;
mod redundant_allocation;
Expand Down Expand Up @@ -355,13 +356,62 @@ declare_clippy_lint! {
"usage of `Rc<Mutex<T>>`"
}

declare_clippy_lint! {
/// ### What it does
/// Detects needlessly owned `Cow` types.
///
/// ### Why is this bad?
/// The borrowed types are usually more flexible, in that e.g. a
/// `Cow<'_, str>`can accept both `&str` and `String` while
/// `Cow<'_, String>` can only accept `&String` and `String`. In
/// particular, `&str` is more general, because it allows for string
/// literals while `&String` can only be borrowed from a heap-owned
/// `String`).
///
/// ### Known Problems:
/// The lint does not check for usage of the type. There may be external
/// interfaces that require the use of an owned type.
///
/// At least the `CString` type also has a richer API than `CStr`, there
/// is no guarantee that other types won't gain additional methods leading
/// to a similar mismatch whenever such a method gets called later.
///
/// In addition, the lint only checks for the known problematic types
/// `String`, `Vec<_>`, `CString`, `OsString` and `PathBuf`. Custom types
/// that implement `ToOwned` will not be detected.
///
/// ### Example
/// ```no_run
/// let wrogn: std::borrow::Cow<'_, Vec<u8>>;
/// ```
/// Use instead:
/// ```no_run
/// let right: std::borrow::Cow<'_, [u8]>;
/// ```
#[clippy::version = "1.85.0"]
pub OWNED_COW,
style,
"needlessly owned Cow type"
}

pub struct Types {
vec_box_size_threshold: u64,
type_complexity_threshold: u64,
avoid_breaking_exported_api: bool,
}

impl_lint_pass!(Types => [BOX_COLLECTION, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, REDUNDANT_ALLOCATION, RC_BUFFER, RC_MUTEX, TYPE_COMPLEXITY]);
impl_lint_pass!(Types => [
BOX_COLLECTION,
VEC_BOX,
OPTION_OPTION,
LINKEDLIST,
BORROWED_BOX,
REDUNDANT_ALLOCATION,
RC_BUFFER,
RC_MUTEX,
TYPE_COMPLEXITY,
OWNED_COW
]);

impl<'tcx> LateLintPass<'tcx> for Types {
fn check_fn(
Expand Down Expand Up @@ -541,6 +591,7 @@ impl Types {
triggered |= option_option::check(cx, hir_ty, qpath, def_id);
triggered |= linked_list::check(cx, hir_ty, def_id);
triggered |= rc_mutex::check(cx, hir_ty, qpath, def_id);
triggered |= owned_cow::check(cx, qpath, def_id);

if triggered {
return;
Expand Down Expand Up @@ -592,6 +643,14 @@ impl Types {
QPath::LangItem(..) => {},
}
},
TyKind::Path(ref qpath) => {
let res = cx.qpath_res(qpath, hir_ty.hir_id);
if let Some(def_id) = res.opt_def_id()
&& self.is_type_change_allowed(context)
{
owned_cow::check(cx, qpath, def_id);
}
},
TyKind::Ref(lt, ref mut_ty) => {
context.is_nested_call = true;
if !borrowed_box::check(cx, hir_ty, lt, mut_ty) {
Expand Down
66 changes: 66 additions & 0 deletions clippy_lints/src/types/owned_cow.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::snippet_opt;
use rustc_errors::Applicability;
use rustc_hir::def_id::DefId;
use rustc_hir::{self as hir};
use rustc_lint::LateContext;
use rustc_span::{Span, sym};

pub(super) fn check(cx: &LateContext<'_>, qpath: &hir::QPath<'_>, def_id: DefId) -> bool {
if cx.tcx.is_diagnostic_item(sym::Cow, def_id)
&& let hir::QPath::Resolved(_, path) = qpath
&& let [.., last_seg] = path.segments
&& let Some(args) = last_seg.args
&& let [_lt, carg] = args.args
&& let hir::GenericArg::Type(cty) = carg
&& let Some((span, repl)) = replacement(cx, cty)
{
span_lint_and_sugg(
cx,
super::OWNED_COW,
span,
"needlessly owned Cow type",
"use",
repl,
Applicability::Unspecified,
);
return true;
}
false
}

fn replacement(cx: &LateContext<'_>, cty: &hir::Ty<'_>) -> Option<(Span, String)> {
if clippy_utils::is_path_lang_item(cx, cty, hir::LangItem::String) {
return Some((cty.span, "str".into()));
}
if clippy_utils::is_path_diagnostic_item(cx, cty, sym::Vec) {
return if let hir::TyKind::Path(hir::QPath::Resolved(_, path)) = cty.kind
&& let [.., last_seg] = path.segments
&& let Some(args) = last_seg.args
&& let [t, ..] = args.args
&& let Some(snip) = snippet_opt(cx, t.span())
{
Some((cty.span, format!("[{snip}]")))
} else {
None
};
}
if clippy_utils::is_path_diagnostic_item(cx, cty, sym::cstring_type) {
return Some((
cty.span,
(if clippy_utils::is_no_std_crate(cx) {
"core::ffi::CStr"
} else {
"std::ffi::CStr"
})
.into(),
));
}
// Neither OsString nor PathBuf are available outside std
for (diag, repl) in [(sym::OsString, "std::ffi::OsStr"), (sym::PathBuf, "std::path::Path")] {
if clippy_utils::is_path_diagnostic_item(cx, cty, diag) {
return Some((cty.span, repl.into()));
}
}
None
}
17 changes: 17 additions & 0 deletions tests/ui/owned_cow.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#![warn(clippy::owned_cow)]

use std::borrow::Cow;
use std::ffi::{CString, OsString};
use std::path::PathBuf;

fn main() {
let x: Cow<'static, str> = Cow::Owned(String::from("Hi!"));
let y: Cow<'_, [u8]> = Cow::Owned(vec![]);
let z: Cow<'_, [_]> = Cow::Owned(vec![2_i32]);
let o: Cow<'_, std::ffi::OsStr> = Cow::Owned(OsString::new());
let c: Cow<'_, std::ffi::CStr> = Cow::Owned(CString::new("").unwrap());
let p: Cow<'_, std::path::Path> = Cow::Owned(PathBuf::new());

// false positive: borrowed type
let b: Cow<'_, str> = Cow::Borrowed("Hi!");
}
17 changes: 17 additions & 0 deletions tests/ui/owned_cow.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#![warn(clippy::owned_cow)]

use std::borrow::Cow;
use std::ffi::{CString, OsString};
use std::path::PathBuf;

fn main() {
let x: Cow<'static, String> = Cow::Owned(String::from("Hi!"));
let y: Cow<'_, Vec<u8>> = Cow::Owned(vec![]);
let z: Cow<'_, Vec<_>> = Cow::Owned(vec![2_i32]);
let o: Cow<'_, OsString> = Cow::Owned(OsString::new());
let c: Cow<'_, CString> = Cow::Owned(CString::new("").unwrap());
let p: Cow<'_, PathBuf> = Cow::Owned(PathBuf::new());

// false positive: borrowed type
let b: Cow<'_, str> = Cow::Borrowed("Hi!");
}
41 changes: 41 additions & 0 deletions tests/ui/owned_cow.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
error: needlessly owned Cow type
--> tests/ui/owned_cow.rs:8:25
|
LL | let x: Cow<'static, String> = Cow::Owned(String::from("Hi!"));
| ^^^^^^ help: use: `str`
|
= note: `-D clippy::owned-cow` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::owned_cow)]`

error: needlessly owned Cow type
--> tests/ui/owned_cow.rs:9:20
|
LL | let y: Cow<'_, Vec<u8>> = Cow::Owned(vec![]);
| ^^^^^^^ help: use: `[u8]`

error: needlessly owned Cow type
--> tests/ui/owned_cow.rs:10:20
|
LL | let z: Cow<'_, Vec<_>> = Cow::Owned(vec![2_i32]);
| ^^^^^^ help: use: `[_]`

error: needlessly owned Cow type
--> tests/ui/owned_cow.rs:11:20
|
LL | let o: Cow<'_, OsString> = Cow::Owned(OsString::new());
| ^^^^^^^^ help: use: `std::ffi::OsStr`

error: needlessly owned Cow type
--> tests/ui/owned_cow.rs:12:20
|
LL | let c: Cow<'_, CString> = Cow::Owned(CString::new("").unwrap());
| ^^^^^^^ help: use: `std::ffi::CStr`

error: needlessly owned Cow type
--> tests/ui/owned_cow.rs:13:20
|
LL | let p: Cow<'_, PathBuf> = Cow::Owned(PathBuf::new());
| ^^^^^^^ help: use: `std::path::Path`

error: aborting due to 6 previous errors

Loading