[Sema] PR25755 Handle out of order designated initializers

[hintonda]

Summary

Keep track of which fields have been initialized when designated initializers are used, and only verify the unseen ones can be implicitly initialized them at the top level object.

[hintonda]

• Fix additional tests

[hintonda]

Moved new test into existing test file.

[hintonda]

Current diff is incorrect, but I did find the problem.

When iterating over the fields of a record, we use an iterator. The problem is that if we use a designator, we reset the iterator value, so we can no longer check Field != FieldEnd if the designators are out of order.

So, if we want to allow designators to be out of order, we'll need to keep track of what was actually initialized, not just that we initialized the correct number. (see SemaInit.cpp:1825)

I'll work up a fix.

[hintonda]

When checking record initialization, maintain a vector of remaining
fields that were not explicitly initialized and check only those
fields to see if they can be default initialized.

[hintonda]

Improve efficiency by using pair<> to maintain counts instead of purging seen fields.

[hintonda]

Improve efficiency by only keeping track of seen fields, and only when
designators have been used.

[rsmith]

lib/Sema/SemaInit.cpp

1732

Please use an llvm::BitVector here instead.

1756–1769

You can get the field index with Field->getFieldIndex() - 1 rather than computing it here (though watch out for the case where Field == FieldEnd).

1793–1795

We don't need to track whether unnamed bit-fields have been "seen". This isn't meaningful.

1808–1809

Once we're in the hadError state, we don't need to track SeenFields any more. I don't think we need this change.

[hintonda]

Addressed comments.

[rsmith]

lib/Sema/SemaInit.cpp

1731–1732

You don't need either of these variables.

1757–1766

Drop this. Instead, check for CheckForMissingFields in the loop below.

1768

Use RD->getNumFields() - 1 here.

1829

Use Field->getFieldIndex()

1880–1898

Don't build a list of missing fields, just walk all the fields and check whether each one is in the set before processing it. (You can optimize this somewhat: if there were any designators -- including the case where Field was not RD->field_begin() on entry to this function -- start from RD->field_begin(), otherwise start from Field.)

[hintonda]

Address additional comments.

Always maintain SeenFields, not just when designators are seen which
simplies the logic. Note that there is no getNumFields() method, so
we still need to determine the total FieldSize to initialize the
BitVector and loop through it.

[hintonda]

lib/Sema/SemaInit.cpp

1734–1736

Still need FieldSize -- see below.

1778

Couldn't find getNumFields(), counted them instead.

[rsmith]

lib/Sema/SemaInit.cpp

1737–1742

FieldStart isn't necessarily the first field (in particular, if we got here from a nested field designator). Instead, how about using

FieldSize = std::distance(RD->field_begin(), RD->field_end());

1872

The CheckForMissingFields test here doesn't make much sense with this name. Please reverse the sense of this flag, rename it to HadAnyDesignators or similar, and initialize it to true if Field != RD->field_begin() before you enter the field initialization loop.

1873

Start from RD->field_begin() here (or track the position at which you first set HadAnyDesignators to true and start from there). In particular, if FieldStart != RD->field_begin(), then an initial sequence of fields is uninitialized unless a later designator jumped back to them.

1876–1877

Use SeenFields[Field->getFieldIndex()] here and then remove the variable i. We don't need to hardcode assumptions about how field indexes are computed here.

[hintonda]

I'm probably missing something, but isn't InitListChecker::CheckStructUnionTypes() called recursively, if indirectly? In which case we'd want to start from the Field iterator we were given, not the look at all the fields.

[hintonda]

lib/Sema/SemaInit.cpp

1876–1877

You raise a good point I missed. SeenFields should be big enough to mark all fields, but we should only set/examine from FieldStart->getFieldIndex(). However, we still need the i.

[rsmith]

In D17407#385208, @hintonda wrote:

I'm probably missing something, but isn't InitListChecker::CheckStructUnionTypes() called recursively, if indirectly? In which case we'd want to start from the Field iterator we were given, not the look at all the fields.

No: if CheckDesignatedInitializer finds a top-level designator (denoting a field in the same struct for which it was called, rather than a subobject of that field), it just returns. CheckStructUnionTypes should never recursively re-enter the same subobject.

However, you do make a good point: we *can* enter CheckStructUnionTypes multiple times for the same subobject, in a case like

struct Q { Q(int); }
struct A { Q x, y; };
struct B { A a; int k; };
B b = { .a.y = 0, 0, .a.x = 0 };

... and we should not CheckEmptyInitializable in that case, nor warn that an initializer for a.x is missing. (But we should do so if the final initializer for a.x is not present.)

It looks like we need to do all this checking as a separate pass after checking the initialization, even in VerifyOnly mode.

[hintonda]

• Initial checkin from original D17407.
• Refactored code and simplify tests based on review comments.

[xbolva00]

Please check also https://bugs.llvm.org/show_bug.cgi?id=40030

[hintonda]

In D17407#1439029, @xbolva00 wrote:

Please check also https://bugs.llvm.org/show_bug.cgi?id=40030

Thanks for pointing that out. I was mainly handling asserts, but will add the restrictions as well.

[hintonda]

I took a look at C++ designator restrictions shown here aggregate initialization, and believe it to be orthogonal to this change. Thus, I'd prefer to put that in a separate patch.

However, based on this, I also believe I should move the tests into test/Sema/designated-initializers.c.

Please let me know if that sounds like the right thing to do.

[hintonda]

In D17407#1439029, @xbolva00 wrote:

Please check also https://bugs.llvm.org/show_bug.cgi?id=40030

If you compile with -pedantic, you'll get the following warning: warning: designated initializers are a C99 feature [-Wc99-extensions]

This warning currently applies for all versions of C++, but should probably not warn for cases supported in c++20. However, that's a bigger fix, since it would need to discriminate between what c++20 does and does not support, e.g., nested and out of order designators are not included in c++20.

@rsmith This is ready for review when you get a chance.

[hintonda]

ping...

lib/Sema/SemaInit.cpp

if (DeclType->isUnionType() && IList->getNumInits() == 0) {
     }
     return;
   }
 
   // If structDecl is a forward declaration, this loop won't do
   // anything except look at designated initializers; That's okay,
   // because an error should get printed out elsewhere. It might be
   // worthwhile to skip over the rest of the initializer, though.
+  unsigned FieldIdx = 0;
+  std::vector<unsigned> SeenFields;
+  RecordDecl::field_iterator FieldStart = Field;
   RecordDecl *RD = DeclType->getAs<RecordType>()->getDecl();
   RecordDecl::field_iterator FieldEnd = RD->field_end();
   bool InitializedSomething = false;
   bool CheckForMissingFields = true;
   while (Index < IList->getNumInits()) {
     Expr *Init = IList->getInit(Index);
 
     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;
 
       // 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))
         hadError = true;
 
+      if (CheckForMissingFields)
+        for (unsigned i = 0; i < FieldIdx; ++i)
+          SeenFields.push_back(i);
+      unsigned idx = 0;
+      for (RecordDecl::field_iterator f = FieldStart;
+           f != FieldEnd; ++idx, ++f) {
+        auto Next = f;
+        if (++Next == Field) {
+          FieldIdx = idx;
+          break;
+        }
+      }
+      SeenFields.push_back(FieldIdx);
+      ++FieldIdx;
       InitializedSomething = true;
 
       // Disable check for missing fields when designators are used.
       // This matches gcc behaviour.
       CheckForMissingFields = false;
       continue;
     }
 
     if (Field == FieldEnd) {
       // We've run out of fields. We're done.
       break;
     }
 
     // We've already initialized a member of a union. We're done.
     if (InitializedSomething && DeclType->isUnionType())
       break;
 
     // If we've hit the flexible array member at the end, we're done.
     if (Field->getType()->isIncompleteArrayType())
       break;
 
     if (Field->isUnnamedBitfield()) {
       // Don't initialize unnamed bitfields, e.g. "int : 20;"
+      if (!CheckForMissingFields)
+        SeenFields.push_back(FieldIdx);
+      ++FieldIdx;
       ++Field;
       continue;
     }
 
     // Make sure we can use this declaration.
     bool InvalidUse;
     if (VerifyOnly)
       InvalidUse = !SemaRef.CanUseDecl(*Field);
     else
       InvalidUse = SemaRef.DiagnoseUseOfDecl(*Field,
                                           IList->getInit(Index)->getLocStart());
     if (InvalidUse) {
+      if (!CheckForMissingFields)
+        SeenFields.push_back(FieldIdx);
+      ++FieldIdx;
       ++Index;
       ++Field;
       hadError = true;
       continue;
     }
 
     InitializedEntity MemberEntity =
       InitializedEntity::InitializeMember(*Field, &Entity);
     CheckSubElementType(MemberEntity, IList, Field->getType(), Index,
                         StructuredList, StructuredIndex);
     InitializedSomething = true;
 
     if (DeclType->isUnionType() && !VerifyOnly) {
       // Initialize the first field within the union.
       StructuredList->setInitializedFieldInUnion(*Field);
     }
 
+    if (!CheckForMissingFields)
+      SeenFields.push_back(FieldIdx);
+    ++FieldIdx;
     ++Field;
   }
 
   // Emit warnings for missing struct field initializers.
   if (!VerifyOnly && InitializedSomething && CheckForMissingFields &&
       Field != FieldEnd && !Field->getType()->isIncompleteArrayType() &&
       !DeclType->isUnionType()) {
     // 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();
          it != end; ++it) {
       if (!it->isUnnamedBitfield() && !it->hasInClassInitializer()) {
         SemaRef.Diag(IList->getSourceRange().getEnd(),
                      diag::warn_missing_field_initializers) << *it;
         break;
       }
     }
   }
 
+  if (!CheckForMissingFields) {
+    std::sort(SeenFields.begin(), SeenFields.end());
+    SeenFields.erase(std::unique(SeenFields.begin(), SeenFields.end()), SeenFields.end());
+  } else
+    FieldStart = Field;
+  SmallVector<RecordDecl::field_iterator, 10> MissingFields;
+  unsigned Idx = 0;
+  for (auto i : SeenFields) {
+    while (Idx < i) {
+      MissingFields.push_back(FieldStart);
+      ++FieldStart;
+      ++Idx;
+    }
+    ++FieldStart;
+    ++Idx;
+  }
+  while (FieldStart != FieldEnd)
+    MissingFields.push_back(FieldStart++);
+
   // Check that any remaining fields can be value-initialized.
-  if (VerifyOnly && Field != FieldEnd && !DeclType->isUnionType() &&
-      !Field->getType()->isIncompleteArrayType()) {
-    // FIXME: Should check for holes left by designated initializers too.
-    for (; Field != FieldEnd && !hadError; ++Field) {
-      if (!Field->isUnnamedBitfield() && !Field->hasInClassInitializer())
+  if (VerifyOnly && MissingFields.size() && !DeclType->isUnionType()) {
+    for (auto MissedField : MissingFields) {
+      Field = MissedField;
+      if (hadError)
+        break;
+      if (!Field->isUnnamedBitfield() && !Field->hasInClassInitializer() &&
+          !Field->getType()->isIncompleteArrayType())
         CheckEmptyInitializable(
             InitializedEntity::InitializeMember(*Field, &Entity),
             IList->getLocEnd());
     }
   }
 
   if (Field == FieldEnd || !Field->getType()->isIncompleteArrayType() ||
       Index >= IList->getNumInits())
     return;
 
   if (CheckFlexibleArrayInit(Entity, IList->getInit(Index), *Field,
                              TopLevelObject)) {
     hadError = true;
     ++Index;
     return;
   }
 
   InitializedEntity MemberEntity =
     InitializedEntity::InitializeMember(*Field, &Entity);
 
   if (isa<InitListExpr>(IList->getInit(Index)))
     CheckSubElementType(MemberEntity, IList, Field->getType(), Index,
                         StructuredList, StructuredIndex);
   else
     CheckImplicitInitList(MemberEntity, IList, Field->getType(), Index,
                           StructuredList, StructuredIndex);
 }
 
 /// \brief Expand a field designator that refers to a member of an
 /// anonymous struct or union into a series of field designators that

test/SemaCXX/cxx0x-initializer-constructor.cpp

namespace PR11410 {
   };
   C c[3] = {
     0, 1
   }; // expected-error {{ambiguous}} expected-note {{in implicit initialization of array element 2}}
   C c2[3] = {
     [0] = 1, [2] = 3
   }; // expected-error {{ambiguous}} expected-note {{in implicit initialization of array element 1}}
 }
+
+namespace PR23514 {
+struct Test {
+  int& a;
+  int& b;
+};
+
+int d = 0;
+auto a = Test { .b = d, .a = d, };
+
+struct S { // expected-note-re 2{{candidate constructor (the implicit {{.*}} constructor) not viable: requires 1 argument, but 0 were provided}}
+  S(int a) {} // expected-note {{candidate constructor not viable: requires single argument 'a', but no arguments were provided}}
+};
+
+struct P {
+  char i1;
+  char i2;
+  S s;
+};
+
+P p1 { .s = 2, .i1 = 1, 1, };
+P p2 { .s = 2, .i1 = 1, };
+
+struct PP {
+  char i1;
+  S s1;
+  S s2; // expected-note {{in implicit initialization of field 's2' with omitted initializer}}
+};
+
+PP pp { .s1 = 2, .i1 = 1, }; // expected-error {{no matching constructor for initialization of 'PR23514::S'}}
+}