[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

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

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

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

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

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

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 ↗

(On Diff #431589)

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 ↗

(On Diff #431589)

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 ↗

(On Diff #431589)

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 ↗

(On Diff #431589)

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 ↗

(On Diff #437656)

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/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/Sema/SemaTemplateInstantiate.cpp

TemplateTypeParmDecl *VisitAdjustedType(const AdjustedType *T) {
       return Visit(T->getOriginalType());
     }
 
     TemplateTypeParmDecl *VisitPackExpansionType(const PackExpansionType *T) {
       return Visit(T->getPattern());
     }
   };
 
+  /// [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.
+  ///
+  /// To achieve that, we remove all non-selected destructors from the AST,
+  /// which is a bit unusual. We can't let those declarations be in AST and rely
+  /// on LookupSpecialMember to return the correct declaration because a lot of
+  /// code relies on CXXRecordDecl::getDestructor() which assumes there can only
+  /// be one destructor for a class.
+  /// The reason that we actually have to insert those declarations in the first
+  /// place (instead of keeping them at the side and adding the selected one
+  /// later) is that this will change the order of declarations in the class
+  /// which determines the vtable order, so this would break ABI unless we
+  /// insert the destructor at the original location, at which point it is
+  /// easier to just remove the unselected ones.
+  LLVM_ATTRIBUTE_NOINLINE
+  // NOINLINE to keep the stack size of `InstantiateClass` small.
+  void ComputeSelectedDestructor(Sema &S, CXXRecordDecl *Instantiation) {
+
+    if (!Instantiation->hasUserDeclaredDestructor())
+      return;
+
+    // FIXME: Is this the correct location?
+    SourceLocation Loc = Instantiation->getLocation();
+    OverloadCandidateSet OCS(Loc, OverloadCandidateSet::CSK_Normal);
+
+    SmallVector<CXXDestructorDecl *, 4> ProspectiveDestructors;
+    for (auto *Decl : Instantiation->decls()) {
+      if (auto *DD = dyn_cast<CXXDestructorDecl>(Decl)) {
+        if (DD->isInvalidDecl())
+          continue;
+        ProspectiveDestructors.push_back(DD);
+        S.AddOverloadCandidate(DD, DeclAccessPair::make(DD, DD->getAccess()),
+                               {}, OCS);
+      }
+    }
+    assert(
+        !OCS.empty() &&
+        "No candidates found even though hasUserDeclaredDestructor is true.");
+
+    OverloadCandidateSet::iterator Best;
+    unsigned Msg = 0;
+    OverloadCandidateDisplayKind DisplayKind;
+
+    switch (OCS.BestViableFunction(S, Loc, Best)) {
+    case OR_Success:
+    case OR_Deleted:
+      for (auto *DD : ProspectiveDestructors) {
+        if (DD != Best->Function) {
+          Instantiation->removeDecl(DD);
+          // FIXME: We should maybe `delete DD` here, but it segfaults.
+        }
+      }
+      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) << Instantiation;
+      OCS.NoteCandidates(PartialDiagnosticAt(Loc, Diag), S, DisplayKind, {});
+      Instantiation->setInvalidDecl();
+    }
+  }
+
 } // namespace
 
 bool Sema::SubstTypeConstraint(
     TemplateTypeParmDecl *Inst, const TypeConstraint *TC,
     const MultiLevelTemplateArgumentList &TemplateArgs) {
   const ASTTemplateArgumentListInfo *TemplArgInfo =
       TC->getTemplateArgsAsWritten();
   TemplateArgumentListInfo InstArgs;
for (auto *Member : Pattern->decls()) {
     } else {
       // FIXME: Eventually, a NULL return will mean that one of the
       // instantiations was a semantic disaster, and we'll want to mark the
       // declaration invalid.
       // For now, we expect to skip some members that we can't yet handle.
     }
   }
 
+  if (getLangOpts().CPlusPlus20)
+    ComputeSelectedDestructor(*this, Instantiation);
+
   // Finish checking fields.
   ActOnFields(nullptr, Instantiation->getLocation(), Instantiation, Fields,
               SourceLocation(), SourceLocation(), ParsedAttributesView());
   CheckCompletedCXXClass(nullptr, Instantiation);
 
   // Default arguments are parsed, if not instantiated. We can go instantiate
   // default arg exprs for default constructors if necessary now. Unless we're
   // parsing a class, in which case wait until that's finished.

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;
 }