Skip to content

Commit 87d47c4

Browse files
marcusklaasflip1995
marcusklaas
authored and
flip1995
committed
Add missed shortcircuit lint for collect calls
1 parent d68b8ce commit 87d47c4

File tree

5 files changed

+265
-2
lines changed

5 files changed

+265
-2
lines changed

‎clippy_lints/src/collect.rs

Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,194 @@
1+
use itertools::{repeat_n, Itertools};
2+
use rustc::hir::*;
3+
use rustc::lint::*;
4+
use rustc::ty::TypeVariants;
5+
use syntax::ast::NodeId;
6+
7+
use std::collections::HashSet;
8+
9+
use crate::utils::{match_trait_method, match_type, span_lint_and_sugg};
10+
use crate::utils::paths;
11+
12+
/// **What it does:** Detects collect calls on iterators to collections
13+
/// of either `Result<_, E>` or `Option<_>` inside functions that also
14+
/// have such a return type.
15+
///
16+
/// **Why is this bad?** It is possible to short-circuit these collect
17+
/// calls and return early whenever a `None` or `Err(E)` is encountered.
18+
///
19+
/// **Known problems:** It may be possible that a collection of options
20+
/// or results is intended. This would then generate a false positive.
21+
///
22+
/// **Example:**
23+
/// ```rust
24+
/// pub fn div(a: i32, b: &[i32]) -> Result<Vec<i32>, String> {
25+
/// let option_vec: Vec<_> = b.into_iter()
26+
/// .cloned()
27+
/// .map(|i| if i != 0 {
28+
/// Ok(a / i)
29+
/// } else {
30+
/// Err("Division by zero!".to_owned())
31+
/// })
32+
/// .collect();
33+
/// let mut int_vec = Vec::new();
34+
/// for opt in option_vec {
35+
/// int_vec.push(opt?);
36+
/// }
37+
/// Ok(int_vec)
38+
/// }
39+
/// ```
40+
declare_clippy_lint! {
41+
pub POSSIBLE_SHORTCIRCUITING_COLLECT,
42+
nursery,
43+
"missed shortcircuit opportunity on collect"
44+
}
45+
46+
#[derive(Clone)]
47+
pub struct Pass {
48+
// To ensure that we do not lint the same expression more than once
49+
seen_expr_nodes: HashSet<NodeId>,
50+
}
51+
52+
impl Pass {
53+
pub fn new() -> Self {
54+
Self { seen_expr_nodes: HashSet::new() }
55+
}
56+
}
57+
58+
impl LintPass for Pass {
59+
fn get_lints(&self) -> LintArray {
60+
lint_array!(POSSIBLE_SHORTCIRCUITING_COLLECT)
61+
}
62+
}
63+
64+
struct Suggestion {
65+
pattern: String,
66+
type_colloquial: &'static str,
67+
success_variant: &'static str,
68+
}
69+
70+
fn format_suggestion_pattern<'a, 'tcx>(
71+
cx: &LateContext<'a, 'tcx>,
72+
collection_ty: TypeVariants,
73+
is_option: bool,
74+
) -> String {
75+
let collection_pat = match collection_ty {
76+
TypeVariants::TyAdt(def, subs) => {
77+
let mut buf = cx.tcx.item_path_str(def.did);
78+
79+
if !subs.is_empty() {
80+
buf.push('<');
81+
buf.push_str(&repeat_n('_', subs.len()).join(", "));
82+
buf.push('>');
83+
}
84+
85+
buf
86+
},
87+
TypeVariants::TyParam(p) => p.to_string(),
88+
_ => "_".into(),
89+
};
90+
91+
if is_option {
92+
format!("Option<{}>", collection_pat)
93+
} else {
94+
format!("Result<{}, _>", collection_pat)
95+
}
96+
}
97+
98+
fn check_expr_for_collect<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) -> Option<Suggestion> {
99+
if let ExprMethodCall(ref method, _, ref args) = expr.node {
100+
if args.len() == 1 && method.name == "collect" && match_trait_method(cx, expr, &paths::ITERATOR) {
101+
let collect_ty = cx.tables.expr_ty(expr);
102+
103+
if match_type(cx, collect_ty, &paths::OPTION) || match_type(cx, collect_ty, &paths::RESULT) {
104+
// Already collecting into an Option or Result - good!
105+
return None;
106+
}
107+
108+
// Get the type of the Item associated to the Iterator on which collect() is
109+
// called.
110+
let arg_ty = cx.tables.expr_ty(&args[0]);
111+
let method_call = cx.tables.type_dependent_defs()[args[0].hir_id];
112+
let trt_id = cx.tcx.trait_of_item(method_call.def_id()).unwrap();
113+
let assoc_item_id = cx.tcx.associated_items(trt_id).next().unwrap().def_id;
114+
let substitutions = cx.tcx.mk_substs_trait(arg_ty, &[]);
115+
let projection = cx.tcx.mk_projection(assoc_item_id, substitutions);
116+
let normal_ty = cx.tcx.normalize_erasing_regions(
117+
cx.param_env,
118+
projection,
119+
);
120+
121+
return if match_type(cx, normal_ty, &paths::OPTION) {
122+
Some(Suggestion {
123+
pattern: format_suggestion_pattern(cx, collect_ty.sty.clone(), true),
124+
type_colloquial: "Option",
125+
success_variant: "Some",
126+
})
127+
} else if match_type(cx, normal_ty, &paths::RESULT) {
128+
Some(Suggestion {
129+
pattern: format_suggestion_pattern(cx, collect_ty.sty.clone(), false),
130+
type_colloquial: "Result",
131+
success_variant: "Ok",
132+
})
133+
} else {
134+
None
135+
};
136+
}
137+
}
138+
139+
None
140+
}
141+
142+
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
143+
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
144+
if self.seen_expr_nodes.contains(&expr.id) {
145+
return;
146+
}
147+
148+
if let Some(suggestion) = check_expr_for_collect(cx, expr) {
149+
let sugg_span = if let ExprMethodCall(_, call_span, _) = expr.node {
150+
expr.span.between(call_span)
151+
} else {
152+
unreachable!()
153+
};
154+
155+
span_lint_and_sugg(
156+
cx,
157+
POSSIBLE_SHORTCIRCUITING_COLLECT,
158+
sugg_span,
159+
&format!("you are creating a collection of `{}`s", suggestion.type_colloquial),
160+
&format!(
161+
"if you are only interested in the case where all values are `{}`, try",
162+
suggestion.success_variant
163+
),
164+
format!("collect::<{}>()", suggestion.pattern),
165+
);
166+
}
167+
}
168+
169+
fn check_stmt(&mut self, cx: &LateContext<'a, 'tcx>, stmt: &'tcx Stmt) {
170+
if_chain! {
171+
if let StmtDecl(ref decl, _) = stmt.node;
172+
if let DeclLocal(ref local) = decl.node;
173+
if let Some(ref ty) = local.ty;
174+
if let Some(ref expr) = local.init;
175+
then {
176+
self.seen_expr_nodes.insert(expr.id);
177+
178+
if let Some(suggestion) = check_expr_for_collect(cx, expr) {
179+
span_lint_and_sugg(
180+
cx,
181+
POSSIBLE_SHORTCIRCUITING_COLLECT,
182+
ty.span,
183+
&format!("you are creating a collection of `{}`s", suggestion.type_colloquial),
184+
&format!(
185+
"if you are only interested in the case where all values are `{}`, try",
186+
suggestion.success_variant
187+
),
188+
suggestion.pattern
189+
);
190+
}
191+
}
192+
}
193+
}
194+
}

‎clippy_lints/src/lib.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ pub mod booleans;
109109
pub mod bytecount;
110110
pub mod collapsible_if;
111111
pub mod const_static_lifetime;
112+
pub mod collect;
112113
pub mod copies;
113114
pub mod cyclomatic_complexity;
114115
pub mod derive;
@@ -423,7 +424,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
423424
reg.register_late_lint_pass(box inherent_impl::Pass::default());
424425
reg.register_late_lint_pass(box neg_cmp_op_on_partial_ord::NoNegCompOpForPartialOrd);
425426
reg.register_late_lint_pass(box unwrap::Pass);
426-
427+
reg.register_late_lint_pass(box collect::Pass::new());
427428

428429
reg.register_lint_group("clippy_restriction", vec![
429430
arithmetic::FLOAT_ARITHMETIC,
@@ -917,6 +918,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
917918

918919
reg.register_lint_group("clippy_nursery", vec![
919920
attrs::EMPTY_LINE_AFTER_OUTER_ATTR,
921+
collect::POSSIBLE_SHORTCIRCUITING_COLLECT,
920922
fallible_impl_from::FALLIBLE_IMPL_FROM,
921923
mutex_atomic::MUTEX_INTEGER,
922924
needless_borrow::NEEDLESS_BORROW,

‎tests/ui/collect.rs

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
#![warn(possible_shortcircuiting_collect)]
2+
3+
use std::iter::FromIterator;
4+
5+
pub fn div(a: i32, b: &[i32]) -> Result<Vec<i32>, String> {
6+
let option_vec: Vec<_> = b.into_iter()
7+
.cloned()
8+
.map(|i| if i != 0 {
9+
Ok(a / i)
10+
} else {
11+
Err("Division by zero!".to_owned())
12+
})
13+
.collect();
14+
let mut int_vec = Vec::new();
15+
for opt in option_vec {
16+
int_vec.push(opt?);
17+
}
18+
Ok(int_vec)
19+
}
20+
21+
pub fn generic<T>(a: &[T]) {
22+
// Make sure that our lint also works for generic functions.
23+
let _result: Vec<_> = a.iter().map(Some).collect();
24+
}
25+
26+
pub fn generic_collection<T, C: FromIterator<T> + FromIterator<Option<T>>>(elem: T) -> C {
27+
Some(Some(elem)).into_iter().collect()
28+
}
29+
30+
fn main() {
31+
// We're collecting into an `Option`. Do not trigger lint.
32+
let _sup: Option<Vec<_>> = (0..5).map(Some).collect();
33+
}

‎tests/ui/collect.stderr

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
error: you are creating a collection of `Result`s
2+
--> $DIR/collect.rs:6:21
3+
|
4+
6 | let option_vec: Vec<_> = b.into_iter()
5+
| ^^^^^^
6+
|
7+
= note: `-D possible-shortcircuiting-collect` implied by `-D warnings`
8+
help: if you are only interested in the case where all values are `Ok`, try
9+
|
10+
6 | let option_vec: Result<std::vec::Vec<_>, _> = b.into_iter()
11+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
12+
13+
error: you are creating a collection of `Option`s
14+
--> $DIR/collect.rs:23:18
15+
|
16+
23 | let _result: Vec<_> = a.iter().map(Some).collect();
17+
| ^^^^^^
18+
help: if you are only interested in the case where all values are `Some`, try
19+
|
20+
23 | let _result: Option<std::vec::Vec<_>> = a.iter().map(Some).collect();
21+
| ^^^^^^^^^^^^^^^^^^^^^^^^
22+
23+
error: you are creating a collection of `Option`s
24+
--> $DIR/collect.rs:27:34
25+
|
26+
27 | Some(Some(elem)).into_iter().collect()
27+
| ^^^^^^^^^
28+
help: if you are only interested in the case where all values are `Some`, try
29+
|
30+
27 | Some(Some(elem)).into_iter().collect::<Option<C>>()
31+
| ^^^^^^^^^^^^^^^^^^^^^^
32+
33+
error: aborting due to 3 previous errors
34+

‎tests/ui/filter_methods.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33

44
#![warn(clippy, clippy_pedantic)]
5-
#![allow(missing_docs_in_private_items)]
5+
#![allow(missing_docs_in_private_items, possible_shortcircuiting_collect)]
66

77
fn main() {
88
let _: Vec<_> = vec![5; 6].into_iter()

0 commit comments

Comments
 (0)