From 414c0d20f746c2e3852c8a5356b8831176c915f6 Mon Sep 17 00:00:00 2001 From: wartman4404 Date: Fri, 30 Oct 2015 23:58:37 -0500 Subject: [PATCH 1/2] 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 0da037e7b16..fcfeca1509e 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 525603dc2b8..37e1ace61de 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 00000000000..570ee91dd7b --- /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 00000000000..9d9f253defe --- /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() { } From 764eedd0508a53b5184741bd05b8d20ea1034c42 Mon Sep 17 00:00:00 2001 From: wartman4404 Date: Tue, 3 Nov 2015 21:11:40 -0600 Subject: [PATCH 2/2] check for Deref conversions --- README.md | 2 +- src/eta_reduction.rs | 6 +----- src/map_clone.rs | 17 ++++++++--------- src/utils.rs | 4 ++++ tests/compile-fail/map_clone.rs | 23 +++++++++++++++++++++++ 5 files changed, 37 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index fcfeca1509e..4c23d6994d2 100644 --- a/README.md +++ b/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 -------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ diff --git a/src/eta_reduction.rs b/src/eta_reduction.rs index 7226a4bad05..855ea51ee8e 100644 --- a/src/eta_reduction.rs +++ b/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() { diff --git a/src/map_clone.rs b/src/map_clone.rs index 570ee91dd7b..e93a8221145 100644 --- a/src/map_clone.rs +++ b/src/map_clone.rs @@ -2,7 +2,7 @@ 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::{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, @@ -30,7 +30,7 @@ impl LateLintPass for MapClonePass { 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) && + 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 { @@ -85,13 +85,12 @@ fn get_arg_name(pat: &Pat) -> Option { } } -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 +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), } } diff --git a/src/utils.rs b/src/utils.rs index 7cbb532cf22..757d7bc379d 100644 --- a/src/utils.rs +++ b/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! { diff --git a/tests/compile-fail/map_clone.rs b/tests/compile-fail/map_clone.rs index 9d9f253defe..f6241114a83 100644 --- a/tests/compile-fail/map_clone.rs +++ b/tests/compile-fail/map_clone.rs @@ -5,6 +5,8 @@ #![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() @@ -66,4 +68,25 @@ fn map_clone_other() { 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 = x.as_ref().map(|y| *y); //~ ERROR you seem to be using .map() + //~^ HELP try + + // Not linted: using deref conversion + let _: Option = x.map(|y| *y); + + // Not linted: using regular deref but also deref conversion + let _: Option = x.as_ref().map(|y| **y); +} + fn main() { }