Auto merge of #5831 - chansuke:to_string_in_display, r=flip1995

Don't use `to_string` in impl Display

fixes #3876

this PR is derived from [Toxyxer's implementation](#5574).
changelog: add [`to_string_in_display`] lint

Author: bors

CHANGELOG.md

@@ -1723,6 +1723,7 @@ Released 2018-09-13
 [`temporary_assignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_assignment
 [`temporary_cstring_as_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_cstring_as_ptr
 [`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
 [`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
 [`too_many_lines`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_lines

clippy_lints/src/lib.rs

@@ -296,6 +296,7 @@ mod swap;
 mod tabs_in_doc_comments;
 mod temporary_assignment;
 mod to_digit_is_some;
+mod to_string_in_display;
 mod trait_bounds;
 mod transmute;
 mod transmuting_null;
@@ -788,6 +789,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &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,
         &trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS,
         &trait_bounds::TYPE_REPETITION_IN_BOUNDS,
         &transmute::CROSSPOINTER_TRANSMUTE,
@@ -1017,6 +1019,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_early_pass(|| box reference::DerefAddrOf);
     store.register_early_pass(|| box reference::RefInDeref);
     store.register_early_pass(|| box double_parens::DoubleParens);
+    store.register_late_pass(|| box to_string_in_display::ToStringInDisplay::new());
     store.register_early_pass(|| box unsafe_removed_from_name::UnsafeNameRemoval);
     store.register_early_pass(|| box if_not_else::IfNotElse);
     store.register_early_pass(|| box else_if_without_else::ElseIfWithoutElse);
@@ -1427,6 +1430,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         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),
@@ -1708,6 +1712,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL),
         LintId::of(&suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL),
         LintId::of(&swap::ALMOST_SWAPPED),
+        LintId::of(&to_string_in_display::TO_STRING_IN_DISPLAY),
         LintId::of(&transmute::UNSOUND_COLLECTION_TRANSMUTE),
         LintId::of(&transmute::WRONG_TRANSMUTE),
         LintId::of(&transmuting_null::TRANSMUTING_NULL),

clippy_lints/src/to_string_in_display.rs

@@ -0,0 +1,100 @@
+use crate::utils::{match_def_path, match_trait_method, paths, span_lint};
+use if_chain::if_chain;
+use rustc_hir::{Expr, ExprKind, Item, ItemKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_tool_lint, impl_lint_pass};
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for uses of `to_string()` in `Display` traits.
+    ///
+    /// **Why is this bad?** Usually `to_string` is implemented indirectly
+    /// via `Display`. Hence using it while implementing `Display` would
+    /// lead to infinite recursion.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **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)
+    ///     }
+    /// }
+    /// ```
+    pub TO_STRING_IN_DISPLAY,
+    correctness,
+    "to_string method used while implementing Display trait"
+}
+
+#[derive(Default)]
+pub struct ToStringInDisplay {
+    in_display_impl: bool,
+}
+
+impl ToStringInDisplay {
+    pub fn new() -> Self {
+        Self { in_display_impl: false }
+    }
+}
+
+impl_lint_pass!(ToStringInDisplay => [TO_STRING_IN_DISPLAY]);
+
+impl LateLintPass<'_> for ToStringInDisplay {
+    fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
+        if is_display_impl(cx, item) {
+            self.in_display_impl = true;
+        }
+    }
+
+    fn check_item_post(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
+        if is_display_impl(cx, item) {
+            self.in_display_impl = false;
+        }
+    }
+
+    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
+        if_chain! {
+            if let ExprKind::MethodCall(ref path, _, _, _) = expr.kind;
+            if path.ident.name == sym!(to_string);
+            if match_trait_method(cx, expr, &paths::TO_STRING);
+            if self.in_display_impl;
+
+            then {
+                span_lint(
+                    cx,
+                    TO_STRING_IN_DISPLAY,
+                    expr.span,
+                    "Using to_string in fmt::Display implementation might lead to infinite recursion",
+                );
+            }
+        }
+    }
+}
+
+fn is_display_impl(cx: &LateContext<'_>, item: &Item<'_>) -> bool {
+    if_chain! {
+        if let ItemKind::Impl { of_trait: Some(trait_ref), .. } = &item.kind;
+        if let Some(did) = trait_ref.trait_def_id();
+        then {
+            match_def_path(cx, did, &paths::DISPLAY_TRAIT)
+        } else {
+            false
+        }
+    }
+}

src/lintlist/mod.rs

@@ -2166,6 +2166,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         module: "to_digit_is_some",
     },
+    Lint {
+        name: "to_string_in_display",
+        group: "correctness",
+        desc: "to_string method used while implementing Display trait",
+        deprecation: None,
+        module: "to_string_in_display",
+    },
     Lint {
         name: "todo",
         group: "restriction",

tests/ui/to_string_in_display.rs

@@ -0,0 +1,55 @@
+#![warn(clippy::to_string_in_display)]
+#![allow(clippy::inherent_to_string_shadow_display)]
+
+use std::fmt;
+
+struct A;
+impl A {
+    fn fmt(&self) {
+        self.to_string();
+    }
+}
+
+trait B {
+    fn fmt(&self) {}
+}
+
+impl B for A {
+    fn fmt(&self) {
+        self.to_string();
+    }
+}
+
+impl fmt::Display for A {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{}", self.to_string())
+    }
+}
+
+fn fmt(a: A) {
+    a.to_string();
+}
+
+struct C;
+
+impl C {
+    fn to_string(&self) -> String {
+        String::from("I am C")
+    }
+}
+
+impl fmt::Display for C {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{}", self.to_string())
+    }
+}
+
+fn main() {
+    let a = A;
+    a.to_string();
+    a.fmt();
+    fmt(a);
+
+    let c = C;
+    c.to_string();
+}

tests/ui/to_string_in_display.stderr

@@ -0,0 +1,10 @@
+error: Using to_string in fmt::Display implementation might lead to infinite recursion
+  --> $DIR/to_string_in_display.rs:25:25
+   |
+LL |         write!(f, "{}", self.to_string())
+   |                         ^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::to-string-in-display` implied by `-D warnings`
+
+error: aborting due to previous error
+