Skip to content

Commit db13e6f

Browse files
borsbors
committed
Auto merge of #3756 - g-bartoszek:redundant-closure-for-methods, r=oli-obk
Redundant closure for methods fixes #3469
2 parents ed32876 + f7c0df9 commit db13e6f

17 files changed

+251
-67
lines changed

‎clippy_dev/src/lib.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ impl Lint {
4747
name: name.to_lowercase(),
4848
group: group.to_string(),
4949
desc: NL_ESCAPE_RE.replace(&desc.replace("\\\"", "\""), "").to_string(),
50-
deprecation: deprecation.map(|d| d.to_string()),
50+
deprecation: deprecation.map(std::string::ToString::to_string),
5151
module: module.to_string(),
5252
}
5353
}
@@ -178,7 +178,7 @@ fn lint_files() -> impl Iterator<Item = walkdir::DirEntry> {
178178
// Otherwise we would not collect all the lints, for example in `clippy_lints/src/methods/`.
179179
WalkDir::new("../clippy_lints/src")
180180
.into_iter()
181-
.filter_map(|f| f.ok())
181+
.filter_map(std::result::Result::ok)
182182
.filter(|f| f.path().extension() == Some(OsStr::new("rs")))
183183
}
184184

‎clippy_lints/src/attrs.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -326,7 +326,7 @@ fn check_clippy_lint_names(cx: &LateContext<'_, '_>, items: &[NestedMetaItem]) {
326326
lint.span,
327327
&format!("unknown clippy lint: clippy::{}", name),
328328
|db| {
329-
if name.as_str().chars().any(|c| c.is_uppercase()) {
329+
if name.as_str().chars().any(char::is_uppercase) {
330330
let name_lower = name.as_str().to_lowercase();
331331
match lint_store.check_lint_name(
332332
&name_lower,

‎clippy_lints/src/cargo_common_metadata.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ fn is_empty_str(value: &Option<String>) -> bool {
5353

5454
fn is_empty_vec(value: &[String]) -> bool {
5555
// This works because empty iterators return true
56-
value.iter().all(|v| v.is_empty())
56+
value.iter().all(std::string::String::is_empty)
5757
}
5858

5959
pub struct Pass;

‎clippy_lints/src/eta_reduction.rs

+123-42
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
use crate::utils::{is_adjusted, iter_input_pats, snippet_opt, span_lint_and_then};
1+
use crate::utils::{is_adjusted, iter_input_pats, snippet_opt, span_lint_and_then, type_is_unsafe_function};
2+
use if_chain::if_chain;
23
use rustc::hir::*;
34
use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
45
use rustc::ty;
@@ -59,56 +60,136 @@ fn check_closure(cx: &LateContext<'_, '_>, expr: &Expr) {
5960
if let ExprKind::Closure(_, ref decl, eid, _, _) = expr.node {
6061
let body = cx.tcx.hir().body(eid);
6162
let ex = &body.value;
62-
if let ExprKind::Call(ref caller, ref args) = ex.node {
63-
if args.len() != decl.inputs.len() {
64-
// Not the same number of arguments, there
65-
// is no way the closure is the same as the function
66-
return;
67-
}
68-
if is_adjusted(cx, ex) || args.iter().any(|arg| is_adjusted(cx, arg)) {
69-
// Are the expression or the arguments type-adjusted? Then we need the closure
70-
return;
71-
}
63+
64+
if_chain!(
65+
if let ExprKind::Call(ref caller, ref args) = ex.node;
66+
67+
// Not the same number of arguments, there is no way the closure is the same as the function return;
68+
if args.len() == decl.inputs.len();
69+
70+
// Are the expression or the arguments type-adjusted? Then we need the closure
71+
if !(is_adjusted(cx, ex) || args.iter().any(|arg| is_adjusted(cx, arg)));
72+
7273
let fn_ty = cx.tables.expr_ty(caller);
73-
match fn_ty.sty {
74-
// Is it an unsafe function? They don't implement the closure traits
75-
ty::FnDef(..) | ty::FnPtr(_) => {
76-
let sig = fn_ty.fn_sig(cx.tcx);
77-
if sig.skip_binder().unsafety == Unsafety::Unsafe || sig.skip_binder().output().sty == ty::Never {
78-
return;
74+
if !type_is_unsafe_function(cx, fn_ty);
75+
76+
if compare_inputs(&mut iter_input_pats(decl, body), &mut args.into_iter());
77+
78+
then {
79+
span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure found", |db| {
80+
if let Some(snippet) = snippet_opt(cx, caller.span) {
81+
db.span_suggestion(
82+
expr.span,
83+
"remove closure as shown",
84+
snippet,
85+
Applicability::MachineApplicable,
86+
);
7987
}
80-
},
81-
_ => (),
88+
});
8289
}
83-
for (a1, a2) in iter_input_pats(decl, body).zip(args) {
84-
if let PatKind::Binding(.., ident, _) = a1.pat.node {
85-
// XXXManishearth Should I be checking the binding mode here?
86-
if let ExprKind::Path(QPath::Resolved(None, ref p)) = a2.node {
87-
if p.segments.len() != 1 {
88-
// If it's a proper path, it can't be a local variable
89-
return;
90-
}
91-
if p.segments[0].ident.name != ident.name {
92-
// The two idents should be the same
93-
return;
94-
}
95-
} else {
96-
return;
97-
}
98-
} else {
99-
return;
100-
}
101-
}
102-
span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure found", |db| {
103-
if let Some(snippet) = snippet_opt(cx, caller.span) {
90+
);
91+
92+
if_chain!(
93+
if let ExprKind::MethodCall(ref path, _, ref args) = ex.node;
94+
95+
// Not the same number of arguments, there is no way the closure is the same as the function return;
96+
if args.len() == decl.inputs.len();
97+
98+
// Are the expression or the arguments type-adjusted? Then we need the closure
99+
if !(is_adjusted(cx, ex) || args.iter().skip(1).any(|arg| is_adjusted(cx, arg)));
100+
101+
let method_def_id = cx.tables.type_dependent_defs()[ex.hir_id].def_id();
102+
if !type_is_unsafe_function(cx, cx.tcx.type_of(method_def_id));
103+
104+
if compare_inputs(&mut iter_input_pats(decl, body), &mut args.into_iter());
105+
106+
if let Some(name) = get_ufcs_type_name(cx, method_def_id, &args[0]);
107+
108+
then {
109+
span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure found", |db| {
104110
db.span_suggestion(
105111
expr.span,
106112
"remove closure as shown",
107-
snippet,
113+
format!("{}::{}", name, path.ident.name),
108114
Applicability::MachineApplicable,
109115
);
116+
});
117+
}
118+
);
119+
}
120+
}
121+
122+
/// Tries to determine the type for universal function call to be used instead of the closure
123+
fn get_ufcs_type_name(
124+
cx: &LateContext<'_, '_>,
125+
method_def_id: def_id::DefId,
126+
self_arg: &Expr,
127+
) -> std::option::Option<String> {
128+
let expected_type_of_self = &cx.tcx.fn_sig(method_def_id).inputs_and_output().skip_binder()[0].sty;
129+
let actual_type_of_self = &cx.tables.node_id_to_type(self_arg.hir_id).sty;
130+
131+
if let Some(trait_id) = cx.tcx.trait_of_item(method_def_id) {
132+
//if the method expectes &self, ufcs requires explicit borrowing so closure can't be removed
133+
return match (expected_type_of_self, actual_type_of_self) {
134+
(ty::Ref(_, _, _), ty::Ref(_, _, _)) => Some(cx.tcx.item_path_str(trait_id)),
135+
(l, r) => match (l, r) {
136+
(ty::Ref(_, _, _), _) | (_, ty::Ref(_, _, _)) => None,
137+
(_, _) => Some(cx.tcx.item_path_str(trait_id)),
138+
},
139+
};
140+
}
141+
142+
cx.tcx.impl_of_method(method_def_id).and_then(|_| {
143+
//a type may implicitly implement other types methods (e.g. Deref)
144+
if match_types(expected_type_of_self, actual_type_of_self) {
145+
return Some(get_type_name(cx, &actual_type_of_self));
146+
}
147+
None
148+
})
149+
}
150+
151+
fn match_types(lhs: &ty::TyKind<'_>, rhs: &ty::TyKind<'_>) -> bool {
152+
match (lhs, rhs) {
153+
(ty::Bool, ty::Bool)
154+
| (ty::Char, ty::Char)
155+
| (ty::Int(_), ty::Int(_))
156+
| (ty::Uint(_), ty::Uint(_))
157+
| (ty::Str, ty::Str) => true,
158+
(ty::Ref(_, t1, _), ty::Ref(_, t2, _))
159+
| (ty::Array(t1, _), ty::Array(t2, _))
160+
| (ty::Slice(t1), ty::Slice(t2)) => match_types(&t1.sty, &t2.sty),
161+
(ty::Adt(def1, _), ty::Adt(def2, _)) => def1 == def2,
162+
(_, _) => false,
163+
}
164+
}
165+
166+
fn get_type_name(cx: &LateContext<'_, '_>, kind: &ty::TyKind<'_>) -> String {
167+
match kind {
168+
ty::Adt(t, _) => cx.tcx.item_path_str(t.did),
169+
ty::Ref(_, r, _) => get_type_name(cx, &r.sty),
170+
_ => kind.to_string(),
171+
}
172+
}
173+
174+
fn compare_inputs(closure_inputs: &mut dyn Iterator<Item = &Arg>, call_args: &mut dyn Iterator<Item = &Expr>) -> bool {
175+
for (closure_input, function_arg) in closure_inputs.zip(call_args) {
176+
if let PatKind::Binding(_, _, _, ident, _) = closure_input.pat.node {
177+
// XXXManishearth Should I be checking the binding mode here?
178+
if let ExprKind::Path(QPath::Resolved(None, ref p)) = function_arg.node {
179+
if p.segments.len() != 1 {
180+
// If it's a proper path, it can't be a local variable
181+
return false;
110182
}
111-
});
183+
if p.segments[0].ident.name != ident.name {
184+
// The two idents should be the same
185+
return false;
186+
}
187+
} else {
188+
return false;
189+
}
190+
} else {
191+
return false;
112192
}
113193
}
194+
true
114195
}

‎clippy_lints/src/lib.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -321,7 +321,7 @@ pub fn read_conf(reg: &rustc_plugin::Registry<'_>) -> Conf {
321321
}
322322
});
323323

324-
let (conf, errors) = utils::conf::read(file_name.as_ref().map(|p| p.as_ref()));
324+
let (conf, errors) = utils::conf::read(file_name.as_ref().map(std::convert::AsRef::as_ref));
325325

326326
// all conf errors are non-fatal, we just use the default conf in case of error
327327
for error in errors {

‎clippy_lints/src/methods/mod.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -829,7 +829,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
829829

830830
let (method_names, arg_lists) = method_calls(expr, 2);
831831
let method_names: Vec<LocalInternedString> = method_names.iter().map(|s| s.as_str()).collect();
832-
let method_names: Vec<&str> = method_names.iter().map(|s| s.as_ref()).collect();
832+
let method_names: Vec<&str> = method_names.iter().map(std::convert::AsRef::as_ref).collect();
833833

834834
match method_names.as_slice() {
835835
["unwrap", "get"] => lint_get_unwrap(cx, expr, arg_lists[1], false),
@@ -1695,7 +1695,7 @@ fn derefs_to_slice(cx: &LateContext<'_, '_>, expr: &hir::Expr, ty: Ty<'_>) -> Op
16951695

16961696
if let hir::ExprKind::MethodCall(ref path, _, ref args) = expr.node {
16971697
if path.ident.name == "iter" && may_slice(cx, cx.tables.expr_ty(&args[0])) {
1698-
sugg::Sugg::hir_opt(cx, &args[0]).map(|sugg| sugg.addr())
1698+
sugg::Sugg::hir_opt(cx, &args[0]).map(sugg::Sugg::addr)
16991699
} else {
17001700
None
17011701
}

‎clippy_lints/src/non_expressive_names.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,7 @@ impl<'a, 'tcx, 'b> SimilarNamesNameVisitor<'a, 'tcx, 'b> {
241241
// or too many chars differ (x_foo, y_boo) or (xfoo, yboo)
242242
continue;
243243
}
244-
split_at = interned_name.chars().next().map(|c| c.len_utf8());
244+
split_at = interned_name.chars().next().map(char::len_utf8);
245245
}
246246
}
247247
span_lint_and_then(

‎clippy_lints/src/utils/conf.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ use toml;
1414
pub fn file_from_args(
1515
args: &[source_map::Spanned<ast::NestedMetaItemKind>],
1616
) -> Result<Option<path::PathBuf>, (&'static str, source_map::Span)> {
17-
for arg in args.iter().filter_map(|a| a.meta_item()) {
17+
for arg in args.iter().filter_map(syntax::source_map::Spanned::meta_item) {
1818
if arg.name() == "conf_file" {
1919
return match arg.node {
2020
ast::MetaItemKind::Word | ast::MetaItemKind::List(_) => {

‎clippy_lints/src/utils/mod.rs

+4-1
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,10 @@ pub fn match_def_path(tcx: TyCtxt<'_, '_, '_>, def_id: DefId, path: &[&str]) ->
142142
pub fn get_def_path(tcx: TyCtxt<'_, '_, '_>, def_id: DefId) -> Vec<&'static str> {
143143
let mut apb = AbsolutePathBuffer { names: vec![] };
144144
tcx.push_item_path(&mut apb, def_id, false);
145-
apb.names.iter().map(|n| n.get()).collect()
145+
apb.names
146+
.iter()
147+
.map(syntax_pos::symbol::LocalInternedString::get)
148+
.collect()
146149
}
147150

148151
/// Check if type is struct, enum or union type with given def path.

‎rustc_tools_util/src/lib.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@ macro_rules! get_version_info {
99
let crate_name = String::from(env!("CARGO_PKG_NAME"));
1010

1111
let host_compiler = $crate::get_channel();
12-
let commit_hash = option_env!("GIT_HASH").map(|s| s.to_string());
13-
let commit_date = option_env!("COMMIT_DATE").map(|s| s.to_string());
12+
let commit_hash = option_env!("GIT_HASH").map(str::to_string);
13+
let commit_date = option_env!("COMMIT_DATE").map(str::to_string);
1414

1515
VersionInfo {
1616
major,

‎src/driver.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ fn arg_value<'a>(
4747
fn test_arg_value() {
4848
let args: Vec<_> = ["--bar=bar", "--foobar", "123", "--foo"]
4949
.iter()
50-
.map(|s| s.to_string())
50+
.map(std::string::ToString::to_string)
5151
.collect();
5252

5353
assert_eq!(arg_value(None, "--foobar", |_| true), None);
@@ -84,7 +84,7 @@ pub fn main() {
8484
let sys_root_arg = arg_value(&orig_args, "--sysroot", |_| true);
8585
let have_sys_root_arg = sys_root_arg.is_some();
8686
let sys_root = sys_root_arg
87-
.map(|s| s.to_string())
87+
.map(std::string::ToString::to_string)
8888
.or_else(|| std::env::var("SYSROOT").ok())
8989
.or_else(|| {
9090
let home = option_env!("RUSTUP_HOME").or(option_env!("MULTIRUST_HOME"));

‎tests/missing-test-files.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ fn explore_directory(dir: &Path) -> Vec<String> {
3232
let mut missing_files: Vec<String> = Vec::new();
3333
let mut current_file = String::new();
3434
let mut files: Vec<DirEntry> = fs::read_dir(dir).unwrap().filter_map(Result::ok).collect();
35-
files.sort_by_key(|e| e.path());
35+
files.sort_by_key(std::fs::DirEntry::path);
3636
for entry in &files {
3737
let path = entry.path();
3838
if path.is_dir() {

‎tests/ui/eta.rs

+62
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
)]
1010
#![warn(clippy::redundant_closure, clippy::needless_borrow)]
1111

12+
use std::path::PathBuf;
13+
1214
fn main() {
1315
let a = Some(1u8).map(|a| foo(a));
1416
meta(|a| foo(a));
@@ -28,6 +30,66 @@ fn main() {
2830
Some(vec![1i32, 2]).map(|v| -> Box<::std::ops::Deref<Target = [i32]>> { Box::new(v) });
2931
}
3032

33+
trait TestTrait {
34+
fn trait_foo(self) -> bool;
35+
fn trait_foo_ref(&self) -> bool;
36+
}
37+
38+
struct TestStruct<'a> {
39+
some_ref: &'a i32,
40+
}
41+
42+
impl<'a> TestStruct<'a> {
43+
fn foo(self) -> bool {
44+
false
45+
}
46+
unsafe fn foo_unsafe(self) -> bool {
47+
true
48+
}
49+
}
50+
51+
impl<'a> TestTrait for TestStruct<'a> {
52+
fn trait_foo(self) -> bool {
53+
false
54+
}
55+
fn trait_foo_ref(&self) -> bool {
56+
false
57+
}
58+
}
59+
60+
impl<'a> std::ops::Deref for TestStruct<'a> {
61+
type Target = char;
62+
fn deref(&self) -> &char {
63+
&'a'
64+
}
65+
}
66+
67+
fn test_redundant_closures_containing_method_calls() {
68+
let i = 10;
69+
let e = Some(TestStruct { some_ref: &i }).map(|a| a.foo());
70+
let e = Some(TestStruct { some_ref: &i }).map(TestStruct::foo);
71+
let e = Some(TestStruct { some_ref: &i }).map(|a| a.trait_foo());
72+
let e = Some(TestStruct { some_ref: &i }).map(|a| a.trait_foo_ref());
73+
let e = Some(TestStruct { some_ref: &i }).map(TestTrait::trait_foo);
74+
let e = Some(&mut vec![1, 2, 3]).map(|v| v.clear());
75+
let e = Some(&mut vec![1, 2, 3]).map(std::vec::Vec::clear);
76+
unsafe {
77+
let e = Some(TestStruct { some_ref: &i }).map(|a| a.foo_unsafe());
78+
}
79+
let e = Some("str").map(|s| s.to_string());
80+
let e = Some("str").map(str::to_string);
81+
let e = Some('a').map(|s| s.to_uppercase());
82+
let e = Some('a').map(char::to_uppercase);
83+
let e: std::vec::Vec<usize> = vec!['a', 'b', 'c'].iter().map(|c| c.len_utf8()).collect();
84+
let e: std::vec::Vec<char> = vec!['a', 'b', 'c'].iter().map(|c| c.to_ascii_uppercase()).collect();
85+
let e: std::vec::Vec<char> = vec!['a', 'b', 'c'].iter().map(char::to_ascii_uppercase).collect();
86+
let p = Some(PathBuf::new());
87+
let e = p.as_ref().and_then(|s| s.to_str());
88+
let c = Some(TestStruct { some_ref: &i })
89+
.as_ref()
90+
.map(|c| c.to_ascii_uppercase());
91+
}
92+
3193
fn meta<F>(f: F)
3294
where
3395
F: Fn(u8),

0 commit comments

Comments
 (0)