From 613945aa7c1411ab8dc0e27b9e801f7731c5dee8 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Sat, 31 Aug 2019 07:22:42 +0200 Subject: [PATCH] Fix false positive in `unnecessary filter_map` Fixes #4433 --- .../src/methods/unnecessary_filter_map.rs | 107 ++++++++++++++---- tests/ui/unnecessary_filter_map.rs | 73 ++++++++++++ 2 files changed, 156 insertions(+), 24 deletions(-) diff --git a/clippy_lints/src/methods/unnecessary_filter_map.rs b/clippy_lints/src/methods/unnecessary_filter_map.rs index 1d562566fdf8..369cdadea271 100644 --- a/clippy_lints/src/methods/unnecessary_filter_map.rs +++ b/clippy_lints/src/methods/unnecessary_filter_map.rs @@ -1,27 +1,90 @@ use crate::utils::paths; -use crate::utils::usage::mutated_variables; use crate::utils::{match_qpath, match_trait_method, span_lint}; -use rustc::hir; use rustc::hir::def::Res; use rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; +use rustc::hir::{Body, Expr, ExprKind, HirId, Pat}; use rustc::lint::LateContext; +use rustc::middle::expr_use_visitor::*; +use rustc::middle::mem_categorization::{cmt_, Categorization}; +use rustc::ty; +use syntax::source_map::Span; use if_chain::if_chain; use super::UNNECESSARY_FILTER_MAP; -pub(super) fn lint(cx: &LateContext<'_, '_>, expr: &hir::Expr, args: &[hir::Expr]) { +fn can_be_ref(cx: &LateContext<'_, '_>, body: &Body, arg_id: HirId) -> bool { + struct CanBeRefDelegate { + arg_id: HirId, + result: bool, + } + + impl<'tcx> Delegate<'tcx> for CanBeRefDelegate { + fn consume(&mut self, _: HirId, _: Span, cmt: &cmt_<'tcx>, consume_mode: ConsumeMode) { + if_chain! { + if self.result; + if consume_mode != ConsumeMode::Copy; + if let Categorization::Local(id) = cmt.cat; + if self.arg_id == id; + then { + self.result = false; + } + } + } + fn borrow(&mut self, _: HirId, _: Span, cmt: &cmt_<'tcx>, _: ty::Region<'_>, bk: ty::BorrowKind, _: LoanCause) { + if_chain! { + if self.result; + if bk != ty::BorrowKind::ImmBorrow; + if let Categorization::Local(id) = cmt.cat; + if self.arg_id == id; + then { + self.result = false; + } + } + } + fn mutate(&mut self, _: HirId, _: Span, cmt: &cmt_<'tcx>, mutate_mode: MutateMode) { + if_chain! { + if self.result; + if mutate_mode != MutateMode::Init; + if let Categorization::Local(id) = cmt.cat; + if self.arg_id == id; + then { + self.result = false; + } + } + } + + fn matched_pat(&mut self, _: &Pat, _: &cmt_<'tcx>, _: MatchMode) {} + fn consume_pat(&mut self, _: &Pat, _: &cmt_<'tcx>, _: ConsumeMode) {} + fn decl_without_init(&mut self, _: HirId, _: Span) {} + } + + let mut delegate = CanBeRefDelegate { arg_id, result: true }; + + let closure_def_id = body.id().hir_id.owner_def_id(); + let region_scope_tree = &cx.tcx.region_scope_tree(closure_def_id); + ExprUseVisitor::new( + &mut delegate, + cx.tcx, + closure_def_id, + cx.param_env, + region_scope_tree, + cx.tables, + None, + ) + .consume_body(body); + + delegate.result +} + +pub(super) fn lint(cx: &LateContext<'_, '_>, expr: &Expr, args: &[Expr]) { if !match_trait_method(cx, expr, &paths::ITERATOR) { return; } - if let hir::ExprKind::Closure(_, _, body_id, ..) = args[1].node { + if let ExprKind::Closure(_, _, body_id, ..) = args[1].node { let body = cx.tcx.hir().body(body_id); let arg_id = body.params[0].pat.hir_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); @@ -40,7 +103,7 @@ pub(super) fn lint(cx: &LateContext<'_, '_>, expr: &hir::Expr, args: &[hir::Expr return; } - if !found_mapping && !mutates_arg { + if !found_mapping && can_be_ref(cx, body, arg_id) { span_lint( cx, UNNECESSARY_FILTER_MAP, @@ -53,19 +116,15 @@ pub(super) fn lint(cx: &LateContext<'_, '_>, expr: &hir::Expr, args: &[hir::Expr } // returns (found_mapping, found_filtering) -fn check_expression<'a, 'tcx>( - cx: &'a LateContext<'a, 'tcx>, - arg_id: hir::HirId, - expr: &'tcx hir::Expr, -) -> (bool, bool) { +fn check_expression<'a, 'tcx>(cx: &'a LateContext<'a, 'tcx>, arg_id: HirId, expr: &'tcx Expr) -> (bool, bool) { match &expr.node { - hir::ExprKind::Call(ref func, ref args) => { + ExprKind::Call(ref func, ref args) => { if_chain! { - if let hir::ExprKind::Path(ref path) = func.node; + if let 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 ExprKind::Path(path) = &args[0].node; if let Res::Local(ref local) = cx.tables.qpath_res(path, args[0].hir_id); then { if arg_id == *local { @@ -82,14 +141,14 @@ fn check_expression<'a, 'tcx>( } (true, true) }, - hir::ExprKind::Block(ref block, _) => { + ExprKind::Block(ref block, _) => { if let Some(expr) = &block.expr { check_expression(cx, arg_id, &expr) } else { (false, false) } }, - hir::ExprKind::Match(_, ref arms, _) => { + ExprKind::Match(_, ref arms, _) => { let mut found_mapping = false; let mut found_filtering = false; for arm in arms { @@ -99,14 +158,14 @@ fn check_expression<'a, 'tcx>( } (found_mapping, found_filtering) }, - hir::ExprKind::Path(path) if match_qpath(path, &paths::OPTION_NONE) => (false, true), + ExprKind::Path(path) if match_qpath(path, &paths::OPTION_NONE) => (false, true), _ => (true, true), } } struct ReturnVisitor<'a, 'tcx> { cx: &'a LateContext<'a, 'tcx>, - arg_id: hir::HirId, + arg_id: HirId, // Found a non-None return that isn't Some(input) found_mapping: bool, // Found a return that isn't Some @@ -114,7 +173,7 @@ struct ReturnVisitor<'a, 'tcx> { } impl<'a, 'tcx> ReturnVisitor<'a, 'tcx> { - fn new(cx: &'a LateContext<'a, 'tcx>, arg_id: hir::HirId) -> ReturnVisitor<'a, 'tcx> { + fn new(cx: &'a LateContext<'a, 'tcx>, arg_id: HirId) -> ReturnVisitor<'a, 'tcx> { ReturnVisitor { cx, arg_id, @@ -125,8 +184,8 @@ impl<'a, 'tcx> ReturnVisitor<'a, 'tcx> { } 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 { + fn visit_expr(&mut self, expr: &'tcx Expr) { + if let 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; diff --git a/tests/ui/unnecessary_filter_map.rs b/tests/ui/unnecessary_filter_map.rs index af858e4abcf5..8b89eddda1b1 100644 --- a/tests/ui/unnecessary_filter_map.rs +++ b/tests/ui/unnecessary_filter_map.rs @@ -14,4 +14,77 @@ fn main() { let _ = (0..4).filter_map(|x| Some(x + 1)); let _ = (0..4).filter_map(i32::checked_abs); + + struct S(u32); + impl S { + fn mutate(&mut self) {} + } + + // Mutating + { + let _: Vec<_> = vec![S(0), S(1), S(2)] + .into_iter() + .filter_map(|mut x| { + x.mutate(); + if x.0 % 2 == 0 { + Some(x) + } else { + None + } + }) + .collect(); + } + + // Moving + { + let mut v = vec![]; + let _: Vec<_> = vec![S(0), S(1), S(2)] + .into_iter() + .filter_map(|x| { + if x.0 % 2 == 0 { + Some(x) + } else { + v.push(x); + None + } + }) + .collect(); + } + + enum E { + A, + B(S), + } + + // Mutating with pattern + { + let _: Vec<_> = vec![E::A, E::B(S(0))] + .into_iter() + .filter_map(|mut x| { + if let E::B(ref mut y) = x { + y.mutate(); + None + } else { + Some(x) + } + }) + .collect(); + } + + // Moving with pattern + { + let mut v = vec![]; + let _: Vec<_> = vec![E::A, E::B(S(0))] + .into_iter() + .filter_map(|x| { + if let E::B(y) = x { + if y.0 % 2 == 0 { + v.push(y); + } + return None; + } + Some(x) + }) + .collect(); + } }