[C++20] Destroying delete and deleted destructors

[AaronBallman]

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.

Fixes #46818

[llvmbot]

@llvm/pr-subscribers-clang

Author: Aaron Ballman (AaronBallman)

Changes

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/118800.diff

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/lib/Sema/SemaExprCXX.cpp (+4-2)
• (modified) clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp (+20-2)

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.
+}

[erichkeane]

1 nit, else LGTM.

[zygoloid]

Oh no! In the converse of this case it seems like we simply have no idea if the delete expression is potentially throwing. Eg, given:

struct A {
  virtual ~A(); // implicitly noexcept
};
struct B : A {
  void operator delete(B *p, std::destroying_delete_t) { throw "oh no"; }
};
A *p = new B;
static_assert(noexcept(delete p));

the assert doesn't fire but delete p can throw.

But there's nothing specific to a destroying delete here. We have the same problem with a normal class-specific delete:

struct C : A {
  void operator delete(void*) { throw "oops"; }
};
A *q = new C;
static_assert(noexcept(delete q));

I think this is a language defect; mailed to CWG for consideration.

[AaronBallman]

Oof, good catch! Also, thank you for reporting this to Core.

Do you think I should make any changes here as part of this PR? I saw your comment on #118687 (comment), I could handle that in a follow-up (probably tomorrow, so if you want me to revert those changes, I can). It's not quite clear to me whether this is a "wait for Core because we don't want behavioral churn" situation or not.

[zygoloid]

I think for now it'd be reasonable to say that:

• if the destructor is virtual, noexcept looks only at the exception specification of the destructor
• otherwise, noexcept looks at the selected deallocation function and, if that function is not a destroying operator delete, also the destructor

That'd match what we do in codegen. I'm not sure how that lines up with other implementations, but it seems like the most important thing is that noexcept is consistent with whether an exception can actually escape from the evaluation.

[AaronBallman]

Thanks, I've updated the logic in SemaExceptionSpec.cpp for this and added an additional test case to cover the noexcept operator behavior.

[zygoloid]

As I just noted in #118687, I think we also need similar treatment for noexcept(delete p).

[AaronBallman]

FYI, I'm going to be out on vacation after today, so if this is ready to land, someone else can feel free to press the button. Otherwise, I can pick this up again when I'm back.