Auto merge of rust-lang#11370 - modelflat:suggest-relpath-in-redundant-closure-for-method-calls, r=blyxyas

[fix] [`redundant_closure_for_method_calls`] Suggest relative paths for local modules

Fixes rust-lang#10854.

Currently, `redundant_closure_for_method_calls` suggest incorrect paths when a method defined on a struct within inline mod is referenced (see the description in the aforementioned issue for an example; also see [this playground link](https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=f7d3c5b2663c9bd3ab7abdb0bd38ee43) for the current-version output for the test cases added in this PR). It will now try to construct a relative path path to the module and suggest it instead.

changelog: [`redundant_closure_for_method_calls`] Fix incorrect path suggestions for types within local modules

Author: bors

clippy_lints/src/eta_reduction.rs

@@ -3,15 +3,14 @@ use clippy_utils::higher::VecArgs;
 use clippy_utils::source::snippet_opt;
 use clippy_utils::ty::type_diagnostic_name;
 use clippy_utils::usage::{local_used_after_expr, local_used_in};
-use clippy_utils::{higher, is_adjusted, path_to_local, path_to_local_id};
+use clippy_utils::{get_path_from_caller_to_method_type, higher, is_adjusted, path_to_local, path_to_local_id};
 use rustc_errors::Applicability;
-use rustc_hir::def_id::DefId;
 use rustc_hir::{BindingAnnotation, Expr, ExprKind, FnRetTy, Param, PatKind, QPath, TyKind, Unsafety};
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty::{
-    self, Binder, ClosureArgs, ClosureKind, EarlyBinder, FnSig, GenericArg, GenericArgKind, GenericArgsRef,
-    ImplPolarity, List, Region, RegionKind, Ty, TypeVisitableExt, TypeckResults,
+    self, Binder, ClosureArgs, ClosureKind, FnSig, GenericArg, GenericArgKind, ImplPolarity, List, Region, RegionKind,
+    Ty, TypeVisitableExt, TypeckResults,
 };
 use rustc_session::declare_lint_pass;
 use rustc_span::symbol::sym;
@@ -203,11 +202,12 @@ impl<'tcx> LateLintPass<'tcx> for EtaReduction {
                         "redundant closure",
                         |diag| {
                             let args = typeck.node_args(body.value.hir_id);
-                            let name = get_ufcs_type_name(cx, method_def_id, args);
+                            let caller = self_.hir_id.owner.def_id;
+                            let type_name = get_path_from_caller_to_method_type(cx.tcx, caller, method_def_id, args);
                             diag.span_suggestion(
                                 expr.span,
                                 "replace the closure with the method itself",
-                                format!("{}::{}", name, path.ident.name),
+                                format!("{}::{}", type_name, path.ident.name),
                                 Applicability::MachineApplicable,
                             );
                         },
@@ -309,27 +309,3 @@ fn has_late_bound_to_non_late_bound_regions(from_sig: FnSig<'_>, to_sig: FnSig<'
         .zip(to_sig.inputs_and_output)
         .any(|(from_ty, to_ty)| check_ty(from_ty, to_ty))
 }
-
-fn get_ufcs_type_name<'tcx>(cx: &LateContext<'tcx>, method_def_id: DefId, args: GenericArgsRef<'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 {
-        ty::TraitContainer => cx.tcx.def_path_str(def_id),
-        ty::ImplContainer => {
-            let ty = cx.tcx.type_of(def_id).instantiate_identity();
-            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::bind(ty).instantiate(cx.tcx, args))
-                },
-                _ => ty.to_string(),
-            }
-        },
-    }
-}

clippy_utils/src/lib.rs

@@ -75,6 +75,7 @@ use core::mem;
 use core::ops::ControlFlow;
 use std::collections::hash_map::Entry;
 use std::hash::BuildHasherDefault;
+use std::iter::{once, repeat};
 use std::sync::{Mutex, MutexGuard, OnceLock};
 
 use itertools::Itertools;
@@ -84,6 +85,7 @@ use rustc_data_structures::packed::Pu128;
 use rustc_data_structures::unhash::UnhashMap;
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::{CrateNum, DefId, LocalDefId, LocalModDefId, LOCAL_CRATE};
+use rustc_hir::definitions::{DefPath, DefPathData};
 use rustc_hir::hir_id::{HirIdMap, HirIdSet};
 use rustc_hir::intravisit::{walk_expr, FnKind, Visitor};
 use rustc_hir::LangItem::{OptionNone, OptionSome, ResultErr, ResultOk};
@@ -102,8 +104,8 @@ use rustc_middle::ty::binding::BindingMode;
 use rustc_middle::ty::fast_reject::SimplifiedType;
 use rustc_middle::ty::layout::IntegerExt;
 use rustc_middle::ty::{
-    self as rustc_ty, Binder, BorrowKind, ClosureKind, FloatTy, IntTy, ParamEnv, ParamEnvAnd, Ty, TyCtxt, TypeAndMut,
-    TypeVisitableExt, UintTy, UpvarCapture,
+    self as rustc_ty, Binder, BorrowKind, ClosureKind, EarlyBinder, FloatTy, GenericArgsRef, IntTy, ParamEnv,
+    ParamEnvAnd, Ty, TyCtxt, TypeAndMut, TypeVisitableExt, UintTy, UpvarCapture,
 };
 use rustc_span::hygiene::{ExpnKind, MacroKind};
 use rustc_span::source_map::SourceMap;
@@ -3264,3 +3266,131 @@ pub fn is_never_expr<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> Option<
             })
     }
 }
+
+/// Produces a path from a local caller to the type of the called method. Suitable for user
+/// output/suggestions.
+///
+/// Returned path can be either absolute (for methods defined non-locally), or relative (for local
+/// methods).
+pub fn get_path_from_caller_to_method_type<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    from: LocalDefId,
+    method: DefId,
+    args: GenericArgsRef<'tcx>,
+) -> String {
+    let assoc_item = tcx.associated_item(method);
+    let def_id = assoc_item.container_id(tcx);
+    match assoc_item.container {
+        rustc_ty::TraitContainer => get_path_to_callee(tcx, from, def_id),
+        rustc_ty::ImplContainer => {
+            let ty = tcx.type_of(def_id).instantiate_identity();
+            get_path_to_ty(tcx, from, ty, args)
+        },
+    }
+}
+
+fn get_path_to_ty<'tcx>(tcx: TyCtxt<'tcx>, from: LocalDefId, ty: Ty<'tcx>, args: GenericArgsRef<'tcx>) -> String {
+    match ty.kind() {
+        rustc_ty::Adt(adt, _) => get_path_to_callee(tcx, from, adt.did()),
+        // TODO these types need to be recursively resolved as well
+        rustc_ty::Array(..)
+        | rustc_ty::Dynamic(..)
+        | rustc_ty::Never
+        | rustc_ty::RawPtr(_)
+        | rustc_ty::Ref(..)
+        | rustc_ty::Slice(_)
+        | rustc_ty::Tuple(_) => format!("<{}>", EarlyBinder::bind(ty).instantiate(tcx, args)),
+        _ => ty.to_string(),
+    }
+}
+
+/// Produce a path from some local caller to the callee. Suitable for user output/suggestions.
+fn get_path_to_callee(tcx: TyCtxt<'_>, from: LocalDefId, callee: DefId) -> String {
+    // only search for a relative path if the call is fully local
+    if callee.is_local() {
+        let callee_path = tcx.def_path(callee);
+        let caller_path = tcx.def_path(from.to_def_id());
+        maybe_get_relative_path(&caller_path, &callee_path, 2)
+    } else {
+        tcx.def_path_str(callee)
+    }
+}
+
+/// Tries to produce a relative path from `from` to `to`; if such a path would contain more than
+/// `max_super` `super` items, produces an absolute path instead. Both `from` and `to` should be in
+/// the local crate.
+///
+/// Suitable for user output/suggestions.
+///
+/// This ignores use items, and assumes that the target path is visible from the source
+/// path (which _should_ be a reasonable assumption since we in order to be able to use an object of
+/// certain type T, T is required to be visible).
+///
+/// TODO make use of `use` items. Maybe we should have something more sophisticated like
+/// rust-analyzer does? <https://docs.rs/ra_ap_hir_def/0.0.169/src/ra_ap_hir_def/find_path.rs.html#19-27>
+fn maybe_get_relative_path(from: &DefPath, to: &DefPath, max_super: usize) -> String {
+    use itertools::EitherOrBoth::{Both, Left, Right};
+
+    // 1. skip the segments common for both paths (regardless of their type)
+    let unique_parts = to
+        .data
+        .iter()
+        .zip_longest(from.data.iter())
+        .skip_while(|el| matches!(el, Both(l, r) if l == r))
+        .map(|el| match el {
+            Both(l, r) => Both(l.data, r.data),
+            Left(l) => Left(l.data),
+            Right(r) => Right(r.data),
+        });
+
+    // 2. for the remaning segments, construct relative path using only mod names and `super`
+    let mut go_up_by = 0;
+    let mut path = Vec::new();
+    for el in unique_parts {
+        match el {
+            Both(l, r) => {
+                // consider:
+                // a::b::sym:: ::    refers to
+                // c::d::e  ::f::sym
+                // result should be super::super::c::d::e::f
+                //
+                // alternatively:
+                // a::b::c  ::d::sym refers to
+                // e::f::sym:: ::
+                // result should be super::super::super::super::e::f
+                if let DefPathData::TypeNs(s) = l {
+                    path.push(s.to_string());
+                }
+                if let DefPathData::TypeNs(_) = r {
+                    go_up_by += 1;
+                }
+            },
+            // consider:
+            // a::b::sym:: ::    refers to
+            // c::d::e  ::f::sym
+            // when looking at `f`
+            Left(DefPathData::TypeNs(sym)) => path.push(sym.to_string()),
+            // consider:
+            // a::b::c  ::d::sym refers to
+            // e::f::sym:: ::
+            // when looking at `d`
+            Right(DefPathData::TypeNs(_)) => go_up_by += 1,
+            _ => {},
+        }
+    }
+
+    if go_up_by > max_super {
+        // `super` chain would be too long, just use the absolute path instead
+        once(String::from("crate"))
+            .chain(to.data.iter().filter_map(|el| {
+                if let DefPathData::TypeNs(sym) = el.data {
+                    Some(sym.to_string())
+                } else {
+                    None
+                }
+            }))
+            .join("::")
+    } else {
+        repeat(String::from("super")).take(go_up_by).chain(path).join("::")
+    }
+}

tests/ui/eta.fixed

@@ -417,3 +417,57 @@ fn _closure_with_types() {
     let _ = f2(|x: u32| f(x));
     let _ = f2(|x| -> u32 { f(x) });
 }
+
+/// https://github.com/rust-lang/rust-clippy/issues/10854
+/// This is to verify that redundant_closure_for_method_calls resolves suggested paths to relative.
+mod issue_10854 {
+    pub mod test_mod {
+        pub struct Test;
+
+        impl Test {
+            pub fn method(self) -> i32 {
+                0
+            }
+        }
+
+        pub fn calls_test(test: Option<Test>) -> Option<i32> {
+            test.map(Test::method)
+        }
+
+        pub fn calls_outer(test: Option<super::Outer>) -> Option<i32> {
+            test.map(super::Outer::method)
+        }
+    }
+
+    pub struct Outer;
+
+    impl Outer {
+        pub fn method(self) -> i32 {
+            0
+        }
+    }
+
+    pub fn calls_into_mod(test: Option<test_mod::Test>) -> Option<i32> {
+        test.map(test_mod::Test::method)
+    }
+
+    mod a {
+        pub mod b {
+            pub mod c {
+                pub fn extreme_nesting(test: Option<super::super::super::d::Test>) -> Option<i32> {
+                    test.map(crate::issue_10854::d::Test::method)
+                }
+            }
+        }
+    }
+
+    mod d {
+        pub struct Test;
+
+        impl Test {
+            pub fn method(self) -> i32 {
+                0
+            }
+        }
+    }
+}

tests/ui/eta.rs

@@ -417,3 +417,57 @@ fn _closure_with_types() {
     let _ = f2(|x: u32| f(x));
     let _ = f2(|x| -> u32 { f(x) });
 }
+
+/// https://github.com/rust-lang/rust-clippy/issues/10854
+/// This is to verify that redundant_closure_for_method_calls resolves suggested paths to relative.
+mod issue_10854 {
+    pub mod test_mod {
+        pub struct Test;
+
+        impl Test {
+            pub fn method(self) -> i32 {
+                0
+            }
+        }
+
+        pub fn calls_test(test: Option<Test>) -> Option<i32> {
+            test.map(|t| t.method())
+        }
+
+        pub fn calls_outer(test: Option<super::Outer>) -> Option<i32> {
+            test.map(|t| t.method())
+        }
+    }
+
+    pub struct Outer;
+
+    impl Outer {
+        pub fn method(self) -> i32 {
+            0
+        }
+    }
+
+    pub fn calls_into_mod(test: Option<test_mod::Test>) -> Option<i32> {
+        test.map(|t| t.method())
+    }
+
+    mod a {
+        pub mod b {
+            pub mod c {
+                pub fn extreme_nesting(test: Option<super::super::super::d::Test>) -> Option<i32> {
+                    test.map(|t| t.method())
+                }
+            }
+        }
+    }
+
+    mod d {
+        pub struct Test;
+
+        impl Test {
+            pub fn method(self) -> i32 {
+                0
+            }
+        }
+    }
+}

tests/ui/eta.stderr

@@ -166,5 +166,29 @@ error: redundant closure
 LL |     let _ = f(&0, |x, y| f2(x, y));
    |                   ^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `f2`
 
-error: aborting due to 27 previous errors
+error: redundant closure
+  --> $DIR/eta.rs:434:22
+   |
+LL |             test.map(|t| t.method())
+   |                      ^^^^^^^^^^^^^^ help: replace the closure with the method itself: `Test::method`
+
+error: redundant closure
+  --> $DIR/eta.rs:438:22
+   |
+LL |             test.map(|t| t.method())
+   |                      ^^^^^^^^^^^^^^ help: replace the closure with the method itself: `super::Outer::method`
+
+error: redundant closure
+  --> $DIR/eta.rs:451:18
+   |
+LL |         test.map(|t| t.method())
+   |                  ^^^^^^^^^^^^^^ help: replace the closure with the method itself: `test_mod::Test::method`
+
+error: redundant closure
+  --> $DIR/eta.rs:458:30
+   |
+LL |                     test.map(|t| t.method())
+   |                              ^^^^^^^^^^^^^^ help: replace the closure with the method itself: `crate::issue_10854::d::Test::method`
+
+error: aborting due to 31 previous errors