diff --git a/clippy_lints/src/methods/unnecessary_filter_map.rs b/clippy_lints/src/methods/unnecessary_filter_map.rs index 12ad3d8d690..108e143f337 100644 --- a/clippy_lints/src/methods/unnecessary_filter_map.rs +++ b/clippy_lints/src/methods/unnecessary_filter_map.rs @@ -1,4 +1,6 @@ +use super::utils::clone_or_copy_needed; use clippy_utils::diagnostics::span_lint; +use clippy_utils::ty::is_copy; use clippy_utils::usage::mutated_variables; use clippy_utils::{is_lang_ctor, is_trait_method, path_to_local_id}; use rustc_hir as hir; @@ -20,6 +22,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, arg: &hir::Expr< let arg_id = body.params[0].pat.hir_id; let mutates_arg = mutated_variables(&body.value, cx).map_or(true, |used_mutably| used_mutably.contains(&arg_id)); + let (clone_or_copy_needed, _) = clone_or_copy_needed(cx, body.params[0].pat, &body.value); let (mut found_mapping, mut found_filtering) = check_expression(cx, arg_id, &body.value); @@ -28,10 +31,10 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, arg: &hir::Expr< found_mapping |= return_visitor.found_mapping; found_filtering |= return_visitor.found_filtering; + let in_ty = cx.typeck_results().node_type(body.params[0].hir_id); let sugg = if !found_filtering { "map" - } else if !found_mapping && !mutates_arg { - let in_ty = cx.typeck_results().node_type(body.params[0].hir_id); + } else if !found_mapping && !mutates_arg && (!clone_or_copy_needed || is_copy(cx, in_ty)) { match cx.typeck_results().expr_ty(&body.value).kind() { ty::Adt(adt, subst) if cx.tcx.is_diagnostic_item(sym::Option, adt.did) && in_ty == subst.type_at(0) => { "filter" diff --git a/clippy_lints/src/methods/unnecessary_iter_cloned.rs b/clippy_lints/src/methods/unnecessary_iter_cloned.rs index 65e94c5f44a..7a39557ad57 100644 --- a/clippy_lints/src/methods/unnecessary_iter_cloned.rs +++ b/clippy_lints/src/methods/unnecessary_iter_cloned.rs @@ -1,14 +1,12 @@ +use super::utils::clone_or_copy_needed; use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::higher::ForLoop; use clippy_utils::source::snippet_opt; use clippy_utils::ty::{get_associated_type, get_iterator_item_ty, implements_trait}; -use clippy_utils::{fn_def_id, get_parent_expr, path_to_local_id, usage}; +use clippy_utils::{fn_def_id, get_parent_expr}; use rustc_errors::Applicability; -use rustc_hir::intravisit::{walk_expr, Visitor}; -use rustc_hir::{def_id::DefId, BorrowKind, Expr, ExprKind, HirId, LangItem, Mutability, Pat}; +use rustc_hir::{def_id::DefId, Expr, ExprKind, LangItem}; use rustc_lint::LateContext; -use rustc_middle::hir::nested_filter; -use rustc_middle::ty; use rustc_span::{sym, Symbol}; use super::UNNECESSARY_TO_OWNED; @@ -100,89 +98,6 @@ pub fn check_for_loop_iter( false } -/// The core logic of `check_for_loop_iter` above, this function wraps a use of -/// `CloneOrCopyVisitor`. -fn clone_or_copy_needed<'tcx>( - cx: &LateContext<'tcx>, - pat: &Pat<'tcx>, - body: &'tcx Expr<'tcx>, -) -> (bool, Vec<&'tcx Expr<'tcx>>) { - let mut visitor = CloneOrCopyVisitor { - cx, - binding_hir_ids: pat_bindings(pat), - clone_or_copy_needed: false, - addr_of_exprs: Vec::new(), - }; - visitor.visit_expr(body); - (visitor.clone_or_copy_needed, visitor.addr_of_exprs) -} - -/// Returns a vector of all `HirId`s bound by the pattern. -fn pat_bindings(pat: &Pat<'_>) -> Vec { - let mut collector = usage::ParamBindingIdCollector { - binding_hir_ids: Vec::new(), - }; - collector.visit_pat(pat); - collector.binding_hir_ids -} - -/// `clone_or_copy_needed` will be false when `CloneOrCopyVisitor` is done visiting if the only -/// operations performed on `binding_hir_ids` are: -/// * to take non-mutable references to them -/// * to use them as non-mutable `&self` in method calls -/// If any of `binding_hir_ids` is used in any other way, then `clone_or_copy_needed` will be true -/// when `CloneOrCopyVisitor` is done visiting. -struct CloneOrCopyVisitor<'cx, 'tcx> { - cx: &'cx LateContext<'tcx>, - binding_hir_ids: Vec, - clone_or_copy_needed: bool, - addr_of_exprs: Vec<&'tcx Expr<'tcx>>, -} - -impl<'cx, 'tcx> Visitor<'tcx> for CloneOrCopyVisitor<'cx, 'tcx> { - type NestedFilter = nested_filter::OnlyBodies; - - fn nested_visit_map(&mut self) -> Self::Map { - self.cx.tcx.hir() - } - - fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { - walk_expr(self, expr); - if self.is_binding(expr) { - if let Some(parent) = get_parent_expr(self.cx, expr) { - match parent.kind { - ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, _) => { - self.addr_of_exprs.push(parent); - return; - }, - ExprKind::MethodCall(_, args, _) => { - if_chain! { - if args.iter().skip(1).all(|arg| !self.is_binding(arg)); - if let Some(method_def_id) = self.cx.typeck_results().type_dependent_def_id(parent.hir_id); - let method_ty = self.cx.tcx.type_of(method_def_id); - let self_ty = method_ty.fn_sig(self.cx.tcx).input(0).skip_binder(); - if matches!(self_ty.kind(), ty::Ref(_, _, Mutability::Not)); - then { - return; - } - } - }, - _ => {}, - } - } - self.clone_or_copy_needed = true; - } - } -} - -impl<'cx, 'tcx> CloneOrCopyVisitor<'cx, 'tcx> { - fn is_binding(&self, expr: &Expr<'tcx>) -> bool { - self.binding_hir_ids - .iter() - .any(|hir_id| path_to_local_id(expr, *hir_id)) - } -} - /// Returns true if the named method is `IntoIterator::into_iter`. pub fn is_into_iter(cx: &LateContext<'_>, callee_def_id: DefId) -> bool { cx.tcx.lang_items().require(LangItem::IntoIterIntoIter) == Ok(callee_def_id) diff --git a/clippy_lints/src/methods/utils.rs b/clippy_lints/src/methods/utils.rs index 63c3273bd68..3015531e843 100644 --- a/clippy_lints/src/methods/utils.rs +++ b/clippy_lints/src/methods/utils.rs @@ -1,10 +1,14 @@ use clippy_utils::source::snippet_with_applicability; use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::{get_parent_expr, path_to_local_id, usage}; use if_chain::if_chain; use rustc_ast::ast; use rustc_errors::Applicability; use rustc_hir as hir; +use rustc_hir::intravisit::{walk_expr, Visitor}; +use rustc_hir::{BorrowKind, Expr, ExprKind, HirId, Mutability, Pat}; use rustc_lint::LateContext; +use rustc_middle::hir::nested_filter; use rustc_middle::ty::{self, Ty}; use rustc_span::symbol::sym; @@ -79,3 +83,86 @@ pub(super) fn get_hint_if_single_char_arg( } } } + +/// The core logic of `check_for_loop_iter` in `unnecessary_iter_cloned.rs`, this function wraps a +/// use of `CloneOrCopyVisitor`. +pub(super) fn clone_or_copy_needed<'tcx>( + cx: &LateContext<'tcx>, + pat: &Pat<'tcx>, + body: &'tcx Expr<'tcx>, +) -> (bool, Vec<&'tcx Expr<'tcx>>) { + let mut visitor = CloneOrCopyVisitor { + cx, + binding_hir_ids: pat_bindings(pat), + clone_or_copy_needed: false, + addr_of_exprs: Vec::new(), + }; + visitor.visit_expr(body); + (visitor.clone_or_copy_needed, visitor.addr_of_exprs) +} + +/// Returns a vector of all `HirId`s bound by the pattern. +fn pat_bindings(pat: &Pat<'_>) -> Vec { + let mut collector = usage::ParamBindingIdCollector { + binding_hir_ids: Vec::new(), + }; + collector.visit_pat(pat); + collector.binding_hir_ids +} + +/// `clone_or_copy_needed` will be false when `CloneOrCopyVisitor` is done visiting if the only +/// operations performed on `binding_hir_ids` are: +/// * to take non-mutable references to them +/// * to use them as non-mutable `&self` in method calls +/// If any of `binding_hir_ids` is used in any other way, then `clone_or_copy_needed` will be true +/// when `CloneOrCopyVisitor` is done visiting. +struct CloneOrCopyVisitor<'cx, 'tcx> { + cx: &'cx LateContext<'tcx>, + binding_hir_ids: Vec, + clone_or_copy_needed: bool, + addr_of_exprs: Vec<&'tcx Expr<'tcx>>, +} + +impl<'cx, 'tcx> Visitor<'tcx> for CloneOrCopyVisitor<'cx, 'tcx> { + type NestedFilter = nested_filter::OnlyBodies; + + fn nested_visit_map(&mut self) -> Self::Map { + self.cx.tcx.hir() + } + + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + walk_expr(self, expr); + if self.is_binding(expr) { + if let Some(parent) = get_parent_expr(self.cx, expr) { + match parent.kind { + ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, _) => { + self.addr_of_exprs.push(parent); + return; + }, + ExprKind::MethodCall(_, args, _) => { + if_chain! { + if args.iter().skip(1).all(|arg| !self.is_binding(arg)); + if let Some(method_def_id) = self.cx.typeck_results().type_dependent_def_id(parent.hir_id); + let method_ty = self.cx.tcx.type_of(method_def_id); + let self_ty = method_ty.fn_sig(self.cx.tcx).input(0).skip_binder(); + if matches!(self_ty.kind(), ty::Ref(_, _, Mutability::Not)); + then { + return; + } + } + }, + _ => {}, + } + } + self.clone_or_copy_needed = true; + } + } +} + +impl<'cx, 'tcx> CloneOrCopyVisitor<'cx, 'tcx> { + fn is_binding(&self, expr: &Expr<'tcx>) -> bool { + self.binding_hir_ids + .iter() + .any(|hir_id| path_to_local_id(expr, *hir_id)) + } +}