[Concepts] Implement overload resolution for destructors (P0848)

[royjacobson]

Summary

This patch implements a necessary part of P0848, the overload resolution for destructors.
It is now possible to overload destructors based on constraints, and the eligible destructor
will be selected at the end of the class.

The approach this patch takes is to perform the overload resolution in Sema::ActOnFields
and to mark the selected destructor using a new property in FunctionDeclBitfields.

CXXRecordDecl::getDestructor is then modified to use this property to return the correct
destructor.

This closes https://github.com/llvm/llvm-project/issues/45614.

[royjacobson]

Fix typo

[erichkeane]

How much of P0848 is missing after this? If nothing/not much, should we update cxx_status.html as well?

clang/lib/Sema/SemaTemplateInstantiate.cpp

2330 ↗

(On Diff #431333)

I don't think this ends up being acceptable (removing them from the AST). Instead, we should probably mark them as "invalid" and update getDestructor to only return the 'only' destructor, or the first not-invalid one.

[cor3ntin]

In D126194#3531267, @erichkeane wrote:

How much of P0848 is missing after this? If nothing/not much, should we update cxx_status.html as well?

P0848 applies to all special member function. At best we could mark it partial but most of the work still need to be done.
I gave a shot to P0848 a few months ago, but my assesment is that clang might have to significantly refactor of special member functions to do that cleanly

clang/lib/Sema/SemaTemplateInstantiate.cpp

2330 ↗

(On Diff #431333)

I'd rather we stay consistent with the wording, keep track of a selected destructor which would be returned by getDestructor. it's more surgery but it's a lot cleaner imo

[royjacobson]

In D126194#3531280, @cor3ntin wrote:

In D126194#3531267, @erichkeane wrote:

How much of P0848 is missing after this? If nothing/not much, should we update cxx_status.html as well?

P0848 applies to all special member function. At best we could mark it partial but most of the work still need to be done.
I gave a shot to P0848 a few months ago, but my assesment is that clang might have to significantly refactor of special member functions to do that cleanly

Corentin, are there other places like getDestructor where we need the constraints Sema information in the AST? I hoped the other special member functions go through LookupSpecialMember which does the needed overload resolution.

So as far as I understand the code base, the P0848 part that remains is computing the SMFs that are 'eligible' and update the type traits (trivial/trivially copyable) accordingly. Currently we mark those inside CXXRecordDecl::addedMember, so we'll probably have to override them. I also don't understand how exactly the eligibility checks interact with the deferred concept checking.

BTW, this also interacts funnily with other type traits. For example, this is apparently legal

#include <type_traits>
template<int N>
struct A {
  A& operator=(A&&) requires true;
  virtual A& operator=(A&&);
};
static_assert(!std::is_aggregate_v<A<1>>);

So ineligible SMFs are ineligible only for the purpose of [copy]triviality, and can still have other visible effects!

About the status page - we're going to break ABI when we implement the type traits change so I don't think we should update it yet.

clang/lib/Sema/SemaTemplateInstantiate.cpp

2330 ↗

(On Diff #431333)

Corentin, do you suggest doing this in CXXRecordDecl explicitly?

Erich - I agree, this seems like a better solution, although this is a bit of abuse to 'invalid' in the case of less-constrained candidates. Ideally we might want another AST property of 'eligible' and keep track of things there, but I don't really know the AST code well enough to do it.

[erichkeane]

clang/lib/Sema/SemaTemplateInstantiate.cpp

2330 ↗

(On Diff #431333)

I like the idea of marking them eligible that way. If this JUST has to do with Destructors for now, adding a bit to CXXDestructorDecl is a pretty trivial thing to do. I'm pretty sure it is defined in DeclCXX.h. At that point, we just need to make sure it is checked during 'getDestructor' (or at least, around any calls to that).

[royjacobson]

clang/lib/Sema/SemaTemplateInstantiate.cpp

2330 ↗

(On Diff #431333)

I think we'll need it soon for the other SMF as well. (Or maybe even for all member functions if CWG2421 is ever resolved).

So maybe I should just take some time to look at FunctionDeclBitfields and see if I can update it. (I hope updating the ast printers isn't too hard)

[erichkeane]

clang/lib/Sema/SemaTemplateInstantiate.cpp

2330 ↗

(On Diff #431333)

I'm pretty sure there is room in there. Updating the printers should be pretty easy as well.

[royjacobson]

Update the approach to use an AST property, and enable this for all language variants (not just CPP20).

There's still one test failing because OpenCL have got their own destructor thing going, I'll try to
see what I can do about it.

[erichkeane]

clang/lib/AST/DeclCXX.cpp

1901

What cases happen when the lookup for a destructor name ends up not being a destructor on a RecordDecl type? I guess I could see this happening with a pseudo-destructor, but that isn't a class type.

clang/lib/Sema/SemaDecl.cpp

17841

How does all of this play with the 'defaulted destructor is constexpr' stuff? We end up storing facts like that, destructor triviality/irrelevance/defaulted-and-constexpr/deleted/etc as a bit in the Decl, rather than calculating it. Can this feature (Allowing overloading) cause those to be inaccurate?

That is, could we do something like use a requires clause to select between a trivial, irrelevant, and constexpr destructor? Do we need to make sure we update those too? I would expect that the destructor here would need to store its OWN trivaility/relevance/constexpr/etc here, so we can then update those flags on the type once it is selected.

[royjacobson]

clang/lib/AST/DeclCXX.cpp

1901

It's a bit silly, but it's because we can have template destructors that we added to the AST even though they're invalid (it ends up as a FunctionTemplateDecl).

I noticed it because of SemaTemplate/destructor-template.cpp

clang/lib/Sema/SemaDecl.cpp

17841

You're right, I also found canPassInRegisters that will need to be adjusted a bit so it seems I need to write some codegen tests to make sure this gets the ABI correctly.

I won't have much time in the coming month, I hope to get to it by the start of July (but if someone wants to pick this up in the meantime feel free to do so).

[royjacobson]

Update the PR to now handle triviality, which turned out to require a different approach.
On the bright side it is now easier to see how to implemenet the rest of P0848.

Added an AST dump test that checks the bitfields of structs and some derived traits.

Also fixed the OpenCL test error - their destructor model is quite incompatible with clang
and already broken in some ways, so I tried my best to continue the current behaviour.

[erichkeane]

I probably need to spend more time on this at one point, but first look seemed fine to me. I think the approach is about right, and the solution is there.

@aaron.ballman is messing with the constexpr-ness of SM functions, so there is likely some collaboration that needs to be doen to make sure we get THIS right as well.

clang/docs/ReleaseNotes.rst

403

Do we have enough to update cxx_status.html?

clang/include/clang/AST/DeclBase.h

1603

Hrumph... its a shame to lose the bit here later. I wonder if at one point in a future patch we should figure out which of these flags can be moved to an 'allocated in Trailing-storage ONLY if required' type deal, like we just did with FunctionTypeBitFields...

clang/include/clang/AST/DeclCXX.h

1430 ↗

(On Diff #437656)

@aaron.ballman is messing around with addedMember recently... I wonder if there are OTHER decisions that need to be updated/moved here?

[royjacobson]

clang/docs/ReleaseNotes.rst

403

I think it's still partial enough that I don't want to communicate 'maybe it will work'.
I plan to finish the rest in time for Clang 15 so I prefer to just update it then :) But I don't feel very strongly about it either way.

clang/include/clang/AST/DeclCXX.h

1430 ↗

(On Diff #437656)

Do you know what papers/DRs he's working on? It seemed like most constexpr stuff is done per-function and not per-class.

I don't think there's something else that needs to change but it's definitely possible I missed something.. I went through addedMember and tried to remove everything destructor related basically.

[erichkeane]

clang/include/clang/AST/DeclCXX.h

1430 ↗

(On Diff #437656)

He's working on DR647, which does a lot of work with special member functions/constexpr/consteval constructors.

That said, I believe he said it isn't a problem, they are related, but not conflicting in a way that he had.

[aaron.ballman]

clang/include/clang/AST/DeclCXX.h

1430 ↗

(On Diff #437656)

I don't see anything here that should conflict with what I'm working on (this is mostly about destructors and I've not started touching those yet). I'm working specifically on implementing this bit: http://eel.is/c++draft/dcl.constexpr#7

[royjacobson]

Fix the AST test on windows and rebase on HEAD.

clang/docs/ReleaseNotes.rst

 
 - The mangling scheme for C++20 modules has incompatibly changed. The
   initial mangling was discovered not to be reversible, and the weak
   ownership design decision did not give the backwards compatibility
   that was hoped for. C++20 since added ``extern "C++"`` semantics
   that can be used for such compatibility. The demangler now demangles
   symbols with named module attachment.
 
+- As per "Conditionally Trivial Special Member Functions" (P0848), it is
+  now possible to overload destructors using concepts. Note that the
+  type traits support for trivial and trivially copyable types is not
+  yet implemented.
+
 C++2b Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
 
 - Implemented `P2128R6: Multidimensional subscript operator <https://wg21.link/P2128R6>`_.
 - Implemented `P0849R8: auto(x): decay-copy in the language <https://wg21.link/P0849R8>`_.
 - Implemented `P2242R3: Non-literal variables (and labels and gotos) in constexpr functions <https://wg21.link/P2242R3>`_.
 
 CUDA/HIP Language Changes in Clang

clang/include/clang/AST/Decl.h

public:
   bool isUserProvided() const {
     auto *DeclAsWritten = this;
     if (FunctionDecl *Pattern = getTemplateInstantiationPattern())
       DeclAsWritten = Pattern;
     return !(DeclAsWritten->isDeleted() ||
              DeclAsWritten->getCanonicalDecl()->isDefaulted());
   }
 
+  bool isEligibleOrSelected() const {
+    return FunctionDeclBits.IsEligibleOrSelected;
+  }
+  void setEligibleOrSelected(bool IE) {
+    FunctionDeclBits.IsEligibleOrSelected = IE;
+  }
+
   /// Whether falling off this function implicitly returns null/zero.
   /// If a more specific implicit return value is required, front-ends
   /// should synthesize the appropriate return statements.
   bool hasImplicitReturnZero() const {
     return FunctionDeclBits.HasImplicitReturnZero;
   }
 
   /// State that falling off this function implicitly returns null/zero.

clang/include/clang/AST/DeclBase.h

class FunctionDeclBitfields {
     /// This flag indicates whether this function is trivial for the purpose of
     /// calls. This is meaningful only when this function is a copy/move
     /// constructor or a destructor.
     uint64_t IsTrivialForCall : 1;
 
     uint64_t IsDefaulted : 1;
     uint64_t IsExplicitlyDefaulted : 1;
     uint64_t HasDefaultedFunctionInfo : 1;
+
+    /// For complete types, whether a member function is an eligible special
+    /// member function or a selected destructor. See [class.mem.special].
+    uint64_t IsEligibleOrSelected : 1;
+
     uint64_t HasImplicitReturnZero : 1;
     uint64_t IsLateTemplateParsed : 1;
 
     /// Kind of contexpr specifier as defined by ConstexprSpecKind.
     uint64_t ConstexprKind : 2;
     uint64_t InstantiationIsPending : 1;
 
     /// Indicates if the function uses __try.
class FunctionDeclBitfields {
     /// Store the ODRHash after first calculation.
     uint64_t HasODRHash : 1;
 
     /// Indicates if the function uses Floating Point Constrained Intrinsics
     uint64_t UsesFPIntrin : 1;
   };
 
   /// Number of non-inherited bits in FunctionDeclBitfields.
-  enum { NumFunctionDeclBits = 27 };
+  enum { NumFunctionDeclBits = 28 };
 
   /// Stores the bits used by CXXConstructorDecl. If modified
   /// NumCXXConstructorDeclBits and the accessor
   /// methods in CXXConstructorDecl should be updated appropriately.
   class CXXConstructorDeclBitfields {
     friend class CXXConstructorDecl;
     /// For the bits in DeclContextBitfields.
     uint64_t : NumDeclContextBits;
     /// For the bits in FunctionDeclBitfields.
     uint64_t : NumFunctionDeclBits;
 
-    /// 24 bits to fit in the remaining available space.
+    /// 23 bits to fit in the remaining available space.
     /// Note that this makes CXXConstructorDeclBitfields take
     /// exactly 64 bits and thus the width of NumCtorInitializers
     /// will need to be shrunk if some bit is added to NumDeclContextBitfields,
     /// NumFunctionDeclBitfields or CXXConstructorDeclBitfields.
-    uint64_t NumCtorInitializers : 21;
+    uint64_t NumCtorInitializers : 20;
     uint64_t IsInheritingConstructor : 1;
 
     /// Whether this constructor has a trail-allocated explicit specifier.
     uint64_t HasTrailingExplicitSpecifier : 1;
     /// If this constructor does't have a trail-allocated explicit specifier.
     /// Whether this constructor is explicit specified.
     uint64_t IsSimpleExplicit : 1;
   };

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_ovl_no_oper :
     Error<"type %0 does not provide a %select{subscript|call}1 operator">;
 def err_ovl_unresolvable : Error<
   "reference to %select{overloaded|multiversioned}1 function could not be "
   "resolved; did you mean to call it%select{| with no arguments}0?">;
 def err_bound_member_function : Error<
   "reference to non-static member function must be called"
   "%select{|; did you mean to call it with no arguments?}0">;
 def note_possible_target_of_call : Note<"possible target for call">;
+def err_no_viable_destructor : Error<
+  "no viable destructor found for class %0">;
+def err_ambiguous_destructor : Error<
+  "destructor of class %0 is ambiguous">;
 
 def err_ovl_no_viable_object_call : Error<
   "no matching function for call to object of type %0">;
 def err_ovl_ambiguous_object_call : Error<
   "call to object of type %0 is ambiguous">;
 def err_ovl_ambiguous_subscript_call : Error<
   "call to subscript operator of type %0 is ambiguous">;
 def err_ovl_deleted_object_call : Error<

clang/lib/AST/Decl.cpp

FunctionDecl::FunctionDecl(Kind DK, ASTContext &C, DeclContext *DC,
   FunctionDeclBits.HasInheritedPrototype = false;
   FunctionDeclBits.HasWrittenPrototype = true;
   FunctionDeclBits.IsDeleted = false;
   FunctionDeclBits.IsTrivial = false;
   FunctionDeclBits.IsTrivialForCall = false;
   FunctionDeclBits.IsDefaulted = false;
   FunctionDeclBits.IsExplicitlyDefaulted = false;
   FunctionDeclBits.HasDefaultedFunctionInfo = false;
+  FunctionDeclBits.IsEligibleOrSelected = false;
   FunctionDeclBits.HasImplicitReturnZero = false;
   FunctionDeclBits.IsLateTemplateParsed = false;
   FunctionDeclBits.ConstexprKind = static_cast<uint64_t>(ConstexprKind);
   FunctionDeclBits.InstantiationIsPending = false;
   FunctionDeclBits.UsesSEHTry = false;
   FunctionDeclBits.UsesFPIntrin = UsesFPIntrin;
   FunctionDeclBits.HasSkippedBody = false;
   FunctionDeclBits.WillHaveBody = false;

clang/lib/AST/DeclCXX.cpp

CXXDestructorDecl *CXXRecordDecl::getDestructor() const {
   QualType ClassType = Context.getTypeDeclType(this);
 
   DeclarationName Name
     = Context.DeclarationNames.getCXXDestructorName(
                                           Context.getCanonicalType(ClassType));
 
   DeclContext::lookup_result R = lookup(Name);
 
+  // If a destructor was marked as selected, return it. We don't always have
+  // a selected destructor: dependent types, unnamed structs.
+  for (auto* Decl : R) {
+    auto* DD = dyn_cast<CXXDestructorDecl>(Decl);
+    if (DD && DD->isEligibleOrSelected())
+      return DD;
+  }
+
   return R.empty() ? nullptr : dyn_cast<CXXDestructorDecl>(R.front());
 }
 
 static bool isDeclContextInNamespace(const DeclContext *DC) {
   while (!DC->isTranslationUnit()) {
     if (DC->isNamespace())
       return true;
     DC = DC->getParent();

clang/lib/Sema/SemaDecl.cpp

Ivar = ObjCIvarDecl::Create(Context, cast<ObjCContainerDecl>(CurContext),
                               Context.CharTy,
                               Context.getTrivialTypeSourceInfo(Context.CharTy,
                                                                DeclLoc),
                               ObjCIvarDecl::Private, BW,
                               true);
   AllIvarDecls.push_back(Ivar);
 }
 
+namespace {
+/// [class.dtor]p4:
+///   At the end of the definition of a class, overload resolution is
+///   performed among the prospective destructors declared in that class with
+///   an empty argument list to select the destructor for the class, also
+///   known as the selected destructor.
+///
+/// We do the overload resolution here, then mark the selected constructor in the AST.
+/// Later CXXRecordDecl::getDestructor() will return the selected constructor.
+void ComputeSelectedDestructor(Sema &S, CXXRecordDecl *Record) {
+  if (!Record->hasUserDeclaredDestructor())
+    return;
+
+  SourceLocation Loc = Record->getLocation();
+  OverloadCandidateSet OCS(Loc, OverloadCandidateSet::CSK_Normal);
+
+  for (auto *Decl : Record->decls()) {
+    if (auto *DD = dyn_cast<CXXDestructorDecl>(Decl)) {
+      if (DD->isInvalidDecl())
+        continue;
+      S.AddOverloadCandidate(DD, DeclAccessPair::make(DD, DD->getAccess()), {},
+                             OCS);
+    }
+  }
+  if (OCS.empty())
+    return;
+
+  OverloadCandidateSet::iterator Best;
+  unsigned Msg = 0;
+  OverloadCandidateDisplayKind DisplayKind;
+
+  switch (OCS.BestViableFunction(S, Loc, Best)) {
+  case OR_Success:
+  case OR_Deleted:
+    Best->Function->setEligibleOrSelected(true);
+    break;
+
+  case OR_Ambiguous:
+    Msg = diag::err_ambiguous_destructor;
+    DisplayKind = OCD_AmbiguousCandidates;
+    break;
+
+  case OR_No_Viable_Function:
+    Msg = diag::err_no_viable_destructor;
+    DisplayKind = OCD_AllCandidates;
+    break;
+  }
+
+  if (Msg) {
+    PartialDiagnostic Diag = S.PDiag(Msg) << Record;
+    OCS.NoteCandidates(PartialDiagnosticAt(Loc, Diag), S, DisplayKind, {});
+    Record->setInvalidDecl();
+  }
+}
+} // namespace
+
 void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, Decl *EnclosingDecl,
                        ArrayRef<Decl *> Fields, SourceLocation LBrac,
                        SourceLocation RBrac,
                        const ParsedAttributesView &Attrs) {
   assert(EnclosingDecl && "missing record or interface decl");
 
   // If this is an Objective-C @implementation or category and we have
   // new fields here we should reset the layout of the interface since
case Decl::ObjCImplementation:
         ResetObjCLayout(cast<ObjCImplementationDecl>(DC)->getClassInterface());
       break;
     }
   }
 
   RecordDecl *Record = dyn_cast<RecordDecl>(EnclosingDecl);
   CXXRecordDecl *CXXRecord = dyn_cast<CXXRecordDecl>(EnclosingDecl);
 
+  if (CXXRecord && !CXXRecord->isDependentType())
+    ComputeSelectedDestructor(*this, CXXRecord);
+
   // Start counting up the number of named members; make sure to include
   // members of anonymous structs and unions in the total.
   unsigned NumNamedMembers = 0;
   if (Record) {
     for (const auto *I : Record->decls()) {
       if (const auto *IFD = dyn_cast<IndirectFieldDecl>(I))
         if (IFD->getDeclName())
           ++NumNamedMembers;

clang/test/CXX/class/class.dtor/p4.cpp

+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template <int N>
+struct A {
+  ~A() = delete; // expected-note {{explicitly marked deleted}}
+  ~A() requires(N == 1) = delete; // expected-note {{explicitly marked deleted}}
+};
+
+template <int N>
+struct B {
+  ~B() requires(N == 1) = delete; // expected-note {{explicitly marked deleted}}
+  virtual ~B() = delete; // expected-note {{explicitly marked deleted}}
+};
+
+template<int N>
+concept CO1 = N == 1;
+
+template<int N>
+concept CO2 = N > 0;
+
+template <int N>
+struct C {
+  ~C() = delete; // expected-note {{explicitly marked deleted}}
+  ~C() requires(CO1<N>) = delete;
+  ~C() requires(CO1<N> && CO2<N>) = delete; // expected-note {{explicitly marked deleted}}
+};
+
+template <int N>
+struct D {
+  ~D() requires(N != 0) = delete; // expected-note {{explicitly marked deleted}}
+  // expected-note@-1 {{candidate function has been explicitly deleted}}
+  // expected-note@-2 {{candidate function not viable: constraints not satisfied}}
+  // expected-note@-3 {{evaluated to false}}
+  ~D() requires(N == 1) = delete;
+  // expected-note@-1 {{candidate function has been explicitly deleted}}
+  // expected-note@-2 {{candidate function not viable: constraints not satisfied}}
+  // expected-note@-3 {{evaluated to false}}
+};
+
+template <class T>
+concept Foo = requires (T t) {
+  { t.foo() };
+};
+
+template<int N>
+struct E {
+  void foo();
+  ~E();
+  ~E() requires Foo<E> = delete;  // expected-note {{explicitly marked deleted}}
+};
+
+template struct A<1>;
+template struct A<2>;
+template struct B<1>;
+template struct B<2>;
+template struct C<1>;
+template struct C<2>;
+template struct D<0>;  // expected-error {{no viable destructor found for class 'D<0>'}} expected-note {{in instantiation of template}}
+template struct D<1>;  // expected-error {{destructor of class 'D<1>' is ambiguous}} expected-note {{in instantiation of template}}
+template struct D<2>;
+template struct E<1>;
+
+int main() {
+  A<1> a1; // expected-error {{attempt to use a deleted function}}
+  A<2> a2; // expected-error {{attempt to use a deleted function}}
+  B<1> b1; // expected-error {{attempt to use a deleted function}}
+  B<2> b2; // expected-error {{attempt to use a deleted function}}
+  C<1> c1; // expected-error {{attempt to use a deleted function}}
+  C<2> c2; // expected-error {{attempt to use a deleted function}}
+  D<0> d0;
+  D<1> d1;
+  D<2> d2; // expected-error {{attempt to use a deleted function}}
+  E<1> e1; // expected-error {{attempt to use a deleted function}}
+}

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

 
 template<typename T>
 struct S {
   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}}
   ~S() requires true;
   operator int() requires true;
   operator int() requires false;
 };
 
 void bar() {
   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;
 }