Implement default_mismatches_new lint

### What it does
If a type has an auto-derived `Default` trait and a `fn new() -> Self`,
this lint checks if the `new()` method performs custom logic rather
than simply calling the `default()` method.

### Why is this bad?
Users expect the `new()` method to be equivalent to `default()`,
so if the `Default` trait is auto-derived, the `new()` method should
not perform custom logic.  Otherwise, there is a risk of different
behavior between the two instantiation methods.

### Example
```rust
#[derive(Default)]
struct MyStruct(i32);
impl MyStruct {
  fn new() -> Self {
    Self(42)
  }
}
```

Users are unlikely to notice that `MyStruct::new()` and `MyStruct::default()` would produce
different results. The `new()` method should use auto-derived `default()` instead to be consistent:

```rust
#[derive(Default)]
struct MyStruct(i32);
impl MyStruct {
  fn new() -> Self {
    Self::default()
  }
}
```

Alternatively, if the `new()` method requires a non-default initialization, implement a custom `Default`.
This also allows you to mark the `new()` implementation as `const`:

```rust
struct MyStruct(i32);
impl MyStruct {
  const fn new() -> Self {
    Self(42)
  }
}
impl Default for MyStruct {
  fn default() -> Self {
    Self::new()
  }
}
```

CHANGELOG.md

@@ -5507,6 +5507,7 @@ Released 2018-09-13
 [`declare_interior_mutable_const`]: https://rust-lang.github.io/rust-clippy/master/index.html#declare_interior_mutable_const
 [`default_constructed_unit_structs`]: https://rust-lang.github.io/rust-clippy/master/index.html#default_constructed_unit_structs
 [`default_instead_of_iter_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#default_instead_of_iter_empty
+[`default_mismatches_new`]: https://rust-lang.github.io/rust-clippy/master/index.html#default_mismatches_new
 [`default_numeric_fallback`]: https://rust-lang.github.io/rust-clippy/master/index.html#default_numeric_fallback
 [`default_trait_access`]: https://rust-lang.github.io/rust-clippy/master/index.html#default_trait_access
 [`default_union_representation`]: https://rust-lang.github.io/rust-clippy/master/index.html#default_union_representation

clippy_lints/src/declared_lints.rs

@@ -569,6 +569,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
     crate::needless_update::NEEDLESS_UPDATE_INFO,
     crate::neg_cmp_op_on_partial_ord::NEG_CMP_OP_ON_PARTIAL_ORD_INFO,
     crate::neg_multiply::NEG_MULTIPLY_INFO,
+    crate::new_without_default::DEFAULT_MISMATCHES_NEW_INFO,
     crate::new_without_default::NEW_WITHOUT_DEFAULT_INFO,
     crate::no_effect::NO_EFFECT_INFO,
     crate::no_effect::NO_EFFECT_UNDERSCORE_BINDING_INFO,

clippy_lints/src/default_constructed_unit_structs.rs

@@ -46,30 +46,23 @@ declare_clippy_lint! {
 }
 declare_lint_pass!(DefaultConstructedUnitStructs => [DEFAULT_CONSTRUCTED_UNIT_STRUCTS]);
 
-fn is_alias(ty: hir::Ty<'_>) -> bool {
-    if let hir::TyKind::Path(ref qpath) = ty.kind {
-        is_ty_alias(qpath)
-    } else {
-        false
-    }
-}
-
 impl LateLintPass<'_> for DefaultConstructedUnitStructs {
     fn check_expr<'tcx>(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'tcx>) {
         if let ExprKind::Call(fn_expr, &[]) = expr.kind
             // make sure we have a call to `Default::default`
             && let ExprKind::Path(ref qpath @ hir::QPath::TypeRelative(base, _)) = fn_expr.kind
             // make sure this isn't a type alias:
             // `<Foo as Bar>::Assoc` cannot be used as a constructor
-            && !is_alias(*base)
+            && !matches!(base.kind, hir::TyKind::Path(ref qpath) if is_ty_alias(qpath))
             && let Res::Def(_, def_id) = cx.qpath_res(qpath, fn_expr.hir_id)
             && cx.tcx.is_diagnostic_item(sym::default_fn, def_id)
             // make sure we have a struct with no fields (unit struct)
             && let ty::Adt(def, ..) = cx.typeck_results().expr_ty(expr).kind()
             && def.is_struct()
             && let var @ ty::VariantDef { ctor: Some((hir::def::CtorKind::Const, _)), .. } = def.non_enum_variant()
             && !var.is_field_list_non_exhaustive()
-            && !expr.span.from_expansion() && !qpath.span().from_expansion()
+            && !expr.span.from_expansion()
+            && !qpath.span().from_expansion()
         {
             span_lint_and_sugg(
                 cx,

clippy_lints/src/new_without_default.rs

@@ -1,13 +1,14 @@
 use clippy_utils::diagnostics::span_lint_hir_and_then;
-use clippy_utils::return_ty;
-use clippy_utils::source::snippet;
+use clippy_utils::source::{snippet, trim_span};
 use clippy_utils::sugg::DiagExt;
+use clippy_utils::{is_default_equivalent_call, return_ty};
 use rustc_errors::Applicability;
 use rustc_hir as hir;
-use rustc_hir::HirIdSet;
+use rustc_hir::HirIdMap;
 use rustc_lint::{LateContext, LateLintPass, LintContext};
+use rustc_middle::ty::{Adt, Ty, VariantDef};
 use rustc_session::impl_lint_pass;
-use rustc_span::sym;
+use rustc_span::{BytePos, Pos as _, Span, sym};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -48,12 +49,75 @@ declare_clippy_lint! {
     "`pub fn new() -> Self` method without `Default` implementation"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// If a type has an auto-derived `Default` trait and a `fn new() -> Self`,
+    /// this lint checks if the `new()` method performs custom logic rather
+    /// than simply calling the `default()` method.
+    ///
+    /// ### Why is this bad?
+    /// Users expect the `new()` method to be equivalent to `default()`,
+    /// so if the `Default` trait is auto-derived, the `new()` method should
+    /// not perform custom logic.  Otherwise, there is a risk of different
+    /// behavior between the two instantiation methods.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// #[derive(Default)]
+    /// struct MyStruct(i32);
+    /// impl MyStruct {
+    ///   fn new() -> Self {
+    ///     Self(42)
+    ///   }
+    /// }
+    /// ```
+    ///
+    /// Users are unlikely to notice that `MyStruct::new()` and `MyStruct::default()` would produce
+    /// different results. The `new()` method should use auto-derived `default()` instead to be consistent:
+    ///
+    /// ```no_run
+    /// #[derive(Default)]
+    /// struct MyStruct(i32);
+    /// impl MyStruct {
+    ///   fn new() -> Self {
+    ///     Self::default()
+    ///   }
+    /// }
+    /// ```
+    ///
+    /// Alternatively, if the `new()` method requires a non-default initialization, implement a custom `Default`.
+    /// This also allows you to mark the `new()` implementation as `const`:
+    ///
+    /// ```no_run
+    /// struct MyStruct(i32);
+    /// impl MyStruct {
+    ///   const fn new() -> Self {
+    ///     Self(42)
+    ///   }
+    /// }
+    /// impl Default for MyStruct {
+    ///   fn default() -> Self {
+    ///     Self::new()
+    ///   }
+    /// }
+    #[clippy::version = "1.86.0"]
+    pub DEFAULT_MISMATCHES_NEW,
+    suspicious,
+    "`fn new() -> Self` method does not forward to auto-derived `Default` implementation"
+}
+
+#[derive(Debug, Clone, Copy)]
+enum DefaultType {
+    AutoDerived,
+    Manual,
+}
+
 #[derive(Clone, Default)]
 pub struct NewWithoutDefault {
-    impling_types: Option<HirIdSet>,
+    impling_types: Option<HirIdMap<DefaultType>>,
 }
 
-impl_lint_pass!(NewWithoutDefault => [NEW_WITHOUT_DEFAULT]);
+impl_lint_pass!(NewWithoutDefault => [NEW_WITHOUT_DEFAULT, DEFAULT_MISMATCHES_NEW]);
 
 impl<'tcx> LateLintPass<'tcx> for NewWithoutDefault {
     fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) {
@@ -71,7 +135,7 @@ impl<'tcx> LateLintPass<'tcx> for NewWithoutDefault {
                     if impl_item.span.in_external_macro(cx.sess().source_map()) {
                         return;
                     }
-                    if let hir::ImplItemKind::Fn(ref sig, _) = impl_item.kind {
+                    if let hir::ImplItemKind::Fn(ref sig, body_id) = impl_item.kind {
                         let name = impl_item.ident.name;
                         let id = impl_item.owner_id;
                         if sig.header.is_unsafe() {
@@ -89,65 +153,62 @@ impl<'tcx> LateLintPass<'tcx> for NewWithoutDefault {
                         }
                         if sig.decl.inputs.is_empty()
                             && name == sym::new
-                            && cx.effective_visibilities.is_reachable(impl_item.owner_id.def_id)
                             && let self_def_id = cx.tcx.hir().get_parent_item(id.into())
                             && let self_ty = cx.tcx.type_of(self_def_id).instantiate_identity()
                             && self_ty == return_ty(cx, id)
                             && let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default)
                         {
                             if self.impling_types.is_none() {
-                                let mut impls = HirIdSet::default();
+                                let mut impls = HirIdMap::default();
                                 cx.tcx.for_each_impl(default_trait_id, |d| {
                                     let ty = cx.tcx.type_of(d).instantiate_identity();
                                     if let Some(ty_def) = ty.ty_adt_def() {
                                         if let Some(local_def_id) = ty_def.did().as_local() {
-                                            impls.insert(cx.tcx.local_def_id_to_hir_id(local_def_id));
+                                            impls.insert(
+                                                cx.tcx.local_def_id_to_hir_id(local_def_id),
+                                                if cx.tcx.is_builtin_derived(d) {
+                                                    DefaultType::AutoDerived
+                                                } else {
+                                                    DefaultType::Manual
+                                                },
+                                            );
                                         }
                                     }
                                 });
                                 self.impling_types = Some(impls);
                             }
 
+                            let mut default_type = None;
                             // Check if a Default implementation exists for the Self type, regardless of
                             // generics
                             if let Some(ref impling_types) = self.impling_types
                                 && let self_def = cx.tcx.type_of(self_def_id).instantiate_identity()
                                 && let Some(self_def) = self_def.ty_adt_def()
                                 && let Some(self_local_did) = self_def.did().as_local()
-                                && let self_id = cx.tcx.local_def_id_to_hir_id(self_local_did)
-                                && impling_types.contains(&self_id)
                             {
-                                return;
+                                let self_id = cx.tcx.local_def_id_to_hir_id(self_local_did);
+                                default_type = impling_types.get(&self_id);
+                                if let Some(DefaultType::Manual) = default_type {
+                                    // both `new` and `default` are manually implemented
+                                    return;
+                                }
                             }
 
-                            let generics_sugg = snippet(cx, generics.span, "");
-                            let where_clause_sugg = if generics.has_where_clause_predicates {
-                                format!("\n{}\n", snippet(cx, generics.where_clause_span, ""))
-                            } else {
-                                String::new()
-                            };
-                            let self_ty_fmt = self_ty.to_string();
-                            let self_type_snip = snippet(cx, impl_self_ty.span, &self_ty_fmt);
-                            span_lint_hir_and_then(
-                                cx,
-                                NEW_WITHOUT_DEFAULT,
-                                id.into(),
-                                impl_item.span,
-                                format!("you should consider adding a `Default` implementation for `{self_type_snip}`"),
-                                |diag| {
-                                    diag.suggest_prepend_item(
-                                        cx,
-                                        item.span,
-                                        "try adding this",
-                                        &create_new_without_default_suggest_msg(
-                                            &self_type_snip,
-                                            &generics_sugg,
-                                            &where_clause_sugg,
-                                        ),
-                                        Applicability::MachineApplicable,
-                                    );
-                                },
-                            );
+                            if default_type.is_none() {
+                                // there are no `Default` implementations for this type
+                                if !cx.effective_visibilities.is_reachable(impl_item.owner_id.def_id) {
+                                    return;
+                                }
+                                suggest_new_without_default(cx, item, impl_item, id, self_ty, generics, impl_self_ty);
+                            } else if let hir::ExprKind::Block(block, _) = cx.tcx.hir().body(body_id).value.kind
+                                && !is_unit_struct(cx, self_ty)
+                            {
+                                // this type has an automatically derived `Default` implementation
+                                // check if `new` and `default` are equivalent
+                                if let Some(span) = check_block_calls_default(cx, block) {
+                                    suggest_default_mismatch_new(cx, span, id, block, self_ty, impl_self_ty);
+                                }
+                            }
                         }
                     }
                 }
@@ -156,16 +217,128 @@ impl<'tcx> LateLintPass<'tcx> for NewWithoutDefault {
     }
 }
 
-fn create_new_without_default_suggest_msg(
-    self_type_snip: &str,
-    generics_sugg: &str,
-    where_clause_sugg: &str,
-) -> String {
-    #[rustfmt::skip]
-    format!(
-"impl{generics_sugg} Default for {self_type_snip}{where_clause_sugg} {{
+// Check if Self is a unit struct, and avoid any kind of suggestions
+// FIXME: this was copied from DefaultConstructedUnitStructs,
+//        and should be refactored into a common function
+fn is_unit_struct(_cx: &LateContext<'_>, ty: Ty<'_>) -> bool {
+    if let Adt(def, ..) = ty.kind()
+        && def.is_struct()
+        && let var @ VariantDef {
+            ctor: Some((hir::def::CtorKind::Const, _)),
+            ..
+        } = def.non_enum_variant()
+        && !var.is_field_list_non_exhaustive()
+    {
+        true
+    } else {
+        false
+    }
+}
+
+/// Check if a block contains one of these:
+/// - Empty block with an expr (e.g., `{ Self::default() }`)
+/// - One statement (e.g., `{ return Self::default(); }`)
+fn check_block_calls_default(cx: &LateContext<'_>, block: &hir::Block<'_>) -> Option<Span> {
+    if let Some(expr) = block.expr
+        && block.stmts.is_empty()
+        && check_expr_call_default(cx, expr)
+    {
+        // Block only has a trailing expression, e.g. `Self::default()`
+        return None;
+    } else if let [hir::Stmt { kind, .. }] = block.stmts
+        && let hir::StmtKind::Expr(expr) | hir::StmtKind::Semi(expr) = kind
+        && let hir::ExprKind::Ret(Some(ret_expr)) = expr.kind
+        && check_expr_call_default(cx, ret_expr)
+    {
+        // Block has a single statement, e.g. `return Self::default();`
+        return None;
+    }
+
+    // trim first and last character, and trim spaces
+    let mut span = block.span;
+    span = span.with_lo(span.lo() + BytePos::from_usize(1));
+    span = span.with_hi(span.hi() - BytePos::from_usize(1));
+    span = trim_span(cx.sess().source_map(), span);
+
+    Some(span)
+}
+
+/// Check for `Self::default()` call syntax or equivalent
+fn check_expr_call_default(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool {
+    if let hir::ExprKind::Call(callee, &[]) = expr.kind
+        // FIXME: does this include `Self { }` style calls, which is equivalent,
+        //        but not the same as `Self::default()`?
+        // FIXME: what should the whole_call_expr (3rd arg) be?
+        && is_default_equivalent_call(cx, callee, None)
+    {
+        true
+    } else {
+        false
+    }
+}
+
+fn suggest_default_mismatch_new<'tcx>(
+    cx: &LateContext<'tcx>,
+    span: Span,
+    id: rustc_hir::OwnerId,
+    block: &rustc_hir::Block<'_>,
+    self_ty: Ty<'tcx>,
+    impl_self_ty: &rustc_hir::Ty<'_>,
+) {
+    let self_ty_fmt = self_ty.to_string();
+    let self_type_snip = snippet(cx, impl_self_ty.span, &self_ty_fmt);
+    span_lint_hir_and_then(
+        cx,
+        DEFAULT_MISMATCHES_NEW,
+        id.into(),
+        block.span,
+        format!("you should consider delegating to the auto-derived `Default` for `{self_type_snip}`"),
+        |diag| {
+            // This would replace any comments, and we could work around the first comment,
+            // but in case of a block of code with multiple statements and comment lines,
+            // we can't do much.  For now, we always mark this as a MaybeIncorrect suggestion.
+            diag.span_suggestion(span, "try using this", "Self::default()", Applicability::MaybeIncorrect);
+        },
+    );
+}
+
+fn suggest_new_without_default<'tcx>(
+    cx: &LateContext<'tcx>,
+    item: &hir::Item<'_>,
+    impl_item: &hir::ImplItem<'_>,
+    id: hir::OwnerId,
+    self_ty: Ty<'tcx>,
+    generics: &hir::Generics<'_>,
+    impl_self_ty: &hir::Ty<'_>,
+) {
+    let generics_sugg = snippet(cx, generics.span, "");
+    let where_clause_sugg = if generics.has_where_clause_predicates {
+        format!("\n{}\n", snippet(cx, generics.where_clause_span, ""))
+    } else {
+        String::new()
+    };
+    let self_ty_fmt = self_ty.to_string();
+    let self_type_snip = snippet(cx, impl_self_ty.span, &self_ty_fmt);
+    span_lint_hir_and_then(
+        cx,
+        NEW_WITHOUT_DEFAULT,
+        id.into(),
+        impl_item.span,
+        format!("you should consider adding a `Default` implementation for `{self_type_snip}`"),
+        |diag| {
+            diag.suggest_prepend_item(
+                cx,
+                item.span,
+                "try adding this",
+                &format!(
+                    "impl{generics_sugg} Default for {self_type_snip}{where_clause_sugg} {{
     fn default() -> Self {{
         Self::new()
     }}
-}}")
+}}"
+                ),
+                Applicability::MachineApplicable,
+            );
+        },
+    );
 }

tests/ui/default_constructed_unit_structs.fixed

@@ -1,4 +1,4 @@
-#![allow(unused)]
+#![allow(unused, clippy::default_mismatches_new)]
 #![warn(clippy::default_constructed_unit_structs)]
 use std::marker::PhantomData;