[C++20] Destroying delete and deleted destructors

clang/docs/ReleaseNotes.rst

@@ -677,6 +677,9 @@ Bug Fixes in This Version
 - Fixed a crash when GNU statement expression contains invalid statement (#GH113468).
 - Fixed a failed assertion when using ``__attribute__((noderef))`` on an
   ``_Atomic``-qualified type (#GH116124).
+- No longer incorrectly diagnosing use of a deleted destructor when the
+  selected overload of ``operator delete`` for that type is a destroying delete
+  (#GH46818).
 - No longer return ``false`` for ``noexcept`` expressions involving a
   ``delete`` which resolves to a destroying delete but the type of the object
   being deleted has a potentially throwing destructor (#GH118660).

clang/lib/Sema/SemaExceptionSpec.cpp

@@ -1200,21 +1200,35 @@ CanThrowResult Sema::canThrow(const Stmt *S) {
 
   case Expr::CXXDeleteExprClass: {
     auto *DE = cast<CXXDeleteExpr>(S);
-    CanThrowResult CT;
+    CanThrowResult CT = CT_Cannot;
     QualType DTy = DE->getDestroyedType();
     if (DTy.isNull() || DTy->isDependentType()) {
       CT = CT_Dependent;
     } else {
+      // C++20 [expr.delete]p6: If the value of the operand of the delete-
+      // expression is not a null pointer value and the selected deallocation
+      // function (see below) is not a destroying operator delete, the delete-
+      // expression will invoke the destructor (if any) for the object or the
+      // elements of the array being deleted.
+      //
+      // So if the destructor is virtual, we only look at the exception
+      // specification of the destructor regardless of what operator delete is
+      // selected. Otherwise, we look at the selected operator delete, and if
+      // it is not a destroying delete, then we look at the destructor.
       const FunctionDecl *OperatorDelete = DE->getOperatorDelete();
-      CT = canCalleeThrow(*this, DE, OperatorDelete);
-      if (!OperatorDelete->isDestroyingOperatorDelete()) {
-        if (const auto *RD = DTy->getAsCXXRecordDecl()) {
-          if (const CXXDestructorDecl *DD = RD->getDestructor())
-            CT = mergeCanThrow(CT, canCalleeThrow(*this, DE, DD));
+      if (const auto *RD = DTy->getAsCXXRecordDecl()) {
+        if (const CXXDestructorDecl *DD = RD->getDestructor()) {
+          if (DD->isVirtual() || !OperatorDelete->isDestroyingOperatorDelete())
+            CT = canCalleeThrow(*this, DE, DD);
         }
-        if (CT == CT_Can)
-          return CT;
       }
+
+      // We always look at the exception specification of the operator delete.
+      CT = mergeCanThrow(CT, canCalleeThrow(*this, DE, OperatorDelete));
+
+      // If we know we can throw, we're done.
+      if (CT == CT_Can)
+        return CT;
     }
     return mergeCanThrow(CT, canSubStmtsThrow(*this, DE));
   }

clang/lib/Sema/SemaExprCXX.cpp

@@ -3768,6 +3768,28 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool UseGlobal,
     DeclarationName DeleteName = Context.DeclarationNames.getCXXOperatorName(
                                       ArrayForm ? OO_Array_Delete : OO_Delete);
 
+    // C++20 [expr.delete]p6: If the value of the operand of the delete-
+    // expression is not a null pointer value and the selected deallocation
+    // function (see below) is not a destroying operator delete, the delete-
+    // expression will invoke the destructor (if any) for the object or the
+    // elements of the array being deleted.
+    //
+    // This means we should not look at the destructor for a destroying
+    // delete operator, as that destructor is never called, unless the
+    // destructor is virtual (see [expr.delete]p8.1) because then the
+    // selected operator depends on the dynamic type of the pointer.
+    auto IsDtorCalled = [](const CXXMethodDecl *Dtor,
+                           const FunctionDecl *OperatorDelete) {
+      if (!OperatorDelete)
+        return true;
+
+      if (!OperatorDelete->isDestroyingOperatorDelete())
+        return true;
+
+      // We have a destroying operator delete, so it depends on the dtor.
+      return Dtor->isVirtual();
+    };
+
     if (PointeeRD) {
       if (!UseGlobal &&
           FindDeallocationFunction(StartLoc, PointeeRD, DeleteName,
@@ -3792,13 +3814,16 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool UseGlobal,
               .HasSizeT;
       }
 
-      if (!PointeeRD->hasIrrelevantDestructor())
+      if (!PointeeRD->hasIrrelevantDestructor()) {
         if (CXXDestructorDecl *Dtor = LookupDestructor(PointeeRD)) {
-          MarkFunctionReferenced(StartLoc,
-                                    const_cast<CXXDestructorDecl*>(Dtor));
-          if (DiagnoseUseOfDecl(Dtor, StartLoc))
-            return ExprError();
+          if (IsDtorCalled(Dtor, OperatorDelete)) {
+            MarkFunctionReferenced(StartLoc,
+                                   const_cast<CXXDestructorDecl *>(Dtor));
+            if (DiagnoseUseOfDecl(Dtor, StartLoc))
+              return ExprError();
+          }
         }
+      }
 
       CheckVirtualDtorCall(PointeeRD->getDestructor(), StartLoc,
                            /*IsDelete=*/true, /*CallCanBeVirtual=*/true,
@@ -3833,8 +3858,9 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool UseGlobal,
     bool IsVirtualDelete = false;
     if (PointeeRD) {
       if (CXXDestructorDecl *Dtor = LookupDestructor(PointeeRD)) {
-        CheckDestructorAccess(Ex.get()->getExprLoc(), Dtor,
-                              PDiag(diag::err_access_dtor) << PointeeElem);
+        if (IsDtorCalled(Dtor, OperatorDelete))
+          CheckDestructorAccess(Ex.get()->getExprLoc(), Dtor,
+                                PDiag(diag::err_access_dtor) << PointeeElem);
         IsVirtualDelete = Dtor->isVirtual();
       }
     }

clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp

@@ -1,7 +1,14 @@
-// RUN: %clang_cc1 -std=c++1z -verify %s
+// RUN: %clang_cc1 -std=c++20 -verify %s
 
 using size_t = decltype(sizeof(0));
-namespace std { enum class align_val_t : size_t {}; }
+namespace std {
+  enum class align_val_t : size_t {};
+  struct destroying_delete_t {
+    explicit destroying_delete_t() = default;
+  };
+
+  inline constexpr destroying_delete_t destroying_delete{};
+}
 
 // Aligned version is preferred over unaligned version,
 // unsized version is preferred over sized version.
@@ -23,3 +30,41 @@ struct alignas(Align) B {
 };
 void f(B<__STDCPP_DEFAULT_NEW_ALIGNMENT__> *p) { delete p; }
 void f(B<__STDCPP_DEFAULT_NEW_ALIGNMENT__ * 2> *p) { delete p; } // expected-error {{deleted}}
+
+// Ensure that a deleted destructor is acceptable when the selected overload
+// for operator delete is a destroying delete. See the comments in GH118660.
+struct S {
+  ~S() = delete;
+  void operator delete(S *, std::destroying_delete_t) noexcept {}
+};
+
+struct T {
+  void operator delete(T *, std::destroying_delete_t) noexcept {}
+private:
+  ~T();
+};
+
+void foo(S *s, T *t) {
+  delete s; // Was rejected, is intended to be accepted.
+  delete t; // Was rejected, is intended to be accepted.
+}
+
+// However, if the destructor is virtual, then it has to be accessible because
+// the behavior depends on which operator delete is selected and that is based
+// on the dynamic type of the pointer.
+struct U {
+  virtual ~U() = delete; // expected-note {{here}}
+  void operator delete(U *, std::destroying_delete_t) noexcept {}
+};
+
+struct V {
+  void operator delete(V *, std::destroying_delete_t) noexcept {}
+private:
+  virtual ~V(); // expected-note {{here}}
+};
+
+void bar(U *u, V *v) {
+  // Both should be rejected because they have virtual destructors.
+  delete u; // expected-error {{attempt to use a deleted function}}
+  delete v; // expected-error {{calling a private destructor of class 'V'}}
+}

clang/test/SemaCXX/cxx2a-destroying-delete.cpp

@@ -146,12 +146,12 @@ namespace dtor_access {
   struct S {
     void operator delete(S *p, std::destroying_delete_t);
   private:
-    ~S(); // expected-note {{here}}
+    ~S();
   };
 
-  // FIXME: PR47474: GCC accepts this, and it seems somewhat reasonable to
-  // allow, even though [expr.delete]p12 says this is ill-formed.
-  void f() { delete new S; } // expected-error {{calling a private destructor}}
+  // C++20 [expr.delete]p12 says this is ill-formed, but GCC accepts and we
+  // filed CWG2889 to resolve in the same way.
+  void f() { delete new S; }
 
   struct T {
     void operator delete(T *, std::destroying_delete_t);
@@ -165,7 +165,7 @@ namespace dtor_access {
     ~U() override;
   };
 
-  void g() { delete (T *)new U; } // expected-error {{calling a protected destructor}}
+  void g() { delete (T *)new U; } // expected-error {{calling a protected destructor of class 'T'}}
 }
 
 namespace delete_from_new {

clang/test/SemaCXX/noexcept-destroying-delete.cpp

@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -fcxx-exceptions -Wno-unevaluated-expression -std=c++20 %s
-// expected-no-diagnostics
 
 namespace std {
   struct destroying_delete_t {
@@ -31,3 +30,19 @@ ThrowingDestroyingDelete *pn = nullptr;
 // noexcept should return false here because the destroying delete itself is a
 // potentially throwing function.
 static_assert(!noexcept(delete(pn)));
+
+
+struct A {
+  virtual ~A(); // implicitly noexcept
+};
+struct B : A {
+  void operator delete(B *p, std::destroying_delete_t) { throw "oh no"; } // expected-warning {{'operator delete' has a non-throwing exception specification but can still throw}} \
+                                                                             expected-note {{deallocator has a implicit non-throwing exception specification}}
+};
+A *p = new B;
+
+// Because the destructor for A is virtual, it is the only thing we consider
+// when determining whether the delete expression can throw or not, despite the
+// fact that the selected operator delete is a destroying delete. For virtual
+// destructors, it's the dynamic type that matters.
+static_assert(noexcept(delete p));