Auto merge of #5616 - euclio:no-derive-suggestion, r=phansch,flip1995

new_without_default: do not suggest deriving

---

changelog: do not suggest deriving `Default` in `new_without_default`

This commit changes the behavior of the `new_without_default` lint to not suggest deriving `Default`. This suggestion is misleading if the `new` implementation does something different than what a derived `Default` implementation would do, because then the two methods would not be equivalent.

Instead, the `can_derive_default` check is removed, and we always suggest implementing `Default` in terms of `new()`.

3 files changed, 64 insertions, 100 deletions

diff --git a/clippy_lints/src/new_without_default.rs b/clippy_lints/src/new_without_default.rs
index a599667b8d8..3b88e4c4cb1 100644
--- a/clippy_lints/src/new_without_default.rs
+++ b/clippy_lints/src/new_without_default.rs
@@ -1,27 +1,20 @@
 use crate::utils::paths;
 use crate::utils::sugg::DiagnosticBuilderExt;
-use crate::utils::{get_trait_def_id, implements_trait, return_ty, same_tys, span_lint_hir_and_then};
+use crate::utils::{get_trait_def_id, return_ty, same_tys, span_lint_hir_and_then};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
-use rustc_hir::def_id::DefId;
 use rustc_hir::HirIdSet;
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
-use rustc_middle::ty::{self, Ty};
+use rustc_middle::ty::Ty;
 use rustc_session::{declare_tool_lint, impl_lint_pass};
-use rustc_span::source_map::Span;
 
 declare_clippy_lint! {
     /// **What it does:** Checks for types with a `fn new() -> Self` method and no
     /// implementation of
     /// [`Default`](https://doc.rust-lang.org/std/default/trait.Default.html).
     ///
-    /// It detects both the case when a manual
-    /// [`Default`](https://doc.rust-lang.org/std/default/trait.Default.html)
-    /// implementation is required and also when it can be created with
-    /// `#[derive(Default)]`
-    ///
     /// **Why is this bad?** The user might expect to be able to use
     /// [`Default`](https://doc.rust-lang.org/std/default/trait.Default.html) as the
     /// type can be constructed without arguments.
@@ -40,46 +33,17 @@ declare_clippy_lint! {
     /// }
     /// ```
     ///
-    /// Instead, use:
+    /// To fix the lint, and a `Default` implementation that delegates to `new`:
     ///
     /// ```ignore
     /// struct Foo(Bar);
     ///
     /// impl Default for Foo {
     ///     fn default() -> Self {
-    ///         Foo(Bar::new())
+    ///         Foo::new()
     ///     }
     /// }
     /// ```
-    ///
-    /// Or, if
-    /// [`Default`](https://doc.rust-lang.org/std/default/trait.Default.html)
-    /// can be derived by `#[derive(Default)]`:
-    ///
-    /// ```rust
-    /// struct Foo;
-    ///
-    /// impl Foo {
-    ///     fn new() -> Self {
-    ///         Foo
-    ///     }
-    /// }
-    /// ```
-    ///
-    /// Instead, use:
-    ///
-    /// ```rust
-    /// #[derive(Default)]
-    /// struct Foo;
-    ///
-    /// impl Foo {
-    ///     fn new() -> Self {
-    ///         Foo
-    ///     }
-    /// }
-    /// ```
-    ///
-    /// You can also have `new()` call `Default::default()`.
     pub NEW_WITHOUT_DEFAULT,
     style,
     "`fn new() -> Self` method without `Default` implementation"
@@ -158,46 +122,25 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NewWithoutDefault {
                                         }
                                     }
 
-                                    if let Some(sp) = can_derive_default(self_ty, cx, default_trait_id) {
-                                        span_lint_hir_and_then(
-                                            cx,
-                                            NEW_WITHOUT_DEFAULT,
-                                            id,
-                                            impl_item.span,
-                                            &format!(
-                                                "you should consider deriving a `Default` implementation for `{}`",
-                                                self_ty
-                                            ),
-                                            |diag| {
-                                                diag.suggest_item_with_attr(
-                                                    cx,
-                                                    sp,
-                                                    "try this",
-                                                    "#[derive(Default)]",
-                                                    Applicability::MaybeIncorrect,
-                                                );
-                                            });
-                                    } else {
-                                        span_lint_hir_and_then(
-                                            cx,
-                                            NEW_WITHOUT_DEFAULT,
-                                            id,
-                                            impl_item.span,
-                                            &format!(
-                                                "you should consider adding a `Default` implementation for `{}`",
-                                                self_ty
-                                            ),
-                                            |diag| {
-                                                diag.suggest_prepend_item(
-                                                    cx,
-                                                    item.span,
-                                                    "try this",
-                                                    &create_new_without_default_suggest_msg(self_ty),
-                                                    Applicability::MaybeIncorrect,
-                                                );
-                                            },
-                                        );
-                                    }
+                                    span_lint_hir_and_then(
+                                        cx,
+                                        NEW_WITHOUT_DEFAULT,
+                                        id,
+                                        impl_item.span,
+                                        &format!(
+                                            "you should consider adding a `Default` implementation for `{}`",
+                                            self_ty
+                                        ),
+                                        |diag| {
+                                            diag.suggest_prepend_item(
+                                                cx,
+                                                item.span,
+                                                "try this",
+                                                &create_new_without_default_suggest_msg(self_ty),
+                                                Applicability::MaybeIncorrect,
+                                            );
+                                        },
+                                    );
                                 }
                             }
                         }
@@ -217,18 +160,3 @@ fn create_new_without_default_suggest_msg(ty: Ty<'_>) -> String {
     }}
 }}", ty)
 }
-
-fn can_derive_default<'t, 'c>(ty: Ty<'t>, cx: &LateContext<'c, 't>, default_trait_id: DefId) -> Option<Span> {
-    match ty.kind {
-        ty::Adt(adt_def, substs) if adt_def.is_struct() => {
-            for field in adt_def.all_fields() {
-                let f_ty = field.ty(cx.tcx, substs);
-                if !implements_trait(cx, f_ty, default_trait_id, &[]) {
-                    return None;
-                }
-            }
-            Some(cx.tcx.def_span(adt_def.did))
-        },
-        _ => None,
-    }
-}
diff --git a/tests/ui/new_without_default.rs b/tests/ui/new_without_default.rs
index 781ea7bb152..3b6041823d8 100644
--- a/tests/ui/new_without_default.rs
+++ b/tests/ui/new_without_default.rs
@@ -148,4 +148,15 @@ impl AllowDerive {
     }
 }
 
+pub struct NewNotEqualToDerive {
+    foo: i32,
+}
+
+impl NewNotEqualToDerive {
+    // This `new` implementation is not equal to a derived `Default`, so do not suggest deriving.
+    pub fn new() -> Self {
+        NewNotEqualToDerive { foo: 1 }
+    }
+}
+
 fn main() {}
diff --git a/tests/ui/new_without_default.stderr b/tests/ui/new_without_default.stderr
index 5e485d40663..e529e441eb7 100644
--- a/tests/ui/new_without_default.stderr
+++ b/tests/ui/new_without_default.stderr
@@ -1,4 +1,4 @@
-error: you should consider deriving a `Default` implementation for `Foo`
+error: you should consider adding a `Default` implementation for `Foo`
   --> $DIR/new_without_default.rs:8:5
    |
 LL | /     pub fn new() -> Foo {
@@ -9,10 +9,14 @@ LL | |     }
    = note: `-D clippy::new-without-default` implied by `-D warnings`
 help: try this
    |
-LL | #[derive(Default)]
+LL | impl Default for Foo {
+LL |     fn default() -> Self {
+LL |         Self::new()
+LL |     }
+LL | }
    |
 
-error: you should consider deriving a `Default` implementation for `Bar`
+error: you should consider adding a `Default` implementation for `Bar`
   --> $DIR/new_without_default.rs:16:5
    |
 LL | /     pub fn new() -> Self {
@@ -22,7 +26,11 @@ LL | |     }
    |
 help: try this
    |
-LL | #[derive(Default)]
+LL | impl Default for Bar {
+LL |     fn default() -> Self {
+LL |         Self::new()
+LL |     }
+LL | }
    |
 
 error: you should consider adding a `Default` implementation for `LtKo<'c>`
@@ -42,5 +50,22 @@ LL |     }
 LL | }
    |
 
-error: aborting due to 3 previous errors
+error: you should consider adding a `Default` implementation for `NewNotEqualToDerive`
+  --> $DIR/new_without_default.rs:157:5
+   |
+LL | /     pub fn new() -> Self {
+LL | |         NewNotEqualToDerive { foo: 1 }
+LL | |     }
+   | |_____^
+   |
+help: try this
+   |
+LL | impl Default for NewNotEqualToDerive {
+LL |     fn default() -> Self {
+LL |         Self::new()
+LL |     }
+LL | }
+   |
+
+error: aborting due to 4 previous errors