Allow getRawCommentForDecl to find comments in macros

[danakj]

Summary

The key part of getRawCommentForDecl() required to find a comment
is determining where to look for it. The location of the decl
itself is usually right, except when macros get involved. The
comment in the macro is stored in RawCommentList at the spelling
location of the decl, not at the place where the decl comes into
being as the macro is instantiated.

getDeclLocForCommentSearch() already contained to branches to try
handle comments inside macros, and we are able to replace them
and handle more cases as well, by returning the spelling location
of the decl's begin location. That is:

SourceMgr.getSpellingLoc(D->getBeginLoc())

[danakj]

Adding back a period that was removed from a comment.

clang/test/Index/annotate-comments-objc.m

121

The comment is no longer attached to the enums generated by NS_ENUM, it is attached to the decl which is found in the text directly after the comment, which is the typedef, only.

[danakj]

Ensure the comment outside the macro is attached to enums defined
inside NS_ENUM() and NS_OPTIONS() macros.

clang/test/Index/annotate-comments-objc.m

121

Restored binding the comment outside the macro to the enums defined inside NS_ENUM() and NS_OPTIONS()

[danakj]

Improve the comment

[danakj]

Add tests for enums inside a macro, other than NS_ENUM and NS_OPTIONS

[Jake-Egan]

Hi, this change causes a failure on AIX. Could you take a look?
https://lab.llvm.org/buildbot/#/builders/214/builds/5502/steps/6/logs/FAIL__Clang__annotate-comments-objc_m

[danakj]

Here's the relevant part of the test:

#define DECLARE_ENUMS(name) \
  /** enumFromMacro IS_DOXYGEN_SINGLE */ \
  enum enumFromMacro { A }; \
  /** namedEnumFromMacro IS_DOXYGEN_SINGLE */ \
  enum name { B };

/// IS_DOXYGEN_NOT_ATTACHED
DECLARE_ENUMS(namedEnumFromMacro)

From the error I see:

• The Decl is enum namedEnumFromMacro, which has a source location at 74:1, and has a spelling location at 74:15.

DECLARE_ENUMS(namedEnumFromMacro)
^ source      ^ spelling

• We then call Decl::getBeginLoc(), which doesn't move the source location itself, but does move spelling location.

#define DECLARE_ENUMS(name) \
  /** enumFromMacro IS_DOXYGEN_SINGLE */ \
  enum enumFromMacro { A }; \
  /** namedEnumFromMacro IS_DOXYGEN_SINGLE */ \
  enum name { B };
  ^ spelling

/// IS_DOXYGEN_NOT_ATTACHED
DECLARE_ENUMS(namedEnumFromMacro)
^ source

It seems (from reading the logs) that Decl::getBeginLoc() does not have the correct spelling location in this test, on AIX. It is staying at the original source location at the macro expansion.

[danakj]

I am not sure how to debug further for AIX, do you have any suggestion?

In case it is helpful, I would debug it by adding these at the bottom of the DeclLoc.isMacroID() branch in getDeclLocForCommentSearch() in ASTContext.cpp:

// The location (and spelling location) of the decl.
DeclLoc.dump(SourceMgr);
// The spelling location dumped should move to inside the macro.
D->getBeginLoc().dump(SourceMgr);

[danakj]

I tried modifying the test to pass an AIX target, running on Linux (thanks thakis for the suggestion):

// RUN: %clang_cc1 -triple powerpc64-ibm-aix -emit-pch -o %t/out.pch -F %S/Inputs/Frameworks %s
// RUN: %clang_cc1 -triple powerpc64-ibm-aix -include-pch %t/out.pch -F %S/Inputs/Frameworks -fsyntax-only %s

However the test passes.

[Jake-Egan]

Thank you for looking into the failure. Fortunately the test is passing on the AIX bot now and locally for me - it looks like a clean build fixed it.

[bnbarham]

clang/test/Index/annotate-comments-objc.m

67–74

What was the original motivation for the PR here? Was it the DECLARE_FUNCTIONS case above?

For this case, personally I would expect that something like:

#define DEFINE_TAG(name) \
  <anytagdecl> name { .... };

/// Comment attached to foo
DEFINE_TAG(foo)

/// Comment attached to bar
DEFINE_TAG(bar)

Is far more common than the test case here. To put another way, the previous

if (const auto *TD = dyn_cast<TagDecl>(D)) {
  // If location of the tag decl is inside a macro, but the spelling of
  // the tag name comes from a macro argument, it looks like a special
  // macro like NS_ENUM is being used to define the tag decl.  In that
  // case, adjust the source location to the expansion loc so that we can
  // attach the comment to the tag decl.
  if (SourceMgr.isMacroArgExpansion(DeclLoc) && TD->isCompleteDefinition())
    return SourceMgr.getExpansionLoc(DeclLoc);
}

Seems more reasonable to me than the hardcoded macro-name check. Even NS_*-wise there's also NS_ERROR_ENUM, NS_TYPED_ENUM, NS_TYPED_EXTENSIBLE_ENUM, NS_CLOSED_ENUM.

clang/lib/AST/ASTContext.cpp

if (isa<ObjCMethodDecl>(D) || isa<ObjCContainerDecl>(D) ||
       isa<RedeclarableTemplateDecl>(D) ||
       isa<ClassTemplateSpecializationDecl>(D) ||
       // Allow association with Y across {} in `typedef struct X {} Y`.
       isa<TypedefDecl>(D))
     return D->getBeginLoc();
 
   const SourceLocation DeclLoc = D->getLocation();
   if (DeclLoc.isMacroID()) {
-    if (isa<TypedefDecl>(D)) {
-      // If location of the typedef name is in a macro, it is because being
-      // declared via a macro. Try using declaration's starting location as
-      // the "declaration location".
-      return D->getBeginLoc();
+    // There are (at least) three types of macros we care about here.
+    //
+    // 1. Macros that are used in the definition of a type outside the macro,
+    //    with a comment attached at the macro call site.
+    //    ```
+    //    // Comment is here, where we use the macro.
+    //    struct MAKE_NAME(Foo) {
+    //        int a;
+    //        int b;
+    //    };
+    //    ```
+    // 2. Macros that define whole things along with the comment.
+    //    ```
+    //    #define MAKE_METHOD(name) \
+    //      /** Comment is here, inside the macro. */ \
+    //      void name() {}
+    //
+    //    struct S {
+    //      MAKE_METHOD(f)
+    //    }
+    //    ```
+    // 3. Macros that both declare a type and name a decl outside the macro.
+    //    ```
+    //    // Comment is here, where we use the macro.
+    //    typedef NS_ENUM(NSInteger, Size) {
+    //        SizeWidth,
+    //        SizeHeight
+    //    };
+    //    ```
+    //    In this case NS_ENUM declares am enum type, and uses the same name for
+    //    the typedef declaration that appears outside the macro. The comment
+    //    here should be applied to both declarations inside and outside the
+    //    macro.
+    //
+    // We have found a Decl name that comes from inside a macro, but
+    // Decl::getLocation() returns the place where the macro is being called.
+    // If the declaration (and not just the name) resides inside the macro,
+    // then we want to map Decl::getLocation() into the macro to where the
+    // declaration and its attached comment (if any) were written.
+    //
+    // This mapping into the macro is done by mapping the location to its
+    // spelling location, however even if the declaration is inside a macro,
+    // the name's spelling can come from a macro argument (case 2 above). In
+    // this case mapping the location to the spelling location finds the
+    // argument's position (at `f` in MAKE_METHOD(`f`) above), which is not
+    // where the declaration and its comment are located.
+    //
+    // To avoid this issue, we make use of Decl::getBeginLocation() instead.
+    // While the declaration's position is where the name is written, the
+    // comment is always attached to the begining of the declaration, not to
+    // the name.
+    //
+    // In the first case, the begin location of the decl is outside the macro,
+    // at the location of `typedef`. This is where the comment is found as
+    // well. The begin location is not inside a macro, so it's spelling
+    // location is the same.
+    //
+    // In the second case, the begin location of the decl is the call to the
+    // macro, at `MAKE_METHOD`. However its spelling location is inside the
+    // the macro at the location of `void`. This is where the comment is found
+    // again.
+    //
+    // In the third case, there's no correct single behaviour. We want to use
+    // the comment outside the macro for the definition that's inside the macro.
+    // There is also a definition outside the macro, and we want the comment to
+    // apply to both. The cases we care about here is NS_ENUM() and
+    // NS_OPTIONS(). In general, if an enum is defined inside a macro, we should
+    // try to find the comment there.
+
+    // This is handling case 3 for NS_ENUM() and NS_OPTIONS(), which define
+    // enum types inside the macro.
+    if (isa<EnumDecl>(D)) {
+      SourceLocation MacroCallLoc = SourceMgr.getExpansionLoc(DeclLoc);
+      if (auto BufferRef =
+              SourceMgr.getBufferOrNone(SourceMgr.getFileID(MacroCallLoc));
+          BufferRef.has_value()) {
+        llvm::StringRef buffer = BufferRef->getBuffer().substr(
+            SourceMgr.getFileOffset(MacroCallLoc));
+        if (buffer.starts_with("NS_ENUM(") ||
+            buffer.starts_with("NS_OPTIONS(")) {
+          // We want to use the comment on the call to NS_ENUM and NS_OPTIONS
+          // macros for the types defined inside the macros, which is at the
+          // expansion location.
+          return MacroCallLoc;
+        }
       }
-
-    if (const auto *TD = dyn_cast<TagDecl>(D)) {
-      // If location of the tag decl is inside a macro, but the spelling of
-      // the tag name comes from a macro argument, it looks like a special
-      // macro like NS_ENUM is being used to define the tag decl.  In that
-      // case, adjust the source location to the expansion loc so that we can
-      // attach the comment to the tag decl.
-      if (SourceMgr.isMacroArgExpansion(DeclLoc) && TD->isCompleteDefinition())
-        return SourceMgr.getExpansionLoc(DeclLoc);
     }
+    return SourceMgr.getSpellingLoc(D->getBeginLoc());
   }
 
   return DeclLoc;
 }
 
 RawComment *ASTContext::getRawCommentForDeclNoCacheImpl(
     const Decl *D, const SourceLocation RepresentativeLocForDecl,
     const std::map<unsigned, RawComment *> &CommentsInTheFile) const {

clang/test/Index/annotate-comments-objc.m

 typedef NS_ENUM(int, An_NS_ENUM_isdoxy1) { Red, Green, Blue };
 
 // In the implementation of attaching comments to enums declared using the
 // NS_ENUM macro, it is tempting to use the fact that enum decl is embedded in
 // the typedef.  Make sure that the heuristic is strong enough that it does not
 // attach unrelated comments in the following cases where tag decls are
 // embedded in declarators.
 
-#define DECLARE_FUNCTION() \
+#define DECLARE_FUNCTIONS(suffix) \
+    /** functionFromMacro IS_DOXYGEN_SINGLE */ \
     void functionFromMacro(void) { \
       typedef struct Struct_notdoxy Struct_notdoxy; \
+    } \
+    /** functionFromMacroWithSuffix IS_DOXYGEN_SINGLE */ \
+    void functionFromMacro##suffix(void) { \
+      typedef struct Struct_notdoxy Struct_notdoxy; \
     }
 
 /// IS_DOXYGEN_NOT_ATTACHED
-DECLARE_FUNCTION()
+DECLARE_FUNCTIONS(WithSuffix)
 
 /// typedef_isdoxy1 IS_DOXYGEN_SINGLE
 typedef struct Struct_notdoxy *typedef_isdoxy1;
 
 #endif
 
 // RUN: rm -rf %t
 // RUN: mkdir %t
 // RUN: mkdir %t/module-cache
 
 // Check that we serialize comment source locations properly.
 // RUN: %clang_cc1 -emit-pch -o %t/out.pch -F %S/Inputs/Frameworks %s
 // RUN: %clang_cc1 -include-pch %t/out.pch -F %S/Inputs/Frameworks -fsyntax-only %s
 
 // RUN: c-index-test -write-pch %t/out.pch -F %S/Inputs/Frameworks %s
 // RUN: c-index-test -test-load-source all -comments-xml-schema=%S/../../bindings/xml/comment-xml-schema.rng %s -F %S/Inputs/Frameworks > %t/out.c-index-direct
 // RUN: c-index-test -test-load-source all -comments-xml-schema=%S/../../bindings/xml/comment-xml-schema.rng %s -F %S/Inputs/Frameworks -fmodules -fmodules-cache-path=%t/module-cache > %t/out.c-index-modules
 // RUN: c-index-test -test-load-tu %t/out.pch all -F %S/Inputs/Frameworks > %t/out.c-index-pch
 
 // RUN: FileCheck %s -check-prefix=WRONG < %t/out.c-index-direct

 // CHECK: annotate-comments-objc.m:23:50: ObjCPropertyDecl=property1_isdoxy1:{{.*}} property1_isdoxy1 IS_DOXYGEN_SINGLE
 // CHECK: annotate-comments-objc.m:24:50: ObjCPropertyDecl=property1_isdoxy2:{{.*}} property1_isdoxy2 IS_DOXYGEN_SINGLE
 // CHECK: annotate-comments-objc.m:25:50: ObjCPropertyDecl=property1_isdoxy3:{{.*}} property1_isdoxy3 IS_DOXYGEN_SINGLE
 // CHECK: annotate-comments-objc.m:26:50: ObjCPropertyDecl=property1_isdoxy4:{{.*}} property1_isdoxy4 IS_DOXYGEN_SINGLE
 // CHECK: annotate-comments-objc.m:29:9: ObjCInstanceMethodDecl=method1_isdoxy1:{{.*}} method1_isdoxy1 IS_DOXYGEN_SINGLE
 // CHECK: annotate-comments-objc.m:30:9: ObjCInstanceMethodDecl=method1_isdoxy2:{{.*}} method1_isdoxy2 IS_DOXYGEN_SINGLE
 // CHECK: annotate-comments-objc.m:31:9: ObjCInstanceMethodDecl=method1_isdoxy3:{{.*}} method1_isdoxy3 IS_DOXYGEN_SINGLE
 // CHECK: annotate-comments-objc.m:32:9: ObjCInstanceMethodDecl=method1_isdoxy4:{{.*}} method1_isdoxy4 IS_DOXYGEN_SINGLE
 // CHECK: annotate-comments-objc.m:43:22: EnumDecl=An_NS_ENUM_isdoxy1:{{.*}} An_NS_ENUM_isdoxy1 IS_DOXYGEN_SINGLE
 // CHECK: annotate-comments-objc.m:43:22: TypedefDecl=An_NS_ENUM_isdoxy1:{{.*}} An_NS_ENUM_isdoxy1 IS_DOXYGEN_SINGLE
 // CHECK: annotate-comments-objc.m:43:22: EnumDecl=An_NS_ENUM_isdoxy1:{{.*}} An_NS_ENUM_isdoxy1 IS_DOXYGEN_SINGLE
-// CHECK: annotate-comments-objc.m:60:32: TypedefDecl=typedef_isdoxy1:{{.*}} typedef_isdoxy1 IS_DOXYGEN_SINGLE
-
+// CHECK: annotate-comments-objc.m:62:1: FunctionDecl=functionFromMacro:{{.*}} BriefComment=[functionFromMacro IS_DOXYGEN_SINGLE]
+// CHECK: annotate-comments-objc.m:62:1: FunctionDecl=functionFromMacroWithSuffix:{{.*}} BriefComment=[functionFromMacroWithSuffix IS_DOXYGEN_SINGLE]
+// CHECK: annotate-comments-objc.m:65:32: TypedefDecl=typedef_isdoxy1:{{.*}} typedef_isdoxy1 IS_DOXYGEN_SINGLE