From cfd32522c077d7ced0ce3386b00b9896680dbdb2 Mon Sep 17 00:00:00 2001 From: "Samuel E. Moelius III" Date: Mon, 28 Feb 2022 04:59:33 -0500 Subject: [PATCH 1/2] Add tests to tests/ui/unnecessary_filter_map.rs The tests currently fail, and are fixed by the next commit. --- tests/ui/unnecessary_filter_map.rs | 129 +++++++++++++++++++++++++ tests/ui/unnecessary_filter_map.stderr | 8 +- 2 files changed, 133 insertions(+), 4 deletions(-) diff --git a/tests/ui/unnecessary_filter_map.rs b/tests/ui/unnecessary_filter_map.rs index c58181f518d..8e01c2674f1 100644 --- a/tests/ui/unnecessary_filter_map.rs +++ b/tests/ui/unnecessary_filter_map.rs @@ -1,3 +1,5 @@ +#![allow(dead_code)] + fn main() { let _ = (0..4).filter_map(|x| if x > 1 { Some(x) } else { None }); let _ = (0..4).filter_map(|x| { @@ -19,3 +21,130 @@ fn main() { fn filter_map_none_changes_item_type() -> impl Iterator { "".chars().filter_map(|_| None) } + +// https://github.com/rust-lang/rust-clippy/issues/4433#issue-483920107 +mod comment_483920107 { + enum Severity { + Warning, + Other, + } + + struct ServerError; + + impl ServerError { + fn severity(&self) -> Severity { + Severity::Warning + } + } + + struct S { + warnings: Vec, + } + + impl S { + fn foo(&mut self, server_errors: Vec) { + #[allow(unused_variables)] + let errors: Vec = server_errors + .into_iter() + .filter_map(|se| match se.severity() { + Severity::Warning => { + self.warnings.push(se); + None + }, + _ => Some(se), + }) + .collect(); + } + } +} + +// https://github.com/rust-lang/rust-clippy/issues/4433#issuecomment-611006622 +mod comment_611006622 { + struct PendingRequest { + reply_to: u8, + token: u8, + expires: u8, + group_id: u8, + } + + enum Value { + Null, + } + + struct Node; + + impl Node { + fn send_response(&self, _reply_to: u8, _token: u8, _value: Value) -> &Self { + self + } + fn on_error_warn(&self) -> &Self { + self + } + } + + struct S { + pending_requests: Vec, + } + + impl S { + fn foo(&mut self, node: Node, now: u8, group_id: u8) { + // "drain_filter" + self.pending_requests = self + .pending_requests + .drain(..) + .filter_map(|pending| { + if pending.expires <= now { + return None; // Expired, remove + } + + if pending.group_id == group_id { + // Matched - reuse strings and remove + node.send_response(pending.reply_to, pending.token, Value::Null) + .on_error_warn(); + None + } else { + // Keep waiting + Some(pending) + } + }) + .collect(); + } + } +} + +// https://github.com/rust-lang/rust-clippy/issues/4433#issuecomment-621925270 +// This extrapolation doesn't reproduce the false positive. Additional context seems necessary. +mod comment_621925270 { + struct Signature(u8); + + fn foo(sig_packets: impl Iterator>) -> impl Iterator { + sig_packets.filter_map(|res| match res { + Ok(Signature(sig_packet)) => Some(sig_packet), + _ => None, + }) + } +} + +// https://github.com/rust-lang/rust-clippy/issues/4433#issuecomment-1052978898 +mod comment_1052978898 { + #![allow(clippy::redundant_closure)] + + pub struct S(u8); + + impl S { + pub fn consume(self) { + println!("yum"); + } + } + + pub fn filter_owned() -> impl Iterator { + (0..10).map(|i| S(i)).filter_map(|s| { + if s.0 & 1 == 0 { + s.consume(); + None + } else { + Some(s) + } + }) + } +} diff --git a/tests/ui/unnecessary_filter_map.stderr b/tests/ui/unnecessary_filter_map.stderr index 041829c3c78..5585b10ab90 100644 --- a/tests/ui/unnecessary_filter_map.stderr +++ b/tests/ui/unnecessary_filter_map.stderr @@ -1,5 +1,5 @@ error: this `.filter_map` can be written more simply using `.filter` - --> $DIR/unnecessary_filter_map.rs:2:13 + --> $DIR/unnecessary_filter_map.rs:4:13 | LL | let _ = (0..4).filter_map(|x| if x > 1 { Some(x) } else { None }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -7,7 +7,7 @@ LL | let _ = (0..4).filter_map(|x| if x > 1 { Some(x) } else { None }); = note: `-D clippy::unnecessary-filter-map` implied by `-D warnings` error: this `.filter_map` can be written more simply using `.filter` - --> $DIR/unnecessary_filter_map.rs:3:13 + --> $DIR/unnecessary_filter_map.rs:5:13 | LL | let _ = (0..4).filter_map(|x| { | _____________^ @@ -19,7 +19,7 @@ LL | | }); | |______^ error: this `.filter_map` can be written more simply using `.filter` - --> $DIR/unnecessary_filter_map.rs:9:13 + --> $DIR/unnecessary_filter_map.rs:11:13 | LL | let _ = (0..4).filter_map(|x| match x { | _____________^ @@ -29,7 +29,7 @@ LL | | }); | |______^ error: this `.filter_map` can be written more simply using `.map` - --> $DIR/unnecessary_filter_map.rs:14:13 + --> $DIR/unnecessary_filter_map.rs:16:13 | LL | let _ = (0..4).filter_map(|x| Some(x + 1)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ From d123ffc6c754b25cc68a67ffab0b5c7707eac6e5 Mon Sep 17 00:00:00 2001 From: "Samuel E. Moelius III" Date: Mon, 28 Feb 2022 05:00:36 -0500 Subject: [PATCH 2/2] Check `clone_or_copy_needed` in `unnecessary_filter_map::check` --- .../src/methods/unnecessary_filter_map.rs | 7 +- .../src/methods/unnecessary_iter_cloned.rs | 91 +------------------ clippy_lints/src/methods/utils.rs | 87 ++++++++++++++++++ 3 files changed, 95 insertions(+), 90 deletions(-) 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)) + } +}