From b36bb0a68d70f091dd03f209f41b614ff1f015d0 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 2 Oct 2018 15:13:43 +0200 Subject: [PATCH] Reimplement the `map_clone` lint from scratch --- CHANGELOG.md | 2 - README.md | 2 +- clippy_lints/src/lib.rs | 4 ++ clippy_lints/src/map_clone.rs | 100 ++++++++++++++++++++++++++++++++++ tests/ui/map_clone.rs | 9 +++ tests/ui/map_clone.stderr | 22 ++++++++ 6 files changed, 136 insertions(+), 3 deletions(-) create mode 100644 clippy_lints/src/map_clone.rs create mode 100644 tests/ui/map_clone.rs create mode 100644 tests/ui/map_clone.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index c9bea1e8ef5..013a508cc76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -688,8 +688,6 @@ All notable changes to this project will be documented in this file. [`float_arithmetic`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#float_arithmetic [`float_cmp`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#float_cmp [`float_cmp_const`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#float_cmp_const -[`fn_to_numeric_cast`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#fn_to_numeric_cast -[`fn_to_numeric_cast_with_truncation`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#fn_to_numeric_cast_with_truncation [`for_kv_map`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#for_kv_map [`for_loop_over_option`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#for_loop_over_option [`for_loop_over_result`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#for_loop_over_result diff --git a/README.md b/README.md index 40ece34c6fa..a2b61703a82 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ We are currently in the process of discussing Clippy 1.0 via the RFC process in A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 279 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html) +[There are 277 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 689dbfa7da9..8b1b626be44 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -129,6 +129,7 @@ pub mod let_if_seq; pub mod lifetimes; pub mod literal_representation; pub mod loops; +pub mod map_clone; pub mod map_unit_fn; pub mod matches; pub mod mem_forget; @@ -327,6 +328,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_late_lint_pass(box strings::StringAdd); reg.register_early_lint_pass(box returns::ReturnPass); reg.register_late_lint_pass(box methods::Pass); + reg.register_late_lint_pass(box map_clone::Pass); reg.register_late_lint_pass(box shadow::Pass); reg.register_late_lint_pass(box types::LetPass); reg.register_late_lint_pass(box types::UnitCmp); @@ -583,6 +585,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { loops::WHILE_IMMUTABLE_CONDITION, loops::WHILE_LET_LOOP, loops::WHILE_LET_ON_ITERATOR, + map_clone::MAP_CLONE, map_unit_fn::OPTION_MAP_UNIT_FN, map_unit_fn::RESULT_MAP_UNIT_FN, matches::MATCH_AS_REF, @@ -742,6 +745,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { loops::FOR_KV_MAP, loops::NEEDLESS_RANGE_LOOP, loops::WHILE_LET_ON_ITERATOR, + map_clone::MAP_CLONE, matches::MATCH_BOOL, matches::MATCH_OVERLAPPING_ARM, matches::MATCH_REF_PATS, diff --git a/clippy_lints/src/map_clone.rs b/clippy_lints/src/map_clone.rs new file mode 100644 index 00000000000..ddbb55a26ff --- /dev/null +++ b/clippy_lints/src/map_clone.rs @@ -0,0 +1,100 @@ +use crate::rustc::hir; +use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; +use crate::rustc::{declare_tool_lint, lint_array}; +use crate::syntax::source_map::Span; +use crate::utils::paths; +use crate::utils::{ + in_macro, match_trait_method, match_type, + remove_blocks, snippet, + span_lint_and_sugg, +}; +use if_chain::if_chain; +use crate::syntax::ast::Ident; + +#[derive(Clone)] +pub struct Pass; + +/// **What it does:** Checks for usage of `iterator.map(|x| x.clone())` and suggests +/// `iterator.cloned()` instead +/// +/// **Why is this bad?** Readability, this can be written more concisely +/// +/// **Known problems:** None. +/// +/// **Example:** +/// +/// ```rust +/// let x = vec![42, 43]; +/// let y = x.iter(); +/// let z = y.map(|i| *i); +/// ``` +/// +/// The correct use would be: +/// +/// ```rust +/// let x = vec![42, 43]; +/// let y = x.iter(); +/// let z = y.cloned(); +/// ``` +declare_clippy_lint! { + pub MAP_CLONE, + style, + "using `iterator.map(|x| x.clone())`, or dereferencing closures for `Copy` types" +} + +impl LintPass for Pass { + fn get_lints(&self) -> LintArray { + lint_array!(MAP_CLONE) + } +} + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { + fn check_expr(&mut self, cx: &LateContext<'_, '_>, e: &hir::Expr) { + if in_macro(e.span) { + return; + } + + if_chain! { + if let hir::ExprKind::MethodCall(ref method, _, ref args) = e.node; + 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); + 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); + then { + match closure_body.arguments[0].pat.node { + hir::PatKind::Ref(ref inner, _) => if let hir::PatKind::Binding(hir::BindingAnnotation::Unannotated, _, name, None) = inner.node { + lint(cx, e.span, args[0].span, name, closure_expr); + }, + hir::PatKind::Binding(hir::BindingAnnotation::Unannotated, _, name, None) => match closure_expr.node { + hir::ExprKind::Unary(hir::UnOp::UnDeref, ref inner) => lint(cx, e.span, args[0].span, name, inner), + hir::ExprKind::MethodCall(ref method, _, ref obj) => if method.ident.as_str() == "clone" { + if match_trait_method(cx, closure_expr, &paths::CLONE_TRAIT) { + lint(cx, e.span, args[0].span, name, &obj[0]); + } + } + _ => {}, + }, + _ => {}, + } + } + } + } +} + +fn lint(cx: &LateContext<'_, '_>, replace: Span, root: Span, name: Ident, path: &hir::Expr) { + if let hir::ExprKind::Path(hir::QPath::Resolved(None, ref path)) = path.node { + if path.segments.len() == 1 && path.segments[0].ident == name { + 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(cx, root, "..")), + ) + } + } +} \ No newline at end of file diff --git a/tests/ui/map_clone.rs b/tests/ui/map_clone.rs new file mode 100644 index 00000000000..11a5316a367 --- /dev/null +++ b/tests/ui/map_clone.rs @@ -0,0 +1,9 @@ +#![feature(tool_lints)] +#![warn(clippy::all, clippy::pedantic)] +#![allow(clippy::missing_docs_in_private_items)] + +fn main() { + let _: Vec = vec![5_i8; 6].iter().map(|x| *x).collect(); + let _: Vec = vec![String::new()].iter().map(|x| x.clone()).collect(); + let _: Vec = vec![42, 43].iter().map(|&x| x).collect(); +} diff --git a/tests/ui/map_clone.stderr b/tests/ui/map_clone.stderr new file mode 100644 index 00000000000..e80983cdbf7 --- /dev/null +++ b/tests/ui/map_clone.stderr @@ -0,0 +1,22 @@ +error: You are using an explicit closure for cloning elements + --> $DIR/map_clone.rs:6:22 + | +6 | let _: Vec = vec![5_i8; 6].iter().map(|x| *x).collect(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Consider calling the dedicated `cloned` method: `vec![5_i8; 6].iter().cloned()` + | + = note: `-D clippy::map-clone` implied by `-D warnings` + +error: You are using an explicit closure for cloning elements + --> $DIR/map_clone.rs:7:26 + | +7 | let _: Vec = vec![String::new()].iter().map(|x| x.clone()).collect(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Consider calling the dedicated `cloned` method: `vec![String::new()].iter().cloned()` + +error: You are using an explicit closure for cloning elements + --> $DIR/map_clone.rs:8:23 + | +8 | let _: Vec = vec![42, 43].iter().map(|&x| x).collect(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Consider calling the dedicated `cloned` method: `vec![42, 43].iter().cloned()` + +error: aborting due to 3 previous errors +