[Concepts] Add an error for unsupported P0848 (destructor overloading) code

[royjacobson]

Summary

We have received numerous (5 already) bug reports over P0848 not being implemented for destructors. We currently allow multiple destructor declarations but we will always select the first one, which might unintendedly compile with the wrong destructor.

Until we get P0848 working, this patch suggests adding diagnostic for classes that try to overload destructors with constraints so users won't be confused when their (legal) code doesn't work.

I would also like backport this to 14: anything that accidentally depends on the current behavior is already broken anyway.

(Will add release notes if accepted)

[royjacobson]

A working version.

[royjacobson]

I've thought about this a bit more and I want to move the diagnostic into SemaTemplateInstantiateDecl or somewhere close. It makes more sense because it's closer to where P0848 needs to be implemented and it will spam less errors.

[royjacobson]

Move diagnostic into Sema, make it fire once for every class template definitions.

[royjacobson]

Fix conventions

[royjacobson]

I continued hacking a bit at this and I think I got a working approach to implementing P0848 (down to 6 failing tests, will probably post at least a draft tomorrow). So I'll close for now. Maybe it's still useful for backporting if someone wants to. Sorry for all the mail spam from today...

clang/include/clang/Basic/DiagnosticSemaKinds.td

def note_ambiguous_atomic_constraints : Note<
   "similar constraint expressions not considered equivalent; constraint "
   "expressions cannot be considered equivalent unless they originate from the "
   "same concept">;
 def note_ambiguous_atomic_constraints_similar_expression : Note<
   "similar constraint expression here">;
 def err_unsupported_placeholder_constraint : Error<
   "constrained placeholder types other than simple 'auto' on non-type template "
   "parameters not supported yet">;
+def err_unsupported_destructor_overloading : Error<
+  "overloading destructors is not supported yet in this version. Consult "
+  "'https://github.com/llvm/llvm-project/issues/45614' for updates">;
+def note_unsupported_destructor_overloading_declaration : Note<
+  "destructor declared here.">;
 
 def err_template_different_requires_clause : Error<
   "requires clause differs in template redeclaration">;
 def err_template_different_type_constraint : Error<
   "type constraint differs in template redeclaration">;
 def err_template_template_parameter_not_at_least_as_constrained : Error<
   "template template argument %0 is more constrained than template template "
   "parameter %1">;

clang/lib/Sema/SemaDeclCXX.cpp

if (Report(O)) {
         IssuedDiagnostic = true;
       }
       S.Diag(O->getLocation(), diag::note_overridden_virtual_function);
     }
   }
   return IssuedDiagnostic;
 }
 
+namespace {
+
+/// We don't support C++20 constrained destructors yet, and we are unlikely to
+/// do so in the near future. Until we do, we check here for multiple
+/// declarations and report an error. We have to handle correctly the case of
+/// merged declarations in modules and the case of invalid templated
+/// destructors so this is a bit involved.
+/// We also don't emit errors on OpenCL code because OpenCL destructors can
+/// overload on address space (https://reviews.llvm.org/D64569).
+void DiagnoseUnsupportedDestructorOverloading(Sema& S, const CXXRecordDecl* Record) {
+  ASTContext &Context = Record->getASTContext();
+  QualType ClassType = Context.getTypeDeclType(Record);
+
+  DeclarationName Name = Context.DeclarationNames.getCXXDestructorName(
+      Context.getCanonicalType(ClassType));
+
+  DeclContext::lookup_result R = Record->lookup(Name);
+
+  if (!(R.empty() || R.isSingleResult()) && !Context.getLangOpts().OpenCL) {
+    bool HasNonTrivialOverloads = false;
+    auto FirstDecl = R.front();
+    for (const auto &Decl : R) {
+      if (!Decl->isTemplateDecl() && !FirstDecl->isTemplateDecl() &&
+          !Context.isSameEntity(Decl, FirstDecl)) {
+        HasNonTrivialOverloads = true;
+      }
+    }
+    if (HasNonTrivialOverloads) {
+      S.Diag(Record->getLocation(),
+             diag::err_unsupported_destructor_overloading);
+      for (const auto &Decl : R) {
+        S.Diag(Decl->getLocation(),
+               diag::note_unsupported_destructor_overloading_declaration);
+      }
+    }
+  }
+}
+} // namespace
+
 /// Perform semantic checks on a class definition that has been
 /// completing, introducing implicitly-declared members, checking for
 /// abstract types, etc.
 ///
 /// \param S The scope in which the class was parsed. Null if we didn't just
 ///        parse a class definition.
 /// \param Record The completed class.
 void Sema::CheckCompletedCXXClass(Scope *S, CXXRecordDecl *Record) {
auto CompleteMemberFunction = [&](CXXMethodDecl *M) {
     // FIXME: We can defer doing this until the vtable is marked as used.
     if (M->isDefaulted() && M->isConstexpr() && M->size_overridden_methods())
       DefineDefaultedFunction(*this, M, M->getLocation());
 
     if (!Incomplete)
       CheckCompletedMemberFunction(M);
   };
 
+  if (!inTemplateInstantiation())
+    DiagnoseUnsupportedDestructorOverloading(*this, Record);
+
   // Check the destructor before any other member function. We need to
   // determine whether it's trivial in order to determine whether the claas
   // type is a literal type, which is a prerequisite for determining whether
   // other special member functions are valid and whether they're implicitly
   // 'constexpr'.
   if (CXXDestructorDecl *Dtor = Record->getDestructor())
     CompleteMemberFunction(Dtor);
 

clang/test/CXX/over/over.match/over.match.viable/p3.cpp

 template<typename>
 struct WrapsStatics {
   static void foo(int) requires false;
   static void foo(A) requires true;
 };
 
 template<typename T>
 struct S {
+  // expected-error@-1  {{overloading destructors is not supported}}
   void foo(int) requires false;
   void foo(A) requires true;
   S(A) requires false;
   S(double) requires true;
   ~S() requires false;
   // expected-note@-1 2{{because 'false' evaluated to false}}
+  // expected-note@-2 {{destructor declared here}}
   ~S() requires true;
+  // expected-note@-1 {{destructor declared here}}
   operator int() requires true;
   operator int() requires false;
 };
 
 void bar() {
+  // Note - we have a hard error in this case until P0848 is implemented.
   WrapsStatics<int>::foo(A{});
   S<int>{1.}.foo(A{});
   // expected-error@-1{{invalid reference to function '~S': constraints not satisfied}}
-  // Note - this behavior w.r.t. constrained dtors is a consequence of current
-  // wording, which does not invoke overload resolution when a dtor is called.
-  // P0848 is set to address this issue.
+
   S<int> s = 1;
   // expected-error@-1{{invalid reference to function '~S': constraints not satisfied}}
   int a = s;
 }