diff --git a/CHANGELOG.md b/CHANGELOG.md index d96c74b6e412..687f74472020 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2104,6 +2104,7 @@ Released 2018-09-13 [`if_not_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_not_else [`if_same_then_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_same_then_else [`ifs_same_cond`]: https://rust-lang.github.io/rust-clippy/master/index.html#ifs_same_cond +[`implicit_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_clone [`implicit_hasher`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_hasher [`implicit_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_return [`implicit_saturating_sub`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_saturating_sub diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index faec9ec31f32..fa8f03eb4453 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -769,6 +769,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &methods::FLAT_MAP_IDENTITY, &methods::FROM_ITER_INSTEAD_OF_COLLECT, &methods::GET_UNWRAP, + &methods::IMPLICIT_CLONE, &methods::INEFFICIENT_TO_STRING, &methods::INSPECT_FOR_EACH, &methods::INTO_ITER_ON_REF, @@ -1380,6 +1381,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&matches::SINGLE_MATCH_ELSE), LintId::of(&methods::FILTER_MAP), LintId::of(&methods::FILTER_MAP_NEXT), + LintId::of(&methods::IMPLICIT_CLONE), LintId::of(&methods::INEFFICIENT_TO_STRING), LintId::of(&methods::MAP_FLATTEN), LintId::of(&methods::MAP_UNWRAP_OR), diff --git a/clippy_lints/src/methods/implicit_clone.rs b/clippy_lints/src/methods/implicit_clone.rs new file mode 100644 index 000000000000..a769493d11d3 --- /dev/null +++ b/clippy_lints/src/methods/implicit_clone.rs @@ -0,0 +1,32 @@ +use crate::utils::span_lint_and_sugg; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir as hir; +use rustc_hir::ExprKind; +use rustc_lint::LateContext; +use rustc_middle::ty::TyS; +use rustc_span::symbol::Symbol; + +use super::IMPLICIT_CLONE; +use clippy_utils::is_diagnostic_assoc_item; + +pub fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, trait_diagnostic: Symbol) { + if_chain! { + if let ExprKind::MethodCall(method_path, _, [arg], _) = &expr.kind; + let return_type = cx.typeck_results().expr_ty(&expr); + let input_type = cx.typeck_results().expr_ty(arg).peel_refs(); + if let Some(expr_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id); + if let Some(ty_name) = input_type.ty_adt_def().map(|adt_def| cx.tcx.item_name(adt_def.did)); + if TyS::same_type(return_type, input_type); + if is_diagnostic_assoc_item(cx, expr_def_id, trait_diagnostic); + then { + span_lint_and_sugg( + cx,IMPLICIT_CLONE,method_path.ident.span, + &format!("implicitly cloning a `{}` by calling `{}` on its dereferenced type", ty_name, method_path.ident.name), + "consider using", + "clone".to_string(), + Applicability::MachineApplicable + ); + } + } +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 5fb3ae1890df..6f491144435a 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1,6 +1,7 @@ mod bind_instead_of_map; mod bytes_nth; mod filter_map_identity; +mod implicit_clone; mod inefficient_to_string; mod inspect_for_each; mod manual_saturating_arithmetic; @@ -1513,6 +1514,32 @@ declare_clippy_lint! { "replace `.bytes().nth()` with `.as_bytes().get()`" } +declare_clippy_lint! { + /// **What it does:** Checks for the usage of `_.to_owned()`, `vec.to_vec()`, or similar when calling `_.clone()` would be clearer. + /// + /// **Why is this bad?** These methods do the same thing as `_.clone()` but may be confusing as + /// to why we are calling `to_vec` on something that is already a `Vec` or calling `to_owned` on something that is already owned. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// let a = vec![1, 2, 3]; + /// let b = a.to_vec(); + /// let c = a.to_owned(); + /// ``` + /// Use instead: + /// ```rust + /// let a = vec![1, 2, 3]; + /// let b = a.clone(); + /// let c = a.clone(); + /// ``` + pub IMPLICIT_CLONE, + pedantic, + "implicitly cloning a value by invoking a function on its dereferenced type" +} + pub struct Methods { msrv: Option, } @@ -1579,6 +1606,7 @@ impl_lint_pass!(Methods => [ MAP_COLLECT_RESULT_UNIT, FROM_ITER_INSTEAD_OF_COLLECT, INSPECT_FOR_EACH, + IMPLICIT_CLONE ]); impl<'tcx> LateLintPass<'tcx> for Methods { @@ -1670,6 +1698,10 @@ impl<'tcx> LateLintPass<'tcx> for Methods { ["ok_or_else", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "ok_or"), ["collect", "map"] => lint_map_collect(cx, expr, arg_lists[1], arg_lists[0]), ["for_each", "inspect"] => inspect_for_each::lint(cx, expr, method_spans[1]), + ["to_owned", ..] => implicit_clone::check(cx, expr, sym::ToOwned), + ["to_os_string", ..] => implicit_clone::check(cx, expr, sym::OsStr), + ["to_path_buf", ..] => implicit_clone::check(cx, expr, sym::Path), + ["to_vec", ..] => implicit_clone::check(cx, expr, sym::slice), _ => {}, } diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 8e5cf5dc6545..2380ea4c7bfa 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -237,6 +237,22 @@ pub fn match_trait_method(cx: &LateContext<'_>, expr: &Expr<'_>, path: &[&str]) trt_id.map_or(false, |trt_id| match_def_path(cx, trt_id, path)) } +/// Checks if the method call given in `expr` belongs to a trait or other container with a given +/// diagnostic item +pub fn is_diagnostic_assoc_item(cx: &LateContext<'_>, def_id: DefId, diag_item: Symbol) -> bool { + cx.tcx + .opt_associated_item(def_id) + .and_then(|associated_item| match associated_item.container { + ty::TraitContainer(assoc_def_id) => Some(assoc_def_id), + ty::ImplContainer(assoc_def_id) => match cx.tcx.type_of(assoc_def_id).kind() { + ty::Adt(adt, _) => Some(adt.did), + ty::Slice(_) => cx.tcx.get_diagnostic_item(sym::slice), // this isn't perfect but it works + _ => None, + }, + }) + .map_or(false, |assoc_def_id| cx.tcx.is_diagnostic_item(diag_item, assoc_def_id)) +} + /// Checks if an expression references a variable of the given name. pub fn match_var(expr: &Expr<'_>, var: Symbol) -> bool { if let ExprKind::Path(QPath::Resolved(None, ref path)) = expr.kind { diff --git a/tests/ui/cmp_owned/without_suggestion.rs b/tests/ui/cmp_owned/without_suggestion.rs index 9ab8795474c6..f44a3901fb48 100644 --- a/tests/ui/cmp_owned/without_suggestion.rs +++ b/tests/ui/cmp_owned/without_suggestion.rs @@ -1,4 +1,5 @@ #[allow(clippy::unnecessary_operation)] +#[allow(clippy::implicit_clone)] fn main() { let x = &Baz; diff --git a/tests/ui/cmp_owned/without_suggestion.stderr b/tests/ui/cmp_owned/without_suggestion.stderr index 6e8a5ad2a17b..2ea3d8fac0d1 100644 --- a/tests/ui/cmp_owned/without_suggestion.stderr +++ b/tests/ui/cmp_owned/without_suggestion.stderr @@ -1,5 +1,5 @@ error: this creates an owned instance just for comparison - --> $DIR/without_suggestion.rs:6:5 + --> $DIR/without_suggestion.rs:7:5 | LL | y.to_owned() == *x; | ^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating @@ -7,13 +7,13 @@ LL | y.to_owned() == *x; = note: `-D clippy::cmp-owned` implied by `-D warnings` error: this creates an owned instance just for comparison - --> $DIR/without_suggestion.rs:10:5 + --> $DIR/without_suggestion.rs:11:5 | LL | y.to_owned() == **x; | ^^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating error: this creates an owned instance just for comparison - --> $DIR/without_suggestion.rs:17:9 + --> $DIR/without_suggestion.rs:18:9 | LL | self.to_owned() == *other | ^^^^^^^^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating diff --git a/tests/ui/implicit_clone.rs b/tests/ui/implicit_clone.rs new file mode 100644 index 000000000000..19101522163f --- /dev/null +++ b/tests/ui/implicit_clone.rs @@ -0,0 +1,108 @@ +#![warn(clippy::implicit_clone)] +#![allow(clippy::redundant_clone)] +use std::borrow::Borrow; +use std::ffi::{OsStr, OsString}; +use std::path::PathBuf; + +fn return_owned_from_slice(slice: &[u32]) -> Vec { + slice.to_owned() +} + +pub fn own_same(v: T) -> T +where + T: ToOwned, +{ + v.to_owned() +} + +pub fn own_same_from_ref(v: &T) -> T +where + T: ToOwned, +{ + v.to_owned() +} + +pub fn own_different(v: T) -> U +where + T: ToOwned, +{ + v.to_owned() +} + +#[derive(Copy, Clone)] +struct Kitten {} +impl Kitten { + // badly named method + fn to_vec(self) -> Kitten { + Kitten {} + } +} +impl Borrow for Kitten { + fn borrow(&self) -> &BorrowedKitten { + static VALUE: BorrowedKitten = BorrowedKitten {}; + &VALUE + } +} + +struct BorrowedKitten {} +impl ToOwned for BorrowedKitten { + type Owned = Kitten; + fn to_owned(&self) -> Kitten { + Kitten {} + } +} + +mod weird { + #[allow(clippy::ptr_arg)] + pub fn to_vec(v: &Vec) -> Vec { + v.clone() + } +} + +fn main() { + let vec = vec![5]; + let _ = return_owned_from_slice(&vec); + let _ = vec.to_owned(); + let _ = vec.to_vec(); + + let vec_ref = &vec; + let _ = return_owned_from_slice(&vec_ref); + let _ = vec_ref.to_owned(); + let _ = vec_ref.to_vec(); + + // we expect no lint for this + let _ = weird::to_vec(&vec); + + // we expect no lints for this + let slice: &[u32] = &[1, 2, 3, 4, 5]; + let _ = return_owned_from_slice(slice); + let _ = slice.to_owned(); + let _ = slice.to_vec(); + + let str = "hello world".to_string(); + let _ = str.to_owned(); + + // testing w/ an arbitrary type + let kitten = Kitten {}; + let _ = kitten.to_owned(); + let _ = own_same_from_ref(&kitten); + // this shouln't lint + let _ = kitten.to_vec(); + + // we expect no lints for this + let borrowed = BorrowedKitten {}; + let _ = borrowed.to_owned(); + + let pathbuf = PathBuf::new(); + let _ = pathbuf.to_owned(); + let _ = pathbuf.to_path_buf(); + + let os_string = OsString::from("foo"); + let _ = os_string.to_owned(); + let _ = os_string.to_os_string(); + + // we expect no lints for this + let os_str = OsStr::new("foo"); + let _ = os_str.to_owned(); + let _ = os_str.to_os_string(); +} diff --git a/tests/ui/implicit_clone.stderr b/tests/ui/implicit_clone.stderr new file mode 100644 index 000000000000..e6f7527b6721 --- /dev/null +++ b/tests/ui/implicit_clone.stderr @@ -0,0 +1,64 @@ +error: implicitly cloning a `Vec` by calling `to_owned` on its dereferenced type + --> $DIR/implicit_clone.rs:65:17 + | +LL | let _ = vec.to_owned(); + | ^^^^^^^^ help: consider using: `clone` + | + = note: `-D clippy::implicit-clone` implied by `-D warnings` + +error: implicitly cloning a `Vec` by calling `to_vec` on its dereferenced type + --> $DIR/implicit_clone.rs:66:17 + | +LL | let _ = vec.to_vec(); + | ^^^^^^ help: consider using: `clone` + +error: implicitly cloning a `Vec` by calling `to_owned` on its dereferenced type + --> $DIR/implicit_clone.rs:70:21 + | +LL | let _ = vec_ref.to_owned(); + | ^^^^^^^^ help: consider using: `clone` + +error: implicitly cloning a `Vec` by calling `to_vec` on its dereferenced type + --> $DIR/implicit_clone.rs:71:21 + | +LL | let _ = vec_ref.to_vec(); + | ^^^^^^ help: consider using: `clone` + +error: implicitly cloning a `String` by calling `to_owned` on its dereferenced type + --> $DIR/implicit_clone.rs:83:17 + | +LL | let _ = str.to_owned(); + | ^^^^^^^^ help: consider using: `clone` + +error: implicitly cloning a `Kitten` by calling `to_owned` on its dereferenced type + --> $DIR/implicit_clone.rs:87:20 + | +LL | let _ = kitten.to_owned(); + | ^^^^^^^^ help: consider using: `clone` + +error: implicitly cloning a `PathBuf` by calling `to_owned` on its dereferenced type + --> $DIR/implicit_clone.rs:97:21 + | +LL | let _ = pathbuf.to_owned(); + | ^^^^^^^^ help: consider using: `clone` + +error: implicitly cloning a `PathBuf` by calling `to_path_buf` on its dereferenced type + --> $DIR/implicit_clone.rs:98:21 + | +LL | let _ = pathbuf.to_path_buf(); + | ^^^^^^^^^^^ help: consider using: `clone` + +error: implicitly cloning a `OsString` by calling `to_owned` on its dereferenced type + --> $DIR/implicit_clone.rs:101:23 + | +LL | let _ = os_string.to_owned(); + | ^^^^^^^^ help: consider using: `clone` + +error: implicitly cloning a `OsString` by calling `to_os_string` on its dereferenced type + --> $DIR/implicit_clone.rs:102:23 + | +LL | let _ = os_string.to_os_string(); + | ^^^^^^^^^^^^ help: consider using: `clone` + +error: aborting due to 10 previous errors + diff --git a/tests/ui/redundant_clone.fixed b/tests/ui/redundant_clone.fixed index cdeefda4c234..a5847e37c976 100644 --- a/tests/ui/redundant_clone.fixed +++ b/tests/ui/redundant_clone.fixed @@ -1,6 +1,7 @@ // run-rustfix // rustfix-only-machine-applicable +#![allow(clippy::implicit_clone)] use std::ffi::OsString; use std::path::Path; diff --git a/tests/ui/redundant_clone.rs b/tests/ui/redundant_clone.rs index acb7ffb305f2..dab8d7fb1c72 100644 --- a/tests/ui/redundant_clone.rs +++ b/tests/ui/redundant_clone.rs @@ -1,6 +1,7 @@ // run-rustfix // rustfix-only-machine-applicable +#![allow(clippy::implicit_clone)] use std::ffi::OsString; use std::path::Path; diff --git a/tests/ui/redundant_clone.stderr b/tests/ui/redundant_clone.stderr index 89b392542991..87c219316ce4 100644 --- a/tests/ui/redundant_clone.stderr +++ b/tests/ui/redundant_clone.stderr @@ -1,168 +1,168 @@ error: redundant clone - --> $DIR/redundant_clone.rs:8:42 + --> $DIR/redundant_clone.rs:9:42 | LL | let _s = ["lorem", "ipsum"].join(" ").to_string(); | ^^^^^^^^^^^^ help: remove this | = note: `-D clippy::redundant-clone` implied by `-D warnings` note: this value is dropped without further use - --> $DIR/redundant_clone.rs:8:14 + --> $DIR/redundant_clone.rs:9:14 | LL | let _s = ["lorem", "ipsum"].join(" ").to_string(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: redundant clone - --> $DIR/redundant_clone.rs:11:15 + --> $DIR/redundant_clone.rs:12:15 | LL | let _s = s.clone(); | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:11:14 + --> $DIR/redundant_clone.rs:12:14 | LL | let _s = s.clone(); | ^ error: redundant clone - --> $DIR/redundant_clone.rs:14:15 + --> $DIR/redundant_clone.rs:15:15 | LL | let _s = s.to_string(); | ^^^^^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:14:14 + --> $DIR/redundant_clone.rs:15:14 | LL | let _s = s.to_string(); | ^ error: redundant clone - --> $DIR/redundant_clone.rs:17:15 + --> $DIR/redundant_clone.rs:18:15 | LL | let _s = s.to_owned(); | ^^^^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:17:14 + --> $DIR/redundant_clone.rs:18:14 | LL | let _s = s.to_owned(); | ^ error: redundant clone - --> $DIR/redundant_clone.rs:19:42 + --> $DIR/redundant_clone.rs:20:42 | LL | let _s = Path::new("/a/b/").join("c").to_owned(); | ^^^^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:19:14 + --> $DIR/redundant_clone.rs:20:14 | LL | let _s = Path::new("/a/b/").join("c").to_owned(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: redundant clone - --> $DIR/redundant_clone.rs:21:42 + --> $DIR/redundant_clone.rs:22:42 | LL | let _s = Path::new("/a/b/").join("c").to_path_buf(); | ^^^^^^^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:21:14 + --> $DIR/redundant_clone.rs:22:14 | LL | let _s = Path::new("/a/b/").join("c").to_path_buf(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: redundant clone - --> $DIR/redundant_clone.rs:23:29 + --> $DIR/redundant_clone.rs:24:29 | LL | let _s = OsString::new().to_owned(); | ^^^^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:23:14 + --> $DIR/redundant_clone.rs:24:14 | LL | let _s = OsString::new().to_owned(); | ^^^^^^^^^^^^^^^ error: redundant clone - --> $DIR/redundant_clone.rs:25:29 + --> $DIR/redundant_clone.rs:26:29 | LL | let _s = OsString::new().to_os_string(); | ^^^^^^^^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:25:14 + --> $DIR/redundant_clone.rs:26:14 | LL | let _s = OsString::new().to_os_string(); | ^^^^^^^^^^^^^^^ error: redundant clone - --> $DIR/redundant_clone.rs:32:19 + --> $DIR/redundant_clone.rs:33:19 | LL | let _t = tup.0.clone(); | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:32:14 + --> $DIR/redundant_clone.rs:33:14 | LL | let _t = tup.0.clone(); | ^^^^^ error: redundant clone - --> $DIR/redundant_clone.rs:62:22 + --> $DIR/redundant_clone.rs:63:22 | LL | (a.clone(), a.clone()) | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:62:21 + --> $DIR/redundant_clone.rs:63:21 | LL | (a.clone(), a.clone()) | ^ error: redundant clone - --> $DIR/redundant_clone.rs:122:15 + --> $DIR/redundant_clone.rs:123:15 | LL | let _s = s.clone(); | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:122:14 + --> $DIR/redundant_clone.rs:123:14 | LL | let _s = s.clone(); | ^ error: redundant clone - --> $DIR/redundant_clone.rs:123:15 + --> $DIR/redundant_clone.rs:124:15 | LL | let _t = t.clone(); | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:123:14 + --> $DIR/redundant_clone.rs:124:14 | LL | let _t = t.clone(); | ^ error: redundant clone - --> $DIR/redundant_clone.rs:133:19 + --> $DIR/redundant_clone.rs:134:19 | LL | let _f = f.clone(); | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:133:18 + --> $DIR/redundant_clone.rs:134:18 | LL | let _f = f.clone(); | ^ error: redundant clone - --> $DIR/redundant_clone.rs:145:14 + --> $DIR/redundant_clone.rs:146:14 | LL | let y = x.clone().join("matthias"); | ^^^^^^^^ help: remove this | note: cloned value is neither consumed nor mutated - --> $DIR/redundant_clone.rs:145:13 + --> $DIR/redundant_clone.rs:146:13 | LL | let y = x.clone().join("matthias"); | ^^^^^^^^^