Skip to content

[Clang] [C++26] Implement P2573R2: = delete("should have a reason"); #86526

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 28 commits into from
Apr 14, 2024

Conversation

Sirraide
Copy link
Member

@Sirraide Sirraide commented Mar 25, 2024

This implements support for the = delete("message") syntax that was only just added to C++26 (P2573R2).

The major parts are all done, from what I can tell, except for one thing: we need to figure out where to store the message. We’re out of bits in FunctionDeclBits, and we can’t put it in the union that stores the DefaultedFunctionInfo because a function may end up having to store both at the same time, so I’m thinking we could store it in the DefaultedFunctionInfo and rename it to ExtraFunctionInfo or something like that.

There are some fairly straight-forward things that are still missing; I’m putting these here so I don’t end up forgetting about any of them (I’ll work on these while we figure out where to put the message):

  • Add a release note.
  • Add a standards citation and update the BNF for = delete if we have that in a comment somewhere, etc.
  • Feature test macro.
  • Update the C++ conformance page.
  • Emit a warning in language modes before C++26 (imo there should be no problem w/ supporting this in all language modes where we also support = delete).
  • More tests for things like allocation functions, which seem to have separate code paths in some cases, as well as for using functions in other ways than just calling them.
  • Also test messages containing unicode characters.
  • Not ‘necessary’ but would be nice: AST Matchers & libclang support for getting the message. (should probably be a separate pr)

@Sirraide Sirraide added clang:frontend Language frontend issues, e.g. anything involving "Sema" c++26 labels Mar 25, 2024
@Sirraide Sirraide requested a review from AaronBallman March 25, 2024 16:21
@Sirraide Sirraide requested a review from Endilll as a code owner March 25, 2024 16:21
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:modules C++20 modules and Clang Header Modules labels Mar 25, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 25, 2024

@llvm/pr-subscribers-clang-modules

Author: None (Sirraide)

Changes

This implements support for the = delete("message") syntax that was only just added to C++26 (P2573R2).

The major parts are all done, from what I can tell, except for one thing: we need to figure out where to store the message. We’re out of bits in FunctionDeclBits, and we can’t put it in the union that stores the DefaultedFunctionInfo because a function may end up having to store both at the same time, so I’m thinking we could store it in the DefaultedFunctionInfo and rename it to ExtraFunctionInfo or something like that.

There are some fairly straight-forward things that are still missing; I’m putting these here so I don’t end up forgetting about any of them (I’ll work on these while we figure out where to put the message):

  • Add a release note.
  • Add a standards citation and update the BNF for = delete if we have that in a comment somewhere, etc.
  • Feature test macro.
  • Update the C++ conformance page.
  • Emit a warning in language modes before C++26 (imo there should be no problem w/ supporting this in all language modes where we also support = delete).
  • More tests for things like allocation functions, which seem to have separate code paths in some cases.
  • Not ‘necessary’ but would be nice: AST Matchers & libclang support for getting the message.

Patch is 24.57 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/86526.diff

20 Files Affected:

  • (modified) clang/include/clang/AST/Decl.h (+13)
  • (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+3-4)
  • (modified) clang/include/clang/Parse/Parser.h (+1)
  • (modified) clang/include/clang/Sema/Sema.h (+9-2)
  • (modified) clang/lib/AST/Decl.cpp (+2-2)
  • (modified) clang/lib/AST/DeclPrinter.cpp (+7-2)
  • (modified) clang/lib/AST/TextNodeDumper.cpp (+3)
  • (modified) clang/lib/Parse/ParseCXXInlineMethods.cpp (+24-1)
  • (modified) clang/lib/Parse/Parser.cpp (+3-1)
  • (modified) clang/lib/Sema/SemaDeclCXX.cpp (+6-5)
  • (modified) clang/lib/Sema/SemaExpr.cpp (+5-2)
  • (modified) clang/lib/Sema/SemaExprCXX.cpp (+9-14)
  • (modified) clang/lib/Sema/SemaOverload.cpp (+21-11)
  • (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+3-2)
  • (modified) clang/lib/Serialization/ASTReaderDecl.cpp (+5)
  • (modified) clang/lib/Serialization/ASTWriterDecl.cpp (+7)
  • (added) clang/test/AST/ast-dump-cxx2c-delete-with-message.cpp (+23)
  • (added) clang/test/Parser/cxx2c-delete-with-message.cpp (+33)
  • (added) clang/test/SemaCXX/ast-print-cxx2c-delete-with-message.cpp (+18)
  • (added) clang/test/SemaCXX/cxx2c-delete-with-message.cpp (+30)
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index a5879591f4c659..9c445eccdf8992 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -2013,6 +2013,12 @@ class FunctionDecl : public DeclaratorDecl,
     DefaultedFunctionInfo *DefaultedInfo;
   };
 
+  /// Message that indicates why this function was deleted.
+  ///
+  /// FIXME: Figure out where to actually put this; maybe in the
+  /// 'DefaultedInfo' above?
+  StringLiteral *DeletedMessage;
+
   unsigned ODRHash;
 
   /// End part of this FunctionDecl's source range.
@@ -2483,6 +2489,10 @@ class FunctionDecl : public DeclaratorDecl,
   }
 
   void setDeletedAsWritten(bool D = true) { FunctionDeclBits.IsDeleted = D; }
+  void setDeletedWithMessage(StringLiteral *Message) {
+    FunctionDeclBits.IsDeleted = true;
+    DeletedMessage = Message;
+  }
 
   /// Determines whether this function is "main", which is the
   /// entry point into an executable program.
@@ -2638,6 +2648,9 @@ class FunctionDecl : public DeclaratorDecl,
       AC.push_back(TRC);
   }
 
+  /// Get the message that indicates why this function was deleted.
+  StringLiteral *getDeletedMessage() const { return DeletedMessage; }
+
   void setPreviousDeclaration(FunctionDecl * PrevDecl);
 
   FunctionDecl *getCanonicalDecl() override;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index d7ab1635cf12bc..37197b60ddfbb8 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4661,11 +4661,10 @@ def err_ovl_no_viable_member_function_in_call : Error<
   "no matching member function for call to %0">;
 def err_ovl_ambiguous_call : Error<
   "call to %0 is ambiguous">;
-def err_ovl_deleted_call : Error<"call to deleted function %0">;
+def err_ovl_deleted_call : Error<"call to deleted"
+  "%select{| member}0 function %1%select{|: %3}2">;
 def err_ovl_ambiguous_member_call : Error<
   "call to member function %0 is ambiguous">;
-def err_ovl_deleted_member_call : Error<
-  "call to deleted member function %0">;
 def note_ovl_too_many_candidates : Note<
     "remaining %0 candidate%s0 omitted; "
     "pass -fshow-overloads=all to show them">;
@@ -8857,7 +8856,7 @@ def err_nontemporal_builtin_must_be_pointer_intfltptr_or_vector : Error<
   "address argument to nontemporal builtin must be a pointer to integer, float, "
   "pointer, or a vector of such types (%0 invalid)">;
 
-def err_deleted_function_use : Error<"attempt to use a deleted function">;
+def err_deleted_function_use : Error<"attempt to use a deleted function%select{|: %1}0">;
 def err_deleted_inherited_ctor_use : Error<
   "constructor inherited by %0 from base class %1 is implicitly deleted">;
 
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index 14df75180ef321..559e6416b7dfb3 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -1600,6 +1600,7 @@ class Parser : public CodeCompletionHandler {
                                      const ParsedTemplateInfo &TemplateInfo,
                                      const VirtSpecifiers &VS,
                                      SourceLocation PureSpecLoc);
+  StringLiteral *ParseCXXDeletedFunctionMessage();
   void ParseCXXNonStaticMemberInitializer(Decl *VarD);
   void ParseLexedAttributes(ParsingClass &Class);
   void ParseLexedAttributeList(LateParsedAttrList &LAs, Decl *D,
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 64de39acc72176..9f6e2639508b2a 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -4926,10 +4926,12 @@ class Sema final {
                                SourceLocation EqualLoc);
 
   void ActOnPureSpecifier(Decl *D, SourceLocation PureSpecLoc);
-  void SetDeclDeleted(Decl *dcl, SourceLocation DelLoc);
+  void SetDeclDeleted(Decl *dcl, SourceLocation DelLoc,
+                      StringLiteral *Message = nullptr);
   void SetDeclDefaulted(Decl *dcl, SourceLocation DefaultLoc);
 
-  void SetFunctionBodyKind(Decl *D, SourceLocation Loc, FnBodyKind BodyKind);
+  void SetFunctionBodyKind(Decl *D, SourceLocation Loc, FnBodyKind BodyKind,
+                           StringLiteral *DeletedMessage = nullptr);
   void ActOnStartTrailingRequiresClause(Scope *S, Declarator &D);
   ExprResult ActOnFinishTrailingRequiresClause(ExprResult ConstraintExpr);
   ExprResult ActOnRequiresClause(ExprResult ConstraintExpr);
@@ -8262,6 +8264,11 @@ class Sema final {
   bool IsFunctionConversion(QualType FromType, QualType ToType,
                             QualType &ResultTy);
   bool DiagnoseMultipleUserDefinedConversion(Expr *From, QualType ToType);
+  void DiagnoseUseOfDeletedFunction(SourceLocation Loc, SourceRange Range,
+                                    DeclarationName Name,
+                                    OverloadCandidateSet &CandidateSet,
+                                    FunctionDecl *Fn, MultiExprArg Args,
+                                    bool IsMember = false);
 
   ExprResult InitializeExplicitObjectArgument(Sema &S, Expr *Obj,
                                               FunctionDecl *Fun);
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 95900afdd2c5d8..b5e61f0eceb6e4 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -3043,8 +3043,8 @@ FunctionDecl::FunctionDecl(Kind DK, ASTContext &C, DeclContext *DC,
                            Expr *TrailingRequiresClause)
     : DeclaratorDecl(DK, DC, NameInfo.getLoc(), NameInfo.getName(), T, TInfo,
                      StartLoc),
-      DeclContext(DK), redeclarable_base(C), Body(), ODRHash(0),
-      EndRangeLoc(NameInfo.getEndLoc()), DNLoc(NameInfo.getInfo()) {
+      DeclContext(DK), redeclarable_base(C), Body(), DeletedMessage(nullptr),
+      ODRHash(0), EndRangeLoc(NameInfo.getEndLoc()), DNLoc(NameInfo.getInfo()) {
   assert(T.isNull() || T->isFunctionType());
   FunctionDeclBits.SClass = S;
   FunctionDeclBits.IsInline = isInlineSpecified;
diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp
index b701581b2474a9..d04cdd5a10e033 100644
--- a/clang/lib/AST/DeclPrinter.cpp
+++ b/clang/lib/AST/DeclPrinter.cpp
@@ -873,9 +873,14 @@ void DeclPrinter::VisitFunctionDecl(FunctionDecl *D) {
 
   if (D->isPureVirtual())
     Out << " = 0";
-  else if (D->isDeletedAsWritten())
+  else if (D->isDeletedAsWritten()) {
     Out << " = delete";
-  else if (D->isExplicitlyDefaulted())
+    if (const auto *M = D->getDeletedMessage()) {
+      Out << "(";
+      M->outputString(Out);
+      Out << ")";
+    }
+  } else if (D->isExplicitlyDefaulted())
     Out << " = default";
   else if (D->doesThisDeclarationHaveABody()) {
     if (!Policy.TerseOutput) {
diff --git a/clang/lib/AST/TextNodeDumper.cpp b/clang/lib/AST/TextNodeDumper.cpp
index b683eb1edd8f13..4a2369330a35e7 100644
--- a/clang/lib/AST/TextNodeDumper.cpp
+++ b/clang/lib/AST/TextNodeDumper.cpp
@@ -1936,6 +1936,9 @@ void TextNodeDumper::VisitFunctionDecl(const FunctionDecl *D) {
   if (D->isTrivial())
     OS << " trivial";
 
+  if (const auto *M = D->getDeletedMessage())
+    AddChild("delete message", [=] { Visit(M); });
+
   if (D->isIneligibleOrNotSelected())
     OS << (isa<CXXDestructorDecl>(D) ? " not_selected" : " ineligible");
 
diff --git a/clang/lib/Parse/ParseCXXInlineMethods.cpp b/clang/lib/Parse/ParseCXXInlineMethods.cpp
index d790060c17c049..9e2f4ce562a4ea 100644
--- a/clang/lib/Parse/ParseCXXInlineMethods.cpp
+++ b/clang/lib/Parse/ParseCXXInlineMethods.cpp
@@ -20,6 +20,28 @@
 
 using namespace clang;
 
+/// Parse the optional ("message") part of a deleted-function-body.
+StringLiteral *Parser::ParseCXXDeletedFunctionMessage() {
+  if (!Tok.is(tok::l_paren))
+    return nullptr;
+  StringLiteral *Message = nullptr;
+  BalancedDelimiterTracker BT{*this, tok::l_paren};
+  BT.consumeOpen();
+
+  if (isTokenStringLiteral()) {
+    ExprResult Res = ParseUnevaluatedStringLiteralExpression();
+    if (Res.isUsable())
+      Message = Res.getAs<StringLiteral>();
+  } else {
+    Diag(Tok.getLocation(), diag::err_expected_string_literal)
+        << /*Source='in'*/ 0 << "'delete'";
+    SkipUntil(tok::r_paren, StopAtSemi | StopBeforeMatch);
+  }
+
+  BT.consumeClose();
+  return Message;
+}
+
 /// ParseCXXInlineMethodDef - We parsed and verified that the specified
 /// Declarator is a well formed C++ inline method definition. Now lex its body
 /// and store its tokens for parsing after the C++ class is complete.
@@ -70,7 +92,8 @@ NamedDecl *Parser::ParseCXXInlineMethodDef(
                       ? diag::warn_cxx98_compat_defaulted_deleted_function
                       : diag::ext_defaulted_deleted_function)
         << 1 /* deleted */;
-      Actions.SetDeclDeleted(FnD, KWLoc);
+      StringLiteral *Message = ParseCXXDeletedFunctionMessage();
+      Actions.SetDeclDeleted(FnD, KWLoc, Message);
       Delete = true;
       if (auto *DeclAsFunction = dyn_cast<FunctionDecl>(FnD)) {
         DeclAsFunction->setRangeEnd(KWEndLoc);
diff --git a/clang/lib/Parse/Parser.cpp b/clang/lib/Parse/Parser.cpp
index cc0e41ed221c4f..d6f2b9f448cd52 100644
--- a/clang/lib/Parse/Parser.cpp
+++ b/clang/lib/Parse/Parser.cpp
@@ -1404,6 +1404,7 @@ Decl *Parser::ParseFunctionDefinition(ParsingDeclarator &D,
 
   // Parse function body eagerly if it is either '= delete;' or '= default;' as
   // ActOnStartOfFunctionDef needs to know whether the function is deleted.
+  StringLiteral *DeletedMessage = nullptr;
   Sema::FnBodyKind BodyKind = Sema::FnBodyKind::Other;
   SourceLocation KWLoc;
   if (TryConsumeToken(tok::equal)) {
@@ -1415,6 +1416,7 @@ Decl *Parser::ParseFunctionDefinition(ParsingDeclarator &D,
                       : diag::ext_defaulted_deleted_function)
           << 1 /* deleted */;
       BodyKind = Sema::FnBodyKind::Delete;
+      DeletedMessage = ParseCXXDeletedFunctionMessage();
     } else if (TryConsumeToken(tok::kw_default, KWLoc)) {
       Diag(KWLoc, getLangOpts().CPlusPlus11
                       ? diag::warn_cxx98_compat_defaulted_deleted_function
@@ -1473,7 +1475,7 @@ Decl *Parser::ParseFunctionDefinition(ParsingDeclarator &D,
   D.getMutableDeclSpec().abort();
 
   if (BodyKind != Sema::FnBodyKind::Other) {
-    Actions.SetFunctionBodyKind(Res, KWLoc, BodyKind);
+    Actions.SetFunctionBodyKind(Res, KWLoc, BodyKind, DeletedMessage);
     Stmt *GeneratedBody = Res ? Res->getBody() : nullptr;
     Actions.ActOnFinishFunctionBody(Res, GeneratedBody, false);
     return Res;
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index e258a4f7c89415..50eff9b02cbef8 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -18108,7 +18108,8 @@ NamedDecl *Sema::ActOnFriendFunctionDecl(Scope *S, Declarator &D,
   return ND;
 }
 
-void Sema::SetDeclDeleted(Decl *Dcl, SourceLocation DelLoc) {
+void Sema::SetDeclDeleted(Decl *Dcl, SourceLocation DelLoc,
+                          StringLiteral *Message) {
   AdjustDeclIfTemplate(Dcl);
 
   FunctionDecl *Fn = dyn_cast_or_null<FunctionDecl>(Dcl);
@@ -18157,7 +18158,7 @@ void Sema::SetDeclDeleted(Decl *Dcl, SourceLocation DelLoc) {
   // C++11 [dcl.fct.def.delete]p4:
   //  A deleted function is implicitly inline.
   Fn->setImplicitlyInline();
-  Fn->setDeletedAsWritten();
+  Fn->setDeletedWithMessage(Message);
 }
 
 void Sema::SetDeclDefaulted(Decl *Dcl, SourceLocation DefaultLoc) {
@@ -18270,11 +18271,11 @@ void Sema::DiagnoseReturnInConstructorExceptionHandler(CXXTryStmt *TryBlock) {
   }
 }
 
-void Sema::SetFunctionBodyKind(Decl *D, SourceLocation Loc,
-                               FnBodyKind BodyKind) {
+void Sema::SetFunctionBodyKind(Decl *D, SourceLocation Loc, FnBodyKind BodyKind,
+                               StringLiteral *DeletedMessage) {
   switch (BodyKind) {
   case FnBodyKind::Delete:
-    SetDeclDeleted(D, Loc);
+    SetDeclDeleted(D, Loc, DeletedMessage);
     break;
   case FnBodyKind::Default:
     SetDeclDefaulted(D, Loc);
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 8725b09f8546cf..76fbce8a95da85 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -271,8 +271,11 @@ bool Sema::DiagnoseUseOfDecl(NamedDecl *D, ArrayRef<SourceLocation> Locs,
         Diag(Loc, diag::err_deleted_inherited_ctor_use)
             << Ctor->getParent()
             << Ctor->getInheritedConstructor().getConstructor()->getParent();
-      else
-        Diag(Loc, diag::err_deleted_function_use);
+      else {
+        StringLiteral *Msg = FD->getDeletedMessage();
+        Diag(Loc, diag::err_deleted_function_use)
+            << !!Msg << (Msg ? Msg->getString() : StringRef());
+      }
       NoteDeletedFunction(FD);
       return true;
     }
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index c34a40fa7c81ac..e5b8d1fbb81554 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -2663,13 +2663,9 @@ static bool resolveAllocationOverload(
     return true;
 
   case OR_Deleted: {
-    if (Diagnose) {
-      Candidates.NoteCandidates(
-          PartialDiagnosticAt(R.getNameLoc(),
-                              S.PDiag(diag::err_ovl_deleted_call)
-                                  << R.getLookupName() << Range),
-          S, OCD_AllCandidates, Args);
-    }
+    if (Diagnose)
+      S.DiagnoseUseOfDeletedFunction(R.getNameLoc(), Range, R.getLookupName(),
+                                     Candidates, Best->Function, Args);
     return true;
   }
   }
@@ -3323,7 +3319,9 @@ bool Sema::FindDeallocationFunction(SourceLocation StartLoc, CXXRecordDecl *RD,
     // FIXME: DiagnoseUseOfDecl?
     if (Operator->isDeleted()) {
       if (Diagnose) {
-        Diag(StartLoc, diag::err_deleted_function_use);
+        StringLiteral *Msg = Operator->getDeletedMessage();
+        Diag(StartLoc, diag::err_deleted_function_use)
+            << !!Msg << (Msg ? Msg->getString() : StringRef());
         NoteDeletedFunction(Operator);
       }
       return true;
@@ -3927,14 +3925,11 @@ static bool resolveBuiltinNewDeleteOverload(Sema &S, CallExpr *TheCall,
         S, OCD_AmbiguousCandidates, Args);
     return true;
 
-  case OR_Deleted: {
-    Candidates.NoteCandidates(
-        PartialDiagnosticAt(R.getNameLoc(), S.PDiag(diag::err_ovl_deleted_call)
-                                                << R.getLookupName() << Range),
-        S, OCD_AllCandidates, Args);
+  case OR_Deleted:
+    S.DiagnoseUseOfDeletedFunction(R.getNameLoc(), Range, R.getLookupName(),
+                                   Candidates, Best->Function, Args);
     return true;
   }
-  }
   llvm_unreachable("Unreachable, bad result from BestViableFunction");
 }
 
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 4e03c3124e39ab..e4350df47c3570 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -14138,15 +14138,13 @@ static ExprResult FinishOverloadedCallExpr(Sema &SemaRef, Scope *S, Expr *Fn,
     break;
 
   case OR_Deleted: {
-    CandidateSet->NoteCandidates(
-        PartialDiagnosticAt(Fn->getBeginLoc(),
-                            SemaRef.PDiag(diag::err_ovl_deleted_call)
-                                << ULE->getName() << Fn->getSourceRange()),
-        SemaRef, OCD_AllCandidates, Args);
+    FunctionDecl *FDecl = (*Best)->Function;
+    SemaRef.DiagnoseUseOfDeletedFunction(Fn->getBeginLoc(),
+                                         Fn->getSourceRange(), ULE->getName(),
+                                         *CandidateSet, FDecl, Args);
 
     // We emitted an error for the unavailable/deleted function call but keep
     // the call in the AST.
-    FunctionDecl *FDecl = (*Best)->Function;
     ExprResult Res =
         SemaRef.FixOverloadedFunctionReference(Fn, (*Best)->FoundDecl, FDecl);
     if (Res.isInvalid())
@@ -15588,11 +15586,9 @@ ExprResult Sema::BuildCallToMemberFunction(Scope *S, Expr *MemExprE,
           *this, OCD_AmbiguousCandidates, Args);
       break;
     case OR_Deleted:
-      CandidateSet.NoteCandidates(
-          PartialDiagnosticAt(UnresExpr->getMemberLoc(),
-                              PDiag(diag::err_ovl_deleted_member_call)
-                                  << DeclName << MemExprE->getSourceRange()),
-          *this, OCD_AllCandidates, Args);
+      DiagnoseUseOfDeletedFunction(
+          UnresExpr->getMemberLoc(), MemExprE->getSourceRange(), DeclName,
+          CandidateSet, Best->Function, Args, /*IsMember=*/true);
       break;
     }
     // Overload resolution fails, try to recover.
@@ -16483,3 +16479,17 @@ bool clang::shouldEnforceArgLimit(bool PartialOverloading,
         return false;
   return true;
 }
+
+void Sema::DiagnoseUseOfDeletedFunction(SourceLocation Loc, SourceRange Range,
+                                        DeclarationName Name,
+                                        OverloadCandidateSet &CandidateSet,
+                                        FunctionDecl *Fn, MultiExprArg Args,
+                                        bool IsMember) {
+  StringLiteral *Msg = Fn->getDeletedMessage();
+  CandidateSet.NoteCandidates(
+      PartialDiagnosticAt(Loc, PDiag(diag::err_ovl_deleted_call)
+                                   << IsMember << Name << !!Msg
+                                   << (Msg ? Msg->getString() : StringRef())
+                                   << Range),
+      *this, OCD_AllCandidates, Args);
+}
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index dc972018e7b281..dfc6edc003e641 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -2429,7 +2429,7 @@ Decl *TemplateDeclInstantiator::VisitFunctionDecl(
       return nullptr;
   }
   if (D->isDeleted())
-    SemaRef.SetDeclDeleted(Function, D->getLocation());
+    SemaRef.SetDeclDeleted(Function, D->getLocation(), D->getDeletedMessage());
 
   NamedDecl *PrincipalDecl =
       (TemplateParams ? cast<NamedDecl>(FunctionTemplate) : Function);
@@ -2805,7 +2805,8 @@ Decl *TemplateDeclInstantiator::VisitCXXMethodDecl(
       return nullptr;
   }
   if (D->isDeletedAsWritten())
-    SemaRef.SetDeclDeleted(Method, Method->getLocation());
+    SemaRef.SetDeclDeleted(Method, Method->getLocation(),
+                           D->getDeletedMessage());
 
   // If this is an explicit specialization, mark the implicitly-instantiated
   // template specialization as being an explicit specialization too.
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index a22f760408c634..37bc61b9a63a34 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -1122,6 +1122,11 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
     }
   }
 
+  // FIXME: See ASTWriterDecl::VisitFunctionDecl.
+  if (FD->isDeletedAsWritten())
+    FD->setDeletedWithMessage(
+        cast_if_present<StringLiteral>(Record.readStmt()));
+
   if (Existing)
     mergeRedeclarable(FD, Existing, Redecl);
   else if (auto Kind = FD->getTemplatedKind();
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp
index 86f64bf2a24250..af7dbe916419eb 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -761,6 +761,13 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
     }
   }
 
+  // FIXME: Hack: We're out of bits in FunctionDeclBits, so always
+  // add this even though it's 0 in the vast majority of cases. We
+  // might really want to consider storing this in the DefaultedFunctionInfo
+  // instead.
+  if (D->isDeletedAsWritten())
+    Record.AddStmt(D->getDeletedMessage());
+
   Record.push_back(D->param_size());
   for (auto *P : D->parameters())
     Record.AddDeclRef(P);
diff --git a/clang/test/AST/ast-dump-cxx2c-delete-with-message.cpp b/clang/test/AST/ast-dump-cxx2c-delete-with-message.cpp
new file mode 100644
index 00000000000000..ea16b97da23e40
--- /dev/null
+++ b/clang/test/AST/ast-dump-cxx2c-delete-with-message.cpp
@@ -0,0 +1,23 @@
+// Without serialization:
+// RUN: %clang_cc1 -ast-dump %s | FileCheck %s
+//
+// With serialization:
+// RUN: %clang_cc1 -emit-pch -o %t %s
+// RUN: %clang_cc1 -x c++ -include-pch %t -ast-dump...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Mar 25, 2024

@llvm/pr-subscribers-clang

Author: None (Sirraide)

Changes

This implements support for the = delete("message") syntax that was only just added to C++26 (P2573R2).

The major parts are all done, from what I can tell, except for one thing: we need to figure out where to store the message. We’re out of bits in FunctionDeclBits, and we can’t put it in the union that stores the DefaultedFunctionInfo because a function may end up having to store both at the same time, so I’m thinking we could store it in the DefaultedFunctionInfo and rename it to ExtraFunctionInfo or something like that.

There are some fairly straight-forward things that are still missing; I’m putting these here so I don’t end up forgetting about any of them (I’ll work on these while we figure out where to put the message):

  • Add a release note.
  • Add a standards citation and update the BNF for = delete if we have that in a comment somewhere, etc.
  • Feature test macro.
  • Update the C++ conformance page.
  • Emit a warning in language modes before C++26 (imo there should be no problem w/ supporting this in all language modes where we also support = delete).
  • More tests for things like allocation functions, which seem to have separate code paths in some cases.
  • Not ‘necessary’ but would be nice: AST Matchers & libclang support for getting the message.

Patch is 24.57 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/86526.diff

20 Files Affected:

  • (modified) clang/include/clang/AST/Decl.h (+13)
  • (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+3-4)
  • (modified) clang/include/clang/Parse/Parser.h (+1)
  • (modified) clang/include/clang/Sema/Sema.h (+9-2)
  • (modified) clang/lib/AST/Decl.cpp (+2-2)
  • (modified) clang/lib/AST/DeclPrinter.cpp (+7-2)
  • (modified) clang/lib/AST/TextNodeDumper.cpp (+3)
  • (modified) clang/lib/Parse/ParseCXXInlineMethods.cpp (+24-1)
  • (modified) clang/lib/Parse/Parser.cpp (+3-1)
  • (modified) clang/lib/Sema/SemaDeclCXX.cpp (+6-5)
  • (modified) clang/lib/Sema/SemaExpr.cpp (+5-2)
  • (modified) clang/lib/Sema/SemaExprCXX.cpp (+9-14)
  • (modified) clang/lib/Sema/SemaOverload.cpp (+21-11)
  • (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+3-2)
  • (modified) clang/lib/Serialization/ASTReaderDecl.cpp (+5)
  • (modified) clang/lib/Serialization/ASTWriterDecl.cpp (+7)
  • (added) clang/test/AST/ast-dump-cxx2c-delete-with-message.cpp (+23)
  • (added) clang/test/Parser/cxx2c-delete-with-message.cpp (+33)
  • (added) clang/test/SemaCXX/ast-print-cxx2c-delete-with-message.cpp (+18)
  • (added) clang/test/SemaCXX/cxx2c-delete-with-message.cpp (+30)
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index a5879591f4c659..9c445eccdf8992 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -2013,6 +2013,12 @@ class FunctionDecl : public DeclaratorDecl,
     DefaultedFunctionInfo *DefaultedInfo;
   };
 
+  /// Message that indicates why this function was deleted.
+  ///
+  /// FIXME: Figure out where to actually put this; maybe in the
+  /// 'DefaultedInfo' above?
+  StringLiteral *DeletedMessage;
+
   unsigned ODRHash;
 
   /// End part of this FunctionDecl's source range.
@@ -2483,6 +2489,10 @@ class FunctionDecl : public DeclaratorDecl,
   }
 
   void setDeletedAsWritten(bool D = true) { FunctionDeclBits.IsDeleted = D; }
+  void setDeletedWithMessage(StringLiteral *Message) {
+    FunctionDeclBits.IsDeleted = true;
+    DeletedMessage = Message;
+  }
 
   /// Determines whether this function is "main", which is the
   /// entry point into an executable program.
@@ -2638,6 +2648,9 @@ class FunctionDecl : public DeclaratorDecl,
       AC.push_back(TRC);
   }
 
+  /// Get the message that indicates why this function was deleted.
+  StringLiteral *getDeletedMessage() const { return DeletedMessage; }
+
   void setPreviousDeclaration(FunctionDecl * PrevDecl);
 
   FunctionDecl *getCanonicalDecl() override;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index d7ab1635cf12bc..37197b60ddfbb8 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4661,11 +4661,10 @@ def err_ovl_no_viable_member_function_in_call : Error<
   "no matching member function for call to %0">;
 def err_ovl_ambiguous_call : Error<
   "call to %0 is ambiguous">;
-def err_ovl_deleted_call : Error<"call to deleted function %0">;
+def err_ovl_deleted_call : Error<"call to deleted"
+  "%select{| member}0 function %1%select{|: %3}2">;
 def err_ovl_ambiguous_member_call : Error<
   "call to member function %0 is ambiguous">;
-def err_ovl_deleted_member_call : Error<
-  "call to deleted member function %0">;
 def note_ovl_too_many_candidates : Note<
     "remaining %0 candidate%s0 omitted; "
     "pass -fshow-overloads=all to show them">;
@@ -8857,7 +8856,7 @@ def err_nontemporal_builtin_must_be_pointer_intfltptr_or_vector : Error<
   "address argument to nontemporal builtin must be a pointer to integer, float, "
   "pointer, or a vector of such types (%0 invalid)">;
 
-def err_deleted_function_use : Error<"attempt to use a deleted function">;
+def err_deleted_function_use : Error<"attempt to use a deleted function%select{|: %1}0">;
 def err_deleted_inherited_ctor_use : Error<
   "constructor inherited by %0 from base class %1 is implicitly deleted">;
 
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index 14df75180ef321..559e6416b7dfb3 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -1600,6 +1600,7 @@ class Parser : public CodeCompletionHandler {
                                      const ParsedTemplateInfo &TemplateInfo,
                                      const VirtSpecifiers &VS,
                                      SourceLocation PureSpecLoc);
+  StringLiteral *ParseCXXDeletedFunctionMessage();
   void ParseCXXNonStaticMemberInitializer(Decl *VarD);
   void ParseLexedAttributes(ParsingClass &Class);
   void ParseLexedAttributeList(LateParsedAttrList &LAs, Decl *D,
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 64de39acc72176..9f6e2639508b2a 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -4926,10 +4926,12 @@ class Sema final {
                                SourceLocation EqualLoc);
 
   void ActOnPureSpecifier(Decl *D, SourceLocation PureSpecLoc);
-  void SetDeclDeleted(Decl *dcl, SourceLocation DelLoc);
+  void SetDeclDeleted(Decl *dcl, SourceLocation DelLoc,
+                      StringLiteral *Message = nullptr);
   void SetDeclDefaulted(Decl *dcl, SourceLocation DefaultLoc);
 
-  void SetFunctionBodyKind(Decl *D, SourceLocation Loc, FnBodyKind BodyKind);
+  void SetFunctionBodyKind(Decl *D, SourceLocation Loc, FnBodyKind BodyKind,
+                           StringLiteral *DeletedMessage = nullptr);
   void ActOnStartTrailingRequiresClause(Scope *S, Declarator &D);
   ExprResult ActOnFinishTrailingRequiresClause(ExprResult ConstraintExpr);
   ExprResult ActOnRequiresClause(ExprResult ConstraintExpr);
@@ -8262,6 +8264,11 @@ class Sema final {
   bool IsFunctionConversion(QualType FromType, QualType ToType,
                             QualType &ResultTy);
   bool DiagnoseMultipleUserDefinedConversion(Expr *From, QualType ToType);
+  void DiagnoseUseOfDeletedFunction(SourceLocation Loc, SourceRange Range,
+                                    DeclarationName Name,
+                                    OverloadCandidateSet &CandidateSet,
+                                    FunctionDecl *Fn, MultiExprArg Args,
+                                    bool IsMember = false);
 
   ExprResult InitializeExplicitObjectArgument(Sema &S, Expr *Obj,
                                               FunctionDecl *Fun);
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 95900afdd2c5d8..b5e61f0eceb6e4 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -3043,8 +3043,8 @@ FunctionDecl::FunctionDecl(Kind DK, ASTContext &C, DeclContext *DC,
                            Expr *TrailingRequiresClause)
     : DeclaratorDecl(DK, DC, NameInfo.getLoc(), NameInfo.getName(), T, TInfo,
                      StartLoc),
-      DeclContext(DK), redeclarable_base(C), Body(), ODRHash(0),
-      EndRangeLoc(NameInfo.getEndLoc()), DNLoc(NameInfo.getInfo()) {
+      DeclContext(DK), redeclarable_base(C), Body(), DeletedMessage(nullptr),
+      ODRHash(0), EndRangeLoc(NameInfo.getEndLoc()), DNLoc(NameInfo.getInfo()) {
   assert(T.isNull() || T->isFunctionType());
   FunctionDeclBits.SClass = S;
   FunctionDeclBits.IsInline = isInlineSpecified;
diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp
index b701581b2474a9..d04cdd5a10e033 100644
--- a/clang/lib/AST/DeclPrinter.cpp
+++ b/clang/lib/AST/DeclPrinter.cpp
@@ -873,9 +873,14 @@ void DeclPrinter::VisitFunctionDecl(FunctionDecl *D) {
 
   if (D->isPureVirtual())
     Out << " = 0";
-  else if (D->isDeletedAsWritten())
+  else if (D->isDeletedAsWritten()) {
     Out << " = delete";
-  else if (D->isExplicitlyDefaulted())
+    if (const auto *M = D->getDeletedMessage()) {
+      Out << "(";
+      M->outputString(Out);
+      Out << ")";
+    }
+  } else if (D->isExplicitlyDefaulted())
     Out << " = default";
   else if (D->doesThisDeclarationHaveABody()) {
     if (!Policy.TerseOutput) {
diff --git a/clang/lib/AST/TextNodeDumper.cpp b/clang/lib/AST/TextNodeDumper.cpp
index b683eb1edd8f13..4a2369330a35e7 100644
--- a/clang/lib/AST/TextNodeDumper.cpp
+++ b/clang/lib/AST/TextNodeDumper.cpp
@@ -1936,6 +1936,9 @@ void TextNodeDumper::VisitFunctionDecl(const FunctionDecl *D) {
   if (D->isTrivial())
     OS << " trivial";
 
+  if (const auto *M = D->getDeletedMessage())
+    AddChild("delete message", [=] { Visit(M); });
+
   if (D->isIneligibleOrNotSelected())
     OS << (isa<CXXDestructorDecl>(D) ? " not_selected" : " ineligible");
 
diff --git a/clang/lib/Parse/ParseCXXInlineMethods.cpp b/clang/lib/Parse/ParseCXXInlineMethods.cpp
index d790060c17c049..9e2f4ce562a4ea 100644
--- a/clang/lib/Parse/ParseCXXInlineMethods.cpp
+++ b/clang/lib/Parse/ParseCXXInlineMethods.cpp
@@ -20,6 +20,28 @@
 
 using namespace clang;
 
+/// Parse the optional ("message") part of a deleted-function-body.
+StringLiteral *Parser::ParseCXXDeletedFunctionMessage() {
+  if (!Tok.is(tok::l_paren))
+    return nullptr;
+  StringLiteral *Message = nullptr;
+  BalancedDelimiterTracker BT{*this, tok::l_paren};
+  BT.consumeOpen();
+
+  if (isTokenStringLiteral()) {
+    ExprResult Res = ParseUnevaluatedStringLiteralExpression();
+    if (Res.isUsable())
+      Message = Res.getAs<StringLiteral>();
+  } else {
+    Diag(Tok.getLocation(), diag::err_expected_string_literal)
+        << /*Source='in'*/ 0 << "'delete'";
+    SkipUntil(tok::r_paren, StopAtSemi | StopBeforeMatch);
+  }
+
+  BT.consumeClose();
+  return Message;
+}
+
 /// ParseCXXInlineMethodDef - We parsed and verified that the specified
 /// Declarator is a well formed C++ inline method definition. Now lex its body
 /// and store its tokens for parsing after the C++ class is complete.
@@ -70,7 +92,8 @@ NamedDecl *Parser::ParseCXXInlineMethodDef(
                       ? diag::warn_cxx98_compat_defaulted_deleted_function
                       : diag::ext_defaulted_deleted_function)
         << 1 /* deleted */;
-      Actions.SetDeclDeleted(FnD, KWLoc);
+      StringLiteral *Message = ParseCXXDeletedFunctionMessage();
+      Actions.SetDeclDeleted(FnD, KWLoc, Message);
       Delete = true;
       if (auto *DeclAsFunction = dyn_cast<FunctionDecl>(FnD)) {
         DeclAsFunction->setRangeEnd(KWEndLoc);
diff --git a/clang/lib/Parse/Parser.cpp b/clang/lib/Parse/Parser.cpp
index cc0e41ed221c4f..d6f2b9f448cd52 100644
--- a/clang/lib/Parse/Parser.cpp
+++ b/clang/lib/Parse/Parser.cpp
@@ -1404,6 +1404,7 @@ Decl *Parser::ParseFunctionDefinition(ParsingDeclarator &D,
 
   // Parse function body eagerly if it is either '= delete;' or '= default;' as
   // ActOnStartOfFunctionDef needs to know whether the function is deleted.
+  StringLiteral *DeletedMessage = nullptr;
   Sema::FnBodyKind BodyKind = Sema::FnBodyKind::Other;
   SourceLocation KWLoc;
   if (TryConsumeToken(tok::equal)) {
@@ -1415,6 +1416,7 @@ Decl *Parser::ParseFunctionDefinition(ParsingDeclarator &D,
                       : diag::ext_defaulted_deleted_function)
           << 1 /* deleted */;
       BodyKind = Sema::FnBodyKind::Delete;
+      DeletedMessage = ParseCXXDeletedFunctionMessage();
     } else if (TryConsumeToken(tok::kw_default, KWLoc)) {
       Diag(KWLoc, getLangOpts().CPlusPlus11
                       ? diag::warn_cxx98_compat_defaulted_deleted_function
@@ -1473,7 +1475,7 @@ Decl *Parser::ParseFunctionDefinition(ParsingDeclarator &D,
   D.getMutableDeclSpec().abort();
 
   if (BodyKind != Sema::FnBodyKind::Other) {
-    Actions.SetFunctionBodyKind(Res, KWLoc, BodyKind);
+    Actions.SetFunctionBodyKind(Res, KWLoc, BodyKind, DeletedMessage);
     Stmt *GeneratedBody = Res ? Res->getBody() : nullptr;
     Actions.ActOnFinishFunctionBody(Res, GeneratedBody, false);
     return Res;
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index e258a4f7c89415..50eff9b02cbef8 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -18108,7 +18108,8 @@ NamedDecl *Sema::ActOnFriendFunctionDecl(Scope *S, Declarator &D,
   return ND;
 }
 
-void Sema::SetDeclDeleted(Decl *Dcl, SourceLocation DelLoc) {
+void Sema::SetDeclDeleted(Decl *Dcl, SourceLocation DelLoc,
+                          StringLiteral *Message) {
   AdjustDeclIfTemplate(Dcl);
 
   FunctionDecl *Fn = dyn_cast_or_null<FunctionDecl>(Dcl);
@@ -18157,7 +18158,7 @@ void Sema::SetDeclDeleted(Decl *Dcl, SourceLocation DelLoc) {
   // C++11 [dcl.fct.def.delete]p4:
   //  A deleted function is implicitly inline.
   Fn->setImplicitlyInline();
-  Fn->setDeletedAsWritten();
+  Fn->setDeletedWithMessage(Message);
 }
 
 void Sema::SetDeclDefaulted(Decl *Dcl, SourceLocation DefaultLoc) {
@@ -18270,11 +18271,11 @@ void Sema::DiagnoseReturnInConstructorExceptionHandler(CXXTryStmt *TryBlock) {
   }
 }
 
-void Sema::SetFunctionBodyKind(Decl *D, SourceLocation Loc,
-                               FnBodyKind BodyKind) {
+void Sema::SetFunctionBodyKind(Decl *D, SourceLocation Loc, FnBodyKind BodyKind,
+                               StringLiteral *DeletedMessage) {
   switch (BodyKind) {
   case FnBodyKind::Delete:
-    SetDeclDeleted(D, Loc);
+    SetDeclDeleted(D, Loc, DeletedMessage);
     break;
   case FnBodyKind::Default:
     SetDeclDefaulted(D, Loc);
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 8725b09f8546cf..76fbce8a95da85 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -271,8 +271,11 @@ bool Sema::DiagnoseUseOfDecl(NamedDecl *D, ArrayRef<SourceLocation> Locs,
         Diag(Loc, diag::err_deleted_inherited_ctor_use)
             << Ctor->getParent()
             << Ctor->getInheritedConstructor().getConstructor()->getParent();
-      else
-        Diag(Loc, diag::err_deleted_function_use);
+      else {
+        StringLiteral *Msg = FD->getDeletedMessage();
+        Diag(Loc, diag::err_deleted_function_use)
+            << !!Msg << (Msg ? Msg->getString() : StringRef());
+      }
       NoteDeletedFunction(FD);
       return true;
     }
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index c34a40fa7c81ac..e5b8d1fbb81554 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -2663,13 +2663,9 @@ static bool resolveAllocationOverload(
     return true;
 
   case OR_Deleted: {
-    if (Diagnose) {
-      Candidates.NoteCandidates(
-          PartialDiagnosticAt(R.getNameLoc(),
-                              S.PDiag(diag::err_ovl_deleted_call)
-                                  << R.getLookupName() << Range),
-          S, OCD_AllCandidates, Args);
-    }
+    if (Diagnose)
+      S.DiagnoseUseOfDeletedFunction(R.getNameLoc(), Range, R.getLookupName(),
+                                     Candidates, Best->Function, Args);
     return true;
   }
   }
@@ -3323,7 +3319,9 @@ bool Sema::FindDeallocationFunction(SourceLocation StartLoc, CXXRecordDecl *RD,
     // FIXME: DiagnoseUseOfDecl?
     if (Operator->isDeleted()) {
       if (Diagnose) {
-        Diag(StartLoc, diag::err_deleted_function_use);
+        StringLiteral *Msg = Operator->getDeletedMessage();
+        Diag(StartLoc, diag::err_deleted_function_use)
+            << !!Msg << (Msg ? Msg->getString() : StringRef());
         NoteDeletedFunction(Operator);
       }
       return true;
@@ -3927,14 +3925,11 @@ static bool resolveBuiltinNewDeleteOverload(Sema &S, CallExpr *TheCall,
         S, OCD_AmbiguousCandidates, Args);
     return true;
 
-  case OR_Deleted: {
-    Candidates.NoteCandidates(
-        PartialDiagnosticAt(R.getNameLoc(), S.PDiag(diag::err_ovl_deleted_call)
-                                                << R.getLookupName() << Range),
-        S, OCD_AllCandidates, Args);
+  case OR_Deleted:
+    S.DiagnoseUseOfDeletedFunction(R.getNameLoc(), Range, R.getLookupName(),
+                                   Candidates, Best->Function, Args);
     return true;
   }
-  }
   llvm_unreachable("Unreachable, bad result from BestViableFunction");
 }
 
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 4e03c3124e39ab..e4350df47c3570 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -14138,15 +14138,13 @@ static ExprResult FinishOverloadedCallExpr(Sema &SemaRef, Scope *S, Expr *Fn,
     break;
 
   case OR_Deleted: {
-    CandidateSet->NoteCandidates(
-        PartialDiagnosticAt(Fn->getBeginLoc(),
-                            SemaRef.PDiag(diag::err_ovl_deleted_call)
-                                << ULE->getName() << Fn->getSourceRange()),
-        SemaRef, OCD_AllCandidates, Args);
+    FunctionDecl *FDecl = (*Best)->Function;
+    SemaRef.DiagnoseUseOfDeletedFunction(Fn->getBeginLoc(),
+                                         Fn->getSourceRange(), ULE->getName(),
+                                         *CandidateSet, FDecl, Args);
 
     // We emitted an error for the unavailable/deleted function call but keep
     // the call in the AST.
-    FunctionDecl *FDecl = (*Best)->Function;
     ExprResult Res =
         SemaRef.FixOverloadedFunctionReference(Fn, (*Best)->FoundDecl, FDecl);
     if (Res.isInvalid())
@@ -15588,11 +15586,9 @@ ExprResult Sema::BuildCallToMemberFunction(Scope *S, Expr *MemExprE,
           *this, OCD_AmbiguousCandidates, Args);
       break;
     case OR_Deleted:
-      CandidateSet.NoteCandidates(
-          PartialDiagnosticAt(UnresExpr->getMemberLoc(),
-                              PDiag(diag::err_ovl_deleted_member_call)
-                                  << DeclName << MemExprE->getSourceRange()),
-          *this, OCD_AllCandidates, Args);
+      DiagnoseUseOfDeletedFunction(
+          UnresExpr->getMemberLoc(), MemExprE->getSourceRange(), DeclName,
+          CandidateSet, Best->Function, Args, /*IsMember=*/true);
       break;
     }
     // Overload resolution fails, try to recover.
@@ -16483,3 +16479,17 @@ bool clang::shouldEnforceArgLimit(bool PartialOverloading,
         return false;
   return true;
 }
+
+void Sema::DiagnoseUseOfDeletedFunction(SourceLocation Loc, SourceRange Range,
+                                        DeclarationName Name,
+                                        OverloadCandidateSet &CandidateSet,
+                                        FunctionDecl *Fn, MultiExprArg Args,
+                                        bool IsMember) {
+  StringLiteral *Msg = Fn->getDeletedMessage();
+  CandidateSet.NoteCandidates(
+      PartialDiagnosticAt(Loc, PDiag(diag::err_ovl_deleted_call)
+                                   << IsMember << Name << !!Msg
+                                   << (Msg ? Msg->getString() : StringRef())
+                                   << Range),
+      *this, OCD_AllCandidates, Args);
+}
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index dc972018e7b281..dfc6edc003e641 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -2429,7 +2429,7 @@ Decl *TemplateDeclInstantiator::VisitFunctionDecl(
       return nullptr;
   }
   if (D->isDeleted())
-    SemaRef.SetDeclDeleted(Function, D->getLocation());
+    SemaRef.SetDeclDeleted(Function, D->getLocation(), D->getDeletedMessage());
 
   NamedDecl *PrincipalDecl =
       (TemplateParams ? cast<NamedDecl>(FunctionTemplate) : Function);
@@ -2805,7 +2805,8 @@ Decl *TemplateDeclInstantiator::VisitCXXMethodDecl(
       return nullptr;
   }
   if (D->isDeletedAsWritten())
-    SemaRef.SetDeclDeleted(Method, Method->getLocation());
+    SemaRef.SetDeclDeleted(Method, Method->getLocation(),
+                           D->getDeletedMessage());
 
   // If this is an explicit specialization, mark the implicitly-instantiated
   // template specialization as being an explicit specialization too.
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index a22f760408c634..37bc61b9a63a34 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -1122,6 +1122,11 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
     }
   }
 
+  // FIXME: See ASTWriterDecl::VisitFunctionDecl.
+  if (FD->isDeletedAsWritten())
+    FD->setDeletedWithMessage(
+        cast_if_present<StringLiteral>(Record.readStmt()));
+
   if (Existing)
     mergeRedeclarable(FD, Existing, Redecl);
   else if (auto Kind = FD->getTemplatedKind();
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp
index 86f64bf2a24250..af7dbe916419eb 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -761,6 +761,13 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
     }
   }
 
+  // FIXME: Hack: We're out of bits in FunctionDeclBits, so always
+  // add this even though it's 0 in the vast majority of cases. We
+  // might really want to consider storing this in the DefaultedFunctionInfo
+  // instead.
+  if (D->isDeletedAsWritten())
+    Record.AddStmt(D->getDeletedMessage());
+
   Record.push_back(D->param_size());
   for (auto *P : D->parameters())
     Record.AddDeclRef(P);
diff --git a/clang/test/AST/ast-dump-cxx2c-delete-with-message.cpp b/clang/test/AST/ast-dump-cxx2c-delete-with-message.cpp
new file mode 100644
index 00000000000000..ea16b97da23e40
--- /dev/null
+++ b/clang/test/AST/ast-dump-cxx2c-delete-with-message.cpp
@@ -0,0 +1,23 @@
+// Without serialization:
+// RUN: %clang_cc1 -ast-dump %s | FileCheck %s
+//
+// With serialization:
+// RUN: %clang_cc1 -emit-pch -o %t %s
+// RUN: %clang_cc1 -x c++ -include-pch %t -ast-dump...
[truncated]

@Sirraide Sirraide changed the title [Clang] Implement P2573R2: = delete("should have a reason"); [Clang] [C++26] Implement P2573R2: = delete("should have a reason"); Mar 25, 2024
Copy link

✅ With the latest revision this PR passed the Python code formatter.

Copy link

github-actions bot commented Mar 25, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on that, I did a quick preliminary review

@cor3ntin
Copy link
Contributor

The last bullet point of your todo list should be done as a follow up PR. everything else can/should be done here.
I agree that back-porting this feature seems reasonable

@Sirraide
Copy link
Member Author

The last bullet point of your todo list should be done as a follow up PR.

Yeah, that was my thinking too more or less; that’s also why I listed it as ‘not necessary’—and I’m also not that familiar w/ libclang or AST matchers.

Comment on lines 768 to 769
if (D->isDeletedAsWritten())
Record.AddStmt(D->getDeletedMessage());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My instinct reaction to this is that we should better use AddString and readString pairs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to go with that, but I couldn’t find any examples of that being done for string literals (at least not in this file), so I wasn’t sure whether that’d be the right approach, but it should be fine since it’s just a string literal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand. Since the string literal is required to be unevaluated string, I feel it may be good to record the string value directly by StringLiteral::getBytes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand. Since the string literal is required to be unevaluated string, I feel it may be good to record the string value directly by StringLiteral::getBytes.

Hmm, from what I can tell, we don’t usually do that for string literals though. E.g. the string literal of a FileScopeASMDecl is also serialised as an expression:

void ASTDeclWriter::VisitFileScopeAsmDecl(FileScopeAsmDecl *D) {
VisitDecl(D);
Record.AddStmt(D->getAsmString());

Also, one issue that I see w/ only storing the string data is that we’d lose e.g. the source location of the string literal.

Sirraide and others added 3 commits March 26, 2024 14:31
@Sirraide
Copy link
Member Author

So, the message is now stored in the DefaultFunctionInfo, which I’ve renamed to ExtraFunctionInfo; because setting the deleted message may require allocating a DefaultFunctionInfo, I’ve added a separate function for that (FunctionDecl::setDeletedMessage()).

@Sirraide
Copy link
Member Author

Sirraide commented Apr 3, 2024

FWIW, I think that use of unreachable is wrong and an assertion would make more sense. It would be worth doing some archeology to see how we got the unreachable in the first place as to whether we should replace it with assert(false); (conservative approach) or assert(Ovl == OR_Deleted) (slightly more aggressive approach).

Yeah, it just seems to me that, generally, any code that follows this pattern:

if (condition) {
  // ... (assuming no control flow out of this 'if' here of course)
  llvm_unreachable("message");
}

should really just be

assert(!condition && "message");

My reasoning is simply this: With assertions enabled, we get a crash either way, and with assertions disabled, the if statement is going to be deleted anyway; if we replace llvm_unreachable with assert(false), then that’s going to get macroed out with assertions disabled, but that also means we have now reached a program state that was supposed to be impossible (if it wasn’t, and we can actually deal with that just fine, then why was the llvm_unreachable() there in the first place?), and sure, it wouldn’t be ideal if we ended up accepting incorrect code (or miscompiling correct code) as a result, but also, either of those is also just straight-up a bug, and I’m not sure how much sense trying to recover from a state that we were never supposed to be in in the first place makes...

But also, this isn’t really specific to this part of the codebase, and it’s also out of scope for this pr; we should probably come back to this at some point, though.

@Sirraide
Copy link
Member Author

Sirraide commented Apr 5, 2024

Tests need to be fleshed out, I'd like to see template specializations + partial template specializations as well as special member functions and operators.

Forgot to mention it explicitly, but I’ve added more tests and implemented this for all the other sema diagnostics that had anything to do w/ explicitly deleted functions (there are a few that say something along the lines of ‘function has been implicitly deleted’, but I didn’t really look at those since this is about explicitly deleting functions).

@cor3ntin
Copy link
Contributor

cor3ntin commented Apr 8, 2024

I wanted to make you aware of this new core issue https://cplusplus.github.io/CWG/issues/2876.html (which i think we should have tests for). Thanks

@Sirraide
Copy link
Member Author

Sirraide commented Apr 9, 2024

I wanted to make you aware of this new core issue https://cplusplus.github.io/CWG/issues/2876.html (which i think we should have tests for). Thanks

So, as of now, this

using T = void ();
using U = int;

T a = delete ("hello");
U b = delete ("hello"), c, d = delete ("hello");

struct C {
    T e = delete ("hello");
    U f = delete ("hello");
};

gives

test.cc:4:7: error: only functions can have deleted definitions
    4 | T a = delete ("hello");
      |       ^
test.cc:5:7: error: only functions can have deleted definitions
    5 | U b = delete ("hello"), c, d = delete ("hello");
      |       ^
test.cc:5:32: error: only functions can have deleted definitions
    5 | U b = delete ("hello"), c, d = delete ("hello");
      |                                ^
test.cc:8:8: error: '= delete' is a function definition and must occur in a standalone declaration
    8 |         T e = delete ("hello");
      |               ^
test.cc:9:8: error: cannot delete expression of type 'const char[6]'
    9 |         U f = delete ("hello");
      |               ^      ~~~~~~~~~

which seems reasonable to me—unless there’s something I’m missing here, but it’s ill-formed either way, so so long as we’re diagnosing it, we’re fine, from what I can tell at least.

One thing I did just now is made sure we discard the ("message") if we encounter = delete in a place where it doesn’t belong so we don’t issue two errors instead of one (there was a ‘missing semicolon at end of declaration’ error that imo is just noise in this context).

@Sirraide
Copy link
Member Author

Sirraide commented Apr 9, 2024

I’ve also added a parser test for that as well just now.

@cor3ntin
Copy link
Contributor

cor3ntin commented Apr 9, 2024

Thanks for the new tests. did you see this comment? #86526 (review)

@Sirraide
Copy link
Member Author

Sirraide commented Apr 9, 2024

did you see this comment? #86526 (review)

Thanks for reminding me; I did see that one but ended up forgetting about it because I was sick for pretty much all of last week; I’ll take a look at it later.

@Sirraide
Copy link
Member Author

Sirraide commented Apr 9, 2024

Just added the deleted message to the ODR hash as well. Not too familiar w/ ODR checking though, so I wasn’t sure whether to include it only if SkipBody is false.

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last batch of nitpicks, and then I think I'll be happy enough to approve this patch!

@Sirraide Sirraide force-pushed the delete-with-message branch from 4848200 to c12f90e Compare April 10, 2024 08:51
Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting up with my comments and working on this feature.
I'm pretty happy with the state of the PR.

Please give it a day or two in case @erichkeane @AaronBallman @shafik @Fznamznon have further comments

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 set of 'nits' a few places, else LGTM.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, this LGTM, thank you! I did spot some minor stuff, but feel free to fix it up when landing.

@Sirraide Sirraide merged commit ef164ce into llvm:main Apr 14, 2024
4 of 5 checks passed
bazuzi pushed a commit to bazuzi/llvm-project that referenced this pull request Apr 15, 2024
llvm#86526)

This implements support for the `= delete("message")` syntax that was
only just added to C++26
([P2573R2](https://isocpp.org/files/papers/P2573R2.html#proposal-scope)).
@Sirraide Sirraide deleted the delete-with-message branch April 23, 2024 18:11
void setDefaultedFunctionInfo(DefaultedFunctionInfo *Info);
DefaultedFunctionInfo *getDefaultedFunctionInfo() const;
void setDefaultedOrDeletedInfo(DefaultedOrDeletedFunctionInfo *Info);
DefaultedOrDeletedFunctionInfo *getDefalutedOrDeletedInfo() const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sirraide There's no reason we can't change this name to fix this typo right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That can be fixed as NFC commit. I missed it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sirraide There's no reason we can't change this name to fix this typo right?

Ah, my bad, yeah, that should just be a NFC fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++26 clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants