Add recursive_format_impl lint

The to_string_in_display lint is renamed to recursive_format_impl
A check is added for the use of self formatted with Display or Debug
inside any format string in the same impl
The to_string_in_display check is kept as is - like in the
format_in_format_args lint

For now only Display and Debug are checked
This could also be extended to other Format traits (Binary, etc.)

Author: jamesmcm

CHANGELOG.md

@@ -1438,7 +1438,7 @@ Released 2020-11-19
 * [`manual_strip`] [#6038](https://github.com/rust-lang/rust-clippy/pull/6038)
 * [`map_err_ignore`] [#5998](https://github.com/rust-lang/rust-clippy/pull/5998)
 * [`rc_buffer`] [#6044](https://github.com/rust-lang/rust-clippy/pull/6044)
-* [`to_string_in_display`] [#5831](https://github.com/rust-lang/rust-clippy/pull/5831)
+* `to_string_in_display` [#5831](https://github.com/rust-lang/rust-clippy/pull/5831)
 * `single_char_push_str` [#5881](https://github.com/rust-lang/rust-clippy/pull/5881)
 
 ### Moves and Deprecations
@@ -1481,7 +1481,7 @@ Released 2020-11-19
   [#5949](https://github.com/rust-lang/rust-clippy/pull/5949)
 * [`doc_markdown`]: allow using "GraphQL" without backticks
   [#5996](https://github.com/rust-lang/rust-clippy/pull/5996)
-* [`to_string_in_display`]: avoid linting when calling `to_string()` on anything that is not `self`
+* `to_string_in_display`: avoid linting when calling `to_string()` on anything that is not `self`
   [#5971](https://github.com/rust-lang/rust-clippy/pull/5971)
 * [`indexing_slicing`] and [`out_of_bounds_indexing`] treat references to arrays as arrays
   [#6034](https://github.com/rust-lang/rust-clippy/pull/6034)
@@ -3385,6 +3385,7 @@ Released 2018-09-13
 [`range_zip_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_zip_with_len
 [`rc_buffer`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_buffer
 [`rc_mutex`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_mutex
+[`recursive_format_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#recursive_format_impl
 [`redundant_allocation`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_allocation
 [`redundant_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_clone
 [`redundant_closure`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure
@@ -3459,7 +3460,6 @@ Released 2018-09-13
 [`tabs_in_doc_comments`]: https://rust-lang.github.io/rust-clippy/master/index.html#tabs_in_doc_comments
 [`temporary_assignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_assignment
 [`to_digit_is_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_digit_is_some
-[`to_string_in_display`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_display
 [`to_string_in_format_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_format_args
 [`todo`]: https://rust-lang.github.io/rust-clippy/master/index.html#todo
 [`too_many_arguments`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments

clippy_lints/src/format_args.rs

@@ -1,8 +1,8 @@
 use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
-use clippy_utils::macros::{FormatArgsArg, FormatArgsExpn};
+use clippy_utils::is_diag_trait_item;
+use clippy_utils::macros::{is_format_macro, FormatArgsArg, FormatArgsExpn};
 use clippy_utils::source::snippet_opt;
 use clippy_utils::ty::implements_trait;
-use clippy_utils::{is_diag_trait_item, match_def_path, paths};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::{Expr, ExprKind};
@@ -65,34 +65,14 @@ declare_clippy_lint! {
 
 declare_lint_pass!(FormatArgs => [FORMAT_IN_FORMAT_ARGS, TO_STRING_IN_FORMAT_ARGS]);
 
-const FORMAT_MACRO_PATHS: &[&[&str]] = &[
-    &paths::FORMAT_ARGS_MACRO,
-    &paths::ASSERT_EQ_MACRO,
-    &paths::ASSERT_MACRO,
-    &paths::ASSERT_NE_MACRO,
-    &paths::EPRINT_MACRO,
-    &paths::EPRINTLN_MACRO,
-    &paths::PRINT_MACRO,
-    &paths::PRINTLN_MACRO,
-    &paths::WRITE_MACRO,
-    &paths::WRITELN_MACRO,
-];
-
-const FORMAT_MACRO_DIAG_ITEMS: &[Symbol] = &[sym::format_macro, sym::std_panic_macro];
-
 impl<'tcx> LateLintPass<'tcx> for FormatArgs {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
         if_chain! {
             if let Some(format_args) = FormatArgsExpn::parse(cx, expr);
             let expr_expn_data = expr.span.ctxt().outer_expn_data();
             let outermost_expn_data = outermost_expn_data(expr_expn_data);
             if let Some(macro_def_id) = outermost_expn_data.macro_def_id;
-            if FORMAT_MACRO_PATHS
-                .iter()
-                .any(|path| match_def_path(cx, macro_def_id, path))
-                || FORMAT_MACRO_DIAG_ITEMS
-                    .iter()
-                    .any(|diag_item| cx.tcx.is_diagnostic_item(*diag_item, macro_def_id));
+            if is_format_macro(cx, macro_def_id);
             if let ExpnKind::Macro(_, name) = outermost_expn_data.kind;
             if let Some(args) = format_args.args();
             then {

clippy_lints/src/lib.register_all.rs

@@ -241,6 +241,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(ranges::MANUAL_RANGE_CONTAINS),
     LintId::of(ranges::RANGE_ZIP_WITH_LEN),
     LintId::of(ranges::REVERSED_EMPTY_RANGES),
+    LintId::of(recursive_format_impl::RECURSIVE_FORMAT_IMPL),
     LintId::of(redundant_clone::REDUNDANT_CLONE),
     LintId::of(redundant_closure_call::REDUNDANT_CLOSURE_CALL),
     LintId::of(redundant_field_names::REDUNDANT_FIELD_NAMES),
@@ -267,7 +268,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(tabs_in_doc_comments::TABS_IN_DOC_COMMENTS),
     LintId::of(temporary_assignment::TEMPORARY_ASSIGNMENT),
     LintId::of(to_digit_is_some::TO_DIGIT_IS_SOME),
-    LintId::of(to_string_in_display::TO_STRING_IN_DISPLAY),
     LintId::of(transmute::CROSSPOINTER_TRANSMUTE),
     LintId::of(transmute::TRANSMUTES_EXPRESSIBLE_AS_PTR_CASTS),
     LintId::of(transmute::TRANSMUTE_BYTES_TO_STR),

clippy_lints/src/lib.register_correctness.rs

@@ -52,12 +52,12 @@ store.register_group(true, "clippy::correctness", Some("clippy_correctness"), ve
     LintId::of(ptr::INVALID_NULL_PTR_USAGE),
     LintId::of(ptr::MUT_FROM_REF),
     LintId::of(ranges::REVERSED_EMPTY_RANGES),
+    LintId::of(recursive_format_impl::RECURSIVE_FORMAT_IMPL),
     LintId::of(regex::INVALID_REGEX),
     LintId::of(self_assignment::SELF_ASSIGNMENT),
     LintId::of(serde_api::SERDE_API_MISUSE),
     LintId::of(size_of_in_element_count::SIZE_OF_IN_ELEMENT_COUNT),
     LintId::of(swap::ALMOST_SWAPPED),
-    LintId::of(to_string_in_display::TO_STRING_IN_DISPLAY),
     LintId::of(transmute::TRANSMUTE_UNDEFINED_REPR),
     LintId::of(transmute::UNSOUND_COLLECTION_TRANSMUTE),
     LintId::of(transmute::WRONG_TRANSMUTE),

clippy_lints/src/lib.register_lints.rs

@@ -416,6 +416,7 @@ store.register_lints(&[
     ranges::RANGE_PLUS_ONE,
     ranges::RANGE_ZIP_WITH_LEN,
     ranges::REVERSED_EMPTY_RANGES,
+    recursive_format_impl::RECURSIVE_FORMAT_IMPL,
     redundant_clone::REDUNDANT_CLONE,
     redundant_closure_call::REDUNDANT_CLOSURE_CALL,
     redundant_else::REDUNDANT_ELSE,
@@ -460,7 +461,6 @@ store.register_lints(&[
     tabs_in_doc_comments::TABS_IN_DOC_COMMENTS,
     temporary_assignment::TEMPORARY_ASSIGNMENT,
     to_digit_is_some::TO_DIGIT_IS_SOME,
-    to_string_in_display::TO_STRING_IN_DISPLAY,
     trailing_empty_array::TRAILING_EMPTY_ARRAY,
     trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS,
     trait_bounds::TYPE_REPETITION_IN_BOUNDS,

clippy_lints/src/lib.rs

@@ -332,6 +332,7 @@ mod ptr_eq;
 mod ptr_offset_with_cast;
 mod question_mark;
 mod ranges;
+mod recursive_format_impl;
 mod redundant_clone;
 mod redundant_closure_call;
 mod redundant_else;
@@ -364,7 +365,6 @@ mod swap;
 mod tabs_in_doc_comments;
 mod temporary_assignment;
 mod to_digit_is_some;
-mod to_string_in_display;
 mod trailing_empty_array;
 mod trait_bounds;
 mod transmute;
@@ -704,7 +704,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(modulo_arithmetic::ModuloArithmetic));
     store.register_early_pass(|| Box::new(reference::DerefAddrOf));
     store.register_early_pass(|| Box::new(double_parens::DoubleParens));
-    store.register_late_pass(|| Box::new(to_string_in_display::ToStringInDisplay::new()));
+    store.register_late_pass(|| Box::new(recursive_format_impl::RecursiveFormatImpl::new()));
     store.register_early_pass(|| Box::new(unsafe_removed_from_name::UnsafeNameRemoval));
     store.register_early_pass(|| Box::new(else_if_without_else::ElseIfWithoutElse));
     store.register_early_pass(|| Box::new(int_plus_one::IntPlusOne));
@@ -938,6 +938,7 @@ pub fn register_renamed(ls: &mut rustc_lint::LintStore) {
     ls.register_renamed("clippy::disallowed_type", "clippy::disallowed_types");
     ls.register_renamed("clippy::disallowed_method", "clippy::disallowed_methods");
     ls.register_renamed("clippy::ref_in_deref", "clippy::needless_borrow");
+    ls.register_renamed("clippy::to_string_in_display", "clippy::recursive_format_impl");
 
     // uplifted lints
     ls.register_renamed("clippy::invalid_ref", "invalid_value");

clippy_lints/src/recursive_format_impl.rs

@@ -0,0 +1,190 @@
+use clippy_utils::diagnostics::span_lint;
+use clippy_utils::macros::{is_format_macro, root_macro_call_first_node, FormatArgsArg, FormatArgsExpn};
+use clippy_utils::{is_diag_trait_item, path_to_local, peel_ref_operators};
+use if_chain::if_chain;
+use rustc_hir::{Expr, ExprKind, Impl, Item, ItemKind, QPath};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_tool_lint, impl_lint_pass};
+use rustc_span::{sym, symbol::kw, Symbol};
+
+#[derive(Clone, Copy)]
+enum FormatTrait {
+    Debug,
+    Display,
+}
+
+impl FormatTrait {
+    fn name(self) -> Symbol {
+        match self {
+            FormatTrait::Debug => sym::Debug,
+            FormatTrait::Display => sym::Display,
+        }
+    }
+}
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for format trait implementations (e.g. `Display`) with a recursive call to itself
+    /// which uses `self` as a parameter.
+    /// This is typically done indirectly with the `write!` macro or with `to_string()`.
+    ///
+    /// ### Why is this bad?
+    /// This will lead to infinite recursion and a stack overflow.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// use std::fmt;
+    ///
+    /// struct Structure(i32);
+    /// impl fmt::Display for Structure {
+    ///     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+    ///         write!(f, "{}", self.to_string())
+    ///     }
+    /// }
+    ///
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// use std::fmt;
+    ///
+    /// struct Structure(i32);
+    /// impl fmt::Display for Structure {
+    ///     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+    ///         write!(f, "{}", self.0)
+    ///     }
+    /// }
+    /// ```
+    #[clippy::version = "1.48.0"]
+    pub RECURSIVE_FORMAT_IMPL,
+    correctness,
+    "Format trait method called while implementing the same Format trait"
+}
+
+#[derive(Default)]
+pub struct RecursiveFormatImpl {
+    // Whether we are inside Display or Debug trait impl - None for neither
+    format_trait_impl: Option<FormatTrait>,
+}
+
+impl RecursiveFormatImpl {
+    pub fn new() -> Self {
+        Self {
+            format_trait_impl: None,
+        }
+    }
+}
+
+impl_lint_pass!(RecursiveFormatImpl => [RECURSIVE_FORMAT_IMPL]);
+
+impl<'tcx> LateLintPass<'tcx> for RecursiveFormatImpl {
+    fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
+        if let Some(format_trait_impl) = is_format_trait_impl(cx, item) {
+            self.format_trait_impl = Some(format_trait_impl);
+        }
+    }
+
+    fn check_item_post(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
+        // Assume no nested Impl of Debug and Display within eachother
+        if is_format_trait_impl(cx, item).is_some() {
+            self.format_trait_impl = None;
+        }
+    }
+
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+        match self.format_trait_impl {
+            Some(FormatTrait::Display) => {
+                check_to_string_in_display(cx, expr);
+                check_self_in_format_args(cx, expr, FormatTrait::Display);
+            },
+            Some(FormatTrait::Debug) => {
+                check_self_in_format_args(cx, expr, FormatTrait::Debug);
+            },
+            None => {},
+        }
+    }
+}
+
+fn check_to_string_in_display(cx: &LateContext<'_>, expr: &Expr<'_>) {
+    if_chain! {
+        // Get the hir_id of the object we are calling the method on
+        if let ExprKind::MethodCall(path, [ref self_arg, ..], _) = expr.kind;
+        // Is the method to_string() ?
+        if path.ident.name == sym!(to_string);
+        // Is the method a part of the ToString trait? (i.e. not to_string() implemented
+        // separately)
+        if let Some(expr_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id);
+        if is_diag_trait_item(cx, expr_def_id, sym::ToString);
+        // Is the method is called on self
+        if let ExprKind::Path(QPath::Resolved(_, path)) = self_arg.kind;
+        if let [segment] = path.segments;
+        if segment.ident.name == kw::SelfLower;
+        then {
+            span_lint(
+                cx,
+                RECURSIVE_FORMAT_IMPL,
+                expr.span,
+                "using `self.to_string` in `fmt::Display` implementation will cause infinite recursion",
+            );
+        }
+    }
+}
+
+fn check_self_in_format_args<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, impl_trait: FormatTrait) {
+    // Check each arg in format calls - do we ever use Display on self (directly or via deref)?
+    if_chain! {
+        if let Some(outer_macro) = root_macro_call_first_node(cx, expr);
+        if let macro_def_id = outer_macro.def_id;
+        if let Some(format_args) = FormatArgsExpn::find_nested(cx, expr, outer_macro.expn);
+        if is_format_macro(cx, macro_def_id);
+        if let Some(args) = format_args.args();
+        then {
+            for arg in args {
+                if arg.format_trait != impl_trait.name() {
+                    continue;
+                }
+                check_format_arg_self(cx, expr, &arg, impl_trait);
+            }
+        }
+    }
+}
+
+fn check_format_arg_self(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &FormatArgsArg<'_>, impl_trait: FormatTrait) {
+    // 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.value);
+    let map = cx.tcx.hir();
+    // Is the reference self?
+    let symbol_ident = impl_trait.name().to_ident_string();
+    if path_to_local(reference).map(|x| map.name(x)) == Some(kw::SelfLower) {
+        span_lint(
+            cx,
+            RECURSIVE_FORMAT_IMPL,
+            expr.span,
+            &format!(
+                "using `self` as `{}` in `impl {}` will cause infinite recursion",
+                &symbol_ident, &symbol_ident
+            ),
+        );
+    }
+}
+
+fn is_format_trait_impl(cx: &LateContext<'_>, item: &Item<'_>) -> Option<FormatTrait> {
+    if_chain! {
+        // Are we at an Impl?
+        if let ItemKind::Impl(Impl { of_trait: Some(trait_ref), .. }) = &item.kind;
+        if let Some(did) = trait_ref.trait_def_id();
+        if let Some(name) = cx.tcx.get_diagnostic_name(did);
+        then {
+            // Is Impl for Debug or Display?
+            match name {
+                sym::Debug => Some(FormatTrait::Debug),
+                sym::Display => Some(FormatTrait::Display),
+                _ => None,
+            }
+        } else {
+            None
+        }
+    }
+}