diff --git a/compiler/rustc_ast/src/ast.rs b/compiler/rustc_ast/src/ast.rs index cec868e5c8e57..fdbafda638bcb 100644 --- a/compiler/rustc_ast/src/ast.rs +++ b/compiler/rustc_ast/src/ast.rs @@ -1455,8 +1455,8 @@ pub enum StructRest { Base(P), /// `..`. Rest(Span), - /// No trailing `..` or expression. - None, + /// No trailing `..` or expression. The `Span` points at the trailing `,` or spot before `}`. + None(Span), } #[derive(Clone, Encodable, Decodable, Debug)] diff --git a/compiler/rustc_ast/src/mut_visit.rs b/compiler/rustc_ast/src/mut_visit.rs index 995924c2a2949..398098ab3caa6 100644 --- a/compiler/rustc_ast/src/mut_visit.rs +++ b/compiler/rustc_ast/src/mut_visit.rs @@ -1771,7 +1771,7 @@ pub fn walk_expr(vis: &mut T, Expr { kind, id, span, attrs, token match rest { StructRest::Base(expr) => vis.visit_expr(expr), StructRest::Rest(_span) => {} - StructRest::None => {} + StructRest::None(_span) => {} } } ExprKind::Paren(expr) => { diff --git a/compiler/rustc_ast/src/visit.rs b/compiler/rustc_ast/src/visit.rs index c7cc772dabb7e..89915aee7be14 100644 --- a/compiler/rustc_ast/src/visit.rs +++ b/compiler/rustc_ast/src/visit.rs @@ -1094,7 +1094,7 @@ pub fn walk_expr<'a, V: Visitor<'a>>(visitor: &mut V, expression: &'a Expr) -> V match rest { StructRest::Base(expr) => try_visit!(visitor.visit_expr(expr)), StructRest::Rest(_span) => {} - StructRest::None => {} + StructRest::None(_span) => {} } } ExprKind::Tup(subexpressions) => { diff --git a/compiler/rustc_ast_lowering/src/expr.rs b/compiler/rustc_ast_lowering/src/expr.rs index d16a3ce390dbe..da410a23079a5 100644 --- a/compiler/rustc_ast_lowering/src/expr.rs +++ b/compiler/rustc_ast_lowering/src/expr.rs @@ -358,7 +358,7 @@ impl<'hir> LoweringContext<'_, 'hir> { let rest = match &se.rest { StructRest::Base(e) => hir::StructTailExpr::Base(self.lower_expr(e)), StructRest::Rest(sp) => hir::StructTailExpr::DefaultFields(*sp), - StructRest::None => hir::StructTailExpr::None, + StructRest::None(sp) => hir::StructTailExpr::None(*sp), }; hir::ExprKind::Struct( self.arena.alloc(self.lower_qpath( @@ -1419,7 +1419,7 @@ impl<'hir> LoweringContext<'_, 'hir> { true } StructRest::Rest(_) => true, - StructRest::None => false, + StructRest::None(_) => false, }; let struct_pat = hir::PatKind::Struct(qpath, field_pats, fields_omitted); return self.pat_without_dbm(lhs.span, struct_pat); @@ -1530,7 +1530,7 @@ impl<'hir> LoweringContext<'_, 'hir> { hir::ExprKind::Struct( self.arena.alloc(hir::QPath::LangItem(lang_item, self.lower_span(span))), fields, - hir::StructTailExpr::None, + hir::StructTailExpr::None(DUMMY_SP), ) } diff --git a/compiler/rustc_ast_pretty/src/pprust/state/expr.rs b/compiler/rustc_ast_pretty/src/pprust/state/expr.rs index dce76fb1e7707..a41cd5bffa06b 100644 --- a/compiler/rustc_ast_pretty/src/pprust/state/expr.rs +++ b/compiler/rustc_ast_pretty/src/pprust/state/expr.rs @@ -161,7 +161,7 @@ impl<'a> State<'a> { self.word("{"); let has_rest = match rest { ast::StructRest::Base(_) | ast::StructRest::Rest(_) => true, - ast::StructRest::None => false, + ast::StructRest::None(_) => false, }; if fields.is_empty() && !has_rest { self.word("}"); diff --git a/compiler/rustc_expand/src/build.rs b/compiler/rustc_expand/src/build.rs index 22bfda34cc0e5..94d95fcec648f 100644 --- a/compiler/rustc_expand/src/build.rs +++ b/compiler/rustc_expand/src/build.rs @@ -367,7 +367,7 @@ impl<'a> ExtCtxt<'a> { qself: None, path, fields, - rest: ast::StructRest::None, + rest: ast::StructRest::None(DUMMY_SP), })), ) } diff --git a/compiler/rustc_hir/src/hir.rs b/compiler/rustc_hir/src/hir.rs index 8cea269f29823..e0e0265275c72 100644 --- a/compiler/rustc_hir/src/hir.rs +++ b/compiler/rustc_hir/src/hir.rs @@ -2116,7 +2116,7 @@ impl Expr<'_> { ExprKind::Struct(_, fields, init) => { let init_side_effects = match init { StructTailExpr::Base(init) => init.can_have_side_effects(), - StructTailExpr::DefaultFields(_) | StructTailExpr::None => false, + StructTailExpr::DefaultFields(_) | StructTailExpr::None(_) => false, }; fields.iter().map(|field| field.expr).any(|e| e.can_have_side_effects()) || init_side_effects @@ -2191,48 +2191,48 @@ impl Expr<'_> { ExprKind::Struct( QPath::LangItem(LangItem::RangeTo, _), [val1], - StructTailExpr::None, + StructTailExpr::None(_), ), ExprKind::Struct( QPath::LangItem(LangItem::RangeTo, _), [val2], - StructTailExpr::None, + StructTailExpr::None(_), ), ) | ( ExprKind::Struct( QPath::LangItem(LangItem::RangeToInclusive, _), [val1], - StructTailExpr::None, + StructTailExpr::None(_), ), ExprKind::Struct( QPath::LangItem(LangItem::RangeToInclusive, _), [val2], - StructTailExpr::None, + StructTailExpr::None(_), ), ) | ( ExprKind::Struct( QPath::LangItem(LangItem::RangeFrom, _), [val1], - StructTailExpr::None, + StructTailExpr::None(_), ), ExprKind::Struct( QPath::LangItem(LangItem::RangeFrom, _), [val2], - StructTailExpr::None, + StructTailExpr::None(_), ), ) => val1.expr.equivalent_for_indexing(val2.expr), ( ExprKind::Struct( QPath::LangItem(LangItem::Range, _), [val1, val3], - StructTailExpr::None, + StructTailExpr::None(_), ), ExprKind::Struct( QPath::LangItem(LangItem::Range, _), [val2, val4], - StructTailExpr::None, + StructTailExpr::None(_), ), ) => { val1.expr.equivalent_for_indexing(val2.expr) @@ -2428,7 +2428,7 @@ pub enum ExprKind<'hir> { #[derive(Debug, Clone, Copy, HashStable_Generic)] pub enum StructTailExpr<'hir> { /// A struct expression where all the fields are explicitly enumerated: `Foo { a, b }`. - None, + None(Span), /// A struct expression with a "base", an expression of the same type as the outer struct that /// will be used to populate any fields not explicitly mentioned: `Foo { ..base }` Base(&'hir Expr<'hir>), diff --git a/compiler/rustc_hir/src/intravisit.rs b/compiler/rustc_hir/src/intravisit.rs index 387a195cb2981..0f40cf678290f 100644 --- a/compiler/rustc_hir/src/intravisit.rs +++ b/compiler/rustc_hir/src/intravisit.rs @@ -749,7 +749,7 @@ pub fn walk_expr<'v, V: Visitor<'v>>(visitor: &mut V, expression: &'v Expr<'v>) walk_list!(visitor, visit_expr_field, fields); match optional_base { StructTailExpr::Base(base) => try_visit!(visitor.visit_expr(base)), - StructTailExpr::None | StructTailExpr::DefaultFields(_) => {} + StructTailExpr::None(_) | StructTailExpr::DefaultFields(_) => {} } } ExprKind::Tup(subexpressions) => { diff --git a/compiler/rustc_hir_pretty/src/lib.rs b/compiler/rustc_hir_pretty/src/lib.rs index 5c1c58921907e..af70d68acbb68 100644 --- a/compiler/rustc_hir_pretty/src/lib.rs +++ b/compiler/rustc_hir_pretty/src/lib.rs @@ -1226,7 +1226,7 @@ impl<'a> State<'a> { self.word(".."); self.end(); } - hir::StructTailExpr::None => { + hir::StructTailExpr::None(_) => { if !fields.is_empty() { self.word(","); } diff --git a/compiler/rustc_hir_typeck/src/expr_use_visitor.rs b/compiler/rustc_hir_typeck/src/expr_use_visitor.rs index ecbae6ac72fa2..0876f166b58c7 100644 --- a/compiler/rustc_hir_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_hir_typeck/src/expr_use_visitor.rs @@ -707,7 +707,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx let with_expr = match *opt_with { hir::StructTailExpr::Base(w) => &*w, - hir::StructTailExpr::DefaultFields(_) | hir::StructTailExpr::None => { + hir::StructTailExpr::DefaultFields(_) | hir::StructTailExpr::None(_) => { return Ok(()); } }; 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..cb92d8fbba04d --- /dev/null +++ b/compiler/rustc_lint/src/default_could_be_derived.rs @@ -0,0 +1,622 @@ +use std::cmp::min; + +use rustc_ast::LitKind; +use rustc_data_structures::fx::FxHashMap; +use rustc_errors::{Applicability, Diag}; +use rustc_hir as hir; +use rustc_hir::def::{CtorKind, DefKind}; +use rustc_hir::def_id::DefId; +use rustc_middle::mir; +use rustc_middle::ty::{self, Instance, Ty}; +use rustc_session::{Session, declare_lint, impl_lint_pass}; +use rustc_span::symbol::{kw, sym}; +use rustc_span::{Span, Symbol}; + +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 + /// #![feature(default_field_values)] + /// struct A { + /// b: Option, + /// c: Option = None, + /// } + /// + /// #[deny(default_could_be_derived)] + /// impl Default for A { + /// fn default() -> A { + /// A { + /// b: None, + /// c: Some(0), + /// } + /// } + /// } + /// ``` + /// + /// {{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", + // FIXME: this logic can be extended to check for types without default field values + // doing the same as `clippy::derivable_impls`, with slightly more information thanks + // to the added "default equivalence" table. + @feature_gate = default_field_values; +} + +declare_lint! { + /// The `default_overrides_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, + /// y: NonDefault, + /// } + /// + /// struct NonDefault; + /// + /// #[deny(default_overrides_default_fields)] + /// impl Default for Foo { + /// fn default() -> Foo { + /// Foo { x: 100, y: NonDefault } + /// } + /// } + /// ``` + /// + /// {{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. If `Default` can't be derived due to some fields not + /// having a `Default` implementation, we encourage the use of `..` for + /// the fields that do have a default field value. + pub DEFAULT_OVERRIDES_DEFAULT_FIELDS, + Warn, + "detect `Default` impl that should use the type's default field values", + @feature_gate = default_field_values; +} + +#[derive(Default)] +pub(crate) struct DefaultCouldBeDerived { + data: Option, +} + +struct Data { + type_def_id: DefId, + parent: DefId, +} + +impl_lint_pass!(DefaultCouldBeDerived => [DEFAULT_COULD_BE_DERIVED, DEFAULT_OVERRIDES_DEFAULT_FIELDS]); + +impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { + fn check_impl_item(&mut self, cx: &LateContext<'_>, impl_item: &hir::ImplItem<'_>) { + let Some(default_def_id) = cx.tcx.get_diagnostic_item(sym::Default) else { return }; + let assoc = cx.tcx.associated_item(impl_item.owner_id); + let parent = assoc.container_id(cx.tcx); + if cx.tcx.has_attr(parent, sym::automatically_derived) { + return; + } + let Some(trait_ref) = cx.tcx.impl_trait_ref(parent) else { return }; + let trait_ref = trait_ref.instantiate_identity(); + if trait_ref.def_id != default_def_id { + return; + } + let ty = trait_ref.self_ty(); + let ty::Adt(def, _) = ty.kind() else { return }; + + // We have a manually written definition of a `::default()`. We store the + // necessary metadata for further analysis of its body in `check_body`. + self.data = Some(Data { type_def_id: def.did(), parent }); + } + + fn check_impl_item_post(&mut self, _cx: &LateContext<'_>, _impl_item: &hir::ImplItem<'_>) { + // Clean up. + self.data = None; + } + + fn check_body(&mut self, cx: &LateContext<'tcx>, body: &hir::Body<'tcx>) { + // We perform this logic here instead of in `check_impl_item` so that we have unconditional + // permission to call `cx.typeck_results()` (because we're within a body). + let Some(Data { type_def_id, parent }) = self.data else { + return; + }; + // FIXME: evaluate bodies with statements and evaluate bindings to see if they would be + // derivable. + let hir::ExprKind::Block(hir::Block { stmts: _, expr: Some(expr), .. }, None) = + body.value.kind + else { + return; + }; + + let hir = cx.tcx.hir(); + + // Keep a mapping of field name to `hir::FieldDef` for every field in the type. We'll use + // these to check for things like checking whether it has a default or using its span for + // suggestions. + let orig_fields = match hir.get_if_local(type_def_id) { + Some(hir::Node::Item(hir::Item { + kind: + hir::ItemKind::Struct(hir::VariantData::Struct { fields, recovered: _ }, _generics), + .. + })) => fields.iter().map(|f| (f.ident.name, f)).collect::>(), + _ => Default::default(), + }; + + // 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()`. + let hir::ExprKind::Struct(_qpath, fields, tail) = expr.kind else { return }; + + // We have a struct literal + // + // struct Foo { + // field: Type, + // } + // + // impl Default for Foo { + // fn default() -> Foo { + // Foo { + // field: val, + // } + // } + // } + // + // We suggest #[derive(Default)] if + // - `field` has a default value, regardless of what it is; we don't want to + // encourage divergent behavior between `Default::default()` and `..` + // - `val` is `Default::default()` + // - `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. + // We have nothing meaninful to do with this. + return; + } + + // All the fields that have been provided can be used by `#[derive(Default)]`. This checks + // against what is *already* in the type. It doesn't account for fields where we can suggest + // adding a default field value. + let all_defaultable = fields.iter().all(|f| { + orig_fields.get(&f.ident.name).and_then(|f| f.default).is_some() + || check_expr(cx, f.expr) + }); + + // Any of the fields provided either already has a default value or the expression passed + // is either a `Default::default()` (or equivalent) call or an expression suitable to be + // used as a default field value. In the former, we suggest `#[derive(Default)]`. In the + // latter, we also suggest setting the field to the expression used in the impl for that + // field. + let any_defaultable = fields.iter().all(|f| { + orig_fields.get(&f.ident.name).and_then(|f| f.default).is_some() + || check_expr(cx, f.expr) + || is_expr_const(cx, f.expr) + }); + #[allow(rustc::potential_query_instability)] + // The type has default field values. We will not attempt to suggest anything unless the + // type already has at least one default. + let any_default_field = orig_fields.iter().any(|(_, f)| f.default.is_some()); + + // At least one of the fields with a default value have been overriden in + // the `Default` implementation. We suggest removing it and relying on `..` + // instead. + let any_default_field_given = + fields.iter().any(|f| orig_fields.get(&f.ident.name).and_then(|f| f.default).is_some()); + + if !(any_default_field_given || (any_defaultable && any_default_field)) { + return; + } + + let Some(local) = parent.as_local() else { return }; + let hir_id = cx.tcx.local_def_id_to_hir_id(local); + let hir::Node::Item(item) = cx.tcx.hir_node(hir_id) else { return }; + cx.tcx.node_span_lint( + if any_default_field_given && !all_defaultable { + DEFAULT_OVERRIDES_DEFAULT_FIELDS + } else { + DEFAULT_COULD_BE_DERIVED + }, + hir_id, + item.span, + |diag| { + mk_lint( + cx, + diag, + type_def_id, + item, + any_default_field_given, + orig_fields, + fields, + &tail, + ); + }, + ); + } +} + +fn mk_lint<'tcx>( + cx: &LateContext<'tcx>, + diag: &mut Diag<'_, ()>, + type_def_id: DefId, + item: &hir::Item<'_>, + any_default_field_given: bool, + orig_fields: FxHashMap>, + fields: &[hir::ExprField<'_>], + tail: &hir::StructTailExpr<'_>, +) { + diag.primary_message(if any_default_field_given { + "`Default` impl doesn't use the declared default field values" + } else { + "`Default` impl that could be derived" + }); + + let mut removals = vec![]; + let mut additions = vec![]; + let removal_span = |removals: &mut Vec, idx: usize| { + // We get the span for the field at `idx` *including* either the previous or + // following `,`. + let field = fields[idx]; + if idx > 0 + && let Some(prev_field) = fields.get(idx - 1) + { + // Span covering the current field *and* the prior `,` for the prior field. + removals.push(prev_field.span.shrink_to_hi().to(field.span)); + } else if let Some(next_field) = fields.get(idx + 1) { + // Span for the current field *and* its trailing comma, all the way to the + // next field. + removals.push(field.span.until(next_field.span)); + } else if idx + 1 == fields.len() + && let hir::StructTailExpr::DefaultFields(span) = tail + { + // This is the last field *and* there's a `, ..`. This span covers this + // entire field and the `, ..`. + removals.push(field.span.until(*span)); + } else { + // The span for the current field, without any commas. This is a fallback + // that shouldn't really trigger. + removals.push(field.span); + } + }; + + // Hold on to your butts, friends. This is a lot of subte logic that I'm trying to + // make as easy to follow as possible, but in the end I managed to confuse myself + // so I'm writing the high level idea here first. + // + // For each field in the struct expression + // - if the field in the type has a default value, we will remove it + // - elif the field is an expression that could be a default value, we will remove + // it and add it to the type's field + // - else, we won't touch this field, it will remain in the impl, but if the + // previous field *was* removed we look for the span of the previous `,` to + // remove it + let mut prev_removed = 0; + let mut removed_all_fields = true; + for (i, field) in fields.iter().enumerate() { + if orig_fields.get(&field.ident.name).and_then(|f| f.default).is_some() { + diag.span_label(field.expr.span, "this field has a default value"); + removal_span(&mut removals, i); + prev_removed = i; + } else if is_expr_const(cx, &field.expr) { + diag.span_label(field.expr.span, "this value can be used as a default field value"); + removal_span(&mut removals, i); + prev_removed = i; + let snippet = cx.tcx.sess.source_map().span_to_snippet(field.expr.span); + if let Ok(snippet) = snippet + && let Some(def_field) = orig_fields.get(&field.ident.name) + // If the value is the same as their type's `Default::default()`, don't + // set the default field value. The user can do that if they want it. + && !check_expr(cx, field.expr) + { + let snippet = reindent(&cx.tcx.sess, snippet, def_field.span); + additions.push(match def_field.default { + Some(anon) => (anon.span, snippet), + None => (def_field.span.shrink_to_hi(), format!(" = {snippet}")), + }); + } + } else { + if prev_removed + 1 == i { + // Remove the `,` between the previous field which was + // removed and the current one, which is kept. + removals.push(fields[prev_removed].span.shrink_to_hi().until(field.span)); + } + removed_all_fields = false; + } + } + + // We cheated above, given `S { a: x, b: y, c: z }` we might + // suggest to remove one of the commas *twice*, which suggestions + // really don't like, so we ensure that even if we're removing + // multiple fields, they never overlap. + removals.sort(); + let mut removals_iter = removals.iter_mut().peekable(); + while let Some(removal) = removals_iter.next() { + if let Some(next) = removals_iter.peek() { + if removal.overlaps(**next) { + *removal = removal.with_hi(next.lo()); + } + } else if let hir::StructTailExpr::None(span) = tail + && removal.overlaps(*span) + { + *removal = removal.with_hi(span.lo()); + } + } + // We don't want empty spans, which suggestions also don't like. + removals.retain(|s| s.lo() != s.hi()); + + // Construct the suggestions. + if !additions.is_empty() && removed_all_fields { + // Suggest `#[derive(Default)]` as we removed all the fields and + // moved their values to their fields' default. + additions.insert( + 0, + (cx.tcx.def_span(type_def_id).shrink_to_lo(), "#[derive(Default)] ".to_string()), + ); + additions.push((item.span, String::new())); + diag.multipart_suggestion_verbose( + "set the default field values of your type to the value used in the \ + `Default` implementation and derive it", + additions, + Applicability::MachineApplicable, + ); + } else if removed_all_fields { + // Suggest `#[derive(Default)]` as we removed all the fields. + 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, + ); + } else { + // Suggest moving some values to their field's default, leaving + // the `impl Default for Type {}` item in place but using the + // default values with `..`. + let mut sugg: Vec<(Span, String)> = removals + .into_iter() + .map(|sp| (sp, String::new())) + .chain(additions.into_iter()) + .collect(); + match tail { + hir::StructTailExpr::Base(_) => { + // There's already a trailing `..`, we don't need to suggest anything. + } + hir::StructTailExpr::DefaultFields(_) => {} + hir::StructTailExpr::None(span) => { + sugg.push((*span, ", ..".to_string())); + } + } + // For: + // + // struct S { + // a: Ty, + // b: i32 = 101, + // } + // + // impl Default for S { + // fn default() -> S { + // S { + // a: foo(), + // b: 100, + // } + // } + // } + // + // We suggest + // + // impl Default for S { + // fn default() -> S { + // S { + // a: foo(), .. + // } + // } + // } + diag.multipart_suggestion_verbose( + "use the default values in the `impl` to avoid them diverging over time", + sugg, + Applicability::MachineApplicable, + ); + } +} + +/// For the `Default` impl for this type, we see if it has a `Default::default()` body composed +/// only of a path, ctor or function call with no arguments. If so, we compare that `DefId` +/// against the `DefId` of this field's value if it is also a call/path/ctor. +/// 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. +fn check_path<'tcx>( + cx: &LateContext<'tcx>, + path: &hir::QPath<'_>, + hir_id: hir::HirId, + ty: Ty<'tcx>, +) -> bool { + let res = cx.qpath_res(&path, hir_id); + let Some(def_id) = res.opt_def_id() else { return false }; + let Some(default_fn_def_id) = cx.tcx.get_diagnostic_item(sym::default_fn) else { + return false; + }; + if default_fn_def_id == def_id { + // We have `field: Default::default(),`. This is what the derive would do already. + return true; + } + + let args = ty::GenericArgs::for_item(cx.tcx, default_fn_def_id, |param, _| { + if let ty::GenericParamDefKind::Lifetime = param.kind { + cx.tcx.lifetimes.re_erased.into() + } else if param.index == 0 && param.name == kw::SelfUpper { + ty.into() + } else { + param.to_error(cx.tcx) + } + }); + let instance = Instance::try_resolve(cx.tcx, cx.typing_env(), default_fn_def_id, args); + + let Ok(Some(instance)) = instance else { return false }; + if let ty::InstanceKind::Item(def) = instance.def + && !cx.tcx.is_mir_available(def) + { + // Avoid ICE while running rustdoc for not providing `optimized_mir` query. + return false; + } + + // Get the MIR Body for the `::default()` function. + // If it is a value or call (either fn or ctor), we compare its DefId against the one for the + // resolution of the expression we had in the path. This lets us identify, for example, that + // the body of ` as Default>::default()` is a `Vec::new()`, and the field was being + // initialized to `Vec::new()` as well. + let body = cx.tcx.instance_mir(instance.def); + for block_data in body.basic_blocks.iter() { + if block_data.statements.len() == 1 + && let mir::StatementKind::Assign(assign) = &block_data.statements[0].kind + && assign.0.local == mir::RETURN_PLACE + && let mir::Rvalue::Aggregate(kind, _places) = &assign.1 + && let mir::AggregateKind::Adt(did, variant_index, _, _, _) = &**kind + && let def = cx.tcx.adt_def(did) + && let variant = &def.variant(*variant_index) + && variant.fields.is_empty() + && let Some((_, did)) = variant.ctor + && did == def_id + { + return true; + } else if block_data.statements.len() == 0 + && let Some(term) = &block_data.terminator + { + match &term.kind { + mir::TerminatorKind::Call { func: mir::Operand::Constant(c), .. } + if let ty::FnDef(did, _args) = c.ty().kind() + && *did == def_id => + { + return true; + } + mir::TerminatorKind::TailCall { func: mir::Operand::Constant(c), .. } + if let ty::FnDef(did, _args) = c.ty().kind() + && *did == def_id => + { + return true; + } + _ => {} + } + } + } + false +} + +/// Given the snippet for an expression, indent it to fit in a field default value. +/// We remove the indent of every line but the first (keeping the general relative shape), and then +/// add the indent for the field span. +fn reindent(sess: &Session, snippet: String, field_span: Span) -> String { + // Remove the preexisting indentation... + let mut indent = usize::MAX; + for line in snippet.lines().skip(1) { + indent = min(indent, line.len() - line.trim_start().len()); + } + if indent == usize::MAX { + indent = 0; + } + let new_indent = sess.source_map().indentation_before(field_span).unwrap_or_else(String::new); + snippet + .lines() + .enumerate() + .map(|(i, s)| { + if i == 0 { + s.to_string() + } else { + // ...and add the indentation of the field. + format!("{new_indent}{}", &s[indent..]) + } + }) + .collect::>() + .join("\n") +} + +/// Given an expression, determine if it would have been the same that would be used by +/// `#[derive(Default)]`. +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(hir::Expr { kind: hir::ExprKind::Path(path), hir_id, .. }, []) => { + // `field: foo(),` or `field: Ty::assoc(),` + let Some(ty) = cx.typeck_results().expr_ty_adjusted_opt(expr) else { + return false; + }; + check_path(cx, &path, *hir_id, ty) + } + hir::ExprKind::Path(path) => { + // `field: qualified::Path,` or `field: ::Assoc,` + let Some(ty) = cx.typeck_results().expr_ty_adjusted_opt(expr) else { + return false; + }; + check_path(cx, &path, expr.hir_id, ty) + } + _ => false, + } +} + +/// Given a path, determine if it corresponds to a `const` item. +fn is_path_const<'tcx>(cx: &LateContext<'tcx>, path: &hir::QPath<'_>, hir_id: hir::HirId) -> bool { + let res = cx.qpath_res(&path, hir_id); + let Some(def_id) = res.opt_def_id() else { return false }; + let def_kind = cx.tcx.def_kind(def_id); + match def_kind { + DefKind::Const + | DefKind::ConstParam + | DefKind::AssocConst + | DefKind::AnonConst + | DefKind::Ctor(_, CtorKind::Const) => true, + + DefKind::Fn | DefKind::AssocFn | DefKind::Ctor(_, CtorKind::Fn) => { + cx.tcx.is_const_fn(def_id) + } + + _ => false, + } +} + +/// Given an expression, determine if it would be suitable for a default field value. +fn is_expr_const(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool { + match expr.kind { + hir::ExprKind::Lit(_) => true, + hir::ExprKind::Call(hir::Expr { kind: hir::ExprKind::Path(path), hir_id, .. }, args) => { + is_path_const(cx, &path, *hir_id) && args.iter().all(|e| is_expr_const(cx, e)) + } + hir::ExprKind::Path(path) => is_path_const(cx, &path, expr.hir_id), + hir::ExprKind::Struct(_, fields, _) => fields.iter().all(|f| is_expr_const(cx, &f.expr)), + // FIXME: support const traits + _ => false, + } +} diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index d7f0d2a6941fb..1465c2cff7bc1 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::default(), DerefIntoDynSupertrait: DerefIntoDynSupertrait, DropForgetUseless: DropForgetUseless, ImproperCTypesDeclarations: ImproperCTypesDeclarations, diff --git a/compiler/rustc_middle/src/query/mod.rs b/compiler/rustc_middle/src/query/mod.rs index 2c2dffe8b88f0..3480cb4a6a461 100644 --- a/compiler/rustc_middle/src/query/mod.rs +++ b/compiler/rustc_middle/src/query/mod.rs @@ -1805,6 +1805,7 @@ rustc_queries! { desc { "looking up lifetime defaults for generic parameter `{}`", 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_mir_build/src/thir/cx/expr.rs b/compiler/rustc_mir_build/src/thir/cx/expr.rs index 0338ac674e5e2..1a4e6c104ce4a 100644 --- a/compiler/rustc_mir_build/src/thir/cx/expr.rs +++ b/compiler/rustc_mir_build/src/thir/cx/expr.rs @@ -620,7 +620,7 @@ impl<'tcx> Cx<'tcx> { .collect(), ) } - hir::StructTailExpr::None => AdtExprBase::None, + hir::StructTailExpr::None(_) => AdtExprBase::None, }, })) } @@ -630,7 +630,7 @@ impl<'tcx> Cx<'tcx> { Res::Def(DefKind::Variant, variant_id) => { assert!(matches!( base, - hir::StructTailExpr::None + hir::StructTailExpr::None(_) | hir::StructTailExpr::DefaultFields(_) )); @@ -659,7 +659,7 @@ impl<'tcx> Cx<'tcx> { hir::StructTailExpr::Base(base) => { span_bug!(base.span, "unexpected res: {:?}", res); } - hir::StructTailExpr::None => AdtExprBase::None, + hir::StructTailExpr::None(_) => AdtExprBase::None, }, })) } diff --git a/compiler/rustc_parse/src/parser/expr.rs b/compiler/rustc_parse/src/parser/expr.rs index 7533e75ffe261..df289dfada08f 100644 --- a/compiler/rustc_parse/src/parser/expr.rs +++ b/compiler/rustc_parse/src/parser/expr.rs @@ -3525,7 +3525,8 @@ impl<'a> Parser<'a> { ), > { let mut fields = ThinVec::new(); - let mut base = ast::StructRest::None; + // This would be retained only if the current token is `}`. Otherwise, it's discarded. + let mut base = ast::StructRest::None(self.token.span.shrink_to_lo()); let mut recovered_async = None; let in_if_guard = self.restrictions.contains(Restrictions::IN_IF_GUARD); @@ -3652,6 +3653,12 @@ impl<'a> Parser<'a> { // Only include the field if there's no parse error for the field name. fields.push(f); } + let span = if token::Comma == self.prev_token.kind { + self.prev_token.span + } else { + self.prev_token.span.shrink_to_hi() + }; + base = ast::StructRest::None(span); } Err(mut e) => { if pth == kw::Async { @@ -3678,6 +3685,12 @@ impl<'a> Parser<'a> { } self.recover_stmt_(SemiColonMode::Comma, BlockMode::Ignore); let _ = self.eat(exp!(Comma)); + let span = if token::Comma == self.prev_token.kind { + self.prev_token.span + } else { + self.prev_token.span.shrink_to_hi() + }; + base = ast::StructRest::None(span); } } } diff --git a/compiler/rustc_passes/src/liveness.rs b/compiler/rustc_passes/src/liveness.rs index b85a987c64119..de740962acfe5 100644 --- a/compiler/rustc_passes/src/liveness.rs +++ b/compiler/rustc_passes/src/liveness.rs @@ -1011,7 +1011,7 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> { hir::StructTailExpr::Base(base) => { self.propagate_through_opt_expr(Some(base), succ) } - hir::StructTailExpr::None | hir::StructTailExpr::DefaultFields(_) => succ, + hir::StructTailExpr::None(_) | hir::StructTailExpr::DefaultFields(_) => succ, }; fields .iter() diff --git a/compiler/rustc_privacy/src/lib.rs b/compiler/rustc_privacy/src/lib.rs index c50c9007a010d..20ceb0c663606 100644 --- a/compiler/rustc_privacy/src/lib.rs +++ b/compiler/rustc_privacy/src/lib.rs @@ -994,7 +994,7 @@ impl<'tcx> Visitor<'tcx> for NamePrivacyVisitor<'tcx> { hir::StructTailExpr::DefaultFields(span) => { self.check_expanded_fields(adt, variant, fields, expr.hir_id, span); } - hir::StructTailExpr::None => { + hir::StructTailExpr::None(_) => { for field in fields { let (hir_id, use_ctxt, span) = (field.hir_id, field.ident.span, field.span); let index = self.typeck_results().field_index(field.hir_id); diff --git a/compiler/rustc_resolve/src/late.rs b/compiler/rustc_resolve/src/late.rs index 7324d3fe7862d..953dfe18942e5 100644 --- a/compiler/rustc_resolve/src/late.rs +++ b/compiler/rustc_resolve/src/late.rs @@ -4676,7 +4676,7 @@ impl<'a, 'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> { match &se.rest { StructRest::Base(expr) => self.visit_expr(expr), StructRest::Rest(_span) => {} - StructRest::None => {} + StructRest::None(_span) => {} } } diff --git a/src/tools/clippy/clippy_lints/src/init_numbered_fields.rs b/src/tools/clippy/clippy_lints/src/init_numbered_fields.rs index 7a14bbfb9e8b7..0cc895621afbd 100644 --- a/src/tools/clippy/clippy_lints/src/init_numbered_fields.rs +++ b/src/tools/clippy/clippy_lints/src/init_numbered_fields.rs @@ -43,7 +43,7 @@ declare_lint_pass!(NumberedFields => [INIT_NUMBERED_FIELDS]); impl<'tcx> LateLintPass<'tcx> for NumberedFields { fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { - if let ExprKind::Struct(path, fields @ [field, ..], StructTailExpr::None) = e.kind + if let ExprKind::Struct(path, fields @ [field, ..], StructTailExpr::None(_)) = e.kind // If the first character of any field is a digit it has to be a tuple. && field.ident.as_str().as_bytes().first().is_some_and(u8::is_ascii_digit) // Type aliases can't be used as functions. diff --git a/src/tools/clippy/clippy_lints/src/no_effect.rs b/src/tools/clippy/clippy_lints/src/no_effect.rs index 9e44bb02c56fa..58e4fec766e3d 100644 --- a/src/tools/clippy/clippy_lints/src/no_effect.rs +++ b/src/tools/clippy/clippy_lints/src/no_effect.rs @@ -239,7 +239,7 @@ fn has_no_effect(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { !has_drop(cx, cx.typeck_results().expr_ty(expr)) && fields.iter().all(|field| has_no_effect(cx, field.expr)) && match &base { - StructTailExpr::None | StructTailExpr::DefaultFields(_) => true, + StructTailExpr::None(_)| StructTailExpr::DefaultFields(_) => true, StructTailExpr::Base(base) => has_no_effect(cx, base), } }, @@ -347,7 +347,7 @@ fn reduce_expression<'a>(cx: &LateContext<'_>, expr: &'a Expr<'a>) -> Option Some(base), - StructTailExpr::None | StructTailExpr::DefaultFields(_) => None, + StructTailExpr::None(_)| StructTailExpr::DefaultFields(_) => None, }; Some(fields.iter().map(|f| &f.expr).chain(base).map(Deref::deref).collect()) } diff --git a/src/tools/clippy/clippy_lints/src/single_range_in_vec_init.rs b/src/tools/clippy/clippy_lints/src/single_range_in_vec_init.rs index bb11daecc07da..bcb69ed78fd48 100644 --- a/src/tools/clippy/clippy_lints/src/single_range_in_vec_init.rs +++ b/src/tools/clippy/clippy_lints/src/single_range_in_vec_init.rs @@ -86,7 +86,7 @@ impl LateLintPass<'_> for SingleRangeInVecInit { return; }; - let ExprKind::Struct(QPath::LangItem(lang_item, ..), [start, end], StructTailExpr::None) = inner_expr.kind else { + let ExprKind::Struct(QPath::LangItem(lang_item, ..), [start, end], StructTailExpr::None(_)) = inner_expr.kind else { return; }; diff --git a/src/tools/clippy/clippy_lints/src/unnecessary_struct_initialization.rs b/src/tools/clippy/clippy_lints/src/unnecessary_struct_initialization.rs index 0a90d31db7e14..fea50d96ca45f 100644 --- a/src/tools/clippy/clippy_lints/src/unnecessary_struct_initialization.rs +++ b/src/tools/clippy/clippy_lints/src/unnecessary_struct_initialization.rs @@ -59,7 +59,7 @@ impl LateLintPass<'_> for UnnecessaryStruct { let field_path = same_path_in_all_fields(cx, expr, fields); let sugg = match (field_path, base) { - (Some(&path), StructTailExpr::None | StructTailExpr::DefaultFields(_)) => { + (Some(&path), StructTailExpr::None(_)| StructTailExpr::DefaultFields(_)) => { // all fields match, no base given path.span }, diff --git a/src/tools/clippy/clippy_lints/src/utils/author.rs b/src/tools/clippy/clippy_lints/src/utils/author.rs index 521bf6a5fede9..00b9c1a38b752 100644 --- a/src/tools/clippy/clippy_lints/src/utils/author.rs +++ b/src/tools/clippy/clippy_lints/src/utils/author.rs @@ -600,7 +600,7 @@ impl<'a, 'tcx> PrintVisitor<'a, 'tcx> { bind!(self, qpath, fields); let base = OptionPat::new(match base { StructTailExpr::Base(base) => Some(self.bind("base", base)), - StructTailExpr::None | StructTailExpr::DefaultFields(_) => None, + StructTailExpr::None(_) | StructTailExpr::DefaultFields(_) => None, }); kind!("Struct({qpath}, {fields}, {base})"); self.qpath(qpath); diff --git a/src/tools/clippy/clippy_utils/src/ast_utils.rs b/src/tools/clippy/clippy_utils/src/ast_utils.rs index 623d9c7608612..342781516b396 100644 --- a/src/tools/clippy/clippy_utils/src/ast_utils.rs +++ b/src/tools/clippy/clippy_utils/src/ast_utils.rs @@ -136,7 +136,7 @@ pub fn eq_expr_opt(l: Option<&P>, r: Option<&P>) -> bool { pub fn eq_struct_rest(l: &StructRest, r: &StructRest) -> bool { match (l, r) { (StructRest::Base(lb), StructRest::Base(rb)) => eq_expr(lb, rb), - (StructRest::Rest(_), StructRest::Rest(_)) | (StructRest::None, StructRest::None) => true, + (StructRest::Rest(_), StructRest::Rest(_)) | (StructRest::None(_), StructRest::None(_)) => true, _ => false, } } diff --git a/src/tools/clippy/clippy_utils/src/higher.rs b/src/tools/clippy/clippy_utils/src/higher.rs index d216879cbd25e..7f1eea1f94f65 100644 --- a/src/tools/clippy/clippy_utils/src/higher.rs +++ b/src/tools/clippy/clippy_utils/src/higher.rs @@ -236,7 +236,7 @@ impl<'a> Range<'a> { limits: ast::RangeLimits::Closed, }) }, - ExprKind::Struct(path, fields, StructTailExpr::None) => match (path, fields) { + ExprKind::Struct(path, fields, StructTailExpr::None(_)) => match (path, fields) { (QPath::LangItem(hir::LangItem::RangeFull, ..), []) => Some(Range { start: None, end: None, diff --git a/src/tools/clippy/clippy_utils/src/hir_utils.rs b/src/tools/clippy/clippy_utils/src/hir_utils.rs index 4b604f658b8d6..60054e3475ae4 100644 --- a/src/tools/clippy/clippy_utils/src/hir_utils.rs +++ b/src/tools/clippy/clippy_utils/src/hir_utils.rs @@ -386,7 +386,7 @@ impl HirEqInterExpr<'_, '_, '_> { self.eq_qpath(l_path, r_path) && match (lo, ro) { (StructTailExpr::Base(l),StructTailExpr::Base(r)) => self.eq_expr(l, r), - (StructTailExpr::None, StructTailExpr::None) => true, + (StructTailExpr::None(_), StructTailExpr::None(_)) => true, (StructTailExpr::DefaultFields(_), StructTailExpr::DefaultFields(_)) => true, _ => false, } diff --git a/src/tools/rustfmt/src/expr.rs b/src/tools/rustfmt/src/expr.rs index 16b7e7aa709dd..a67f84bdeed16 100644 --- a/src/tools/rustfmt/src/expr.rs +++ b/src/tools/rustfmt/src/expr.rs @@ -1649,7 +1649,7 @@ fn rewrite_struct_lit<'a>( let path_str = rewrite_path(context, PathContext::Expr, qself, path, path_shape)?; let has_base_or_rest = match struct_rest { - ast::StructRest::None if fields.is_empty() => return Ok(format!("{path_str} {{}}")), + ast::StructRest::None(_) if fields.is_empty() => return Ok(format!("{path_str} {{}}")), ast::StructRest::Rest(_) if fields.is_empty() => { return Ok(format!("{path_str} {{ .. }}")); } @@ -1679,7 +1679,7 @@ fn rewrite_struct_lit<'a>( match struct_rest { ast::StructRest::Base(expr) => Some(StructLitField::Base(&**expr)), ast::StructRest::Rest(span) => Some(StructLitField::Rest(*span)), - ast::StructRest::None => None, + ast::StructRest::None(_) => None, } .into_iter(), ); 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..f893d7de9c588 --- /dev/null +++ b/tests/ui/structs/manual-default-impl-could-be-derived.fixed @@ -0,0 +1,147 @@ +// Warn when we encounter a manual `Default` impl that could be derived. +// Restricted only to types using `default_field_values`. +//@ run-rustfix +#![feature(default_field_values)] +#![allow(dead_code)] +#![deny(default_could_be_derived, default_overrides_default_fields)] +struct S(i32); +fn s() -> S { S(1) } + +struct A { + x: S, + y: i32 = 1, +} + +impl Default for A { //~ ERROR default_overrides_default_fields + fn default() -> Self { + A { + x: s(), .. + } + } +} + +#[derive(Default)] struct B { + x: S = S(3), + y: i32 = 1, +} + + +struct C { + x: S, + y: i32 = 1, + z: i32 = 1, +} + +impl Default for C { //~ ERROR default_overrides_default_fields + fn default() -> Self { + C { + x: s(), + .. + } + } +} + +struct D { + x: S, + y: i32 = 1, + z: i32 = 1, +} + +impl Default for D { //~ ERROR default_overrides_default_fields + fn default() -> Self { + D { + x: s(), + .. + } + } +} + +struct E { + x: S, + y: i32 = 1, + z: i32 = 1, +} + +impl Default for E { //~ ERROR default_overrides_default_fields + fn default() -> Self { + E { + x: s(), .. + } + } +} + +// Let's ensure that the span for `x` and the span for `y` don't overlap when suggesting their +// removal in favor of their default field values. +struct E2 { + x: S, + y: i32 = 1, + z: i32 = 1, +} + +impl Default for E2 { //~ ERROR default_overrides_default_fields + fn default() -> Self { + E2 { + x: s(), .. + } + } +} + +fn i() -> i32 { + 1 +} + +// Account for a `const fn` being the `Default::default()` of a field's type. +#[derive(Default)] struct F { + x: G, + y: i32 = 1, +} + + +struct G; + +impl Default for G { // ok + fn default() -> Self { + g_const() + } +} + +const fn g_const() -> G { + G +} + +// Account for a `const fn` being used in `Default::default()`, even if the type doesn't use it as +// its own `Default`. We suggest setting the default field value in that case. +#[derive(Default)] struct H { + x: I = i_const(), + y: i32 = 1, +} + + +struct I; + +const fn i_const() -> I { + I +} + +// Account for a `const` and struct literal being the `Default::default()` of a field's type. +#[derive(Default)] struct M { + x: N = N_CONST, + y: i32 = 1, + z: A = A { + x: S(0), + y: 0, + }, +} + + +struct N; + +const N_CONST: N = N; + +#[derive(Default)] struct O { + x: Option, + y: i32 = 1, +} + + +fn main() {} 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..4f2d46069612e --- /dev/null +++ b/tests/ui/structs/manual-default-impl-could-be-derived.rs @@ -0,0 +1,195 @@ +// Warn when we encounter a manual `Default` impl that could be derived. +// Restricted only to types using `default_field_values`. +//@ run-rustfix +#![feature(default_field_values)] +#![allow(dead_code)] +#![deny(default_could_be_derived, default_overrides_default_fields)] +struct S(i32); +fn s() -> S { S(1) } + +struct A { + x: S, + y: i32 = 1, +} + +impl Default for A { //~ ERROR default_overrides_default_fields + fn default() -> Self { + A { + y: 0, + x: s(), + } + } +} + +struct B { + x: S = S(3), + y: i32 = 1, +} + +impl Default for B { //~ ERROR default_could_be_derived + fn default() -> Self { + B { + x: s(), + y: 0, + } + } +} + +struct C { + x: S, + y: i32 = 1, + z: i32 = 1, +} + +impl Default for C { //~ ERROR default_overrides_default_fields + fn default() -> Self { + C { + x: s(), + y: 0, + .. + } + } +} + +struct D { + x: S, + y: i32 = 1, + z: i32 = 1, +} + +impl Default for D { //~ ERROR default_overrides_default_fields + fn default() -> Self { + D { + y: 0, + x: s(), + .. + } + } +} + +struct E { + x: S, + y: i32 = 1, + z: i32 = 1, +} + +impl Default for E { //~ ERROR default_overrides_default_fields + fn default() -> Self { + E { + y: 0, + z: 0, + x: s(), + } + } +} + +// Let's ensure that the span for `x` and the span for `y` don't overlap when suggesting their +// removal in favor of their default field values. +struct E2 { + x: S, + y: i32 = 1, + z: i32 = 1, +} + +impl Default for E2 { //~ ERROR default_overrides_default_fields + fn default() -> Self { + E2 { + x: s(), + y: i(), + z: 0, + } + } +} + +fn i() -> i32 { + 1 +} + +// Account for a `const fn` being the `Default::default()` of a field's type. +struct F { + x: G, + y: i32 = 1, +} + +impl Default for F { //~ ERROR default_could_be_derived + fn default() -> Self { + F { + x: g_const(), + y: 0, + } + } +} + +struct G; + +impl Default for G { // ok + fn default() -> Self { + g_const() + } +} + +const fn g_const() -> G { + G +} + +// Account for a `const fn` being used in `Default::default()`, even if the type doesn't use it as +// its own `Default`. We suggest setting the default field value in that case. +struct H { + x: I, + y: i32 = 1, +} + +impl Default for H { //~ ERROR default_overrides_default_fields + fn default() -> Self { + H { + x: i_const(), + y: 0, + } + } +} + +struct I; + +const fn i_const() -> I { + I +} + +// Account for a `const` and struct literal being the `Default::default()` of a field's type. +struct M { + x: N, + y: i32 = 1, + z: A, +} + +impl Default for M { //~ ERROR default_could_be_derived + fn default() -> Self { + M { + x: N_CONST, + z: A { + x: S(0), + y: 0, + }, + .. + } + } +} + +struct N; + +const N_CONST: N = N; + +struct O { + x: Option, + y: i32 = 1, +} + +impl Default for O { //~ ERROR default_could_be_derived + fn default() -> Self { + O { + x: None, + y: 1, + } + } +} + +fn main() {} 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..4bd7e62003d8c --- /dev/null +++ b/tests/ui/structs/manual-default-impl-could-be-derived.stderr @@ -0,0 +1,216 @@ +error: `Default` impl doesn't use the declared default field values + --> $DIR/manual-default-impl-could-be-derived.rs:15:1 + | +LL | / impl Default for A { +LL | | fn default() -> Self { +LL | | A { +LL | | y: 0, + | | - this field has a default value +... | +LL | | } + | |_^ + | +note: the lint level is defined here + --> $DIR/manual-default-impl-could-be-derived.rs:6:35 + | +LL | #![deny(default_could_be_derived, default_overrides_default_fields)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: use the default values in the `impl` to avoid them diverging over time + | +LL - y: 0, +LL + x: s(), .. + | + +error: `Default` impl doesn't use the declared default field values + --> $DIR/manual-default-impl-could-be-derived.rs:29:1 + | +LL | / impl Default for B { +LL | | fn default() -> Self { +LL | | B { +LL | | x: s(), + | | --- this field has a default value +LL | | y: 0, + | | - this field has a default value +... | +LL | | } + | |_^ + | +note: the lint level is defined here + --> $DIR/manual-default-impl-could-be-derived.rs:6:9 + | +LL | #![deny(default_could_be_derived, default_overrides_default_fields)] + | ^^^^^^^^^^^^^^^^^^^^^^^^ +help: to avoid divergence in behavior between `Struct { .. }` and `::default()`, derive the `Default` + | +LL ~ #[derive(Default)] struct B { + | + +error: `Default` impl doesn't use the declared default field values + --> $DIR/manual-default-impl-could-be-derived.rs:44:1 + | +LL | / impl Default for C { +LL | | fn default() -> Self { +LL | | C { +LL | | x: s(), +LL | | y: 0, + | | - this field has a default value +... | +LL | | } + | |_^ + | +help: use the default values in the `impl` to avoid them diverging over time + | +LL - x: s(), +LL - y: 0, +LL + x: s(), + | + +error: `Default` impl doesn't use the declared default field values + --> $DIR/manual-default-impl-could-be-derived.rs:60:1 + | +LL | / impl Default for D { +LL | | fn default() -> Self { +LL | | D { +LL | | y: 0, + | | - this field has a default value +... | +LL | | } + | |_^ + | +help: use the default values in the `impl` to avoid them diverging over time + | +LL - y: 0, +LL + x: s(), + | + +error: `Default` impl doesn't use the declared default field values + --> $DIR/manual-default-impl-could-be-derived.rs:76:1 + | +LL | / impl Default for E { +LL | | fn default() -> Self { +LL | | E { +LL | | y: 0, + | | - this field has a default value +LL | | z: 0, + | | - this field has a default value +... | +LL | | } + | |_^ + | +help: use the default values in the `impl` to avoid them diverging over time + | +LL - y: 0, +LL + x: s(), .. + | + +error: `Default` impl doesn't use the declared default field values + --> $DIR/manual-default-impl-could-be-derived.rs:94:1 + | +LL | / impl Default for E2 { +LL | | fn default() -> Self { +LL | | E2 { +LL | | x: s(), +LL | | y: i(), + | | --- this field has a default value +LL | | z: 0, + | | - this field has a default value +... | +LL | | } + | |_^ + | +help: use the default values in the `impl` to avoid them diverging over time + | +LL - x: s(), +LL - y: i(), +LL + x: s(), .. + | + +error: `Default` impl doesn't use the declared default field values + --> $DIR/manual-default-impl-could-be-derived.rs:114:1 + | +LL | / impl Default for F { +LL | | fn default() -> Self { +LL | | F { +LL | | x: g_const(), + | | --------- this value can be used as a default field value +LL | | y: 0, + | | - this field has a default value +... | +LL | | } + | |_^ + | +help: to avoid divergence in behavior between `Struct { .. }` and `::default()`, derive the `Default` + | +LL ~ #[derive(Default)] struct F { + | + +error: `Default` impl doesn't use the declared default field values + --> $DIR/manual-default-impl-could-be-derived.rs:142:1 + | +LL | / impl Default for H { +LL | | fn default() -> Self { +LL | | H { +LL | | x: i_const(), + | | --------- this value can be used as a default field value +LL | | y: 0, + | | - this field has a default value +... | +LL | | } + | |_^ + | +help: set the default field values of your type to the value used in the `Default` implementation and derive it + | +LL ~ #[derive(Default)] struct H { +LL ~ x: I = i_const(), + | + +error: `Default` impl that could be derived + --> $DIR/manual-default-impl-could-be-derived.rs:164:1 + | +LL | / impl Default for M { +LL | | fn default() -> Self { +LL | | M { +LL | | x: N_CONST, + | | ------- this value can be used as a default field value +LL | | z: A { + | | ________________- +LL | || x: S(0), +LL | || y: 0, +LL | || }, + | ||_____________- this value can be used as a default field value +... | +LL | | } + | |__^ + | +help: set the default field values of your type to the value used in the `Default` implementation and derive it + | +LL ~ #[derive(Default)] struct M { +LL ~ x: N = N_CONST, +LL | y: i32 = 1, +LL ~ z: A = A { +LL + x: S(0), +LL + y: 0, +LL ~ }, + | + +error: `Default` impl doesn't use the declared default field values + --> $DIR/manual-default-impl-could-be-derived.rs:186:1 + | +LL | / impl Default for O { +LL | | fn default() -> Self { +LL | | O { +LL | | x: None, + | | ---- this value can be used as a default field value +LL | | y: 1, + | | - this field has a default value +... | +LL | | } + | |_^ + | +help: to avoid divergence in behavior between `Struct { .. }` and `::default()`, derive the `Default` + | +LL ~ #[derive(Default)] struct O { + | + +error: aborting due to 10 previous errors +