add tostring_impl lint

Author: andrewbanchich

CHANGELOG.md

@@ -5625,6 +5625,7 @@ Released 2018-09-13
 [`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
 [`toplevel_ref_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#toplevel_ref_arg
+[`tostring_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#tostring_impl
 [`trailing_empty_array`]: https://rust-lang.github.io/rust-clippy/master/index.html#trailing_empty_array
 [`trait_duplication_in_bounds`]: https://rust-lang.github.io/rust-clippy/master/index.html#trait_duplication_in_bounds
 [`transmute_bytes_to_str`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmute_bytes_to_str

clippy_lints/src/declared_lints.rs

@@ -655,6 +655,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::tests_outside_test_module::TESTS_OUTSIDE_TEST_MODULE_INFO,
     crate::thread_local_initializer_can_be_made_const::THREAD_LOCAL_INITIALIZER_CAN_BE_MADE_CONST_INFO,
     crate::to_digit_is_some::TO_DIGIT_IS_SOME_INFO,
+    crate::tostring_impl::TOSTRING_IMPL_INFO,
     crate::trailing_empty_array::TRAILING_EMPTY_ARRAY_INFO,
     crate::trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS_INFO,
     crate::trait_bounds::TYPE_REPETITION_IN_BOUNDS_INFO,

clippy_lints/src/lib.rs

@@ -325,6 +325,7 @@ mod temporary_assignment;
 mod tests_outside_test_module;
 mod thread_local_initializer_can_be_made_const;
 mod to_digit_is_some;
+mod tostring_impl;
 mod trailing_empty_array;
 mod trait_bounds;
 mod transmute;
@@ -1092,6 +1093,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(move |_| {
         Box::new(thread_local_initializer_can_be_made_const::ThreadLocalInitializerCanBeMadeConst::new(msrv()))
     });
+    store.register_late_pass(|_| Box::new(tostring_impl::TostringImpl));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

clippy_lints/src/tostring_impl.rs

@@ -0,0 +1,57 @@
+use clippy_utils::diagnostics::span_lint_and_help;
+use rustc_hir::*;
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::declare_lint_pass;
+use rustc_span::sym;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for direct implementations of `ToString`.
+    /// ### Why is this bad?
+    /// This trait is automatically implemented for any type which implements the `Display` trait.
+    /// As such, `ToString` shouldn’t be implemented directly: `Display` should be implemented instead,
+    /// and you get the `ToString` implementation for free.
+    /// ### Example
+    /// ```no_run
+    /// impl ToString for Point {
+    ///   fn to_string(&self) -> String {
+    ///     format!("({}, {})", self.x, self.y)
+    ///   }
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// impl fmt::Display for Point {
+    ///   fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+    ///     write!(f, "({}, {})", self.x, self.y)
+    ///   }
+    /// }
+    /// ```
+    #[clippy::version = "1.77.0"]
+    pub TOSTRING_IMPL,
+    style,
+    "default lint description"
+}
+
+declare_lint_pass!(TostringImpl => [TOSTRING_IMPL]);
+
+impl<'tcx> LateLintPass<'tcx> for TostringImpl {
+    fn check_item(&mut self, cx: &LateContext<'tcx>, it: &'tcx Item<'tcx>) {
+        if let ItemKind::Impl(Impl {
+            of_trait: Some(trait_ref),
+            ..
+        }) = it.kind
+            && let Some(trait_did) = trait_ref.trait_def_id()
+            && cx.tcx.is_diagnostic_item(sym::ToString, trait_did)
+        {
+            span_lint_and_help(
+                cx,
+                TOSTRING_IMPL,
+                it.span,
+                "direct implementation of `ToString`",
+                None,
+                "prefer implementing `Display` instead",
+            );
+        }
+    }
+}

tests/ui/format_args.rs

@@ -14,6 +14,7 @@ use std::panic::Location;
 
 struct Somewhere;
 
+#[allow(tostring_impl)]
 impl ToString for Somewhere {
     fn to_string(&self) -> String {
         String::from("somewhere")

tests/ui/tostring_impl.rs

@@ -0,0 +1,31 @@
+#![warn(clippy::tostring_impl)]
+
+use std::fmt::{self, Display};
+
+struct Point {
+    x: usize,
+    y: usize,
+}
+
+impl ToString for Point {
+    fn to_string(&self) -> String {
+        format!("({}, {})", self.x, self.y)
+    }
+}
+
+struct Foo;
+
+impl Display for Foo {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "Foo")
+    }
+}
+
+struct Bar;
+
+impl Bar {
+    #[allow(clippy::inherent_to_string)]
+    fn to_string(&self) -> String {
+        String::from("Bar")
+    }
+}

tests/ui/tostring_impl.stderr

@@ -0,0 +1,16 @@
+error: direct implementation of `ToString`
+  --> $DIR/tostring_impl.rs:10:1
+   |
+LL | / impl ToString for Point {
+LL | |     fn to_string(&self) -> String {
+LL | |         format!("({}, {})", self.x, self.y)
+LL | |     }
+LL | | }
+   | |_^
+   |
+   = help: prefer implementing `Display` instead
+   = note: `-D clippy::tostring-impl` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::tostring_impl)]`
+
+error: aborting due to 1 previous error
+

tests/ui/unconditional_recursion.rs

@@ -206,6 +206,7 @@ impl PartialEq for S8 {
 
 struct S9;
 
+#[allow(tostring_impl)]
 impl std::string::ToString for S9 {
     fn to_string(&self) -> String {
         //~^ ERROR: function cannot return without recursing
@@ -215,6 +216,7 @@ impl std::string::ToString for S9 {
 
 struct S10;
 
+#[allow(tostring_impl)]
 impl std::string::ToString for S10 {
     fn to_string(&self) -> String {
         //~^ ERROR: function cannot return without recursing
@@ -225,6 +227,7 @@ impl std::string::ToString for S10 {
 
 struct S11;
 
+#[allow(tostring_impl)]
 impl std::string::ToString for S11 {
     fn to_string(&self) -> String {
         //~^ ERROR: function cannot return without recursing

tests/ui/unnecessary_to_owned.rs

@@ -27,6 +27,7 @@ impl AsRef<str> for X {
     }
 }
 
+#[allow(tostring_impl)]
 impl ToString for X {
     fn to_string(&self) -> String {
         self.0.to_string()
@@ -265,6 +266,7 @@ mod issue_8507 {
         }
     }
 
+    #[allow(tostring_impl)]
     impl ToString for Y {
         fn to_string(&self) -> String {
             self.0.to_string()
@@ -338,6 +340,7 @@ mod issue_9317 {
 
     struct Bytes {}
 
+    #[allow(tostring_impl)]
     impl ToString for Bytes {
         fn to_string(&self) -> String {
             "123".to_string()

tests/ui/unnecessary_to_owned_on_split.rs

@@ -8,6 +8,7 @@ impl AsRef<str> for Issue12068 {
     }
 }
 
+#[allow(tostring_impl)]
 impl ToString for Issue12068 {
     fn to_string(&self) -> String {
         String::new()