Merge pull request #427 from wartman4404/master

Prefer `.cloned()` over `.map(|x| x.clone())`

Author: llogiq

README.md

@@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
 [Jump to usage instructions](#usage)
 
 ##Lints
-There are 71 lints included in this crate:
+There are 72 lints included in this crate:
 
 name                                                                                                   | default | meaning
 -------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -34,6 +34,7 @@ name
 [let_and_return](https://github.com/Manishearth/rust-clippy/wiki#let_and_return)                       | warn    | creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a block
 [let_unit_value](https://github.com/Manishearth/rust-clippy/wiki#let_unit_value)                       | warn    | creating a let binding to a value of unit type, which usually can't be used afterwards
 [linkedlist](https://github.com/Manishearth/rust-clippy/wiki#linkedlist)                               | warn    | usage of LinkedList, usually a vector is faster, or a more specialized data structure like a VecDeque
+[map_clone](https://github.com/Manishearth/rust-clippy/wiki#map_clone)                                 | warn    | using `.map(|x| x.clone())` to clone an iterator or option's contents (recommends `.cloned()` instead)
 [match_bool](https://github.com/Manishearth/rust-clippy/wiki#match_bool)                               | warn    | a match on boolean expression; recommends `if..else` block instead
 [match_ref_pats](https://github.com/Manishearth/rust-clippy/wiki#match_ref_pats)                       | warn    | a match has all arms prefixed with `&`; the match expression can be dereferenced instead
 [min_max](https://github.com/Manishearth/rust-clippy/wiki#min_max)                                     | warn    | `min(_, max(_, _))` (or vice versa) with bounds clamping the result to a constant

src/eta_reduction.rs

@@ -2,7 +2,7 @@ use rustc::lint::*;
 use rustc_front::hir::*;
 use rustc::middle::ty;
 
-use utils::{snippet, span_lint};
+use utils::{snippet, span_lint, is_adjusted};
 
 
 #[allow(missing_copy_implementations)]
@@ -32,10 +32,6 @@ impl LateLintPass for EtaPass {
     }
 }
 
-fn is_adjusted(cx: &LateContext, e: &Expr) -> bool {
-    cx.tcx.tables.borrow().adjustments.get(&e.id).is_some()
-}
-
 fn check_closure(cx: &LateContext, expr: &Expr) {
     if let ExprClosure(_, ref decl, ref blk) = expr.node {
         if !blk.stmts.is_empty() {

src/lib.rs

@@ -45,6 +45,7 @@ pub mod returns;
 pub mod lifetimes;
 pub mod loops;
 pub mod ranges;
+pub mod map_clone;
 pub mod matches;
 pub mod precedence;
 pub mod mutex_atomic;
@@ -100,6 +101,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
     reg.register_late_lint_pass(box needless_features::NeedlessFeaturesPass);
     reg.register_late_lint_pass(box needless_update::NeedlessUpdatePass);
     reg.register_late_lint_pass(box no_effect::NoEffectPass);
+    reg.register_late_lint_pass(box map_clone::MapClonePass);
 
     reg.register_lint_group("clippy_pedantic", vec![
         methods::OPTION_UNWRAP_USED,
@@ -141,6 +143,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         loops::UNUSED_COLLECT,
         loops::WHILE_LET_LOOP,
         loops::WHILE_LET_ON_ITERATOR,
+        map_clone::MAP_CLONE,
         matches::MATCH_BOOL,
         matches::MATCH_REF_PATS,
         matches::SINGLE_MATCH,

src/map_clone.rs

@@ -0,0 +1,101 @@
+use rustc::lint::*;
+use rustc_front::hir::*;
+use syntax::ast::Ident;
+use utils::OPTION_PATH;
+use utils::{is_adjusted, match_trait_method, match_type, snippet, span_help_and_lint};
+use utils::{walk_ptrs_ty, walk_ptrs_ty_depth};
+
+declare_lint!(pub MAP_CLONE, Warn,
+              "using `.map(|x| x.clone())` to clone an iterator or option's contents (recommends \
+              `.cloned()` instead)");
+
+#[derive(Copy, Clone)]
+pub struct MapClonePass;
+
+impl LateLintPass for MapClonePass {
+    fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
+        if_let_chain! {
+            [
+                // call to .map()
+                let ExprMethodCall(name, _, ref args) = expr.node,
+                name.node.as_str() == "map" && args.len() == 2,
+                let ExprClosure(_, ref decl, ref blk) = args[1].node,
+                // just one expression in the closure
+                blk.stmts.is_empty(),
+                let Some(ref closure_expr) = blk.expr,
+                // nothing special in the argument, besides reference bindings
+                // (e.g. .map(|&x| x) )
+                let Some(arg_ident) = get_arg_name(&*decl.inputs[0].pat),
+                // the method is being called on a known type (option or iterator)
+                let Some(type_name) = get_type_name(cx, expr, &args[0])
+            ], {
+                // look for derefs, for .map(|x| *x)
+                if only_derefs(cx, &*closure_expr, arg_ident) &&
+                    // .cloned() only removes one level of indirection, don't lint on more
+                    walk_ptrs_ty_depth(cx.tcx.pat_ty(&*decl.inputs[0].pat)).1 == 1
+                {
+                    span_help_and_lint(cx, MAP_CLONE, expr.span, &format!(
+                        "you seem to be using .map() to clone the contents of an {}, consider \
+                        using `.cloned()`", type_name),
+                        &format!("try\n{}.cloned()", snippet(cx, args[0].span, "..")));
+                }
+                // explicit clone() calls ( .map(|x| x.clone()) )
+                else if let ExprMethodCall(clone_call, _, ref clone_args) = closure_expr.node {
+                    if clone_call.node.as_str() == "clone" &&
+                        clone_args.len() == 1 &&
+                        match_trait_method(cx, closure_expr, &["core", "clone", "Clone"]) &&
+                        expr_eq_ident(&clone_args[0], arg_ident)
+                    {
+                        span_help_and_lint(cx, MAP_CLONE, expr.span, &format!(
+                            "you seem to be using .map() to clone the contents of an {}, consider \
+                            using `.cloned()`", type_name),
+                            &format!("try\n{}.cloned()", snippet(cx, args[0].span, "..")));
+                    }
+                }
+            }
+        }
+    }
+}
+
+fn expr_eq_ident(expr: &Expr, id: Ident) -> bool {
+    match expr.node {
+        ExprPath(None, ref path) => {
+            let arg_segment = [PathSegment { identifier: id, parameters: PathParameters::none() }];
+            !path.global && path.segments == arg_segment
+        },
+        _ => false,
+    }
+}
+
+fn get_type_name(cx: &LateContext, expr: &Expr, arg: &Expr) -> Option<&'static str> {
+    if match_trait_method(cx, expr, &["core", "iter", "Iterator"]) {
+        Some("iterator")
+    } else if match_type(cx, walk_ptrs_ty(cx.tcx.expr_ty(arg)), &OPTION_PATH) {
+        Some("Option")
+    } else {
+        None
+    }
+}
+
+fn get_arg_name(pat: &Pat) -> Option<Ident> {
+    match pat.node {
+        PatIdent(_, ident, None) => Some(ident.node),
+        PatRegion(ref subpat, _) => get_arg_name(subpat),
+        _ => None,
+    }
+}
+
+fn only_derefs(cx: &LateContext, expr: &Expr, id: Ident) -> bool {
+    match expr.node {
+        ExprUnary(UnDeref, ref subexpr) if !is_adjusted(cx, subexpr) => {
+            only_derefs(cx, subexpr, id)
+        },
+        _ => expr_eq_ident(expr, id),
+    }
+}
+
+impl LintPass for MapClonePass {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(MAP_CLONE)
+    }
+}

src/utils.rs

@@ -347,6 +347,10 @@ pub fn is_integer_literal(expr: &Expr, value: u64) -> bool
     false
 }
 
+pub fn is_adjusted(cx: &LateContext, e: &Expr) -> bool {
+    cx.tcx.tables.borrow().adjustments.get(&e.id).is_some()
+}
+
 /// Produce a nested chain of if-lets and ifs from the patterns:
 ///
 ///     if_let_chain! {

tests/compile-fail/map_clone.rs

@@ -0,0 +1,92 @@
+#![feature(plugin)]
+
+#![plugin(clippy)]
+#![deny(map_clone)]
+
+#![allow(unused)]
+
+use std::ops::Deref;
+
+fn map_clone_iter() {
+    let x = [1,2,3];
+    x.iter().map(|y| y.clone()); //~ ERROR you seem to be using .map()
+                                 //~^ HELP try
+    x.iter().map(|&y| y); //~ ERROR you seem to be using .map()
+                          //~^ HELP try
+    x.iter().map(|y| *y); //~ ERROR you seem to be using .map()
+                          //~^ HELP try
+}
+
+fn map_clone_option() {
+    let x = Some(4);
+    x.as_ref().map(|y| y.clone()); //~ ERROR you seem to be using .map()
+                                   //~^ HELP try
+    x.as_ref().map(|&y| y); //~ ERROR you seem to be using .map()
+                            //~^ HELP try
+    x.as_ref().map(|y| *y); //~ ERROR you seem to be using .map()
+                            //~^ HELP try
+}
+
+fn not_linted_option() {
+    let x = Some(5);
+
+    // Not linted: other statements
+    x.as_ref().map(|y| {
+        println!("y: {}", y);
+        y.clone()
+    });
+
+    // Not linted: argument bindings
+    let x = Some((6, 7));
+    x.map(|(y, _)| y.clone());
+
+    // Not linted: cloning something else
+    x.map(|y| y.0.clone());
+
+    // Not linted: no dereferences
+    x.map(|y| y);
+
+    // Not linted: multiple dereferences
+    let _: Option<(i32, i32)> = x.as_ref().as_ref().map(|&&x| x);
+}
+
+#[derive(Copy, Clone)]
+struct Wrapper<T>(T);
+impl<T> Wrapper<T> {
+    fn map<U, F: FnOnce(T) -> U>(self, f: F) -> Wrapper<U> {
+        Wrapper(f(self.0))
+    }
+}
+
+fn map_clone_other() {
+    let eight = 8;
+    let x = Wrapper(&eight);
+
+    // Not linted: not a linted type
+    x.map(|y| y.clone());
+    x.map(|&y| y);
+    x.map(|y| *y);
+}
+
+#[derive(Copy, Clone)]
+struct UnusualDeref;
+static NINE: i32 = 9;
+
+impl Deref for UnusualDeref {
+    type Target = i32;
+    fn deref(&self) -> &i32 { &NINE }
+}
+
+fn map_clone_deref() {
+    let x = Some(UnusualDeref);
+    let _: Option<UnusualDeref> = x.as_ref().map(|y| *y); //~ ERROR you seem to be using .map()
+                                                          //~^ HELP try
+
+    // Not linted: using deref conversion
+    let _: Option<i32> = x.map(|y| *y);
+
+    // Not linted: using regular deref but also deref conversion
+    let _: Option<i32> = x.as_ref().map(|y| **y);
+}
+
+fn main() { }