[clang] [C++20] Destroying delete and deleted destructors (PR #118800)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman updated https://github.com/llvm/llvm-project/pull/118800

>From 34d3d3000bc6096bbc9eb35ce85b6ceca50b91ca Mon Sep 17 00:00:00 2001
From: Aaron Ballman <aaron at aaronballman.com>
Date: Thu, 5 Dec 2024 08:31:24 -0500
Subject: [PATCH 1/2] [C++20] Destroying delete and deleted destructors

When a destroying delete overload is selected, the destructor is not
automatically called. Therefore, the destructor can be deleted without
causing the program to be ill-formed.
---
 clang/docs/ReleaseNotes.rst                   |  2 ++
 clang/lib/Sema/SemaExprCXX.cpp                |  6 +++--
 .../CXX/expr/expr.unary/expr.delete/p10.cpp   | 22 +++++++++++++++++--
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e484eb7a76e63a..4a72e4046e2d03 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -663,6 +663,8 @@ 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.
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index db9ea7fb66e05a..45840dfa31ac92 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -3792,13 +3792,15 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool UseGlobal,
               .HasSizeT;
       }
 
-      if (!PointeeRD->hasIrrelevantDestructor())
+      if (!PointeeRD->hasIrrelevantDestructor() &&
+          (!OperatorDelete || !OperatorDelete->isDestroyingOperatorDelete())) {
         if (CXXDestructorDecl *Dtor = LookupDestructor(PointeeRD)) {
           MarkFunctionReferenced(StartLoc,
-                                    const_cast<CXXDestructorDecl*>(Dtor));
+                                 const_cast<CXXDestructorDecl *>(Dtor));
           if (DiagnoseUseOfDecl(Dtor, StartLoc))
             return ExprError();
         }
+      }
 
       CheckVirtualDtorCall(PointeeRD->getDestructor(), StartLoc,
                            /*IsDelete=*/true, /*CallCanBeVirtual=*/true,
diff --git a/clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp b/clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp
index aad2747dd32f24..b2c0a2c79695fc 100644
--- a/clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp
+++ b/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,14 @@ 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 {}
+};
+
+void foo(S *s) {
+  delete s; // Was rejected, is intended to be accepted.
+}

>From 133a8aa2934f9d6ed733e74af65180131d59cc91 Mon Sep 17 00:00:00 2001
From: Aaron Ballman <aaron at aaronballman.com>
Date: Thu, 5 Dec 2024 10:14:46 -0500
Subject: [PATCH 2/2] Update based on review feedback

* Added a standards reference
* Added a test case
* Fixed an issue the new test case identified
---
 clang/lib/Sema/SemaExprCXX.cpp                     | 13 +++++++++++--
 clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp | 10 ++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 45840dfa31ac92..6ac44ae7af28c3 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -3792,6 +3792,14 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool UseGlobal,
               .HasSizeT;
       }
 
+      // 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.
       if (!PointeeRD->hasIrrelevantDestructor() &&
           (!OperatorDelete || !OperatorDelete->isDestroyingOperatorDelete())) {
         if (CXXDestructorDecl *Dtor = LookupDestructor(PointeeRD)) {
@@ -3831,9 +3839,10 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool UseGlobal,
     MarkFunctionReferenced(StartLoc, OperatorDelete);
 
     // Check access and ambiguity of destructor if we're going to call it.
-    // Note that this is required even for a virtual delete.
+    // Note that this is required even for a virtual delete, but not for a
+    // destroying delete operator.
     bool IsVirtualDelete = false;
-    if (PointeeRD) {
+    if (PointeeRD && !OperatorDelete->isDestroyingOperatorDelete()) {
       if (CXXDestructorDecl *Dtor = LookupDestructor(PointeeRD)) {
         CheckDestructorAccess(Ex.get()->getExprLoc(), Dtor,
                               PDiag(diag::err_access_dtor) << PointeeElem);
diff --git a/clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp b/clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp
index b2c0a2c79695fc..aff3e2b449da37 100644
--- a/clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp
+++ b/clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp
@@ -38,6 +38,16 @@ struct S {
   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) {
   delete s; // Was rejected, is intended to be accepted.
 }
+
+void bar(T *t) {
+  delete t; // Was rejected, is intended to be accepted.
+}