From 7b26197532bfcfc8317610d099d415e886e1b89b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 22 Jan 2025 01:27:26 +0000 Subject: [PATCH 1/2] Move `default_field_values` tests to one subdirectory --- .../default-field-values-non_exhaustive.rs | 0 .../default-field-values-non_exhaustive.stderr | 0 .../manual-default-impl-could-be-derived.rs | 0 .../manual-default-impl-could-be-derived.stderr | 0 4 files changed, 0 insertions(+), 0 deletions(-) rename tests/ui/structs/{ => default-field-values}/default-field-values-non_exhaustive.rs (100%) rename tests/ui/structs/{ => default-field-values}/default-field-values-non_exhaustive.stderr (100%) rename tests/ui/structs/{ => default-field-values}/manual-default-impl-could-be-derived.rs (100%) rename tests/ui/structs/{ => default-field-values}/manual-default-impl-could-be-derived.stderr (100%) diff --git a/tests/ui/structs/default-field-values-non_exhaustive.rs b/tests/ui/structs/default-field-values/default-field-values-non_exhaustive.rs similarity index 100% rename from tests/ui/structs/default-field-values-non_exhaustive.rs rename to tests/ui/structs/default-field-values/default-field-values-non_exhaustive.rs diff --git a/tests/ui/structs/default-field-values-non_exhaustive.stderr b/tests/ui/structs/default-field-values/default-field-values-non_exhaustive.stderr similarity index 100% rename from tests/ui/structs/default-field-values-non_exhaustive.stderr rename to tests/ui/structs/default-field-values/default-field-values-non_exhaustive.stderr diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.rs b/tests/ui/structs/default-field-values/manual-default-impl-could-be-derived.rs similarity index 100% rename from tests/ui/structs/manual-default-impl-could-be-derived.rs rename to tests/ui/structs/default-field-values/manual-default-impl-could-be-derived.rs diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.stderr b/tests/ui/structs/default-field-values/manual-default-impl-could-be-derived.stderr similarity index 100% rename from tests/ui/structs/manual-default-impl-could-be-derived.stderr rename to tests/ui/structs/default-field-values/manual-default-impl-could-be-derived.stderr From 9f14773cece96039f245ead4e18757cf14a3b2d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 22 Jan 2025 04:12:22 +0000 Subject: [PATCH 2/2] Detect when a field default is not using that field's type's default values Given ```rust pub struct Foo { pub something: i32 = 2, } pub struct Bar { pub foo: Foo = Foo { something: 10 }, } ``` The value for `Bar { .. }.foo.something` is different to the value in `Foo { .. }.something`. This can cause confusion so we lint against it. We don't check that the values actually differ. At just the mere presence of the default value we suggest `..` to avoid the possibility of divergence. This is just a lint to allow for APIs where that divergence is intentional. --- .../src/default_could_be_derived.rs | 131 +++++++++++++++++- compiler/rustc_lint/src/lib.rs | 5 +- ...diverging-default-field-values-in-field.rs | 38 +++++ ...rging-default-field-values-in-field.stderr | 35 +++++ 4 files changed, 204 insertions(+), 5 deletions(-) create mode 100644 tests/ui/structs/default-field-values/diverging-default-field-values-in-field.rs create mode 100644 tests/ui/structs/default-field-values/diverging-default-field-values-in-field.stderr diff --git a/compiler/rustc_lint/src/default_could_be_derived.rs b/compiler/rustc_lint/src/default_could_be_derived.rs index bae9defa68700..e41af4449d8e8 100644 --- a/compiler/rustc_lint/src/default_could_be_derived.rs +++ b/compiler/rustc_lint/src/default_could_be_derived.rs @@ -1,5 +1,7 @@ -use rustc_data_structures::fx::FxHashMap; -use rustc_errors::{Applicability, Diag}; +use hir::def::{CtorOf, DefKind, Res}; +use rustc_ast::Recovered; +use rustc_data_structures::fx::{FxHashMap, FxHashSet}; +use rustc_errors::{Applicability, Diag, MultiSpan}; use rustc_hir as hir; use rustc_middle::ty; use rustc_middle::ty::TyCtxt; @@ -50,7 +52,6 @@ declare_lint! { @feature_gate = default_field_values; } -#[derive(Default)] pub(crate) struct DefaultCouldBeDerived; impl_lint_pass!(DefaultCouldBeDerived => [DEFAULT_OVERRIDES_DEFAULT_FIELDS]); @@ -201,3 +202,127 @@ fn mk_lint( diag.help(msg); } } + +declare_lint! { + /// The `default_field_overrides_default_field` lint checks for struct literals in field default + /// values with fields that have in turn default values. These should instead be skipped and + /// rely on `..` for them. + /// + /// ### Example + /// + /// ```rust,compile_fail + /// #![feature(default_field_values)] + /// #![deny(default_field_overrides_default_field)] + /// + /// struct Foo { + /// x: Bar = Bar { x: 0 }, // `Foo { .. }.x.x` != `Bar { .. }.x` + /// } + /// + /// struct Bar { + /// x: i32 = 101, + /// } + /// ``` + /// + /// {{produces}} + /// + /// ### Explanation + /// + /// Defaulting a field to a value different to that field's type already defined default can + /// easily lead to confusion due to diverging behavior. Acknowleding that there can be reasons + /// for one to write an API that does this, this is not outright rejected by the compiler, + /// merely linted against. + pub DEFAULT_FIELD_OVERRIDES_DEFAULT_FIELD, + Deny, + "detect default field value that should use the type's default field values", + @feature_gate = default_field_values; +} + +pub(crate) struct DefaultFieldOverride; + +impl_lint_pass!(DefaultFieldOverride => [DEFAULT_FIELD_OVERRIDES_DEFAULT_FIELD]); + +impl DefaultFieldOverride { + fn lint_variant(&mut self, cx: &LateContext<'_>, data: &hir::VariantData<'_>) { + if !cx.tcx.features().default_field_values() { + return; + } + let hir::VariantData::Struct { fields, recovered: Recovered::No } = data else { + return; + }; + + for default in fields.iter().filter_map(|f| f.default) { + let body = cx.tcx.hir().body(default.body); + let hir::ExprKind::Struct(hir::QPath::Resolved(_, path), fields, _) = body.value.kind + else { + continue; + }; + let Res::Def( + DefKind::Variant + | DefKind::Struct + | DefKind::Ctor(CtorOf::Variant | CtorOf::Struct, ..), + def_id, + ) = path.res + else { + continue; + }; + let fields_set: FxHashSet<_> = fields.iter().map(|f| f.ident.name).collect(); + let variant = cx.tcx.expect_variant_res(path.res); + let mut to_lint = vec![]; + let mut defs = vec![]; + + for field in &variant.fields { + if fields_set.contains(&field.name) { + for f in fields { + if f.ident.name == field.name + && let Some(default) = field.value + { + to_lint.push((f.expr.span, f.ident.name)); + defs.push(cx.tcx.def_span(default)); + } + } + } + } + + if to_lint.is_empty() { + continue; + } + cx.tcx.node_span_lint( + DEFAULT_FIELD_OVERRIDES_DEFAULT_FIELD, + body.value.hir_id, + to_lint.iter().map(|&(span, _)| span).collect::>(), + |diag| { + diag.primary_message("default field overrides that field's type's default"); + diag.span_label(path.span, "when constructing this value"); + let type_name = cx.tcx.item_name(def_id); + for (span, name) in to_lint { + diag.span_label( + span, + format!( + "this overrides the default of field `{name}` in `{type_name}`" + ), + ); + } + let mut overriden_spans: MultiSpan = defs.clone().into(); + overriden_spans.push_span_label(cx.tcx.def_span(def_id), ""); + diag.span_note( + overriden_spans, + format!( + "{this} field's default value in `{type_name}` is overriden", + this = if defs.len() == 1 { "this" } else { "these" } + ), + ); + }, + ); + } + } +} + +impl<'tcx> LateLintPass<'tcx> for DefaultFieldOverride { + fn check_item(&mut self, cx: &LateContext<'_>, item: &hir::Item<'_>) { + let hir::ItemKind::Struct(data, _) = item.kind else { return }; + self.lint_variant(cx, &data); + } + fn check_variant(&mut self, cx: &LateContext<'_>, variant: &hir::Variant<'_>) { + self.lint_variant(cx, &variant.data); + } +} diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index 1465c2cff7bc1..f7241a1fa562b 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -86,7 +86,7 @@ use async_closures::AsyncClosureUsage; use async_fn_in_trait::AsyncFnInTrait; use builtin::*; use dangling::*; -use default_could_be_derived::DefaultCouldBeDerived; +use default_could_be_derived::{DefaultCouldBeDerived, DefaultFieldOverride}; use deref_into_dyn_supertrait::*; use drop_forget_useless::*; use enum_intrinsics_non_enums::EnumIntrinsicsNonEnums; @@ -191,7 +191,8 @@ late_lint_methods!( BuiltinCombinedModuleLateLintPass, [ ForLoopsOverFallibles: ForLoopsOverFallibles, - DefaultCouldBeDerived: DefaultCouldBeDerived::default(), + DefaultCouldBeDerived: DefaultCouldBeDerived, + DefaultFieldOverride: DefaultFieldOverride, DerefIntoDynSupertrait: DerefIntoDynSupertrait, DropForgetUseless: DropForgetUseless, ImproperCTypesDeclarations: ImproperCTypesDeclarations, diff --git a/tests/ui/structs/default-field-values/diverging-default-field-values-in-field.rs b/tests/ui/structs/default-field-values/diverging-default-field-values-in-field.rs new file mode 100644 index 0000000000000..b886018c0a65b --- /dev/null +++ b/tests/ui/structs/default-field-values/diverging-default-field-values-in-field.rs @@ -0,0 +1,38 @@ +#![feature(default_field_values)] +#![allow(dead_code)] + +mod a { + pub struct Foo { + pub something: i32 = 2, + } + + pub struct Bar { + pub foo: Foo = Foo { something: 10 }, //~ ERROR default field overrides that field's type's default + } +} + +mod b { + pub enum Foo { + X { + something: i32 = 2, + } + } + + pub enum Bar { + Y { + foo: Foo = Foo::X { something: 10 }, //~ ERROR default field overrides that field's type's default + } + } +} + +fn main() { + let x = a::Bar { .. }; + let y = a::Foo { .. }; + assert_eq!(x.foo.something, y.something); + let x = b::Bar::Y { .. }; + let y = b::Foo::X { .. }; + match (x, y) { + (b::Bar::Y { foo: b::Foo::X { something: a} }, b::Foo::X { something:b }) if a == b=> {} + _ => panic!(), + } +} diff --git a/tests/ui/structs/default-field-values/diverging-default-field-values-in-field.stderr b/tests/ui/structs/default-field-values/diverging-default-field-values-in-field.stderr new file mode 100644 index 0000000000000..b642ee5bb664c --- /dev/null +++ b/tests/ui/structs/default-field-values/diverging-default-field-values-in-field.stderr @@ -0,0 +1,35 @@ +error: default field overrides that field's type's default + --> $DIR/diverging-default-field-values-in-field.rs:10:41 + | +LL | pub foo: Foo = Foo { something: 10 }, + | --- ^^ this overrides the default of field `something` in `Foo` + | | + | when constructing this value + | +note: this field's default value in `Foo` is overriden + --> $DIR/diverging-default-field-values-in-field.rs:6:30 + | +LL | pub struct Foo { + | -------------- +LL | pub something: i32 = 2, + | ^ + = note: `#[deny(default_field_overrides_default_field)]` on by default + +error: default field overrides that field's type's default + --> $DIR/diverging-default-field-values-in-field.rs:23:44 + | +LL | foo: Foo = Foo::X { something: 10 }, + | ------ ^^ this overrides the default of field `something` in `X` + | | + | when constructing this value + | +note: this field's default value in `X` is overriden + --> $DIR/diverging-default-field-values-in-field.rs:17:30 + | +LL | X { + | - +LL | something: i32 = 2, + | ^ + +error: aborting due to 2 previous errors +