Fix min_ident_chars: add trait/impl awareness

Author: t-webber

clippy_lints/src/min_ident_chars.rs

@@ -5,11 +5,13 @@ use rustc_data_structures::fx::FxHashSet;
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::intravisit::{Visitor, walk_item, walk_trait_item};
 use rustc_hir::{
-    GenericParamKind, HirId, Impl, ImplItemKind, Item, ItemKind, ItemLocalId, Node, Pat, PatKind, TraitItem, UsePath,
+    GenericParamKind, HirId, Impl, ImplItem, ImplItemKind, Item, ItemKind, ItemLocalId, Node, Pat, PatKind, TraitItem,
+    UsePath,
 };
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_session::impl_lint_pass;
-use rustc_span::Span;
+use rustc_span::symbol::Ident;
+use rustc_span::{Span, Symbol};
 use std::borrow::Cow;
 
 declare_clippy_lint! {
@@ -34,6 +36,10 @@ declare_clippy_lint! {
     ///     let title = movie.title;
     /// }
     /// ```
+    ///
+    /// ### Limitations
+    /// Trait implementations which use the same function or parameter name as the trait declaration will
+    /// not be warned about, even if the name is below the configured limit.
     #[clippy::version = "1.72.0"]
     pub MIN_IDENT_CHARS,
     restriction,
@@ -78,6 +84,18 @@ impl LateLintPass<'_> for MinIdentChars {
             return;
         }
 
+        // If the function is declared but not defined in a trait, check_pat isn't called so we need to
+        // check this explicitly
+        if matches!(&item.kind, rustc_hir::TraitItemKind::Fn(_, _)) {
+            let param_names = cx.tcx.fn_arg_idents(item.owner_id.to_def_id());
+            for ident in param_names.iter().flatten() {
+                let str = ident.as_str();
+                if self.is_ident_too_short(cx, str, ident.span) {
+                    emit_min_ident_chars(self, cx, str, ident.span);
+                }
+            }
+        }
+
         walk_trait_item(&mut IdentVisitor { conf: self, cx }, item);
     }
 
@@ -86,7 +104,7 @@ impl LateLintPass<'_> for MinIdentChars {
         if let PatKind::Binding(_, _, ident, ..) = pat.kind
             && let str = ident.as_str()
             && self.is_ident_too_short(cx, str, ident.span)
-            && is_not_in_trait_impl(cx, pat)
+            && is_not_in_trait_impl(cx, pat, ident)
         {
             emit_min_ident_chars(self, cx, str, ident.span);
         }
@@ -121,6 +139,11 @@ impl Visitor<'_> for IdentVisitor<'_, '_> {
 
         let str = ident.as_str();
         if conf.is_ident_too_short(cx, str, ident.span) {
+            // Check whether the node is part of a `impl` for a trait.
+            if matches!(cx.tcx.parent_hir_node(hir_id), Node::TraitRef(_)) {
+                return;
+            }
+
             // Check whether the node is part of a `use` statement. We don't want to emit a warning if the user
             // has no control over the type.
             let usenode = opt_as_use_node(node).or_else(|| {
@@ -205,8 +228,9 @@ fn opt_as_use_node(node: Node<'_>) -> Option<&'_ UsePath<'_>> {
     }
 }
 
-/// Check if a pattern is a function param in an impl block for a trait.
-fn is_not_in_trait_impl(cx: &LateContext<'_>, pat: &Pat<'_>) -> bool {
+/// Check if a pattern is a function param in an impl block for a trait and that the param name is
+/// the same than in the trait definition.
+fn is_not_in_trait_impl(cx: &LateContext<'_>, pat: &Pat<'_>, ident: Ident) -> bool {
     let parent_node = cx.tcx.parent_hir_node(pat.hir_id);
     if !matches!(parent_node, Node::Param(_)) {
         return true;
@@ -219,12 +243,36 @@ fn is_not_in_trait_impl(cx: &LateContext<'_>, pat: &Pat<'_>) -> bool {
             let impl_parent_node = cx.tcx.parent_hir_node(impl_item.hir_id());
             if let Node::Item(parent_item) = impl_parent_node
                 && let ItemKind::Impl(Impl { of_trait: Some(_), .. }) = &parent_item.kind
+                && let Some(name) = get_param_name(impl_item, cx, ident)
             {
-                return false;
+                return name != ident.name;
             }
+
             return true;
         }
     }
 
     true
 }
+
+fn get_param_name(impl_item: &ImplItem<'_>, cx: &LateContext<'_>, ident: Ident) -> Option<Symbol> {
+    if let Some(trait_item_def_id) = impl_item.trait_item_def_id {
+        let trait_param_names = cx.tcx.fn_arg_idents(trait_item_def_id);
+
+        let ImplItemKind::Fn(_, body_id) = impl_item.kind else {
+            return None;
+        };
+
+        if let Some(param_index) = cx
+            .tcx
+            .hir_body_param_idents(body_id)
+            .position(|param_ident| param_ident.is_some_and(|param_ident| param_ident.span == ident.span))
+            && let Some(trait_param_name) = trait_param_names.get(param_index)
+            && let Some(trait_param_ident) = trait_param_name
+        {
+            return Some(trait_param_ident.name);
+        }
+    }
+
+    None
+}

tests/ui/min_ident_chars.rs

@@ -132,3 +132,44 @@ impl core::fmt::Display for Issue13396 {
         write!(f, "Issue13396")
     }
 }
+
+impl core::fmt::Debug for Issue13396 {
+    fn fmt(&self, g: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        //~^ min_ident_chars
+        write!(g, "Issue13396")
+    }
+}
+
+fn issue13396() {
+    let a = |f: i8| f;
+    //~^ min_ident_chars
+    //~| min_ident_chars
+}
+
+trait D {
+    //~^ min_ident_chars
+    fn f(g: i32);
+    //~^ min_ident_chars
+    //~| min_ident_chars
+    fn long(long: i32);
+
+    fn g(arg: i8) {
+        //~^ min_ident_chars
+        fn c(d: u8) {}
+        //~^ min_ident_chars
+        //~| min_ident_chars
+    }
+}
+
+impl D for Issue13396 {
+    fn f(g: i32) {
+        fn h() {}
+        //~^ min_ident_chars
+        fn inner(a: i32) {}
+        //~^ min_ident_chars
+        let a = |f: String| f;
+        //~^ min_ident_chars
+        //~| min_ident_chars
+    }
+    fn long(long: i32) {}
+}

tests/ui/min_ident_chars.stderr

@@ -193,5 +193,83 @@ error: this ident consists of a single char
 LL | fn wrong_pythagoras(a: f32, b: f32) -> f32 {
    |                             ^
 
-error: aborting due to 32 previous errors
+error: this ident consists of a single char
+  --> tests/ui/min_ident_chars.rs:137:19
+   |
+LL |     fn fmt(&self, g: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+   |                   ^
+
+error: this ident consists of a single char
+  --> tests/ui/min_ident_chars.rs:144:14
+   |
+LL |     let a = |f: i8| f;
+   |              ^
+
+error: this ident consists of a single char
+  --> tests/ui/min_ident_chars.rs:144:9
+   |
+LL |     let a = |f: i8| f;
+   |         ^
+
+error: this ident consists of a single char
+  --> tests/ui/min_ident_chars.rs:149:7
+   |
+LL | trait D {
+   |       ^
+
+error: this ident consists of a single char
+  --> tests/ui/min_ident_chars.rs:151:10
+   |
+LL |     fn f(g: i32);
+   |          ^
+
+error: this ident consists of a single char
+  --> tests/ui/min_ident_chars.rs:151:8
+   |
+LL |     fn f(g: i32);
+   |        ^
+
+error: this ident consists of a single char
+  --> tests/ui/min_ident_chars.rs:156:8
+   |
+LL |     fn g(arg: i8) {
+   |        ^
+
+error: this ident consists of a single char
+  --> tests/ui/min_ident_chars.rs:158:12
+   |
+LL |         fn c(d: u8) {}
+   |            ^
+
+error: this ident consists of a single char
+  --> tests/ui/min_ident_chars.rs:158:14
+   |
+LL |         fn c(d: u8) {}
+   |              ^
+
+error: this ident consists of a single char
+  --> tests/ui/min_ident_chars.rs:166:12
+   |
+LL |         fn h() {}
+   |            ^
+
+error: this ident consists of a single char
+  --> tests/ui/min_ident_chars.rs:168:18
+   |
+LL |         fn inner(a: i32) {}
+   |                  ^
+
+error: this ident consists of a single char
+  --> tests/ui/min_ident_chars.rs:170:18
+   |
+LL |         let a = |f: String| f;
+   |                  ^
+
+error: this ident consists of a single char
+  --> tests/ui/min_ident_chars.rs:170:13
+   |
+LL |         let a = |f: String| f;
+   |             ^
+
+error: aborting due to 45 previous errors