Auto merge of #3756 - g-bartoszek:redundant-closure-for-methods, r=ol…

…i-obk

Redundant closure for methods

fixes  #3469

clippy_dev/src/lib.rs

@@ -47,7 +47,7 @@ impl Lint {
             name: name.to_lowercase(),
             group: group.to_string(),
             desc: NL_ESCAPE_RE.replace(&desc.replace("\\\"", "\""), "").to_string(),
-            deprecation: deprecation.map(|d| d.to_string()),
+            deprecation: deprecation.map(std::string::ToString::to_string),
             module: module.to_string(),
         }
     }
@@ -178,7 +178,7 @@ fn lint_files() -> impl Iterator<Item = walkdir::DirEntry> {
     // Otherwise we would not collect all the lints, for example in `clippy_lints/src/methods/`.
     WalkDir::new("../clippy_lints/src")
         .into_iter()
-        .filter_map(|f| f.ok())
+        .filter_map(std::result::Result::ok)
         .filter(|f| f.path().extension() == Some(OsStr::new("rs")))
 }
 

clippy_lints/src/attrs.rs

@@ -326,7 +326,7 @@ fn check_clippy_lint_names(cx: &LateContext<'_, '_>, items: &[NestedMetaItem]) {
                     lint.span,
                     &format!("unknown clippy lint: clippy::{}", name),
                     |db| {
-                        if name.as_str().chars().any(|c| c.is_uppercase()) {
+                        if name.as_str().chars().any(char::is_uppercase) {
                             let name_lower = name.as_str().to_lowercase();
                             match lint_store.check_lint_name(
                                 &name_lower,

clippy_lints/src/cargo_common_metadata.rs

@@ -53,7 +53,7 @@ fn is_empty_str(value: &Option<String>) -> bool {
 
 fn is_empty_vec(value: &[String]) -> bool {
     // This works because empty iterators return true
-    value.iter().all(|v| v.is_empty())
+    value.iter().all(std::string::String::is_empty)
 }
 
 pub struct Pass;

clippy_lints/src/eta_reduction.rs

@@ -1,4 +1,5 @@
-use crate::utils::{is_adjusted, iter_input_pats, snippet_opt, span_lint_and_then};
+use crate::utils::{is_adjusted, iter_input_pats, snippet_opt, span_lint_and_then, type_is_unsafe_function};
+use if_chain::if_chain;
 use rustc::hir::*;
 use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
 use rustc::ty;
@@ -59,56 +60,136 @@ fn check_closure(cx: &LateContext<'_, '_>, expr: &Expr) {
     if let ExprKind::Closure(_, ref decl, eid, _, _) = expr.node {
         let body = cx.tcx.hir().body(eid);
         let ex = &body.value;
-        if let ExprKind::Call(ref caller, ref args) = ex.node {
-            if args.len() != decl.inputs.len() {
-                // Not the same number of arguments, there
-                // is no way the closure is the same as the function
-                return;
-            }
-            if is_adjusted(cx, ex) || args.iter().any(|arg| is_adjusted(cx, arg)) {
-                // Are the expression or the arguments type-adjusted? Then we need the closure
-                return;
-            }
+
+        if_chain!(
+            if let ExprKind::Call(ref caller, ref args) = ex.node;
+
+            // Not the same number of arguments, there is no way the closure is the same as the function return;
+            if args.len() == decl.inputs.len();
+
+            // Are the expression or the arguments type-adjusted? Then we need the closure
+            if !(is_adjusted(cx, ex) || args.iter().any(|arg| is_adjusted(cx, arg)));
+
             let fn_ty = cx.tables.expr_ty(caller);
-            match fn_ty.sty {
-                // Is it an unsafe function? They don't implement the closure traits
-                ty::FnDef(..) | ty::FnPtr(_) => {
-                    let sig = fn_ty.fn_sig(cx.tcx);
-                    if sig.skip_binder().unsafety == Unsafety::Unsafe || sig.skip_binder().output().sty == ty::Never {
-                        return;
+            if !type_is_unsafe_function(cx, fn_ty);
+
+            if compare_inputs(&mut iter_input_pats(decl, body), &mut args.into_iter());
+
+            then {
+                span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure found", |db| {
+                    if let Some(snippet) = snippet_opt(cx, caller.span) {
+                        db.span_suggestion(
+                            expr.span,
+                            "remove closure as shown",
+                            snippet,
+                            Applicability::MachineApplicable,
+                        );
                     }
-                },
-                _ => (),
+                });
             }
-            for (a1, a2) in iter_input_pats(decl, body).zip(args) {
-                if let PatKind::Binding(.., ident, _) = a1.pat.node {
-                    // XXXManishearth Should I be checking the binding mode here?
-                    if let ExprKind::Path(QPath::Resolved(None, ref p)) = a2.node {
-                        if p.segments.len() != 1 {
-                            // If it's a proper path, it can't be a local variable
-                            return;
-                        }
-                        if p.segments[0].ident.name != ident.name {
-                            // The two idents should be the same
-                            return;
-                        }
-                    } else {
-                        return;
-                    }
-                } else {
-                    return;
-                }
-            }
-            span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure found", |db| {
-                if let Some(snippet) = snippet_opt(cx, caller.span) {
+        );
+
+        if_chain!(
+            if let ExprKind::MethodCall(ref path, _, ref args) = ex.node;
+
+            // Not the same number of arguments, there is no way the closure is the same as the function return;
+            if args.len() == decl.inputs.len();
+
+            // Are the expression or the arguments type-adjusted? Then we need the closure
+            if !(is_adjusted(cx, ex) || args.iter().skip(1).any(|arg| is_adjusted(cx, arg)));
+
+            let method_def_id = cx.tables.type_dependent_defs()[ex.hir_id].def_id();
+            if !type_is_unsafe_function(cx, cx.tcx.type_of(method_def_id));
+
+            if compare_inputs(&mut iter_input_pats(decl, body), &mut args.into_iter());
+
+            if let Some(name) = get_ufcs_type_name(cx, method_def_id, &args[0]);
+
+            then {
+                span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure found", |db| {
                     db.span_suggestion(
                         expr.span,
                         "remove closure as shown",
-                        snippet,
+                        format!("{}::{}", name, path.ident.name),
                         Applicability::MachineApplicable,
                     );
+                });
+            }
+        );
+    }
+}
+
+/// Tries to determine the type for universal function call to be used instead of the closure
+fn get_ufcs_type_name(
+    cx: &LateContext<'_, '_>,
+    method_def_id: def_id::DefId,
+    self_arg: &Expr,
+) -> std::option::Option<String> {
+    let expected_type_of_self = &cx.tcx.fn_sig(method_def_id).inputs_and_output().skip_binder()[0].sty;
+    let actual_type_of_self = &cx.tables.node_id_to_type(self_arg.hir_id).sty;
+
+    if let Some(trait_id) = cx.tcx.trait_of_item(method_def_id) {
+        //if the method expectes &self, ufcs requires explicit borrowing so closure can't be removed
+        return match (expected_type_of_self, actual_type_of_self) {
+            (ty::Ref(_, _, _), ty::Ref(_, _, _)) => Some(cx.tcx.item_path_str(trait_id)),
+            (l, r) => match (l, r) {
+                (ty::Ref(_, _, _), _) | (_, ty::Ref(_, _, _)) => None,
+                (_, _) => Some(cx.tcx.item_path_str(trait_id)),
+            },
+        };
+    }
+
+    cx.tcx.impl_of_method(method_def_id).and_then(|_| {
+        //a type may implicitly implement other types methods (e.g. Deref)
+        if match_types(expected_type_of_self, actual_type_of_self) {
+            return Some(get_type_name(cx, &actual_type_of_self));
+        }
+        None
+    })
+}
+
+fn match_types(lhs: &ty::TyKind<'_>, rhs: &ty::TyKind<'_>) -> bool {
+    match (lhs, rhs) {
+        (ty::Bool, ty::Bool)
+        | (ty::Char, ty::Char)
+        | (ty::Int(_), ty::Int(_))
+        | (ty::Uint(_), ty::Uint(_))
+        | (ty::Str, ty::Str) => true,
+        (ty::Ref(_, t1, _), ty::Ref(_, t2, _))
+        | (ty::Array(t1, _), ty::Array(t2, _))
+        | (ty::Slice(t1), ty::Slice(t2)) => match_types(&t1.sty, &t2.sty),
+        (ty::Adt(def1, _), ty::Adt(def2, _)) => def1 == def2,
+        (_, _) => false,
+    }
+}
+
+fn get_type_name(cx: &LateContext<'_, '_>, kind: &ty::TyKind<'_>) -> String {
+    match kind {
+        ty::Adt(t, _) => cx.tcx.item_path_str(t.did),
+        ty::Ref(_, r, _) => get_type_name(cx, &r.sty),
+        _ => kind.to_string(),
+    }
+}
+
+fn compare_inputs(closure_inputs: &mut dyn Iterator<Item = &Arg>, call_args: &mut dyn Iterator<Item = &Expr>) -> bool {
+    for (closure_input, function_arg) in closure_inputs.zip(call_args) {
+        if let PatKind::Binding(_, _, _, ident, _) = closure_input.pat.node {
+            // XXXManishearth Should I be checking the binding mode here?
+            if let ExprKind::Path(QPath::Resolved(None, ref p)) = function_arg.node {
+                if p.segments.len() != 1 {
+                    // If it's a proper path, it can't be a local variable
+                    return false;
                 }
-            });
+                if p.segments[0].ident.name != ident.name {
+                    // The two idents should be the same
+                    return false;
+                }
+            } else {
+                return false;
+            }
+        } else {
+            return false;
         }
     }
+    true
 }

clippy_lints/src/lib.rs

@@ -321,7 +321,7 @@ pub fn read_conf(reg: &rustc_plugin::Registry<'_>) -> Conf {
                 }
             });
 
-            let (conf, errors) = utils::conf::read(file_name.as_ref().map(|p| p.as_ref()));
+            let (conf, errors) = utils::conf::read(file_name.as_ref().map(std::convert::AsRef::as_ref));
 
             // all conf errors are non-fatal, we just use the default conf in case of error
             for error in errors {

clippy_lints/src/methods/mod.rs

@@ -829,7 +829,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
 
         let (method_names, arg_lists) = method_calls(expr, 2);
         let method_names: Vec<LocalInternedString> = method_names.iter().map(|s| s.as_str()).collect();
-        let method_names: Vec<&str> = method_names.iter().map(|s| s.as_ref()).collect();
+        let method_names: Vec<&str> = method_names.iter().map(std::convert::AsRef::as_ref).collect();
 
         match method_names.as_slice() {
             ["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
 
     if let hir::ExprKind::MethodCall(ref path, _, ref args) = expr.node {
         if path.ident.name == "iter" && may_slice(cx, cx.tables.expr_ty(&args[0])) {
-            sugg::Sugg::hir_opt(cx, &args[0]).map(|sugg| sugg.addr())
+            sugg::Sugg::hir_opt(cx, &args[0]).map(sugg::Sugg::addr)
         } else {
             None
         }

clippy_lints/src/non_expressive_names.rs

@@ -241,7 +241,7 @@ impl<'a, 'tcx, 'b> SimilarNamesNameVisitor<'a, 'tcx, 'b> {
                         // or too many chars differ (x_foo, y_boo) or (xfoo, yboo)
                         continue;
                     }
-                    split_at = interned_name.chars().next().map(|c| c.len_utf8());
+                    split_at = interned_name.chars().next().map(char::len_utf8);
                 }
             }
             span_lint_and_then(

clippy_lints/src/utils/conf.rs

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

clippy_lints/src/utils/mod.rs

@@ -142,7 +142,10 @@ pub fn match_def_path(tcx: TyCtxt<'_, '_, '_>, def_id: DefId, path: &[&str]) ->
 pub fn get_def_path(tcx: TyCtxt<'_, '_, '_>, def_id: DefId) -> Vec<&'static str> {
     let mut apb = AbsolutePathBuffer { names: vec![] };
     tcx.push_item_path(&mut apb, def_id, false);
-    apb.names.iter().map(|n| n.get()).collect()
+    apb.names
+        .iter()
+        .map(syntax_pos::symbol::LocalInternedString::get)
+        .collect()
 }
 
 /// Check if type is struct, enum or union type with given def path.

rustc_tools_util/src/lib.rs

@@ -9,8 +9,8 @@ macro_rules! get_version_info {
         let crate_name = String::from(env!("CARGO_PKG_NAME"));
 
         let host_compiler = $crate::get_channel();
-        let commit_hash = option_env!("GIT_HASH").map(|s| s.to_string());
-        let commit_date = option_env!("COMMIT_DATE").map(|s| s.to_string());
+        let commit_hash = option_env!("GIT_HASH").map(str::to_string);
+        let commit_date = option_env!("COMMIT_DATE").map(str::to_string);
 
         VersionInfo {
             major,

src/driver.rs

@@ -47,7 +47,7 @@ fn arg_value<'a>(
 fn test_arg_value() {
     let args: Vec<_> = ["--bar=bar", "--foobar", "123", "--foo"]
         .iter()
-        .map(|s| s.to_string())
+        .map(std::string::ToString::to_string)
         .collect();
 
     assert_eq!(arg_value(None, "--foobar", |_| true), None);
@@ -84,7 +84,7 @@ pub fn main() {
             let sys_root_arg = arg_value(&orig_args, "--sysroot", |_| true);
             let have_sys_root_arg = sys_root_arg.is_some();
             let sys_root = sys_root_arg
-                .map(|s| s.to_string())
+                .map(std::string::ToString::to_string)
                 .or_else(|| std::env::var("SYSROOT").ok())
                 .or_else(|| {
                     let home = option_env!("RUSTUP_HOME").or(option_env!("MULTIRUST_HOME"));

tests/missing-test-files.rs

@@ -32,7 +32,7 @@ fn explore_directory(dir: &Path) -> Vec<String> {
     let mut missing_files: Vec<String> = Vec::new();
     let mut current_file = String::new();
     let mut files: Vec<DirEntry> = fs::read_dir(dir).unwrap().filter_map(Result::ok).collect();
-    files.sort_by_key(|e| e.path());
+    files.sort_by_key(std::fs::DirEntry::path);
     for entry in &files {
         let path = entry.path();
         if path.is_dir() {

tests/ui/eta.rs

@@ -9,6 +9,8 @@
 )]
 #![warn(clippy::redundant_closure, clippy::needless_borrow)]
 
+use std::path::PathBuf;
+
 fn main() {
     let a = Some(1u8).map(|a| foo(a));
     meta(|a| foo(a));
@@ -28,6 +30,66 @@ fn main() {
         Some(vec![1i32, 2]).map(|v| -> Box<::std::ops::Deref<Target = [i32]>> { Box::new(v) });
 }
 
+trait TestTrait {
+    fn trait_foo(self) -> bool;
+    fn trait_foo_ref(&self) -> bool;
+}
+
+struct TestStruct<'a> {
+    some_ref: &'a i32,
+}
+
+impl<'a> TestStruct<'a> {
+    fn foo(self) -> bool {
+        false
+    }
+    unsafe fn foo_unsafe(self) -> bool {
+        true
+    }
+}
+
+impl<'a> TestTrait for TestStruct<'a> {
+    fn trait_foo(self) -> bool {
+        false
+    }
+    fn trait_foo_ref(&self) -> bool {
+        false
+    }
+}
+
+impl<'a> std::ops::Deref for TestStruct<'a> {
+    type Target = char;
+    fn deref(&self) -> &char {
+        &'a'
+    }
+}
+
+fn test_redundant_closures_containing_method_calls() {
+    let i = 10;
+    let e = Some(TestStruct { some_ref: &i }).map(|a| a.foo());
+    let e = Some(TestStruct { some_ref: &i }).map(TestStruct::foo);
+    let e = Some(TestStruct { some_ref: &i }).map(|a| a.trait_foo());
+    let e = Some(TestStruct { some_ref: &i }).map(|a| a.trait_foo_ref());
+    let e = Some(TestStruct { some_ref: &i }).map(TestTrait::trait_foo);
+    let e = Some(&mut vec![1, 2, 3]).map(|v| v.clear());
+    let e = Some(&mut vec![1, 2, 3]).map(std::vec::Vec::clear);
+    unsafe {
+        let e = Some(TestStruct { some_ref: &i }).map(|a| a.foo_unsafe());
+    }
+    let e = Some("str").map(|s| s.to_string());
+    let e = Some("str").map(str::to_string);
+    let e = Some('a').map(|s| s.to_uppercase());
+    let e = Some('a').map(char::to_uppercase);
+    let e: std::vec::Vec<usize> = vec!['a', 'b', 'c'].iter().map(|c| c.len_utf8()).collect();
+    let e: std::vec::Vec<char> = vec!['a', 'b', 'c'].iter().map(|c| c.to_ascii_uppercase()).collect();
+    let e: std::vec::Vec<char> = vec!['a', 'b', 'c'].iter().map(char::to_ascii_uppercase).collect();
+    let p = Some(PathBuf::new());
+    let e = p.as_ref().and_then(|s| s.to_str());
+    let c = Some(TestStruct { some_ref: &i })
+        .as_ref()
+        .map(|c| c.to_ascii_uppercase());
+}
+
 fn meta<F>(f: F)
 where
     F: Fn(u8),