From c3dfcee48301f15d5a8dbfb0a671abfeb40102db Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Mon, 14 Dec 2020 13:50:53 -0600 Subject: [PATCH 1/5] Fix comment --- clippy_lints/src/methods/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 6a141d777daf..a2ee6ebc2563 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2994,7 +2994,7 @@ fn lint_filter_map_map<'tcx>( _filter_args: &'tcx [hir::Expr<'_>], _map_args: &'tcx [hir::Expr<'_>], ) { - // lint if caller of `.filter().map()` is an Iterator + // lint if caller of `.filter_map().map()` is an Iterator if match_trait_method(cx, expr, &paths::ITERATOR) { let msg = "called `filter_map(..).map(..)` on an `Iterator`"; let hint = "this is more succinctly expressed by only calling `.filter_map(..)` instead"; From dd7d72c675838f4164234ce0ec65cbed84c5ec8a Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Mon, 14 Dec 2020 12:19:27 -0600 Subject: [PATCH 2/5] Add heuristic to filter_map --- clippy_lints/src/methods/mod.rs | 42 ++++++++++++++++++++++++++------- clippy_lints/src/utils/usage.rs | 38 +++++++++++++++++++++++++++++ tests/ui/filter_methods.rs | 6 +++-- tests/ui/filter_methods.stderr | 18 ++++---------- 4 files changed, 81 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index a2ee6ebc2563..f97041b526ad 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -14,7 +14,8 @@ use if_chain::if_chain; use rustc_ast::ast; use rustc_errors::Applicability; use rustc_hir as hir; -use rustc_hir::{TraitItem, TraitItemKind}; +use rustc_hir::{ExprKind, TraitItem, TraitItemKind}; +use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::{LateContext, LateLintPass, Lint, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_middle::ty::{self, TraitRef, Ty, TyS}; @@ -22,10 +23,11 @@ use rustc_semver::RustcVersion; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::source_map::Span; use rustc_span::symbol::{sym, SymbolStr}; +use rustc_typeck::expr_use_visitor::ExprUseVisitor; use crate::consts::{constant, Constant}; use crate::utils::eager_or_lazy::is_lazyness_candidate; -use crate::utils::usage::mutated_variables; +use crate::utils::usage::{mutated_variables, UsesOptionOrResult}; use crate::utils::{ contains_return, contains_ty, get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, in_macro, is_copy, is_expn_of, is_type_diagnostic_item, iter_input_pats, last_path_segment, @@ -2927,17 +2929,41 @@ fn lint_skip_while_next<'tcx>( fn lint_filter_map<'tcx>( cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, - _filter_args: &'tcx [hir::Expr<'_>], + filter_args: &'tcx [hir::Expr<'_>], _map_args: &'tcx [hir::Expr<'_>], ) { - // lint if caller of `.filter().map()` is an Iterator - if match_trait_method(cx, expr, &paths::ITERATOR) { - let msg = "called `filter(..).map(..)` on an `Iterator`"; - let hint = "this is more succinctly expressed by calling `.filter_map(..)` instead"; - span_lint_and_help(cx, FILTER_MAP, expr.span, msg, None, hint); + if_chain! { + // lint if caller of `.filter().map()` is an Iterator + if match_trait_method(cx, expr, &paths::ITERATOR); + if let [_, filter_arg] = filter_args; + if filter_map_heuristic(filter_arg, cx); + then { + let msg = "called `filter(..).map(..)` on an `Iterator`"; + let hint = "this is more succinctly expressed by calling `.filter_map(..)` instead"; + span_lint_and_help(cx, FILTER_MAP, expr.span, msg, None, hint); + } } } +/// Checks the argument to `filter` to see if it is a candidate for replacement with `filter_map`. +/// Or similarly check `find` for replacement with `find_map`. +/// It simply checks if `Option` or `Result` is used at all. +fn filter_map_heuristic(filter_arg: &hir::Expr<'_>, cx: &LateContext<'_>) -> bool { + if let ExprKind::Closure(_, _, body_id, ..) = filter_arg.kind { + let map = cx.tcx.hir(); + let fn_def_id = map.local_def_id(filter_arg.hir_id); + let body = map.body(body_id); + let mut delegate = UsesOptionOrResult::new(cx); + cx.tcx.infer_ctxt().enter(|infcx| { + ExprUseVisitor::new(&mut delegate, &infcx, fn_def_id, cx.param_env, cx.typeck_results()).consume_body(body); + }); + if delegate.found { + return true; + } + } + false +} + const FILTER_MAP_NEXT_MSRV: RustcVersion = RustcVersion::new(1, 30, 0); /// lint use of `filter_map().next()` for `Iterators` diff --git a/clippy_lints/src/utils/usage.rs b/clippy_lints/src/utils/usage.rs index fc0db7f64ec9..550f98a0e45e 100644 --- a/clippy_lints/src/utils/usage.rs +++ b/clippy_lints/src/utils/usage.rs @@ -10,6 +10,7 @@ use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::LateContext; use rustc_middle::hir::map::Map; use rustc_middle::ty; +use rustc_span::sym; use rustc_span::symbol::{Ident, Symbol}; use rustc_typeck::expr_use_visitor::{ConsumeMode, Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId}; @@ -228,3 +229,40 @@ pub fn contains_return_break_continue_macro(expression: &Expr<'_>) -> bool { recursive_visitor.visit_expr(expression); recursive_visitor.seen_return_break_continue } + +pub struct UsesOptionOrResult<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + pub found: bool, +} + +impl<'a, 'tcx> UsesOptionOrResult<'a, 'tcx> { + pub fn new(cx: &'a LateContext<'tcx>) -> Self { + Self { cx, found: false } + } + + fn check_place_with_id(&mut self, place_with_id: &PlaceWithHirId<'tcx>) { + if !self.found { + if let Some(adt_def) = place_with_id.place.ty().peel_refs().ty_adt_def() { + if self.cx.tcx.is_diagnostic_item(sym::option_type, adt_def.did) + || self.cx.tcx.is_diagnostic_item(sym::result_type, adt_def.did) + { + self.found = true; + } + } + } + } +} + +impl<'a, 'tcx> Delegate<'tcx> for UsesOptionOrResult<'a, 'tcx> { + fn consume(&mut self, place_with_id: &PlaceWithHirId<'tcx>, _: HirId, _: ConsumeMode) { + self.check_place_with_id(place_with_id); + } + + fn borrow(&mut self, place_with_id: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) { + self.check_place_with_id(place_with_id); + } + + fn mutate(&mut self, place_with_id: &PlaceWithHirId<'tcx>, _: HirId) { + self.check_place_with_id(place_with_id); + } +} diff --git a/tests/ui/filter_methods.rs b/tests/ui/filter_methods.rs index 514502416192..5bc266451171 100644 --- a/tests/ui/filter_methods.rs +++ b/tests/ui/filter_methods.rs @@ -3,8 +3,6 @@ #![allow(clippy::missing_docs_in_private_items)] fn main() { - let _: Vec<_> = vec![5; 6].into_iter().filter(|&x| x == 0).map(|x| x * 2).collect(); - let _: Vec<_> = vec![5_i8; 6] .into_iter() .filter(|&x| x == 0) @@ -23,3 +21,7 @@ fn main() { .map(|x| x.checked_mul(2)) .collect(); } + +fn no_lint() { + let _: Vec<_> = vec![5; 6].into_iter().filter(|&x| x == 0).map(|x| x * 2).collect(); +} diff --git a/tests/ui/filter_methods.stderr b/tests/ui/filter_methods.stderr index 119226813793..8972fbd843b1 100644 --- a/tests/ui/filter_methods.stderr +++ b/tests/ui/filter_methods.stderr @@ -1,14 +1,5 @@ -error: called `filter(..).map(..)` on an `Iterator` - --> $DIR/filter_methods.rs:6:21 - | -LL | let _: Vec<_> = vec![5; 6].into_iter().filter(|&x| x == 0).map(|x| x * 2).collect(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = note: `-D clippy::filter-map` implied by `-D warnings` - = help: this is more succinctly expressed by calling `.filter_map(..)` instead - error: called `filter(..).flat_map(..)` on an `Iterator` - --> $DIR/filter_methods.rs:8:21 + --> $DIR/filter_methods.rs:6:21 | LL | let _: Vec<_> = vec![5_i8; 6] | _____________________^ @@ -17,10 +8,11 @@ LL | | .filter(|&x| x == 0) LL | | .flat_map(|x| x.checked_mul(2)) | |_______________________________________^ | + = note: `-D clippy::filter-map` implied by `-D warnings` = help: this is more succinctly expressed by calling `.flat_map(..)` and filtering by returning `iter::empty()` error: called `filter_map(..).flat_map(..)` on an `Iterator` - --> $DIR/filter_methods.rs:14:21 + --> $DIR/filter_methods.rs:12:21 | LL | let _: Vec<_> = vec![5_i8; 6] | _____________________^ @@ -32,7 +24,7 @@ LL | | .flat_map(|x| x.checked_mul(2)) = help: this is more succinctly expressed by calling `.flat_map(..)` and filtering by returning `iter::empty()` error: called `filter_map(..).map(..)` on an `Iterator` - --> $DIR/filter_methods.rs:20:21 + --> $DIR/filter_methods.rs:18:21 | LL | let _: Vec<_> = vec![5_i8; 6] | _____________________^ @@ -43,5 +35,5 @@ LL | | .map(|x| x.checked_mul(2)) | = help: this is more succinctly expressed by only calling `.filter_map(..)` instead -error: aborting due to 4 previous errors +error: aborting due to 3 previous errors From eeffba76cb3d9194f5599a7baa22e2a7142695c1 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Mon, 14 Dec 2020 13:50:36 -0600 Subject: [PATCH 3/5] Add heuristic to find_map --- clippy_lints/src/methods/mod.rs | 17 +++++++++++------ tests/ui/find_map.rs | 14 +++----------- tests/ui/find_map.stderr | 17 +---------------- 3 files changed, 15 insertions(+), 33 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index f97041b526ad..766081ba315d 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3002,14 +3002,19 @@ fn lint_filter_map_next<'tcx>( fn lint_find_map<'tcx>( cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, - _find_args: &'tcx [hir::Expr<'_>], + find_args: &'tcx [hir::Expr<'_>], map_args: &'tcx [hir::Expr<'_>], ) { - // lint if caller of `.filter().map()` is an Iterator - if match_trait_method(cx, &map_args[0], &paths::ITERATOR) { - let msg = "called `find(..).map(..)` on an `Iterator`"; - let hint = "this is more succinctly expressed by calling `.find_map(..)` instead"; - span_lint_and_help(cx, FIND_MAP, expr.span, msg, None, hint); + if_chain! { + // lint if caller of `.filter().map()` is an Iterator + if match_trait_method(cx, &map_args[0], &paths::ITERATOR); + if let [_, find_arg] = find_args; + if filter_map_heuristic(find_arg, cx); + then { + let msg = "called `find(..).map(..)` on an `Iterator`"; + let hint = "this is more succinctly expressed by calling `.find_map(..)` instead"; + span_lint_and_help(cx, FIND_MAP, expr.span, msg, None, hint); + } } } diff --git a/tests/ui/find_map.rs b/tests/ui/find_map.rs index 88d3b0e74900..a27eea521d60 100644 --- a/tests/ui/find_map.rs +++ b/tests/ui/find_map.rs @@ -18,16 +18,8 @@ fn main() { let a = ["lol", "NaN", "2", "5", "Xunda"]; let _: Option = a.iter().find(|s| s.parse::().is_ok()).map(|s| s.parse().unwrap()); +} - #[allow(clippy::match_like_matches_macro)] - let _: Option = desserts_of_the_week - .iter() - .find(|dessert| match *dessert { - Dessert::Cake(_) => true, - _ => false, - }) - .map(|dessert| match *dessert { - Dessert::Cake(ref flavor) => *flavor, - _ => unreachable!(), - }); +fn no_lint() { + let _ = vec![1].into_iter().find(|n| *n > 5).map(|n| n + 7); } diff --git a/tests/ui/find_map.stderr b/tests/ui/find_map.stderr index aea3cc62afcc..493688e53c60 100644 --- a/tests/ui/find_map.stderr +++ b/tests/ui/find_map.stderr @@ -7,20 +7,5 @@ LL | let _: Option = a.iter().find(|s| s.parse::().is_ok()).map(|s = note: `-D clippy::find-map` implied by `-D warnings` = help: this is more succinctly expressed by calling `.find_map(..)` instead -error: called `find(..).map(..)` on an `Iterator` - --> $DIR/find_map.rs:23:29 - | -LL | let _: Option = desserts_of_the_week - | _____________________________^ -LL | | .iter() -LL | | .find(|dessert| match *dessert { -LL | | Dessert::Cake(_) => true, -... | -LL | | _ => unreachable!(), -LL | | }); - | |__________^ - | - = help: this is more succinctly expressed by calling `.find_map(..)` instead - -error: aborting due to 2 previous errors +error: aborting due to previous error From c9cde4f09208d88b998be156cfda012b6ee272f0 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Mon, 14 Dec 2020 14:03:11 -0600 Subject: [PATCH 4/5] Remove unneeded allow's --- clippy_dev/src/ra_setup.rs | 2 -- clippy_lints/src/loops.rs | 1 - clippy_lints/src/utils/mod.rs | 2 -- 3 files changed, 5 deletions(-) diff --git a/clippy_dev/src/ra_setup.rs b/clippy_dev/src/ra_setup.rs index 40bf4a9505a8..39538e632b11 100644 --- a/clippy_dev/src/ra_setup.rs +++ b/clippy_dev/src/ra_setup.rs @@ -1,5 +1,3 @@ -#![allow(clippy::filter_map)] - use std::fs; use std::fs::File; use std::io::prelude::*; diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 1bd96b2b4c89..d566bd8ca3ea 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1061,7 +1061,6 @@ fn get_assignments<'a: 'c, 'tcx: 'c, 'c>( ) -> impl Iterator, &'tcx Expr<'tcx>)>> + 'c { // As the `filter` and `map` below do different things, I think putting together // just increases complexity. (cc #3188 and #4193) - #[allow(clippy::filter_map)] stmts .iter() .filter_map(move |stmt| match stmt.kind { diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index e83371f8b99a..6d8cfaa5f0d1 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -356,8 +356,6 @@ pub fn path_to_res(cx: &LateContext<'_>, path: &[&str]) -> Option { let item_def_id = current_item.res.def_id(); if cx.tcx.def_kind(item_def_id) == DefKind::Struct; then { - // Bad `find_map` suggestion. See #4193. - #[allow(clippy::find_map)] return cx.tcx.inherent_impls(item_def_id).iter() .flat_map(|&impl_def_id| cx.tcx.item_children(impl_def_id)) .find(|item| item.ident.name.as_str() == *segment) From ddaebbd4ee6ff36a194e92bcf09e20af6c775f9d Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Mon, 14 Dec 2020 14:03:54 -0600 Subject: [PATCH 5/5] Use new filter map heuristic in clippy --- clippy_lints/src/multiple_crate_versions.rs | 3 ++- clippy_lints/src/shadow.rs | 4 +--- clippy_lints/src/suspicious_operation_groupings.rs | 3 +-- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/multiple_crate_versions.rs b/clippy_lints/src/multiple_crate_versions.rs index c1773cef7a8b..b356b3a0e0c8 100644 --- a/clippy_lints/src/multiple_crate_versions.rs +++ b/clippy_lints/src/multiple_crate_versions.rs @@ -51,7 +51,8 @@ impl LateLintPass<'_> for MultipleCrateVersions { if let Some(resolve) = &metadata.resolve; if let Some(local_id) = packages .iter() - .find_map(|p| if p.name == *local_name { Some(&p.id) } else { None }); + .find(|p| p.name == *local_name) + .map(|p| &p.id); then { for (name, group) in &packages.iter().group_by(|p| p.name.clone()) { let group: Vec<&Package> = group.collect(); diff --git a/clippy_lints/src/shadow.rs b/clippy_lints/src/shadow.rs index 225fe58906f7..1f74065eb866 100644 --- a/clippy_lints/src/shadow.rs +++ b/clippy_lints/src/shadow.rs @@ -203,9 +203,7 @@ fn check_pat<'tcx>( if let ExprKind::Struct(_, ref efields, _) = init_struct.kind { for field in pfields { let name = field.ident.name; - let efield = efields - .iter() - .find_map(|f| if f.ident.name == name { Some(&*f.expr) } else { None }); + let efield = efields.iter().find(|f| f.ident.name == name).map(|f| f.expr); check_pat(cx, &field.pat, efield, span, bindings); } } else { diff --git a/clippy_lints/src/suspicious_operation_groupings.rs b/clippy_lints/src/suspicious_operation_groupings.rs index cccd24ccf940..66f2c7b38f04 100644 --- a/clippy_lints/src/suspicious_operation_groupings.rs +++ b/clippy_lints/src/suspicious_operation_groupings.rs @@ -688,6 +688,5 @@ fn skip_index(iter: Iter, index: usize) -> impl Iterator where Iter: Iterator, { - iter.enumerate() - .filter_map(move |(i, a)| if i == index { None } else { Some(a) }) + iter.enumerate().filter(move |&(i, _)| i != index).map(|(_, a)| a) }