[Sema] Add c++2a designated initializer warnings

[rsmith]

Summary

Basic designated initializers are allowed in c++2a, so only
issue warnings for the extended ones that aren't allowed:

• out of order
• nested
• arrays
• mixed

[xbolva00]

clang/lib/Sema/SemaInit.cpp

2002

typo

[hintonda]

• Fix variable typo/spellings, and add missing flag assignment and additional test to catch it.

[Rakete1111]

clang/lib/Sema/SemaInit.cpp

2017

Can Field ever be FieldEnd at this point?

2044

!VerifyOnly and braces please.

2066

Same here. It should be !VerifyOnly. Also braces for the if statement.

3092–3099

Missing dot.

3094

Braces.

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

30

Those warnings are misleading, since C++20 does have designated initializers; they just don't support some stuff that C99 does. It would be better IMO if you could separate them. As in, the above should give you: out-of-order designated initializers are a C99 feature or something like that.

[hintonda]

Thanks for the review.

clang/lib/Sema/SemaInit.cpp

2017

Yes. If the last trip through the loop was a designated initializer, and it happened to be the last field in the RecordDecl, Field will be equal FieldEnd. At this point, the loop will exit below unless there's another designated initializer that's out of order.

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

30

I think that would be a good idea as well, but wanted to get advise first.

[lebedev.ri]

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

30

As in, the above should give you: out-of-order designated initializers are a C99 feature or something like that.

I suppose also the question is, whether to error-out, or support them as an extension?

[hintonda]

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

30

Although most of them seem fine, the nested ones can be problematic. Please see https://reviews.llvm.org/D17407 for a proposal on how to fix them.

[Rakete1111]

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

30

I suppose also the question is, whether to error-out, or support them as an extension?

Yes that's true. gcc doesn't support them at all in C++, and it seems like we accept it as well, but only for C classes (constructors make clang crash).

But making it an error now breaks backwards compatibility. So I think the best solution is to accept it for now, as an extension.

[hintonda]

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

30

Btw, several cpp tests use nested designated initializers. Here's a quick list I got by temporarily making it an error:

CodeGenCXX/mangle-exprs.cpp
SemaCXX/cxx2a-initializer-aggregates.cpp
SemaCXX/decltype.cpp
SemaTemplate/instantiate-init.cpp

I believe there are a several out of order ones as well, but those might have been unintentional.

[hintonda]

clang/lib/Sema/SemaInit.cpp

2044

Will commit this change as soon as check-clang finishes, but not sure I grok the VerityOnly/!VerifyOnly criteria. Could you help me out?

[hintonda]

• Address comments.

[hintonda]

• Missed one set of braces.

[Rakete1111]

clang/lib/Sema/SemaInit.cpp

2044

VerifyOnly is used if you only want to verify and not actually generate any diagnostics or actually do anything that would commit to that change. So only generate the warning if !VerifyOnly.

[hintonda]

ping, and should we add new warning messages as suggested by @Rakete1111 ?

[rsmith]

clang/lib/Sema/SemaInit.cpp

2017

Please use an actual type rather than auto here and below.

2046

This should use a distinct diagnostic that says what part is an extension.

3096

Likewise here, we should say what specifically is an extension.

[hintonda]

• Removed auto and added specific warnings.

[hintonda]

• Fix typo.

[rsmith]

I think you're missing the enforcement of the rule that the same field name cannot be designated multiple times in a single designated-initializer-list. I'm fine with that being done separately (not as part of this patch), though.

clang/include/clang/Basic/DiagnosticSemaKinds.td

158 ↗

(On Diff #195611)

See http://clang.llvm.org/docs/InternalsManual.html#the-format-string -- do not pass English text as arguments to diagnostics as this interferes with localization. Use %select instead, or use different diagnostics for the different kinds of problem.

I think use of multiple diagnostics would be preferable anyway, as it'd give you room to explain the nature of the problem better (for instance, rather than "mixed designated initializers", you could say "mixing designated and non-designated initializers in the same initializer list is a C99 feature not permitted in C++" or something like that).

Also, we use "X is a C++20 extension" to mean "this is a C++20 feature but you don't have C++20 enabled", which is very different from what you're using it to mean here. "ISO C++20 does not support X" is how we'd usually phrase "X is not a feature of C++20 but we know what you mean".

159–160 ↗

(On Diff #195611)

In C++ <=17 mode, if we see a designated initializer that would be valid in C++20, we should diagnose that designated initializers are a C++20 feature, not that they're a C99 feature.

clang/lib/Sema/SemaInit.cpp

2065–2068

I think it would be preferable to diagnose the "mixed" case in the parser rather than here (it's a grammatical restriction in C++, after all). I'm worried that handling it here will get some cases wrong, such as perhaps:

struct A {
  union { int x, y; };
  int z;
};
A a = { .x = 123, 456 }; // should be rejected, but I think this patch might accept

... and it might also get template instantiation cases wrong for a case like:

struct Q { Q(); };
struct A { Q x, y; };
template<typename T> void f() {
  A a = {.y = Q()};
}
void g() { f<int>(); }

... where we might possibly end up passing an InitListExpr representing {Q(), .y = Q()} into InitListChecker.

I think the only C++20 restriction that it makes sense to check in InitListChecker is that the field names are in the right order; everything else should be checked earlier. This would also match the semantics of overload resolution, for which the "fields are in the right order" check is deferred until after a function is selected, whereas all the other checks are performed eagerly.

[hintonda]

clang/lib/Sema/SemaInit.cpp

2065–2068

Will work on moving these to the parser.

Btw, the first one is diagnosed correctly, but doesn't complain about the second. Not sure I grok the problem there either since Q has a default ctor.

[hintonda]

clang/lib/Sema/SemaInit.cpp

2065–2068

Woke up this morning and realized what you meant about the union. I'll take care of it in the next patch.

thanks again...

[sokolovd]

This looks like exactly what we need for my project. We're using Clang and Designated Initializers but would like to make sure that we use those in C++20 compatible manner. Is this blocked on something? Any way I can help?

[hintonda]

In D59754#1543949, @sokolovd wrote:

This looks like exactly what we need for my project. We're using Clang and Designated Initializers but would like to make sure that we use those in C++20 compatible manner. Is this blocked on something? Any way I can help?

The basic logic is here modulo @rsmith's comments. I just haven't had time to address them, and unfortunately, it might be another few weeks before I can get back to it.

If you'd like to work on it and finish it up, that would be great.

[rsmith]

I'm taking this over to finish it off and submit.

[hintonda]

In D59754#1651894, @rsmith wrote:

I'm taking this over to finish it off and submit.

Thanks Richard...

[xbolva00]

cfe/trunk/lib/Sema/SemaInit.cpp

3112 ↗

(On Diff #218179)

warning: variable ‘HasArrayDesignator’ set but not used [-Wunused-but-set-variable]

bool HasArrayDesignator = false;

[leonardchan]

Hi! We've noticed that for our arm bots, we're getting some flaky builds that sometimes fail with error: array designators are a C99 extension [-Werror,-Wc99-designator] and sometimes don't fail. 2 questions:

1. I can't see it off the patch immediately, but do you know why for arm specifically we can only get this warning sometimes?
2. I noticed that for the test/SemaCXX/c99.cpp test, this warning is also diagnosed for the -std=c++17 case. Are C-style designated initializers only invalid in c++20, or are they also invalid in 17?

Thanks.

[phosek]

In D59754#1656217, @leonardchan wrote:

Hi! We've noticed that for our arm bots, we're getting some flaky builds that sometimes fail with error: array designators are a C99 extension [-Werror,-Wc99-designator] and sometimes don't fail. 2 questions:

1. I can't see it off the patch immediately, but do you know why for arm specifically we can only get this warning sometimes?
2. I noticed that for the test/SemaCXX/c99.cpp test, this warning is also diagnosed for the -std=c++17 case. Are C-style designated initializers only invalid in c++20, or are they also invalid in 17?

Thanks.

@rsmith ping? We're still hitting this issue in our build. Should this warning be diagnosed even when using -std=c++17?

[rsmith]

In D59754#1656217, @leonardchan wrote:

Hi! We've noticed that for our arm bots, we're getting some flaky builds that sometimes fail with error: array designators are a C99 extension [-Werror,-Wc99-designator] and sometimes don't fail. 2 questions:

1. I can't see it off the patch immediately, but do you know why for arm specifically we can only get this warning sometimes?

That is strange; can you provide me with buildbot links for a pass and a failure?

2. I noticed that for the test/SemaCXX/c99.cpp test, this warning is also diagnosed for the -std=c++17 case. Are C-style designated initializers only invalid in c++20, or are they also invalid in 17?

C-style designated initializers are invalid in all C++ standards before C++20, and some of the features are still invalid in C++20. We allow those features as an extension, but as of this change we produce a warning by default when that extension is used. We really should have been doing that for years, but it matters a lot more now, because people are going to look at Clang's diagnostics to understand which parts of C designator syntax are valid in C++ and which parts are not.

[thakis]

Another question about this, sorry. Do you know _why_ C++20 is more restrictive than C99 wrt "mixture of designated and non-designated initializers in the same initializer list is a C99 extension"? Is there some interaction with other C++ features that makes the C99 behavior difficult in C++20?

[rsmith]

In D59754#1665601, @thakis wrote:

Another question about this, sorry. Do you know _why_ C++20 is more restrictive than C99 wrt "mixture of designated and non-designated initializers in the same initializer list is a C99 extension"? Is there some interaction with other C++ features that makes the C99 behavior difficult in C++20?

The design paper P0329R0 describes this behavior but doesn't give much rationale. According to the minutes and my recollections, this was part of the design as presented and didn't really receive any pushback at any stage of the feature design and discussion. I think generally the feeling was that there was insufficient motivation for adding that level of complexity (much like with the restriction to single-level designators and lack of support for array designators).

clang/lib/Sema/SemaInit.cpp

void InitListChecker::CheckStructUnionTypes(
   // 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.
   RecordDecl *RD = DeclType->getAs<RecordType>()->getDecl();
   RecordDecl::field_iterator FieldEnd = RD->field_end();
   bool CheckForMissingFields =
     !IList->isIdiomaticZeroInitializer(SemaRef.getLangOpts());
   bool HasDesignatedInit = false;
+  bool HasNonDesignatedInit = false;
 
   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;
 
+      auto LastIdx = Field != FieldEnd
+                         ? Field->getFieldIndex()
+                         : std::distance(RD->field_begin(), RD->field_end());
+
       // 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;
       else if (!VerifyOnly) {
         // Find the field named by the designated initializer.
         RecordDecl::field_iterator F = RD->field_begin();
         while (std::next(F) != Field)
           ++F;
         QualType ET = SemaRef.Context.getBaseElementType(F->getType());
         if (hasAccessibleDestructor(ET, InitLoc, SemaRef)) {
           hadError = true;
           return;
         }
       }
 
+      // Warn on out of order and mixed designators in C++20.
+      auto NextIdx = Field != FieldEnd
+                         ? Field->getFieldIndex()
+                         : std::distance(RD->field_begin(), RD->field_end());
+      if (!VerifyOnly && (LastIdx >= NextIdx || HasNonDesignatedInit) &&
+          SemaRef.getLangOpts().CPlusPlus2a) {
+        SemaRef.Diag(Init->getBeginLoc(), diag::ext_designated_init)
+            << Init->getSourceRange();
+      }
+
       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;
     }
 
+    HasNonDesignatedInit = true;
+
+    // Warn on mixed designators in C++20.
+    if (!VerifyOnly && HasDesignatedInit && SemaRef.getLangOpts().CPlusPlus2a) {
+      SemaRef.Diag(Init->getBeginLoc(), diag::ext_designated_init)
+          << Init->getSourceRange();
+    }
+
     // 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;
 
ExprResult Sema::ActOnDesignatedInitializer(Designation &Desig,
                                             SourceLocation Loc,
                                             bool GNUSyntax,
                                             ExprResult Init) {
   typedef DesignatedInitExpr::Designator ASTDesignator;
 
   bool Invalid = false;
   SmallVector<ASTDesignator, 32> Designators;
   SmallVector<Expr *, 32> InitExpressions;
+  bool HasArrayDesignator = false;
 
   // Build designators and check array designator expressions.
   for (unsigned Idx = 0; Idx < Desig.getNumDesignators(); ++Idx) {
     const Designator &D = Desig.getDesignator(Idx);
     switch (D.getKind()) {
     case Designator::FieldDesignator:
       Designators.push_back(ASTDesignator(D.getField(), D.getDotLoc(),
                                           D.getFieldLoc()));
       break;
 
     case Designator::ArrayDesignator: {
       Expr *Index = static_cast<Expr *>(D.getArrayIndex());
       llvm::APSInt IndexValue;
       if (!Index->isTypeDependent() && !Index->isValueDependent())
         Index = CheckArrayDesignatorExpr(*this, Index, IndexValue).get();
       if (!Index)
         Invalid = true;
       else {
         Designators.push_back(ASTDesignator(InitExpressions.size(),
                                             D.getLBracketLoc(),
                                             D.getRBracketLoc()));
         InitExpressions.push_back(Index);
       }
+      HasArrayDesignator = true;
       break;
     }
 
     case Designator::ArrayRangeDesignator: {
       Expr *StartIndex = static_cast<Expr *>(D.getArrayRangeStart());
       Expr *EndIndex = static_cast<Expr *>(D.getArrayRangeEnd());
       llvm::APSInt StartValue;
       llvm::APSInt EndValue;
case Designator::ArrayRangeDesignator: {
           Designators.push_back(ASTDesignator(InitExpressions.size(),
                                               D.getLBracketLoc(),
                                               D.getEllipsisLoc(),
                                               D.getRBracketLoc()));
           InitExpressions.push_back(StartIndex);
           InitExpressions.push_back(EndIndex);
         }
       }
+      HasArrayDesignator = true;
       break;
     }
     }
   }
 
   if (Invalid || Init.isInvalid())
     return ExprError();
 
   // Clear out the expressions within the designation.
   Desig.ClearExprs(*this);
 
   DesignatedInitExpr *DIE
     = DesignatedInitExpr::Create(Context,
                                  Designators,
                                  InitExpressions, Loc, GNUSyntax,
                                  Init.getAs<Expr>());
 
-  if (!getLangOpts().C99)
+  if (!getLangOpts().C99) {
+    // Warn on nested and array designators in C++20.
+    if (!getLangOpts().CPlusPlus2a || Desig.getNumDesignators() > 1 ||
+        HasArrayDesignator) {
       Diag(DIE->getBeginLoc(), diag::ext_designated_init)
           << DIE->getSourceRange();
+    }
+  }
 
   return DIE;
 }
 
 //===----------------------------------------------------------------------===//
 // Initialization entity
 //===----------------------------------------------------------------------===//
 

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

-// RUN: %clang_cc1 -std=c++2a %s -verify
+// RUN: %clang_cc1 -std=c++2a %s -verify -pedantic
 
 namespace class_with_ctor {
   struct A { // expected-note 6{{candidate}}
     A() = default; // expected-note 3{{candidate}}
     int x;
     int y;
   };
   A a = {1, 2}; // expected-error {{no matching constructor}}
 
   struct B {
     int x;
     int y;
   };
   B b1 = B(); // trigger declaration of implicit ctors
   B b2 = {1, 2}; // ok
 
   struct C : A {
     A a;
   };
   C c1 = {{}, {}}; // ok, call default ctor twice
   C c2 = {{1, 2}, {3, 4}}; // expected-error 2{{no matching constructor}}
 }
+
+namespace designator {
+struct A { int x, y; };
+struct B { A a; };
+
+// out of order designators
+A a1 = {.y = 1, .x = 2}; // expected-warning {{designated initializers are a C99 feature}}
+
+// array designator
+int arr[3] = {[1] = 5}; // expected-warning {{designated initializers are a C99 feature}}
+
+// nested designator
+B b = {.a.x = 0}; // expected-warning {{designated initializers are a C99 feature}}
+
+// mixed designator and non-designator
+A a2 = {.x = 1, 2}; // expected-warning {{designated initializers are a C99 feature}}
+A a3 = {1, .y = 2}; // expected-warning {{designated initializers are a C99 feature}}
+}