Defer checking for mismatches between the deletedness of and overriding

function and an overridden function until we know whether the overriding
function is deleted.

We previously did these checks when we first built the declaration,
which was too soon in some cases. We now defer all these checks to the
end of the class.

Also add missing check that a consteval function cannot override a
non-consteval function and vice versa.

Author: zygoloid

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -1509,9 +1509,12 @@ def err_deleted_decl_not_first : Error<
 
 def err_deleted_override : Error<
   "deleted function %0 cannot override a non-deleted function">;
-
 def err_non_deleted_override : Error<
   "non-deleted function %0 cannot override a deleted function">;
+def err_consteval_override : Error<
+  "consteval function %0 cannot override a non-consteval function">;
+def err_non_consteval_override : Error<
+  "non-consteval function %0 cannot override a consteval function">;
 
 def warn_weak_vtable : Warning<
   "%0 has no out-of-line virtual method definitions; its vtable will be "

clang/lib/Sema/SemaDecl.cpp

@@ -8032,30 +8032,8 @@ struct FindOverriddenMethod {
     return false;
   }
 };
-
-enum OverrideErrorKind { OEK_All, OEK_NonDeleted, OEK_Deleted };
 } // end anonymous namespace
 
-/// Report an error regarding overriding, along with any relevant
-/// overridden methods.
-///
-/// \param DiagID the primary error to report.
-/// \param MD the overriding method.
-/// \param OEK which overrides to include as notes.
-static void ReportOverrides(Sema& S, unsigned DiagID, const CXXMethodDecl *MD,
-                            OverrideErrorKind OEK = OEK_All) {
-  S.Diag(MD->getLocation(), DiagID) << MD->getDeclName();
-  for (const CXXMethodDecl *O : MD->overridden_methods()) {
-    // This check (& the OEK parameter) could be replaced by a predicate, but
-    // without lambdas that would be overkill. This is still nicer than writing
-    // out the diag loop 3 times.
-    if ((OEK == OEK_All) ||
-        (OEK == OEK_NonDeleted && !O->isDeleted()) ||
-        (OEK == OEK_Deleted && O->isDeleted()))
-      S.Diag(O->getLocation(), diag::note_overridden_virtual_function);
-  }
-}
-
 /// AddOverriddenMethods - See if a method overrides any in the base classes,
 /// and if so, check that it's a valid override and remember it.
 bool Sema::AddOverriddenMethods(CXXRecordDecl *DC, CXXMethodDecl *MD) {
@@ -8064,8 +8042,6 @@ bool Sema::AddOverriddenMethods(CXXRecordDecl *DC, CXXMethodDecl *MD) {
   FindOverriddenMethod FOM;
   FOM.Method = MD;
   FOM.S = this;
-  bool hasDeletedOverridenMethods = false;
-  bool hasNonDeletedOverridenMethods = false;
   bool AddedAny = false;
   if (DC->lookupInBases(FOM, Paths)) {
     for (auto *I : Paths.found_decls()) {
@@ -8075,21 +8051,12 @@ bool Sema::AddOverriddenMethods(CXXRecordDecl *DC, CXXMethodDecl *MD) {
             !CheckOverridingFunctionAttributes(MD, OldMD) &&
             !CheckOverridingFunctionExceptionSpec(MD, OldMD) &&
             !CheckIfOverriddenFunctionIsMarkedFinal(MD, OldMD)) {
-          hasDeletedOverridenMethods |= OldMD->isDeleted();
-          hasNonDeletedOverridenMethods |= !OldMD->isDeleted();
           AddedAny = true;
         }
       }
     }
   }
 
-  if (hasDeletedOverridenMethods && !MD->isDeleted()) {
-    ReportOverrides(*this, diag::err_non_deleted_override, MD, OEK_Deleted);
-  }
-  if (hasNonDeletedOverridenMethods && MD->isDeleted()) {
-    ReportOverrides(*this, diag::err_deleted_override, MD, OEK_NonDeleted);
-  }
-
   return AddedAny;
 }
 
@@ -9095,8 +9062,7 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
     }
 
     // If a function is defined as defaulted or deleted, mark it as such now.
-    // FIXME: Does this ever happen? ActOnStartOfFunctionDef forces the function
-    // definition kind to FDK_Definition.
+    // We'll do the relevant checks on defaulted / deleted functions later.
     switch (D.getFunctionDefinitionKind()) {
       case FDK_Declaration:
       case FDK_Definition:
@@ -10745,12 +10711,7 @@ bool Sema::CheckFunctionDeclaration(Scope *S, FunctionDecl *NewFD,
       if (!Method->isFunctionTemplateSpecialization() &&
           !Method->getDescribedFunctionTemplate() &&
           Method->isCanonicalDecl()) {
-        if (AddOverriddenMethods(Method->getParent(), Method)) {
-          // If the function was marked as "static", we have a problem.
-          if (NewFD->getStorageClass() == SC_Static) {
-            ReportOverrides(*this, diag::err_static_overrides_virtual, Method);
-          }
-        }
+        AddOverriddenMethods(Method->getParent(), Method);
       }
       if (Method->isVirtual() && NewFD->getTrailingRequiresClause())
         // C++2a [class.virtual]p6

clang/lib/Sema/SemaDeclCXX.cpp

@@ -6172,27 +6172,31 @@ Sema::getDefaultedFunctionKind(const FunctionDecl *FD) {
   return DefaultedFunctionKind();
 }
 
-static void DefineImplicitSpecialMember(Sema &S, CXXMethodDecl *MD,
-                                        SourceLocation DefaultLoc) {
-  switch (S.getSpecialMember(MD)) {
+static void DefineDefaultedFunction(Sema &S, FunctionDecl *FD,
+                                    SourceLocation DefaultLoc) {
+  Sema::DefaultedFunctionKind DFK = S.getDefaultedFunctionKind(FD);
+  if (DFK.isComparison())
+    return S.DefineDefaultedComparison(DefaultLoc, FD, DFK.asComparison());
+
+  switch (DFK.asSpecialMember()) {
   case Sema::CXXDefaultConstructor:
     S.DefineImplicitDefaultConstructor(DefaultLoc,
-                                       cast<CXXConstructorDecl>(MD));
+                                       cast<CXXConstructorDecl>(FD));
     break;
   case Sema::CXXCopyConstructor:
-    S.DefineImplicitCopyConstructor(DefaultLoc, cast<CXXConstructorDecl>(MD));
+    S.DefineImplicitCopyConstructor(DefaultLoc, cast<CXXConstructorDecl>(FD));
     break;
   case Sema::CXXCopyAssignment:
-    S.DefineImplicitCopyAssignment(DefaultLoc, MD);
+    S.DefineImplicitCopyAssignment(DefaultLoc, cast<CXXMethodDecl>(FD));
     break;
   case Sema::CXXDestructor:
-    S.DefineImplicitDestructor(DefaultLoc, cast<CXXDestructorDecl>(MD));
+    S.DefineImplicitDestructor(DefaultLoc, cast<CXXDestructorDecl>(FD));
     break;
   case Sema::CXXMoveConstructor:
-    S.DefineImplicitMoveConstructor(DefaultLoc, cast<CXXConstructorDecl>(MD));
+    S.DefineImplicitMoveConstructor(DefaultLoc, cast<CXXConstructorDecl>(FD));
     break;
   case Sema::CXXMoveAssignment:
-    S.DefineImplicitMoveAssignment(DefaultLoc, MD);
+    S.DefineImplicitMoveAssignment(DefaultLoc, cast<CXXMethodDecl>(FD));
     break;
   case Sema::CXXInvalid:
     llvm_unreachable("Invalid special member.");
@@ -6313,6 +6317,28 @@ static bool canPassInRegisters(Sema &S, CXXRecordDecl *D,
   return HasNonDeletedCopyOrMove;
 }
 
+/// Report an error regarding overriding, along with any relevant
+/// overridden methods.
+///
+/// \param DiagID the primary error to report.
+/// \param MD the overriding method.
+/// \param OEK which overrides to include as notes.
+static bool
+ReportOverrides(Sema &S, unsigned DiagID, const CXXMethodDecl *MD,
+                llvm::function_ref<bool(const CXXMethodDecl *)> Report) {
+  bool IssuedDiagnostic = false;
+  for (const CXXMethodDecl *O : MD->overridden_methods()) {
+    if (Report(O)) {
+      if (!IssuedDiagnostic) {
+        S.Diag(MD->getLocation(), DiagID) << MD->getDeclName();
+        IssuedDiagnostic = true;
+      }
+      S.Diag(O->getLocation(), diag::note_overridden_virtual_function);
+    }
+  }
+  return IssuedDiagnostic;
+}
+
 /// Perform semantic checks on a class definition that has been
 /// completing, introducing implicitly-declared members, checking for
 /// abstract types, etc.
@@ -6427,21 +6453,64 @@ void Sema::CheckCompletedCXXClass(Scope *S, CXXRecordDecl *Record) {
   // primary comparison functions (==, <=>).
   llvm::SmallVector<FunctionDecl*, 5> DefaultedSecondaryComparisons;
 
-  auto CheckForDefaultedFunction = [&](FunctionDecl *FD) {
-    if (!FD || FD->isInvalidDecl() || !FD->isExplicitlyDefaulted())
+  // Perform checks that can't be done until we know all the properties of a
+  // member function (whether it's defaulted, deleted, virtual, overriding,
+  // ...).
+  auto CheckCompletedMemberFunction = [&](CXXMethodDecl *MD) {
+    // A static function cannot override anything.
+    if (MD->getStorageClass() == SC_Static) {
+      if (ReportOverrides(*this, diag::err_static_overrides_virtual, MD,
+                          [](const CXXMethodDecl *) { return true; }))
+        return;
+    }
+
+    // A deleted function cannot override a non-deleted function and vice
+    // versa.
+    if (ReportOverrides(*this,
+                        MD->isDeleted() ? diag::err_deleted_override
+                                        : diag::err_non_deleted_override,
+                        MD, [&](const CXXMethodDecl *V) {
+                          return MD->isDeleted() != V->isDeleted();
+                        })) {
+      if (MD->isDefaulted() && MD->isDeleted())
+        // Explain why this defaulted function was deleted.
+        DiagnoseDeletedDefaultedFunction(MD);
       return;
+    }
+
+    // A consteval function cannot override a non-consteval function and vice
+    // versa.
+    if (ReportOverrides(*this,
+                        MD->isConsteval() ? diag::err_consteval_override
+                                          : diag::err_non_consteval_override,
+                        MD, [&](const CXXMethodDecl *V) {
+                          return MD->isConsteval() != V->isConsteval();
+                        })) {
+      if (MD->isDefaulted() && MD->isDeleted())
+        // Explain why this defaulted function was deleted.
+        DiagnoseDeletedDefaultedFunction(MD);
+      return;
+    }
+  };
+
+  auto CheckForDefaultedFunction = [&](FunctionDecl *FD) -> bool {
+    if (!FD || FD->isInvalidDecl() || !FD->isExplicitlyDefaulted())
+      return false;
 
     DefaultedFunctionKind DFK = getDefaultedFunctionKind(FD);
     if (DFK.asComparison() == DefaultedComparisonKind::NotEqual ||
-        DFK.asComparison() == DefaultedComparisonKind::Relational)
+        DFK.asComparison() == DefaultedComparisonKind::Relational) {
       DefaultedSecondaryComparisons.push_back(FD);
-    else
-      CheckExplicitlyDefaultedFunction(S, FD);
+      return true;
+    }
+
+    CheckExplicitlyDefaultedFunction(S, FD);
+    return false;
   };
 
   auto CompleteMemberFunction = [&](CXXMethodDecl *M) {
     // Check whether the explicitly-defaulted members are valid.
-    CheckForDefaultedFunction(M);
+    bool Incomplete = CheckForDefaultedFunction(M);
 
     // Skip the rest of the checks for a member of a dependent class.
     if (Record->isDependentType())
@@ -6488,7 +6557,10 @@ void Sema::CheckCompletedCXXClass(Scope *S, CXXRecordDecl *Record) {
     // function right away.
     // FIXME: We can defer doing this until the vtable is marked as used.
     if (M->isDefaulted() && M->isConstexpr() && M->size_overridden_methods())
-      DefineImplicitSpecialMember(*this, M, M->getLocation());
+      DefineDefaultedFunction(*this, M, M->getLocation());
+
+    if (!Incomplete)
+      CheckCompletedMemberFunction(M);
   };
 
   // Check the destructor before any other member function. We need to
@@ -6534,9 +6606,14 @@ void Sema::CheckCompletedCXXClass(Scope *S, CXXRecordDecl *Record) {
   }
 
   // Check the defaulted secondary comparisons after any other member functions.
-  for (FunctionDecl *FD : DefaultedSecondaryComparisons)
+  for (FunctionDecl *FD : DefaultedSecondaryComparisons) {
     CheckExplicitlyDefaultedFunction(S, FD);
 
+    // If this is a member function, we deferred checking it until now.
+    if (auto *MD = dyn_cast<CXXMethodDecl>(FD))
+      CheckCompletedMemberFunction(MD);
+  }
+
   // ms_struct is a request to use the same ABI rules as MSVC.  Check
   // whether this class uses any C++ features that are implemented
   // completely differently in MSVC, and if so, emit a diagnostic.
@@ -13023,7 +13100,7 @@ void Sema::ActOnFinishCXXNonNestedClass() {
     SmallVector<CXXMethodDecl*, 4> WorkList;
     std::swap(DelayedDllExportMemberFunctions, WorkList);
     for (CXXMethodDecl *M : WorkList) {
-      DefineImplicitSpecialMember(*this, M, M->getLocation());
+      DefineDefaultedFunction(*this, M, M->getLocation());
 
       // Pass the method to the consumer to get emitted. This is not necessary
       // for explicit instantiation definitions, as they will get emitted
@@ -16368,9 +16445,6 @@ void Sema::SetDeclDeleted(Decl *Dcl, SourceLocation DelLoc) {
     Fn->setInvalidDecl();
   }
 
-  if (Fn->isDeleted())
-    return;
-
   // C++11 [basic.start.main]p3:
   //   A program that defines main as deleted [...] is ill-formed.
   if (Fn->isMain())
@@ -16380,25 +16454,6 @@ void Sema::SetDeclDeleted(Decl *Dcl, SourceLocation DelLoc) {
   //  A deleted function is implicitly inline.
   Fn->setImplicitlyInline();
   Fn->setDeletedAsWritten();
-
-  // See if we're deleting a function which is already known to override a
-  // non-deleted virtual function.
-  if (CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(Fn)) {
-    bool IssuedDiagnostic = false;
-    for (const CXXMethodDecl *O : MD->overridden_methods()) {
-      if (!(*MD->begin_overridden_methods())->isDeleted()) {
-        if (!IssuedDiagnostic) {
-          Diag(DelLoc, diag::err_deleted_override) << MD->getDeclName();
-          IssuedDiagnostic = true;
-        }
-        Diag(O->getLocation(), diag::note_overridden_virtual_function);
-      }
-    }
-    // If this function was implicitly deleted because it was defaulted,
-    // explain why it was deleted.
-    if (IssuedDiagnostic && MD->isDefaulted())
-      DiagnoseDeletedDefaultedFunction(MD);
-  }
 }
 
 void Sema::SetDeclDefaulted(Decl *Dcl, SourceLocation DefaultLoc) {
@@ -16480,10 +16535,12 @@ void Sema::SetDeclDefaulted(Decl *Dcl, SourceLocation DefaultLoc) {
   if (Primary->getCanonicalDecl()->isDefaulted())
     return;
 
+  // FIXME: Once we support defining comparisons out of class, check for a
+  // defaulted comparison here.
   if (CheckExplicitlyDefaultedSpecialMember(MD, DefKind.asSpecialMember()))
     MD->setInvalidDecl();
   else
-    DefineImplicitSpecialMember(*this, MD, DefaultLoc);
+    DefineDefaultedFunction(*this, MD, DefaultLoc);
 }
 
 static void SearchForReturnInStmt(Sema &Self, Stmt *S) {

clang/test/CXX/class.derived/class.abstract/p16.cpp

@@ -35,15 +35,13 @@ struct E : D {};
 // expected-error@-1 {{deleted function '~E' cannot override a non-deleted function}}
 // expected-note@-2 {{destructor of 'E' is implicitly deleted because base class 'D' has an inaccessible destructor}}
 // expected-error@-3 {{deleted function 'operator=' cannot override a non-deleted function}}
-// expected-note@-4 {{while declaring the implicit copy assignment operator for 'E'}}
-// expected-note@-5 {{copy assignment operator of 'E' is implicitly deleted because base class 'D' has an inaccessible copy assignment operator}}
+// expected-note@-4 {{copy assignment operator of 'E' is implicitly deleted because base class 'D' has an inaccessible copy assignment operator}}
 struct F : D {};
 struct G : D {};
 // expected-error@-1 {{deleted function '~G' cannot override a non-deleted function}}
 // expected-note@-2 {{destructor of 'G' is implicitly deleted because base class 'D' has an inaccessible destructor}}
 // expected-error@-3 {{deleted function 'operator=' cannot override a non-deleted function}}
-// expected-note@-4 {{while declaring the implicit move assignment operator for 'G'}}
-// expected-note@-5 {{move assignment operator of 'G' is implicitly deleted because base class 'D' has an inaccessible move assignment operator}}
+// expected-note@-4 {{move assignment operator of 'G' is implicitly deleted because base class 'D' has an inaccessible move assignment operator}}
 struct H : D { // expected-note {{deleted because base class 'D' has an inaccessible move assignment}}
   H &operator=(H&&) = default; // expected-warning {{implicitly deleted}}
   // expected-error@-1 {{deleted function 'operator=' cannot override a non-deleted function}}

clang/test/CXX/special/class.dtor/p5-0x.cpp

@@ -86,9 +86,9 @@ struct C4 : virtual InaccessibleDtor { C4(); } c4; // expected-error {{deleted f
 // -- for a virtual destructor, lookup of the non-array deallocation function
 // results in an ambiguity or a function that is deleted or inaccessible.
 class D1 {
-  void operator delete(void*);
+  void operator delete(void*); // expected-note {{here}}
 public:
-  virtual ~D1() = default; // expected-note {{here}}
+  virtual ~D1() = default; // expected-note 2{{here}}
 } d1; // ok
 struct D2 : D1 { // expected-note 2{{virtual destructor requires an unambiguous, accessible 'operator delete'}} \
                  // expected-error {{deleted function '~D2' cannot override a non-deleted}}
@@ -103,3 +103,10 @@ struct D4 { // expected-note {{virtual destructor requires an unambiguous, acces
   virtual ~D4() = default; // expected-note {{implicitly deleted here}}
   void operator delete(void*) = delete;
 } d4; // expected-error {{deleted function}}
+struct D5 : D1 {
+  ~D5(); // ok (but not definable)
+} d5;
+D5::~D5() {} // expected-error {{'operator delete' is a private member of 'D1'}}
+struct D6 : D1 {
+  ~D6() = delete; // expected-error {{deleted function '~D6' cannot override a non-deleted}}
+};

clang/test/SemaCXX/PR9572.cpp

@@ -16,8 +16,7 @@ struct Foo : public Base {
 // expected-error@-2 {{base class 'Base' has private destructor}}
 #else
 // expected-error@-4 {{deleted function '~Foo' cannot override a non-deleted function}}
-// expected-note@-5 {{overridden virtual function is here}}
-// expected-note@-6 3 {{destructor of 'Foo' is implicitly deleted because base class 'Base' has an inaccessible destructor}}
+// expected-note@-5 3{{destructor of 'Foo' is implicitly deleted because base class 'Base' has an inaccessible destructor}}
 #endif
 
   const int kBlah = 3;
@@ -29,10 +28,6 @@ struct Foo : public Base {
 };
 
 struct Bar : public Foo {
-#if __cplusplus >= 201103L
-// expected-error@-2 {{non-deleted function '~Bar' cannot override a deleted function}}
-// expected-note@-3 {{while declaring the implicit destructor for 'Bar'}}
-#endif
   Bar() { }
 #if __cplusplus <= 199711L
   // expected-note@-2 {{implicit destructor for 'Foo' first required here}}

clang/test/SemaCXX/cxx0x-cursory-default-delete.cpp

@@ -211,3 +211,9 @@ struct DeletedMemberTemplateTooLate {
 };
 template<typename T> void DeletedMemberTemplateTooLate::f() = delete; // expected-error {{must be first decl}} expected-note {{substitution failure}}
 void use_member_template_deleted_too_late() { DeletedMemberTemplateTooLate::f<int>(); } // expected-error {{no matching function}}
+
+namespace deleted_overrides_deleted {
+  struct A { virtual void f() = delete; };
+  template<typename T> struct B : A { virtual void f() = delete; };
+  template struct B<int>;
+}