Suggest .copied() instead of .cloned() in map_clone where applicable

clippy_lints/src/map_clone.rs

@@ -1,6 +1,6 @@
 use crate::utils::paths;
 use crate::utils::{
- in_macro, match_trait_method, match_type, remove_blocks, snippet_with_applicability, span_lint_and_sugg,
+ in_macro, is_copy, match_trait_method, match_type, remove_blocks, snippet_with_applicability, span_lint_and_sugg,
 };
 use if_chain::if_chain;
 use rustc::hir;
@@ -63,7 +63,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
  if args.len() == 2;
  if method.ident.as_str() == "map";
  let ty = cx.tables.expr_ty(&args[0]);
- if match_type(cx, ty, &paths::OPTION) || match_trait_method(cx, e, &paths::ITERATOR);
+ let is_option = match_type(cx, ty, &paths::OPTION);
+ if is_option || match_trait_method(cx, e, &paths::ITERATOR);
  if let hir::ExprKind::Closure(_, _, body_id, _, _) = args[1].node;
  let closure_body = cx.tcx.hir().body(body_id);
  let closure_expr = remove_blocks(&closure_body.value);
@@ -73,23 +74,26 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
  hir::BindingAnnotation::Unannotated, .., name, None
  ) = inner.node {
  if ident_eq(name, closure_expr) {
- lint(cx, e.span, args[0].span);
+ // FIXME When Iterator::copied() stabilizes we can remove is_option
+ // from here and the other lint() calls
+ lint(cx, e.span, args[0].span, is_option);
  }
  },
  hir::PatKind::Binding(hir::BindingAnnotation::Unannotated, .., name, None) => {
  match closure_expr.node {
  hir::ExprKind::Unary(hir::UnOp::UnDeref, ref inner) => {
  if ident_eq(name, inner) && !cx.tables.expr_ty(inner).is_box() {
- lint(cx, e.span, args[0].span);
+ lint(cx, e.span, args[0].span, is_option);
  }
  },
  hir::ExprKind::MethodCall(ref method, _, ref obj) => {
  if ident_eq(name, &obj[0]) && method.ident.as_str() == "clone"
  && match_trait_method(cx, closure_expr, &paths::CLONE_TRAIT) {
 
  let obj_ty = cx.tables.expr_ty(&obj[0]);
- if let ty::Ref(..) = obj_ty.sty {
- lint(cx, e.span, args[0].span);
+ if let ty::Ref(_, ty, _) = obj_ty.sty {
+ let copy = is_copy(cx, ty);
+ lint(cx, e.span, args[0].span, is_option && copy);
  } else {
  lint_needless_cloning(cx, e.span, args[0].span);
  }
@@ -125,18 +129,33 @@ fn lint_needless_cloning(cx: &LateContext<'_, '_>, root: Span, receiver: Span) {
  )
 }
 
-fn lint(cx: &LateContext<'_, '_>, replace: Span, root: Span) {
+fn lint(cx: &LateContext<'_, '_>, replace: Span, root: Span, copied: bool) {
  let mut applicability = Applicability::MachineApplicable;
- span_lint_and_sugg(
- cx,
- MAP_CLONE,
- replace,
- "You are using an explicit closure for cloning elements",
- "Consider calling the dedicated `cloned` method",
- format!(
- "{}.cloned()",
- snippet_with_applicability(cx, root, "..", &mut applicability)
- ),
- applicability,
- )
+ if copied {
+ span_lint_and_sugg(
+ cx,
+ MAP_CLONE,
+ replace,
+ "You are using an explicit closure for copying elements",
+ "Consider calling the dedicated `copied` method",
+ format!(
+ "{}.copied()",
+ snippet_with_applicability(cx, root, "..", &mut applicability)
+ ),
+ applicability,
+ )
+ } else {
+ span_lint_and_sugg(
+ cx,
+ MAP_CLONE,
+ replace,
+ "You are using an explicit closure for cloning elements",
+ "Consider calling the dedicated `cloned` method",
+ format!(
+ "{}.cloned()",
+ snippet_with_applicability(cx, root, "..", &mut applicability)
+ ),
+ applicability,
+ )
+ }
 }

tests/ui/map_clone.fixed

@@ -10,6 +10,8 @@ fn main() {
  let _: Vec<String> = vec![String::new()].iter().cloned().collect();
  let _: Vec<u32> = vec![42, 43].iter().cloned().collect();
  let _: Option<u64> = Some(Box::new(16)).map(|b| *b);
+ let _: Option<u64> = Some(&16).copied();
+ let _: Option<u8> = Some(&1).copied();
 
  // Don't lint these
  let v = vec![5_i8; 6];

tests/ui/map_clone.rs

@@ -10,6 +10,8 @@ fn main() {
  let _: Vec<String> = vec![String::new()].iter().map(|x| x.clone()).collect();
  let _: Vec<u32> = vec![42, 43].iter().map(|&x| x).collect();
  let _: Option<u64> = Some(Box::new(16)).map(|b| *b);
+ let _: Option<u64> = Some(&16).map(|b| *b);
+ let _: Option<u8> = Some(&1).map(|x| x.clone());
 
  // Don't lint these
  let v = vec![5_i8; 6];

tests/ui/map_clone.stderr

@@ -18,11 +18,23 @@ error: You are using an explicit closure for cloning elements
 LL | let _: Vec<u32> = vec![42, 43].iter().map(|&x| x).collect();
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Consider calling the dedicated `cloned` method: `vec![42, 43].iter().cloned()`
 
+error: You are using an explicit closure for copying elements
+ --> $DIR/map_clone.rs:13:26
+ |
+LL | let _: Option<u64> = Some(&16).map(|b| *b);
+ | ^^^^^^^^^^^^^^^^^^^^^ help: Consider calling the dedicated `copied` method: `Some(&16).copied()`
+
+error: You are using an explicit closure for copying elements
+ --> $DIR/map_clone.rs:14:25
+ |
+LL | let _: Option<u8> = Some(&1).map(|x| x.clone());
+ | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Consider calling the dedicated `copied` method: `Some(&1).copied()`
+
 error: You are needlessly cloning iterator elements
- --> $DIR/map_clone.rs:23:29
+ --> $DIR/map_clone.rs:25:29
  |
 LL | let _ = std::env::args().map(|v| v.clone());
  | ^^^^^^^^^^^^^^^^^^^ help: Remove the map call
 
-error: aborting due to 4 previous errors
+error: aborting due to 6 previous errors