From 81572736db8898460c1ffc427a459f879dbc4c1d Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Sun, 16 Feb 2025 13:12:44 -0500 Subject: [PATCH 1/3] default_constructed_unit_structs -> is_unit_struct Move is_unit_struct into clippy_utils to use it in the new lint default_mismatches_new --- .../src/default_constructed_unit_structs.rs | 20 +++++-------------- clippy_utils/src/ty/mod.rs | 16 +++++++++++++++ 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/default_constructed_unit_structs.rs b/clippy_lints/src/default_constructed_unit_structs.rs index c831f96443c6..38750d583ccd 100644 --- a/clippy_lints/src/default_constructed_unit_structs.rs +++ b/clippy_lints/src/default_constructed_unit_structs.rs @@ -1,12 +1,12 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::is_ty_alias; use clippy_utils::source::SpanRangeExt as _; +use clippy_utils::ty::is_unit_struct; use hir::ExprKind; use hir::def::Res; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty; use rustc_session::declare_lint_pass; use rustc_span::sym; @@ -50,14 +50,6 @@ 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 @@ -65,15 +57,13 @@ 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) - && 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() + && is_unit_struct(cx.typeck_results().expr_ty(expr)) + && !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_utils/src/ty/mod.rs b/clippy_utils/src/ty/mod.rs index 056eb818c1ac..a06f76baf57e 100644 --- a/clippy_utils/src/ty/mod.rs +++ b/clippy_utils/src/ty/mod.rs @@ -363,6 +363,22 @@ pub fn is_must_use_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool { } } +/// Returns `true` if the given type is a unit struct (a struct with no fields). +pub fn is_unit_struct(ty: Ty<'_>) -> bool { + if let ty::Adt(def, _) = ty.kind() + && def.is_struct() + && let var @ VariantDef { + ctor: Some((CtorKind::Const, _)), + .. + } = def.non_enum_variant() + && !var.is_field_list_non_exhaustive() + { + true + } else { + false + } +} + /// Returns `true` if the given type is a non aggregate primitive (a `bool` or `char`, any /// integer or floating-point number type). /// From 67aed03fe992b64946f451df2cecb2d87df0677e Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Sat, 6 Jun 2026 15:30:43 +0200 Subject: [PATCH 2/3] move `new_without_default.rs` to `new_vs_default.rs` Mostly to simplify next commit's diff --- clippy_lints/src/declared_lints.rs | 2 +- clippy_lints/src/lib.rs | 4 ++-- .../src/{new_without_default.rs => new_vs_default.rs} | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) rename clippy_lints/src/{new_without_default.rs => new_vs_default.rs} (98%) diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index d9eeba275224..1341089aa42b 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -572,7 +572,7 @@ pub static LINTS: &[&::declare_clippy_lint::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::NEW_WITHOUT_DEFAULT_INFO, + crate::new_vs_default::NEW_WITHOUT_DEFAULT_INFO, crate::no_effect::NO_EFFECT_INFO, crate::no_effect::NO_EFFECT_UNDERSCORE_BINDING_INFO, crate::no_effect::UNNECESSARY_OPERATION_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8195baeac67d..7a6632846d33 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -269,7 +269,7 @@ mod needless_question_mark; mod needless_update; mod neg_cmp_op_on_partial_ord; mod neg_multiply; -mod new_without_default; +mod new_vs_default; mod no_effect; mod no_mangle_with_rust_abi; mod non_canonical_impls; @@ -626,7 +626,7 @@ rustc_lint::late_lint_methods!( UselessFormat: format::UselessFormat = format::UselessFormat::new(format_args.clone()), Swap: swap::Swap = swap::Swap, PanickingOverflowChecks: panicking_overflow_checks::PanickingOverflowChecks = panicking_overflow_checks::PanickingOverflowChecks, - NewWithoutDefault: new_without_default::NewWithoutDefault = ::default(), + NewVsDefault: new_vs_default::NewVsDefault = ::default(), DisallowedNames: disallowed_names::DisallowedNames = disallowed_names::DisallowedNames::new(conf), Functions: functions::Functions = functions::Functions::new(tcx, conf), Documentation: doc::Documentation = doc::Documentation::new(conf), diff --git a/clippy_lints/src/new_without_default.rs b/clippy_lints/src/new_vs_default.rs similarity index 98% rename from clippy_lints/src/new_without_default.rs rename to clippy_lints/src/new_vs_default.rs index 430b6f055f5f..067573d08773 100644 --- a/clippy_lints/src/new_without_default.rs +++ b/clippy_lints/src/new_vs_default.rs @@ -50,14 +50,14 @@ declare_clippy_lint! { "`pub fn new() -> Self` method without `Default` implementation" } -impl_lint_pass!(NewWithoutDefault => [NEW_WITHOUT_DEFAULT]); +impl_lint_pass!(NewVsDefault => [NEW_WITHOUT_DEFAULT]); #[derive(Clone, Default)] -pub struct NewWithoutDefault { +pub struct NewVsDefault { impling_types: Option, } -impl<'tcx> LateLintPass<'tcx> for NewWithoutDefault { +impl<'tcx> LateLintPass<'tcx> for NewVsDefault { #[expect(clippy::too_many_lines)] fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) { let hir::ItemKind::Impl(hir::Impl { From 3e33c84fc6748f7d43d8017ae154bb56bef1ce84 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Sun, 16 Feb 2025 13:12:44 -0500 Subject: [PATCH 3/3] 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 + clippy_lints/src/new_vs_default.rs | 337 +++++++++++++----- .../ui/default_constructed_unit_structs.fixed | 1 + tests/ui/default_constructed_unit_structs.rs | 1 + .../default_constructed_unit_structs.stderr | 14 +- tests/ui/default_mismatches_new.fixed | 177 +++++++++ tests/ui/default_mismatches_new.rs | 191 ++++++++++ tests/ui/default_mismatches_new.stderr | 86 +++++ tests/ui/new_without_default.fixed | 4 +- tests/ui/new_without_default.rs | 4 +- tests/ui/new_without_default.stderr | 30 +- 12 files changed, 738 insertions(+), 109 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 73e913cf2609..0c8b51e6b3b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6727,6 +6727,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 1341089aa42b..7ab3b89e860f 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -572,6 +572,7 @@ pub static LINTS: &[&::declare_clippy_lint::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_vs_default::DEFAULT_MISMATCHES_NEW_INFO, crate::new_vs_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/new_vs_default.rs b/clippy_lints/src/new_vs_default.rs index 067573d08773..4f427b31fc9f 100644 --- a/clippy_lints/src/new_vs_default.rs +++ b/clippy_lints/src/new_vs_default.rs @@ -1,15 +1,75 @@ use clippy_utils::diagnostics::span_lint_hir_and_then; -use clippy_utils::return_ty; -use clippy_utils::source::{indent_of, reindent_multiline, snippet_with_applicability}; +use clippy_utils::source::{indent_of, reindent_multiline, snippet, snippet_with_applicability, trim_span}; use clippy_utils::sugg::DiagExt; +use clippy_utils::ty::is_unit_struct; +use clippy_utils::{is_default_equivalent_call, return_ty}; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_hir::attrs::AttributeKind; -use rustc_hir::{Attribute, HirIdSet}; +use rustc_hir::{Attribute, Constness, ExprKind, HirIdMap, ImplItemKind, ItemKind, StmtKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; -use rustc_middle::ty::AssocKind; +use rustc_middle::ty::{AssocKind, Ty}; use rustc_session::impl_lint_pass; -use rustc_span::sym; +use rustc_span::{BytePos, Span, sym}; + +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, 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); + /// impl MyStruct { + /// const fn new() -> Self { + /// Self(42) + /// } + /// } + /// impl Default for MyStruct { + /// fn default() -> Self { + /// Self::new() + /// } + /// } + /// ``` + #[clippy::version = "1.97.0"] + pub DEFAULT_MISMATCHES_NEW, + suspicious, + "`fn new() -> Self` method does not forward to auto-derived `Default` implementation" +} declare_clippy_lint! { /// ### What it does @@ -50,17 +110,23 @@ declare_clippy_lint! { "`pub fn new() -> Self` method without `Default` implementation" } -impl_lint_pass!(NewVsDefault => [NEW_WITHOUT_DEFAULT]); +impl_lint_pass!(NewVsDefault => [DEFAULT_MISMATCHES_NEW, NEW_WITHOUT_DEFAULT]); + +#[derive(Debug, Clone, Copy)] +enum DefaultType { + AutoDerived, + Manual, +} #[derive(Clone, Default)] pub struct NewVsDefault { - impling_types: Option, + impling_types: Option>, } impl<'tcx> LateLintPass<'tcx> for NewVsDefault { #[expect(clippy::too_many_lines)] fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) { - let hir::ItemKind::Impl(hir::Impl { + let ItemKind::Impl(hir::Impl { of_trait: None, generics, self_ty: impl_self_ty, @@ -79,7 +145,7 @@ impl<'tcx> LateLintPass<'tcx> for NewVsDefault { && let assoc_item_hir_id = cx.tcx.local_def_id_to_hir_id(assoc_item.def_id.expect_local()) && let impl_item = cx.tcx.hir_node(assoc_item_hir_id).expect_impl_item() && !impl_item.span.in_external_macro(cx.sess().source_map()) - && let hir::ImplItemKind::Fn(ref sig, _) = impl_item.kind + && let ImplItemKind::Fn(ref sig, body_id) = impl_item.kind && let id = impl_item.owner_id // can't be implemented for unsafe new && !sig.header.is_unsafe() @@ -89,19 +155,25 @@ impl<'tcx> LateLintPass<'tcx> for NewVsDefault { // an impl of `Default` && impl_item.generics.params.is_empty() && sig.decl.inputs.is_empty() - && cx.effective_visibilities.is_exported(impl_item.owner_id.def_id) && let self_ty = cx.tcx.type_of(item.owner_id).instantiate_identity().skip_norm_wip() && self_ty == return_ty(cx, impl_item.owner_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(); for &d in cx.tcx.local_trait_impls(default_trait_id) { let ty = cx.tcx.type_of(d).instantiate_identity().skip_norm_wip(); 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)); + impls.insert( + cx.tcx.local_def_id_to_hir_id(local_def_id), + if cx.tcx.is_builtin_derived(d.into()) { + DefaultType::AutoDerived + } else { + DefaultType::Manual + }, + ); } } self.impling_types = Some(impls); @@ -109,90 +181,118 @@ impl<'tcx> LateLintPass<'tcx> for NewVsDefault { // Check if a Default implementation exists for the Self type, regardless of // generics - if let Some(ref impling_types) = self.impling_types + let default_type = if let Some(ref impling_types) = self.impling_types && let self_def = cx.tcx.type_of(item.owner_id).instantiate_identity().skip_norm_wip() && 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; - } + impling_types + .get(&cx.tcx.local_def_id_to_hir_id(self_local_did)) + .copied() + } else { + None + }; - let mut app = Applicability::MachineApplicable; - let attrs_sugg = { - let mut sugg = String::new(); - for attr in cx.tcx.hir_attrs(assoc_item_hir_id) { - let Attribute::Parsed(AttributeKind::CfgTrace(attrs)) = attr else { - // This might be some other attribute that the `impl Default` ought to inherit. - // But it could also be one of the many attributes that: - // - can't be put on an impl block -- like `#[inline]` - // - we can't even build a suggestion for, since `Attribute::span` may panic. - // - // Because of all that, remain on the safer side -- don't inherit this attr, and just - // reduce the applicability - app = Applicability::MaybeIncorrect; + match default_type { + Some(DefaultType::AutoDerived) => { + // Don't lint when new() is const - user may need const fn and derived + // Default::default() is not const + if sig.header.constness == Constness::Const { continue; - }; - - for (_, attr_span) in attrs { - sugg.push_str(&snippet_with_applicability(cx.sess(), *attr_span, "_", &mut app)); - sugg.push('\n'); } - } - sugg - }; - let generics_sugg = snippet_with_applicability(cx, generics.span, "", &mut app); - let where_clause_sugg = if generics.has_where_clause_predicates { - let where_clause_sugg = - snippet_with_applicability(cx, generics.where_clause_span, "", &mut app).to_string(); - let mut where_clause_sugg = reindent_multiline(&where_clause_sugg, true, Some(4)); - if impl_item.generics.has_where_clause_predicates { - if !where_clause_sugg.ends_with(',') { - where_clause_sugg.push(','); + if let ExprKind::Block(block, _) = cx.tcx.hir_body(body_id).value.kind + && !is_unit_struct(self_ty) + && generics.params.is_empty() + && let Some(span) = block_span_unless_only_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 => { + if !cx.effective_visibilities.is_exported(impl_item.owner_id.def_id) { + return; } - let additional_where_preds = - snippet_with_applicability(cx, impl_item.generics.where_clause_span, "", &mut app); - let ident = indent_of(cx, generics.where_clause_span).unwrap_or(0); - // Remove the leading `where ` keyword - let additional_where_preds = additional_where_preds.trim_start_matches("where").trim_start(); - where_clause_sugg.push('\n'); - where_clause_sugg.extend(std::iter::repeat_n(' ', ident)); - where_clause_sugg.push_str(additional_where_preds); - } - format!("\n{where_clause_sugg}\n") - } else if impl_item.generics.has_where_clause_predicates { - let where_clause_sugg = - snippet_with_applicability(cx, impl_item.generics.where_clause_span, "", &mut app); - let where_clause_sugg = reindent_multiline(&where_clause_sugg, true, Some(4)); - format!("\n{}\n", where_clause_sugg.trim_start()) - } else { - String::new() - }; - let self_ty_fmt = self_ty.to_string(); - let self_type_snip = snippet_with_applicability(cx, impl_self_ty.span, &self_ty_fmt, &mut app); - 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( + let mut app = Applicability::MachineApplicable; + let attrs_sugg = { + let mut sugg = String::new(); + for attr in cx.tcx.hir_attrs(assoc_item_hir_id) { + let Attribute::Parsed(AttributeKind::CfgTrace(attrs)) = attr else { + // This might be some other attribute that the `impl Default` ought to inherit. + // But it could also be one of the many attributes that: + // - can't be put on an impl block -- like `#[inline]` + // - we can't even build a suggestion for, since `Attribute::span` may panic. + // + // Because of all that, remain on the safer side -- don't inherit this attr, and + // just reduce the applicability + app = Applicability::MaybeIncorrect; + continue; + }; + + for (_, attr_span) in attrs { + sugg.push_str(&snippet_with_applicability(cx.sess(), *attr_span, "_", &mut app)); + sugg.push('\n'); + } + } + sugg + }; + let generics_sugg = snippet_with_applicability(cx, generics.span, "", &mut app); + let where_clause_sugg = if generics.has_where_clause_predicates { + let where_clause_sugg = + snippet_with_applicability(cx, generics.where_clause_span, "", &mut app).to_string(); + let mut where_clause_sugg = reindent_multiline(&where_clause_sugg, true, Some(4)); + if impl_item.generics.has_where_clause_predicates { + if !where_clause_sugg.ends_with(',') { + where_clause_sugg.push(','); + } + + let additional_where_preds = + snippet_with_applicability(cx, impl_item.generics.where_clause_span, "", &mut app); + let ident = indent_of(cx, generics.where_clause_span).unwrap_or(0); + // Remove the leading `where ` keyword + let additional_where_preds = + additional_where_preds.trim_start_matches("where").trim_start(); + where_clause_sugg.push('\n'); + where_clause_sugg.extend(std::iter::repeat_n(' ', ident)); + where_clause_sugg.push_str(additional_where_preds); + } + format!("\n{where_clause_sugg}\n") + } else if impl_item.generics.has_where_clause_predicates { + let where_clause_sugg = + snippet_with_applicability(cx, impl_item.generics.where_clause_span, "", &mut app); + let where_clause_sugg = reindent_multiline(&where_clause_sugg, true, Some(4)); + format!("\n{}\n", where_clause_sugg.trim_start()) + } else { + String::new() + }; + let self_ty_fmt = self_ty.to_string(); + let self_type_snip = snippet_with_applicability(cx, impl_self_ty.span, &self_ty_fmt, &mut app); + span_lint_hir_and_then( cx, - item.span, - "try adding this", - &create_new_without_default_suggest_msg( - &attrs_sugg, - &self_type_snip, - &generics_sugg, - &where_clause_sugg, - ), - app, + 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( + &attrs_sugg, + &self_type_snip, + &generics_sugg, + &where_clause_sugg, + ), + app, + ); + }, ); }, - ); + } } } } @@ -210,5 +310,72 @@ fn create_new_without_default_suggest_msg( fn default() -> Self {{ Self::new() }} -}}") +}}" + ) +} + +/// Returns `Some(span)` if the block contains custom logic (not just a call to `default()`). +/// Returns `None` if the block only contains `Self::default()` or `return Self::default();`. +fn block_span_unless_only_default(cx: &LateContext<'_>, block: &hir::Block<'_>) -> Option { + if let Some(expr) = block.expr + && block.stmts.is_empty() + && expr_calls_default(cx, expr) + { + // Block only has a trailing expression, e.g. `Self::default()` + return None; + } + if let [hir::Stmt { kind, .. }] = block.stmts + && let StmtKind::Expr(expr) | StmtKind::Semi(expr) = kind + && let ExprKind::Ret(Some(ret_expr)) = expr.kind + && expr_calls_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(1)); + span = span.with_hi(span.hi() - BytePos(1)); + span = trim_span(cx.sess().source_map(), span); + + Some(span) +} + +/// Check for `Self::default()` call syntax or equivalent +fn expr_calls_default(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool { + if let ExprKind::Call(callee, &[]) = expr.kind + && is_default_equivalent_call(cx, callee, Some(expr)) + { + 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!("`new()` may produce different values than the auto-derived `Default` for `{self_type_snip}`"), + |diag| { + diag.help( + "when a type derives `Default`, users expect `new()` and `default()` to be equivalent. \ + Consider delegating to `Self::default()` for consistency, or rename `new` if the \ + behavior is intentionally different", + ); + diag.span_suggestion(span, "use", "Self::default()", Applicability::MaybeIncorrect); + }, + ); } diff --git a/tests/ui/default_constructed_unit_structs.fixed b/tests/ui/default_constructed_unit_structs.fixed index 0ab7518fd0b0..8de4788c773f 100644 --- a/tests/ui/default_constructed_unit_structs.fixed +++ b/tests/ui/default_constructed_unit_structs.fixed @@ -1,3 +1,4 @@ +#![allow(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 cef331f56a6a..4929f440c1b3 100644 --- a/tests/ui/default_constructed_unit_structs.rs +++ b/tests/ui/default_constructed_unit_structs.rs @@ -1,3 +1,4 @@ +#![allow(clippy::default_mismatches_new)] #![warn(clippy::default_constructed_unit_structs)] use std::marker::PhantomData; diff --git a/tests/ui/default_constructed_unit_structs.stderr b/tests/ui/default_constructed_unit_structs.stderr index ee621c09f0e0..7b30c18dc3cf 100644 --- a/tests/ui/default_constructed_unit_structs.stderr +++ b/tests/ui/default_constructed_unit_structs.stderr @@ -1,5 +1,5 @@ error: use of `default` to create a unit struct - --> tests/ui/default_constructed_unit_structs.rs:10:9 + --> tests/ui/default_constructed_unit_structs.rs:11:9 | LL | Self::default() | ^^^^^^^^^^^^^^^ @@ -13,7 +13,7 @@ LL + Self | error: use of `default` to create a unit struct - --> tests/ui/default_constructed_unit_structs.rs:53:20 + --> tests/ui/default_constructed_unit_structs.rs:54:20 | LL | inner: PhantomData::default(), | ^^^^^^^^^^^^^^^^^^^^^^ @@ -25,7 +25,7 @@ LL + inner: PhantomData, | error: use of `default` to create a unit struct - --> tests/ui/default_constructed_unit_structs.rs:127:13 + --> tests/ui/default_constructed_unit_structs.rs:128:13 | LL | let _ = PhantomData::::default(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -37,7 +37,7 @@ LL + let _ = PhantomData::; | error: use of `default` to create a unit struct - --> tests/ui/default_constructed_unit_structs.rs:129:31 + --> tests/ui/default_constructed_unit_structs.rs:130:31 | LL | let _: PhantomData = PhantomData::default(); | ^^^^^^^^^^^^^^^^^^^^^^ @@ -49,7 +49,7 @@ LL + let _: PhantomData = PhantomData; | error: use of `default` to create a unit struct - --> tests/ui/default_constructed_unit_structs.rs:131:31 + --> tests/ui/default_constructed_unit_structs.rs:132:31 | LL | let _: PhantomData = std::marker::PhantomData::default(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -61,7 +61,7 @@ LL + let _: PhantomData = std::marker::PhantomData; | error: use of `default` to create a unit struct - --> tests/ui/default_constructed_unit_structs.rs:133:13 + --> tests/ui/default_constructed_unit_structs.rs:134:13 | LL | let _ = UnitStruct::default(); | ^^^^^^^^^^^^^^^^^^^^^ @@ -73,7 +73,7 @@ LL + let _ = UnitStruct; | error: use of `default` to create a unit struct - --> tests/ui/default_constructed_unit_structs.rs:171:7 + --> tests/ui/default_constructed_unit_structs.rs:172:7 | LL | f(::default()); | ^^^^^^^^^^^^^^ diff --git a/tests/ui/default_mismatches_new.fixed b/tests/ui/default_mismatches_new.fixed new file mode 100644 index 000000000000..0fabd0b3cead --- /dev/null +++ b/tests/ui/default_mismatches_new.fixed @@ -0,0 +1,177 @@ +#![allow(clippy::needless_return, clippy::diverging_sub_expression)] +#![warn(clippy::default_mismatches_new)] + +fn main() {} + +// Don't warn: Default impl is manual +struct ManualDefault(i32); +impl ManualDefault { + fn new() -> Self { + Self(42) + } +} +impl Default for ManualDefault { + fn default() -> Self { + Self(42) + } +} + +// Don't warn: new() calls automatic Default impl +#[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(); + } +} + +// Don't warn: new() returns Result, so it can't return Self::default() +#[derive(Default)] +struct MakeResultSelf(i32); +impl MakeResultSelf { + fn new() -> Result { + Ok(Self(10)) + } +} + +// Don't warn: new() takes parameters, so it is semantically different from default() +#[derive(Default)] +struct WithParams(i32); +impl WithParams { + fn new(val: i32) -> Self { + Self(val) + } +} + +// Don't warn: new() is async +#[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; + } +} + +// Don't warn: new() is const - user may need const fn and derived Default isn't const (yet) +#[derive(Default)] +struct ConstNew(i32); +impl ConstNew { + const fn new() -> Self { + Self(42) + } +} + +// +// 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..cc73c000d930 --- /dev/null +++ b/tests/ui/default_mismatches_new.rs @@ -0,0 +1,191 @@ +#![allow(clippy::needless_return, clippy::diverging_sub_expression)] +#![warn(clippy::default_mismatches_new)] + +fn main() {} + +// Don't warn: Default impl is manual +struct ManualDefault(i32); +impl ManualDefault { + fn new() -> Self { + Self(42) + } +} +impl Default for ManualDefault { + fn default() -> Self { + Self(42) + } +} + +// Don't warn: new() calls automatic Default impl +#[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(); + } +} + +// Don't warn: new() returns Result, so it can't return Self::default() +#[derive(Default)] +struct MakeResultSelf(i32); +impl MakeResultSelf { + fn new() -> Result { + Ok(Self(10)) + } +} + +// Don't warn: new() takes parameters, so it is semantically different from default() +#[derive(Default)] +struct WithParams(i32); +impl WithParams { + fn new(val: i32) -> Self { + Self(val) + } +} + +// Don't warn: new() is async +#[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; + } +} + +// Don't warn: new() is const - user may need const fn and derived Default isn't const (yet) +#[derive(Default)] +struct ConstNew(i32); +impl ConstNew { + const fn new() -> Self { + Self(42) + } +} + +// +// 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..919f8bce3870 --- /dev/null +++ b/tests/ui/default_mismatches_new.stderr @@ -0,0 +1,86 @@ +error: `new()` may produce different values than the auto-derived `Default` for `DeriveIntDefault` + --> tests/ui/default_mismatches_new.rs:123:22 + | +LL | fn new() -> Self { + | _______________________^ +LL | |/ +LL | || Self { value: 0 } + | ||_________________________- help: use: `Self::default()` +LL | | } + | |______^ + | + = help: when a type derives `Default`, users expect `new()` and `default()` to be equivalent. Consider delegating to `Self::default()` for consistency, or rename `new` if the behavior is intentionally different + = note: `-D clippy::default-mismatches-new` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::default_mismatches_new)]` + +error: `new()` may produce different values than the auto-derived `Default` for `DeriveTupleDefault` + --> tests/ui/default_mismatches_new.rs:132:22 + | +LL | fn new() -> Self { + | _______________________^ +LL | |/ +LL | || Self(0) + | ||_______________- help: use: `Self::default()` +LL | | } + | |______^ + | + = help: when a type derives `Default`, users expect `new()` and `default()` to be equivalent. Consider delegating to `Self::default()` for consistency, or rename `new` if the behavior is intentionally different + +error: `new()` may produce different values than the auto-derived `Default` for `NonZeroDeriveDefault` + --> tests/ui/default_mismatches_new.rs:141:22 + | +LL | fn new() -> Self { + | _______________________^ +LL | |/ +LL | || Self(42) + | ||________________- help: use: `Self::default()` +LL | | } + | |______^ + | + = help: when a type derives `Default`, users expect `new()` and `default()` to be equivalent. Consider delegating to `Self::default()` for consistency, or rename `new` if the behavior is intentionally different + +error: `new()` may produce different values than the auto-derived `Default` for `ExtraBlockDefault` + --> tests/ui/default_mismatches_new.rs:150:22 + | +LL | fn new() -> Self { + | _______________________^ +LL | |/ +LL | || { Self::default() } + | ||___________________________- help: use: `Self::default()` +LL | | } + | |______^ + | + = help: when a type derives `Default`, users expect `new()` and `default()` to be equivalent. Consider delegating to `Self::default()` for consistency, or rename `new` if the behavior is intentionally different + +error: `new()` may produce different values than the auto-derived `Default` for `ExtraBlockRetDefault` + --> tests/ui/default_mismatches_new.rs:159:22 + | +LL | fn new() -> Self { + | _______________________^ +LL | |/ +LL | || return { +... || +LL | || }; + | ||__________- help: use: `Self::default()` +LL | | } + | |______^ + | + = help: when a type derives `Default`, users expect `new()` and `default()` to be equivalent. Consider delegating to `Self::default()` for consistency, or rename `new` if the behavior is intentionally different + +error: `new()` may produce different values than the auto-derived `Default` for `MultiStatements` + --> tests/ui/default_mismatches_new.rs:174:22 + | +LL | fn new() -> Self { + | _______________________^ +LL | |/ +LL | || println!("Hello, world!"); +LL | || let i = 42; +LL | || Self(i) + | ||_______________- help: use: `Self::default()` +LL | | } + | |______^ + | + = help: when a type derives `Default`, users expect `new()` and `default()` to be equivalent. Consider delegating to `Self::default()` for consistency, or rename `new` if the behavior is intentionally different + +error: aborting due to 6 previous errors + diff --git a/tests/ui/new_without_default.fixed b/tests/ui/new_without_default.fixed index f6591820feeb..099eb2c7b729 100644 --- a/tests/ui/new_without_default.fixed +++ b/tests/ui/new_without_default.fixed @@ -1,7 +1,9 @@ #![allow( - clippy::missing_safety_doc, + dead_code, + 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 d3447f2e16b2..cade41a0891b 100644 --- a/tests/ui/new_without_default.rs +++ b/tests/ui/new_without_default.rs @@ -1,7 +1,9 @@ #![allow( - clippy::missing_safety_doc, + dead_code, + 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 6c0f73d13185..edbf7320d49d 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:12: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:22: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:88: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:122: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:183: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:193: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:202: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:215: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:262:5 + --> tests/ui/new_without_default.rs:264:5 | LL | / pub fn new() -> Self { LL | | @@ -175,7 +175,7 @@ LL + } | error: you should consider adding a `Default` implementation for `NewWithCfg` - --> tests/ui/new_without_default.rs:273:5 + --> tests/ui/new_without_default.rs:275:5 | LL | / pub fn new() -> Self { LL | | @@ -194,7 +194,7 @@ LL + } | error: you should consider adding a `Default` implementation for `NewWith2Cfgs` - --> tests/ui/new_without_default.rs:283:5 + --> tests/ui/new_without_default.rs:285:5 | LL | / pub fn new() -> Self { LL | | @@ -214,7 +214,7 @@ LL + } | error: you should consider adding a `Default` implementation for `NewWithExtraneous` - --> tests/ui/new_without_default.rs:292:5 + --> tests/ui/new_without_default.rs:294:5 | LL | / pub fn new() -> Self { LL | | @@ -232,7 +232,7 @@ LL + } | error: you should consider adding a `Default` implementation for `NewWithCfgAndExtraneous` - --> tests/ui/new_without_default.rs:302:5 + --> tests/ui/new_without_default.rs:304:5 | LL | / pub fn new() -> Self { LL | | @@ -251,7 +251,7 @@ LL + } | error: you should consider adding a `Default` implementation for `Foo` - --> tests/ui/new_without_default.rs:340:9 + --> tests/ui/new_without_default.rs:342:9 | LL | / pub fn new() -> Self LL | | @@ -278,7 +278,7 @@ LL ~ impl Foo | error: you should consider adding a `Default` implementation for `Bar` - --> tests/ui/new_without_default.rs:354:9 + --> tests/ui/new_without_default.rs:356:9 | LL | / pub fn new() -> Self LL | |