X-Git-Url: https://git.lizzy.rs/?a=blobdiff_plain;f=clippy_lints%2Fsrc%2Fmap_clone.rs;h=4743dec9d5c28d2677ec4df54bb63159b8ca0e7d;hb=6feed2713c7740eb5868eba43745bc508b8b77aa;hp=2a7177d1fb9a4a0f5a477edb3baeadca9ee0279c;hpb=6cba3da727f8e084c5124c77901b7ffba2264076;p=rust.git diff --git a/clippy_lints/src/map_clone.rs b/clippy_lints/src/map_clone.rs index 2a7177d1fb9..4743dec9d5c 100644 --- a/clippy_lints/src/map_clone.rs +++ b/clippy_lints/src/map_clone.rs @@ -1,64 +1,47 @@ -// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT -// file at the top-level directory of this distribution. -// -// Licensed under the Apache License, Version 2.0 or the MIT license -// , at your -// option. This file may not be copied, modified, or distributed -// except according to those terms. - use crate::utils::paths; use crate::utils::{ - in_macro, match_trait_method, match_type, remove_blocks, snippet_with_applicability, span_lint_and_sugg, + in_macro, is_copy, match_trait_method, match_type, remove_blocks, snippet_with_applicability, span_lint_and_sugg, }; use if_chain::if_chain; use rustc::hir; use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; -use rustc::{declare_tool_lint, lint_array}; +use rustc::ty; +use rustc::{declare_lint_pass, declare_tool_lint}; use rustc_errors::Applicability; use syntax::ast::Ident; use syntax::source_map::Span; -#[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:** Sometimes `.cloned()` requires stricter trait -/// bound than `.map(|e| e.clone())` (which works because of the coercion). -/// See [#498](https://github.com/rust-lang-nursery/rust-clippy/issues/498). -/// -/// **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! { + /// **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(); + /// ``` 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) - } -} +declare_lint_pass!(MapClone => [MAP_CLONE]); -impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MapClone { fn check_expr(&mut self, cx: &LateContext<'_, '_>, e: &hir::Expr) { if in_macro(e.span) { return; @@ -69,28 +52,40 @@ fn check_expr(&mut self, cx: &LateContext<'_, '_>, e: &hir::Expr) { 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); + let is_option = match_type(cx, ty, &paths::OPTION); + if is_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 + hir::BindingAnnotation::Unannotated, .., name, None ) = inner.node { - lint(cx, e.span, args[0].span, name, closure_expr); + if ident_eq(name, closure_expr) { + // FIXME When Iterator::copied() stabilizes we can remove is_option + // from here and the other lint() calls + lint(cx, e.span, args[0].span, is_option); + } }, - hir::PatKind::Binding(hir::BindingAnnotation::Unannotated, _, name, None) => { + hir::PatKind::Binding(hir::BindingAnnotation::Unannotated, .., name, None) => { match closure_expr.node { hir::ExprKind::Unary(hir::UnOp::UnDeref, ref inner) => { - if !cx.tables.expr_ty(inner).is_box() { - lint(cx, e.span, args[0].span, name, inner); + if ident_eq(name, inner) && !cx.tables.expr_ty(inner).is_box() { + lint(cx, e.span, args[0].span, is_option); } }, hir::ExprKind::MethodCall(ref method, _, ref obj) => { - if method.ident.as_str() == "clone" + if ident_eq(name, &obj[0]) && method.ident.as_str() == "clone" && match_trait_method(cx, closure_expr, &paths::CLONE_TRAIT) { - lint(cx, e.span, args[0].span, name, &obj[0]); + + let obj_ty = cx.tables.expr_ty(&obj[0]); + if let ty::Ref(_, ty, _) = obj_ty.sty { + let copy = is_copy(cx, ty); + lint(cx, e.span, args[0].span, is_option && copy); + } else { + lint_needless_cloning(cx, e.span, args[0].span); + } } }, _ => {}, @@ -103,22 +98,53 @@ fn check_expr(&mut self, cx: &LateContext<'_, '_>, e: &hir::Expr) { } } -fn lint(cx: &LateContext<'_, '_>, replace: Span, root: Span, name: Ident, path: &hir::Expr) { +fn ident_eq(name: Ident, path: &hir::Expr) -> bool { if let hir::ExprKind::Path(hir::QPath::Resolved(None, ref path)) = path.node { - if path.segments.len() == 1 && path.segments[0].ident == name { - let mut applicability = Applicability::MachineApplicable; - 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_with_applicability(cx, root, "..", &mut applicability) - ), - applicability, - ) - } + path.segments.len() == 1 && path.segments[0].ident == name + } else { + false + } +} + +fn lint_needless_cloning(cx: &LateContext<'_, '_>, root: Span, receiver: Span) { + span_lint_and_sugg( + cx, + MAP_CLONE, + root.trim_start(receiver).unwrap(), + "You are needlessly cloning iterator elements", + "Remove the map call", + String::new(), + Applicability::MachineApplicable, + ) +} + +fn lint(cx: &LateContext<'_, '_>, replace: Span, root: Span, copied: bool) { + let mut applicability = Applicability::MachineApplicable; + if copied { + span_lint_and_sugg( + cx, + MAP_CLONE, + replace, + "You are using an explicit closure for copying elements", + "Consider calling the dedicated `copied` method", + format!( + "{}.copied()", + snippet_with_applicability(cx, root, "..", &mut applicability) + ), + applicability, + ) + } else { + 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_with_applicability(cx, root, "..", &mut applicability) + ), + applicability, + ) } }