Introduce `default_field_values` feature #129514

estebank · 2024-08-24T17:25:50Z

Initial implementation of #[feature(default_field_values], proposed in rust-lang/rfcs#3681.

We now parse const expressions after a = in a field definition, to specify a struct field default value.

We now allow Struct { field, .. } where there's no base after ...

#[derive(Default)] now uses the default value if present, continuing to use Default::default() if not.

#[derive(Debug)]
pub struct S;

#[derive(Debug, Default)]
pub struct Foo {
    pub bar: S = S,
    pub baz: i32 = 42 + 3,
}

fn main () {
    let x = Foo { .. };
    let y = Foo::default();
    let z = Foo { baz: 1, .. };

    assert_eq!(45, x.baz);
    assert_eq!(45, y.baz);
    assert_eq!(1, z.baz);
}

rustbot · 2024-08-24T17:25:58Z

r? @cjgillot

rustbot has assigned @cjgillot.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

compiler/rustc_mir_build/src/build/expr/into.rs

estebank · 2024-08-24T19:07:09Z

It just occurred to me to try how the following fails, and it Just Works™️ 😄:

pub struct Foo {
    pub baz: i32 = Self::X,
}

impl Foo {
    const X: i32 = 1;
}

cjgillot

I made a first pass. I don't have anything substantial to say, that's a great pr.

Just an avalanche of nits. An additional one: could you prefer matching on Rest now there are >2 cases?

compiler/rustc_ast/src/ast.rs

compiler/rustc_builtin_macros/src/deriving/default.rs

compiler/rustc_hir/src/intravisit.rs

compiler/rustc_hir/src/hir.rs

compiler/rustc_hir/src/intravisit.rs

compiler/rustc_middle/src/ty/mod.rs

compiler/rustc_mir_build/src/build/custom/parse/instruction.rs

compiler/rustc_mir_build/src/build/expr/into.rs

tests/ui/structs/default-field-values-support.rs

tests/ui/structs/default-field-values-failures.stderr

estebank · 2024-08-25T19:01:18Z

compiler/rustc_builtin_macros/src/deriving/default.rs

+                                    // We use `Default::default()`.
+                                    None => default_call(cx, field.span),


Given this, I filed rust-lang/rfcs#3683 as this bit is independent of the default values RFC.

compiler-errors · 2024-12-09T18:30:20Z

compiler/rustc_hir_analysis/src/check/wfcheck.rs

@@ -1104,6 +1104,23 @@ fn check_type_defn<'tcx>(
        for variant in variants.iter() {
            // All field types must be well-formed.
            for field in &variant.fields {
+                if let Some(def_id) = field.value


For the record, erroring eagerly even if we don't instantiate the default is consistent with how defaults in struct const generics works:

struct Foo<const N: u8 = {130 + 130}> {}

which errors with:

error[E0080]: evaluation of constant value failed --> src/lib.rs:1:27 | 1 | struct Foo<const N: u8 = {130 + 130}> {} | ^^^^^^^^^ attempt to compute `130_u8 + 130_u8`, which would overflow For more information about this error, try `rustc --explain E0080`. error: could not compile `playground` (lib) due to 1 previous error

Not totally certain why the RFC specified that it would only be linted against, but this seems to me to be the correct behavior.

compiler/rustc_hir_analysis/src/check/wfcheck.rs

compiler-errors · 2024-12-09T18:33:00Z

Please address nits and then please clean up the history a bit (that zip_eq commit could be squashed into whatever commit, and bbdd6ba too -- have you ever heard of git absorb? that might be useful to automate this type of cleanup).

Then r=me

Initial implementation of `#[feature(default_field_values]`, proposed in rust-lang/rfcs#3681. Support default fields in enum struct variant Allow default values in an enum struct variant definition: ```rust pub enum Bar { Foo { bar: S = S, baz: i32 = 42 + 3, } } ``` Allow using `..` without a base on an enum struct variant ```rust Bar::Foo { .. } ``` `#[derive(Default)]` doesn't account for these as it is still gating `#[default]` only being allowed on unit variants. Support `#[derive(Default)]` on enum struct variants with all defaulted fields ```rust pub enum Bar { #[default] Foo { bar: S = S, baz: i32 = 42 + 3, } } ``` Check for missing fields in typeck instead of mir_build. Expand test with `const` param case (needs `generic_const_exprs` enabled). Properly instantiate MIR const The following works: ```rust struct S<A> { a: Vec<A> = Vec::new(), } S::<i32> { .. } ``` Add lint for default fields that will always fail const-eval We *allow* this to happen for API writers that might want to rely on users' getting a compile error when using the default field, different to the error that they would get when the field isn't default. We could change this to *always* error instead of being a lint, if we wanted. This will *not* catch errors for partially evaluated consts, like when the expression relies on a const parameter. Suggestions when encountering `Foo { .. }` without `#[feature(default_field_values)]`: - Suggest adding a base expression if there are missing fields. - Suggest enabling the feature if all the missing fields have optional values. - Suggest removing `..` if there are no missing fields.

Emit a specific error for unsupported default field value syntax in tuple structs.

People might extrapolate from `Struct { .. }` that `Struct(..)` would work, but it doesn't.

…errors Emit E0080 always on struct definition with default fields that have unconditional const errors and remove `default_field_always_invalid_const` lint.

compiler-errors · 2024-12-09T22:15:08Z

@bors r+ rollup=never

bors · 2024-12-09T22:15:11Z

📌 Commit fa331f4 has been approved by compiler-errors

It is now in the queue for this repository.

fmease · 2024-12-09T22:35:33Z

@bors p=10 bitrotty

bors · 2024-12-10T00:07:03Z

⌛ Testing commit fa331f4 with merge 974ccc1...

bors · 2024-12-10T02:55:22Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 974ccc1 to master...

rust-timer · 2024-12-10T04:27:43Z

Finished benchmarking commit (974ccc1): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.7%	[0.3%, 2.3%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.7%	[-0.7%, -0.7%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -0.0%, secondary 2.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.3%	[1.1%, 1.5%]	2
Regressions ❌ (secondary)	2.4%	[2.4%, 2.4%]	1
Improvements ✅ (primary)	-1.4%	[-2.2%, -0.7%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-2.2%, 1.5%]	4

Cycles

Results (secondary 2.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.1%, secondary 0.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.2%]	53
Regressions ❌ (secondary)	0.0%	[0.0%, 0.1%]	13
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.0%, 0.2%]	53

Bootstrap: 767.262s -> 767.421s (0.02%)
Artifact size: 330.86 MiB -> 331.00 MiB (0.04%)

ytmimi · 2024-12-22T21:54:58Z

@estebank @compiler-errors I'm not sure how, but this change broken rustfmt macro formatting. rust-lang/rustfmt#6424 was recently reported and I bisected the change in behavior back to this PR.

compiler-errors · 2024-12-22T21:57:33Z

I'll look into it. Probably has to do with changes to the compiler representation of that default value.

…ue-fmt, r=ytmimi Make sure we don't lose default struct value when formatting struct The reason why rust-lang/rustfmt#6424 broke when rust-lang#129514 landed is because the parser now *successfully* parses default struct values. That means that where we used to fail in `rewrite_macro_inner`: https://github.com/rust-lang/rust/blob/e108481f74ff123ad98a63bd107a18d13035b275/src/tools/rustfmt/src/macros.rs#L263-L267 ... we now succeed, and we now proceed to format the inner struct as a macro arg. Since formatting was never implemented for default struct values, this means that we’ll always rewrite the struct to exclude the default value. This PR makes it so that we simply don’t rewrite struct fields if they have a default value. r? `@ytmimi`

Rollup merge of rust-lang#134668 - compiler-errors:default-struct-value-fmt, r=ytmimi Make sure we don't lose default struct value when formatting struct The reason why rust-lang/rustfmt#6424 broke when rust-lang#129514 landed is because the parser now *successfully* parses default struct values. That means that where we used to fail in `rewrite_macro_inner`: https://github.com/rust-lang/rust/blob/e108481f74ff123ad98a63bd107a18d13035b275/src/tools/rustfmt/src/macros.rs#L263-L267 ... we now succeed, and we now proceed to format the inner struct as a macro arg. Since formatting was never implemented for default struct values, this means that we’ll always rewrite the struct to exclude the default value. This PR makes it so that we simply don’t rewrite struct fields if they have a default value. r? `@ytmimi`

rustbot assigned cjgillot Aug 24, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Aug 24, 2024

estebank commented Aug 24, 2024

View reviewed changes

compiler/rustc_mir_build/src/build/expr/into.rs Outdated Show resolved Hide resolved