Function with unparsed body is a definition

[sepavloff]

Summary

While a function body is being parsed, the function declaration is not considered
as a definition because it does not have a body yet. In some cases it leads to
incorrect interpretation, the case is presented in
https://bugs.llvm.org/show_bug.cgi?id=14785:

    template<typename T> struct Somewhat {
      void internal() const {}
      friend void operator+(int const &, Somewhat<T> const &) {}
    };
void operator+(int const &, Somewhat<char> const &x) { x.internal(); }

When statement x.internal() in the body of global operator+ is parsed, the type
of x must be completed, so the instantiation of Somewhat<char> is started. It
instantiates the declaration of operator+ defined inline, and makes a check for
redefinition. The check does not detect another definition because the declaration
of operator+ is still not defining as does not have a body yet.

To solves this problem the function isThisDeclarationADefinition considers
a function declaration as a definition if it has flag WillHaveBody set.

This change fixes PR14785.

[jlebar]

This doesn't look wrong to me, but I feel utterly unqualified to review this.

[sepavloff]

Rebased patch

[rsmith]

If you want to take this path, you should also add ActOnStartOfFunctionBody calls to the other parts of the parser that parse function definitions: Parser::ParseLateTemplatedFuncDef, Parser::ParseLexedObjCMethodDefs, and Parser::ParseLexedMethodDefs. You should also rename ActOnFinishFunctionBody to something like ActOnFinishFunctionDef to avoid confusion, because it pairs with ActOnStartOfFunctionDef not the new ActOnStartFunctionBody.

However, I wonder if there isn't a simpler way to handle this, rather than adding another function that is called almost every time that ActOnStartOfFunctionDef is called: the Declarator object passed to ActOnStartOfFunctionDef already carries a FunctionDefinitionKind which tells you whether the function definition has a body. You could extend the non-Declarator overload to also take a FunctionDefinitionKind and then update the callers to pass in the right value. Then in ActOnStartOfFunctionDef, only set WillHaveBody if the kind is FDK_Definition.

[sepavloff]

Thank you for explanation. Probably making new semantic action is an overkill.

The intent was to postpone setting flag WillHaveBody as late as possible, to the moment when it becomes clear that the function indeed will have a body. Otherwise parsing of deleted functions is broken. Also in the case of invalid input the resulting function declaration become definition while previously they were not, it also changes existing behavior. The new semantic action set the flag enough late after the call to ActOnStartOfFunctionBody, when this treatment is already made.

The new solution uses different path. As previously, ActOnStartOfFunctionBody sets WillHaveBody but this flag is reset when it becomes clear that body will be available. Such technique is already uses for defaulted functions, so only deleted require the flag reset. WillHaveBody is also reset when parsing of function is finished. This transient flag is not needed after the function parsing is done. If ActOnFinishFunctionBody is always called after ActOnStartOfFunctionBody, erroneous functions are not leaked as definition.

[sepavloff]

Changed implementation

[rsmith]

LGTM as-is, but a couple of questions for possible further improvements.

lib/Sema/SemaDecl.cpp

12221 ↗

(On Diff #101993)

Would it make sense to leave this true for a defaulted function whose body has not yet been synthesized?

lib/Sema/SemaTemplateInstantiateDecl.cpp

1812–1819 ↗

(On Diff #101993)

Can we replace this...

1841–1853 ↗

(On Diff #101993)

... and this with

SemaRef.CheckForFunctionRedefinition(Function);
Function->setWillHaveBody(true);

?

[sepavloff]

Thank you!
Committed as is, improvements will be made as separate patches.

[sepavloff]

Initial commit (r305379) was reverted (r305381) because it broke self builds. The reason was not related to the WillHaveBody flag but was due to the change:

if (isFriend) {
  Function->setObjectOfFriendDecl();
  if (FunctionTemplate)
    FunctionTemplate->setObjectOfFriendDecl();
}

Attempt to execute FunctionTemplate->setObjectOfFriendDecl() caused assertion violation because the declaration had IDNS_NonMemberOperator in its IdentifierNamespace. There is nothing wrong with this flag, setObjectOfFriendDecl must be updated accordingly. This is however a problem of friend function templates, it will be addressed in other patch. For now the call to setObjectOfFriendDecl is made as previously, with the exception that it is called for Function prior to call to CheckFunctionDeclaration, as it is necessary for proper diagnostics.

cfe/trunk/include/clang/AST/Decl.h

public:
   /// Returns whether this specific declaration of the function is also a
   /// definition that does not contain uninstantiated body.
   ///
   /// This does not determine whether the function has been defined (e.g., in a
   /// previous definition); for that information, use isDefined.
   ///
   bool isThisDeclarationADefinition() const {
     return IsDeleted || IsDefaulted || Body || IsLateTemplateParsed ||
-      hasDefiningAttr();
+      WillHaveBody || hasDefiningAttr();
   }
 
   /// doesThisDeclarationHaveABody - Returns whether this specific
   /// declaration of the function has a body - that is, if it is a non-
   /// deleted definition.
   bool doesThisDeclarationHaveABody() const {
     return Body || IsLateTemplateParsed;
   }

cfe/trunk/lib/Sema/SemaCUDA.cpp

static bool IsKnownEmitted(Sema &S, FunctionDecl *FD) {
   // Check whether this function is externally visible -- if so, it's
   // known-emitted.
   //
   // We have to check the GVA linkage of the function's *definition* -- if we
   // only have a declaration, we don't know whether or not the function will be
   // emitted, because (say) the definition could include "inline".
   FunctionDecl *Def = FD->getDefinition();
 
-  // We may currently be parsing the body of FD, in which case
-  // FD->getDefinition() will be null, but we still want to treat FD as though
-  // it's a definition.
-  if (!Def && FD->willHaveBody())
-    Def = FD;
-
   if (Def &&
       !isDiscardableGVALinkage(S.getASTContext().GetGVALinkageForFunction(Def)))
     return true;
 
   // Otherwise, the function is known-emitted if it's in our set of
   // known-emitted functions.
   return S.CUDAKnownEmittedFns.count(FD) > 0;
 }

cfe/trunk/lib/Sema/SemaDecl.cpp

Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body,
   sema::AnalysisBasedWarnings::Policy WP = AnalysisWarnings.getDefaultPolicy();
   sema::AnalysisBasedWarnings::Policy *ActivePolicy = nullptr;
 
   if (getLangOpts().CoroutinesTS && getCurFunction()->isCoroutine())
     CheckCompletedCoroutineBody(FD, Body);
 
   if (FD) {
     FD->setBody(Body);
+    FD->setWillHaveBody(false);
 
     if (getLangOpts().CPlusPlus14) {
       if (!FD->isInvalidDecl() && Body && !FD->isDependentContext() &&
           FD->getReturnType()->isUndeducedType()) {
         // If the function has a deduced result type but contains no 'return'
         // statements, the result type as written must be exactly 'auto', and
         // the deduced result type is 'void'.
         if (!FD->getReturnType()->getAs<AutoType>()) {

cfe/trunk/lib/Sema/SemaDeclCXX.cpp

void Sema::SetDeclDeleted(Decl *Dcl, SourceLocation DelLoc) {
   AdjustDeclIfTemplate(Dcl);
 
   FunctionDecl *Fn = dyn_cast_or_null<FunctionDecl>(Dcl);
   if (!Fn) {
     Diag(DelLoc, diag::err_deleted_non_function);
     return;
   }
 
+  // Deleted function does not have a body.
+  Fn->setWillHaveBody(false);
+
   if (const FunctionDecl *Prev = Fn->getPreviousDecl()) {
     // Don't consider the implicit declaration we generate for explicit
     // specializations. FIXME: Do not generate these implicit declarations.
     if ((Prev->getTemplateSpecializationKind() != TSK_ExplicitSpecialization ||
          Prev->getPreviousDecl()) &&
         !Prev->isDefined()) {
       Diag(DelLoc, diag::err_deleted_decl_not_first);
       Diag(Prev->getLocation().isInvalid() ? DelLoc : Prev->getLocation(),

cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp

if (DependentFunctionTemplateSpecializationInfo *Info
     // In C++, the previous declaration we find might be a tag type
     // (class or enum). In this case, the new declaration will hide the
     // tag type. Note that this does does not apply if we're declaring a
     // typedef (C++ [dcl.typedef]p4).
     if (Previous.isSingleTagDecl())
       Previous.clear();
   }
 
+  if (isFriend) {
+    Function->setObjectOfFriendDecl();
+    if (FunctionTemplate)
+      FunctionTemplate->setObjectOfFriendDecl();
+  }
+
   SemaRef.CheckFunctionDeclaration(/*Scope*/ nullptr, Function, Previous,
                                    isExplicitSpecialization);
 
   NamedDecl *PrincipalDecl = (TemplateParams
                               ? cast<NamedDecl>(FunctionTemplate)
                               : Function);
 
   // If the original function was part of a friend declaration,
   // inherit its namespace state and add it to the owner.
   if (isFriend) {
-    PrincipalDecl->setObjectOfFriendDecl();
     DC->makeDeclVisibleInContext(PrincipalDecl);
 
     bool QueuedInstantiation = false;
 
     // C++11 [temp.friend]p4 (DR329):
     //   When a function is defined in a friend function declaration in a class
     //   template, the function is instantiated when the function is odr-used.
     //   The same restrictions on multiple declarations and definitions that

cfe/trunk/test/SemaCXX/friend2.cpp

 template <typename T>
 struct Test {
   friend void Function() { }
 };
 
 template class Test<int>;
 
 }
+
+namespace pr14785 {
+template<typename T>
+struct Somewhat {
+  void internal() const { }
+  friend void operator+(int const &, Somewhat<T> const &) {}  // expected-error{{redefinition of 'operator+'}}
+};
+
+void operator+(int const &, Somewhat<char> const &x) {  // expected-note {{previous definition is here}}
+  x.internal();  // expected-note{{in instantiation of template class 'pr14785::Somewhat<char>' requested here}}
+}
+}