[Wdocumentation] fixes an assertion failure with typedefed function and block pointer

[Mordante]

Summary

The assertion happens when compiling with -Wdocumentation with variable declaration to a typedefed function pointer. I not too familiar with the ObjC syntax but first two tests assert without this patch.

Fixes https://bugs.llvm.org/show_bug.cgi?id=42844

[gribozavr]

clang/lib/AST/Comment.cpp

151

Why is the new functionality turned off sometimes? It seems to me that we always want to look through typedefs.

[Mordante]

clang/lib/AST/Comment.cpp

151

Setting testTypedefTypeLoc to true breaks a unit test in test/Sema/warn-documentation.cpp:358

typedef int (*test_not_function_like_typedef1)(int aaa);

// expected-warning@+1 {{'\param' command used in a comment that is not attached to a function declaration}}
/// \param aaa Meow.                                                            
typedef test_not_function_like_typedef1 test_not_function_like_typedef2;

and its corresponding test using a using instead of typedef. This has been introduced in:

commit 49fdf8d3f5e114e6f8b49fde29a30b0eaaa3c5dd
Author: Dmitri Gribenko <[e-mail]>
Date:   Sat Sep 15 21:13:36 2012 +0000

    Comment parsing: don't treat typedef to a typedef to a function as a
    'function-like' type that can be annotated with \param.
    
    Thanks to Eli Friedman for noticing!
    
    llvm-svn: 163985

I'm not sure whether or not we should allow this typedef documentation. I just tested with Doxygen. It doesn't complain and shows the parameter documentation for test_not_function_like_typedef2. So on that basis we could remove this expected-warning and testTypedefTypeLoc.

What do you think?

[gribozavr]

clang/lib/AST/Comment.cpp

151

Thanks for the explanation. I can't find the context for that decision, and the commit description does not explain the reason either.

It is really a policy decision -- when introducing a comment for function return value and arguments, should the declaration include the said return value and arguments, or can they be visible through a typedef?

Thinking about it, my inclination is to say that comments for the return value and arguments should be present on the declaration that introduces them. Otherwise, it is breaking an abstraction barrier.

WDYT?

[Mordante]

clang/lib/AST/Comment.cpp

151

(I just noticed I forgot to submit this comment.)

I tend to agree. However Doxygen accepts the code, but Doxygen doesn't seem to validate whether parameters exist.
So I wonder whether we do our users a service by warning about code that Doxygen properly handles. On that basis I think we can allow it.

We could add an extra warning in the pedantic group and let the user decide whether or not she thinks it warrants a warning.

I think that would be the best solution: accept the comment but allow the user to opt-in on a warning.

[gribozavr]

clang/lib/AST/Comment.cpp

151

Doxygen and C++ both allow a lot of dubious stuff. That can't be a reason to avoid producing a warning -- we would have no warnings if that was the bar.

I think what we should discuss is, what is the use case for adding a \returns command in this position -- and how is documenting parameters not a use case. If there is a valid use case, how common it is in real world.

[Mordante]

As discussed on IRC no longer allow \return on a typedef'ed type. This is consistent with \param.

• Reverts all prior changes in lib/AST/Comment.cpp
• Adds extra tests in Sema::isFunctionOrBlockPointerVarLikeDecl
• Updates unit tests

[gribozavr]

LGTM, but could you also verify that references are not an issue?

using D = void();
D &d = ...; ///< \return none

[Mordante]

Adds extra unit tests for references.

[Mordante]

Thanks for the review.

using D = void();
D &d = ...; ///< \return none

Is no issue, I added tests for it.

Can you commit the code for me?

[gribozavr]

Thanks!

clang/lib/AST/Comment.cpp

if (const TextComment *TC = dyn_cast<TextComment>(*I)) {
       if (!TC->isWhitespace())
         return false;
     } else
       return false;
   }
   return true;
 }
 
-static TypeLoc lookThroughTypedefOrTypeAliasLocs(TypeLoc &SrcTL) {
+static TypeLoc lookThroughTypedefOrTypeAliasLocs(TypeLoc &SrcTL,
+                                                 bool testTypedefTypeLoc) {
   TypeLoc TL = SrcTL.IgnoreParens();
 
   // Look through attribute types.
   if (AttributedTypeLoc AttributeTL = TL.getAs<AttributedTypeLoc>())
     return AttributeTL.getModifiedLoc();
   // Look through qualified types.
   if (QualifiedTypeLoc QualifiedTL = TL.getAs<QualifiedTypeLoc>())
     return QualifiedTL.getUnqualifiedLoc();
   // Look through pointer types.
   if (PointerTypeLoc PointerTL = TL.getAs<PointerTypeLoc>())
     return PointerTL.getPointeeLoc().getUnqualifiedLoc();
   // Look through reference types.
   if (ReferenceTypeLoc ReferenceTL = TL.getAs<ReferenceTypeLoc>())
     return ReferenceTL.getPointeeLoc().getUnqualifiedLoc();
   // Look through adjusted types.
   if (AdjustedTypeLoc ATL = TL.getAs<AdjustedTypeLoc>())
     return ATL.getOriginalLoc();
   if (BlockPointerTypeLoc BlockPointerTL = TL.getAs<BlockPointerTypeLoc>())
     return BlockPointerTL.getPointeeLoc().getUnqualifiedLoc();
   if (MemberPointerTypeLoc MemberPointerTL = TL.getAs<MemberPointerTypeLoc>())
     return MemberPointerTL.getPointeeLoc().getUnqualifiedLoc();
   if (ElaboratedTypeLoc ETL = TL.getAs<ElaboratedTypeLoc>())
     return ETL.getNamedTypeLoc();
+  if (testTypedefTypeLoc)
+    if (TypedefTypeLoc TTL = TL.getAs<TypedefTypeLoc>()) {
+      TypedefNameDecl *TND = TTL.getTypedefNameDecl();
+      if (const TypeSourceInfo *TSI = TND->getTypeSourceInfo())
+        return TSI->getTypeLoc().getUnqualifiedLoc();
+    }
 
   return TL;
 }
 
-static bool getFunctionTypeLoc(TypeLoc TL, FunctionTypeLoc &ResFTL) {
+static bool getFunctionTypeLoc(TypeLoc TL, FunctionTypeLoc &ResFTL,
+                               bool testTypedefTypeLoc = false) {
   TypeLoc PrevTL;
   while (PrevTL != TL) {
     PrevTL = TL;
-    TL = lookThroughTypedefOrTypeAliasLocs(TL);
+    TL = lookThroughTypedefOrTypeAliasLocs(TL, testTypedefTypeLoc);
   }
 
   if (FunctionTypeLoc FTL = TL.getAs<FunctionTypeLoc>()) {
     ResFTL = FTL;
     return true;
   }
 
   if (TemplateSpecializationTypeLoc STL =
if (const auto *VD = dyn_cast<DeclaratorDecl>(CommentDecl))
       TSI = VD->getTypeSourceInfo();
     else if (const auto *PD = dyn_cast<ObjCPropertyDecl>(CommentDecl))
       TSI = PD->getTypeSourceInfo();
     else
       TSI = nullptr;
     if (TSI) {
       TypeLoc TL = TSI->getTypeLoc().getUnqualifiedLoc();
       FunctionTypeLoc FTL;
-      if (getFunctionTypeLoc(TL, FTL)) {
+      // Need to look through the typedef when
+      // Sema::isFunctionOrBlockPointerVarLikeDecl found a FunctionPointerType
+      // or a BlockPointerType else the ReturnType will not be set.
+      if (getFunctionTypeLoc(TL, FTL, true)) {
         ParamVars = FTL.getParams();
         ReturnType = FTL.getReturnLoc().getType();
       }
     }
     Kind = VariableKind;
     break;
   }
   case Decl::Namespace:

clang/lib/AST/CommentSema.cpp

void Sema::checkReturnsCommand(const BlockCommandComment *Command) {
 
   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()) {
+    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;
         else
           DiagKind = 0;

clang/test/Sema/warn-documentation.cpp

 // expected-warning@+2 {{empty paragraph passed to '@note' command}}
 /**
 @note
 \f$\texttt{mu}_{00}=\texttt{m}_{00}\f$, \f$\texttt{nu}_{00}=1\f$
 \f$\texttt{nu}_{10}=\texttt{mu}_{10}=\texttt{mu}_{01}=\texttt{mu}_{10}=0\f$
  */
 class EmptyNoteNoCrash {
 };
+
+namespace PR42844 { // Assertion failures when using typedefed function pointers
+typedef void (*AA)();
+typedef AA A();
+A *a1; ///< \return none
+A a2;  ///< \return none
+
+typedef int B();
+B *b; ///< \return none
+
+typedef void C();
+C *c; ///< \return none
+// expected-warning@-1 {{'\return' command used in a comment that is attached to a function returning void}}
+
+using DD = void(*)();
+using D = DD();
+D *d1; ///< \return none
+D d2;  ///< \return none
+
+using E = int();
+E *e; ///< \return none
+
+using F = void();
+F *f; ///< \return none
+// expected-warning@-1 {{'\return' command used in a comment that is attached to a function returning void}}
+
+} // namespace PR42844

clang/test/Sema/warn-documentation.m

  *
  * @param a
  * works
  *
  * @param ...
  * now should work too.
  */
 typedef void (^VariadicBlockType)(int a, ...);
+
+// PR42844 - Assertion failures when using typedefed block pointers
+typedef void(^VoidBlockType)();
+typedef VoidBlockType VoidBlockTypeCall();
+VoidBlockTypeCall *d; ///< \return none
+VoidBlockTypeCall ^e; ///< \return none
+VoidBlockTypeCall f;  ///< \return none