Merge pull request #3223 from mikerite/unnecessary_filter_map

Implement unnecesary_filter_map lint

Author: phansch

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

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:
 

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,

clippy_lints/src/lifetimes.rs

@@ -103,9 +103,9 @@ fn check_fn_inner<'a, 'tcx>(
     }
 
     let mut bounds_lts = Vec::new();
-    let types = generics.params.iter().filter_map(|param| match param.kind {
-        GenericParamKind::Type { .. } => Some(param),
-        GenericParamKind::Lifetime { .. } => None,
+    let types = generics.params.iter().filter(|param| match param.kind {
+        GenericParamKind::Type { .. } => true,
+        GenericParamKind::Lifetime { .. } => false,
     });
     for typ in types {
         for bound in &typ.bounds {

clippy_lints/src/methods.rs renamed to clippy_lints/src/methods/mod.rs

@@ -20,6 +20,8 @@ use std::borrow::Cow;
 use std::fmt;
 use std::iter;
 
+mod unnecessary_filter_map;
+
 #[derive(Clone)]
 pub struct Pass;
 
@@ -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);

clippy_lints/src/methods/unnecessary_filter_map.rs

@@ -0,0 +1,148 @@
+use crate::rustc::hir;
+use crate::rustc::hir::def::Def;
+use crate::rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
+use crate::rustc::lint::LateContext;
+use crate::syntax::ast;
+use crate::utils::paths;
+use crate::utils::usage::mutated_variables;
+use crate::utils::{match_qpath, match_trait_method, span_lint};
+
+use if_chain::if_chain;
+
+use super::UNNECESSARY_FILTER_MAP;
+
+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
+    }
+}

clippy_lints/src/utils/mod.rs

@@ -682,13 +682,10 @@ impl LimitStack {
 }
 
 pub fn get_attr<'a>(attrs: &'a [ast::Attribute], name: &'static str) -> impl Iterator<Item = &'a ast::Attribute> {
-    attrs.iter().filter_map(move |attr| {
-        if attr.path.segments.len() == 2 && attr.path.segments[0].ident.to_string() == "clippy" && attr.path.segments[1].ident.to_string() == name {
-            Some(attr)
-        } else {
-            None
-        }
-    })
+    attrs.iter().filter(move |attr|
+        attr.path.segments.len() == 2 &&
+        attr.path.segments[0].ident.to_string() == "clippy" &&
+        attr.path.segments[1].ident.to_string() == name)
 }
 
 fn parse_attrs<F: FnMut(u64)>(sess: &Session, attrs: &[ast::Attribute], name: &'static str, mut f: F) {

tests/ui/unnecessary_filter_map.rs

@@ -0,0 +1,12 @@
+fn main() {
+    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);
+}

tests/ui/unnecessary_filter_map.stderr

@@ -0,0 +1,32 @@
+error: this `.filter_map` can be written more simply using `.filter`
+ --> $DIR/unnecessary_filter_map.rs:2:13
+  |
+2 |     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
+  |
+3 |     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/unnecessary_filter_map.rs:4:13
+  |
+4 |       let _ = (0..4).filter_map(|x| match x {
+  |  _____________^
+5 | |         0 | 1 => None,
+6 | |         _ => Some(x),
+7 | |     });
+  | |______^
+
+error: this `.filter_map` can be written more simply using `.map`
+ --> $DIR/unnecessary_filter_map.rs:9:13
+  |
+9 |     let _ = (0..4).filter_map(|x| Some(x + 1));
+  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 4 previous errors
+

util/update_lints.py

@@ -46,7 +46,12 @@ def collect(deprecated_lints, clippy_lints, fn):
         # remove \-newline escapes from description string
         desc = nl_escape_re.sub('', match.group('desc'))
         cat = match.group('cat')
-        clippy_lints[cat].append((os.path.splitext(os.path.basename(fn))[0],
+        if cat in ('internal', 'internal_warn'):
+            continue
+        module_name = os.path.splitext(os.path.basename(fn))[0]
+        if module_name == 'mod':
+            module_name = os.path.basename(os.path.dirname(fn))
+        clippy_lints[cat].append((module_name,
                                   match.group('name').lower(),
                                   "allow",
                                   desc.replace('\\"', '"')))
@@ -138,10 +143,11 @@ def main(print_only=False, check=False):
         return
 
     # collect all lints from source files
-    for fn in os.listdir('clippy_lints/src'):
-        if fn.endswith('.rs'):
-            collect(deprecated_lints, clippy_lints,
-                    os.path.join('clippy_lints', 'src', fn))
+    for root, dirs, files in os.walk('clippy_lints/src'):
+        for fn in files:
+            if fn.endswith('.rs'):
+                collect(deprecated_lints, clippy_lints,
+                        os.path.join(root, fn))
 
     # determine version
     with open('Cargo.toml') as fp: