From efdc739dfc1babd93540bd2d36c91bf2f7d53502 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Sat, 29 Sep 2018 14:12:40 +0200 Subject: [PATCH] Move unnecessary_filter_map to a submodule --- .../src/{methods.rs => methods/mod.rs} | 142 +---------------- .../src/methods/unnecessary_filter_map.rs | 146 ++++++++++++++++++ 2 files changed, 148 insertions(+), 140 deletions(-) rename clippy_lints/src/{methods.rs => methods/mod.rs} (93%) create mode 100644 clippy_lints/src/methods/unnecessary_filter_map.rs diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods/mod.rs similarity index 93% rename from clippy_lints/src/methods.rs rename to clippy_lints/src/methods/mod.rs index a0c9e34b8..828ef5b12 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1,6 +1,5 @@ use crate::rustc::hir; use crate::rustc::hir::def::Def; -use crate::rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; use crate::rustc::lint::{in_external_macro, LateContext, LateLintPass, Lint, LintArray, LintContext, LintPass}; use crate::rustc::ty::{self, Ty}; use crate::rustc::{declare_tool_lint, lint_array}; @@ -9,7 +8,6 @@ use crate::syntax::ast; use crate::syntax::source_map::{BytePos, Span}; use crate::utils::paths; use crate::utils::sugg; -use crate::utils::usage::mutated_variables; use crate::utils::{ get_arg_name, get_trait_def_id, implements_trait, in_macro, is_copy, is_expn_of, is_self, is_self_ty, iter_input_pats, last_path_segment, match_def_path, match_path, match_qpath, match_trait_method, match_type, @@ -22,6 +20,8 @@ use std::borrow::Cow; use std::fmt; use std::iter; +mod unnecessary_filter_map; + #[derive(Clone)] pub struct Pass; @@ -1424,144 +1424,6 @@ fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args: }; } -mod unnecessary_filter_map { - use super::*; - - pub(super) fn lint(cx: &LateContext<'_, '_>, expr: &hir::Expr, args: &[hir::Expr]) { - - if !match_trait_method(cx, expr, &paths::ITERATOR) { - return; - } - - if let hir::ExprKind::Closure(_, _, body_id, ..) = args[1].node { - - let body = cx.tcx.hir.body(body_id); - let arg_id = body.arguments[0].pat.id; - let mutates_arg = match mutated_variables(&body.value, cx) { - Some(used_mutably) => used_mutably.contains(&arg_id), - None => true, - }; - - let (mut found_mapping, mut found_filtering) = check_expression(&cx, arg_id, &body.value); - - let mut return_visitor = ReturnVisitor::new(&cx, arg_id); - return_visitor.visit_expr(&body.value); - found_mapping |= return_visitor.found_mapping; - found_filtering |= return_visitor.found_filtering; - - if !found_filtering { - span_lint( - cx, - UNNECESSARY_FILTER_MAP, - expr.span, - "this `.filter_map` can be written more simply using `.map`", - ); - return; - } - - if !found_mapping && !mutates_arg { - span_lint( - cx, - UNNECESSARY_FILTER_MAP, - expr.span, - "this `.filter_map` can be written more simply using `.filter`", - ); - return; - } - } - } - - // returns (found_mapping, found_filtering) - fn check_expression<'a, 'tcx: 'a>(cx: &'a LateContext<'a, 'tcx>, arg_id: ast::NodeId, expr: &'tcx hir::Expr) -> (bool, bool) { - match &expr.node { - hir::ExprKind::Call(ref func, ref args) => { - if_chain! { - if let hir::ExprKind::Path(ref path) = func.node; - then { - if match_qpath(path, &paths::OPTION_SOME) { - if_chain! { - if let hir::ExprKind::Path(path) = &args[0].node; - if let Def::Local(ref local) = cx.tables.qpath_def(path, args[0].hir_id); - then { - if arg_id == *local { - return (false, false) - } - } - } - return (true, false); - } else { - // We don't know. It might do anything. - return (true, true); - } - } - } - (true, true) - }, - hir::ExprKind::Block(ref block, _) => { - if let Some(expr) = &block.expr { - check_expression(cx, arg_id, &expr) - } else { - (false, false) - } - }, - // There must be an else_arm or there will be a type error - hir::ExprKind::If(_, ref if_arm, Some(ref else_arm)) => { - let if_check = check_expression(cx, arg_id, if_arm); - let else_check = check_expression(cx, arg_id, else_arm); - (if_check.0 | else_check.0, if_check.1 | else_check.1) - }, - hir::ExprKind::Match(_, ref arms, _) => { - let mut found_mapping = false; - let mut found_filtering = false; - for arm in arms { - let (m, f) = check_expression(cx, arg_id, &arm.body); - found_mapping |= m; - found_filtering |= f; - } - (found_mapping, found_filtering) - }, - hir::ExprKind::Path(path) if match_qpath(path, &paths::OPTION_NONE) => (false, true), - _ => (true, true) - } - } - - struct ReturnVisitor<'a, 'tcx: 'a> { - cx: &'a LateContext<'a, 'tcx>, - arg_id: ast::NodeId, - // Found a non-None return that isn't Some(input) - found_mapping: bool, - // Found a return that isn't Some - found_filtering: bool, - } - - impl<'a, 'tcx: 'a> ReturnVisitor<'a, 'tcx> { - fn new(cx: &'a LateContext<'a, 'tcx>, arg_id: ast::NodeId) -> ReturnVisitor<'a, 'tcx> { - ReturnVisitor { - cx, - arg_id, - found_mapping: false, - found_filtering: false, - } - } - } - - impl<'a, 'tcx> Visitor<'tcx> for ReturnVisitor<'a, 'tcx> { - fn visit_expr(&mut self, expr: &'tcx hir::Expr) { - if let hir::ExprKind::Ret(Some(expr)) = &expr.node { - let (found_mapping, found_filtering) = check_expression(self.cx, self.arg_id, expr); - self.found_mapping |= found_mapping; - self.found_filtering |= found_filtering; - } else { - walk_expr(self, expr); - } - } - - fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { - NestedVisitorMap::None - } - } -} - fn lint_iter_nth(cx: &LateContext<'_, '_>, expr: &hir::Expr, iter_args: &[hir::Expr], is_mut: bool) { let mut_str = if is_mut { "_mut" } else { "" }; let caller_type = if derefs_to_slice(cx, &iter_args[0], cx.tables.expr_ty(&iter_args[0])).is_some() { diff --git a/clippy_lints/src/methods/unnecessary_filter_map.rs b/clippy_lints/src/methods/unnecessary_filter_map.rs new file mode 100644 index 000000000..691c08ef0 --- /dev/null +++ b/clippy_lints/src/methods/unnecessary_filter_map.rs @@ -0,0 +1,146 @@ +use crate::rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; +use crate::rustc::lint::LateContext; +use crate::rustc::hir; +use crate::rustc::hir::def::Def; +use crate::syntax::ast; +use crate::utils::{match_qpath, match_trait_method, span_lint}; +use crate::utils::paths; +use crate::utils::usage::mutated_variables; + +use if_chain::if_chain; + +use super::UNNECESSARY_FILTER_MAP; + +pub(super) fn lint(cx: &LateContext<'_, '_>, expr: &hir::Expr, args: &[hir::Expr]) { + + if !match_trait_method(cx, expr, &paths::ITERATOR) { + return; + } + + if let hir::ExprKind::Closure(_, _, body_id, ..) = args[1].node { + + let body = cx.tcx.hir.body(body_id); + let arg_id = body.arguments[0].pat.id; + let mutates_arg = match mutated_variables(&body.value, cx) { + Some(used_mutably) => used_mutably.contains(&arg_id), + None => true, + }; + + let (mut found_mapping, mut found_filtering) = check_expression(&cx, arg_id, &body.value); + + let mut return_visitor = ReturnVisitor::new(&cx, arg_id); + return_visitor.visit_expr(&body.value); + found_mapping |= return_visitor.found_mapping; + found_filtering |= return_visitor.found_filtering; + + if !found_filtering { + span_lint( + cx, + UNNECESSARY_FILTER_MAP, + expr.span, + "this `.filter_map` can be written more simply using `.map`", + ); + return; + } + + if !found_mapping && !mutates_arg { + span_lint( + cx, + UNNECESSARY_FILTER_MAP, + expr.span, + "this `.filter_map` can be written more simply using `.filter`", + ); + return; + } + } +} + +// returns (found_mapping, found_filtering) +fn check_expression<'a, 'tcx: 'a>(cx: &'a LateContext<'a, 'tcx>, arg_id: ast::NodeId, expr: &'tcx hir::Expr) -> (bool, bool) { + match &expr.node { + hir::ExprKind::Call(ref func, ref args) => { + if_chain! { + if let hir::ExprKind::Path(ref path) = func.node; + then { + if match_qpath(path, &paths::OPTION_SOME) { + if_chain! { + if let hir::ExprKind::Path(path) = &args[0].node; + if let Def::Local(ref local) = cx.tables.qpath_def(path, args[0].hir_id); + then { + if arg_id == *local { + return (false, false) + } + } + } + return (true, false); + } else { + // We don't know. It might do anything. + return (true, true); + } + } + } + (true, true) + }, + hir::ExprKind::Block(ref block, _) => { + if let Some(expr) = &block.expr { + check_expression(cx, arg_id, &expr) + } else { + (false, false) + } + }, + // There must be an else_arm or there will be a type error + hir::ExprKind::If(_, ref if_arm, Some(ref else_arm)) => { + let if_check = check_expression(cx, arg_id, if_arm); + let else_check = check_expression(cx, arg_id, else_arm); + (if_check.0 | else_check.0, if_check.1 | else_check.1) + }, + hir::ExprKind::Match(_, ref arms, _) => { + let mut found_mapping = false; + let mut found_filtering = false; + for arm in arms { + let (m, f) = check_expression(cx, arg_id, &arm.body); + found_mapping |= m; + found_filtering |= f; + } + (found_mapping, found_filtering) + }, + hir::ExprKind::Path(path) if match_qpath(path, &paths::OPTION_NONE) => (false, true), + _ => (true, true) + } +} + +struct ReturnVisitor<'a, 'tcx: 'a> { + cx: &'a LateContext<'a, 'tcx>, + arg_id: ast::NodeId, + // Found a non-None return that isn't Some(input) + found_mapping: bool, + // Found a return that isn't Some + found_filtering: bool, +} + +impl<'a, 'tcx: 'a> ReturnVisitor<'a, 'tcx> { + fn new(cx: &'a LateContext<'a, 'tcx>, arg_id: ast::NodeId) -> ReturnVisitor<'a, 'tcx> { + ReturnVisitor { + cx, + arg_id, + found_mapping: false, + found_filtering: false, + } + } +} + +impl<'a, 'tcx> Visitor<'tcx> for ReturnVisitor<'a, 'tcx> { + fn visit_expr(&mut self, expr: &'tcx hir::Expr) { + if let hir::ExprKind::Ret(Some(expr)) = &expr.node { + let (found_mapping, found_filtering) = check_expression(self.cx, self.arg_id, expr); + self.found_mapping |= found_mapping; + self.found_filtering |= found_filtering; + } else { + walk_expr(self, expr); + } + } + + fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { + NestedVisitorMap::None + } +}