New lint for using `.cloned()`

diff --git a/README.md b/README.md
index 0da037e7b16d3d651a1f0637ca6b8557caf4a24d..fcfeca1509eb3adefa4f5a392fbb54212a721665 100644 (file)
--- a/README.md
+++ b/README.md
@@ -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

diff --git a/src/lib.rs b/src/lib.rs
index 525603dc2b805f1b6339f789ce1b585fd4ebee09..37e1ace61ded590091d440c70023a058c08f28a4 100755 (executable)
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -45,6 +45,7 @@
 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,

diff --git a/src/map_clone.rs b/src/map_clone.rs
new file mode 100644 (file)
index 0000000..570ee91
--- /dev/null
+++ b/src/map_clone.rs
@@ -0,0 +1,102 @@
+use rustc::lint::*;
+use rustc_front::hir::*;
+use syntax::ast::Ident;
+use utils::OPTION_PATH;
+use utils::{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(&*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(expr: &Expr, id: Ident) -> bool {
+    if expr_eq_ident(expr, id) {
+        true
+    } else if let ExprUnary(UnDeref, ref subexpr) = expr.node {
+        only_derefs(subexpr, id)
+    } else {
+        false
+    }
+}
+
+impl LintPass for MapClonePass {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(MAP_CLONE)
+    }
+}

diff --git a/tests/compile-fail/map_clone.rs b/tests/compile-fail/map_clone.rs
new file mode 100644 (file)
index 0000000..9d9f253
--- /dev/null
+++ b/tests/compile-fail/map_clone.rs
@@ -0,0 +1,69 @@
+#![feature(plugin)]
+
+#![plugin(clippy)]
+#![deny(map_clone)]
+
+#![allow(unused)]
+
+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);
+}
+
+fn main() { }