missing-field-initializers warning not reporting missing designated initializers

[ottojo]

In the following code, the missing-field-initializers does not produce a warning that b is not explicitly initialized.

Changing CheckForMissingFields to True here: https://github.com/llvm/llvm-project/blob/a9a60f20e6cc80855864b8f559073bc31f34554b/clang/lib/Sema/SemaInit.cpp#L2167-L2169 seems to produce the expected warning.

The comment suggests

// Disable check for missing fields when designators are used.
// This matches gcc behaviour.

However, GCC does produce the warning in that case. Comparison with GCC: https://godbolt.org/z/WeYdY11q3

struct ExampleStruct {
    int a;
    int b;
};

ExampleStruct buildExampleStruct(int a) {
    ExampleStruct e{
            .a=a
    };
    return e;
}

Build flags: -Werror=missing-field-initializers -std=c++20

Expected behavior:

main.cpp:10:5: error: missing field 'b' initializer [-Werror,-Wmissing-field-initializers]

matching GCCs

<source>:9:5: error: missing initializer for member 'ExampleStruct::b' [-Werror=missing-field-initializers]

Actual behavior: Compiles without warning/error.

[dbelousoff]

Is there any updates/workarounds for this?

[shafik]

I don't see a workaround, it looks out of date at this point. Someone needs to remove the CheckForMissingFields = false; but this is a potentially breaking change since there may be folks that using this flag that won't expect the warning to start popping up. Although it should.

[ottojo]

I see how this would be a breaking change, although i guess if someone enabled Wextra and ~~Wall~~ Werror they would expect some breakage when updating the compiler...

Anyways, i felt like hacking at this a bit and maybe introducing a dedicated switch for this might be an idea? I think the warning is useful, and i would certainly enable it in my projects (although discovery of this flag might be a problem...) An attempt of an implementation is here, which seems to work in the above test at least :smile: https://github.com/ottojo/llvm-project/pull/2/files

[gracicot]

I'm actually hit by this issue and I actually missed a bug in my code because I assumed the warning was working on Clang too.

[shafik]

@AaronBallman sounds like missing this case is not ideal, wdyt about making this change?

[AaronBallman]

@AaronBallman sounds like missing this case is not ideal, wdyt about making this change?

I'm comfortable with the change. If it turns out to be highly disruptive, we can always put the diagnostic under a new warning flag grouped under -Wmissing-field-initializers so that users can selectively disable this (but hopefully we won't need such a measure).

[Fznamznon]

https://reviews.llvm.org/D157879

[jamesruan]

It is how we understand designated initializers that matters. IMHO it means we want some fields to be initialized as designated while other fields default initialized. It explicitly leave the 'not designated fields' to be default initialized. Somissing-field-initializers should not report it as warning.

Our project builds failed with this change. We want the diagnostic flag to report for fields we are really forget to init instead of reporting designated initializers that DO means other fields to be default initialized.

The GCC community is still discussing this issue and has not drawn a conclusion yet ( for C++ ).

[shafik]

So we are hitting dcl.init.aggr p5.2:

Otherwise, if the element is not a reference, the element is copy-initialized from an empty initializer list ([dcl.init.list]).

and if we try another example w/o designated init: https://godbolt.org/z/Yn48KWvrM

ExampleStruct buildExampleStruct(int a) {
    ExampleStruct e{1};
    return e;
}

We obtain the same diagnostic, so we are being entirely consistent here. I don't see a convincing argument that we should not warn for designated init but we should warn for non-designated aggregate init. The standard does not differentiate these cases.

[AaronBallman]

IMHO it means we want some fields to be initialized as designated while other fields default initialized. It explicitly leave the 'not designated fields' to be default initialized. Somissing-field-initializers should not report it as warning.

I think that any field initializer not explicitly mentioned is missing (per the usual English sense of the word) and to get your reading of the diagnostic, we'd want it named differently.

Our project builds failed with this change. We want the diagnostic flag to report for fields we are really forget to init instead of reporting designated initializers that DO means other fields to be default initialized.

That said, this could be a reason to add a new group under -Wmissing-field-initializers for this functionality. I'm curious what the GCC folks think on the topic.

[gracicot]

@jamesruan Also note that with this warning, it's really easy for an implementer to specify when that warning happen.

struct AB {
    int a = {}; // A is an optional struct parameter
    int b;
};

AB test{
    .b = 0 // no warning for a since it has a default value
};

That way this warning only happen when users really make a mistake.

[floooh]

It looks to me like the missing-field-initializers warning for designated initialization in Clang 18.0.0 is also triggered in C, which definitely looks like a bug (since missing field initializers for designated init in C are a well-defined feature, such items are explicitly initialized to zero, which can be detected and then patched to default values in libraries which are designed with designated init in mind - FWIW, this change breaks all my C code when compiling with -Wall -Wextra -Werror).

The Clang 18 release notes say this:

• Clang now reports missing-field-initializers warning for missing designated initializers in C++. (#56628)

...but it also happens in C mode.

I'm seeing this issue in the latest Emscripten SDK (3.1.51) which is using Clang 18.0.0.

See code like this to understand why the warning is a serious issue:

https://github.com/floooh/sokol-samples/blob/b3bc55c4411fa03770e2ff4303e1d49f9f36616d/sapp/triangle-sapp.c#L42-L52

The sg_pipeline_desc struct has dozens of members which are checked for zero and patched to sane defaults inside the sg_make_pipeline() call. Having to provide dozens of explicit = 0 statements in the struct initialization would kill all readability, because the important non-default items would disappear in the noise.

[shafik]

Note there are several discussions in the review: http://108.170.204.19/D157879

notably: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96868

CC @Fznamznon @AaronBallman

[Fznamznon]

@floooh , the issue you're seeing can be a side effect of https://github.com/llvm/llvm-project/pull/70829 which I reverted before holidays season since there was no intention in enabling the warnings in C. Please check with clang built with trunk.