[clang] Report missing designated initializers in C++

[Fznamznon]

Summary

Prior to this change clang didn't emit missing-field-initializers
warning for designated initializers. The comments say that it is done to
match gcc behavior. However, gcc behaves so only for C. For C++ warnings
are emitted.

Fixes https://github.com/llvm/llvm-project/issues/56628

[aaron.ballman]

Thank you for working on this! The changes should come with a release note.

I think the diagnostic is triggered a bit to aggressively in the case where there is an invalid initializer for a field -- we'll claim there is no initializer in that case, but that effectively duplicates the diagnostics without adding anything actionable for the user to do (once they fix the initialization, both diagnostics go away). Can we silence the diagnostic in these cases?

clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp

64

This second warning seems wrong, there is an initializer for a, it's just not a valid one.

72–79

This warning is quite wrong, there's a whole pile of initializers for x. :-)

228

I also think this one is wrong.

[Fznamznon]

Add a release note, apply feedback:

• Do not report invalid initializers as missing
• Fix wrong warning if record has bitfields

[Fznamznon]

Thank you for working on this! The changes should come with a release note.

Thanks for feedback, I added a release note.

Can we silence the diagnostic in these cases?

And this is done.

[tbaeder]

Tangentially related: Now that we have this InitializedFields set, would it be easy to add a warning for double-initialization of fields (that would also trigger in C)?

[Fznamznon]

In D157879#4594790, @tbaeder wrote:

Tangentially related: Now that we have this InitializedFields set, would it be easy to add a warning for double-initialization of fields (that would also trigger in C)?

Isn't a warning like this already in place - https://godbolt.org/z/E1dGsY3ze ? Or you meant something else?

[tbaeder]

In D157879#4595058, @Fznamznon wrote:

In D157879#4594790, @tbaeder wrote:

Tangentially related: Now that we have this InitializedFields set, would it be easy to add a warning for double-initialization of fields (that would also trigger in C)?

Isn't a warning like this already in place - https://godbolt.org/z/E1dGsY3ze ? Or you meant something else?

Yes, thanks. Someone else was asking for this feature a while back but looks like they didn't notice it already exists.

[Fznamznon]

Rebase

[aaron.ballman]

LGTM!

[Fznamznon]

Rebase to maybe pass precommit

[Fznamznon]

The failure is unrelated, since the patch doesn't touch modules and I've seen the test failing for other patches as well.

[aeubanks]

is there any way to suppress this for a specific field? (I believe) I'm seeing user code where a field is intentionally not being initialized

[aaron.ballman]

In D157879#4604046, @aeubanks wrote:

is there any way to suppress this for a specific field? (I believe) I'm seeing user code where a field is intentionally not being initialized

Hmmm, so they're wanting to rely on the implicit initialization (http://eel.is/c++draft/dcl.init#aggr-5)? It seems a bit odd to pass -Wmissing-field-initializers and then rely on implicit initialization of fields, so some more details might be helpful.

[aeubanks]

ah I thought this was in -Wall but it's not

the struct is

struct Foo {
  const void* buffer;
  uint32_t capacity;
  uint32_t reserved;
};

where reserved isn't explicitly initialized. that seems like reasonable code, but I suppose we can just explicitly initialize reserved here

[aaron.ballman]

In D157879#4604233, @aeubanks wrote:

ah I thought this was in -Wall but it's not

the struct is

struct Foo {
  const void* buffer;
  uint32_t capacity;
  uint32_t reserved;
};

where reserved isn't explicitly initialized. that seems like reasonable code, but I suppose we can just explicitly initialize reserved here

Thank you for sharing the use case, that's actually a rather interesting one! Leaving the identifier reserved off the field would trigger a -Wmissing-declarations diagnostic (which is on by default). You can leave the field named and use an in-class initializer for it and that should silence the diagnostic. But I kind of wonder if we want to special case fields named reserved or similar (wReserved, reserved2, etc)?

[phosek]

Note that there's an ongoing discussion on the GCC bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96868 whether -Wmissing-field-initializers should apply to both C and C++ or just C.

[ianloic]

In D157879#4604233, @aeubanks wrote:

ah I thought this was in -Wall but it's not

the struct is

struct Foo {
  const void* buffer;
  uint32_t capacity;
  uint32_t reserved;
};

where reserved isn't explicitly initialized. that seems like reasonable code, but I suppose we can just explicitly initialize reserved here

FWIW, if this is the specific piece of code that led me to this conversation, that declaration is in a header which at least in theory is supposed to remain compatible with C. It's inside extern "C" { ... } even. We can't add explicit initialize reserved and keep the definition C comaptible.

[aaron.ballman]

In D157879#4604288, @phosek wrote:

Note that there's an ongoing discussion on the GCC bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96868 whether -Wmissing-field-initializers should apply to both C and C++ or just C.

Thank you for posting this! I'm with @jwakely on this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96868#c3 -- the warning is for people who want to be told they've not explicitly specified all of the initializers, it is not for telling you "this hasn't been initialized". I think the desire makes as much sense in C++ as it does in C, but this is more of a style warning than a correctness warning.

In D157879#4604471, @ianloic wrote:

In D157879#4604233, @aeubanks wrote:

ah I thought this was in -Wall but it's not

the struct is

struct Foo {
  const void* buffer;
  uint32_t capacity;
  uint32_t reserved;
};

where reserved isn't explicitly initialized. that seems like reasonable code, but I suppose we can just explicitly initialize reserved here

FWIW, if this is the specific piece of code that led me to this conversation, that declaration is in a header which at least in theory is supposed to remain compatible with C. It's inside extern "C" { ... } even. We can't add explicit initialize reserved and keep the definition C comaptible.

Ouch, yeah, that does make it a challenge. But why is -Wmissing-field-initializers enabled in the first place for the code? It seems like there are fields you *don't* want explicit initialization for, so perhaps the answer is to disable the diagnostic for those types?

[aeubanks]

In D157879#4606531, @aaron.ballman wrote:

In D157879#4604288, @phosek wrote:

Note that there's an ongoing discussion on the GCC bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96868 whether -Wmissing-field-initializers should apply to both C and C++ or just C.

Thank you for posting this! I'm with @jwakely on this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96868#c3 -- the warning is for people who want to be told they've not explicitly specified all of the initializers, it is not for telling you "this hasn't been initialized". I think the desire makes as much sense in C++ as it does in C, but this is more of a style warning than a correctness warning.

In D157879#4604471, @ianloic wrote:

In D157879#4604233, @aeubanks wrote:

ah I thought this was in -Wall but it's not

the struct is

struct Foo {
  const void* buffer;
  uint32_t capacity;
  uint32_t reserved;
};

where reserved isn't explicitly initialized. that seems like reasonable code, but I suppose we can just explicitly initialize reserved here

FWIW, if this is the specific piece of code that led me to this conversation, that declaration is in a header which at least in theory is supposed to remain compatible with C. It's inside extern "C" { ... } even. We can't add explicit initialize reserved and keep the definition C comaptible.

Ouch, yeah, that does make it a challenge. But why is -Wmissing-field-initializers enabled in the first place for the code? It seems like there are fields you *don't* want explicit initialization for, so perhaps the answer is to disable the diagnostic for those types?

It's an unfortunate configuration. We saw this in Chrome where some projects were manually setting -Wmissing-field-initializers, but when building for Fuchsia, some Fuchsia system headers had this code. We can just suppress this for all Fuchsia builds, I think that's fine.

[rsmith]

This change has introduced a false positive for anonymous union members:

struct A {
    int m;
    union { int n = 0; };
};

A a = A{.m = 0};

now produces a false positive warning saying that the anonymous union member in A is uninitialized.

[Fznamznon]

In D157879#4655321, @rsmith wrote:

This change has introduced a false positive for anonymous union members:

struct A {
    int m;
    union { int n = 0; };
};

A a = A{.m = 0};

now produces a false positive warning saying that the anonymous union member in A is uninitialized.

Thanks! I've created https://github.com/llvm/llvm-project/issues/70444 and will figure something to fix this asap.

clang/docs/ReleaseNotes.rst

- Fixed an issue where a class template specialization whose declaration is
   instantiated in one module and whose definition is instantiated in another
   module may end up with members associated with the wrong declaration of the
   class, which can result in miscompiles in some cases.
 - Fix crash on use of a variadic overloaded operator.
   (`#42535 <https://github.com/llvm/llvm-project/issues/42535>_`)
 - Fix a hang on valid C code passing a function type as an argument to
   ``typeof`` to form a function declaration.
   (`#64713 <https://github.com/llvm/llvm-project/issues/64713>_`)
+- Clang now reports missing-field-initializers warning for missing designated
+  initializers in C++.
+  (`#56628 <https://github.com/llvm/llvm-project/issues/56628>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to C++ Support

clang/lib/Sema/SemaInit.cpp

void InitListChecker::CheckStructUnionTypes(
   RecordDecl::field_iterator FieldEnd = RD->field_end();
   size_t NumRecordDecls = llvm::count_if(RD->decls(), [&](const Decl *D) {
     return isa<FieldDecl>(D) || isa<RecordDecl>(D);
   });
   bool CheckForMissingFields =
     !IList->isIdiomaticZeroInitializer(SemaRef.getLangOpts());
   bool HasDesignatedInit = false;
 
+  llvm::SmallPtrSet<FieldDecl *, 4> InitializedFields;
+
   while (Index < IList->getNumInits()) {
     Expr *Init = IList->getInit(Index);
     SourceLocation InitLoc = Init->getBeginLoc();
 
     if (DesignatedInitExpr *DIE = dyn_cast<DesignatedInitExpr>(Init)) {
       // If we're not the subobject that matches up with the '{' for
       // the designator, we shouldn't be handling the
       // designator. Return immediately.
       if (!SubobjectIsDesignatorContext)
         return;
 
       HasDesignatedInit = true;
 
       // Handle this designated initializer. Field will be updated to
       // the next field that we'll be initializing.
-      if (CheckDesignatedInitializer(Entity, IList, DIE, 0,
-                                     DeclType, &Field, nullptr, Index,
-                                     StructuredList, StructuredIndex,
-                                     true, TopLevelObject))
+      bool DesignatedInitFailed = CheckDesignatedInitializer(
+          Entity, IList, DIE, 0, DeclType, &Field, nullptr, Index,
+          StructuredList, StructuredIndex, true, TopLevelObject);
+      if (DesignatedInitFailed)
         hadError = true;
-      else if (!VerifyOnly) {
-        // Find the field named by the designated initializer.
-        RecordDecl::field_iterator F = RD->field_begin();
-        while (std::next(F) != Field)
-          ++F;
+
+      DesignatedInitExpr::Designator *D = DIE->getDesignator(0);
+      if (!VerifyOnly && D->isFieldDesignator()) {
+        FieldDecl *F = D->getFieldDecl();
+        InitializedFields.insert(F);
+        if (!DesignatedInitFailed) {
           QualType ET = SemaRef.Context.getBaseElementType(F->getType());
           if (checkDestructorReference(ET, InitLoc, SemaRef)) {
             hadError = true;
             return;
           }
         }
+      }
 
       InitializedSomething = true;
 
       // Disable check for missing fields when designators are used.
       // This matches gcc behaviour.
+      if (!SemaRef.getLangOpts().CPlusPlus)
         CheckForMissingFields = false;
       continue;
     }
 
     // Check if this is an initializer of forms:
     //
     //   struct foo f = {};
     //   struct foo g = {0};
     //
if (!VerifyOnly) {
       }
     }
 
     InitializedEntity MemberEntity =
       InitializedEntity::InitializeMember(*Field, &Entity);
     CheckSubElementType(MemberEntity, IList, Field->getType(), Index,
                         StructuredList, StructuredIndex);
     InitializedSomething = true;
+    InitializedFields.insert(*Field);
 
     if (RD->isUnion() && StructuredList) {
       // Initialize the first field within the union.
       StructuredList->setInitializedFieldInUnion(*Field);
     }
 
     ++Field;
   }
 
   // Emit warnings for missing struct field initializers.
   if (!VerifyOnly && InitializedSomething && CheckForMissingFields &&
-      Field != FieldEnd && !Field->getType()->isIncompleteArrayType() &&
       !RD->isUnion()) {
     // It is possible we have one or more unnamed bitfields remaining.
     // Find first (if any) named field and emit warning.
-    for (RecordDecl::field_iterator it = Field, end = RD->field_end();
+    for (RecordDecl::field_iterator it = HasDesignatedInit ? RD->field_begin()
+                                                           : Field,
+                                    end = RD->field_end();
          it != end; ++it) {
+      if (HasDesignatedInit && InitializedFields.count(*it))
+        continue;
+
       if (!it->isUnnamedBitfield() && !it->hasInClassInitializer()) {
         SemaRef.Diag(IList->getSourceRange().getEnd(),
-                     diag::warn_missing_field_initializers) << *it;
+                     diag::warn_missing_field_initializers)
+            << *it;
         break;
       }
     }
   }
 
   // Check that any remaining fields can be value-initialized if we're not
   // building a structured list. (If we are, we'll check this later.)
   if (!StructuredList && Field != FieldEnd && !RD->isUnion() &&

clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp

 // RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected,pedantic,override,reorder -pedantic-errors
 // RUN: %clang_cc1 -std=c++17 %s -verify=expected,pedantic,override,reorder -Wno-c++20-designator -pedantic-errors
 // RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected,pedantic -Werror=c99-designator -Wno-reorder-init-list -Wno-initializer-overrides
 // RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected,reorder -Wno-c99-designator -Werror=reorder-init-list -Wno-initializer-overrides
 // RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected,override -Wno-c99-designator -Wno-reorder-init-list -Werror=initializer-overrides
 // RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected -Wno-c99-designator -Wno-reorder-init-list -Wno-initializer-overrides
+// RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected,wmissing -Wmissing-field-initializers -Wno-c99-designator -Wno-reorder-init-list -Wno-initializer-overrides
 
 
 namespace class_with_ctor {
   struct A { // cxx20-note 6{{candidate}}
     A() = default; // cxx20-note 3{{candidate}}
     int x;
     int y;
   };

 };
 A a3 = {
   1, // pedantic-note {{first non-designated initializer is here}}
   .y = 2 // pedantic-error {{mixture of designated and non-designated initializers in the same initializer list is a C99 extension}}
 };
 A a4 = {
   .x = 1, // override-note {{previous}}
   .x = 1 // override-error {{overrides prior initialization}}
-};
+}; // wmissing-warning {{missing field 'y' initializer}}
 A a5 = {
   .y = 1, // override-note {{previous}}
   .y = 1 // override-error {{overrides prior initialization}}
-};
+}; // wmissing-warning {{missing field 'x' initializer}}
 B b2 = {.a = 1}; // pedantic-error {{brace elision for designated initializer is a C99 extension}}
+                 // wmissing-warning@-1 {{missing field 'y' initializer}}
 B b3 = {.a = 1, 2}; // pedantic-error {{mixture of designated and non-designated}} pedantic-note {{first non-designated}} pedantic-error {{brace elision}}
 B b4 = {.a = 1, 2, 3}; // pedantic-error {{mixture of designated and non-designated}} pedantic-note {{first non-designated}} pedantic-error {{brace elision}} expected-error {{excess elements}}
 B b5 = {.a = nullptr}; // expected-error {{cannot initialize}}
+                       // wmissing-warning@-1 {{missing field 'y' initializer}}
 struct C { int :0, x, :0, y, :0; };
 C c = {
   .x = 1, // override-note {{previous}}
   .x = 1, // override-error {{overrides prior initialization}} override-note {{previous}}
   .y = 1, // override-note {{previous}}
   .y = 1, // override-error {{overrides prior initialization}} // reorder-note {{previous initialization for field 'y' is here}}
   .x = 1, // reorder-error {{declaration order}} override-error {{overrides prior initialization}} override-note {{previous}}
   .x = 1, // override-error {{overrides prior initialization}}
 };
+
+struct Foo { int a, b; };
+
+struct Foo foo0 = { 1 }; // wmissing-warning {{missing field 'b' initializer}}
+struct Foo foo1 = { .a = 1 }; // wmissing-warning {{missing field 'b' initializer}}
+struct Foo foo2 = { .b = 1 }; // wmissing-warning {{missing field 'a' initializer}}
+
 }
 
 namespace base_class {
   struct base {
     int x;
   };
   struct derived : base {
     int y;
struct : public A, public B {
    .a=4}, // reorder-error {{field 'b' will be initialized after field 'a'}}
     .e = 1, // reorder-note {{previous initialization for field 'e' is here}}
             // pedantic-error@-1 {{mixture of designated and non-designated initializers in the same initializer list is a C99 extension}}
     .d = 1, // reorder-error {{field 'e' will be initialized after field 'd'}}
             // reorder-note@-1 {{previous initialization for field 'd' is here}}
     .c = 1, // reorder-error {{field 'd' will be initialized after field 'c'}} // reorder-note {{previous initialization for field 'c' is here}}
     .b = 1, // reorder-error {{field 'c' will be initialized after field 'b'}} // reorder-note {{previous initialization for field 'b' is here}}
     .a = 1, // reorder-error {{field 'b' will be initialized after field 'a'}}
 };
 }