Auto merge of #12274 - nyurik:nit-format-impl, r=xFrednet

Minor refactor format-impls

Move all linting logic into a single format implementations struct

This should help with the future format-args improvements.

TODO: do the same with format_args.rs, perhaps in the same PR

**NOTE TO REVIEWERS**:  use "hide whitespace" in the github diff -- most of the code has shifted, but relatively low number of lines actually modified.

changelog: none

Author: bors

clippy_lints/src/format_impl.rs

@@ -7,7 +7,7 @@ use rustc_hir::{Expr, ExprKind, Impl, ImplItem, ImplItemKind, QPath};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::impl_lint_pass;
 use rustc_span::symbol::kw;
-use rustc_span::{sym, Span, Symbol};
+use rustc_span::{sym, Symbol};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -119,123 +119,132 @@ impl<'tcx> LateLintPass<'tcx> for FormatImpl {
     }
 
     fn check_impl_item_post(&mut self, cx: &LateContext<'_>, impl_item: &ImplItem<'_>) {
-        // Assume no nested Impl of Debug and Display within eachother
+        // Assume no nested Impl of Debug and Display within each other
         if is_format_trait_impl(cx, impl_item).is_some() {
             self.format_trait_impl = None;
         }
     }
 
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-        let Some(format_trait_impl) = self.format_trait_impl else {
-            return;
-        };
-
-        if format_trait_impl.name == sym::Display {
-            check_to_string_in_display(cx, expr);
+        if let Some(format_trait_impl) = self.format_trait_impl {
+            let linter = FormatImplExpr {
+                cx,
+                expr,
+                format_trait_impl,
+            };
+            linter.check_to_string_in_display();
+            linter.check_self_in_format_args();
+            linter.check_print_in_format_impl();
         }
-
-        check_self_in_format_args(cx, expr, format_trait_impl);
-        check_print_in_format_impl(cx, expr, format_trait_impl);
     }
 }
 
-fn check_to_string_in_display(cx: &LateContext<'_>, expr: &Expr<'_>) {
-    if let ExprKind::MethodCall(path, self_arg, ..) = expr.kind
-        // Get the hir_id of the object we are calling the method on
-        // Is the method to_string() ?
-        && path.ident.name == sym::to_string
-        // Is the method a part of the ToString trait? (i.e. not to_string() implemented
-        // separately)
-        && let Some(expr_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id)
-        && is_diag_trait_item(cx, expr_def_id, sym::ToString)
-        // Is the method is called on self
-        && let ExprKind::Path(QPath::Resolved(_, path)) = self_arg.kind
-        && let [segment] = path.segments
-        && segment.ident.name == kw::SelfLower
-    {
-        span_lint(
-            cx,
-            RECURSIVE_FORMAT_IMPL,
-            expr.span,
-            "using `self.to_string` in `fmt::Display` implementation will cause infinite recursion",
-        );
-    }
+struct FormatImplExpr<'a, 'tcx> {
+    cx: &'a LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    format_trait_impl: FormatTraitNames,
 }
 
-fn check_self_in_format_args<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, impl_trait: FormatTraitNames) {
-    // Check each arg in format calls - do we ever use Display on self (directly or via deref)?
-    if let Some(outer_macro) = root_macro_call_first_node(cx, expr)
-        && let macro_def_id = outer_macro.def_id
-        && is_format_macro(cx, macro_def_id)
-        && let Some(format_args) = find_format_args(cx, expr, outer_macro.expn)
-    {
-        for piece in &format_args.template {
-            if let FormatArgsPiece::Placeholder(placeholder) = piece
-                && let trait_name = match placeholder.format_trait {
-                    FormatTrait::Display => sym::Display,
-                    FormatTrait::Debug => sym::Debug,
-                    FormatTrait::LowerExp => sym!(LowerExp),
-                    FormatTrait::UpperExp => sym!(UpperExp),
-                    FormatTrait::Octal => sym!(Octal),
-                    FormatTrait::Pointer => sym::Pointer,
-                    FormatTrait::Binary => sym!(Binary),
-                    FormatTrait::LowerHex => sym!(LowerHex),
-                    FormatTrait::UpperHex => sym!(UpperHex),
+impl<'a, 'tcx> FormatImplExpr<'a, 'tcx> {
+    fn check_to_string_in_display(&self) {
+        if self.format_trait_impl.name == sym::Display
+            && let ExprKind::MethodCall(path, self_arg, ..) = self.expr.kind
+            // Get the hir_id of the object we are calling the method on
+            // Is the method to_string() ?
+            && path.ident.name == sym::to_string
+            // Is the method a part of the ToString trait? (i.e. not to_string() implemented
+            // separately)
+            && let Some(expr_def_id) = self.cx.typeck_results().type_dependent_def_id(self.expr.hir_id)
+            && is_diag_trait_item(self.cx, expr_def_id, sym::ToString)
+            // Is the method is called on self
+            && let ExprKind::Path(QPath::Resolved(_, path)) = self_arg.kind
+            && let [segment] = path.segments
+            && segment.ident.name == kw::SelfLower
+        {
+            span_lint(
+                self.cx,
+                RECURSIVE_FORMAT_IMPL,
+                self.expr.span,
+                "using `self.to_string` in `fmt::Display` implementation will cause infinite recursion",
+            );
+        }
+    }
+
+    fn check_self_in_format_args(&self) {
+        // Check each arg in format calls - do we ever use Display on self (directly or via deref)?
+        if let Some(outer_macro) = root_macro_call_first_node(self.cx, self.expr)
+            && let macro_def_id = outer_macro.def_id
+            && is_format_macro(self.cx, macro_def_id)
+            && let Some(format_args) = find_format_args(self.cx, self.expr, outer_macro.expn)
+        {
+            for piece in &format_args.template {
+                if let FormatArgsPiece::Placeholder(placeholder) = piece
+                    && let trait_name = match placeholder.format_trait {
+                        FormatTrait::Display => sym::Display,
+                        FormatTrait::Debug => sym::Debug,
+                        FormatTrait::LowerExp => sym!(LowerExp),
+                        FormatTrait::UpperExp => sym!(UpperExp),
+                        FormatTrait::Octal => sym!(Octal),
+                        FormatTrait::Pointer => sym::Pointer,
+                        FormatTrait::Binary => sym!(Binary),
+                        FormatTrait::LowerHex => sym!(LowerHex),
+                        FormatTrait::UpperHex => sym!(UpperHex),
+                    }
+                    && trait_name == self.format_trait_impl.name
+                    && let Ok(index) = placeholder.argument.index
+                    && let Some(arg) = format_args.arguments.all_args().get(index)
+                    && let Ok(arg_expr) = find_format_arg_expr(self.expr, arg)
+                {
+                    self.check_format_arg_self(arg_expr);
                 }
-                && trait_name == impl_trait.name
-                && let Ok(index) = placeholder.argument.index
-                && let Some(arg) = format_args.arguments.all_args().get(index)
-                && let Ok(arg_expr) = find_format_arg_expr(expr, arg)
-            {
-                check_format_arg_self(cx, expr.span, arg_expr, impl_trait);
             }
         }
     }
-}
 
-fn check_format_arg_self(cx: &LateContext<'_>, span: Span, arg: &Expr<'_>, impl_trait: FormatTraitNames) {
-    // Handle multiple dereferencing of references e.g. &&self
-    // Handle dereference of &self -> self that is equivalent (i.e. via *self in fmt() impl)
-    // Since the argument to fmt is itself a reference: &self
-    let reference = peel_ref_operators(cx, arg);
-    let map = cx.tcx.hir();
-    // Is the reference self?
-    if path_to_local(reference).map(|x| map.name(x)) == Some(kw::SelfLower) {
-        let FormatTraitNames { name, .. } = impl_trait;
-        span_lint(
-            cx,
-            RECURSIVE_FORMAT_IMPL,
-            span,
-            &format!("using `self` as `{name}` in `impl {name}` will cause infinite recursion"),
-        );
+    fn check_format_arg_self(&self, arg: &Expr<'_>) {
+        // Handle multiple dereferencing of references e.g. &&self
+        // Handle dereference of &self -> self that is equivalent (i.e. via *self in fmt() impl)
+        // Since the argument to fmt is itself a reference: &self
+        let reference = peel_ref_operators(self.cx, arg);
+        let map = self.cx.tcx.hir();
+        // Is the reference self?
+        if path_to_local(reference).map(|x| map.name(x)) == Some(kw::SelfLower) {
+            let FormatTraitNames { name, .. } = self.format_trait_impl;
+            span_lint(
+                self.cx,
+                RECURSIVE_FORMAT_IMPL,
+                self.expr.span,
+                &format!("using `self` as `{name}` in `impl {name}` will cause infinite recursion"),
+            );
+        }
     }
-}
 
-fn check_print_in_format_impl(cx: &LateContext<'_>, expr: &Expr<'_>, impl_trait: FormatTraitNames) {
-    if let Some(macro_call) = root_macro_call_first_node(cx, expr)
-        && let Some(name) = cx.tcx.get_diagnostic_name(macro_call.def_id)
-    {
-        let replacement = match name {
-            sym::print_macro | sym::eprint_macro => "write",
-            sym::println_macro | sym::eprintln_macro => "writeln",
-            _ => return,
-        };
+    fn check_print_in_format_impl(&self) {
+        if let Some(macro_call) = root_macro_call_first_node(self.cx, self.expr)
+            && let Some(name) = self.cx.tcx.get_diagnostic_name(macro_call.def_id)
+        {
+            let replacement = match name {
+                sym::print_macro | sym::eprint_macro => "write",
+                sym::println_macro | sym::eprintln_macro => "writeln",
+                _ => return,
+            };
 
-        let name = name.as_str().strip_suffix("_macro").unwrap();
+            let name = name.as_str().strip_suffix("_macro").unwrap();
 
-        span_lint_and_sugg(
-            cx,
-            PRINT_IN_FORMAT_IMPL,
-            macro_call.span,
-            &format!("use of `{name}!` in `{}` impl", impl_trait.name),
-            "replace with",
-            if let Some(formatter_name) = impl_trait.formatter_name {
-                format!("{replacement}!({formatter_name}, ..)")
-            } else {
-                format!("{replacement}!(..)")
-            },
-            Applicability::HasPlaceholders,
-        );
+            span_lint_and_sugg(
+                self.cx,
+                PRINT_IN_FORMAT_IMPL,
+                macro_call.span,
+                &format!("use of `{name}!` in `{}` impl", self.format_trait_impl.name),
+                "replace with",
+                if let Some(formatter_name) = self.format_trait_impl.formatter_name {
+                    format!("{replacement}!({formatter_name}, ..)")
+                } else {
+                    format!("{replacement}!(..)")
+                },
+                Applicability::HasPlaceholders,
+            );
+        }
     }
 }