[clang] improving diagnotics for invalid constexpr defaulted special membres

[Tyker]

Summary

this patch improves diagnostic for invalid constexpr defaulted special members by adding notes explaining why the special member cannot be constexpr.

example
input:

01 struct B {};
02 
03 struct C : virtual B {
04 };
05 
06 struct D {
07   C c;
08 };
09 
10 struct A : D {
11   constexpr A() = default;
12 };
13 
14 union U {
15   A a;
16   int b;
17   constexpr U() = default;
18 };
19 
20 struct E {
21   ~E() {}
22 };
23 
24 struct F {
25   E e;
26   constexpr F& operator=(const F&) =default;
27 };

output with patch:

test.cpp:11:3: error: defaulted definition of default constructor is not constexpr because:
  constexpr A() = default;
  ^
test.cpp:10:12: note: base class 'D' of 'A' has a non-constexpr implicit default constructor
struct A : D {
           ^
test.cpp:7:5: note: non-static data member 'c' of 'D' has a non-constexpr implicit default constructor
  C c;
    ^
test.cpp:3:12: note: 'C' inherits virtually from 'B'
struct C : virtual B {
           ^
test.cpp:17:3: error: defaulted definition of default constructor is not constexpr because:
  constexpr U() = default;
  ^
note: unions require exactly one non-static data member initializer to have a constexpr default constructor
test.cpp:26:3: error: defaulted definition of copy assignment operator is not constexpr because:
  constexpr F& operator=(const F&) =default;
  ^
note: 'F' is not a literal type
3 errors generated.

I didn't adapt exitsing tests yet because the diagnostics emitted are likely be adapted during review.

[Tyker]

ping @rsmith

[Tyker]

ping @rsmith

[Tyker]

Rebased on current master

@rsmith Ping

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_defaulted_special_member_copy_const_param : Error<
   "the parameter for this explicitly-defaulted copy "
   "%select{constructor|assignment operator}0 is const, but a member or base "
   "requires it to be non-const">;
 def err_defaulted_copy_assign_not_ref : Error<
   "the parameter for an explicitly-defaulted copy assignment operator must be an "
   "lvalue reference type">;
 def err_incorrect_defaulted_constexpr : Error<
   "defaulted definition of %sub{select_special_member_kind}0 "
-  "is not constexpr">;
+  "is not constexpr%select{| because:}1">;
 def warn_defaulted_method_deleted : Warning<
   "explicitly defaulted %sub{select_special_member_kind}0 is implicitly "
   "deleted">, InGroup<DiagGroup<"defaulted-function-deleted">>;
 def err_out_of_line_default_deletes : Error<
   "defaulting this %sub{select_special_member_kind}0 "
   "would delete it after its first declaration">;
 def note_deleted_type_mismatch : Note<
   "function is implicitly deleted because its declared type does not match "
   "the type of an implicit %sub{select_special_member_kind}0">;
 def warn_cxx17_compat_defaulted_method_type_mismatch : Warning<
   "explicitly defaulting this %sub{select_special_member_kind}0 with a type "
   "different from the implicit type is incompatible with C++ standards before "
   "C++2a">, InGroup<CXXPre2aCompat>, DefaultIgnore;
 def warn_vbase_moved_multiple_times : Warning<
   "defaulted move assignment operator of %0 will move assign virtual base "
   "class %1 multiple times">, InGroup<DiagGroup<"multiple-move-vbase">>;
 def note_vbase_moved_here : Note<
   "%select{%1 is a virtual base class of base class %2 declared here|"
   "virtual base class %1 declared here}0">;
-
+def note_special_member_declared_here : Note<
+  "%sub{select_special_member_kind}0 declared here">;
+def note_constexpr_defaulted_special_member : Note<
+  "%select{|anonymous }5%select{non-static data member|base class}1"
+  "%select{ %2|}5 of %3 has a non-constexpr %select{|implicit }4"
+  "%sub{select_special_member_kind}0">;
+def note_constexpr_defaulted_virtual_base : Note<
+  "%0 inherits virtually from %1">;
+def note_constexpr_defaulted_union : Note<
+  "unions require exactly one non-static data member initializer"
+  " to have a constexpr %sub{select_special_member_kind}0">;
+def note_constexpr_defaulted_non_literal : Note<
+  "%0 is not a literal type">;
 def ext_implicit_exception_spec_mismatch : ExtWarn<
   "function previously declared with an %select{explicit|implicit}0 exception "
   "specification redeclared with an %select{implicit|explicit}0 exception "
   "specification">, InGroup<DiagGroup<"implicit-exception-spec-mismatch">>;
 
 def warn_ptr_arith_precedes_bounds : Warning<
   "the pointer decremented by %0 refers before the beginning of the array">,
   InGroup<ArrayBoundsPointerArithmetic>, DefaultIgnore;

clang/lib/Sema/SemaDeclCXX.cpp

 
 /// Is the special member function which would be selected to perform the
 /// specified operation on the specified class type a constexpr constructor?
 static bool
 specialMemberIsConstexpr(Sema &S, CXXRecordDecl *ClassDecl,
                          Sema::CXXSpecialMember CSM, unsigned Quals,
                          bool ConstRHS,
                          CXXConstructorDecl *InheritedCtor = nullptr,
-                         Sema::InheritedConstructorInfo *Inherited = nullptr) {
+                         Sema::InheritedConstructorInfo *Inherited = nullptr,
+                         SourceLocation* SMLoc = nullptr) {
   // If we're inheriting a constructor, see if we need to call it for this base
   // class.
   if (InheritedCtor) {
     assert(CSM == Sema::CXXDefaultConstructor);
     auto BaseCtor =
         Inherited->findConstructorForBase(ClassDecl, InheritedCtor).first;
-    if (BaseCtor)
+    if (BaseCtor) {
+      if (SMLoc && !BaseCtor->isImplicit())
+        *SMLoc = BaseCtor->getLocation();
       return BaseCtor->isConstexpr();
     }
+  }
 
-  if (CSM == Sema::CXXDefaultConstructor)
-    return ClassDecl->hasConstexprDefaultConstructor();
+  if (CSM == Sema::CXXDefaultConstructor) {
+    bool IsConstexprCtor = ClassDecl->hasConstexprDefaultConstructor();
+    if (IsConstexprCtor || !SMLoc)
+      return IsConstexprCtor;
+  }
 
   Sema::SpecialMemberOverloadResult SMOR =
       lookupCallFromSpecialMember(S, ClassDecl, CSM, Quals, ConstRHS);
   if (!SMOR.getMethod())
     // A constructor we wouldn't select can't be "involved in initializing"
     // anything.
     return true;
+  if (SMLoc && !SMOR.getMethod()->isImplicit())
+    *SMLoc = SMOR.getMethod()->getLocation();
   return SMOR.getMethod()->isConstexpr();
 }
 
 /// Determine whether the specified special member function would be constexpr
 /// if it were implicitly defined.
 static bool defaultedSpecialMemberIsConstexpr(
-    Sema &S, CXXRecordDecl *ClassDecl, Sema::CXXSpecialMember CSM,
-    bool ConstArg, CXXConstructorDecl *InheritedCtor = nullptr,
-    Sema::InheritedConstructorInfo *Inherited = nullptr) {
+    Sema &S, const CXXRecordDecl *ClassDecl, Sema::CXXSpecialMember CSM,
+    bool ConstArg,
+    CXXConstructorDecl *InheritedCtor = nullptr,
+    Sema::InheritedConstructorInfo *Inherited = nullptr,
+    SmallVectorImpl<PartialDiagnosticAt>* DiagNotes = nullptr) {
   if (!S.getLangOpts().CPlusPlus11)
     return false;
 
+  SourceLocation SMLoc;
+  SourceLocation DiagLoc;
+  enum class FailKind {
+    Subobject,
+    Union,
+    VirtualBase,
+    NonLiteral,
+  };
+  /// This lambda will gathering all information and add notes for diagnostics.
+  auto Fail = [&](FailKind FailureKind, const Decl *FaillingDecl = nullptr) {
+    if (DiagNotes) {
+      switch (FailureKind) {
+      case FailKind::Subobject: {
+        PartialDiagnostic PDiag(diag::note_constexpr_defaulted_special_member, S.Context.getDiagAllocator());
+        auto *Field = dyn_cast<FieldDecl>(FaillingDecl);
+        auto *Record = dyn_cast<CXXRecordDecl>(FaillingDecl);
+        SourceLocation Loc = FaillingDecl->getLocation();
+        if (!Record)
+          Record = Field->getType()->getAsCXXRecordDecl();
+        if (DiagLoc.isValid())
+          Loc = DiagLoc;
+        PDiag << CSM << isa<CXXRecordDecl>(FaillingDecl);
+        if (Field)
+          PDiag << Field;
+        else
+          PDiag << Record;
+        PDiag << ClassDecl << SMLoc.isInvalid()
+              << (Field ? Field->getDeclName().isEmpty() : false);
+        DiagNotes->emplace_back(Loc, PDiag);
+        if (SMLoc.isValid()) {
+          DiagNotes->emplace_back(
+              SMLoc, PartialDiagnostic(diag::note_special_member_declared_here,
+                                       S.Context.getDiagAllocator())
+                         << CSM);
+          return false;
+        }
+        if (!Record)
+          return false;
+        defaultedSpecialMemberIsConstexpr(S, Record, CSM, ConstArg, nullptr,
+                                          nullptr, DiagNotes);
+        return false;
+      }
+      case FailKind::VirtualBase: {
+        PartialDiagnostic PDiag(diag::note_constexpr_defaulted_virtual_base,
+                                S.Context.getDiagAllocator());
+        const CXXBaseSpecifier& Base = *ClassDecl->bases_begin();
+        PDiag << ClassDecl << Base.getType();
+        DiagNotes->emplace_back(Base.getBeginLoc(), PDiag);
+        return false;
+      }
+      case FailKind::Union:
+        DiagNotes->emplace_back(
+            SourceLocation(),
+            PartialDiagnostic(diag::note_constexpr_defaulted_union,
+                              S.Context.getDiagAllocator())
+                << CSM);
+        return false;
+      case FailKind::NonLiteral:
+        // FIXME: Explain why it is not literal.
+        DiagNotes->emplace_back(
+            SourceLocation(),
+            PartialDiagnostic(diag::note_constexpr_defaulted_non_literal,
+                              S.Context.getDiagAllocator())
+                << ClassDecl);
+        return false;
+      }
+    }
+    return false;
+  };
+
   // C++11 [dcl.constexpr]p4:
   // In the definition of a constexpr constructor [...]
   bool Ctor = true;
+  bool IsCtorConstexpr = false;
   switch (CSM) {
   case Sema::CXXDefaultConstructor:
     if (Inherited)
       break;
     // Since default constructor lookup is essentially trivial (and cannot
     // involve, for instance, template instantiation), we compute whether a
     // defaulted default constructor is constexpr directly within CXXRecordDecl.
     //
     // This is important for performance; we need to know whether the default
     // constructor is constexpr to determine whether the type is a literal type.
-    return ClassDecl->defaultedDefaultConstructorIsConstexpr();
+    IsCtorConstexpr = ClassDecl->defaultedDefaultConstructorIsConstexpr();
+    if (DiagNotes && !IsCtorConstexpr)
+      break;
+    return IsCtorConstexpr;
 
   case Sema::CXXCopyConstructor:
   case Sema::CXXMoveConstructor:
     // For copy or move constructors, we need to perform overload resolution.
     break;
 
   case Sema::CXXCopyAssignment:
   case Sema::CXXMoveAssignment:
static bool defaultedSpecialMemberIsConstexpr(
 
   //   -- if the class is a non-empty union, or for each non-empty anonymous
   //      union member of a non-union class, exactly one non-static data member
   //      shall be initialized; [DR1359]
   //
   // If we squint, this is guaranteed, since exactly one non-static data member
   // will be initialized (if the constructor isn't deleted), we just don't know
   // which one.
-  if (Ctor && ClassDecl->isUnion())
-    return CSM == Sema::CXXDefaultConstructor
-               ? ClassDecl->hasInClassInitializer() ||
-                     !ClassDecl->hasVariantMembers()
-               : true;
+  if (Ctor && ClassDecl->isUnion()) {
+    if (CSM == Sema::CXXDefaultConstructor &&
+        !(ClassDecl->hasInClassInitializer() ||
+          !ClassDecl->hasVariantMembers()))
+      return Fail(FailKind::Union);
+    else
+      return true;
+  }
 
   //   -- the class shall not have any virtual base classes;
   if (Ctor && ClassDecl->getNumVBases())
-    return false;
+    return Fail(FailKind::VirtualBase);
 
   // C++1y [class.copy]p26:
   //   -- [the class] is a literal type, and
   if (!Ctor && !ClassDecl->isLiteral())
-    return false;
+    return Fail(FailKind::NonLiteral);
 
   //   -- every constructor involved in initializing [...] base class
   //      sub-objects shall be a constexpr constructor;
   //   -- the assignment operator selected to copy/move each direct base
   //      class is a constexpr function, and
   for (const auto &B : ClassDecl->bases()) {
     const RecordType *BaseType = B.getType()->getAs<RecordType>();
     if (!BaseType) continue;
 
+    DiagLoc = B.getBeginLoc();
     CXXRecordDecl *BaseClassDecl = cast<CXXRecordDecl>(BaseType->getDecl());
     if (!specialMemberIsConstexpr(S, BaseClassDecl, CSM, 0, ConstArg,
-                                  InheritedCtor, Inherited))
-      return false;
+                                  InheritedCtor, Inherited, DiagNotes ? &SMLoc : nullptr))
+      return Fail(FailKind::Subobject, BaseClassDecl);
   }
 
   //   -- every constructor involved in initializing non-static data members
   //      [...] shall be a constexpr constructor;
   //   -- every non-static data member and base class sub-object shall be
   //      initialized
   //   -- for each non-static data member of X that is of class type (or array
   //      thereof), the assignment operator selected to copy/move that member is
   //      a constexpr function
   for (const auto *F : ClassDecl->fields()) {
     if (F->isInvalidDecl())
       continue;
     if (CSM == Sema::CXXDefaultConstructor && F->hasInClassInitializer())
       continue;
     QualType BaseType = S.Context.getBaseElementType(F->getType());
     if (const RecordType *RecordTy = BaseType->getAs<RecordType>()) {
       CXXRecordDecl *FieldRecDecl = cast<CXXRecordDecl>(RecordTy->getDecl());
       if (!specialMemberIsConstexpr(S, FieldRecDecl, CSM,
                                     BaseType.getCVRQualifiers(),
-                                    ConstArg && !F->isMutable()))
-        return false;
+                                    ConstArg && !F->isMutable(),
+                                    nullptr, nullptr, DiagNotes ? &SMLoc : nullptr))
+        return Fail(FailKind::Subobject, F);
     } else if (CSM == Sema::CXXDefaultConstructor) {
-      return false;
+      return Fail(FailKind::Subobject, F);
     }
   }
 
   // All OK, it's constexpr!
   return true;
 }
 
 static Sema::ImplicitExceptionSpecification
void Sema::CheckExplicitlyDefaultedSpecialMember(CXXMethodDecl *MD) {
   //   An explicitly-defaulted function may be declared constexpr only if it
   //   would have been implicitly declared as constexpr,
   // Do not apply this rule to members of class templates, since core issue 1358
   // makes such functions always instantiate to constexpr functions. For
   // functions which cannot be constexpr (for non-constructors in C++11 and for
   // destructors in C++1y), this is checked elsewhere.
   //
   // FIXME: This should not apply if the member is deleted.
-  bool Constexpr = defaultedSpecialMemberIsConstexpr(*this, RD, CSM,
-                                                     HasConstParam);
-  if ((getLangOpts().CPlusPlus14 ? !isa<CXXDestructorDecl>(MD)
+  bool InvalidIfNotConstexpr =
+      ((getLangOpts().CPlusPlus14 ? !isa<CXXDestructorDecl>(MD)
                                   : isa<CXXConstructorDecl>(MD)) &&
-      MD->isConstexpr() && !Constexpr &&
-      MD->getTemplatedKind() == FunctionDecl::TK_NonTemplate) {
-    Diag(MD->getBeginLoc(), diag::err_incorrect_defaulted_constexpr) << CSM;
-    // FIXME: Explain why the special member can't be constexpr.
+       MD->isConstexpr() &&
+       MD->getTemplatedKind() == FunctionDecl::TK_NonTemplate);
+  llvm::SmallVector<PartialDiagnosticAt, 8> DiagNotes;
+  bool Constexpr = defaultedSpecialMemberIsConstexpr(
+      *this, RD, CSM, HasConstParam, /*InheritedCtor=*/nullptr,
+      /*Inherited=*/nullptr,  InvalidIfNotConstexpr ? &DiagNotes : nullptr);
+  if (InvalidIfNotConstexpr && !Constexpr) {
+    Diag(MD->getBeginLoc(), diag::err_incorrect_defaulted_constexpr)
+        << CSM << (DiagNotes.size() != 0);
+    for (auto Note : DiagNotes)
+      Diag(Note.first, Note.second);
     HadError = true;
   }
 
   if (First) {
     // C++2a [dcl.fct.def.default]p3:
     //   If a function is explicitly defaulted on its first declaration, it is
     //   implicitly considered to be constexpr if the implicit declaration
     //   would be.