From 706a961d24c6c00754e3e8d118c3c2a0dce6ffa8 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Sun, 16 Feb 2025 13:12:44 -0500 Subject: [PATCH 1/2] Implement `default_mismatches_new` lint 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. 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. ```rust 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 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 | 1 + clippy_lints/src/declared_lints.rs | 1 + .../src/default_constructed_unit_structs.rs | 13 +- clippy_lints/src/new_without_default.rs | 355 +++++++++++++----- .../ui/default_constructed_unit_structs.fixed | 2 +- tests/ui/default_constructed_unit_structs.rs | 2 +- tests/ui/default_mismatches_new.fixed | 166 ++++++++ tests/ui/default_mismatches_new.rs | 180 +++++++++ tests/ui/default_mismatches_new.stderr | 75 ++++ tests/ui/new_without_default.fixed | 3 +- tests/ui/new_without_default.rs | 3 +- tests/ui/new_without_default.stderr | 18 +- 12 files changed, 708 insertions(+), 111 deletions(-) create mode 100644 tests/ui/default_mismatches_new.fixed create mode 100644 tests/ui/default_mismatches_new.rs create mode 100644 tests/ui/default_mismatches_new.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index a8b703028f24..8f6ed3507429 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5607,6 +5607,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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 3e9469256e51..df0098fdad9a 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -576,6 +576,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, diff --git a/clippy_lints/src/default_constructed_unit_structs.rs b/clippy_lints/src/default_constructed_unit_structs.rs index f8a9037fc804..6d6a1349bf26 100644 --- a/clippy_lints/src/default_constructed_unit_structs.rs +++ b/clippy_lints/src/default_constructed_unit_structs.rs @@ -47,14 +47,6 @@ 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 @@ -62,7 +54,7 @@ impl LateLintPass<'_> for DefaultConstructedUnitStructs { && let ExprKind::Path(ref qpath @ hir::QPath::TypeRelative(base, _)) = fn_expr.kind // make sure this isn't a type alias: // `::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) @@ -70,7 +62,8 @@ impl LateLintPass<'_> for DefaultConstructedUnitStructs { && 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() // do not suggest replacing an expression by a type name with placeholders && !base.is_suggestable_infer_ty() { diff --git a/clippy_lints/src/new_without_default.rs b/clippy_lints/src/new_without_default.rs index 4b73a4455f55..2b80e7a6e69e 100644 --- a/clippy_lints/src/new_without_default.rs +++ b/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, + impling_types: Option>, } -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,83 +135,86 @@ 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 { - let name = impl_item.ident.name; - let id = impl_item.owner_id; - if sig.header.is_unsafe() { - // can't be implemented for unsafe new - return; - } - if cx.tcx.is_doc_hidden(impl_item.owner_id.def_id) { - // shouldn't be implemented when it is hidden in docs - return; - } - if !impl_item.generics.params.is_empty() { - // when the result of `new()` depends on a parameter we should not require - // an impl of `Default` - return; - } - 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(); - for &d in cx.tcx.local_trait_impls(default_trait_id) { - let ty = cx.tcx.type_of(d).instantiate_identity(); - if let Some(ty_def) = ty.ty_adt_def() - && let Some(local_def_id) = ty_def.did().as_local() - { - impls.insert(cx.tcx.local_def_id_to_hir_id(local_def_id)); - } + let hir::ImplItemKind::Fn(ref sig, body_id) = impl_item.kind else { + continue; + }; + + let name = impl_item.ident.name; + let id = impl_item.owner_id; + if sig.header.is_unsafe() { + // can't be implemented for unsafe new + return; + } + if cx.tcx.is_doc_hidden(impl_item.owner_id.def_id) { + // shouldn't be implemented when it is hidden in docs + return; + } + if !impl_item.generics.params.is_empty() { + // when the result of `new()` depends on a parameter we should not require + // an impl of `Default` + return; + } + if sig.decl.inputs.is_empty() + && name == sym::new + && 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 = HirIdMap::default(); + for &d in cx.tcx.local_trait_impls(default_trait_id) { + let ty = cx.tcx.type_of(d).instantiate_identity(); + if let Some(ty_def) = ty.ty_adt_def() + && let Some(local_def_id) = ty_def.did().as_local() + { + 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); } + self.impling_types = Some(impls); + } - // 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; - } + // Check if a Default implementation exists for the Self type, regardless of generics + let default_type = 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() + { + impling_types.get(&cx.tcx.local_def_id_to_hir_id(self_local_did)) + } else { + None + }; - 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, - ); - }, - ); + match default_type { + Some(DefaultType::AutoDerived) => { + if let hir::ExprKind::Block(block, _) = cx.tcx.hir_body(body_id).value.kind + && !is_unit_struct(cx, self_ty) + // TODO: handle generics + && generics.params.is_empty() + // this type has an automatically derived `Default` implementation + // check if `new` and `default` are equivalent + && let Some(span) = check_block_calls_default(cx, block) + { + suggest_default_mismatch_new(cx, span, id, block, self_ty, impl_self_ty); + } + }, + Some(DefaultType::Manual) => { + // both `new` and `default` are manually implemented + }, + 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); + }, } } } @@ -156,16 +223,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 { + 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!("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, "use", "Self::default()", Applicability::Unspecified); + }, + ); +} + +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, + ); + }, + ); } diff --git a/tests/ui/default_constructed_unit_structs.fixed b/tests/ui/default_constructed_unit_structs.fixed index 1ca9be0ceddc..d97028323fe1 100644 --- a/tests/ui/default_constructed_unit_structs.fixed +++ b/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; diff --git a/tests/ui/default_constructed_unit_structs.rs b/tests/ui/default_constructed_unit_structs.rs index 99eb8913fc3c..50b9146d90c3 100644 --- a/tests/ui/default_constructed_unit_structs.rs +++ b/tests/ui/default_constructed_unit_structs.rs @@ -1,4 +1,4 @@ -#![allow(unused)] +#![allow(unused, clippy::default_mismatches_new)] #![warn(clippy::default_constructed_unit_structs)] use std::marker::PhantomData; diff --git a/tests/ui/default_mismatches_new.fixed b/tests/ui/default_mismatches_new.fixed new file mode 100644 index 000000000000..9c392d735bd5 --- /dev/null +++ b/tests/ui/default_mismatches_new.fixed @@ -0,0 +1,166 @@ +#![allow(clippy::needless_return, clippy::diverging_sub_expression)] +#![warn(clippy::default_mismatches_new)] + +fn main() {} + +// +// Nothing to change +// +struct ManualDefault(i32); +impl ManualDefault { + fn new() -> Self { + Self(42) + } +} +impl Default for ManualDefault { + fn default() -> Self { + Self(42) + } +} + +#[derive(Default)] +struct CallToDefaultDefault(i32); +impl CallToDefaultDefault { + fn new() -> Self { + Default::default() + } +} + +#[derive(Default)] +struct CallToSelfDefault(i32); +impl CallToSelfDefault { + fn new() -> Self { + Self::default() + } +} + +#[derive(Default)] +struct CallToTypeDefault(i32); +impl CallToTypeDefault { + fn new() -> Self { + CallToTypeDefault::default() + } +} + +#[derive(Default)] +struct CallToFullTypeDefault(i32); +impl CallToFullTypeDefault { + fn new() -> Self { + crate::CallToFullTypeDefault::default() + } +} + +#[derive(Default)] +struct ReturnCallToSelfDefault(i32); +impl ReturnCallToSelfDefault { + fn new() -> Self { + return Self::default(); + } +} + +#[derive(Default)] +struct MakeResultSelf(i32); +impl MakeResultSelf { + fn new() -> Result { + Ok(Self(10)) + } +} + +#[derive(Default)] +struct WithParams(i32); +impl WithParams { + fn new(val: i32) -> Self { + Self(val) + } +} + +#[derive(Default)] +struct Async(i32); +impl Async { + async fn new() -> Self { + Self(42) + } +} + +#[derive(Default)] +struct DeriveDefault; +impl DeriveDefault { + fn new() -> Self { + // Adding ::default() would cause clippy::default_constructed_unit_structs + Self + } +} + +#[derive(Default)] +struct DeriveTypeDefault; +impl DeriveTypeDefault { + fn new() -> Self { + // Adding ::default() would cause clippy::default_constructed_unit_structs + return crate::DeriveTypeDefault; + } +} + +// +// Offer suggestions +// + +#[derive(Default)] +struct DeriveIntDefault { + value: i32, +} +impl DeriveIntDefault { + fn new() -> Self { + Self::default() + } +} + +#[derive(Default)] +struct DeriveTupleDefault(i32); +impl DeriveTupleDefault { + fn new() -> Self { + Self::default() + } +} + +#[derive(Default)] +struct NonZeroDeriveDefault(i32); +impl NonZeroDeriveDefault { + fn new() -> Self { + Self::default() + } +} + +#[derive(Default)] +struct ExtraBlockDefault(i32); +impl ExtraBlockDefault { + fn new() -> Self { + Self::default() + } +} + +#[derive(Default)] +struct ExtraBlockRetDefault(i32); +impl ExtraBlockRetDefault { + fn new() -> Self { + Self::default() + } +} + +#[derive(Default)] +struct MultiStatements(i32); +impl MultiStatements { + fn new() -> Self { + Self::default() + } +} + +// +// TODO: Fix in the future +// +#[derive(Default)] +struct OptionGeneric(Option); +impl OptionGeneric { + fn new() -> Self { + OptionGeneric(None) + } +} diff --git a/tests/ui/default_mismatches_new.rs b/tests/ui/default_mismatches_new.rs new file mode 100644 index 000000000000..6da9a1a61fbd --- /dev/null +++ b/tests/ui/default_mismatches_new.rs @@ -0,0 +1,180 @@ +#![allow(clippy::needless_return, clippy::diverging_sub_expression)] +#![warn(clippy::default_mismatches_new)] + +fn main() {} + +// +// Nothing to change +// +struct ManualDefault(i32); +impl ManualDefault { + fn new() -> Self { + Self(42) + } +} +impl Default for ManualDefault { + fn default() -> Self { + Self(42) + } +} + +#[derive(Default)] +struct CallToDefaultDefault(i32); +impl CallToDefaultDefault { + fn new() -> Self { + Default::default() + } +} + +#[derive(Default)] +struct CallToSelfDefault(i32); +impl CallToSelfDefault { + fn new() -> Self { + Self::default() + } +} + +#[derive(Default)] +struct CallToTypeDefault(i32); +impl CallToTypeDefault { + fn new() -> Self { + CallToTypeDefault::default() + } +} + +#[derive(Default)] +struct CallToFullTypeDefault(i32); +impl CallToFullTypeDefault { + fn new() -> Self { + crate::CallToFullTypeDefault::default() + } +} + +#[derive(Default)] +struct ReturnCallToSelfDefault(i32); +impl ReturnCallToSelfDefault { + fn new() -> Self { + return Self::default(); + } +} + +#[derive(Default)] +struct MakeResultSelf(i32); +impl MakeResultSelf { + fn new() -> Result { + Ok(Self(10)) + } +} + +#[derive(Default)] +struct WithParams(i32); +impl WithParams { + fn new(val: i32) -> Self { + Self(val) + } +} + +#[derive(Default)] +struct Async(i32); +impl Async { + async fn new() -> Self { + Self(42) + } +} + +#[derive(Default)] +struct DeriveDefault; +impl DeriveDefault { + fn new() -> Self { + // Adding ::default() would cause clippy::default_constructed_unit_structs + Self + } +} + +#[derive(Default)] +struct DeriveTypeDefault; +impl DeriveTypeDefault { + fn new() -> Self { + // Adding ::default() would cause clippy::default_constructed_unit_structs + return crate::DeriveTypeDefault; + } +} + +// +// Offer suggestions +// + +#[derive(Default)] +struct DeriveIntDefault { + value: i32, +} +impl DeriveIntDefault { + fn new() -> Self { + //~^ default_mismatches_new + Self { value: 0 } + } +} + +#[derive(Default)] +struct DeriveTupleDefault(i32); +impl DeriveTupleDefault { + fn new() -> Self { + //~^ default_mismatches_new + Self(0) + } +} + +#[derive(Default)] +struct NonZeroDeriveDefault(i32); +impl NonZeroDeriveDefault { + fn new() -> Self { + //~^ default_mismatches_new + Self(42) + } +} + +#[derive(Default)] +struct ExtraBlockDefault(i32); +impl ExtraBlockDefault { + fn new() -> Self { + //~^ default_mismatches_new + { Self::default() } + } +} + +#[derive(Default)] +struct ExtraBlockRetDefault(i32); +impl ExtraBlockRetDefault { + fn new() -> Self { + //~^ default_mismatches_new + return { + { + { + return Self::default(); + } + } + }; + } +} + +#[derive(Default)] +struct MultiStatements(i32); +impl MultiStatements { + fn new() -> Self { + //~^ default_mismatches_new + println!("Hello, world!"); + let i = 42; + Self(i) + } +} + +// +// TODO: Fix in the future +// +#[derive(Default)] +struct OptionGeneric(Option); +impl OptionGeneric { + fn new() -> Self { + OptionGeneric(None) + } +} diff --git a/tests/ui/default_mismatches_new.stderr b/tests/ui/default_mismatches_new.stderr new file mode 100644 index 000000000000..c28574ae9bc8 --- /dev/null +++ b/tests/ui/default_mismatches_new.stderr @@ -0,0 +1,75 @@ +error: consider delegating to the auto-derived `Default` for `DeriveIntDefault` + --> tests/ui/default_mismatches_new.rs:112:22 + | +LL | fn new() -> Self { + | _______________________^ +LL | |/ +LL | || Self { value: 0 } + | ||_________________________- help: use: `Self::default()` +LL | | } + | |______^ + | + = note: `-D clippy::default-mismatches-new` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::default_mismatches_new)]` + +error: consider delegating to the auto-derived `Default` for `DeriveTupleDefault` + --> tests/ui/default_mismatches_new.rs:121:22 + | +LL | fn new() -> Self { + | _______________________^ +LL | |/ +LL | || Self(0) + | ||_______________- help: use: `Self::default()` +LL | | } + | |______^ + +error: consider delegating to the auto-derived `Default` for `NonZeroDeriveDefault` + --> tests/ui/default_mismatches_new.rs:130:22 + | +LL | fn new() -> Self { + | _______________________^ +LL | |/ +LL | || Self(42) + | ||________________- help: use: `Self::default()` +LL | | } + | |______^ + +error: consider delegating to the auto-derived `Default` for `ExtraBlockDefault` + --> tests/ui/default_mismatches_new.rs:139:22 + | +LL | fn new() -> Self { + | _______________________^ +LL | |/ +LL | || { Self::default() } + | ||___________________________- help: use: `Self::default()` +LL | | } + | |______^ + +error: consider delegating to the auto-derived `Default` for `ExtraBlockRetDefault` + --> tests/ui/default_mismatches_new.rs:148:22 + | +LL | fn new() -> Self { + | _______________________^ +LL | |/ +LL | || return { +... || +LL | || }; + | ||__________- help: use: `Self::default()` +LL | | } + | |______^ + +error: consider delegating to the auto-derived `Default` for `MultiStatements` + --> tests/ui/default_mismatches_new.rs:163:22 + | +LL | fn new() -> Self { + | _______________________^ +LL | |/ +LL | || println!("Hello, world!"); +LL | || let i = 42; +LL | || Self(i) + | ||_______________- help: use: `Self::default()` +LL | | } + | |______^ + +error: aborting due to 6 previous errors + diff --git a/tests/ui/new_without_default.fixed b/tests/ui/new_without_default.fixed index 277c335cd885..fba9c20d72c9 100644 --- a/tests/ui/new_without_default.fixed +++ b/tests/ui/new_without_default.fixed @@ -1,8 +1,9 @@ #![allow( dead_code, - clippy::missing_safety_doc, + clippy::default_mismatches_new, clippy::extra_unused_lifetimes, clippy::extra_unused_type_parameters, + clippy::missing_safety_doc, clippy::needless_lifetimes )] #![warn(clippy::new_without_default)] diff --git a/tests/ui/new_without_default.rs b/tests/ui/new_without_default.rs index f2844897c93d..e2240bc3ca48 100644 --- a/tests/ui/new_without_default.rs +++ b/tests/ui/new_without_default.rs @@ -1,8 +1,9 @@ #![allow( dead_code, - clippy::missing_safety_doc, + clippy::default_mismatches_new, clippy::extra_unused_lifetimes, clippy::extra_unused_type_parameters, + clippy::missing_safety_doc, clippy::needless_lifetimes )] #![warn(clippy::new_without_default)] diff --git a/tests/ui/new_without_default.stderr b/tests/ui/new_without_default.stderr index 70a65aba464b..90f45d296dda 100644 --- a/tests/ui/new_without_default.stderr +++ b/tests/ui/new_without_default.stderr @@ -1,5 +1,5 @@ error: you should consider adding a `Default` implementation for `Foo` - --> tests/ui/new_without_default.rs:13:5 + --> tests/ui/new_without_default.rs:14:5 | LL | / pub fn new() -> Foo { LL | | @@ -20,7 +20,7 @@ LL + } | error: you should consider adding a `Default` implementation for `Bar` - --> tests/ui/new_without_default.rs:23:5 + --> tests/ui/new_without_default.rs:24:5 | LL | / pub fn new() -> Self { LL | | @@ -39,7 +39,7 @@ LL + } | error: you should consider adding a `Default` implementation for `LtKo<'c>` - --> tests/ui/new_without_default.rs:89:5 + --> tests/ui/new_without_default.rs:90:5 | LL | / pub fn new() -> LtKo<'c> { LL | | @@ -58,7 +58,7 @@ LL + } | error: you should consider adding a `Default` implementation for `Const` - --> tests/ui/new_without_default.rs:123:5 + --> tests/ui/new_without_default.rs:124:5 | LL | / pub const fn new() -> Const { LL | | @@ -76,7 +76,7 @@ LL + } | error: you should consider adding a `Default` implementation for `NewNotEqualToDerive` - --> tests/ui/new_without_default.rs:184:5 + --> tests/ui/new_without_default.rs:185:5 | LL | / pub fn new() -> Self { LL | | @@ -95,7 +95,7 @@ LL + } | error: you should consider adding a `Default` implementation for `FooGenerics` - --> tests/ui/new_without_default.rs:194:5 + --> tests/ui/new_without_default.rs:195:5 | LL | / pub fn new() -> Self { LL | | @@ -114,7 +114,7 @@ LL + } | error: you should consider adding a `Default` implementation for `BarGenerics` - --> tests/ui/new_without_default.rs:203:5 + --> tests/ui/new_without_default.rs:204:5 | LL | / pub fn new() -> Self { LL | | @@ -133,7 +133,7 @@ LL + } | error: you should consider adding a `Default` implementation for `Foo` - --> tests/ui/new_without_default.rs:216:9 + --> tests/ui/new_without_default.rs:217:9 | LL | / pub fn new() -> Self { LL | | @@ -154,7 +154,7 @@ LL ~ impl Foo { | error: you should consider adding a `Default` implementation for `MyStruct` - --> tests/ui/new_without_default.rs:263:5 + --> tests/ui/new_without_default.rs:264:5 | LL | / pub fn new() -> Self { LL | | From 2a2d1194b4aa24ff15d4591d8526cd45f1832109 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Sun, 13 Apr 2025 13:03:52 -0400 Subject: [PATCH 2/2] better docs per review suggestion --- clippy_lints/src/new_without_default.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/new_without_default.rs b/clippy_lints/src/new_without_default.rs index 2b80e7a6e69e..a77776d605a5 100644 --- a/clippy_lints/src/new_without_default.rs +++ b/clippy_lints/src/new_without_default.rs @@ -85,8 +85,9 @@ declare_clippy_lint! { /// } /// ``` /// - /// 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`: + /// Alternatively, if the `new()` method requires a non-default initialization, consider renaming + /// it to another less standard name. Lastly, if the `new()` method needs to be `const`, + /// implement a custom `Default`: /// /// ```no_run /// struct MyStruct(i32); @@ -170,7 +171,7 @@ impl<'tcx> LateLintPass<'tcx> for NewWithoutDefault { { impls.insert( cx.tcx.local_def_id_to_hir_id(local_def_id), - if cx.tcx.is_builtin_derived(d) { + if cx.tcx.is_builtin_derived(d.into()) { DefaultType::AutoDerived } else { DefaultType::Manual