Comment AST: Declare function pointer variables as functions

[aaronpuchert]

Summary

We were doing this already for type aliases, and it deduplicates the
code looking through aliases and pointers to find a function type. As
a side effect, this finds two warnings that we apparently missed before.

[gribozavr2]

I feel uneasy about claiming that variable decls of function type are FunctionKind for the purposes of comment parsing (even doing it for typedefs is questionable). It seems like a better way would be to make "is function like" an extra dimension that can be combined with any decl kind.

However since no tests break, it might be not an issue in practice.

[aaronpuchert]

In D111264#3115104, @gribozavr2 wrote:

I feel uneasy about claiming that variable decls of function type are FunctionKind for the purposes of comment parsing (even doing it for typedefs is questionable). It seems like a better way would be to make "is function like" an extra dimension that can be combined with any decl kind.

Agreed, I'll try to write a patch for that. Essentially we want to know if some kind of function type (or maybe just FunctionProtoType?) is involved in that declaration (without desugaring), either of a type declaration, or of the declared type of a non-type declaration. That's pretty much orthogonal to the kind. (Though it's of course incompatible with some kinds, such as EnumKind or NamespaceKind.)

I'm wondering about global lambdas declarations. These should have class type, but also have parameters like a function.

clang/include/clang/AST/Comment.h

enum DeclKind {
     /// Something that we consider a "function":
     /// \li function,
     /// \li function template,
     /// \li function template specialization,
     /// \li member function,
     /// \li member function template,
     /// \li member function template specialization,
     /// \li ObjC method,
+    /// \li variable of function pointer, member function pointer or block type,
     /// \li a typedef for a function pointer, member function pointer,
     ///     ObjC block.
     FunctionKind,
 
     /// Something that we consider a "class":
     /// \li class/struct,
     /// \li class template,
     /// \li class template (partial) specialization.

clang/include/clang/AST/CommentSema.h

public:
   void resolveParamCommandIndexes(const FullComment *FC);
 
   bool isFunctionDecl();
   bool isAnyFunctionDecl();
 
   /// \returns \c true if declaration that this comment is attached to declares
   /// a function pointer.
   bool isFunctionPointerVarDecl();
-  /// \returns \c true if the declaration that this comment is attached to
-  /// declares a variable or a field whose type is a function or a block
-  /// pointer.
-  bool isFunctionOrBlockPointerVarLikeDecl();
   bool isFunctionOrMethodVariadic();
   bool isObjCMethodDecl();
   bool isObjCPropertyDecl();
   bool isTemplateOrSpecialization();
   bool isRecordLikeDecl();
   bool isClassOrStructDecl();
   /// \return \c true if the declaration that this comment is attached to
   /// declares either struct, class or tag typedef.

clang/lib/AST/Comment.cpp

void DeclInfo::fill() {
   }
 
   // If the type is a typedef / using to something we consider a function,
   // extract arguments and return type.
   if (TSI) {
     TypeLoc TL = TSI->getTypeLoc().getUnqualifiedLoc();
     FunctionTypeLoc FTL;
     if (getFunctionTypeLoc(TL, FTL)) {
-      if (Kind == TypedefKind)
-        Kind = FunctionKind;
       ParamVars = FTL.getParams();
       ReturnType = FTL.getReturnLoc().getType();
     }
   }
 
   IsFilled = true;
 }
 

clang/lib/AST/CommentSema.cpp

ParamCommandComment *Sema::actOnParamCommandStart(
                                       SourceLocation LocBegin,
                                       SourceLocation LocEnd,
                                       unsigned CommandID,
                                       CommandMarkerKind CommandMarker) {
   ParamCommandComment *Command =
       new (Allocator) ParamCommandComment(LocBegin, LocEnd, CommandID,
                                           CommandMarker);
 
-  if (!isFunctionDecl() && !isFunctionOrBlockPointerVarLikeDecl())
+  if (!isFunctionDecl())
     Diag(Command->getLocation(),
          diag::warn_doc_param_not_attached_to_a_function_decl)
       << CommandMarker
       << Command->getCommandNameRange(Traits);
 
   return Command;
 }
 
if (!Traits.getCommandInfo(Command->getCommandID())->IsReturnsCommand)
     return;
 
   assert(ThisDeclInfo && "should not call this check on a bare comment");
 
   // We allow the return command for all @properties because it can be used
   // to document the value that the property getter returns.
   if (isObjCPropertyDecl())
     return;
-  if (isFunctionDecl() || isFunctionOrBlockPointerVarLikeDecl()) {
+  if (isFunctionDecl()) {
     assert(!ThisDeclInfo->ReturnType.isNull() &&
            "should have a valid return type");
     if (ThisDeclInfo->ReturnType->isVoidType()) {
       unsigned DiagKind;
       switch (ThisDeclInfo->CommentDecl->getKind()) {
       default:
         if (ThisDeclInfo->IsObjCMethod)
           DiagKind = 3;
if (ThisDeclInfo->getKind() == DeclInfo::VariableKind) {
     if (const VarDecl *VD = dyn_cast_or_null<VarDecl>(ThisDeclInfo->CurrentDecl)) {
       QualType QT = VD->getType();
       return QT->isFunctionPointerType();
     }
   }
   return false;
 }
 
-bool Sema::isFunctionOrBlockPointerVarLikeDecl() {
-  if (!ThisDeclInfo)
-    return false;
-  if (!ThisDeclInfo->IsFilled)
-    inspectThisDecl();
-  if (ThisDeclInfo->getKind() != DeclInfo::VariableKind ||
-      !ThisDeclInfo->CurrentDecl)
-    return false;
-  QualType QT;
-  if (const auto *VD = dyn_cast<DeclaratorDecl>(ThisDeclInfo->CurrentDecl))
-    QT = VD->getType();
-  else if (const auto *PD =
-               dyn_cast<ObjCPropertyDecl>(ThisDeclInfo->CurrentDecl))
-    QT = PD->getType();
-  else
-    return false;
-  // We would like to warn about the 'returns'/'param' commands for
-  // variables that don't directly specify the function type, so type aliases
-  // can be ignored.
-  if (QT->getAs<TypedefType>())
-    return false;
-  if (const auto *P = QT->getAs<PointerType>())
-    if (P->getPointeeType()->getAs<TypedefType>())
-      return false;
-  if (const auto *P = QT->getAs<BlockPointerType>())
-    if (P->getPointeeType()->getAs<TypedefType>())
-      return false;
-  return QT->isFunctionPointerType() || QT->isBlockPointerType();
-}
-
 bool Sema::isObjCPropertyDecl() {
   if (!ThisDeclInfo)
     return false;
   if (!ThisDeclInfo->IsFilled)
     inspectThisDecl();
   return ThisDeclInfo->CurrentDecl->getKind() == Decl::ObjCProperty;
 }
 

clang/test/Sema/warn-documentation.cpp

struct HasFields {
    * functionPointerField
    *
    * @param i is integer.
    * @returns integer.
    */
   int (*functionPointerField)(int i);
 };
 
+// expected-warning@+5 {{parameter 'p' not found in the function declaration}}
 // expected-warning@+5 {{'\returns' command used in a comment that is attached to a function returning void}}
 /**
  * functionPointerVariable
  *
  * \param p not here.
  * \returns integer.
  */
 void (*functionPointerVariableThatLeadsNowhere)();

clang/test/Sema/warn-documentation.m

struct HasFields {
    * blockPointerField
    *
    * \param i is integer.
    * \returns integer.
    */
   int (^blockPointerFields)(int i);
 };
 
+// expected-warning@+5 {{parameter 'p' not found in the function declaration}}
 // expected-warning@+5 {{'\returns' command used in a comment that is attached to a function returning void}}
 /**
  * functionPointerVariable
  *
  * \param p not here.
  * \returns integer.
  */
 void (^_Nullable blockPointerVariableThatLeadsNowhere)();