Skip to content

Commit 9d0b79d

Browse files
authored
Merge pull request #3223 from mikerite/unnecessary_filter_map
Implement unnecesary_filter_map lint
2 parents 37ba42b + 50133fb commit 9d0b79d

File tree

10 files changed

+241
-17
lines changed

10 files changed

+241
-17
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -864,6 +864,7 @@ All notable changes to this project will be documented in this file.
864864
[`unit_arg`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unit_arg
865865
[`unit_cmp`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unit_cmp
866866
[`unnecessary_cast`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unnecessary_cast
867+
[`unnecessary_filter_map`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unnecessary_filter_map
867868
[`unnecessary_fold`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unnecessary_fold
868869
[`unnecessary_mut_passed`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unnecessary_mut_passed
869870
[`unnecessary_operation`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unnecessary_operation

README.md

+1-1
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ We are currently in the process of discussing Clippy 1.0 via the RFC process in
99

1010
A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
1111

12-
[There are 278 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html)
12+
[There are 279 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html)
1313

1414
We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
1515

clippy_lints/src/lib.rs

+2
Original file line numberDiff line numberDiff line change
@@ -614,6 +614,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
614614
methods::SINGLE_CHAR_PATTERN,
615615
methods::STRING_EXTEND_CHARS,
616616
methods::TEMPORARY_CSTRING_AS_PTR,
617+
methods::UNNECESSARY_FILTER_MAP,
617618
methods::UNNECESSARY_FOLD,
618619
methods::USELESS_ASREF,
619620
methods::WRONG_SELF_CONVENTION,
@@ -829,6 +830,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
829830
methods::CLONE_ON_COPY,
830831
methods::FILTER_NEXT,
831832
methods::SEARCH_IS_SOME,
833+
methods::UNNECESSARY_FILTER_MAP,
832834
methods::USELESS_ASREF,
833835
misc::SHORT_CIRCUIT_STATEMENT,
834836
misc_early::REDUNDANT_CLOSURE_CALL,

clippy_lints/src/lifetimes.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -103,9 +103,9 @@ fn check_fn_inner<'a, 'tcx>(
103103
}
104104

105105
let mut bounds_lts = Vec::new();
106-
let types = generics.params.iter().filter_map(|param| match param.kind {
107-
GenericParamKind::Type { .. } => Some(param),
108-
GenericParamKind::Lifetime { .. } => None,
106+
let types = generics.params.iter().filter(|param| match param.kind {
107+
GenericParamKind::Type { .. } => true,
108+
GenericParamKind::Lifetime { .. } => false,
109109
});
110110
for typ in types {
111111
for bound in &typ.bounds {

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

+27-1
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ use std::borrow::Cow;
2020
use std::fmt;
2121
use std::iter;
2222

23+
mod unnecessary_filter_map;
24+
2325
#[derive(Clone)]
2426
pub struct Pass;
2527

@@ -692,6 +694,27 @@ declare_clippy_lint! {
692694
"using `fold` when a more succinct alternative exists"
693695
}
694696

697+
698+
/// **What it does:** Checks for `filter_map` calls which could be replaced by `filter` or `map`.
699+
///
700+
/// **Why is this bad?** Complexity
701+
///
702+
/// **Known problems:** None
703+
///
704+
/// **Example:**
705+
/// ```rust
706+
/// let _ = (0..3).filter_map(|x| if x > 2 { Some(x) } else { None });
707+
/// ```
708+
/// This could be written as:
709+
/// ```rust
710+
/// let _ = (0..3).filter(|&x| x > 2);
711+
/// ```
712+
declare_clippy_lint! {
713+
pub UNNECESSARY_FILTER_MAP,
714+
complexity,
715+
"using `filter_map` when a more succinct alternative exists"
716+
}
717+
695718
impl LintPass for Pass {
696719
fn get_lints(&self) -> LintArray {
697720
lint_array!(
@@ -725,7 +748,8 @@ impl LintPass for Pass {
725748
STRING_EXTEND_CHARS,
726749
ITER_CLONED_COLLECT,
727750
USELESS_ASREF,
728-
UNNECESSARY_FOLD
751+
UNNECESSARY_FOLD,
752+
UNNECESSARY_FILTER_MAP
729753
)
730754
}
731755
}
@@ -791,6 +815,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
791815
lint_asref(cx, expr, "as_mut", arglists[0]);
792816
} else if let Some(arglists) = method_chain_args(expr, &["fold"]) {
793817
lint_unnecessary_fold(cx, expr, arglists[0]);
818+
} else if let Some(arglists) = method_chain_args(expr, &["filter_map"]) {
819+
unnecessary_filter_map::lint(cx, expr, arglists[0]);
794820
}
795821

796822
lint_or_fun_call(cx, expr, *method_span, &method_call.ident.as_str(), args);
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
use crate::rustc::hir;
2+
use crate::rustc::hir::def::Def;
3+
use crate::rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
4+
use crate::rustc::lint::LateContext;
5+
use crate::syntax::ast;
6+
use crate::utils::paths;
7+
use crate::utils::usage::mutated_variables;
8+
use crate::utils::{match_qpath, match_trait_method, span_lint};
9+
10+
use if_chain::if_chain;
11+
12+
use super::UNNECESSARY_FILTER_MAP;
13+
14+
pub(super) fn lint(cx: &LateContext<'_, '_>, expr: &hir::Expr, args: &[hir::Expr]) {
15+
if !match_trait_method(cx, expr, &paths::ITERATOR) {
16+
return;
17+
}
18+
19+
if let hir::ExprKind::Closure(_, _, body_id, ..) = args[1].node {
20+
let body = cx.tcx.hir.body(body_id);
21+
let arg_id = body.arguments[0].pat.id;
22+
let mutates_arg = match mutated_variables(&body.value, cx) {
23+
Some(used_mutably) => used_mutably.contains(&arg_id),
24+
None => true,
25+
};
26+
27+
let (mut found_mapping, mut found_filtering) = check_expression(&cx, arg_id, &body.value);
28+
29+
let mut return_visitor = ReturnVisitor::new(&cx, arg_id);
30+
return_visitor.visit_expr(&body.value);
31+
found_mapping |= return_visitor.found_mapping;
32+
found_filtering |= return_visitor.found_filtering;
33+
34+
if !found_filtering {
35+
span_lint(
36+
cx,
37+
UNNECESSARY_FILTER_MAP,
38+
expr.span,
39+
"this `.filter_map` can be written more simply using `.map`",
40+
);
41+
return;
42+
}
43+
44+
if !found_mapping && !mutates_arg {
45+
span_lint(
46+
cx,
47+
UNNECESSARY_FILTER_MAP,
48+
expr.span,
49+
"this `.filter_map` can be written more simply using `.filter`",
50+
);
51+
return;
52+
}
53+
}
54+
}
55+
56+
// returns (found_mapping, found_filtering)
57+
fn check_expression<'a, 'tcx: 'a>(
58+
cx: &'a LateContext<'a, 'tcx>,
59+
arg_id: ast::NodeId,
60+
expr: &'tcx hir::Expr,
61+
) -> (bool, bool) {
62+
match &expr.node {
63+
hir::ExprKind::Call(ref func, ref args) => {
64+
if_chain! {
65+
if let hir::ExprKind::Path(ref path) = func.node;
66+
then {
67+
if match_qpath(path, &paths::OPTION_SOME) {
68+
if_chain! {
69+
if let hir::ExprKind::Path(path) = &args[0].node;
70+
if let Def::Local(ref local) = cx.tables.qpath_def(path, args[0].hir_id);
71+
then {
72+
if arg_id == *local {
73+
return (false, false)
74+
}
75+
}
76+
}
77+
return (true, false);
78+
} else {
79+
// We don't know. It might do anything.
80+
return (true, true);
81+
}
82+
}
83+
}
84+
(true, true)
85+
},
86+
hir::ExprKind::Block(ref block, _) => {
87+
if let Some(expr) = &block.expr {
88+
check_expression(cx, arg_id, &expr)
89+
} else {
90+
(false, false)
91+
}
92+
},
93+
// There must be an else_arm or there will be a type error
94+
hir::ExprKind::If(_, ref if_arm, Some(ref else_arm)) => {
95+
let if_check = check_expression(cx, arg_id, if_arm);
96+
let else_check = check_expression(cx, arg_id, else_arm);
97+
(if_check.0 | else_check.0, if_check.1 | else_check.1)
98+
},
99+
hir::ExprKind::Match(_, ref arms, _) => {
100+
let mut found_mapping = false;
101+
let mut found_filtering = false;
102+
for arm in arms {
103+
let (m, f) = check_expression(cx, arg_id, &arm.body);
104+
found_mapping |= m;
105+
found_filtering |= f;
106+
}
107+
(found_mapping, found_filtering)
108+
},
109+
hir::ExprKind::Path(path) if match_qpath(path, &paths::OPTION_NONE) => (false, true),
110+
_ => (true, true),
111+
}
112+
}
113+
114+
struct ReturnVisitor<'a, 'tcx: 'a> {
115+
cx: &'a LateContext<'a, 'tcx>,
116+
arg_id: ast::NodeId,
117+
// Found a non-None return that isn't Some(input)
118+
found_mapping: bool,
119+
// Found a return that isn't Some
120+
found_filtering: bool,
121+
}
122+
123+
impl<'a, 'tcx: 'a> ReturnVisitor<'a, 'tcx> {
124+
fn new(cx: &'a LateContext<'a, 'tcx>, arg_id: ast::NodeId) -> ReturnVisitor<'a, 'tcx> {
125+
ReturnVisitor {
126+
cx,
127+
arg_id,
128+
found_mapping: false,
129+
found_filtering: false,
130+
}
131+
}
132+
}
133+
134+
impl<'a, 'tcx> Visitor<'tcx> for ReturnVisitor<'a, 'tcx> {
135+
fn visit_expr(&mut self, expr: &'tcx hir::Expr) {
136+
if let hir::ExprKind::Ret(Some(expr)) = &expr.node {
137+
let (found_mapping, found_filtering) = check_expression(self.cx, self.arg_id, expr);
138+
self.found_mapping |= found_mapping;
139+
self.found_filtering |= found_filtering;
140+
} else {
141+
walk_expr(self, expr);
142+
}
143+
}
144+
145+
fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
146+
NestedVisitorMap::None
147+
}
148+
}

clippy_lints/src/utils/mod.rs

+4-7
Original file line numberDiff line numberDiff line change
@@ -682,13 +682,10 @@ impl LimitStack {
682682
}
683683

684684
pub fn get_attr<'a>(attrs: &'a [ast::Attribute], name: &'static str) -> impl Iterator<Item = &'a ast::Attribute> {
685-
attrs.iter().filter_map(move |attr| {
686-
if attr.path.segments.len() == 2 && attr.path.segments[0].ident.to_string() == "clippy" && attr.path.segments[1].ident.to_string() == name {
687-
Some(attr)
688-
} else {
689-
None
690-
}
691-
})
685+
attrs.iter().filter(move |attr|
686+
attr.path.segments.len() == 2 &&
687+
attr.path.segments[0].ident.to_string() == "clippy" &&
688+
attr.path.segments[1].ident.to_string() == name)
692689
}
693690

694691
fn parse_attrs<F: FnMut(u64)>(sess: &Session, attrs: &[ast::Attribute], name: &'static str, mut f: F) {

tests/ui/unnecessary_filter_map.rs

+12
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
fn main() {
2+
let _ = (0..4).filter_map(|x| if x > 1 { Some(x) } else { None });
3+
let _ = (0..4).filter_map(|x| { if x > 1 { return Some(x); }; None });
4+
let _ = (0..4).filter_map(|x| match x {
5+
0 | 1 => None,
6+
_ => Some(x),
7+
});
8+
9+
let _ = (0..4).filter_map(|x| Some(x + 1));
10+
11+
let _ = (0..4).filter_map(i32::checked_abs);
12+
}
+32
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
error: this `.filter_map` can be written more simply using `.filter`
2+
--> $DIR/unnecessary_filter_map.rs:2:13
3+
|
4+
2 | let _ = (0..4).filter_map(|x| if x > 1 { Some(x) } else { None });
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
6+
|
7+
= note: `-D clippy::unnecessary-filter-map` implied by `-D warnings`
8+
9+
error: this `.filter_map` can be written more simply using `.filter`
10+
--> $DIR/unnecessary_filter_map.rs:3:13
11+
|
12+
3 | let _ = (0..4).filter_map(|x| { if x > 1 { return Some(x); }; None });
13+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
14+
15+
error: this `.filter_map` can be written more simply using `.filter`
16+
--> $DIR/unnecessary_filter_map.rs:4:13
17+
|
18+
4 | let _ = (0..4).filter_map(|x| match x {
19+
| _____________^
20+
5 | | 0 | 1 => None,
21+
6 | | _ => Some(x),
22+
7 | | });
23+
| |______^
24+
25+
error: this `.filter_map` can be written more simply using `.map`
26+
--> $DIR/unnecessary_filter_map.rs:9:13
27+
|
28+
9 | let _ = (0..4).filter_map(|x| Some(x + 1));
29+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
30+
31+
error: aborting due to 4 previous errors
32+

util/update_lints.py

+11-5
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,12 @@ def collect(deprecated_lints, clippy_lints, fn):
4646
# remove \-newline escapes from description string
4747
desc = nl_escape_re.sub('', match.group('desc'))
4848
cat = match.group('cat')
49-
clippy_lints[cat].append((os.path.splitext(os.path.basename(fn))[0],
49+
if cat in ('internal', 'internal_warn'):
50+
continue
51+
module_name = os.path.splitext(os.path.basename(fn))[0]
52+
if module_name == 'mod':
53+
module_name = os.path.basename(os.path.dirname(fn))
54+
clippy_lints[cat].append((module_name,
5055
match.group('name').lower(),
5156
"allow",
5257
desc.replace('\\"', '"')))
@@ -138,10 +143,11 @@ def main(print_only=False, check=False):
138143
return
139144

140145
# collect all lints from source files
141-
for fn in os.listdir('clippy_lints/src'):
142-
if fn.endswith('.rs'):
143-
collect(deprecated_lints, clippy_lints,
144-
os.path.join('clippy_lints', 'src', fn))
146+
for root, dirs, files in os.walk('clippy_lints/src'):
147+
for fn in files:
148+
if fn.endswith('.rs'):
149+
collect(deprecated_lints, clippy_lints,
150+
os.path.join(root, fn))
145151

146152
# determine version
147153
with open('Cargo.toml') as fp:

0 commit comments

Comments
 (0)