Skip to content

Commit 28dbcd8

Browse files
borsbors
committed
Auto merge of #7098 - camsteffen:cloned-copied, r=Manishearth
Add `cloned_instead_of_copied` lint Don't go cloning all willy-nilly. Featuring a new `get_iterator_item_ty` util! changelog: Add cloned_instead_of_copied lint Closes #3870
2 parents 7f2068c + b049c88 commit 28dbcd8

17 files changed

+163
-13
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2148,6 +2148,7 @@ Released 2018-09-13
21482148
[`clone_double_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_double_ref
21492149
[`clone_on_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
21502150
[`clone_on_ref_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_ref_ptr
2151+
[`cloned_instead_of_copied`]: https://rust-lang.github.io/rust-clippy/master/index.html#cloned_instead_of_copied
21512152
[`cmp_nan`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_nan
21522153
[`cmp_null`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_null
21532154
[`cmp_owned`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_owned

‎clippy_lints/src/booleans.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,7 @@ fn simplify_not(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<String> {
261261
}
262262
METHODS_WITH_NEGATION
263263
.iter()
264-
.cloned()
264+
.copied()
265265
.flat_map(|(a, b)| vec![(a, b), (b, a)])
266266
.find(|&(a, _)| {
267267
let path: &str = &path.ident.name.as_str();

‎clippy_lints/src/checked_conversions.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -323,7 +323,7 @@ fn get_implementing_type<'a>(path: &QPath<'_>, candidates: &'a [&str], function:
323323
if let [int] = &*tp.segments;
324324
then {
325325
let name = &int.ident.name.as_str();
326-
candidates.iter().find(|c| name == *c).cloned()
326+
candidates.iter().find(|c| name == *c).copied()
327327
} else {
328328
None
329329
}
@@ -337,7 +337,7 @@ fn int_ty_to_sym<'tcx>(path: &QPath<'_>) -> Option<&'tcx str> {
337337
if let [ty] = &*path.segments;
338338
then {
339339
let name = &ty.ident.name.as_str();
340-
INTS.iter().find(|c| name == *c).cloned()
340+
INTS.iter().find(|c| name == *c).copied()
341341
} else {
342342
None
343343
}

‎clippy_lints/src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -759,6 +759,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
759759
methods::BYTES_NTH,
760760
methods::CHARS_LAST_CMP,
761761
methods::CHARS_NEXT_CMP,
762+
methods::CLONED_INSTEAD_OF_COPIED,
762763
methods::CLONE_DOUBLE_REF,
763764
methods::CLONE_ON_COPY,
764765
methods::CLONE_ON_REF_PTR,
@@ -1380,6 +1381,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
13801381
LintId::of(matches::MATCH_WILDCARD_FOR_SINGLE_VARIANTS),
13811382
LintId::of(matches::MATCH_WILD_ERR_ARM),
13821383
LintId::of(matches::SINGLE_MATCH_ELSE),
1384+
LintId::of(methods::CLONED_INSTEAD_OF_COPIED),
13831385
LintId::of(methods::FILTER_MAP_NEXT),
13841386
LintId::of(methods::IMPLICIT_CLONE),
13851387
LintId::of(methods::INEFFICIENT_TO_STRING),

‎clippy_lints/src/loops/never_loop.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ fn never_loop_expr(expr: &Expr<'_>, main_loop_id: HirId) -> NeverLoopResult {
100100
ExprKind::Binary(_, e1, e2)
101101
| ExprKind::Assign(e1, e2, _)
102102
| ExprKind::AssignOp(_, e1, e2)
103-
| ExprKind::Index(e1, e2) => never_loop_expr_all(&mut [e1, e2].iter().cloned(), main_loop_id),
103+
| ExprKind::Index(e1, e2) => never_loop_expr_all(&mut [e1, e2].iter().copied(), main_loop_id),
104104
ExprKind::Loop(b, _, _, _) => {
105105
// Break can come from the inner loop so remove them.
106106
absorb_break(&never_loop_block(b, main_loop_id))

‎clippy_lints/src/matches.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1129,7 +1129,7 @@ fn check_wild_enum_match(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>])
11291129
Applicability::MaybeIncorrect,
11301130
),
11311131
variants => {
1132-
let mut suggestions: Vec<_> = variants.iter().cloned().map(format_suggestion).collect();
1132+
let mut suggestions: Vec<_> = variants.iter().copied().map(format_suggestion).collect();
11331133
let message = if adt_def.is_variant_list_non_exhaustive() {
11341134
suggestions.push("_".into());
11351135
"wildcard matches known variants and will also match future added variants"

‎clippy_lints/src/methods/cloned_instead_of_copied.rs

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
use clippy_utils::diagnostics::span_lint_and_sugg;
2+
use clippy_utils::is_trait_method;
3+
use clippy_utils::ty::{get_iterator_item_ty, is_copy};
4+
use rustc_errors::Applicability;
5+
use rustc_hir::Expr;
6+
use rustc_lint::LateContext;
7+
use rustc_middle::ty;
8+
use rustc_span::{sym, Span};
9+
10+
use super::CLONED_INSTEAD_OF_COPIED;
11+
12+
pub fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span: Span) {
13+
let recv_ty = cx.typeck_results().expr_ty_adjusted(recv);
14+
let inner_ty = match recv_ty.kind() {
15+
// `Option<T>` -> `T`
16+
ty::Adt(adt, subst) if cx.tcx.is_diagnostic_item(sym::option_type, adt.did) => subst.type_at(0),
17+
_ if is_trait_method(cx, expr, sym::Iterator) => match get_iterator_item_ty(cx, recv_ty) {
18+
// <T as Iterator>::Item
19+
Some(ty) => ty,
20+
_ => return,
21+
},
22+
_ => return,
23+
};
24+
match inner_ty.kind() {
25+
// &T where T: Copy
26+
ty::Ref(_, ty, _) if is_copy(cx, ty) => {},
27+
_ => return,
28+
};
29+
span_lint_and_sugg(
30+
cx,
31+
CLONED_INSTEAD_OF_COPIED,
32+
span,
33+
"used `cloned` where `copied` could be used instead",
34+
"try",
35+
"copied".into(),
36+
Applicability::MachineApplicable,
37+
)
38+
}

‎clippy_lints/src/methods/mod.rs

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ mod chars_next_cmp;
88
mod chars_next_cmp_with_unwrap;
99
mod clone_on_copy;
1010
mod clone_on_ref_ptr;
11+
mod cloned_instead_of_copied;
1112
mod expect_fun_call;
1213
mod expect_used;
1314
mod filetype_is_file;
@@ -73,6 +74,29 @@ use rustc_span::symbol::SymbolStr;
7374
use rustc_span::{sym, Span};
7475
use rustc_typeck::hir_ty_to_ty;
7576

77+
declare_clippy_lint! {
78+
/// **What it does:** Checks for usages of `cloned()` on an `Iterator` or `Option` where
79+
/// `copied()` could be used instead.
80+
///
81+
/// **Why is this bad?** `copied()` is better because it guarantees that the type being cloned
82+
/// implements `Copy`.
83+
///
84+
/// **Known problems:** None.
85+
///
86+
/// **Example:**
87+
///
88+
/// ```rust
89+
/// [1, 2, 3].iter().cloned();
90+
/// ```
91+
/// Use instead:
92+
/// ```rust
93+
/// [1, 2, 3].iter().copied();
94+
/// ```
95+
pub CLONED_INSTEAD_OF_COPIED,
96+
pedantic,
97+
"used `cloned` where `copied` could be used instead"
98+
}
99+
76100
declare_clippy_lint! {
77101
/// **What it does:** Checks for `.unwrap()` calls on `Option`s and on `Result`s.
78102
///
@@ -1638,6 +1662,7 @@ impl_lint_pass!(Methods => [
16381662
CLONE_ON_COPY,
16391663
CLONE_ON_REF_PTR,
16401664
CLONE_DOUBLE_REF,
1665+
CLONED_INSTEAD_OF_COPIED,
16411666
INEFFICIENT_TO_STRING,
16421667
NEW_RET_NO_SELF,
16431668
SINGLE_CHAR_PATTERN,
@@ -1909,6 +1934,7 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
19091934
("as_mut", []) => useless_asref::check(cx, expr, "as_mut", recv),
19101935
("as_ref", []) => useless_asref::check(cx, expr, "as_ref", recv),
19111936
("assume_init", []) => uninit_assumed_init::check(cx, expr, recv),
1937+
("cloned", []) => cloned_instead_of_copied::check(cx, expr, recv, span),
19121938
("collect", []) => match method_call!(recv) {
19131939
Some(("cloned", [recv2], _)) => iter_cloned_collect::check(cx, expr, recv2),
19141940
Some(("map", [m_recv, m_arg], _)) => {

‎clippy_lints/src/needless_pass_by_value.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -279,7 +279,7 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByValue {
279279
spans.extend(
280280
deref_span
281281
.iter()
282-
.cloned()
282+
.copied()
283283
.map(|span| (span, format!("*{}", snippet(cx, span, "<expr>")))),
284284
);
285285
spans.sort_by_key(|&(span, _)| span);

‎clippy_lints/src/suspicious_operation_groupings.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ fn attempt_to_emit_no_difference_lint(
195195
i: usize,
196196
expected_loc: IdentLocation,
197197
) {
198-
if let Some(binop) = binops.get(i).cloned() {
198+
if let Some(binop) = binops.get(i).copied() {
199199
// We need to try and figure out which identifier we should
200200
// suggest using instead. Since there could be multiple
201201
// replacement candidates in a given expression, and we're

‎clippy_lints/src/write.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -573,7 +573,7 @@ impl Write {
573573
diag.multipart_suggestion(
574574
"try this",
575575
iter::once((comma_span.to(token_expr.span), String::new()))
576-
.chain(fmt_spans.iter().cloned().zip(iter::repeat(replacement)))
576+
.chain(fmt_spans.iter().copied().zip(iter::repeat(replacement)))
577577
.collect(),
578578
Applicability::MachineApplicable,
579579
);

‎clippy_utils/src/hir_utils.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -424,7 +424,7 @@ fn reduce_exprkind<'hir>(cx: &LateContext<'_>, kind: &'hir ExprKind<'hir>) -> &'
424424
TokenKind::LineComment { .. } | TokenKind::BlockComment { .. } | TokenKind::Whitespace
425425
)
426426
})
427-
.ne([TokenKind::OpenBrace, TokenKind::CloseBrace].iter().cloned()) =>
427+
.ne([TokenKind::OpenBrace, TokenKind::CloseBrace].iter().copied()) =>
428428
{
429429
kind
430430
},

‎clippy_utils/src/lib.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1053,7 +1053,7 @@ pub fn is_refutable(cx: &LateContext<'_>, pat: &Pat<'_>) -> bool {
10531053
/// the function once on the given pattern.
10541054
pub fn recurse_or_patterns<'tcx, F: FnMut(&'tcx Pat<'tcx>)>(pat: &'tcx Pat<'tcx>, mut f: F) {
10551055
if let PatKind::Or(pats) = pat.kind {
1056-
pats.iter().cloned().for_each(f)
1056+
pats.iter().copied().for_each(f)
10571057
} else {
10581058
f(pat)
10591059
}
@@ -1230,14 +1230,14 @@ pub fn match_any_def_paths(cx: &LateContext<'_>, did: DefId, paths: &[&[&str]])
12301230
let search_path = cx.get_def_path(did);
12311231
paths
12321232
.iter()
1233-
.position(|p| p.iter().map(|x| Symbol::intern(x)).eq(search_path.iter().cloned()))
1233+
.position(|p| p.iter().map(|x| Symbol::intern(x)).eq(search_path.iter().copied()))
12341234
}
12351235

12361236
/// Checks if the given `DefId` matches the path.
12371237
pub fn match_def_path<'tcx>(cx: &LateContext<'tcx>, did: DefId, syms: &[&str]) -> bool {
12381238
// We should probably move to Symbols in Clippy as well rather than interning every time.
12391239
let path = cx.get_def_path(did);
1240-
syms.iter().map(|x| Symbol::intern(x)).eq(path.iter().cloned())
1240+
syms.iter().map(|x| Symbol::intern(x)).eq(path.iter().copied())
12411241
}
12421242

12431243
pub fn match_panic_call(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {

‎clippy_utils/src/ty.rs

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use rustc_lint::LateContext;
1313
use rustc_middle::ty::subst::{GenericArg, GenericArgKind};
1414
use rustc_middle::ty::{self, AdtDef, IntTy, Ty, TypeFoldable, UintTy};
1515
use rustc_span::sym;
16-
use rustc_span::symbol::Symbol;
16+
use rustc_span::symbol::{Ident, Symbol};
1717
use rustc_span::DUMMY_SP;
1818
use rustc_trait_selection::traits::query::normalize::AtExt;
1919

@@ -52,6 +52,25 @@ pub fn contains_adt_constructor(ty: Ty<'_>, adt: &AdtDef) -> bool {
5252
})
5353
}
5454

55+
/// Resolves `<T as Iterator>::Item` for `T`
56+
/// Do not invoke without first verifying that the type implements `Iterator`
57+
pub fn get_iterator_item_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<Ty<'tcx>> {
58+
cx.tcx
59+
.get_diagnostic_item(sym::Iterator)
60+
.and_then(|iter_did| {
61+
cx.tcx.associated_items(iter_did).find_by_name_and_kind(
62+
cx.tcx,
63+
Ident::from_str("Item"),
64+
ty::AssocKind::Type,
65+
iter_did,
66+
)
67+
})
68+
.map(|assoc| {
69+
let proj = cx.tcx.mk_projection(assoc.def_id, cx.tcx.mk_substs_trait(ty, &[]));
70+
cx.tcx.normalize_erasing_regions(cx.param_env, proj)
71+
})
72+
}
73+
5574
/// Returns true if ty has `iter` or `iter_mut` methods
5675
pub fn has_iter_method(cx: &LateContext<'_>, probably_ref_ty: Ty<'_>) -> Option<Symbol> {
5776
// FIXME: instead of this hard-coded list, we should check if `<adt>::iter`

‎tests/ui/cloned_instead_of_copied.fixed

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// run-rustfix
2+
#![warn(clippy::cloned_instead_of_copied)]
3+
4+
fn main() {
5+
// yay
6+
let _ = [1].iter().copied();
7+
let _ = vec!["hi"].iter().copied();
8+
let _ = Some(&1).copied();
9+
let _ = Box::new([1].iter()).copied();
10+
let _ = Box::new(Some(&1)).copied();
11+
12+
// nay
13+
let _ = [String::new()].iter().cloned();
14+
let _ = Some(&String::new()).cloned();
15+
}

‎tests/ui/cloned_instead_of_copied.rs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// run-rustfix
2+
#![warn(clippy::cloned_instead_of_copied)]
3+
4+
fn main() {
5+
// yay
6+
let _ = [1].iter().cloned();
7+
let _ = vec!["hi"].iter().cloned();
8+
let _ = Some(&1).cloned();
9+
let _ = Box::new([1].iter()).cloned();
10+
let _ = Box::new(Some(&1)).cloned();
11+
12+
// nay
13+
let _ = [String::new()].iter().cloned();
14+
let _ = Some(&String::new()).cloned();
15+
}

‎tests/ui/cloned_instead_of_copied.stderr

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
error: used `cloned` where `copied` could be used instead
2+
--> $DIR/cloned_instead_of_copied.rs:6:24
3+
|
4+
LL | let _ = [1].iter().cloned();
5+
| ^^^^^^ help: try: `copied`
6+
|
7+
= note: `-D clippy::cloned-instead-of-copied` implied by `-D warnings`
8+
9+
error: used `cloned` where `copied` could be used instead
10+
--> $DIR/cloned_instead_of_copied.rs:7:31
11+
|
12+
LL | let _ = vec!["hi"].iter().cloned();
13+
| ^^^^^^ help: try: `copied`
14+
15+
error: used `cloned` where `copied` could be used instead
16+
--> $DIR/cloned_instead_of_copied.rs:8:22
17+
|
18+
LL | let _ = Some(&1).cloned();
19+
| ^^^^^^ help: try: `copied`
20+
21+
error: used `cloned` where `copied` could be used instead
22+
--> $DIR/cloned_instead_of_copied.rs:9:34
23+
|
24+
LL | let _ = Box::new([1].iter()).cloned();
25+
| ^^^^^^ help: try: `copied`
26+
27+
error: used `cloned` where `copied` could be used instead
28+
--> $DIR/cloned_instead_of_copied.rs:10:32
29+
|
30+
LL | let _ = Box::new(Some(&1)).cloned();
31+
| ^^^^^^ help: try: `copied`
32+
33+
error: aborting due to 5 previous errors
34+

0 commit comments

Comments
 (0)