From 414c0d20f746c2e3852c8a5356b8831176c915f6 Mon Sep 17 00:00:00 2001 From: wartman4404 Date: Fri, 30 Oct 2015 23:58:37 -0500 Subject: [PATCH] New lint for using `.cloned()` --- README.md | 1 + src/lib.rs | 3 + src/map_clone.rs | 102 ++++++++++++++++++++++++++++++++ tests/compile-fail/map_clone.rs | 69 +++++++++++++++++++++ 4 files changed, 175 insertions(+) create mode 100644 src/map_clone.rs create mode 100644 tests/compile-fail/map_clone.rs diff --git a/README.md b/README.md index 0da037e7b16d..fcfeca1509eb 100644 --- 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 525603dc2b80..37e1ace61ded 100755 --- a/src/lib.rs +++ b/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, diff --git a/src/map_clone.rs b/src/map_clone.rs new file mode 100644 index 000000000000..570ee91dd7bd --- /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 { + 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 index 000000000000..9d9f253defe8 --- /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); +impl Wrapper { + fn map U>(self, f: F) -> Wrapper { + 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() { }