From 1b36289ed7a6cb2d5bc95823c6cbfc3baae310b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 11 Dec 2024 01:37:17 +0000 Subject: [PATCH 01/26] Make some manual `impl Default` derived Some caught by the new lint introduced in the next commit. Others noticed manually. --- compiler/rustc_ast/src/ast.rs | 19 +++---------------- compiler/rustc_lint/src/unused.rs | 7 +------ compiler/rustc_session/src/config.rs | 9 ++------- library/core/src/ascii/ascii_char.rs | 3 ++- library/core/src/default.rs | 3 --- library/std/src/panicking.rs | 9 ++------- 6 files changed, 10 insertions(+), 40 deletions(-) diff --git a/compiler/rustc_ast/src/ast.rs b/compiler/rustc_ast/src/ast.rs index 262e418ecbf74..b2283cf32ecea 100644 --- a/compiler/rustc_ast/src/ast.rs +++ b/compiler/rustc_ast/src/ast.rs @@ -32,7 +32,7 @@ use rustc_macros::{Decodable, Encodable, HashStable_Generic}; pub use rustc_span::AttrId; use rustc_span::source_map::{Spanned, respan}; use rustc_span::symbol::{Ident, Symbol, kw, sym}; -use rustc_span::{DUMMY_SP, ErrorGuaranteed, Span}; +use rustc_span::{ErrorGuaranteed, Span}; use thin_vec::{ThinVec, thin_vec}; pub use crate::format::*; @@ -388,22 +388,15 @@ impl GenericParam { /// Represents lifetime, type and const parameters attached to a declaration of /// a function, enum, trait, etc. -#[derive(Clone, Encodable, Decodable, Debug)] +#[derive(Clone, Encodable, Decodable, Debug, Default)] pub struct Generics { pub params: ThinVec, pub where_clause: WhereClause, pub span: Span, } -impl Default for Generics { - /// Creates an instance of `Generics`. - fn default() -> Generics { - Generics { params: ThinVec::new(), where_clause: Default::default(), span: DUMMY_SP } - } -} - /// A where-clause in a definition. -#[derive(Clone, Encodable, Decodable, Debug)] +#[derive(Clone, Encodable, Decodable, Debug, Default)] pub struct WhereClause { /// `true` if we ate a `where` token. /// @@ -420,12 +413,6 @@ impl WhereClause { } } -impl Default for WhereClause { - fn default() -> WhereClause { - WhereClause { has_where_token: false, predicates: ThinVec::new(), span: DUMMY_SP } - } -} - /// A single predicate in a where-clause. #[derive(Clone, Encodable, Decodable, Debug)] pub struct WherePredicate { diff --git a/compiler/rustc_lint/src/unused.rs b/compiler/rustc_lint/src/unused.rs index 9cad5d98562b6..47c1c2d5fe986 100644 --- a/compiler/rustc_lint/src/unused.rs +++ b/compiler/rustc_lint/src/unused.rs @@ -1024,6 +1024,7 @@ declare_lint! { "`if`, `match`, `while` and `return` do not need parentheses" } +#[derive(Default)] pub(crate) struct UnusedParens { with_self_ty_parens: bool, /// `1 as (i32) < 2` parses to ExprKind::Lt @@ -1031,12 +1032,6 @@ pub(crate) struct UnusedParens { parens_in_cast_in_lt: Vec, } -impl Default for UnusedParens { - fn default() -> Self { - Self { with_self_ty_parens: false, parens_in_cast_in_lt: Vec::new() } - } -} - impl_lint_pass!(UnusedParens => [UNUSED_PARENS]); impl UnusedDelimLint for UnusedParens { diff --git a/compiler/rustc_session/src/config.rs b/compiler/rustc_session/src/config.rs index 936c2ca87d69b..03de6811fe130 100644 --- a/compiler/rustc_session/src/config.rs +++ b/compiler/rustc_session/src/config.rs @@ -162,9 +162,10 @@ pub struct CoverageOptions { } /// Controls whether branch coverage or MC/DC coverage is enabled. -#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)] +#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug, Default)] pub enum CoverageLevel { /// Instrument for coverage at the MIR block level. + #[default] Block, /// Also instrument branch points (includes block coverage). Branch, @@ -189,12 +190,6 @@ pub enum CoverageLevel { Mcdc, } -impl Default for CoverageLevel { - fn default() -> Self { - Self::Block - } -} - /// Settings for `-Z instrument-xray` flag. #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Hash)] pub struct InstrumentXRay { diff --git a/library/core/src/ascii/ascii_char.rs b/library/core/src/ascii/ascii_char.rs index 48de4f17b1b3a..fcb4584f5dc7c 100644 --- a/library/core/src/ascii/ascii_char.rs +++ b/library/core/src/ascii/ascii_char.rs @@ -54,12 +54,13 @@ use crate::{assert_unsafe_precondition, fmt}; /// [chart]: https://www.unicode.org/charts/PDF/U0000.pdf /// [NIST FIPS 1-2]: https://nvlpubs.nist.gov/nistpubs/Legacy/FIPS/fipspub1-2-1977.pdf /// [NamesList]: https://www.unicode.org/Public/15.0.0/ucd/NamesList.txt -#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] +#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Default)] #[unstable(feature = "ascii_char", issue = "110998")] #[repr(u8)] pub enum AsciiChar { /// U+0000 (The default variant) #[unstable(feature = "ascii_char_variants", issue = "110998")] + #[default] Null = 0, /// U+0001 #[unstable(feature = "ascii_char_variants", issue = "110998")] diff --git a/library/core/src/default.rs b/library/core/src/default.rs index 4c30290ff263b..426cfe9cdfefc 100644 --- a/library/core/src/default.rs +++ b/library/core/src/default.rs @@ -2,8 +2,6 @@ #![stable(feature = "rust1", since = "1.0.0")] -use crate::ascii::Char as AsciiChar; - /// A trait for giving a type a useful default value. /// /// Sometimes, you want to fall back to some kind of default value, and @@ -163,7 +161,6 @@ macro_rules! default_impl { default_impl! { (), (), "Returns the default value of `()`" } default_impl! { bool, false, "Returns the default value of `false`" } default_impl! { char, '\x00', "Returns the default value of `\\x00`" } -default_impl! { AsciiChar, AsciiChar::Null, "Returns the default value of `Null`" } default_impl! { usize, 0, "Returns the default value of `0`" } default_impl! { u8, 0, "Returns the default value of `0`" } diff --git a/library/std/src/panicking.rs b/library/std/src/panicking.rs index dca5ccca0c404..e7ce5bc61401d 100644 --- a/library/std/src/panicking.rs +++ b/library/std/src/panicking.rs @@ -81,7 +81,9 @@ extern "C" fn __rust_foreign_exception() -> ! { rtabort!("Rust cannot catch foreign exceptions"); } +#[derive(Default)] enum Hook { + #[default] Default, Custom(Box) + 'static + Sync + Send>), } @@ -96,13 +98,6 @@ impl Hook { } } -impl Default for Hook { - #[inline] - fn default() -> Hook { - Hook::Default - } -} - static HOOK: RwLock = RwLock::new(Hook::Default); /// Registers a custom panic hook, replacing the previously registered hook. From 75f1249bfeaf699f1f7d0e091aab720b312926e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 11 Dec 2024 01:40:44 +0000 Subject: [PATCH 02/26] Introduce `default_could_be_derived` lint New lint that detects when an enum with no type paremeters has a manual `Default` impl that uses a single unit variant and suggests deriving instead. ``` error: `impl Default` that could be derived --> $DIR/manual-default-impl-could-be-derived.rs:58:1 | LL | / impl Default for F { LL | | fn default() -> Self { LL | | F::Unit LL | | } LL | | } | |_^ | note: the lint level is defined here --> $DIR/manual-default-impl-could-be-derived.rs:4:9 | LL | #![deny(default_could_be_derived)] | ^^^^^^^^^^^^^^^^^^^^^^^^ help: you don't need to manually `impl Default`, you can derive it | LL ~ #[derive(Default)] enum F { LL ~ #[default] Unit, | ``` The rendering of the suggestion doesn't include the deletion, but it is there and gets machine applied. --- .../src/default_could_be_derived.rs | 122 ++++++++++++++++++ compiler/rustc_lint/src/lib.rs | 3 + ...manual-default-impl-could-be-derived.fixed | 66 ++++++++++ .../manual-default-impl-could-be-derived.rs | 71 ++++++++++ ...anual-default-impl-could-be-derived.stderr | 23 ++++ 5 files changed, 285 insertions(+) create mode 100644 compiler/rustc_lint/src/default_could_be_derived.rs create mode 100644 tests/ui/structs/manual-default-impl-could-be-derived.fixed create mode 100644 tests/ui/structs/manual-default-impl-could-be-derived.rs create mode 100644 tests/ui/structs/manual-default-impl-could-be-derived.stderr diff --git a/compiler/rustc_lint/src/default_could_be_derived.rs b/compiler/rustc_lint/src/default_could_be_derived.rs new file mode 100644 index 0000000000000..6c9123f4dd065 --- /dev/null +++ b/compiler/rustc_lint/src/default_could_be_derived.rs @@ -0,0 +1,122 @@ +use rustc_errors::Applicability; +use rustc_hir as hir; +use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res}; +use rustc_session::{declare_lint, declare_lint_pass}; +use rustc_span::symbol::{kw, sym}; + +use crate::{LateContext, LateLintPass}; + +declare_lint! { + /// The `default_could_be_derived` lint checks for manual `impl` blocks + /// of the `Default` trait that could have been derived. + /// + /// ### Example + /// + /// ```rust,compile_fail + /// enum Foo { + /// Bar, + /// } + /// + /// #[deny(default_could_be_derived)] + /// impl Default for Foo { + /// fn default() -> Foo { + /// Foo::Bar + /// } + /// } + /// ``` + /// + /// {{produces}} + /// + pub DEFAULT_COULD_BE_DERIVED, + Warn, + "detect `Default` impl that could be derived" +} + +declare_lint_pass!(DefaultCouldBeDerived => [DEFAULT_COULD_BE_DERIVED]); + +impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'tcx>) { + let hir::ItemKind::Impl(data) = item.kind else { return }; + let Some(trait_ref) = data.of_trait else { return }; + let Res::Def(DefKind::Trait, def_id) = trait_ref.path.res else { return }; + if Some(def_id) != cx.tcx.get_diagnostic_item(sym::Default) { + return; + } + if cx.tcx.has_attr(def_id, sym::automatically_derived) { + return; + } + let hir_self_ty = data.self_ty; + let hir::TyKind::Path(hir::QPath::Resolved(_, path)) = hir_self_ty.kind else { return }; + let Res::Def(_, type_def_id) = path.res else { return }; + let generics = cx.tcx.generics_of(type_def_id); + if !generics.own_params.is_empty() { + return; + } + // We have a manual `impl Default for Ty {}` item, where `Ty` has no type parameters. + + let hir = cx.tcx.hir(); + for assoc in data.items { + let hir::AssocItemKind::Fn { has_self: false } = assoc.kind else { continue }; + if assoc.ident.name != kw::Default { + continue; + } + let assoc = hir.impl_item(assoc.id); + let hir::ImplItemKind::Fn(_ty, body) = assoc.kind else { continue }; + let body = hir.body(body); + let hir::ExprKind::Block(hir::Block { stmts: [], expr: Some(expr), .. }, None) = + body.value.kind + else { + continue; + }; + + match expr.kind { + hir::ExprKind::Path(hir::QPath::Resolved(_, path)) + if let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Const), def_id) = + path.res => + { + // We have a unit variant as the default of an enum in a manual impl. + // + // enum Foo { + // Bar, + // } + // + // impl Default for Foo { + // fn default() -> Foo { + // Foo::Bar + // } + // } + // + // We suggest + // + // #[derive(Default)] enum Foo { + // #[default] Bar, + // } + cx.tcx.node_span_lint( + DEFAULT_COULD_BE_DERIVED, + item.hir_id(), + item.span, + |diag| { + diag.primary_message("`impl Default` that could be derived"); + diag.multipart_suggestion_verbose( + "you don't need to manually `impl Default`, you can derive it", + vec![ + ( + cx.tcx.def_span(type_def_id).shrink_to_lo(), + "#[derive(Default)] ".to_string(), + ), + ( + cx.tcx.def_span(def_id).shrink_to_lo(), + "#[default] ".to_string(), + ), + (item.span, String::new()), + ], + Applicability::MachineApplicable, + ); + }, + ); + } + _ => {} + } + } + } +} diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index a99c94592b302..b5e1db0283c9c 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -41,6 +41,7 @@ mod async_fn_in_trait; pub mod builtin; mod context; mod dangling; +mod default_could_be_derived; mod deref_into_dyn_supertrait; mod drop_forget_useless; mod early; @@ -85,6 +86,7 @@ use async_closures::AsyncClosureUsage; use async_fn_in_trait::AsyncFnInTrait; use builtin::*; use dangling::*; +use default_could_be_derived::DefaultCouldBeDerived; use deref_into_dyn_supertrait::*; use drop_forget_useless::*; use enum_intrinsics_non_enums::EnumIntrinsicsNonEnums; @@ -189,6 +191,7 @@ late_lint_methods!( BuiltinCombinedModuleLateLintPass, [ ForLoopsOverFallibles: ForLoopsOverFallibles, + DefaultCouldBeDerived: DefaultCouldBeDerived, DerefIntoDynSupertrait: DerefIntoDynSupertrait, DropForgetUseless: DropForgetUseless, ImproperCTypesDeclarations: ImproperCTypesDeclarations, diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.fixed b/tests/ui/structs/manual-default-impl-could-be-derived.fixed new file mode 100644 index 0000000000000..23a771a8e1fde --- /dev/null +++ b/tests/ui/structs/manual-default-impl-could-be-derived.fixed @@ -0,0 +1,66 @@ +// Warn when we encounter a manual `Default` impl that could be derived. +//@ run-rustfix +#![allow(dead_code)] +#![deny(default_could_be_derived)] + +// #[derive(Debug)] +// struct A; +// +// impl Default for A { +// fn default() -> Self { A } +// } +// +// #[derive(Debug)] +// struct B(Option); +// +// impl Default for B { +// fn default() -> Self { B(Default::default()) } +// } +// +// #[derive(Debug)] +// struct C(Option); +// +// impl Default for C { +// fn default() -> Self { C(None) } +// } +// +// #[derive(Debug)] +// struct D { +// x: Option, +// } +// +// impl Default for D { +// fn default() -> Self { +// D { +// x: Default::default(), +// } +// } +// } +// +// #[derive(Debug)] +// struct E { +// x: Option, +// } +// +// impl Default for E { +// fn default() -> Self { +// E { +// x: None, +// } +// } +// } + +#[derive(Default)] enum F { + #[default] Unit, + Tuple(i32), +} + + +fn main() { +// let _ = A::default(); +// let _ = B::default(); +// let _ = C::default(); +// let _ = D::default(); +// let _ = E::default(); + let _ = F::default(); +} diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.rs b/tests/ui/structs/manual-default-impl-could-be-derived.rs new file mode 100644 index 0000000000000..a54a7a0d890aa --- /dev/null +++ b/tests/ui/structs/manual-default-impl-could-be-derived.rs @@ -0,0 +1,71 @@ +// Warn when we encounter a manual `Default` impl that could be derived. +//@ run-rustfix +#![allow(dead_code)] +#![deny(default_could_be_derived)] + +// #[derive(Debug)] +// struct A; +// +// impl Default for A { +// fn default() -> Self { A } +// } +// +// #[derive(Debug)] +// struct B(Option); +// +// impl Default for B { +// fn default() -> Self { B(Default::default()) } +// } +// +// #[derive(Debug)] +// struct C(Option); +// +// impl Default for C { +// fn default() -> Self { C(None) } +// } +// +// #[derive(Debug)] +// struct D { +// x: Option, +// } +// +// impl Default for D { +// fn default() -> Self { +// D { +// x: Default::default(), +// } +// } +// } +// +// #[derive(Debug)] +// struct E { +// x: Option, +// } +// +// impl Default for E { +// fn default() -> Self { +// E { +// x: None, +// } +// } +// } + +enum F { + Unit, + Tuple(i32), +} + +impl Default for F { //~ ERROR + fn default() -> Self { + F::Unit + } +} + +fn main() { +// let _ = A::default(); +// let _ = B::default(); +// let _ = C::default(); +// let _ = D::default(); +// let _ = E::default(); + let _ = F::default(); +} diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.stderr b/tests/ui/structs/manual-default-impl-could-be-derived.stderr new file mode 100644 index 0000000000000..06343d3c7bc22 --- /dev/null +++ b/tests/ui/structs/manual-default-impl-could-be-derived.stderr @@ -0,0 +1,23 @@ +error: `impl Default` that could be derived + --> $DIR/manual-default-impl-could-be-derived.rs:58:1 + | +LL | / impl Default for F { +LL | | fn default() -> Self { +LL | | F::Unit +LL | | } +LL | | } + | |_^ + | +note: the lint level is defined here + --> $DIR/manual-default-impl-could-be-derived.rs:4:9 + | +LL | #![deny(default_could_be_derived)] + | ^^^^^^^^^^^^^^^^^^^^^^^^ +help: you don't need to manually `impl Default`, you can derive it + | +LL ~ #[derive(Default)] enum F { +LL ~ #[default] Unit, + | + +error: aborting due to 1 previous error + From 476fe03ee48a72beff39ddda5638d6c14e493168 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 11 Dec 2024 04:02:27 +0000 Subject: [PATCH 03/26] Use `derive(Default)` --- library/proc_macro/src/bridge/fxhash.rs | 8 +------- library/std/src/sys_common/process.rs | 8 +------- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/library/proc_macro/src/bridge/fxhash.rs b/library/proc_macro/src/bridge/fxhash.rs index 74a41451825ff..3345e099a3724 100644 --- a/library/proc_macro/src/bridge/fxhash.rs +++ b/library/proc_macro/src/bridge/fxhash.rs @@ -22,6 +22,7 @@ pub type FxHashMap = HashMap>; /// out-performs an FNV-based hash within rustc itself -- the collision rate is /// similar or slightly worse than FNV, but the speed of the hash function /// itself is much higher because it works on up to 8 bytes at a time. +#[derive(Default)] pub struct FxHasher { hash: usize, } @@ -31,13 +32,6 @@ const K: usize = 0x9e3779b9; #[cfg(target_pointer_width = "64")] const K: usize = 0x517cc1b727220a95; -impl Default for FxHasher { - #[inline] - fn default() -> FxHasher { - FxHasher { hash: 0 } - } -} - impl FxHasher { #[inline] fn add_to_hash(&mut self, i: usize) { diff --git a/library/std/src/sys_common/process.rs b/library/std/src/sys_common/process.rs index 5333ee146f7d6..9f61d69d85875 100644 --- a/library/std/src/sys_common/process.rs +++ b/library/std/src/sys_common/process.rs @@ -8,19 +8,13 @@ use crate::sys::process::{EnvKey, ExitStatus, Process, StdioPipes}; use crate::{env, fmt, io}; // Stores a set of changes to an environment -#[derive(Clone)] +#[derive(Clone, Default)] pub struct CommandEnv { clear: bool, saw_path: bool, vars: BTreeMap>, } -impl Default for CommandEnv { - fn default() -> Self { - CommandEnv { clear: false, saw_path: false, vars: Default::default() } - } -} - impl fmt::Debug for CommandEnv { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let mut debug_command_env = f.debug_struct("CommandEnv"); From 9b209dc10a385668ba74390fb5a913dbcf92ded6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 11 Dec 2024 04:03:16 +0000 Subject: [PATCH 04/26] Detect `impl Default` where fields are literals and `Default::default()` calls ``` error: `impl Default` that could be derived --> $DIR/manual-default-impl-could-be-derived.rs:33:1 | LL | / impl Default for D { LL | | fn default() -> Self { LL | | D { LL | | x: Default::default(), ... | LL | | } LL | | } | |_^ | help: you don't need to manually `impl Default`, you can derive it | LL ~ #[derive(Default)] struct D { | ``` --- .../src/default_could_be_derived.rs | 64 +++++++++++++++++++ ...manual-default-impl-could-be-derived.fixed | 18 ++---- .../manual-default-impl-could-be-derived.rs | 26 ++++---- ...anual-default-impl-could-be-derived.stderr | 25 ++++++-- 4 files changed, 105 insertions(+), 28 deletions(-) diff --git a/compiler/rustc_lint/src/default_could_be_derived.rs b/compiler/rustc_lint/src/default_could_be_derived.rs index 6c9123f4dd065..34b4c671a491a 100644 --- a/compiler/rustc_lint/src/default_could_be_derived.rs +++ b/compiler/rustc_lint/src/default_could_be_derived.rs @@ -1,6 +1,8 @@ +use rustc_ast::LitKind; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res}; +use rustc_middle::ty::TyCtxt; use rustc_session::{declare_lint, declare_lint_pass}; use rustc_span::symbol::{kw, sym}; @@ -115,6 +117,68 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { }, ); } + hir::ExprKind::Struct(_qpath, fields, _tail) => { + // We have a struct literal + // + // struct Foo { + // field: Type, + // } + // + // impl Default for Foo { + // fn default() -> Foo { + // Foo { + // field: val, + // } + // } + // } + // + // We suggest #[derive(Default)] if + // - `val` is `Default::default()` + // - `val` is `0` + // - `val` is `false` + fn check_expr(tcx: TyCtxt<'_>, kind: hir::ExprKind<'_>) -> bool { + match kind { + hir::ExprKind::Lit(spanned_lit) => match spanned_lit.node { + LitKind::Int(val, _) if val == 0 => true, // field: 0, + LitKind::Bool(false) => true, // field: false, + _ => false, + }, + hir::ExprKind::Call(expr, []) + if let hir::ExprKind::Path(hir::QPath::Resolved(_, path)) = + expr.kind + && let Some(def_id) = path.res.opt_def_id() + && tcx.is_diagnostic_item(sym::default_fn, def_id) => + { + // field: Default::default(), + true + } + _ => { + false + } + } + } + if fields.iter().all(|f| check_expr(cx.tcx, f.expr.kind)) { + cx.tcx.node_span_lint( + DEFAULT_COULD_BE_DERIVED, + item.hir_id(), + item.span, + |diag| { + diag.primary_message("`impl Default` that could be derived"); + diag.multipart_suggestion_verbose( + "you don't need to manually `impl Default`, you can derive it", + vec![ + ( + cx.tcx.def_span(type_def_id).shrink_to_lo(), + "#[derive(Default)] ".to_string(), + ), + (item.span, String::new()), + ], + Applicability::MachineApplicable, + ); + }, + ); + } + } _ => {} } } diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.fixed b/tests/ui/structs/manual-default-impl-could-be-derived.fixed index 23a771a8e1fde..fe21f90bf7860 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.fixed +++ b/tests/ui/structs/manual-default-impl-could-be-derived.fixed @@ -24,18 +24,12 @@ // fn default() -> Self { C(None) } // } // -// #[derive(Debug)] -// struct D { -// x: Option, -// } -// -// impl Default for D { -// fn default() -> Self { -// D { -// x: Default::default(), -// } -// } -// } + +#[derive(Default)] struct D { + x: Option, + y: i32, +} + // // #[derive(Debug)] // struct E { diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.rs b/tests/ui/structs/manual-default-impl-could-be-derived.rs index a54a7a0d890aa..f4f596cc5c961 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.rs +++ b/tests/ui/structs/manual-default-impl-could-be-derived.rs @@ -24,18 +24,20 @@ // fn default() -> Self { C(None) } // } // -// #[derive(Debug)] -// struct D { -// x: Option, -// } -// -// impl Default for D { -// fn default() -> Self { -// D { -// x: Default::default(), -// } -// } -// } + +struct D { + x: Option, + y: i32, +} + +impl Default for D { //~ ERROR + fn default() -> Self { + D { + x: Default::default(), + y: 0, + } + } +} // // #[derive(Debug)] // struct E { diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.stderr b/tests/ui/structs/manual-default-impl-could-be-derived.stderr index 06343d3c7bc22..e9096e250a800 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.stderr +++ b/tests/ui/structs/manual-default-impl-could-be-derived.stderr @@ -1,9 +1,11 @@ error: `impl Default` that could be derived - --> $DIR/manual-default-impl-could-be-derived.rs:58:1 + --> $DIR/manual-default-impl-could-be-derived.rs:33:1 | -LL | / impl Default for F { +LL | / impl Default for D { LL | | fn default() -> Self { -LL | | F::Unit +LL | | D { +LL | | x: Default::default(), +... | LL | | } LL | | } | |_^ @@ -15,9 +17,24 @@ LL | #![deny(default_could_be_derived)] | ^^^^^^^^^^^^^^^^^^^^^^^^ help: you don't need to manually `impl Default`, you can derive it | +LL ~ #[derive(Default)] struct D { + | + +error: `impl Default` that could be derived + --> $DIR/manual-default-impl-could-be-derived.rs:60:1 + | +LL | / impl Default for F { +LL | | fn default() -> Self { +LL | | F::Unit +LL | | } +LL | | } + | |_^ + | +help: you don't need to manually `impl Default`, you can derive it + | LL ~ #[derive(Default)] enum F { LL ~ #[default] Unit, | -error: aborting due to 1 previous error +error: aborting due to 2 previous errors From 3ec241bf4746d920f08ccdc2009bdbe43a13f1e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 11 Dec 2024 16:39:39 +0000 Subject: [PATCH 05/26] Explicitly consider fields set to `None` as `Default` derivable --- .../src/default_could_be_derived.rs | 21 +++++++-- ...manual-default-impl-could-be-derived.fixed | 40 ++++++++++------ .../manual-default-impl-could-be-derived.rs | 47 +++++++++++++------ ...anual-default-impl-could-be-derived.stderr | 23 +++++++-- 4 files changed, 93 insertions(+), 38 deletions(-) diff --git a/compiler/rustc_lint/src/default_could_be_derived.rs b/compiler/rustc_lint/src/default_could_be_derived.rs index 34b4c671a491a..0ffa11196165c 100644 --- a/compiler/rustc_lint/src/default_could_be_derived.rs +++ b/compiler/rustc_lint/src/default_could_be_derived.rs @@ -133,9 +133,9 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { // } // // We suggest #[derive(Default)] if - // - `val` is `Default::default()` - // - `val` is `0` - // - `val` is `false` + // - `val` is `Default::default()` + // - `val` is `0` + // - `val` is `false` fn check_expr(tcx: TyCtxt<'_>, kind: hir::ExprKind<'_>) -> bool { match kind { hir::ExprKind::Lit(spanned_lit) => match spanned_lit.node { @@ -152,9 +152,20 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { // field: Default::default(), true } - _ => { - false + hir::ExprKind::Path(hir::QPath::Resolved(_, path)) + if let Res::Def( + DefKind::Ctor(CtorOf::Variant, CtorKind::Const), + def_id, + ) = path.res + && let def_id = tcx.parent(def_id) // From Ctor to variant + && tcx.is_lang_item(def_id, hir::LangItem::OptionNone) => + { + // FIXME: We should use a better check where we explore existing + // `impl Default for def_id` of the found type and see compare them + // against what we have here. For now, special case `Option::None`. + true } + _ => false, } } if fields.iter().all(|f| check_expr(cx.tcx, f.expr.kind)) { diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.fixed b/tests/ui/structs/manual-default-impl-could-be-derived.fixed index fe21f90bf7860..ebb0d61e02c3d 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.fixed +++ b/tests/ui/structs/manual-default-impl-could-be-derived.fixed @@ -25,36 +25,46 @@ // } // +// Explicit check against numeric literals and `Default::default()` calls. #[derive(Default)] struct D { x: Option, y: i32, } -// -// #[derive(Debug)] -// struct E { -// x: Option, -// } -// -// impl Default for E { -// fn default() -> Self { -// E { -// x: None, -// } -// } -// } +// Explicit check against `None` literal, in the same way that we check against numeric literals. +#[derive(Debug)] +#[derive(Default)] struct E { + x: Option, +} + + +// Detection of unit variant ctors that could have been marked `#[default]`. #[derive(Default)] enum F { #[default] Unit, Tuple(i32), } +// Comparison of `impl` *fields* with their `Default::default()` bodies. +// struct G { +// f: F, +// } + +// impl Default for G { +// fn default() -> Self { +// G { +// f: F::Unit, +// } +// } +// } + fn main() { // let _ = A::default(); // let _ = B::default(); // let _ = C::default(); -// let _ = D::default(); -// let _ = E::default(); + let _ = D::default(); + let _ = E::default(); let _ = F::default(); +// let _ = G::default(); } diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.rs b/tests/ui/structs/manual-default-impl-could-be-derived.rs index f4f596cc5c961..a5909a4bfa48f 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.rs +++ b/tests/ui/structs/manual-default-impl-could-be-derived.rs @@ -25,6 +25,7 @@ // } // +// Explicit check against numeric literals and `Default::default()` calls. struct D { x: Option, y: i32, @@ -38,20 +39,22 @@ impl Default for D { //~ ERROR } } } -// -// #[derive(Debug)] -// struct E { -// x: Option, -// } -// -// impl Default for E { -// fn default() -> Self { -// E { -// x: None, -// } -// } -// } +// Explicit check against `None` literal, in the same way that we check against numeric literals. +#[derive(Debug)] +struct E { + x: Option, +} + +impl Default for E { //~ ERROR + fn default() -> Self { + E { + x: None, + } + } +} + +// Detection of unit variant ctors that could have been marked `#[default]`. enum F { Unit, Tuple(i32), @@ -63,11 +66,25 @@ impl Default for F { //~ ERROR } } +// Comparison of `impl` *fields* with their `Default::default()` bodies. +// struct G { +// f: F, +// } + +// impl Default for G { +// fn default() -> Self { +// G { +// f: F::Unit, +// } +// } +// } + fn main() { // let _ = A::default(); // let _ = B::default(); // let _ = C::default(); -// let _ = D::default(); -// let _ = E::default(); + let _ = D::default(); + let _ = E::default(); let _ = F::default(); +// let _ = G::default(); } diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.stderr b/tests/ui/structs/manual-default-impl-could-be-derived.stderr index e9096e250a800..e712fa8d5006c 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.stderr +++ b/tests/ui/structs/manual-default-impl-could-be-derived.stderr @@ -1,5 +1,5 @@ error: `impl Default` that could be derived - --> $DIR/manual-default-impl-could-be-derived.rs:33:1 + --> $DIR/manual-default-impl-could-be-derived.rs:34:1 | LL | / impl Default for D { LL | | fn default() -> Self { @@ -21,7 +21,24 @@ LL ~ #[derive(Default)] struct D { | error: `impl Default` that could be derived - --> $DIR/manual-default-impl-could-be-derived.rs:60:1 + --> $DIR/manual-default-impl-could-be-derived.rs:49:1 + | +LL | / impl Default for E { +LL | | fn default() -> Self { +LL | | E { +LL | | x: None, +LL | | } +LL | | } +LL | | } + | |_^ + | +help: you don't need to manually `impl Default`, you can derive it + | +LL ~ #[derive(Default)] struct E { + | + +error: `impl Default` that could be derived + --> $DIR/manual-default-impl-could-be-derived.rs:63:1 | LL | / impl Default for F { LL | | fn default() -> Self { @@ -36,5 +53,5 @@ LL ~ #[derive(Default)] enum F { LL ~ #[default] Unit, | -error: aborting due to 2 previous errors +error: aborting due to 3 previous errors From 9ef798c7e942edb9762369f8959e2580be006ca3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 11 Dec 2024 17:58:27 +0000 Subject: [PATCH 06/26] Detect when user uses default unit variant in default impl When checking if a field being defaulted to a unit variant, look at the (local) `Default` impl for that enum and if they match, consider that field defaultable for the purposes of the lint. --- .../src/default_could_be_derived.rs | 89 +++++++++++++++++-- ...manual-default-impl-could-be-derived.fixed | 15 +--- .../manual-default-impl-could-be-derived.rs | 22 ++--- ...anual-default-impl-could-be-derived.stderr | 19 +++- 4 files changed, 114 insertions(+), 31 deletions(-) diff --git a/compiler/rustc_lint/src/default_could_be_derived.rs b/compiler/rustc_lint/src/default_could_be_derived.rs index 0ffa11196165c..98741f58528c7 100644 --- a/compiler/rustc_lint/src/default_could_be_derived.rs +++ b/compiler/rustc_lint/src/default_could_be_derived.rs @@ -41,7 +41,8 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { let hir::ItemKind::Impl(data) = item.kind else { return }; let Some(trait_ref) = data.of_trait else { return }; let Res::Def(DefKind::Trait, def_id) = trait_ref.path.res else { return }; - if Some(def_id) != cx.tcx.get_diagnostic_item(sym::Default) { + let Some(default_def_id) = cx.tcx.get_diagnostic_item(sym::Default) else { return }; + if Some(def_id) != Some(default_def_id) { return; } if cx.tcx.has_attr(def_id, sym::automatically_derived) { @@ -137,6 +138,9 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { // - `val` is `0` // - `val` is `false` fn check_expr(tcx: TyCtxt<'_>, kind: hir::ExprKind<'_>) -> bool { + let Some(default_def_id) = tcx.get_diagnostic_item(sym::Default) else { + return false; + }; match kind { hir::ExprKind::Lit(spanned_lit) => match spanned_lit.node { LitKind::Int(val, _) if val == 0 => true, // field: 0, @@ -155,15 +159,84 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { hir::ExprKind::Path(hir::QPath::Resolved(_, path)) if let Res::Def( DefKind::Ctor(CtorOf::Variant, CtorKind::Const), - def_id, - ) = path.res - && let def_id = tcx.parent(def_id) // From Ctor to variant - && tcx.is_lang_item(def_id, hir::LangItem::OptionNone) => + ctor_def_id, + ) = path.res => { // FIXME: We should use a better check where we explore existing - // `impl Default for def_id` of the found type and see compare them - // against what we have here. For now, special case `Option::None`. - true + // `impl Default for def_id` of the found type when `def_id` is not + // local and see compare them against what we have here. For now, + // we special case `Option::None` and only check unit variants of + // local `Default` impls. + let var_def_id = tcx.parent(ctor_def_id); // From Ctor to variant + + // We explicitly check for `Option::::None`. If `Option` was + // local, it would be accounted by the logic further down, but + // because the analysis uses purely the HIR, that doesn't work + // accross crates. + // + // field: None, + let mut found = + tcx.is_lang_item(var_def_id, hir::LangItem::OptionNone); + + // Look at the local `impl Default for ty` of the field's `ty`. + let ty_def_id = tcx.parent(var_def_id); // From variant to enum + let ty = tcx.type_of(ty_def_id).instantiate_identity(); + tcx.for_each_relevant_impl(default_def_id, ty, |impl_did| { + let hir = tcx.hir(); + let Some(hir::Node::Item(impl_item)) = + hir.get_if_local(impl_did) + else { + return; + }; + let hir::ItemKind::Impl(impl_item) = impl_item.kind else { + return; + }; + for assoc in impl_item.items { + let hir::AssocItemKind::Fn { has_self: false } = assoc.kind + else { + continue; + }; + if assoc.ident.name != kw::Default { + continue; + } + let assoc = hir.impl_item(assoc.id); + let hir::ImplItemKind::Fn(_ty, body) = assoc.kind else { + continue; + }; + let body = hir.body(body); + let hir::ExprKind::Block( + hir::Block { stmts: [], expr: Some(expr), .. }, + None, + ) = body.value.kind + else { + continue; + }; + // Look at a specific implementation of `Default::default()` + // for their content and see if they are requivalent to what + // the user wrote in their manual `impl` for a given field. + match expr.kind { + hir::ExprKind::Path(hir::QPath::Resolved(_, path)) + if let Res::Def( + DefKind::Ctor(CtorOf::Variant, CtorKind::Const), + orig_def_id, + ) = path.res => + { + // We found + // + // field: Foo::Unit, + // + // and + // + // impl Default for Foo { + // fn default() -> Foo { Foo::Unit } + // } + found |= orig_def_id == ctor_def_id + } + _ => {} + } + } + }); + found } _ => false, } diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.fixed b/tests/ui/structs/manual-default-impl-could-be-derived.fixed index ebb0d61e02c3d..19e57d8484aca 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.fixed +++ b/tests/ui/structs/manual-default-impl-could-be-derived.fixed @@ -47,17 +47,10 @@ // Comparison of `impl` *fields* with their `Default::default()` bodies. -// struct G { -// f: F, -// } +#[derive(Default)] struct G { + f: F, +} -// impl Default for G { -// fn default() -> Self { -// G { -// f: F::Unit, -// } -// } -// } fn main() { // let _ = A::default(); @@ -66,5 +59,5 @@ fn main() { let _ = D::default(); let _ = E::default(); let _ = F::default(); -// let _ = G::default(); + let _ = G::default(); } diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.rs b/tests/ui/structs/manual-default-impl-could-be-derived.rs index a5909a4bfa48f..20c43d62bd6d4 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.rs +++ b/tests/ui/structs/manual-default-impl-could-be-derived.rs @@ -67,17 +67,17 @@ impl Default for F { //~ ERROR } // Comparison of `impl` *fields* with their `Default::default()` bodies. -// struct G { -// f: F, -// } +struct G { + f: F, +} -// impl Default for G { -// fn default() -> Self { -// G { -// f: F::Unit, -// } -// } -// } +impl Default for G { //~ ERROR + fn default() -> Self { + G { + f: F::Unit, + } + } +} fn main() { // let _ = A::default(); @@ -86,5 +86,5 @@ fn main() { let _ = D::default(); let _ = E::default(); let _ = F::default(); -// let _ = G::default(); + let _ = G::default(); } diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.stderr b/tests/ui/structs/manual-default-impl-could-be-derived.stderr index e712fa8d5006c..8f70ee6a3b597 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.stderr +++ b/tests/ui/structs/manual-default-impl-could-be-derived.stderr @@ -53,5 +53,22 @@ LL ~ #[derive(Default)] enum F { LL ~ #[default] Unit, | -error: aborting due to 3 previous errors +error: `impl Default` that could be derived + --> $DIR/manual-default-impl-could-be-derived.rs:74:1 + | +LL | / impl Default for G { +LL | | fn default() -> Self { +LL | | G { +LL | | f: F::Unit, +LL | | } +LL | | } +LL | | } + | |_^ + | +help: you don't need to manually `impl Default`, you can derive it + | +LL ~ #[derive(Default)] struct G { + | + +error: aborting due to 4 previous errors From 9ab4f0c8100fd411f786de4425db4f2d14949bda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 11 Dec 2024 18:18:54 +0000 Subject: [PATCH 07/26] Use `#[derive(Default)]` for `std::option::Option` The impl predates the introduction of `#[default]`, but now we *can* derive it, as it doesn't introduce unnecessary `Default` bounds on enum type parameters. --- library/core/src/option.rs | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/library/core/src/option.rs b/library/core/src/option.rs index f4ac7af63961b..6f06d327734d9 100644 --- a/library/core/src/option.rs +++ b/library/core/src/option.rs @@ -564,7 +564,7 @@ use crate::{cmp, convert, hint, mem, slice}; /// The `Option` type. See [the module level documentation](self) for more. #[doc(search_unbox)] -#[derive(Copy, Eq, Debug, Hash)] +#[derive(Copy, Eq, Debug, Hash, Default)] #[rustc_diagnostic_item = "Option"] #[lang = "Option"] #[stable(feature = "rust1", since = "1.0.0")] @@ -573,6 +573,7 @@ pub enum Option { /// No value. #[lang = "None"] #[stable(feature = "rust1", since = "1.0.0")] + #[default] None, /// Some value of type `T`. #[lang = "Some"] @@ -2044,22 +2045,6 @@ where } } -#[stable(feature = "rust1", since = "1.0.0")] -impl Default for Option { - /// Returns [`None`][Option::None]. - /// - /// # Examples - /// - /// ``` - /// let opt: Option = Option::default(); - /// assert!(opt.is_none()); - /// ``` - #[inline] - fn default() -> Option { - None - } -} - #[stable(feature = "rust1", since = "1.0.0")] impl IntoIterator for Option { type Item = T; From 2c7e43fbcb6d1ddb750eccca4aaba650baa3647a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 11 Dec 2024 18:21:23 +0000 Subject: [PATCH 08/26] Also lint enums with type parameters with unit variant default --- compiler/rustc_lint/src/default_could_be_derived.rs | 8 ++++++-- .../ui/structs/manual-default-impl-could-be-derived.rs | 10 +++++----- .../manual-default-impl-could-be-derived.stderr | 4 ++-- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_lint/src/default_could_be_derived.rs b/compiler/rustc_lint/src/default_could_be_derived.rs index 98741f58528c7..7b6a3a4b4ed33 100644 --- a/compiler/rustc_lint/src/default_could_be_derived.rs +++ b/compiler/rustc_lint/src/default_could_be_derived.rs @@ -50,9 +50,13 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { } let hir_self_ty = data.self_ty; let hir::TyKind::Path(hir::QPath::Resolved(_, path)) = hir_self_ty.kind else { return }; - let Res::Def(_, type_def_id) = path.res else { return }; + let Res::Def(def_kind, type_def_id) = path.res else { return }; let generics = cx.tcx.generics_of(type_def_id); - if !generics.own_params.is_empty() { + if !generics.own_params.is_empty() && def_kind != DefKind::Enum { + // For enums, `#[derive(Default)]` forces you to select a unit variant to avoid + // "imperfect derives", unnecessary bounds on type parameters, so even if the enum has + // type parameters we can still lint on the manual impl if the return is a unit + // variant. return; } // We have a manual `impl Default for Ty {}` item, where `Ty` has no type parameters. diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.rs b/tests/ui/structs/manual-default-impl-could-be-derived.rs index 20c43d62bd6d4..9102c7bf2a102 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.rs +++ b/tests/ui/structs/manual-default-impl-could-be-derived.rs @@ -55,12 +55,12 @@ impl Default for E { //~ ERROR } // Detection of unit variant ctors that could have been marked `#[default]`. -enum F { +enum F { Unit, - Tuple(i32), + Tuple(T), } -impl Default for F { //~ ERROR +impl Default for F { //~ ERROR fn default() -> Self { F::Unit } @@ -68,7 +68,7 @@ impl Default for F { //~ ERROR // Comparison of `impl` *fields* with their `Default::default()` bodies. struct G { - f: F, + f: F, } impl Default for G { //~ ERROR @@ -85,6 +85,6 @@ fn main() { // let _ = C::default(); let _ = D::default(); let _ = E::default(); - let _ = F::default(); + let _ = F::::default(); let _ = G::default(); } diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.stderr b/tests/ui/structs/manual-default-impl-could-be-derived.stderr index 8f70ee6a3b597..097c600680c0d 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.stderr +++ b/tests/ui/structs/manual-default-impl-could-be-derived.stderr @@ -40,7 +40,7 @@ LL ~ #[derive(Default)] struct E { error: `impl Default` that could be derived --> $DIR/manual-default-impl-could-be-derived.rs:63:1 | -LL | / impl Default for F { +LL | / impl Default for F { LL | | fn default() -> Self { LL | | F::Unit LL | | } @@ -49,7 +49,7 @@ LL | | } | help: you don't need to manually `impl Default`, you can derive it | -LL ~ #[derive(Default)] enum F { +LL ~ #[derive(Default)] enum F { LL ~ #[default] Unit, | From c35fd36ac9b06a8737e61a7a2b01b95325ec21a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 11 Dec 2024 18:51:43 +0000 Subject: [PATCH 09/26] Lint against manual `Default` impl if all fields are defaulted --- .../src/default_could_be_derived.rs | 43 ++++++++++++++++++- ...manual-default-impl-could-be-derived.fixed | 17 ++++++-- .../manual-default-impl-could-be-derived.rs | 16 +++++++ ...anual-default-impl-could-be-derived.stderr | 32 +++++++++++--- 4 files changed, 98 insertions(+), 10 deletions(-) diff --git a/compiler/rustc_lint/src/default_could_be_derived.rs b/compiler/rustc_lint/src/default_could_be_derived.rs index 7b6a3a4b4ed33..2bf149d93f73f 100644 --- a/compiler/rustc_lint/src/default_could_be_derived.rs +++ b/compiler/rustc_lint/src/default_could_be_derived.rs @@ -38,6 +38,7 @@ declare_lint_pass!(DefaultCouldBeDerived => [DEFAULT_COULD_BE_DERIVED]); impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'tcx>) { + let hir = cx.tcx.hir(); let hir::ItemKind::Impl(data) = item.kind else { return }; let Some(trait_ref) = data.of_trait else { return }; let Res::Def(DefKind::Trait, def_id) = trait_ref.path.res else { return }; @@ -51,6 +52,47 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { let hir_self_ty = data.self_ty; let hir::TyKind::Path(hir::QPath::Resolved(_, path)) = hir_self_ty.kind else { return }; let Res::Def(def_kind, type_def_id) = path.res else { return }; + match hir.get_if_local(type_def_id) { + Some(hir::Node::Item(hir::Item { + kind: + hir::ItemKind::Struct(hir::VariantData::Struct { fields, recovered: _ }, _generics), + .. + })) => { + if fields.iter().all(|f| f.default.is_some()) { + cx.tcx.node_span_lint( + DEFAULT_COULD_BE_DERIVED, + item.hir_id(), + item.span, + |diag| { + diag.primary_message("`impl Default` that could be derived"); + diag.span_label( + cx.tcx.def_span(type_def_id), + "all the fields in this struct have default values", + ); + for field in &fields[..] { + if let Some(anon) = field.default { + diag.span_label(anon.span, ""); + } + } + diag.multipart_suggestion_verbose( + "to avoid divergence in behavior between `Struct { .. }` and \ + `::default()`, derive the `Default`", + vec![ + ( + cx.tcx.def_span(type_def_id).shrink_to_lo(), + "#[derive(Default)] ".to_string(), + ), + (item.span, String::new()), + ], + Applicability::MachineApplicable, + ); + }, + ); + return; + } + } + _ => {} + } let generics = cx.tcx.generics_of(type_def_id); if !generics.own_params.is_empty() && def_kind != DefKind::Enum { // For enums, `#[derive(Default)]` forces you to select a unit variant to avoid @@ -61,7 +103,6 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { } // We have a manual `impl Default for Ty {}` item, where `Ty` has no type parameters. - let hir = cx.tcx.hir(); for assoc in data.items { let hir::AssocItemKind::Fn { has_self: false } = assoc.kind else { continue }; if assoc.ident.name != kw::Default { diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.fixed b/tests/ui/structs/manual-default-impl-could-be-derived.fixed index 19e57d8484aca..80fa65c98e369 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.fixed +++ b/tests/ui/structs/manual-default-impl-could-be-derived.fixed @@ -2,6 +2,7 @@ //@ run-rustfix #![allow(dead_code)] #![deny(default_could_be_derived)] +#![feature(default_field_values)] // #[derive(Debug)] // struct A; @@ -40,15 +41,22 @@ // Detection of unit variant ctors that could have been marked `#[default]`. -#[derive(Default)] enum F { +#[derive(Default)] enum F { #[default] Unit, - Tuple(i32), + Tuple(T), } // Comparison of `impl` *fields* with their `Default::default()` bodies. #[derive(Default)] struct G { - f: F, + f: F, +} + + +// Always lint against manual `Default` impl if all fields are defaulted. +#[derive(PartialEq, Debug)] +#[derive(Default)] struct H { + x: i32 = 101, } @@ -58,6 +66,7 @@ fn main() { // let _ = C::default(); let _ = D::default(); let _ = E::default(); - let _ = F::default(); + let _ = F::::default(); let _ = G::default(); + assert_eq!(H::default(), H { .. }); } diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.rs b/tests/ui/structs/manual-default-impl-could-be-derived.rs index 9102c7bf2a102..2ac56bf62a29f 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.rs +++ b/tests/ui/structs/manual-default-impl-could-be-derived.rs @@ -2,6 +2,7 @@ //@ run-rustfix #![allow(dead_code)] #![deny(default_could_be_derived)] +#![feature(default_field_values)] // #[derive(Debug)] // struct A; @@ -79,6 +80,20 @@ impl Default for G { //~ ERROR } } +// Always lint against manual `Default` impl if all fields are defaulted. +#[derive(PartialEq, Debug)] +struct H { + x: i32 = 101, +} + +impl Default for H { //~ ERROR + fn default() -> Self { + H { + x: 1, + } + } +} + fn main() { // let _ = A::default(); // let _ = B::default(); @@ -87,4 +102,5 @@ fn main() { let _ = E::default(); let _ = F::::default(); let _ = G::default(); + assert_eq!(H::default(), H { .. }); } diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.stderr b/tests/ui/structs/manual-default-impl-could-be-derived.stderr index 097c600680c0d..cc825bfae537d 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.stderr +++ b/tests/ui/structs/manual-default-impl-could-be-derived.stderr @@ -1,5 +1,5 @@ error: `impl Default` that could be derived - --> $DIR/manual-default-impl-could-be-derived.rs:34:1 + --> $DIR/manual-default-impl-could-be-derived.rs:35:1 | LL | / impl Default for D { LL | | fn default() -> Self { @@ -21,7 +21,7 @@ LL ~ #[derive(Default)] struct D { | error: `impl Default` that could be derived - --> $DIR/manual-default-impl-could-be-derived.rs:49:1 + --> $DIR/manual-default-impl-could-be-derived.rs:50:1 | LL | / impl Default for E { LL | | fn default() -> Self { @@ -38,7 +38,7 @@ LL ~ #[derive(Default)] struct E { | error: `impl Default` that could be derived - --> $DIR/manual-default-impl-could-be-derived.rs:63:1 + --> $DIR/manual-default-impl-could-be-derived.rs:64:1 | LL | / impl Default for F { LL | | fn default() -> Self { @@ -54,7 +54,7 @@ LL ~ #[default] Unit, | error: `impl Default` that could be derived - --> $DIR/manual-default-impl-could-be-derived.rs:74:1 + --> $DIR/manual-default-impl-could-be-derived.rs:75:1 | LL | / impl Default for G { LL | | fn default() -> Self { @@ -70,5 +70,27 @@ help: you don't need to manually `impl Default`, you can derive it LL ~ #[derive(Default)] struct G { | -error: aborting due to 4 previous errors +error: `impl Default` that could be derived + --> $DIR/manual-default-impl-could-be-derived.rs:89:1 + | +LL | struct H { + | -------- all the fields in this struct have default values +LL | x: i32 = 101, + | --- +... +LL | / impl Default for H { +LL | | fn default() -> Self { +LL | | H { +LL | | x: 1, +LL | | } +LL | | } +LL | | } + | |_^ + | +help: to avoid divergence in behavior between `Struct { .. }` and `::default()`, derive the `Default` + | +LL ~ #[derive(Default)] struct H { + | + +error: aborting due to 5 previous errors From e5a4a88d9262c6d20889266224328332990ebabf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 11 Dec 2024 20:22:28 +0000 Subject: [PATCH 10/26] Lint structs with default field values and manual `Default` --- .../src/default_could_be_derived.rs | 43 ++++++++++++++++--- ...manual-default-impl-could-be-derived.fixed | 8 ++++ .../manual-default-impl-could-be-derived.rs | 16 +++++++ ...anual-default-impl-could-be-derived.stderr | 28 +++++++++++- 4 files changed, 87 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_lint/src/default_could_be_derived.rs b/compiler/rustc_lint/src/default_could_be_derived.rs index 2bf149d93f73f..3055f8809bf1d 100644 --- a/compiler/rustc_lint/src/default_could_be_derived.rs +++ b/compiler/rustc_lint/src/default_could_be_derived.rs @@ -58,21 +58,52 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { hir::ItemKind::Struct(hir::VariantData::Struct { fields, recovered: _ }, _generics), .. })) => { - if fields.iter().all(|f| f.default.is_some()) { + let fields_with_default_value: Vec<_> = + fields.iter().filter_map(|f| f.default).collect(); + let fields_with_default_impl: Vec<_> = fields + .iter() + .filter_map(|f| match (f.ty.kind, f.default) { + (hir::TyKind::Path(hir::QPath::Resolved(_, path)), None) + if let Some(def_id) = path.res.opt_def_id() + && let DefKind::Struct | DefKind::Enum = + cx.tcx.def_kind(def_id) => + { + let ty = cx.tcx.type_of(def_id).instantiate_identity(); + let mut count = 0; + cx.tcx.for_each_relevant_impl(default_def_id, ty, |_| count += 1); + if count > 0 { Some(f.ty.span) } else { None } + } + _ => None, + }) + .collect(); + if !fields_with_default_value.is_empty() + && fields.len() + == fields_with_default_value.len() + fields_with_default_impl.len() + { cx.tcx.node_span_lint( DEFAULT_COULD_BE_DERIVED, item.hir_id(), item.span, |diag| { diag.primary_message("`impl Default` that could be derived"); + let msg = match ( + !fields_with_default_value.is_empty(), + !fields_with_default_impl.is_empty(), + ) { + (true, true) => "default values or a type that impls `Default`", + (true, false) => "default values", + (false, true) => "a type that impls `Default`", + (false, false) => unreachable!(), + }; diag.span_label( cx.tcx.def_span(type_def_id), - "all the fields in this struct have default values", + format!("all the fields in this struct have {msg}"), ); - for field in &fields[..] { - if let Some(anon) = field.default { - diag.span_label(anon.span, ""); - } + for anon in fields_with_default_value { + diag.span_label(anon.span, "default value"); + } + for field in fields_with_default_impl { + diag.span_label(field, "implements `Default`"); } diag.multipart_suggestion_verbose( "to avoid divergence in behavior between `Struct { .. }` and \ diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.fixed b/tests/ui/structs/manual-default-impl-could-be-derived.fixed index 80fa65c98e369..be1e1d3e55bb3 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.fixed +++ b/tests/ui/structs/manual-default-impl-could-be-derived.fixed @@ -60,6 +60,14 @@ } +// Always lint against manual `Default` impl if all fields are defaulted. +#[derive(PartialEq, Debug)] +#[derive(Default)] struct I { + x: i32 = 101, + y: Option, +} + + fn main() { // let _ = A::default(); // let _ = B::default(); diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.rs b/tests/ui/structs/manual-default-impl-could-be-derived.rs index 2ac56bf62a29f..59c69b8a9ea16 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.rs +++ b/tests/ui/structs/manual-default-impl-could-be-derived.rs @@ -94,6 +94,22 @@ impl Default for H { //~ ERROR } } +// Always lint against manual `Default` impl if all fields are defaulted. +#[derive(PartialEq, Debug)] +struct I { + x: i32 = 101, + y: Option, +} + +impl Default for I { //~ ERROR + fn default() -> Self { + I { + x: 1, + y: None, + } + } +} + fn main() { // let _ = A::default(); // let _ = B::default(); diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.stderr b/tests/ui/structs/manual-default-impl-could-be-derived.stderr index cc825bfae537d..194d7bd24d71e 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.stderr +++ b/tests/ui/structs/manual-default-impl-could-be-derived.stderr @@ -76,7 +76,7 @@ error: `impl Default` that could be derived LL | struct H { | -------- all the fields in this struct have default values LL | x: i32 = 101, - | --- + | --- default value ... LL | / impl Default for H { LL | | fn default() -> Self { @@ -92,5 +92,29 @@ help: to avoid divergence in behavior between `Struct { .. }` and ` $DIR/manual-default-impl-could-be-derived.rs:104:1 + | +LL | struct I { + | -------- all the fields in this struct have default values or a type that impls `Default` +LL | x: i32 = 101, + | --- default value +LL | y: Option, + | ----------- implements `Default` +... +LL | / impl Default for I { +LL | | fn default() -> Self { +LL | | I { +LL | | x: 1, +... | +LL | | } +LL | | } + | |_^ + | +help: to avoid divergence in behavior between `Struct { .. }` and `::default()`, derive the `Default` + | +LL ~ #[derive(Default)] struct I { + | + +error: aborting due to 6 previous errors From 34ecbb97557bf33d6675fbb7ecfed5231030e55a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 11 Dec 2024 23:48:28 +0000 Subject: [PATCH 11/26] Extend `Default` lint to tuple-structs --- .../src/default_could_be_derived.rs | 229 ++++++++++-------- ...manual-default-impl-could-be-derived.fixed | 22 +- .../manual-default-impl-could-be-derived.rs | 32 +-- ...anual-default-impl-could-be-derived.stderr | 40 ++- 4 files changed, 183 insertions(+), 140 deletions(-) diff --git a/compiler/rustc_lint/src/default_could_be_derived.rs b/compiler/rustc_lint/src/default_could_be_derived.rs index 3055f8809bf1d..7745c06d05f87 100644 --- a/compiler/rustc_lint/src/default_could_be_derived.rs +++ b/compiler/rustc_lint/src/default_could_be_derived.rs @@ -213,110 +213,6 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { // - `val` is `Default::default()` // - `val` is `0` // - `val` is `false` - fn check_expr(tcx: TyCtxt<'_>, kind: hir::ExprKind<'_>) -> bool { - let Some(default_def_id) = tcx.get_diagnostic_item(sym::Default) else { - return false; - }; - match kind { - hir::ExprKind::Lit(spanned_lit) => match spanned_lit.node { - LitKind::Int(val, _) if val == 0 => true, // field: 0, - LitKind::Bool(false) => true, // field: false, - _ => false, - }, - hir::ExprKind::Call(expr, []) - if let hir::ExprKind::Path(hir::QPath::Resolved(_, path)) = - expr.kind - && let Some(def_id) = path.res.opt_def_id() - && tcx.is_diagnostic_item(sym::default_fn, def_id) => - { - // field: Default::default(), - true - } - hir::ExprKind::Path(hir::QPath::Resolved(_, path)) - if let Res::Def( - DefKind::Ctor(CtorOf::Variant, CtorKind::Const), - ctor_def_id, - ) = path.res => - { - // FIXME: We should use a better check where we explore existing - // `impl Default for def_id` of the found type when `def_id` is not - // local and see compare them against what we have here. For now, - // we special case `Option::None` and only check unit variants of - // local `Default` impls. - let var_def_id = tcx.parent(ctor_def_id); // From Ctor to variant - - // We explicitly check for `Option::::None`. If `Option` was - // local, it would be accounted by the logic further down, but - // because the analysis uses purely the HIR, that doesn't work - // accross crates. - // - // field: None, - let mut found = - tcx.is_lang_item(var_def_id, hir::LangItem::OptionNone); - - // Look at the local `impl Default for ty` of the field's `ty`. - let ty_def_id = tcx.parent(var_def_id); // From variant to enum - let ty = tcx.type_of(ty_def_id).instantiate_identity(); - tcx.for_each_relevant_impl(default_def_id, ty, |impl_did| { - let hir = tcx.hir(); - let Some(hir::Node::Item(impl_item)) = - hir.get_if_local(impl_did) - else { - return; - }; - let hir::ItemKind::Impl(impl_item) = impl_item.kind else { - return; - }; - for assoc in impl_item.items { - let hir::AssocItemKind::Fn { has_self: false } = assoc.kind - else { - continue; - }; - if assoc.ident.name != kw::Default { - continue; - } - let assoc = hir.impl_item(assoc.id); - let hir::ImplItemKind::Fn(_ty, body) = assoc.kind else { - continue; - }; - let body = hir.body(body); - let hir::ExprKind::Block( - hir::Block { stmts: [], expr: Some(expr), .. }, - None, - ) = body.value.kind - else { - continue; - }; - // Look at a specific implementation of `Default::default()` - // for their content and see if they are requivalent to what - // the user wrote in their manual `impl` for a given field. - match expr.kind { - hir::ExprKind::Path(hir::QPath::Resolved(_, path)) - if let Res::Def( - DefKind::Ctor(CtorOf::Variant, CtorKind::Const), - orig_def_id, - ) = path.res => - { - // We found - // - // field: Foo::Unit, - // - // and - // - // impl Default for Foo { - // fn default() -> Foo { Foo::Unit } - // } - found |= orig_def_id == ctor_def_id - } - _ => {} - } - } - }); - found - } - _ => false, - } - } if fields.iter().all(|f| check_expr(cx.tcx, f.expr.kind)) { cx.tcx.node_span_lint( DEFAULT_COULD_BE_DERIVED, @@ -339,8 +235,133 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { ); } } + hir::ExprKind::Call(expr, args) => { + if let hir::ExprKind::Path(hir::QPath::Resolved(_, path)) = expr.kind + && let Res::Def(DefKind::Ctor(CtorOf::Struct, CtorKind::Fn), ctor_def_id) = + path.res + { + let type_def_id = cx.tcx.parent(ctor_def_id); // From Ctor to struct + if args.iter().all(|expr| check_expr(cx.tcx, expr.kind)) { + cx.tcx.node_span_lint( + DEFAULT_COULD_BE_DERIVED, + item.hir_id(), + item.span, + |diag| { + diag.primary_message("`impl Default` that could be derived"); + diag.multipart_suggestion_verbose( + "you don't need to manually `impl Default`, you can derive it", + vec![ + ( + cx.tcx.def_span(type_def_id).shrink_to_lo(), + "#[derive(Default)] ".to_string(), + ), + (item.span, String::new()), + ], + Applicability::MachineApplicable, + ); + }, + ); + } + } + } _ => {} } } } } + +fn check_expr(tcx: TyCtxt<'_>, kind: hir::ExprKind<'_>) -> bool { + let Some(default_def_id) = tcx.get_diagnostic_item(sym::Default) else { + return false; + }; + match kind { + hir::ExprKind::Lit(spanned_lit) => match spanned_lit.node { + LitKind::Int(val, _) if val == 0 => true, // field: 0, + LitKind::Bool(false) => true, // field: false, + _ => false, + }, + hir::ExprKind::Call(expr, []) + if let hir::ExprKind::Path(hir::QPath::Resolved(_, path)) = expr.kind + && let Some(def_id) = path.res.opt_def_id() + && tcx.is_diagnostic_item(sym::default_fn, def_id) => + { + // field: Default::default(), + true + } + hir::ExprKind::Path(hir::QPath::Resolved(_, path)) + if let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Const), ctor_def_id) = + path.res => + { + // FIXME: We should use a better check where we explore existing + // `impl Default for def_id` of the found type when `def_id` is not + // local and see compare them against what we have here. For now, + // we special case `Option::None` and only check unit variants of + // local `Default` impls. + let var_def_id = tcx.parent(ctor_def_id); // From Ctor to variant + + // We explicitly check for `Option::::None`. If `Option` was + // local, it would be accounted by the logic further down, but + // because the analysis uses purely the HIR, that doesn't work + // accross crates. + // + // field: None, + let mut found = tcx.is_lang_item(var_def_id, hir::LangItem::OptionNone); + + // Look at the local `impl Default for ty` of the field's `ty`. + let ty_def_id = tcx.parent(var_def_id); // From variant to enum + let ty = tcx.type_of(ty_def_id).instantiate_identity(); + tcx.for_each_relevant_impl(default_def_id, ty, |impl_did| { + let hir = tcx.hir(); + let Some(hir::Node::Item(impl_item)) = hir.get_if_local(impl_did) else { + return; + }; + let hir::ItemKind::Impl(impl_item) = impl_item.kind else { + return; + }; + for assoc in impl_item.items { + let hir::AssocItemKind::Fn { has_self: false } = assoc.kind else { + continue; + }; + if assoc.ident.name != kw::Default { + continue; + } + let assoc = hir.impl_item(assoc.id); + let hir::ImplItemKind::Fn(_ty, body) = assoc.kind else { + continue; + }; + let body = hir.body(body); + let hir::ExprKind::Block(hir::Block { stmts: [], expr: Some(expr), .. }, None) = + body.value.kind + else { + continue; + }; + // Look at a specific implementation of `Default::default()` + // for their content and see if they are requivalent to what + // the user wrote in their manual `impl` for a given field. + match expr.kind { + hir::ExprKind::Path(hir::QPath::Resolved(_, path)) + if let Res::Def( + DefKind::Ctor(CtorOf::Variant, CtorKind::Const), + orig_def_id, + ) = path.res => + { + // We found + // + // field: Foo::Unit, + // + // and + // + // impl Default for Foo { + // fn default() -> Foo { Foo::Unit } + // } + found |= orig_def_id == ctor_def_id + } + _ => {} + } + } + }); + found + } + _ => false, + } +} diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.fixed b/tests/ui/structs/manual-default-impl-could-be-derived.fixed index be1e1d3e55bb3..56915267c98d3 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.fixed +++ b/tests/ui/structs/manual-default-impl-could-be-derived.fixed @@ -11,20 +11,14 @@ // fn default() -> Self { A } // } // -// #[derive(Debug)] -// struct B(Option); -// -// impl Default for B { -// fn default() -> Self { B(Default::default()) } -// } -// -// #[derive(Debug)] -// struct C(Option); -// -// impl Default for C { -// fn default() -> Self { C(None) } -// } -// +#[derive(Debug)] +#[derive(Default)] struct B(Option); + + +#[derive(Debug)] +#[derive(Default)] struct C(Option); + + // Explicit check against numeric literals and `Default::default()` calls. #[derive(Default)] struct D { diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.rs b/tests/ui/structs/manual-default-impl-could-be-derived.rs index 59c69b8a9ea16..79e18e6153fd3 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.rs +++ b/tests/ui/structs/manual-default-impl-could-be-derived.rs @@ -11,20 +11,20 @@ // fn default() -> Self { A } // } // -// #[derive(Debug)] -// struct B(Option); -// -// impl Default for B { -// fn default() -> Self { B(Default::default()) } -// } -// -// #[derive(Debug)] -// struct C(Option); -// -// impl Default for C { -// fn default() -> Self { C(None) } -// } -// +#[derive(Debug)] +struct B(Option); + +impl Default for B { //~ ERROR + fn default() -> Self { B(Default::default()) } +} + +#[derive(Debug)] +struct C(Option); + +impl Default for C { //~ ERROR + fn default() -> Self { C(None) } +} + // Explicit check against numeric literals and `Default::default()` calls. struct D { @@ -112,8 +112,8 @@ impl Default for I { //~ ERROR fn main() { // let _ = A::default(); -// let _ = B::default(); -// let _ = C::default(); + let _ = B::default(); + let _ = C::default(); let _ = D::default(); let _ = E::default(); let _ = F::::default(); diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.stderr b/tests/ui/structs/manual-default-impl-could-be-derived.stderr index 194d7bd24d71e..7ad8e26dd7a9e 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.stderr +++ b/tests/ui/structs/manual-default-impl-could-be-derived.stderr @@ -1,3 +1,36 @@ +error: `impl Default` that could be derived + --> $DIR/manual-default-impl-could-be-derived.rs:17:1 + | +LL | / impl Default for B { +LL | | fn default() -> Self { B(Default::default()) } +LL | | } + | |_^ + | +note: the lint level is defined here + --> $DIR/manual-default-impl-could-be-derived.rs:4:9 + | +LL | #![deny(default_could_be_derived)] + | ^^^^^^^^^^^^^^^^^^^^^^^^ +help: you don't need to manually `impl Default`, you can derive it + | +LL - struct B(Option); +LL + #[derive(Default)] struct B(Option); + | + +error: `impl Default` that could be derived + --> $DIR/manual-default-impl-could-be-derived.rs:24:1 + | +LL | / impl Default for C { +LL | | fn default() -> Self { C(None) } +LL | | } + | |_^ + | +help: you don't need to manually `impl Default`, you can derive it + | +LL - struct C(Option); +LL + #[derive(Default)] struct C(Option); + | + error: `impl Default` that could be derived --> $DIR/manual-default-impl-could-be-derived.rs:35:1 | @@ -10,11 +43,6 @@ LL | | } LL | | } | |_^ | -note: the lint level is defined here - --> $DIR/manual-default-impl-could-be-derived.rs:4:9 - | -LL | #![deny(default_could_be_derived)] - | ^^^^^^^^^^^^^^^^^^^^^^^^ help: you don't need to manually `impl Default`, you can derive it | LL ~ #[derive(Default)] struct D { @@ -116,5 +144,5 @@ help: to avoid divergence in behavior between `Struct { .. }` and ` Date: Thu, 12 Dec 2024 00:27:41 +0000 Subject: [PATCH 12/26] Extend support to unit-structs and add comments --- .../src/default_could_be_derived.rs | 49 +++++++++++++++++++ ...manual-default-impl-could-be-derived.fixed | 17 +++---- .../manual-default-impl-could-be-derived.rs | 16 +++--- ...anual-default-impl-could-be-derived.stderr | 22 +++++++-- 4 files changed, 82 insertions(+), 22 deletions(-) diff --git a/compiler/rustc_lint/src/default_could_be_derived.rs b/compiler/rustc_lint/src/default_could_be_derived.rs index 7745c06d05f87..da69b500d1469 100644 --- a/compiler/rustc_lint/src/default_could_be_derived.rs +++ b/compiler/rustc_lint/src/default_could_be_derived.rs @@ -242,6 +242,20 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { { let type_def_id = cx.tcx.parent(ctor_def_id); // From Ctor to struct if args.iter().all(|expr| check_expr(cx.tcx, expr.kind)) { + // We have a struct literal + // + // struct Foo(Type); + // + // impl Default for Foo { + // fn default() -> Foo { + // Foo(val) + // } + // } + // + // We suggest #[derive(Default)] if + // - `val` is `Default::default()` + // - `val` is `0` + // - `val` is `false` cx.tcx.node_span_lint( DEFAULT_COULD_BE_DERIVED, item.hir_id(), @@ -264,6 +278,41 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { } } } + hir::ExprKind::Path(hir::QPath::Resolved(_, path)) + if let Res::Def(DefKind::Ctor(CtorOf::Struct, CtorKind::Const), _) = + path.res => + { + // We have a struct literal + // + // struct Foo; + // + // impl Default for Foo { + // fn default() -> Foo { + // Foo + // } + // } + // + // We always suggest `#[derive(Default)]`. + cx.tcx.node_span_lint( + DEFAULT_COULD_BE_DERIVED, + item.hir_id(), + item.span, + |diag| { + diag.primary_message("`impl Default` that could be derived"); + diag.multipart_suggestion_verbose( + "you don't need to manually `impl Default`, you can derive it", + vec![ + ( + cx.tcx.def_span(type_def_id).shrink_to_lo(), + "#[derive(Default)] ".to_string(), + ), + (item.span, String::new()), + ], + Applicability::MachineApplicable, + ); + }, + ); + } _ => {} } } diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.fixed b/tests/ui/structs/manual-default-impl-could-be-derived.fixed index 56915267c98d3..922e14580b98b 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.fixed +++ b/tests/ui/structs/manual-default-impl-could-be-derived.fixed @@ -4,13 +4,10 @@ #![deny(default_could_be_derived)] #![feature(default_field_values)] -// #[derive(Debug)] -// struct A; -// -// impl Default for A { -// fn default() -> Self { A } -// } -// +#[derive(Debug)] +#[derive(Default)] struct A; + + #[derive(Debug)] #[derive(Default)] struct B(Option); @@ -63,9 +60,9 @@ fn main() { -// let _ = A::default(); -// let _ = B::default(); -// let _ = C::default(); + let _ = A::default(); + let _ = B::default(); + let _ = C::default(); let _ = D::default(); let _ = E::default(); let _ = F::::default(); diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.rs b/tests/ui/structs/manual-default-impl-could-be-derived.rs index 79e18e6153fd3..24f4adf071072 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.rs +++ b/tests/ui/structs/manual-default-impl-could-be-derived.rs @@ -4,13 +4,13 @@ #![deny(default_could_be_derived)] #![feature(default_field_values)] -// #[derive(Debug)] -// struct A; -// -// impl Default for A { -// fn default() -> Self { A } -// } -// +#[derive(Debug)] +struct A; + +impl Default for A { //~ ERROR + fn default() -> Self { A } +} + #[derive(Debug)] struct B(Option); @@ -111,7 +111,7 @@ impl Default for I { //~ ERROR } fn main() { -// let _ = A::default(); + let _ = A::default(); let _ = B::default(); let _ = C::default(); let _ = D::default(); diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.stderr b/tests/ui/structs/manual-default-impl-could-be-derived.stderr index 7ad8e26dd7a9e..eb13043a76025 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.stderr +++ b/tests/ui/structs/manual-default-impl-could-be-derived.stderr @@ -1,8 +1,8 @@ error: `impl Default` that could be derived - --> $DIR/manual-default-impl-could-be-derived.rs:17:1 + --> $DIR/manual-default-impl-could-be-derived.rs:10:1 | -LL | / impl Default for B { -LL | | fn default() -> Self { B(Default::default()) } +LL | / impl Default for A { +LL | | fn default() -> Self { A } LL | | } | |_^ | @@ -13,6 +13,20 @@ LL | #![deny(default_could_be_derived)] | ^^^^^^^^^^^^^^^^^^^^^^^^ help: you don't need to manually `impl Default`, you can derive it | +LL - struct A; +LL + #[derive(Default)] struct A; + | + +error: `impl Default` that could be derived + --> $DIR/manual-default-impl-could-be-derived.rs:17:1 + | +LL | / impl Default for B { +LL | | fn default() -> Self { B(Default::default()) } +LL | | } + | |_^ + | +help: you don't need to manually `impl Default`, you can derive it + | LL - struct B(Option); LL + #[derive(Default)] struct B(Option); | @@ -144,5 +158,5 @@ help: to avoid divergence in behavior between `Struct { .. }` and ` Date: Thu, 12 Dec 2024 18:55:30 +0000 Subject: [PATCH 13/26] Rework logic to make it work x-crate and remove special cases Store a mapping betweein the `DefId` for an `impl Default for Ty` and the `DefId` of either a Unit variant/struct or an fn with no arguments that is called within `::default()`. When linting `impl`s, if it is for `Default`, we evaluate the contents of their `fn default()`. If it is *only* an ADT literal for `Self` and every field is either a "known to be defaulted" value (`0` or `false`), an explicit `Default::default()` call or a call or path to the same "equivalent" `DefId` from that field's type's `Default::default()` implementation. --- .../src/default_could_be_derived.rs | 158 ++++++++---------- compiler/rustc_metadata/src/rmeta/decoder.rs | 6 + .../src/rmeta/decoder/cstore_impl.rs | 2 + compiler/rustc_metadata/src/rmeta/encoder.rs | 4 + compiler/rustc_metadata/src/rmeta/mod.rs | 1 + compiler/rustc_middle/src/query/mod.rs | 6 + compiler/rustc_middle/src/ty/context.rs | 54 +++++- ...manual-default-impl-could-be-derived.fixed | 30 ++++ .../manual-default-impl-could-be-derived.rs | 44 +++++ ...anual-default-impl-could-be-derived.stderr | 36 +++- 10 files changed, 253 insertions(+), 88 deletions(-) diff --git a/compiler/rustc_lint/src/default_could_be_derived.rs b/compiler/rustc_lint/src/default_could_be_derived.rs index da69b500d1469..054ff1657c28f 100644 --- a/compiler/rustc_lint/src/default_could_be_derived.rs +++ b/compiler/rustc_lint/src/default_could_be_derived.rs @@ -2,7 +2,7 @@ use rustc_ast::LitKind; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res}; -use rustc_middle::ty::TyCtxt; +use rustc_middle::ty::Ty; use rustc_session::{declare_lint, declare_lint_pass}; use rustc_span::symbol::{kw, sym}; @@ -135,6 +135,8 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { // We have a manual `impl Default for Ty {}` item, where `Ty` has no type parameters. for assoc in data.items { + // We look for the user's `fn default() -> Self` associated fn of the `Default` impl. + let hir::AssocItemKind::Fn { has_self: false } = assoc.kind else { continue }; if assoc.ident.name != kw::Default { continue; @@ -148,6 +150,8 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { continue; }; + // We check `fn default()` body is a single ADT literal and all the fields are being + // set to something equivalent to the corresponding types' `Default::default()`. match expr.kind { hir::ExprKind::Path(hir::QPath::Resolved(_, path)) if let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Const), def_id) = @@ -211,9 +215,10 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { // // We suggest #[derive(Default)] if // - `val` is `Default::default()` + // - `val` matches the `Default::default()` body for that type // - `val` is `0` // - `val` is `false` - if fields.iter().all(|f| check_expr(cx.tcx, f.expr.kind)) { + if fields.iter().all(|f| check_expr(cx, f.expr)) { cx.tcx.node_span_lint( DEFAULT_COULD_BE_DERIVED, item.hir_id(), @@ -241,7 +246,7 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { path.res { let type_def_id = cx.tcx.parent(ctor_def_id); // From Ctor to struct - if args.iter().all(|expr| check_expr(cx.tcx, expr.kind)) { + if args.iter().all(|expr| check_expr(cx, expr)) { // We have a struct literal // // struct Foo(Type); @@ -254,6 +259,7 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { // // We suggest #[derive(Default)] if // - `val` is `Default::default()` + // - `val` matches the `Default::default()` body for that type // - `val` is `0` // - `val` is `false` cx.tcx.node_span_lint( @@ -319,97 +325,77 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { } } -fn check_expr(tcx: TyCtxt<'_>, kind: hir::ExprKind<'_>) -> bool { - let Some(default_def_id) = tcx.get_diagnostic_item(sym::Default) else { +fn check_path<'tcx>( + cx: &LateContext<'tcx>, + path: &hir::QPath<'_>, + hir_id: hir::HirId, + ty: Ty<'tcx>, +) -> bool { + let Some(default_def_id) = cx.tcx.get_diagnostic_item(sym::Default) else { return false; }; - match kind { + let res = cx.qpath_res(&path, hir_id); + let Some(def_id) = res.opt_def_id() else { return false }; + if cx.tcx.is_diagnostic_item(sym::default_fn, def_id) { + // We have `field: Default::default(),`. This is what the derive would do already. + return true; + } + // For every `Default` impl for this type (there should be a single one), we see if it + // has a "canonical" `DefId` for a fn call with no arguments, or a path. If it does, we + // check that `DefId` with the `DefId` of this field's value if it is also a call/path. + // If there's a match, it means that the contents of that type's `Default` impl are the + // same to what the user wrote on *their* `Default` impl for this field. + let mut equivalents = vec![]; + cx.tcx.for_each_relevant_impl(default_def_id, ty, |impl_def_id| { + let equivalent = match impl_def_id.as_local() { + None => cx.tcx.get_default_impl_equivalent(impl_def_id), + Some(local) => { + let def_kind = cx.tcx.def_kind(impl_def_id); + cx.tcx.get_default_equivalent(def_kind, local) + } + }; + if let Some(did) = equivalent { + equivalents.push(did); + } + }); + for did in equivalents { + if did == def_id { + return true; + } + } + false +} + +fn check_expr(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool { + match expr.kind { hir::ExprKind::Lit(spanned_lit) => match spanned_lit.node { LitKind::Int(val, _) if val == 0 => true, // field: 0, LitKind::Bool(false) => true, // field: false, _ => false, }, - hir::ExprKind::Call(expr, []) - if let hir::ExprKind::Path(hir::QPath::Resolved(_, path)) = expr.kind - && let Some(def_id) = path.res.opt_def_id() - && tcx.is_diagnostic_item(sym::default_fn, def_id) => - { - // field: Default::default(), - true + hir::ExprKind::Call(hir::Expr { kind: hir::ExprKind::Path(path), hir_id, .. }, []) => { + // `field: foo(),` or `field: Ty::assoc(),` + let Some(ty) = cx + .tcx + .has_typeck_results(expr.hir_id.owner.def_id) + .then(|| cx.tcx.typeck(expr.hir_id.owner.def_id)) + .and_then(|typeck| typeck.expr_ty_adjusted_opt(expr)) + else { + return false; + }; + check_path(cx, &path, *hir_id, ty) } - hir::ExprKind::Path(hir::QPath::Resolved(_, path)) - if let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Const), ctor_def_id) = - path.res => - { - // FIXME: We should use a better check where we explore existing - // `impl Default for def_id` of the found type when `def_id` is not - // local and see compare them against what we have here. For now, - // we special case `Option::None` and only check unit variants of - // local `Default` impls. - let var_def_id = tcx.parent(ctor_def_id); // From Ctor to variant - - // We explicitly check for `Option::::None`. If `Option` was - // local, it would be accounted by the logic further down, but - // because the analysis uses purely the HIR, that doesn't work - // accross crates. - // - // field: None, - let mut found = tcx.is_lang_item(var_def_id, hir::LangItem::OptionNone); - - // Look at the local `impl Default for ty` of the field's `ty`. - let ty_def_id = tcx.parent(var_def_id); // From variant to enum - let ty = tcx.type_of(ty_def_id).instantiate_identity(); - tcx.for_each_relevant_impl(default_def_id, ty, |impl_did| { - let hir = tcx.hir(); - let Some(hir::Node::Item(impl_item)) = hir.get_if_local(impl_did) else { - return; - }; - let hir::ItemKind::Impl(impl_item) = impl_item.kind else { - return; - }; - for assoc in impl_item.items { - let hir::AssocItemKind::Fn { has_self: false } = assoc.kind else { - continue; - }; - if assoc.ident.name != kw::Default { - continue; - } - let assoc = hir.impl_item(assoc.id); - let hir::ImplItemKind::Fn(_ty, body) = assoc.kind else { - continue; - }; - let body = hir.body(body); - let hir::ExprKind::Block(hir::Block { stmts: [], expr: Some(expr), .. }, None) = - body.value.kind - else { - continue; - }; - // Look at a specific implementation of `Default::default()` - // for their content and see if they are requivalent to what - // the user wrote in their manual `impl` for a given field. - match expr.kind { - hir::ExprKind::Path(hir::QPath::Resolved(_, path)) - if let Res::Def( - DefKind::Ctor(CtorOf::Variant, CtorKind::Const), - orig_def_id, - ) = path.res => - { - // We found - // - // field: Foo::Unit, - // - // and - // - // impl Default for Foo { - // fn default() -> Foo { Foo::Unit } - // } - found |= orig_def_id == ctor_def_id - } - _ => {} - } - } - }); - found + hir::ExprKind::Path(path) => { + // `field: qualified::Path,` or `field: ::Assoc,` + let Some(ty) = cx + .tcx + .has_typeck_results(expr.hir_id.owner.def_id) + .then(|| cx.tcx.typeck(expr.hir_id.owner.def_id)) + .and_then(|typeck| typeck.expr_ty_adjusted_opt(expr)) + else { + return false; + }; + check_path(cx, &path, expr.hir_id, ty) } _ => false, } diff --git a/compiler/rustc_metadata/src/rmeta/decoder.rs b/compiler/rustc_metadata/src/rmeta/decoder.rs index 6eae4f9a8d6ea..7b8662b0adf11 100644 --- a/compiler/rustc_metadata/src/rmeta/decoder.rs +++ b/compiler/rustc_metadata/src/rmeta/decoder.rs @@ -1174,6 +1174,12 @@ impl<'a> CrateMetadataRef<'a> { self.root.tables.default_fields.get(self, id).map(|d| d.decode(self)) } + /// For a given `impl Default for Ty`, return the "equivalence" for it, namely the `DefId` of + /// a path or function with no arguments that get's called from `::default()`. + fn get_default_impl_equivalent(self, id: DefIndex) -> Option { + self.root.tables.default_impl_equivalent.get(self, id).map(|d| d.decode(self)) + } + fn get_trait_item_def_id(self, id: DefIndex) -> Option { self.root.tables.trait_item_def_id.get(self, id).map(|d| d.decode_from_cdata(self)) } diff --git a/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs b/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs index 3077312ccf97e..c1d1e48c9c14b 100644 --- a/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs +++ b/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs @@ -423,6 +423,8 @@ provide! { tcx, def_id, other, cdata, syms } + get_default_impl_equivalent => { cdata.get_default_impl_equivalent(def_id.index) } + crate_extern_paths => { cdata.source().paths().cloned().collect() } expn_that_defined => { cdata.get_expn_that_defined(def_id.index, tcx.sess) } is_doc_hidden => { cdata.get_attr_flags(def_id.index).contains(AttrFlags::IS_DOC_HIDDEN) } diff --git a/compiler/rustc_metadata/src/rmeta/encoder.rs b/compiler/rustc_metadata/src/rmeta/encoder.rs index 92c0e8c3a501d..b548c43671150 100644 --- a/compiler/rustc_metadata/src/rmeta/encoder.rs +++ b/compiler/rustc_metadata/src/rmeta/encoder.rs @@ -1568,6 +1568,10 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> { let table = tcx.associated_types_for_impl_traits_in_associated_fn(def_id); record_defaulted_array!(self.tables.associated_types_for_impl_traits_in_associated_fn[def_id] <- table); } + + if let Some(path_def_id) = tcx.get_default_equivalent(def_kind, local_id) { + record!(self.tables.default_impl_equivalent[def_id] <- path_def_id); + } } for (def_id, impls) in &tcx.crate_inherent_impls(()).0.inherent_impls { diff --git a/compiler/rustc_metadata/src/rmeta/mod.rs b/compiler/rustc_metadata/src/rmeta/mod.rs index fa843a10adf12..9c70a5053f427 100644 --- a/compiler/rustc_metadata/src/rmeta/mod.rs +++ b/compiler/rustc_metadata/src/rmeta/mod.rs @@ -451,6 +451,7 @@ define_tables! { trait_item_def_id: Table, expn_that_defined: Table>, default_fields: Table>, + default_impl_equivalent: Table>, params_in_repr: Table>>, repr_options: Table>, // `def_keys` and `def_path_hashes` represent a lazy version of a diff --git a/compiler/rustc_middle/src/query/mod.rs b/compiler/rustc_middle/src/query/mod.rs index 906a47713f4eb..91188d79e3e60 100644 --- a/compiler/rustc_middle/src/query/mod.rs +++ b/compiler/rustc_middle/src/query/mod.rs @@ -1806,6 +1806,12 @@ rustc_queries! { desc { "looking up lifetime defaults for generic parameter `{}`", tcx.def_path_str(def_id) } separate_provide_extern } + + query get_default_impl_equivalent(def_id: DefId) -> Option { + desc { |tcx| "looking up the unit variant or fn call with no arguments equivalence for the `Default::default()` of `{}`", tcx.def_path_str(def_id) } + separate_provide_extern + } + query late_bound_vars_map(owner_id: hir::OwnerId) -> &'tcx SortedMap> { desc { |tcx| "looking up late bound vars inside `{}`", tcx.def_path_str(owner_id) } diff --git a/compiler/rustc_middle/src/ty/context.rs b/compiler/rustc_middle/src/ty/context.rs index f32656decd212..a83eccaf20fdc 100644 --- a/compiler/rustc_middle/src/ty/context.rs +++ b/compiler/rustc_middle/src/ty/context.rs @@ -29,7 +29,7 @@ use rustc_data_structures::unord::UnordSet; use rustc_errors::{ Applicability, Diag, DiagCtxtHandle, ErrorGuaranteed, LintDiagnostic, MultiSpan, }; -use rustc_hir::def::{CtorKind, DefKind}; +use rustc_hir::def::{CtorKind, DefKind, Res}; use rustc_hir::def_id::{CrateNum, DefId, LOCAL_CRATE, LocalDefId}; use rustc_hir::definitions::Definitions; use rustc_hir::intravisit::Visitor; @@ -3205,6 +3205,58 @@ impl<'tcx> TyCtxt<'tcx> { } } + fn get_expr_equivalent(self, expr: &hir::Expr<'_>) -> Option { + match &expr.kind { + hir::ExprKind::Path(qpath) => { + if self.has_typeck_results(expr.hir_id.owner.def_id) { + let res = self.typeck(expr.hir_id.owner.def_id).qpath_res(&qpath, expr.hir_id); + return res.opt_def_id(); + } + } + hir::ExprKind::Call(hir::Expr { kind: hir::ExprKind::Path(qpath), hir_id, .. }, []) => { + if self.has_typeck_results(expr.hir_id.owner.def_id) { + let res = self.typeck(expr.hir_id.owner.def_id).qpath_res(&qpath, *hir_id); + return res.opt_def_id(); + } + } + _ => {} + } + None + } + + /// Given an `impl Default for Ty` item, return the `DefId` of the single path or fn call within + /// the ` Option { + if (DefKind::Impl { of_trait: true }) == def_kind + && let hir::Node::Item(item) = self.hir_node_by_def_id(local_id) + && let hir::ItemKind::Impl(data) = item.kind + && let Some(trait_ref) = data.of_trait + && let Res::Def(DefKind::Trait, trait_def_id) = trait_ref.path.res + && let Some(default_def_id) = self.get_diagnostic_item(sym::Default) + && Some(trait_def_id) == Some(default_def_id) + { + let hir = self.hir(); + for assoc in data.items { + let hir::AssocItemKind::Fn { has_self: false } = assoc.kind else { continue }; + if assoc.ident.name != kw::Default { + continue; + } + let assoc = hir.impl_item(assoc.id); + let hir::ImplItemKind::Fn(_ty, body) = assoc.kind else { continue }; + let body = hir.body(body); + let hir::ExprKind::Block(hir::Block { stmts: [], expr: Some(expr), .. }, None) = + body.value.kind + else { + continue; + }; + return self.get_expr_equivalent(&expr); + } + } + None + } + /// Whether this is a trait implementation that has `#[diagnostic::do_not_recommend]` pub fn do_not_recommend_impl(self, def_id: DefId) -> bool { self.get_diagnostic_attr(def_id, sym::do_not_recommend).is_some() diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.fixed b/tests/ui/structs/manual-default-impl-could-be-derived.fixed index 922e14580b98b..21350c4a4f4bb 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.fixed +++ b/tests/ui/structs/manual-default-impl-could-be-derived.fixed @@ -59,6 +59,32 @@ } +// Account for fn calls that are not assoc fns, still check that they match between what the user +// wrote and the Default impl. +#[derive(Default)] struct J { + x: K, +} + + +struct K; + +impl Default for K { // *could* be derived, but it isn't lintable because of the `foo()` call + fn default() -> Self { + foo() + } +} + +fn foo() -> K { + K +} + +// Verify that cross-crate tracking of "equivalences" keeps working. +#[derive(PartialEq, Debug)] +#[derive(Default)] struct L { + x: Vec, +} + + fn main() { let _ = A::default(); let _ = B::default(); @@ -68,4 +94,8 @@ fn main() { let _ = F::::default(); let _ = G::default(); assert_eq!(H::default(), H { .. }); + let _ = I::default(); + let _ = J::default(); + let _ = K::default(); + let _ = L::default(); } diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.rs b/tests/ui/structs/manual-default-impl-could-be-derived.rs index 24f4adf071072..f8483639ab02b 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.rs +++ b/tests/ui/structs/manual-default-impl-could-be-derived.rs @@ -110,6 +110,46 @@ impl Default for I { //~ ERROR } } +// Account for fn calls that are not assoc fns, still check that they match between what the user +// wrote and the Default impl. +struct J { + x: K, +} + +impl Default for J { //~ ERROR + fn default() -> Self { + J { + x: foo(), // fn call that isn't an assoc fn + } + } +} + +struct K; + +impl Default for K { // *could* be derived, but it isn't lintable because of the `foo()` call + fn default() -> Self { + foo() + } +} + +fn foo() -> K { + K +} + +// Verify that cross-crate tracking of "equivalences" keeps working. +#[derive(PartialEq, Debug)] +struct L { + x: Vec, +} + +impl Default for L { //~ ERROR + fn default() -> Self { + L { + x: Vec::new(), // `::default()` just calls `Vec::new()` + } + } +} + fn main() { let _ = A::default(); let _ = B::default(); @@ -119,4 +159,8 @@ fn main() { let _ = F::::default(); let _ = G::default(); assert_eq!(H::default(), H { .. }); + let _ = I::default(); + let _ = J::default(); + let _ = K::default(); + let _ = L::default(); } diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.stderr b/tests/ui/structs/manual-default-impl-could-be-derived.stderr index eb13043a76025..124f25b37ab30 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.stderr +++ b/tests/ui/structs/manual-default-impl-could-be-derived.stderr @@ -158,5 +158,39 @@ help: to avoid divergence in behavior between `Struct { .. }` and ` $DIR/manual-default-impl-could-be-derived.rs:119:1 + | +LL | / impl Default for J { +LL | | fn default() -> Self { +LL | | J { +LL | | x: foo(), // fn call that isn't an assoc fn +LL | | } +LL | | } +LL | | } + | |_^ + | +help: you don't need to manually `impl Default`, you can derive it + | +LL ~ #[derive(Default)] struct J { + | + +error: `impl Default` that could be derived + --> $DIR/manual-default-impl-could-be-derived.rs:145:1 + | +LL | / impl Default for L { +LL | | fn default() -> Self { +LL | | L { +LL | | x: Vec::new(), // `::default()` just calls `Vec::new()` +LL | | } +LL | | } +LL | | } + | |_^ + | +help: you don't need to manually `impl Default`, you can derive it + | +LL ~ #[derive(Default)] struct L { + | + +error: aborting due to 11 previous errors From da449e2436069f9e3676a90b3c200412980ba4e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 12 Dec 2024 19:01:32 +0000 Subject: [PATCH 14/26] update test output --- tests/ui/lint/lint-unconditional-recursion.rs | 2 +- .../lint/lint-unconditional-recursion.stderr | 20 ++++++++++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/tests/ui/lint/lint-unconditional-recursion.rs b/tests/ui/lint/lint-unconditional-recursion.rs index ad052d36f206a..fe72accde1739 100644 --- a/tests/ui/lint/lint-unconditional-recursion.rs +++ b/tests/ui/lint/lint-unconditional-recursion.rs @@ -182,7 +182,7 @@ pub struct Point { pub y: f32, } -impl Default for Point { +impl Default for Point { //~ WARN fn default() -> Self { //~ ERROR function cannot return without recursing Point { x: Default::default(), diff --git a/tests/ui/lint/lint-unconditional-recursion.stderr b/tests/ui/lint/lint-unconditional-recursion.stderr index d75754bf9f900..b5c5e016aa94a 100644 --- a/tests/ui/lint/lint-unconditional-recursion.stderr +++ b/tests/ui/lint/lint-unconditional-recursion.stderr @@ -197,5 +197,23 @@ LL | ..Default::default() | = help: a `loop` may express intention better if this is on purpose -error: aborting due to 18 previous errors +warning: `impl Default` that could be derived + --> $DIR/lint-unconditional-recursion.rs:185:1 + | +LL | / impl Default for Point { +LL | | fn default() -> Self { +LL | | Point { +LL | | x: Default::default(), +... | +LL | | } +LL | | } + | |_^ + | + = note: `#[warn(default_could_be_derived)]` on by default +help: you don't need to manually `impl Default`, you can derive it + | +LL ~ #[derive(Default)] pub struct Point { + | + +error: aborting due to 18 previous errors; 1 warning emitted From 132b49dabe74c6327443243f7f38241e16812b8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 12 Dec 2024 19:28:37 +0000 Subject: [PATCH 15/26] Add test for `const` "equivalent" --- ...manual-default-impl-could-be-derived.fixed | 19 +++++++++++++- .../manual-default-impl-could-be-derived.rs | 26 ++++++++++++++++++- ...anual-default-impl-could-be-derived.stderr | 21 +++++++++++++-- 3 files changed, 62 insertions(+), 4 deletions(-) diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.fixed b/tests/ui/structs/manual-default-impl-could-be-derived.fixed index 21350c4a4f4bb..dcf9161fd798b 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.fixed +++ b/tests/ui/structs/manual-default-impl-could-be-derived.fixed @@ -79,12 +79,27 @@ fn foo() -> K { } // Verify that cross-crate tracking of "equivalences" keeps working. -#[derive(PartialEq, Debug)] #[derive(Default)] struct L { x: Vec, } +// Account for `const`s +#[derive(Default)] struct M { + x: N, +} + + +struct N; + +impl Default for N { // ok + fn default() -> Self { + N_CONST + } +} + +const N_CONST: N = N; + fn main() { let _ = A::default(); let _ = B::default(); @@ -98,4 +113,6 @@ fn main() { let _ = J::default(); let _ = K::default(); let _ = L::default(); + let _ = M::default(); + let _ = N::default(); } diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.rs b/tests/ui/structs/manual-default-impl-could-be-derived.rs index f8483639ab02b..c3e661e10cf31 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.rs +++ b/tests/ui/structs/manual-default-impl-could-be-derived.rs @@ -137,7 +137,6 @@ fn foo() -> K { } // Verify that cross-crate tracking of "equivalences" keeps working. -#[derive(PartialEq, Debug)] struct L { x: Vec, } @@ -150,6 +149,29 @@ impl Default for L { //~ ERROR } } +// Account for `const`s +struct M { + x: N, +} + +impl Default for M { //~ ERROR + fn default() -> Self { + M { + x: N_CONST, + } + } +} + +struct N; + +impl Default for N { // ok + fn default() -> Self { + N_CONST + } +} + +const N_CONST: N = N; + fn main() { let _ = A::default(); let _ = B::default(); @@ -163,4 +185,6 @@ fn main() { let _ = J::default(); let _ = K::default(); let _ = L::default(); + let _ = M::default(); + let _ = N::default(); } diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.stderr b/tests/ui/structs/manual-default-impl-could-be-derived.stderr index 124f25b37ab30..af3112bc41f2d 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.stderr +++ b/tests/ui/structs/manual-default-impl-could-be-derived.stderr @@ -176,7 +176,7 @@ LL ~ #[derive(Default)] struct J { | error: `impl Default` that could be derived - --> $DIR/manual-default-impl-could-be-derived.rs:145:1 + --> $DIR/manual-default-impl-could-be-derived.rs:144:1 | LL | / impl Default for L { LL | | fn default() -> Self { @@ -192,5 +192,22 @@ help: you don't need to manually `impl Default`, you can derive it LL ~ #[derive(Default)] struct L { | -error: aborting due to 11 previous errors +error: `impl Default` that could be derived + --> $DIR/manual-default-impl-could-be-derived.rs:157:1 + | +LL | / impl Default for M { +LL | | fn default() -> Self { +LL | | M { +LL | | x: N_CONST, +LL | | } +LL | | } +LL | | } + | |_^ + | +help: you don't need to manually `impl Default`, you can derive it + | +LL ~ #[derive(Default)] struct M { + | + +error: aborting due to 12 previous errors From f10d5607bebf23eee33b7234ce661e6f306f1f28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 12 Dec 2024 19:39:21 +0000 Subject: [PATCH 16/26] `#[derive(Default)] struct FixupContext` --- .../rustc_ast_pretty/src/pprust/state/fixup.rs | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs b/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs index 6f5382ce61d3b..10dacf5fb9139 100644 --- a/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs +++ b/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs @@ -1,7 +1,9 @@ use rustc_ast::Expr; use rustc_ast::util::{classify, parser}; -#[derive(Copy, Clone, Debug)] +// The default amount of fixing is minimal fixing. Fixups should be turned on +// in a targeted fashion where needed. +#[derive(Copy, Clone, Debug, Default)] pub(crate) struct FixupContext { /// Print expression such that it can be parsed back as a statement /// consisting of the original expression. @@ -93,20 +95,6 @@ pub(crate) struct FixupContext { parenthesize_exterior_struct_lit: bool, } -/// The default amount of fixing is minimal fixing. Fixups should be turned on -/// in a targeted fashion where needed. -impl Default for FixupContext { - fn default() -> Self { - FixupContext { - stmt: false, - leftmost_subexpression_in_stmt: false, - match_arm: false, - leftmost_subexpression_in_match_arm: false, - parenthesize_exterior_struct_lit: false, - } - } -} - impl FixupContext { /// Create the initial fixup for printing an expression in statement /// position. From 7c8b8018c6ec4d466d5f538554c42c6e6c40869e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 12 Dec 2024 20:32:05 +0000 Subject: [PATCH 17/26] Tweak lint output --- .../src/default_could_be_derived.rs | 55 ++++++++++++++++++- tests/ui/lint/lint-unconditional-recursion.rs | 2 +- .../lint/lint-unconditional-recursion.stderr | 20 +------ ...anual-default-impl-could-be-derived.stderr | 14 ++++- 4 files changed, 69 insertions(+), 22 deletions(-) diff --git a/compiler/rustc_lint/src/default_could_be_derived.rs b/compiler/rustc_lint/src/default_could_be_derived.rs index 054ff1657c28f..b3671cbc6e711 100644 --- a/compiler/rustc_lint/src/default_could_be_derived.rs +++ b/compiler/rustc_lint/src/default_could_be_derived.rs @@ -180,6 +180,10 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { item.span, |diag| { diag.primary_message("`impl Default` that could be derived"); + diag.span_label( + expr.span, + "this enum variant has no fields, so it's trivially derivable", + ); diag.multipart_suggestion_verbose( "you don't need to manually `impl Default`, you can derive it", vec![ @@ -198,7 +202,7 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { }, ); } - hir::ExprKind::Struct(_qpath, fields, _tail) => { + hir::ExprKind::Struct(_qpath, fields, tail) => { // We have a struct literal // // struct Foo { @@ -218,6 +222,16 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { // - `val` matches the `Default::default()` body for that type // - `val` is `0` // - `val` is `false` + if let hir::StructTailExpr::Base(_) = tail { + // This is *very* niche. We'd only get here if someone wrote + // impl Default for Ty { + // fn default() -> Ty { + // Ty { ..something() } + // } + // } + // where `something()` would have to be a call or path. + return; + } if fields.iter().all(|f| check_expr(cx, f.expr)) { cx.tcx.node_span_lint( DEFAULT_COULD_BE_DERIVED, @@ -225,6 +239,27 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { item.span, |diag| { diag.primary_message("`impl Default` that could be derived"); + for (i, field) in fields.iter().enumerate() { + let msg = if i == fields.len() - 1 { + if fields.len() == 1 { + "this is the same value the expansion of \ + `#[derive(Default)]` would use" + } else { + "these are the same values the expansion of \ + `#[derive(Default)]` would use" + } + } else { + "" + }; + diag.span_label(field.expr.span, msg); + } + if let hir::StructTailExpr::DefaultFields(span) = tail { + diag.span_label( + span, + "all remaining fields will use the same default field \ + values that the `#[derive(Default)]` would use", + ); + } diag.multipart_suggestion_verbose( "you don't need to manually `impl Default`, you can derive it", vec![ @@ -268,6 +303,20 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { item.span, |diag| { diag.primary_message("`impl Default` that could be derived"); + for (i, field) in args.iter().enumerate() { + let msg = if i == args.len() - 1 { + if args.len() == 1 { + "this is the same value the expansion of \ + `#[derive(Default)]` would use" + } else { + "these are the same values the expansion of \ + `#[derive(Default)]` would use" + } + } else { + "" + }; + diag.span_label(field.span, msg); + } diag.multipart_suggestion_verbose( "you don't need to manually `impl Default`, you can derive it", vec![ @@ -305,6 +354,10 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { item.span, |diag| { diag.primary_message("`impl Default` that could be derived"); + diag.span_label( + expr.span, + "this type has no fields, so it's trivially derivable", + ); diag.multipart_suggestion_verbose( "you don't need to manually `impl Default`, you can derive it", vec![ diff --git a/tests/ui/lint/lint-unconditional-recursion.rs b/tests/ui/lint/lint-unconditional-recursion.rs index fe72accde1739..ad052d36f206a 100644 --- a/tests/ui/lint/lint-unconditional-recursion.rs +++ b/tests/ui/lint/lint-unconditional-recursion.rs @@ -182,7 +182,7 @@ pub struct Point { pub y: f32, } -impl Default for Point { //~ WARN +impl Default for Point { fn default() -> Self { //~ ERROR function cannot return without recursing Point { x: Default::default(), diff --git a/tests/ui/lint/lint-unconditional-recursion.stderr b/tests/ui/lint/lint-unconditional-recursion.stderr index b5c5e016aa94a..d75754bf9f900 100644 --- a/tests/ui/lint/lint-unconditional-recursion.stderr +++ b/tests/ui/lint/lint-unconditional-recursion.stderr @@ -197,23 +197,5 @@ LL | ..Default::default() | = help: a `loop` may express intention better if this is on purpose -warning: `impl Default` that could be derived - --> $DIR/lint-unconditional-recursion.rs:185:1 - | -LL | / impl Default for Point { -LL | | fn default() -> Self { -LL | | Point { -LL | | x: Default::default(), -... | -LL | | } -LL | | } - | |_^ - | - = note: `#[warn(default_could_be_derived)]` on by default -help: you don't need to manually `impl Default`, you can derive it - | -LL ~ #[derive(Default)] pub struct Point { - | - -error: aborting due to 18 previous errors; 1 warning emitted +error: aborting due to 18 previous errors diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.stderr b/tests/ui/structs/manual-default-impl-could-be-derived.stderr index af3112bc41f2d..8b624f57151ca 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.stderr +++ b/tests/ui/structs/manual-default-impl-could-be-derived.stderr @@ -3,6 +3,7 @@ error: `impl Default` that could be derived | LL | / impl Default for A { LL | | fn default() -> Self { A } + | | - this type has no fields, so it's trivially derivable LL | | } | |_^ | @@ -22,6 +23,7 @@ error: `impl Default` that could be derived | LL | / impl Default for B { LL | | fn default() -> Self { B(Default::default()) } + | | ------------------ this is the same value the expansion of `#[derive(Default)]` would use LL | | } | |_^ | @@ -36,6 +38,7 @@ error: `impl Default` that could be derived | LL | / impl Default for C { LL | | fn default() -> Self { C(None) } + | | ---- this is the same value the expansion of `#[derive(Default)]` would use LL | | } | |_^ | @@ -52,7 +55,10 @@ LL | / impl Default for D { LL | | fn default() -> Self { LL | | D { LL | | x: Default::default(), -... | + | | ------------------ +LL | | y: 0, + | | - these are the same values the expansion of `#[derive(Default)]` would use +LL | | } LL | | } LL | | } | |_^ @@ -69,6 +75,7 @@ LL | / impl Default for E { LL | | fn default() -> Self { LL | | E { LL | | x: None, + | | ---- this is the same value the expansion of `#[derive(Default)]` would use LL | | } LL | | } LL | | } @@ -85,6 +92,7 @@ error: `impl Default` that could be derived LL | / impl Default for F { LL | | fn default() -> Self { LL | | F::Unit + | | ------- this enum variant has no fields, so it's trivially derivable LL | | } LL | | } | |_^ @@ -102,6 +110,7 @@ LL | / impl Default for G { LL | | fn default() -> Self { LL | | G { LL | | f: F::Unit, + | | ------- this is the same value the expansion of `#[derive(Default)]` would use LL | | } LL | | } LL | | } @@ -165,6 +174,7 @@ LL | / impl Default for J { LL | | fn default() -> Self { LL | | J { LL | | x: foo(), // fn call that isn't an assoc fn + | | ----- this is the same value the expansion of `#[derive(Default)]` would use LL | | } LL | | } LL | | } @@ -182,6 +192,7 @@ LL | / impl Default for L { LL | | fn default() -> Self { LL | | L { LL | | x: Vec::new(), // `::default()` just calls `Vec::new()` + | | ---------- this is the same value the expansion of `#[derive(Default)]` would use LL | | } LL | | } LL | | } @@ -199,6 +210,7 @@ LL | / impl Default for M { LL | | fn default() -> Self { LL | | M { LL | | x: N_CONST, + | | ------- this is the same value the expansion of `#[derive(Default)]` would use LL | | } LL | | } LL | | } From 4c421beb2a15aad21769e4e358180ef93ad18186 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 12 Dec 2024 21:01:04 +0000 Subject: [PATCH 18/26] Split lints --- .../src/default_could_be_derived.rs | 65 ++++++++++++++++++- ...manual-default-impl-could-be-derived.fixed | 2 +- .../manual-default-impl-could-be-derived.rs | 26 ++++---- ...anual-default-impl-could-be-derived.stderr | 11 +++- 4 files changed, 84 insertions(+), 20 deletions(-) diff --git a/compiler/rustc_lint/src/default_could_be_derived.rs b/compiler/rustc_lint/src/default_could_be_derived.rs index b3671cbc6e711..4028a038f36fa 100644 --- a/compiler/rustc_lint/src/default_could_be_derived.rs +++ b/compiler/rustc_lint/src/default_could_be_derived.rs @@ -15,6 +15,19 @@ declare_lint! { /// ### Example /// /// ```rust,compile_fail + /// struct A { + /// b: Option, + /// } + /// + /// #[deny(default_could_be_derived)] + /// impl Default for Foo { + /// fn default() -> Foo { + /// A { + /// b: None, + /// } + /// } + /// } + /// /// enum Foo { /// Bar, /// } @@ -29,12 +42,56 @@ declare_lint! { /// /// {{produces}} /// + /// ### Explanation + /// + /// `#[derive(Default)]` uses the `Default` impl for every field of your + /// type. If your manual `Default` impl either invokes `Default::default()` + /// or uses the same value that that associated function produces, then it + /// is better to use the derive to avoid the different `Default` impls from + /// diverging over time. + /// + /// This lint also triggers on cases where there the type has no fields, + /// so the derive for `Default` for a struct is trivial, and for an enum + /// variant with no fields, which can be annotated with `#[default]`. pub DEFAULT_COULD_BE_DERIVED, Warn, "detect `Default` impl that could be derived" } -declare_lint_pass!(DefaultCouldBeDerived => [DEFAULT_COULD_BE_DERIVED]); +declare_lint! { + /// The `default_could_be_derived` lint checks for manual `impl` blocks + /// of the `Default` trait that could have been derived. + /// + /// ### Example + /// + /// ```rust,compile_fail + /// struct Foo { + /// x: i32 = 101, + /// } + /// + /// #[deny(manual_default_for_type_with_default_fields)] + /// impl Default for Foo { + /// fn default() -> Foo { + /// Foo { x: 100 } + /// } + /// } + /// ``` + /// + /// {{produces}} + /// + /// ### Explanation + /// + /// Manually writing a `Default` implementation for a type that has + /// default field values runs the risk of diverging behavior between + /// `Type { .. }` and `::default()`, which would be a + /// foot-gun for users of that type that would expect these to be + /// equivalent. + pub MANUAL_DEFAULT_FOR_TYPE_WITH_DEFAULT_FIELDS, + Warn, + "detect `Default` impl on type with default field values that should be derived" +} + +declare_lint_pass!(DefaultCouldBeDerived => [DEFAULT_COULD_BE_DERIVED, MANUAL_DEFAULT_FOR_TYPE_WITH_DEFAULT_FIELDS]); impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'tcx>) { @@ -81,11 +138,13 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { == fields_with_default_value.len() + fields_with_default_impl.len() { cx.tcx.node_span_lint( - DEFAULT_COULD_BE_DERIVED, + MANUAL_DEFAULT_FOR_TYPE_WITH_DEFAULT_FIELDS, item.hir_id(), item.span, |diag| { - diag.primary_message("`impl Default` that could be derived"); + diag.primary_message( + "manual `Default` impl for type with default field values", + ); let msg = match ( !fields_with_default_value.is_empty(), !fields_with_default_impl.is_empty(), diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.fixed b/tests/ui/structs/manual-default-impl-could-be-derived.fixed index dcf9161fd798b..acfd8e704a827 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.fixed +++ b/tests/ui/structs/manual-default-impl-could-be-derived.fixed @@ -1,7 +1,7 @@ // Warn when we encounter a manual `Default` impl that could be derived. //@ run-rustfix #![allow(dead_code)] -#![deny(default_could_be_derived)] +#![deny(default_could_be_derived, manual_default_for_type_with_default_fields)] #![feature(default_field_values)] #[derive(Debug)] diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.rs b/tests/ui/structs/manual-default-impl-could-be-derived.rs index c3e661e10cf31..2019b972c3282 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.rs +++ b/tests/ui/structs/manual-default-impl-could-be-derived.rs @@ -1,27 +1,27 @@ // Warn when we encounter a manual `Default` impl that could be derived. //@ run-rustfix #![allow(dead_code)] -#![deny(default_could_be_derived)] +#![deny(default_could_be_derived, manual_default_for_type_with_default_fields)] #![feature(default_field_values)] #[derive(Debug)] struct A; -impl Default for A { //~ ERROR +impl Default for A { //~ ERROR default_could_be_derived fn default() -> Self { A } } #[derive(Debug)] struct B(Option); -impl Default for B { //~ ERROR +impl Default for B { //~ ERROR default_could_be_derived fn default() -> Self { B(Default::default()) } } #[derive(Debug)] struct C(Option); -impl Default for C { //~ ERROR +impl Default for C { //~ ERROR default_could_be_derived fn default() -> Self { C(None) } } @@ -32,7 +32,7 @@ struct D { y: i32, } -impl Default for D { //~ ERROR +impl Default for D { //~ ERROR default_could_be_derived fn default() -> Self { D { x: Default::default(), @@ -47,7 +47,7 @@ struct E { x: Option, } -impl Default for E { //~ ERROR +impl Default for E { //~ ERROR default_could_be_derived fn default() -> Self { E { x: None, @@ -61,7 +61,7 @@ enum F { Tuple(T), } -impl Default for F { //~ ERROR +impl Default for F { //~ ERROR default_could_be_derived fn default() -> Self { F::Unit } @@ -72,7 +72,7 @@ struct G { f: F, } -impl Default for G { //~ ERROR +impl Default for G { //~ ERROR default_could_be_derived fn default() -> Self { G { f: F::Unit, @@ -86,7 +86,7 @@ struct H { x: i32 = 101, } -impl Default for H { //~ ERROR +impl Default for H { //~ ERROR [manual_default_for_type_with_default_fields] fn default() -> Self { H { x: 1, @@ -101,7 +101,7 @@ struct I { y: Option, } -impl Default for I { //~ ERROR +impl Default for I { //~ ERROR [manual_default_for_type_with_default_fields] fn default() -> Self { I { x: 1, @@ -116,7 +116,7 @@ struct J { x: K, } -impl Default for J { //~ ERROR +impl Default for J { //~ ERROR default_could_be_derived fn default() -> Self { J { x: foo(), // fn call that isn't an assoc fn @@ -141,7 +141,7 @@ struct L { x: Vec, } -impl Default for L { //~ ERROR +impl Default for L { //~ ERROR default_could_be_derived fn default() -> Self { L { x: Vec::new(), // `::default()` just calls `Vec::new()` @@ -154,7 +154,7 @@ struct M { x: N, } -impl Default for M { //~ ERROR +impl Default for M { //~ ERROR default_could_be_derived fn default() -> Self { M { x: N_CONST, diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.stderr b/tests/ui/structs/manual-default-impl-could-be-derived.stderr index 8b624f57151ca..027d4869a4fb9 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.stderr +++ b/tests/ui/structs/manual-default-impl-could-be-derived.stderr @@ -10,7 +10,7 @@ LL | | } note: the lint level is defined here --> $DIR/manual-default-impl-could-be-derived.rs:4:9 | -LL | #![deny(default_could_be_derived)] +LL | #![deny(default_could_be_derived, manual_default_for_type_with_default_fields)] | ^^^^^^^^^^^^^^^^^^^^^^^^ help: you don't need to manually `impl Default`, you can derive it | @@ -121,7 +121,7 @@ help: you don't need to manually `impl Default`, you can derive it LL ~ #[derive(Default)] struct G { | -error: `impl Default` that could be derived +error: manual `Default` impl for type with default field values --> $DIR/manual-default-impl-could-be-derived.rs:89:1 | LL | struct H { @@ -138,12 +138,17 @@ LL | | } LL | | } | |_^ | +note: the lint level is defined here + --> $DIR/manual-default-impl-could-be-derived.rs:4:35 + | +LL | #![deny(default_could_be_derived, manual_default_for_type_with_default_fields)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: to avoid divergence in behavior between `Struct { .. }` and `::default()`, derive the `Default` | LL ~ #[derive(Default)] struct H { | -error: `impl Default` that could be derived +error: manual `Default` impl for type with default field values --> $DIR/manual-default-impl-could-be-derived.rs:104:1 | LL | struct I { From 64c4938dffa94f0f7cab0757dd2e6d59900f37e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 12 Dec 2024 21:24:26 +0000 Subject: [PATCH 19/26] Fix clippy tests --- .../clippy/tests/ui/derivable_impls.fixed | 2 +- src/tools/clippy/tests/ui/derivable_impls.rs | 2 +- .../clippy/tests/ui/new_without_default.fixed | 7 +-- .../tests/ui/new_without_default.stderr | 20 ++++++++- src/tools/clippy/tests/ui/or_fun_call.fixed | 16 ++----- src/tools/clippy/tests/ui/or_fun_call.stderr | 44 ++++++++++++++++++- .../tests/ui/unwrap_or_else_default.fixed | 8 +--- .../tests/ui/unwrap_or_else_default.stderr | 24 +++++++++- 8 files changed, 94 insertions(+), 29 deletions(-) diff --git a/src/tools/clippy/tests/ui/derivable_impls.fixed b/src/tools/clippy/tests/ui/derivable_impls.fixed index c85f384fd6eb9..af8540038a007 100644 --- a/src/tools/clippy/tests/ui/derivable_impls.fixed +++ b/src/tools/clippy/tests/ui/derivable_impls.fixed @@ -1,4 +1,4 @@ -#![allow(dead_code)] +#![allow(dead_code, default_could_be_derived)] use std::collections::HashMap; diff --git a/src/tools/clippy/tests/ui/derivable_impls.rs b/src/tools/clippy/tests/ui/derivable_impls.rs index 21d73ba8b7778..81d54ab0357be 100644 --- a/src/tools/clippy/tests/ui/derivable_impls.rs +++ b/src/tools/clippy/tests/ui/derivable_impls.rs @@ -1,4 +1,4 @@ -#![allow(dead_code)] +#![allow(dead_code, default_could_be_derived)] use std::collections::HashMap; diff --git a/src/tools/clippy/tests/ui/new_without_default.fixed b/src/tools/clippy/tests/ui/new_without_default.fixed index 5a6a92394a7fd..a47e20858dcfb 100644 --- a/src/tools/clippy/tests/ui/new_without_default.fixed +++ b/src/tools/clippy/tests/ui/new_without_default.fixed @@ -38,7 +38,7 @@ impl Bar { } } -pub struct Ok; +#[derive(Default)] pub struct Ok; impl Ok { pub fn new() -> Self { @@ -46,11 +46,6 @@ impl Ok { } } -impl Default for Ok { - fn default() -> Self { - Ok - } -} pub struct Params; diff --git a/src/tools/clippy/tests/ui/new_without_default.stderr b/src/tools/clippy/tests/ui/new_without_default.stderr index 57bf4bd847cce..1ef62238982c6 100644 --- a/src/tools/clippy/tests/ui/new_without_default.stderr +++ b/src/tools/clippy/tests/ui/new_without_default.stderr @@ -166,5 +166,23 @@ LL + } LL + } | -error: aborting due to 9 previous errors +error: `impl Default` that could be derived + --> tests/ui/new_without_default.rs:37:1 + | +LL | / impl Default for Ok { +LL | | fn default() -> Self { +LL | | Ok + | | -- this type has no fields, so it's trivially derivable +LL | | } +LL | | } + | |_^ + | + = note: `-D default-could-be-derived` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(default_could_be_derived)]` +help: you don't need to manually `impl Default`, you can derive it + | +LL ~ #[derive(Default)] pub struct Ok; + | + +error: aborting due to 10 previous errors diff --git a/src/tools/clippy/tests/ui/or_fun_call.fixed b/src/tools/clippy/tests/ui/or_fun_call.fixed index 625d654dd3964..b1ac63b594efc 100644 --- a/src/tools/clippy/tests/ui/or_fun_call.fixed +++ b/src/tools/clippy/tests/ui/or_fun_call.fixed @@ -21,18 +21,14 @@ fn or_fun_call() { } } - struct FakeDefault; + #[derive(Default)] struct FakeDefault; impl FakeDefault { fn default() -> Self { FakeDefault } } - impl Default for FakeDefault { - fn default() -> Self { - FakeDefault - } - } + enum Enum { A(i32), @@ -268,18 +264,14 @@ mod lazy { } } - struct FakeDefault; + #[derive(Default)] struct FakeDefault; impl FakeDefault { fn default() -> Self { FakeDefault } } - impl Default for FakeDefault { - fn default() -> Self { - FakeDefault - } - } + let with_new = Some(vec![1]); with_new.unwrap_or_default(); diff --git a/src/tools/clippy/tests/ui/or_fun_call.stderr b/src/tools/clippy/tests/ui/or_fun_call.stderr index 9f90a830a2114..1cd2afcc14dd6 100644 --- a/src/tools/clippy/tests/ui/or_fun_call.stderr +++ b/src/tools/clippy/tests/ui/or_fun_call.stderr @@ -244,5 +244,47 @@ error: function call inside of `unwrap_or` LL | let _ = opt_foo.unwrap_or(Foo { val: String::default() }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| Foo { val: String::default() })` -error: aborting due to 38 previous errors +error: `impl Default` that could be derived + --> tests/ui/or_fun_call.rs:31:5 + | +LL | / impl Default for FakeDefault { +LL | | fn default() -> Self { +LL | | FakeDefault + | | ----------- this type has no fields, so it's trivially derivable +LL | | } +LL | | } + | |_____^ + | + = note: `-D default-could-be-derived` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(default_could_be_derived)]` +help: you don't need to manually `impl Default`, you can derive it + | +LL ~ #[derive(Default)] struct FakeDefault; +LL | impl FakeDefault { +... +LL | +LL ~ + | + +error: `impl Default` that could be derived + --> tests/ui/or_fun_call.rs:278:9 + | +LL | / impl Default for FakeDefault { +LL | | fn default() -> Self { +LL | | FakeDefault + | | ----------- this type has no fields, so it's trivially derivable +LL | | } +LL | | } + | |_________^ + | +help: you don't need to manually `impl Default`, you can derive it + | +LL ~ #[derive(Default)] struct FakeDefault; +LL | impl FakeDefault { +... +LL | +LL ~ + | + +error: aborting due to 40 previous errors diff --git a/src/tools/clippy/tests/ui/unwrap_or_else_default.fixed b/src/tools/clippy/tests/ui/unwrap_or_else_default.fixed index 8d5d34175c525..fb88bd4e7952c 100644 --- a/src/tools/clippy/tests/ui/unwrap_or_else_default.fixed +++ b/src/tools/clippy/tests/ui/unwrap_or_else_default.fixed @@ -17,7 +17,7 @@ fn unwrap_or_else_default() { } } - struct HasDefaultAndDuplicate; + #[derive(Default)] struct HasDefaultAndDuplicate; impl HasDefaultAndDuplicate { fn default() -> Self { @@ -25,11 +25,7 @@ fn unwrap_or_else_default() { } } - impl Default for HasDefaultAndDuplicate { - fn default() -> Self { - HasDefaultAndDuplicate - } - } + enum Enum { A(), diff --git a/src/tools/clippy/tests/ui/unwrap_or_else_default.stderr b/src/tools/clippy/tests/ui/unwrap_or_else_default.stderr index e4b4a0a1f6aa7..50c3a6db8531f 100644 --- a/src/tools/clippy/tests/ui/unwrap_or_else_default.stderr +++ b/src/tools/clippy/tests/ui/unwrap_or_else_default.stderr @@ -97,5 +97,27 @@ error: use of `or_insert_with` to construct default value LL | let _ = inner_map.entry(0).or_insert_with(Default::default); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `or_default()` -error: aborting due to 16 previous errors +error: `impl Default` that could be derived + --> tests/ui/unwrap_or_else_default.rs:28:5 + | +LL | / impl Default for HasDefaultAndDuplicate { +LL | | fn default() -> Self { +LL | | HasDefaultAndDuplicate + | | ---------------------- this type has no fields, so it's trivially derivable +LL | | } +LL | | } + | |_____^ + | + = note: `-D default-could-be-derived` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(default_could_be_derived)]` +help: you don't need to manually `impl Default`, you can derive it + | +LL ~ #[derive(Default)] struct HasDefaultAndDuplicate; +LL | +... +LL | +LL ~ + | + +error: aborting due to 17 previous errors From 810a2457539e5e1f511deb808d107e265987d710 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 12 Dec 2024 21:54:24 +0000 Subject: [PATCH 20/26] Use `derive(Default)` in rustfmt --- src/tools/rustfmt/src/config/options.rs | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/tools/rustfmt/src/config/options.rs b/src/tools/rustfmt/src/config/options.rs index c526b5c678fd7..7ca0f62ff8ea2 100644 --- a/src/tools/rustfmt/src/config/options.rs +++ b/src/tools/rustfmt/src/config/options.rs @@ -155,9 +155,11 @@ pub enum ReportTactic { /// What Rustfmt should emit. Mostly corresponds to the `--emit` command line /// option. +#[derive(Default)] #[config_type] pub enum EmitMode { /// Emits to files. + #[default] Files, /// Writes the output to stdout. Stdout, @@ -310,12 +312,6 @@ impl ::std::str::FromStr for WidthHeuristics { } } -impl Default for EmitMode { - fn default() -> EmitMode { - EmitMode::Files - } -} - /// A set of directories, files and modules that rustfmt should ignore. #[derive(Default, Clone, Debug, PartialEq)] pub struct IgnoreList { @@ -425,10 +421,12 @@ pub trait CliOptions { } /// The edition of the syntax and semantics of code (RFC 2052). +#[derive(Default)] #[config_type] pub enum Edition { #[value = "2015"] #[doc_hint = "2015"] + #[default] /// Edition 2015. Edition2015, #[value = "2018"] @@ -445,12 +443,6 @@ pub enum Edition { Edition2024, } -impl Default for Edition { - fn default() -> Edition { - Edition::Edition2015 - } -} - impl From for rustc_span::edition::Edition { fn from(edition: Edition) -> Self { match edition { From 448d80daacf086d3a2947e8f30d5f6777e19e205 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 12 Dec 2024 22:46:11 +0000 Subject: [PATCH 21/26] Derive `Default` in `tests::hash` --- library/core/tests/hash/mod.rs | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/library/core/tests/hash/mod.rs b/library/core/tests/hash/mod.rs index bf91e9e5df0e2..9f14995f73fe2 100644 --- a/library/core/tests/hash/mod.rs +++ b/library/core/tests/hash/mod.rs @@ -4,16 +4,11 @@ use std::hash::{BuildHasher, Hash, Hasher}; use std::ptr; use std::rc::Rc; +#[derive(Default)] struct MyHasher { hash: u64, } -impl Default for MyHasher { - fn default() -> MyHasher { - MyHasher { hash: 0 } - } -} - impl Hasher for MyHasher { fn write(&mut self, buf: &[u8]) { for byte in buf { @@ -107,6 +102,8 @@ fn test_writer_hasher() { struct Custom { hash: u64, } + +#[derive(Default)] struct CustomHasher { output: u64, } @@ -123,12 +120,6 @@ impl Hasher for CustomHasher { } } -impl Default for CustomHasher { - fn default() -> CustomHasher { - CustomHasher { output: 0 } - } -} - impl Hash for Custom { fn hash(&self, state: &mut H) { state.write_u64(self.hash); From 433947ba48c51ad3a17d3bddf3a741ae1f79f230 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 12 Dec 2024 22:47:20 +0000 Subject: [PATCH 22/26] Make lint deny-by-default (for crater run) --- compiler/rustc_lint/src/default_could_be_derived.rs | 2 +- src/tools/clippy/tests/ui/new_without_default.stderr | 3 +-- src/tools/clippy/tests/ui/or_fun_call.stderr | 3 +-- src/tools/clippy/tests/ui/unwrap_or_else_default.stderr | 3 +-- 4 files changed, 4 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_lint/src/default_could_be_derived.rs b/compiler/rustc_lint/src/default_could_be_derived.rs index 4028a038f36fa..d0635c8f9067c 100644 --- a/compiler/rustc_lint/src/default_could_be_derived.rs +++ b/compiler/rustc_lint/src/default_could_be_derived.rs @@ -54,7 +54,7 @@ declare_lint! { /// so the derive for `Default` for a struct is trivial, and for an enum /// variant with no fields, which can be annotated with `#[default]`. pub DEFAULT_COULD_BE_DERIVED, - Warn, + Deny, "detect `Default` impl that could be derived" } diff --git a/src/tools/clippy/tests/ui/new_without_default.stderr b/src/tools/clippy/tests/ui/new_without_default.stderr index 1ef62238982c6..0c4a5f4f5df71 100644 --- a/src/tools/clippy/tests/ui/new_without_default.stderr +++ b/src/tools/clippy/tests/ui/new_without_default.stderr @@ -177,8 +177,7 @@ LL | | } LL | | } | |_^ | - = note: `-D default-could-be-derived` implied by `-D warnings` - = help: to override `-D warnings` add `#[allow(default_could_be_derived)]` + = note: `#[deny(default_could_be_derived)]` on by default help: you don't need to manually `impl Default`, you can derive it | LL ~ #[derive(Default)] pub struct Ok; diff --git a/src/tools/clippy/tests/ui/or_fun_call.stderr b/src/tools/clippy/tests/ui/or_fun_call.stderr index 1cd2afcc14dd6..44f96a1592316 100644 --- a/src/tools/clippy/tests/ui/or_fun_call.stderr +++ b/src/tools/clippy/tests/ui/or_fun_call.stderr @@ -255,8 +255,7 @@ LL | | } LL | | } | |_____^ | - = note: `-D default-could-be-derived` implied by `-D warnings` - = help: to override `-D warnings` add `#[allow(default_could_be_derived)]` + = note: `#[deny(default_could_be_derived)]` on by default help: you don't need to manually `impl Default`, you can derive it | LL ~ #[derive(Default)] struct FakeDefault; diff --git a/src/tools/clippy/tests/ui/unwrap_or_else_default.stderr b/src/tools/clippy/tests/ui/unwrap_or_else_default.stderr index 50c3a6db8531f..6e86aac95bbbd 100644 --- a/src/tools/clippy/tests/ui/unwrap_or_else_default.stderr +++ b/src/tools/clippy/tests/ui/unwrap_or_else_default.stderr @@ -108,8 +108,7 @@ LL | | } LL | | } | |_____^ | - = note: `-D default-could-be-derived` implied by `-D warnings` - = help: to override `-D warnings` add `#[allow(default_could_be_derived)]` + = note: `#[deny(default_could_be_derived)]` on by default help: you don't need to manually `impl Default`, you can derive it | LL ~ #[derive(Default)] struct HasDefaultAndDuplicate; From 32b933e0ceb27558c20b650909f300800278acec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Fri, 13 Dec 2024 06:13:03 +0000 Subject: [PATCH 23/26] `allow(default_could_be_derived)` in `Default` examples --- library/core/src/default.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/core/src/default.rs b/library/core/src/default.rs index 426cfe9cdfefc..cdfbca9f897c1 100644 --- a/library/core/src/default.rs +++ b/library/core/src/default.rs @@ -77,6 +77,7 @@ /// your type that should be the default: /// /// ``` +/// # #![cfg_attr(not(bootstrap), allow(default_could_be_derived))] /// # #![allow(dead_code)] /// enum Kind { /// A, @@ -121,6 +122,7 @@ pub trait Default: Sized { /// Making your own: /// /// ``` + /// # #![cfg_attr(not(bootstrap), allow(default_could_be_derived))] /// # #[allow(dead_code)] /// enum Kind { /// A, From 814146c31357da9be9101c33f8a917871de83b02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Fri, 13 Dec 2024 22:59:26 +0000 Subject: [PATCH 24/26] Mention `#[default]` in E0655 code index --- .../rustc_error_codes/src/error_codes/E0665.md | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_error_codes/src/error_codes/E0665.md b/compiler/rustc_error_codes/src/error_codes/E0665.md index ae54d6d15798d..d5fd2130840de 100644 --- a/compiler/rustc_error_codes/src/error_codes/E0665.md +++ b/compiler/rustc_error_codes/src/error_codes/E0665.md @@ -16,18 +16,30 @@ The `Default` cannot be derived on an enum for the simple reason that the compiler doesn't know which value to pick by default whereas it can for a struct as long as all its fields implement the `Default` trait as well. -If you still want to implement `Default` on your enum, you'll have to do it "by -hand": +For the case where the desired default variant has no data, you can annotate +it with `#[default]` to derive it: ``` +#[derive(Default)] enum Food { + #[default] Sweet, Salty, } +``` + +In the case where the default variant does have data, you will have to +implement `Default` on your enum "by hand": + +``` +enum Food { + Sweet(i32), + Salty, +} impl Default for Food { fn default() -> Food { - Food::Sweet + Food::Sweet(1) } } ``` From c666eaecfefc58f2649932f3220bba22f483dc35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sat, 14 Dec 2024 00:07:07 +0000 Subject: [PATCH 25/26] Fix lint example --- .../rustc_lint/src/default_could_be_derived.rs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_lint/src/default_could_be_derived.rs b/compiler/rustc_lint/src/default_could_be_derived.rs index d0635c8f9067c..c6c97cfdf788f 100644 --- a/compiler/rustc_lint/src/default_could_be_derived.rs +++ b/compiler/rustc_lint/src/default_could_be_derived.rs @@ -20,8 +20,8 @@ declare_lint! { /// } /// /// #[deny(default_could_be_derived)] - /// impl Default for Foo { - /// fn default() -> Foo { + /// impl Default for A { + /// fn default() -> A { /// A { /// b: None, /// } @@ -59,12 +59,14 @@ declare_lint! { } declare_lint! { - /// The `default_could_be_derived` lint checks for manual `impl` blocks - /// of the `Default` trait that could have been derived. + /// The `manual_default_for_type_with_default_fields` lint checks for + /// manual `impl` blocks of the `Default` trait of types with default + /// field values. /// /// ### Example /// /// ```rust,compile_fail + /// #![feature(default_field_values)] /// struct Foo { /// x: i32 = 101, /// } @@ -114,7 +116,7 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { kind: hir::ItemKind::Struct(hir::VariantData::Struct { fields, recovered: _ }, _generics), .. - })) => { + })) if cx.tcx.features().default_field_values() => { let fields_with_default_value: Vec<_> = fields.iter().filter_map(|f| f.default).collect(); let fields_with_default_impl: Vec<_> = fields @@ -133,6 +135,10 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { _ => None, }) .collect(); + // FIXME: look at the `Default::default()` implementation and call check_expr and + // check_const_expr on every field. If all are either of those, then we suggest + // adding a default field value for the const-able ones and deriving if the feature + // is enabled. if !fields_with_default_value.is_empty() && fields.len() == fields_with_default_value.len() + fields_with_default_impl.len() From 6fa30b6666ab5c81ae29d90d49a76b288cc219d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sun, 15 Dec 2024 23:29:59 +0000 Subject: [PATCH 26/26] fix rebase --- ...anual-default-impl-could-be-derived.stderr | 22 ++++++------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.stderr b/tests/ui/structs/manual-default-impl-could-be-derived.stderr index 027d4869a4fb9..e5c92baf031e9 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.stderr +++ b/tests/ui/structs/manual-default-impl-could-be-derived.stderr @@ -58,8 +58,7 @@ LL | | x: Default::default(), | | ------------------ LL | | y: 0, | | - these are the same values the expansion of `#[derive(Default)]` would use -LL | | } -LL | | } +... | LL | | } | |_^ | @@ -76,8 +75,7 @@ LL | | fn default() -> Self { LL | | E { LL | | x: None, | | ---- this is the same value the expansion of `#[derive(Default)]` would use -LL | | } -LL | | } +... | LL | | } | |_^ | @@ -111,8 +109,7 @@ LL | | fn default() -> Self { LL | | G { LL | | f: F::Unit, | | ------- this is the same value the expansion of `#[derive(Default)]` would use -LL | | } -LL | | } +... | LL | | } | |_^ | @@ -133,8 +130,7 @@ LL | / impl Default for H { LL | | fn default() -> Self { LL | | H { LL | | x: 1, -LL | | } -LL | | } +... | LL | | } | |_^ | @@ -163,7 +159,6 @@ LL | | fn default() -> Self { LL | | I { LL | | x: 1, ... | -LL | | } LL | | } | |_^ | @@ -180,8 +175,7 @@ LL | | fn default() -> Self { LL | | J { LL | | x: foo(), // fn call that isn't an assoc fn | | ----- this is the same value the expansion of `#[derive(Default)]` would use -LL | | } -LL | | } +... | LL | | } | |_^ | @@ -198,8 +192,7 @@ LL | | fn default() -> Self { LL | | L { LL | | x: Vec::new(), // `::default()` just calls `Vec::new()` | | ---------- this is the same value the expansion of `#[derive(Default)]` would use -LL | | } -LL | | } +... | LL | | } | |_^ | @@ -216,8 +209,7 @@ LL | | fn default() -> Self { LL | | M { LL | | x: N_CONST, | | ------- this is the same value the expansion of `#[derive(Default)]` would use -LL | | } -LL | | } +... | LL | | } | |_^ |