Auto merge of rust-lang#9745 - matttpt:fix-redundant-closure-for-method-calls-suggestion, r=flip1995

Fix `redundant_closure_for_method_calls` suggestion

Fixes rust-lang#7746. The issue turns out to be more general than raw pointers. The `redundant_closure_for_method_calls` lint produces incorrect suggestions when the method is associated with a type that must be enclosed in angle brackets or must be written with generic arguments substituted. For example:

```rust
fn main() {
    // Clippy's suggestion: [T; N]::as_slice
    // Correct suggestion:  <[u8; 3]>::as_slice
    let array_opt: Option<&[u8; 3]> = Some(&[4, 8, 7]);
    array_opt.map(|a| a.as_slice());

    // Clippy's suggestion: [T]::len
    // Correct suggestion:  <[u8]>::len
    let slice_opt: Option<&[u8]> = Some(b"slice");
    slice_opt.map(|s| s.len());

    // Clippy's suggestion: *const T::is_null
    // Correct suggestion:  <*const usize>::is_null
    let ptr_opt: Option<*const usize> = Some(&487);
    ptr_opt.map(|p| p.is_null());

    // Clippy's suggestion: dyn TestTrait::method_on_dyn
    // Correct suggestion:  <dyn TestTrait>::method_on_dyn
    let test_struct = TestStruct {};
    let dyn_opt: Option<&dyn TestTrait> = Some(&test_struct);
    dyn_opt.map(|d| d.method_on_dyn());
}

// For the trait object example:
trait TestTrait {}
struct TestStruct {}
impl TestTrait for TestStruct {}

impl dyn TestTrait + '_ {
    fn method_on_dyn(&self) -> bool {
        false
    }
}
```

The issue also affects references and tuples, though I had to patch the standard library with non-trait methods for those types to test that. Just in case, I also included handling for `!`, since it appeared to be possible to call methods on it with angle brackets. I just couldn't verify the resulting suggestion, since dead-code analysis eliminates the code first.

This is my first exposure to Rust compiler internals, so please let me know if I'm taking the wrong approach here!

changelog: [`redundant_closure_for_method_calls`]: add angle brackets and substitute generic arguments in suggestion when needed

Author: bors

Diff for: clippy_lints/src/eta_reduction.rs

@@ -11,7 +11,7 @@ use rustc_hir::{Closure, Expr, ExprKind, Param, PatKind, Unsafety};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow};
 use rustc_middle::ty::binding::BindingMode;
-use rustc_middle::ty::{self, Ty, TypeVisitable};
+use rustc_middle::ty::{self, EarlyBinder, SubstsRef, Ty, TypeVisitable};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::symbol::sym;
 
@@ -150,7 +150,7 @@ impl<'tcx> LateLintPass<'tcx> for EtaReduction {
             if check_sig(cx, closure_ty, call_ty);
             then {
                 span_lint_and_then(cx, REDUNDANT_CLOSURE_FOR_METHOD_CALLS, expr.span, "redundant closure", |diag| {
-                    let name = get_ufcs_type_name(cx, method_def_id);
+                    let name = get_ufcs_type_name(cx, method_def_id, substs);
                     diag.span_suggestion(
                         expr.span,
                         "replace the closure with the method itself",
@@ -220,7 +220,7 @@ fn check_sig<'tcx>(cx: &LateContext<'tcx>, closure_ty: Ty<'tcx>, call_ty: Ty<'tc
     cx.tcx.erase_late_bound_regions(closure_sig) == cx.tcx.erase_late_bound_regions(call_sig)
 }
 
-fn get_ufcs_type_name(cx: &LateContext<'_>, method_def_id: DefId) -> String {
+fn get_ufcs_type_name<'tcx>(cx: &LateContext<'tcx>, method_def_id: DefId, substs: SubstsRef<'tcx>) -> String {
     let assoc_item = cx.tcx.associated_item(method_def_id);
     let def_id = assoc_item.container_id(cx.tcx);
     match assoc_item.container {
@@ -229,6 +229,15 @@ fn get_ufcs_type_name(cx: &LateContext<'_>, method_def_id: DefId) -> String {
             let ty = cx.tcx.type_of(def_id);
             match ty.kind() {
                 ty::Adt(adt, _) => cx.tcx.def_path_str(adt.did()),
+                ty::Array(..)
+                | ty::Dynamic(..)
+                | ty::Never
+                | ty::RawPtr(_)
+                | ty::Ref(..)
+                | ty::Slice(_)
+                | ty::Tuple(_) => {
+                    format!("<{}>", EarlyBinder(ty).subst(cx.tcx, substs))
+                },
                 _ => ty.to_string(),
             }
         },

Diff for: tests/ui/eta.fixed

@@ -316,3 +316,25 @@ pub fn mutable_impl_fn_mut(mut f: impl FnMut(), mut f_used_once: impl FnMut()) -
 
     move || takes_fn_mut(&mut f_used_once)
 }
+
+impl dyn TestTrait + '_ {
+    fn method_on_dyn(&self) -> bool {
+        false
+    }
+}
+
+// https://github.com/rust-lang/rust-clippy/issues/7746
+fn angle_brackets_and_substs() {
+    let array_opt: Option<&[u8; 3]> = Some(&[4, 8, 7]);
+    array_opt.map(<[u8; 3]>::as_slice);
+
+    let slice_opt: Option<&[u8]> = Some(b"slice");
+    slice_opt.map(<[u8]>::len);
+
+    let ptr_opt: Option<*const usize> = Some(&487);
+    ptr_opt.map(<*const usize>::is_null);
+
+    let test_struct = TestStruct { some_ref: &487 };
+    let dyn_opt: Option<&dyn TestTrait> = Some(&test_struct);
+    dyn_opt.map(<dyn TestTrait>::method_on_dyn);
+}

Diff for: tests/ui/eta.rs

@@ -316,3 +316,25 @@ pub fn mutable_impl_fn_mut(mut f: impl FnMut(), mut f_used_once: impl FnMut()) -
 
     move || takes_fn_mut(|| f_used_once())
 }
+
+impl dyn TestTrait + '_ {
+    fn method_on_dyn(&self) -> bool {
+        false
+    }
+}
+
+// https://github.com/rust-lang/rust-clippy/issues/7746
+fn angle_brackets_and_substs() {
+    let array_opt: Option<&[u8; 3]> = Some(&[4, 8, 7]);
+    array_opt.map(|a| a.as_slice());
+
+    let slice_opt: Option<&[u8]> = Some(b"slice");
+    slice_opt.map(|s| s.len());
+
+    let ptr_opt: Option<*const usize> = Some(&487);
+    ptr_opt.map(|p| p.is_null());
+
+    let test_struct = TestStruct { some_ref: &487 };
+    let dyn_opt: Option<&dyn TestTrait> = Some(&test_struct);
+    dyn_opt.map(|d| d.method_on_dyn());
+}

Diff for: tests/ui/eta.stderr

@@ -134,5 +134,29 @@ error: redundant closure
 LL |     move || takes_fn_mut(|| f_used_once())
    |                          ^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `&mut f_used_once`
 
-error: aborting due to 22 previous errors
+error: redundant closure
+  --> $DIR/eta.rs:329:19
+   |
+LL |     array_opt.map(|a| a.as_slice());
+   |                   ^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `<[u8; 3]>::as_slice`
+
+error: redundant closure
+  --> $DIR/eta.rs:332:19
+   |
+LL |     slice_opt.map(|s| s.len());
+   |                   ^^^^^^^^^^^ help: replace the closure with the method itself: `<[u8]>::len`
+
+error: redundant closure
+  --> $DIR/eta.rs:335:17
+   |
+LL |     ptr_opt.map(|p| p.is_null());
+   |                 ^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `<*const usize>::is_null`
+
+error: redundant closure
+  --> $DIR/eta.rs:339:17
+   |
+LL |     dyn_opt.map(|d| d.method_on_dyn());
+   |                 ^^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `<dyn TestTrait>::method_on_dyn`
+
+error: aborting due to 26 previous errors