Skip to content

Commit 7154b23

Browse files
committed
Auto merge of #6730 - anall:feature/owned_to_owned_methods, r=camsteffen
added new lint `owned_to_owned` Adding new lint `owned_to_owned` Creating draft PR to have this looked over. I think this takes all advice I received into account. I did have to update the `redundant_clone` test to ignore this lint -- I felt that was the safest action. closes: #6715 changelog: added new lint `implicit_clone`
2 parents 6343446 + 3d3cfd3 commit 7154b23

12 files changed

+289
-31
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -2104,6 +2104,7 @@ Released 2018-09-13
21042104
[`if_not_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_not_else
21052105
[`if_same_then_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_same_then_else
21062106
[`ifs_same_cond`]: https://rust-lang.github.io/rust-clippy/master/index.html#ifs_same_cond
2107+
[`implicit_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_clone
21072108
[`implicit_hasher`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_hasher
21082109
[`implicit_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_return
21092110
[`implicit_saturating_sub`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_saturating_sub

clippy_lints/src/lib.rs

+2
Original file line numberDiff line numberDiff line change
@@ -769,6 +769,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
769769
&methods::FLAT_MAP_IDENTITY,
770770
&methods::FROM_ITER_INSTEAD_OF_COLLECT,
771771
&methods::GET_UNWRAP,
772+
&methods::IMPLICIT_CLONE,
772773
&methods::INEFFICIENT_TO_STRING,
773774
&methods::INSPECT_FOR_EACH,
774775
&methods::INTO_ITER_ON_REF,
@@ -1380,6 +1381,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
13801381
LintId::of(&matches::SINGLE_MATCH_ELSE),
13811382
LintId::of(&methods::FILTER_MAP),
13821383
LintId::of(&methods::FILTER_MAP_NEXT),
1384+
LintId::of(&methods::IMPLICIT_CLONE),
13831385
LintId::of(&methods::INEFFICIENT_TO_STRING),
13841386
LintId::of(&methods::MAP_FLATTEN),
13851387
LintId::of(&methods::MAP_UNWRAP_OR),
+32
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
use crate::utils::span_lint_and_sugg;
2+
use if_chain::if_chain;
3+
use rustc_errors::Applicability;
4+
use rustc_hir as hir;
5+
use rustc_hir::ExprKind;
6+
use rustc_lint::LateContext;
7+
use rustc_middle::ty::TyS;
8+
use rustc_span::symbol::Symbol;
9+
10+
use super::IMPLICIT_CLONE;
11+
use clippy_utils::is_diagnostic_assoc_item;
12+
13+
pub fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, trait_diagnostic: Symbol) {
14+
if_chain! {
15+
if let ExprKind::MethodCall(method_path, _, [arg], _) = &expr.kind;
16+
let return_type = cx.typeck_results().expr_ty(&expr);
17+
let input_type = cx.typeck_results().expr_ty(arg).peel_refs();
18+
if let Some(expr_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id);
19+
if let Some(ty_name) = input_type.ty_adt_def().map(|adt_def| cx.tcx.item_name(adt_def.did));
20+
if TyS::same_type(return_type, input_type);
21+
if is_diagnostic_assoc_item(cx, expr_def_id, trait_diagnostic);
22+
then {
23+
span_lint_and_sugg(
24+
cx,IMPLICIT_CLONE,method_path.ident.span,
25+
&format!("implicitly cloning a `{}` by calling `{}` on its dereferenced type", ty_name, method_path.ident.name),
26+
"consider using",
27+
"clone".to_string(),
28+
Applicability::MachineApplicable
29+
);
30+
}
31+
}
32+
}

clippy_lints/src/methods/mod.rs

+32
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
mod bind_instead_of_map;
22
mod bytes_nth;
33
mod filter_map_identity;
4+
mod implicit_clone;
45
mod inefficient_to_string;
56
mod inspect_for_each;
67
mod manual_saturating_arithmetic;
@@ -1513,6 +1514,32 @@ declare_clippy_lint! {
15131514
"replace `.bytes().nth()` with `.as_bytes().get()`"
15141515
}
15151516

1517+
declare_clippy_lint! {
1518+
/// **What it does:** Checks for the usage of `_.to_owned()`, `vec.to_vec()`, or similar when calling `_.clone()` would be clearer.
1519+
///
1520+
/// **Why is this bad?** These methods do the same thing as `_.clone()` but may be confusing as
1521+
/// to why we are calling `to_vec` on something that is already a `Vec` or calling `to_owned` on something that is already owned.
1522+
///
1523+
/// **Known problems:** None.
1524+
///
1525+
/// **Example:**
1526+
///
1527+
/// ```rust
1528+
/// let a = vec![1, 2, 3];
1529+
/// let b = a.to_vec();
1530+
/// let c = a.to_owned();
1531+
/// ```
1532+
/// Use instead:
1533+
/// ```rust
1534+
/// let a = vec![1, 2, 3];
1535+
/// let b = a.clone();
1536+
/// let c = a.clone();
1537+
/// ```
1538+
pub IMPLICIT_CLONE,
1539+
pedantic,
1540+
"implicitly cloning a value by invoking a function on its dereferenced type"
1541+
}
1542+
15161543
pub struct Methods {
15171544
msrv: Option<RustcVersion>,
15181545
}
@@ -1579,6 +1606,7 @@ impl_lint_pass!(Methods => [
15791606
MAP_COLLECT_RESULT_UNIT,
15801607
FROM_ITER_INSTEAD_OF_COLLECT,
15811608
INSPECT_FOR_EACH,
1609+
IMPLICIT_CLONE
15821610
]);
15831611

15841612
impl<'tcx> LateLintPass<'tcx> for Methods {
@@ -1670,6 +1698,10 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
16701698
["ok_or_else", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "ok_or"),
16711699
["collect", "map"] => lint_map_collect(cx, expr, arg_lists[1], arg_lists[0]),
16721700
["for_each", "inspect"] => inspect_for_each::lint(cx, expr, method_spans[1]),
1701+
["to_owned", ..] => implicit_clone::check(cx, expr, sym::ToOwned),
1702+
["to_os_string", ..] => implicit_clone::check(cx, expr, sym::OsStr),
1703+
["to_path_buf", ..] => implicit_clone::check(cx, expr, sym::Path),
1704+
["to_vec", ..] => implicit_clone::check(cx, expr, sym::slice),
16731705
_ => {},
16741706
}
16751707

clippy_utils/src/lib.rs

+16
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,22 @@ pub fn match_trait_method(cx: &LateContext<'_>, expr: &Expr<'_>, path: &[&str])
237237
trt_id.map_or(false, |trt_id| match_def_path(cx, trt_id, path))
238238
}
239239

240+
/// Checks if the method call given in `expr` belongs to a trait or other container with a given
241+
/// diagnostic item
242+
pub fn is_diagnostic_assoc_item(cx: &LateContext<'_>, def_id: DefId, diag_item: Symbol) -> bool {
243+
cx.tcx
244+
.opt_associated_item(def_id)
245+
.and_then(|associated_item| match associated_item.container {
246+
ty::TraitContainer(assoc_def_id) => Some(assoc_def_id),
247+
ty::ImplContainer(assoc_def_id) => match cx.tcx.type_of(assoc_def_id).kind() {
248+
ty::Adt(adt, _) => Some(adt.did),
249+
ty::Slice(_) => cx.tcx.get_diagnostic_item(sym::slice), // this isn't perfect but it works
250+
_ => None,
251+
},
252+
})
253+
.map_or(false, |assoc_def_id| cx.tcx.is_diagnostic_item(diag_item, assoc_def_id))
254+
}
255+
240256
/// Checks if an expression references a variable of the given name.
241257
pub fn match_var(expr: &Expr<'_>, var: Symbol) -> bool {
242258
if let ExprKind::Path(QPath::Resolved(None, ref path)) = expr.kind {

tests/ui/cmp_owned/without_suggestion.rs

+1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#[allow(clippy::unnecessary_operation)]
2+
#[allow(clippy::implicit_clone)]
23

34
fn main() {
45
let x = &Baz;

tests/ui/cmp_owned/without_suggestion.stderr

+3-3
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,19 @@
11
error: this creates an owned instance just for comparison
2-
--> $DIR/without_suggestion.rs:6:5
2+
--> $DIR/without_suggestion.rs:7:5
33
|
44
LL | y.to_owned() == *x;
55
| ^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating
66
|
77
= note: `-D clippy::cmp-owned` implied by `-D warnings`
88

99
error: this creates an owned instance just for comparison
10-
--> $DIR/without_suggestion.rs:10:5
10+
--> $DIR/without_suggestion.rs:11:5
1111
|
1212
LL | y.to_owned() == **x;
1313
| ^^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating
1414

1515
error: this creates an owned instance just for comparison
16-
--> $DIR/without_suggestion.rs:17:9
16+
--> $DIR/without_suggestion.rs:18:9
1717
|
1818
LL | self.to_owned() == *other
1919
| ^^^^^^^^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating

tests/ui/implicit_clone.rs

+108
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
#![warn(clippy::implicit_clone)]
2+
#![allow(clippy::redundant_clone)]
3+
use std::borrow::Borrow;
4+
use std::ffi::{OsStr, OsString};
5+
use std::path::PathBuf;
6+
7+
fn return_owned_from_slice(slice: &[u32]) -> Vec<u32> {
8+
slice.to_owned()
9+
}
10+
11+
pub fn own_same<T>(v: T) -> T
12+
where
13+
T: ToOwned<Owned = T>,
14+
{
15+
v.to_owned()
16+
}
17+
18+
pub fn own_same_from_ref<T>(v: &T) -> T
19+
where
20+
T: ToOwned<Owned = T>,
21+
{
22+
v.to_owned()
23+
}
24+
25+
pub fn own_different<T, U>(v: T) -> U
26+
where
27+
T: ToOwned<Owned = U>,
28+
{
29+
v.to_owned()
30+
}
31+
32+
#[derive(Copy, Clone)]
33+
struct Kitten {}
34+
impl Kitten {
35+
// badly named method
36+
fn to_vec(self) -> Kitten {
37+
Kitten {}
38+
}
39+
}
40+
impl Borrow<BorrowedKitten> for Kitten {
41+
fn borrow(&self) -> &BorrowedKitten {
42+
static VALUE: BorrowedKitten = BorrowedKitten {};
43+
&VALUE
44+
}
45+
}
46+
47+
struct BorrowedKitten {}
48+
impl ToOwned for BorrowedKitten {
49+
type Owned = Kitten;
50+
fn to_owned(&self) -> Kitten {
51+
Kitten {}
52+
}
53+
}
54+
55+
mod weird {
56+
#[allow(clippy::ptr_arg)]
57+
pub fn to_vec(v: &Vec<u32>) -> Vec<u32> {
58+
v.clone()
59+
}
60+
}
61+
62+
fn main() {
63+
let vec = vec![5];
64+
let _ = return_owned_from_slice(&vec);
65+
let _ = vec.to_owned();
66+
let _ = vec.to_vec();
67+
68+
let vec_ref = &vec;
69+
let _ = return_owned_from_slice(&vec_ref);
70+
let _ = vec_ref.to_owned();
71+
let _ = vec_ref.to_vec();
72+
73+
// we expect no lint for this
74+
let _ = weird::to_vec(&vec);
75+
76+
// we expect no lints for this
77+
let slice: &[u32] = &[1, 2, 3, 4, 5];
78+
let _ = return_owned_from_slice(slice);
79+
let _ = slice.to_owned();
80+
let _ = slice.to_vec();
81+
82+
let str = "hello world".to_string();
83+
let _ = str.to_owned();
84+
85+
// testing w/ an arbitrary type
86+
let kitten = Kitten {};
87+
let _ = kitten.to_owned();
88+
let _ = own_same_from_ref(&kitten);
89+
// this shouln't lint
90+
let _ = kitten.to_vec();
91+
92+
// we expect no lints for this
93+
let borrowed = BorrowedKitten {};
94+
let _ = borrowed.to_owned();
95+
96+
let pathbuf = PathBuf::new();
97+
let _ = pathbuf.to_owned();
98+
let _ = pathbuf.to_path_buf();
99+
100+
let os_string = OsString::from("foo");
101+
let _ = os_string.to_owned();
102+
let _ = os_string.to_os_string();
103+
104+
// we expect no lints for this
105+
let os_str = OsStr::new("foo");
106+
let _ = os_str.to_owned();
107+
let _ = os_str.to_os_string();
108+
}

tests/ui/implicit_clone.stderr

+64
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
error: implicitly cloning a `Vec` by calling `to_owned` on its dereferenced type
2+
--> $DIR/implicit_clone.rs:65:17
3+
|
4+
LL | let _ = vec.to_owned();
5+
| ^^^^^^^^ help: consider using: `clone`
6+
|
7+
= note: `-D clippy::implicit-clone` implied by `-D warnings`
8+
9+
error: implicitly cloning a `Vec` by calling `to_vec` on its dereferenced type
10+
--> $DIR/implicit_clone.rs:66:17
11+
|
12+
LL | let _ = vec.to_vec();
13+
| ^^^^^^ help: consider using: `clone`
14+
15+
error: implicitly cloning a `Vec` by calling `to_owned` on its dereferenced type
16+
--> $DIR/implicit_clone.rs:70:21
17+
|
18+
LL | let _ = vec_ref.to_owned();
19+
| ^^^^^^^^ help: consider using: `clone`
20+
21+
error: implicitly cloning a `Vec` by calling `to_vec` on its dereferenced type
22+
--> $DIR/implicit_clone.rs:71:21
23+
|
24+
LL | let _ = vec_ref.to_vec();
25+
| ^^^^^^ help: consider using: `clone`
26+
27+
error: implicitly cloning a `String` by calling `to_owned` on its dereferenced type
28+
--> $DIR/implicit_clone.rs:83:17
29+
|
30+
LL | let _ = str.to_owned();
31+
| ^^^^^^^^ help: consider using: `clone`
32+
33+
error: implicitly cloning a `Kitten` by calling `to_owned` on its dereferenced type
34+
--> $DIR/implicit_clone.rs:87:20
35+
|
36+
LL | let _ = kitten.to_owned();
37+
| ^^^^^^^^ help: consider using: `clone`
38+
39+
error: implicitly cloning a `PathBuf` by calling `to_owned` on its dereferenced type
40+
--> $DIR/implicit_clone.rs:97:21
41+
|
42+
LL | let _ = pathbuf.to_owned();
43+
| ^^^^^^^^ help: consider using: `clone`
44+
45+
error: implicitly cloning a `PathBuf` by calling `to_path_buf` on its dereferenced type
46+
--> $DIR/implicit_clone.rs:98:21
47+
|
48+
LL | let _ = pathbuf.to_path_buf();
49+
| ^^^^^^^^^^^ help: consider using: `clone`
50+
51+
error: implicitly cloning a `OsString` by calling `to_owned` on its dereferenced type
52+
--> $DIR/implicit_clone.rs:101:23
53+
|
54+
LL | let _ = os_string.to_owned();
55+
| ^^^^^^^^ help: consider using: `clone`
56+
57+
error: implicitly cloning a `OsString` by calling `to_os_string` on its dereferenced type
58+
--> $DIR/implicit_clone.rs:102:23
59+
|
60+
LL | let _ = os_string.to_os_string();
61+
| ^^^^^^^^^^^^ help: consider using: `clone`
62+
63+
error: aborting due to 10 previous errors
64+

tests/ui/redundant_clone.fixed

+1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// run-rustfix
22
// rustfix-only-machine-applicable
33

4+
#![allow(clippy::implicit_clone)]
45
use std::ffi::OsString;
56
use std::path::Path;
67

tests/ui/redundant_clone.rs

+1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// run-rustfix
22
// rustfix-only-machine-applicable
33

4+
#![allow(clippy::implicit_clone)]
45
use std::ffi::OsString;
56
use std::path::Path;
67

0 commit comments

Comments
 (0)