[Clang] CWG 1394: Incomplete types as parameters of deleted functions

[rZhBoYao]

Summary

According to CWG 1394, dcl.fct.def.general p2 and this StackOverflow answer, Clang should not diagnose incomplete types if function body is "= delete;".

For example:

struct Incomplete;
Incomplete f(Incomplete) = delete; // well-formed

Inspired by this tweet.

Also close PR52802.

[ChuanqiXu]

clang/lib/Sema/SemaDecl.cpp

14473–14474

14477

I think we could remove the use of FnDeleted by the use of FD->isDeleted(). Also we should constrain the behavior only if std >= 11.

[rZhBoYao]

clang/lib/Parse/Parser.cpp

1376–1377

I'm thinking should we merge FnDeleted and Deleted into one variable or leave it as is? On the one hand, Deleted's scope is tight so very readable. On the other hand, it seems redundant.

1376–1377

FunctionDecl::setDeletedAsWritten not called until this SetDeclDeleted call.

clang/lib/Sema/SemaDecl.cpp

14473–14474

I think the sentence is not yet complete there so I mimic the old one on the left.

14477

I tried FD->isDeleted() at first only to find out it always returns false. Looks like it's not handled until Parser.cpp:1342

Also, looks like deleted functions are implemented as an extension. A warning is issued at Parser.cpp:1338 if language mode < C++11

[ChuanqiXu]

clang/lib/Sema/SemaDecl.cpp

14477

I see. My suggestion might be to defer the diagnose message after we set FD->setDeletedAsWritten(). I understand it might be harder. But the current implementation looks a little bit not good... (redundant variables and passing information by argument)

[rZhBoYao]

I think an extra parameter is inevitable without complicating things too much.

[erichkeane]

clang/include/clang/Sema/Sema.h

2920

I tend to prefer not doing bool params, and instead some type of 'enum class', as that makes it more clear at the call site.

clang/lib/Parse/Parser.cpp

1341

I'm not sure about doing this 'look ahead' here, this feels dangerous to me. First, does this work with comments? Second, it seems we wouldn't normally look at 'deleted' if SkipBody.ShouldSkip (see below with the early exit)?

Next I'm not a fan of double-parsing these tokens with this lookahead. I wonder, if we should move hte logic from ~1334 and 1338 up here and calculate the 'deleted'/'defaulted' 'earlier', before we 'actOnStartOfFunctionDef`.

This would result in us being able to instead change the signature of ActOnStartOfFunctionDef to take some enum as to whether it is deleted/defaulted, AND create the function decl as deleted/defaulted 'in place' (or, at least, call SetDeclDeleted or SetDeclDefaulted immediately).

clang/lib/Sema/SemaDecl.cpp

14477

I disagree on doing this for C++11 mode. As we allow them as an extension, we want this to work in the extension mode as well.

[ChuanqiXu]

In D122981#3432233, @rZhBoYao wrote:

I think an extra parameter is inevitable without complicating things too much.

My thought is that it might not be worth to fix the problem for the workaround. I feel like the problem is less countered. Let's try to fix it in a cleaner way.

[rZhBoYao]

clang/lib/Parse/Parser.cpp

1341

I'm not sure about doing this 'look ahead' here, this feels dangerous to me. First, does this work with comments?

Yes, it returns a normal token after phase 5, so comments are long gone.

Second, it seems we wouldn't normally look at 'deleted' if SkipBody.ShouldSkip (see below with the early exit)?

SkipBody.ShouldSkip is an output parameter of ActOnStartOfFunctionDef. We need to either look ahead or consume "delete" before entering ActOnStartOfFunctionDef anyway.

Next I'm not a fan of double-parsing these tokens with this lookahead.

It does look weird. Consume them I will. Updated diff coming.

AND create the function decl as deleted/defaulted 'in place' (or, at least, call SetDeclDeleted or SetDeclDefaulted immediately).

SetDecl{Deleted | Defaulted} needs KWLoc tho. I haven't thought of a way of doing that "in place" inside ActOnStartOfFunctionDef.

clang/lib/Sema/SemaDecl.cpp

14477

Thanks for the endorsement.

[erichkeane]

clang/lib/Parse/Parser.cpp

1341

My point is: do that parsing in this function BEFORE the call to ActOnStartOfFunctionDef?

Alternatively, we could add a function to Sema to 'ActOnFunctionDefinition(DefType)' and move this diagnostic THERE instead of ActOnStartofFunctionDef, and call it AFTER we have handled the '= delete/=default/etc'.

[rZhBoYao]

clang/lib/Parse/Parser.cpp

1341

do that parsing in this function BEFORE the call to ActOnStartOfFunctionDef?

But we need Decl *Res returned by ActOnStartOfFunctionDef.

I did try to factor out the diagnostic right after ActOnFunctionDefinition and 27 tests were failing.
Furthermore, we are not the only caller of ActOnFunctionDefinition.

[rZhBoYao]

Handling of eagerly parsed deleted or defaulted function must happen AFTER D.complete(Res);.

A nicer looking diff: https://github.com/poyaoc97/llvm-project/commit/dc37a262582f6a2d8143c6f1f586dc657b4b5311

[rZhBoYao]

clang/lib/Parse/Parser.cpp

1341

I meant to say ActOnStartOfFunctionDef.

[erichkeane]

This definitely looks like it is 'nicer' than before, a few smaller/nit-ish comments.

Additionally, Phab made a REAL mess of the diff, if you could give a quick summary of what changes were actually made (vs things that were moved slightly and causing massive red/green diffs), it would be helpful.

clang/include/clang/Sema/Sema.h

2906

2910

Might suggest 'Other'?

Also, the EqDelete and EqDefault could probably just be Delete/Default and be equally as clear.

clang/lib/Parse/Parser.cpp

1363

Since the FnBodyKind is in Sema, I would suggest just adding a new function to replace all of this, "SetFunctionBodyKind(Res, BodyKind)", and make Sema do the 'switch' here.

clang/lib/Sema/SemaDecl.cpp

14473–14474

This is C++ specific, so could we be specific about that here?

[rZhBoYao]

Removed member function test cases and addressed comments,
which includes:

1. Sema::SetFunctionBodyKind
2. Change enum names
3. Be clear about delete being C++ specific.

A nicer looking diff: https://github.com/poyaoc97/llvm-project/commit/f594442f9614
It seems that GitHub has a VERY different way of displaying diff.

Tests passed on my machine (x86_64 Ubuntu)

[rZhBoYao]

Updating the status of CWG 1394 is the only change.

[rZhBoYao]

Added release note.

Also, push to my repo to make sure the release note is correctly rendered:
https://github.com/poyaoc97/llvm-project/commit/1167f0fc6b91

[ChuanqiXu]

I agree this one is better.

clang/include/clang/Sema/Sema.h

2899–2909

Agree to @erichkeane

[rZhBoYao]

clang/include/clang/Sema/Sema.h

2899–2909

With all due respect, this code suggestion doesn't make any sense to me. My best guess is @ChuanqiXu was thinking the order specified by the grammar as noted in dcl.fct.def.general p1. Even if that was the case, CompoundStmt is not quite right either. Also, differentiating ctor-initializer[opt] compound-statement and function-try-block is meaningless here, hence the name Other.

I adopted the same order as to how Parser::ParseFunctionDefinition has always been parsing function-body. The order is not significant in any meaningful way as each of the 4 grammar productions of function-body is VERY different and mutually exclusive. Putting Delete and Default upfront not only emphasizes the "specialness" of them but also conveys how we handle function-body.

What say you, @erichkeane ?

[ChuanqiXu]

clang/include/clang/Sema/Sema.h

2899–2909

Yeah, the order comes from the standard. I think the comment should be consistent with the spec. And for the name, I agree CompoundStmt is not accurate indeed... I thought Default should be a good name but there is Default already. But I don't feel Other is good since the compound-statement is much more common.

Putting Delete and Default upfront not only emphasizes the "specialness" of them but also conveys how we handle function-body.

This don't makes sense. Generally, we would put common items in front. And the order here shouldn't be related to the implementation.

[rZhBoYao]

clang/include/clang/Sema/Sema.h

2899–2909

But I don't feel Other is good since the compound-statement is much more common.

Other reads "not special like Delete and Default".

This don't makes sense. Generally, we would put common items in front. And the order here shouldn't be related to the implementation.

Think of it as a not-so-special else clause at the end of an if/else chain. We tend to process special cases upfront. It is only natural.

I'll await @erichkeane 's final verdict.

[erichkeane]

I'm going to accept, I am ok with it as-is, though I would like to see @ChuanqiXu be accepting of whatever we do in Sema's enum as well. So please wait for his feedback too.

clang/include/clang/Sema/Sema.h

2899–2909

Think of it as a not-so-special else clause at the end of an if/else chain. We tend to process special cases upfront. It is only natural.

This was my understanding. This is a single value that means 'not default and not deleted'. I think the idea of doing it in standards order is a really good one, but have the top 2 just both result in Other, Unknown, etc. So something like:

//ctor-initializeropt compound-statement
//function-try-block
Unknown,
//= default ;
Default,
//= delete ;
Delete

BUT as this is not an 'ast' level flag, it is just for the purposes of simplifying this call, I don't think it needs to conform that closely.

[rZhBoYao]

Reordered enumerators.

Does this look good to you, @ChuanqiXu ?

I will land this later next week if no feedback is received.

[rZhBoYao]

Revisiting @ChuanqiXu 's code suggestion...

clang/include/clang/Sema/Sema.h

2899–2909

I actually don't know what you were trying to say here:

We don't yet part function-try-block yet.
FunctionTryBlock,

especially:

We don't yet part function-try-block yet.

Nonetheless, I did my best to cater to your comment within reason, such as reordering enumerators to align with the C++ standard text.

[ChuanqiXu]

LGTM then.

clang/include/clang/Sema/Sema.h

DeclGroupPtrTy FinalizeDeclaratorGroup(Scope *S, const DeclSpec &DS,
                                          ArrayRef<Decl *> Group);
   DeclGroupPtrTy BuildDeclaratorGroup(MutableArrayRef<Decl *> Group);
 
   /// Should be called on all declarations that might have attached
   /// documentation comments.
   void ActOnDocumentableDecl(Decl *D);
   void ActOnDocumentableDecls(ArrayRef<Decl *> Group);
 
+  enum class FnBodyKind {
+    /// C++ [dcl.fct.def.general]p1
+    /// function-body:
+    ///   = delete ;
+    ///   = default ;
+    Delete,
+    Default,
+
+    /// Could be one of:
+    ///   ctor-initializer[opt] compound-statement
+    ///   function-try-block
+    Other
+  };
+
   void ActOnFinishKNRParamDeclarations(Scope *S, Declarator &D,
                                        SourceLocation LocAfterDecls);
   void CheckForFunctionRedefinition(
       FunctionDecl *FD, const FunctionDecl *EffectiveDefinition = nullptr,
       SkipBodyInfo *SkipBody = nullptr);
   Decl *ActOnStartOfFunctionDef(Scope *S, Declarator &D,
                                 MultiTemplateParamsArg TemplateParamLists,
-                                SkipBodyInfo *SkipBody = nullptr);
+                                SkipBodyInfo *SkipBody = nullptr,
+                                FnBodyKind BodyKind = FnBodyKind::Other);
   Decl *ActOnStartOfFunctionDef(Scope *S, Decl *D,
-                                SkipBodyInfo *SkipBody = nullptr);
+                                SkipBodyInfo *SkipBody = nullptr,
+                                FnBodyKind BodyKind = FnBodyKind::Other);
+  void SetFunctionBodyKind(Decl *D, SourceLocation Loc, FnBodyKind BodyKind);
   void ActOnStartTrailingRequiresClause(Scope *S, Declarator &D);
   ExprResult ActOnFinishTrailingRequiresClause(ExprResult ConstraintExpr);
   ExprResult ActOnRequiresClause(ExprResult ConstraintExpr);
   void ActOnStartOfObjCMethodDef(Scope *S, Decl *D);
   bool isObjCMethodDecl(Decl *D) {
     return D && isa<ObjCMethodDecl>(D);
   }
 

clang/lib/Parse/Parser.cpp

else if (CurParsedObjCImpl &&
     }
     // FIXME: Should we really fall through here?
   }
 
   // Enter a scope for the function body.
   ParseScope BodyScope(this, Scope::FnScope | Scope::DeclScope |
                                  Scope::CompoundStmtScope);
 
-  // Tell the actions module that we have entered a function definition with the
-  // specified Declarator for the function.
-  Sema::SkipBodyInfo SkipBody;
-  Decl *Res = Actions.ActOnStartOfFunctionDef(getCurScope(), D,
-                                              TemplateInfo.TemplateParams
-                                                  ? *TemplateInfo.TemplateParams
-                                                  : MultiTemplateParamsArg(),
-                                              &SkipBody);
-
-  if (SkipBody.ShouldSkip) {
-    SkipFunctionBody();
-    return Res;
-  }
-
-  // Break out of the ParsingDeclarator context before we parse the body.
-  D.complete(Res);
-
-  // Break out of the ParsingDeclSpec context, too.  This const_cast is
-  // safe because we're always the sole owner.
-  D.getMutableDeclSpec().abort();
-
-  // With abbreviated function templates - we need to explicitly add depth to
-  // account for the implicit template parameter list induced by the template.
-  if (auto *Template = dyn_cast_or_null<FunctionTemplateDecl>(Res))
-    if (Template->isAbbreviated() &&
-        Template->getTemplateParameters()->getParam(0)->isImplicit())
-      // First template parameter is implicit - meaning no explicit template
-      // parameter list was specified.
-      CurTemplateDepthTracker.addDepth(1);
-
+  // Parse function body eagerly if it is either '= delete;' or '= default;' as
+  // ActOnStartOfFunctionDef needs to know whether the function is deleted.
+  Sema::FnBodyKind BodyKind = Sema::FnBodyKind::Other;
+  SourceLocation KWLoc;
   if (TryConsumeToken(tok::equal)) {
     assert(getLangOpts().CPlusPlus && "Only C++ function definitions have '='");
 
-    bool Delete = false;
-    SourceLocation KWLoc;
     if (TryConsumeToken(tok::kw_delete, KWLoc)) {
       Diag(KWLoc, getLangOpts().CPlusPlus11
                       ? diag::warn_cxx98_compat_defaulted_deleted_function
                       : diag::ext_defaulted_deleted_function)
           << 1 /* deleted */;
-      Actions.SetDeclDeleted(Res, KWLoc);
-      Delete = true;
+      BodyKind = Sema::FnBodyKind::Delete;
     } else if (TryConsumeToken(tok::kw_default, KWLoc)) {
       Diag(KWLoc, getLangOpts().CPlusPlus11
                       ? diag::warn_cxx98_compat_defaulted_deleted_function
                       : diag::ext_defaulted_deleted_function)
           << 0 /* defaulted */;
-      Actions.SetDeclDefaulted(Res, KWLoc);
+      BodyKind = Sema::FnBodyKind::Default;
     } else {
       llvm_unreachable("function definition after = not 'delete' or 'default'");
     }
 
     if (Tok.is(tok::comma)) {
       Diag(KWLoc, diag::err_default_delete_in_multiple_declaration)
-        << Delete;
+          << (BodyKind == Sema::FnBodyKind::Delete);
       SkipUntil(tok::semi);
     } else if (ExpectAndConsume(tok::semi, diag::err_expected_after,
-                                Delete ? "delete" : "default")) {
+                                BodyKind == Sema::FnBodyKind::Delete
+                                    ? "delete"
+                                    : "default")) {
       SkipUntil(tok::semi);
     }
+  }
+
+  // Tell the actions module that we have entered a function definition with the
+  // specified Declarator for the function.
+  Sema::SkipBodyInfo SkipBody;
+  Decl *Res = Actions.ActOnStartOfFunctionDef(getCurScope(), D,
+                                              TemplateInfo.TemplateParams
+                                                  ? *TemplateInfo.TemplateParams
+                                                  : MultiTemplateParamsArg(),
+                                              &SkipBody, BodyKind);
+
+  if (SkipBody.ShouldSkip) {
+    // Do NOT enter SkipFunctionBody if we already consumed the tokens.
+    if (BodyKind == Sema::FnBodyKind::Other)
+      SkipFunctionBody();
 
+    return Res;
+  }
+
+  // Break out of the ParsingDeclarator context before we parse the body.
+  D.complete(Res);
+
+  // Break out of the ParsingDeclSpec context, too.  This const_cast is
+  // safe because we're always the sole owner.
+  D.getMutableDeclSpec().abort();
+
+  if (BodyKind != Sema::FnBodyKind::Other) {
+    Actions.SetFunctionBodyKind(Res, KWLoc, BodyKind);
     Stmt *GeneratedBody = Res ? Res->getBody() : nullptr;
     Actions.ActOnFinishFunctionBody(Res, GeneratedBody, false);
     return Res;
   }
 
+  // With abbreviated function templates - we need to explicitly add depth to
+  // account for the implicit template parameter list induced by the template.
+  if (auto *Template = dyn_cast_or_null<FunctionTemplateDecl>(Res))
+    if (Template->isAbbreviated() &&
+        Template->getTemplateParameters()->getParam(0)->isImplicit())
+      // First template parameter is implicit - meaning no explicit template
+      // parameter list was specified.
+      CurTemplateDepthTracker.addDepth(1);
+
   if (SkipFunctionBodies && (!Res || Actions.canSkipFunctionBody(Res)) &&
       trySkippingFunctionBody()) {
     BodyScope.Exit();
     Actions.ActOnSkippedFunctionBody(Res);
     return Actions.ActOnFinishFunctionBody(Res, nullptr, false);
   }
 
   if (Tok.is(tok::kw_try))
     return ParseFunctionTryBlock(Res, BodyScope);

clang/lib/Sema/SemaDecl.cpp

for (int i = FTI.NumParams; i != 0; /* decrement in loop */) {
       }
     }
   }
 }
 
 Decl *
 Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Declarator &D,
                               MultiTemplateParamsArg TemplateParameterLists,
-                              SkipBodyInfo *SkipBody) {
+                              SkipBodyInfo *SkipBody, FnBodyKind BodyKind) {
   assert(getCurFunctionDecl() == nullptr && "Function parsing confused");
   assert(D.isFunctionDeclarator() && "Not a function declarator!");
   Scope *ParentScope = FnBodyScope->getParent();
 
   // Check if we are in an `omp begin/end declare variant` scope. If we are, and
   // we define a non-templated function definition, we will create a declaration
   // instead (=BaseFD), and emit the definition with a mangled name afterwards.
   // The base function declaration will have the equivalent of an `omp declare
   // variant` annotation which specifies the mangled definition as a
   // specialization function under the OpenMP context defined as part of the
   // `omp begin declare variant`.
   SmallVector<FunctionDecl *, 4> Bases;
   if (LangOpts.OpenMP && isInOpenMPDeclareVariantScope())
     ActOnStartOfFunctionDefinitionInOpenMPDeclareVariantScope(
         ParentScope, D, TemplateParameterLists, Bases);
 
   D.setFunctionDefinitionKind(FunctionDefinitionKind::Definition);
   Decl *DP = HandleDeclarator(ParentScope, D, TemplateParameterLists);
-  Decl *Dcl = ActOnStartOfFunctionDef(FnBodyScope, DP, SkipBody);
+  Decl *Dcl = ActOnStartOfFunctionDef(FnBodyScope, DP, SkipBody, BodyKind);
 
   if (!Bases.empty())
     ActOnFinishedFunctionDefinitionInOpenMPDeclareVariantScope(Dcl, Bases);
 
   return Dcl;
 }
 
 void Sema::ActOnFinishInlineFunctionDef(FunctionDecl *D) {
if (C.capturesVariable()) {
       LSI->addVLATypeCapture(C.getLocation(), I->getCapturedVLAType(),
                              I->getType());
     }
     ++I;
   }
 }
 
 Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D,
-                                    SkipBodyInfo *SkipBody) {
+                                    SkipBodyInfo *SkipBody,
+                                    FnBodyKind BodyKind) {
   if (!D) {
     // Parsing the function declaration failed in some way. Push on a fake scope
     // anyway so we can try to parse the function body.
     PushFunctionScope();
     PushExpressionEvaluationContext(ExprEvalContexts.back().Context);
     return D;
   }
 
Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D,
   if (unsigned BuiltinID = FD->getBuiltinID()) {
     if (!Context.BuiltinInfo.isPredefinedLibFunction(BuiltinID) &&
         !Context.BuiltinInfo.isPredefinedRuntimeFunction(BuiltinID)) {
       Diag(FD->getLocation(), diag::err_builtin_definition) << FD;
       FD->setInvalidDecl();
     }
   }
 
-  // The return type of a function definition must be complete
-  // (C99 6.9.1p3, C++ [dcl.fct]p6).
+  // The return type of a function definition must be complete (C99 6.9.1p3),
+  // unless the function is deleted (C++ specifc, C++ [dcl.fct.def.general]p2)
   QualType ResultType = FD->getReturnType();
   if (!ResultType->isDependentType() && !ResultType->isVoidType() &&
-      !FD->isInvalidDecl() &&
+      !FD->isInvalidDecl() && BodyKind != FnBodyKind::Delete &&
       RequireCompleteType(FD->getLocation(), ResultType,
                           diag::err_func_def_incomplete_result))
     FD->setInvalidDecl();
 
   if (FnBodyScope)
     PushDeclContext(FnBodyScope, FD);
 
   // Check the validity of our function parameters
+  if (BodyKind != FnBodyKind::Delete)
     CheckParmsForFunctionDef(FD->parameters(),
                              /*CheckParameterNames=*/true);
 
   // Add non-parameter declarations already in the function to the current
   // scope.
   if (FnBodyScope) {
     for (Decl *NPD : FD->decls()) {
       auto *NonParmDecl = dyn_cast<NamedDecl>(NPD);
       if (!NonParmDecl)
         continue;

clang/lib/Sema/SemaDeclCXX.cpp

 
 void Sema::DiagnoseReturnInConstructorExceptionHandler(CXXTryStmt *TryBlock) {
   for (unsigned I = 0, E = TryBlock->getNumHandlers(); I != E; ++I) {
     CXXCatchStmt *Handler = TryBlock->getHandler(I);
     SearchForReturnInStmt(*this, Handler);
   }
 }
 
+void Sema::SetFunctionBodyKind(Decl *D, SourceLocation Loc,
+                               FnBodyKind BodyKind) {
+  switch (BodyKind) {
+  case FnBodyKind::Delete:
+    SetDeclDeleted(D, Loc);
+    break;
+  case FnBodyKind::Default:
+    SetDeclDefaulted(D, Loc);
+    break;
+  case FnBodyKind::Other:
+    llvm_unreachable(
+        "Parsed function body should be '= delete;' or '= default;'");
+  }
+}
+
 bool Sema::CheckOverridingFunctionAttributes(const CXXMethodDecl *New,
                                              const CXXMethodDecl *Old) {
   const auto *NewFT = New->getType()->castAs<FunctionProtoType>();
   const auto *OldFT = Old->getType()->castAs<FunctionProtoType>();
 
   if (OldFT->hasExtParameterInfos()) {
     for (unsigned I = 0, E = OldFT->getNumParams(); I != E; ++I)
       // A parameter of the overriding method should be annotated with noescape

clang/test/CXX/dcl.decl/dcl.fct.def/dcl.fct.def.general/p2.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct Incomplete;                 // expected-note 2{{forward declaration of 'Incomplete'}}
+Incomplete f(Incomplete) = delete; // well-formed
+Incomplete g(Incomplete) {}        // expected-error{{incomplete result type 'Incomplete' in function definition}}\
+// expected-error{{variable has incomplete type 'Incomplete'}}