diff options
| author | Esteban Küber <esteban@kuber.com.ar> | 2024-12-24 23:41:50 +0000 |
|---|---|---|
| committer | Esteban Küber <esteban@kuber.com.ar> | 2024-12-25 23:25:45 +0000 |
| commit | 01307cf03f5b95759a8e84bce659f6326e247049 (patch) | |
| tree | 3d67cde5d6e90a786f0b1a294be2be3e1f90cba6 /compiler | |
| parent | e108481f74ff123ad98a63bd107a18d13035b275 (diff) | |
| download | rust-01307cf03f5b95759a8e84bce659f6326e247049.tar.gz rust-01307cf03f5b95759a8e84bce659f6326e247049.zip | |
Implement `default_overrides_default_fields` lint
Detect when a manual `Default` implementation isn't using the existing default field values and suggest using `..` instead:
```
error: `Default` impl doesn't use the declared default field values
--> $DIR/manual-default-impl-could-be-derived.rs:14:1
|
LL | / impl Default for A {
LL | | fn default() -> Self {
LL | | A {
LL | | y: 0,
| | - this field has a default value
... |
LL | | }
| |_^
|
= help: use the default values in the `impl` with `Struct { mandatory_field, .. }` to avoid them diverging over time
note: the lint level is defined here
--> $DIR/manual-default-impl-could-be-derived.rs:5:9
|
LL | #![deny(default_overrides_default_fields)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```
Diffstat (limited to 'compiler')
| -rw-r--r-- | compiler/rustc_lint/src/default_could_be_derived.rs | 185 | ||||
| -rw-r--r-- | compiler/rustc_lint/src/lib.rs | 3 |
2 files changed, 188 insertions, 0 deletions
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 00000000000..d95cbb05158 --- /dev/null +++ b/compiler/rustc_lint/src/default_could_be_derived.rs @@ -0,0 +1,185 @@ +use rustc_data_structures::fx::FxHashMap; +use rustc_errors::Diag; +use rustc_hir as hir; +use rustc_middle::ty; +use rustc_session::{declare_lint, impl_lint_pass}; +use rustc_span::Symbol; +use rustc_span::symbol::sym; + +use crate::{LateContext, LateLintPass}; + +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 `<Type as Default>::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, + Deny, + "detect `Default` impl that should use the type's default field values", + @feature_gate = default_field_values; +} + +#[derive(Default)] +pub(crate) struct DefaultCouldBeDerived; + +impl_lint_pass!(DefaultCouldBeDerived => [DEFAULT_OVERRIDES_DEFAULT_FIELDS]); + +impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { + fn check_impl_item(&mut self, cx: &LateContext<'_>, impl_item: &hir::ImplItem<'_>) { + // Look for manual implementations of `Default`. + let Some(default_def_id) = cx.tcx.get_diagnostic_item(sym::Default) else { return }; + let hir::ImplItemKind::Fn(_sig, body_id) = impl_item.kind 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) { + // We don't care about what `#[derive(Default)]` produces in this lint. + 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 now know we have a manually written definition of a `<Type as Default>::default()`. + + let hir = cx.tcx.hir(); + + let type_def_id = def.did(); + let body = hir.body(body_id); + + // 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; + }; + + // 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::<FxHashMap<_, _>>(), + _ => return, + }; + + // We check `fn default()` body is a single ADT literal and get all the fields that are + // being set. + 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 would 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 + // `..`. + + 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; + } + + // 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 { + // None of the default fields were actually provided explicitly, so the manual impl + // doesn't override them (the user used `..`), so there's no risk of divergent behavior. + 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(DEFAULT_OVERRIDES_DEFAULT_FIELDS, hir_id, item.span, |diag| { + mk_lint(diag, orig_fields, fields); + }); + } +} + +fn mk_lint( + diag: &mut Diag<'_, ()>, + orig_fields: FxHashMap<Symbol, &hir::FieldDef<'_>>, + fields: &[hir::ExprField<'_>], +) { + diag.primary_message("`Default` impl doesn't use the declared default field values"); + + // For each field in the struct expression + // - if the field in the type has a default value, it should be removed + // - elif the field is an expression that could be a default value, it should be used as the + // field's default value (FIXME: not done). + // - else, we wouldn't touch this field, it would remain in the manual impl + let mut removed_all_fields = true; + for field in fields { + 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"); + } else { + removed_all_fields = false; + } + } + + diag.help(if removed_all_fields { + "to avoid divergence in behavior between `Struct { .. }` and \ + `<Struct as Default>::default()`, derive the `Default`" + } else { + "use the default values in the `impl` with `Struct { mandatory_field, .. }` to avoid them \ + diverging over time" + }); +} diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index d7f0d2a6941..1465c2cff7b 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, |
