Implement unnecesary_filter_map lint

Michael Wright · Michael Wright · commit f5ffac4fce52 · 2018-09-26T06:52:36.000+02:00
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
 
