diff --git a/CHANGELOG.md b/CHANGELOG.md index 2bc4a8a8f91..c9bea1e8ef5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -864,6 +864,7 @@ All notable changes to this project will be documented in this file. [`unit_arg`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unit_arg [`unit_cmp`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unit_cmp [`unnecessary_cast`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unnecessary_cast +[`unnecessary_filter_map`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unnecessary_filter_map [`unnecessary_fold`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unnecessary_fold [`unnecessary_mut_passed`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unnecessary_mut_passed [`unnecessary_operation`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unnecessary_operation diff --git a/README.md b/README.md index 100d7649678..b8684b38631 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ We are currently in the process of discussing Clippy 1.0 via the RFC process in A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 278 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html) +[There are 279 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 3e25c79a7fd..9af4850b15c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -614,6 +614,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { methods::SINGLE_CHAR_PATTERN, methods::STRING_EXTEND_CHARS, methods::TEMPORARY_CSTRING_AS_PTR, + methods::UNNECESSARY_FILTER_MAP, methods::UNNECESSARY_FOLD, methods::USELESS_ASREF, methods::WRONG_SELF_CONVENTION, @@ -829,6 +830,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { methods::CLONE_ON_COPY, methods::FILTER_NEXT, methods::SEARCH_IS_SOME, + methods::UNNECESSARY_FILTER_MAP, methods::USELESS_ASREF, misc::SHORT_CIRCUIT_STATEMENT, misc_early::REDUNDANT_CLOSURE_CALL, diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 5a9818a6b63..a0c9e34b861 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -1,5 +1,6 @@ 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}; @@ -8,6 +9,7 @@ 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, @@ -692,6 +694,27 @@ declare_clippy_lint! { "using `fold` when a more succinct alternative exists" } + +/// **What it does:** Checks for `filter_map` calls which could be replaced by `filter` or `map`. +/// +/// **Why is this bad?** Complexity +/// +/// **Known problems:** None +/// +/// **Example:** +/// ```rust +/// let _ = (0..3).filter_map(|x| if x > 2 { Some(x) } else { None }); +/// ``` +/// This could be written as: +/// ```rust +/// let _ = (0..3).filter(|&x| x > 2); +/// ``` +declare_clippy_lint! { + pub UNNECESSARY_FILTER_MAP, + complexity, + "using `filter_map` when a more succinct alternative exists" +} + impl LintPass for Pass { fn get_lints(&self) -> LintArray { lint_array!( @@ -725,7 +748,8 @@ impl LintPass for Pass { STRING_EXTEND_CHARS, ITER_CLONED_COLLECT, USELESS_ASREF, - UNNECESSARY_FOLD + UNNECESSARY_FOLD, + UNNECESSARY_FILTER_MAP ) } } @@ -791,6 +815,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { lint_asref(cx, expr, "as_mut", arglists[0]); } else if let Some(arglists) = method_chain_args(expr, &["fold"]) { lint_unnecessary_fold(cx, expr, arglists[0]); + } else if let Some(arglists) = method_chain_args(expr, &["filter_map"]) { + unnecessary_filter_map::lint(cx, expr, arglists[0]); } lint_or_fun_call(cx, expr, *method_span, &method_call.ident.as_str(), args); @@ -1398,6 +1424,144 @@ 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/tests/ui/methods.rs b/tests/ui/methods.rs index 7faa45b987d..111c6ce5a8c 100644 --- a/tests/ui/methods.rs +++ b/tests/ui/methods.rs @@ -443,3 +443,17 @@ fn main() { let opt = Some(0); let _ = opt.unwrap(); } + +/// Checks implementation of `UNNECESSARY_FILTER_MAP` lint +fn unnecessary_filter_map() { + let _ = (0..4).filter_map(|x| if x > 1 { Some(x) } else { None }); + let _ = (0..4).filter_map(|x| { if x > 1 { return Some(x); }; None }); + let _ = (0..4).filter_map(|x| match x { + 0 | 1 => None, + _ => Some(x), + }); + + let _ = (0..4).filter_map(|x| Some(x + 1)); + + let _ = (0..4).filter_map(i32::checked_abs); +} diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index 3189f375647..b006813e26d 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -453,5 +453,35 @@ error: used unwrap() on an Option value. If you don't want to handle the None ca | = note: `-D clippy::option-unwrap-used` implied by `-D warnings` -error: aborting due to 56 previous errors +error: this `.filter_map` can be written more simply using `.filter` + --> $DIR/methods.rs:449:13 + | +449 | 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/methods.rs:450:13 + | +450 | let _ = (0..4).filter_map(|x| { if x > 1 { return Some(x); }; None }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this `.filter_map` can be written more simply using `.filter` + --> $DIR/methods.rs:451:13 + | +451 | let _ = (0..4).filter_map(|x| match x { + | _____________^ +452 | | 0 | 1 => None, +453 | | _ => Some(x), +454 | | }); + | |______^ + +error: this `.filter_map` can be written more simply using `.map` + --> $DIR/methods.rs:456:13 + | +456 | let _ = (0..4).filter_map(|x| Some(x + 1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 60 previous errors