[clang] Prioritze decl comments from macro expansion site

[daniel-grumberg]

For declarations declared inside a macro, e.g.:

`#define MAKE_FUNC(suffix) \
         /// Not selected doc comment \
         void func_##suffix(void) {  }

/// Doc comment foo
MAKE_FUNC(foo)

/// Doc comment bar
MAKE_FUNC(bar)

Prefer the doc comment at the expansion site instead of the one defined in the macro.

rdar://113995729

[daniel-grumberg]

This is a follow up on https://reviews.llvm.org/D142560 I couldn't find Dana on here to but in the the reviewer list. Maybe @gribozavr can help locate them?

[bnbarham]

Seems like this should also use the macro logic. In particular this has a || isa<TypedefDecl>(D) and so the TypedefDecl case in DeclLoc.isMacroID() isn't actually used. Missing tests?

As far as I understand, the cases here are:

• Use the beginning of the decl vs the identifier
• Expansion + spelling of a macro for either of those locations if we're in a macro
• Possibly something special for the NS_ENUM-like cases?

So we could instead do something like:

SmallVector<SourceLocation, 2> locations;
SourceLocation baseLocation;
if (isa<...>(D) || ...) {
  baseLocation = D->getBeginLoc();
} else {
  baseLocation = D->getLocation();
}

if (!DeclLoc.isMacroID()) {
  locations.emplace_back(baseLocation);
} else {
  // Maybe something special for the NS_ENUM case?
  locations.emplace_back(SourceMgr.getExpansionLoc(baseLocation), SourceMgr.getSpellingLoc(baseLocation));
}

[daniel-grumberg]

Yeah that is definitely neater, went for something along those lines in the updated patch.

[bnbarham]

Thanks for looking into this @daniel-grumberg!

[bnbarham]

FWIW @gribozavr the issue here is the hardcoding of the NS_ENUM/NS_OPTIONS logic, so the idea here is to make that more generic and still handle the comment inside macro case.

[cor3ntin]

Just a few nitpicks. Hope it helps!
This is subtle enough that I do not have a good opinion on the changes

[cor3ntin]

Suggested change

	const auto DeclLocs = getDeclLocsForCommentSearch(D, SourceMgr);
	const SourceLocation DeclLocs = getDeclLocsForCommentSearch(D, SourceMgr);

[daniel-grumberg]

This isn't a SourceLocation anymore but rather SmallVector<SourceLocation, 2>, should I still explicitly put in the type?

[cor3ntin]

Suggested change

	// findthe comment at the location of the expansion but if there is no
	// find the comment at the location of the expansion but if there is no

[cor3ntin]

Suggested change

	const auto DeclLocs = getDeclLocsForCommentSearch(D, SourceMgr);
	const SourceLocation DeclLocs = getDeclLocsForCommentSearch(D, SourceMgr);

[bnbarham]

Thanks for looking into this @daniel-grumberg! LGTM except for the few small comments.

[bnbarham]

Comment doesn't quite match now with the change to use BaseLocation.

[bnbarham]

This can be pulled out of the loop

[bnbarham]

I think it would be worth adding another test case where we have this one and check that it uses this comment over the one in the macro.

[bnbarham]

Comment isn't true any more. The comment on line 370 is also confusing to me, since... it isn't including the comment? Unless I'm missing something there.

[daniel-grumberg]

Good spot, can't believe I removed the FIXME and not the comment explaining the FIXME 😵‍💫

[llvmbot]

@llvm/pr-subscribers-clang

Author: Daniel Grumberg (daniel-grumberg)

Changes

For declarations declared inside a macro, e.g.:

`#define MAKE_FUNC(suffix) \
         /// Not selected doc comment \
         void func_##suffix(void) {  }

/// Doc comment foo
MAKE_FUNC(foo)

/// Doc comment bar
MAKE_FUNC(bar)

Prefer the doc comment at the expansion site instead of the one defined in the macro.

rdar://113995729

---

Full diff: https://github.com/llvm/llvm-project/pull/65481.diff

3 Files Affected:

• (modified) clang/lib/AST/ASTContext.cpp (+69-128)
• (modified) clang/test/Index/annotate-comments-objc.m (+23-18)
• (modified) clang/unittests/Tooling/SourceCodeTest.cpp (+3-5)

diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 4b1d9e86797b778..ec08a0d3ba7d84a 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -112,10 +112,10 @@ enum FloatingRank {
   Ibm128Rank
 };
 
-/// \returns location that is relevant when searching for Doc comments related
-/// to \p D.
-static SourceLocation getDeclLocForCommentSearch(const Decl *D,
-                                                 SourceManager &SourceMgr) {
+/// \returns The locations that are relevant when searching for Doc comments
+/// related to \p D.
+static SmallVector<SourceLocation, 2>
+getDeclLocsForCommentSearch(const Decl *D, SourceManager &SourceMgr) {
   assert(D);
 
   // User can not attach documentation to implicit declarations.
@@ -167,115 +167,48 @@ static SourceLocation getDeclLocForCommentSearch(const Decl *D,
       isa<TemplateTemplateParmDecl>(D))
     return {};
 
+  SmallVector<SourceLocation, 2> Locations;
   // Find declaration location.
   // For Objective-C declarations we generally don't expect to have multiple
   // declarators, thus use declaration starting location as the "declaration
   // location".
   // For all other declarations multiple declarators are used quite frequently,
   // so we use the location of the identifier as the "declaration location".
+  SourceLocation BaseLocation;
   if (isa<ObjCMethodDecl>(D) || isa<ObjCContainerDecl>(D) ||
-      isa<ObjCPropertyDecl>(D) ||
-      isa<RedeclarableTemplateDecl>(D) ||
+      isa<ObjCPropertyDecl>(D) || isa<RedeclarableTemplateDecl>(D) ||
       isa<ClassTemplateSpecializationDecl>(D) ||
       // Allow association with Y across {} in `typedef struct X {} Y`.
       isa<TypedefDecl>(D))
-    return D->getBeginLoc();
+    BaseLocation = D->getBeginLoc();
+  else
+    BaseLocation = D->getLocation();
 
-  const SourceLocation DeclLoc = D->getLocation();
-  if (DeclLoc.isMacroID()) {
-    // 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.
-    //    ```
-    //    #define MAKE_NAME(Foo) Name##Foo
-    //
-    //    /// 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 (!D->getLocation().isMacroID()) {
+    Locations.emplace_back(BaseLocation);
+  } else {
+    const auto *DeclCtx = D->getDeclContext();
+
+    // When encountering definitions generated from a macro (that are not
+    // contained by another declaration in the macro) we need to try and find
+    // the comment at the location of the expansion but if there is no comment
+    // there we should retry to see if there is a comment inside the macro as
+    // well. To this end we return first BaseLocation to first look at the
+    // expansion site, the second value is the spelling location of the
+    // beginning of the declaration defined inside the macro.
+    if (!(DeclCtx &&
+          Decl::castFromDeclContext(DeclCtx)->getLocation().isMacroID())) {
+      Locations.emplace_back(SourceMgr.getExpansionLoc(BaseLocation));
     }
-    return SourceMgr.getSpellingLoc(D->getBeginLoc());
+
+    // We use Decl::getBeginLoc() and not just BaseLocation here to ensure that
+    // we don't refer to the macro argument location at the expansion site (this
+    // can happen if the name's spelling is provided via macro argument), and
+    // always to the declaration itself.
+    Locations.emplace_back(SourceMgr.getSpellingLoc(D->getBeginLoc()));
   }
 
-  return DeclLoc;
+  return Locations;
 }
 
 RawComment *ASTContext::getRawCommentForDeclNoCacheImpl(
@@ -357,30 +290,36 @@ RawComment *ASTContext::getRawCommentForDeclNoCacheImpl(
 }
 
 RawComment *ASTContext::getRawCommentForDeclNoCache(const Decl *D) const {
-  const SourceLocation DeclLoc = getDeclLocForCommentSearch(D, SourceMgr);
+  const auto DeclLocs = getDeclLocsForCommentSearch(D, SourceMgr);
 
-  // If the declaration doesn't map directly to a location in a file, we
-  // can't find the comment.
-  if (DeclLoc.isInvalid() || !DeclLoc.isFileID())
-    return nullptr;
+  for (const auto DeclLoc : DeclLocs) {
+    // If the declaration doesn't map directly to a location in a file, we
+    // can't find the comment.
+    if (DeclLoc.isInvalid() || !DeclLoc.isFileID())
+      continue;
 
-  if (ExternalSource && !CommentsLoaded) {
-    ExternalSource->ReadComments();
-    CommentsLoaded = true;
-  }
+    if (ExternalSource && !CommentsLoaded) {
+      ExternalSource->ReadComments();
+      CommentsLoaded = true;
+    }
 
-  if (Comments.empty())
-    return nullptr;
+    if (Comments.empty())
+      continue;
 
-  const FileID File = SourceMgr.getDecomposedLoc(DeclLoc).first;
-  if (!File.isValid()) {
-    return nullptr;
+    const FileID File = SourceMgr.getDecomposedLoc(DeclLoc).first;
+    if (!File.isValid())
+      continue;
+
+    const auto CommentsInThisFile = Comments.getCommentsInFile(File);
+    if (!CommentsInThisFile || CommentsInThisFile->empty())
+      continue;
+
+    if (RawComment *Comment =
+            getRawCommentForDeclNoCacheImpl(D, DeclLoc, *CommentsInThisFile))
+      return Comment;
   }
-  const auto CommentsInThisFile = Comments.getCommentsInFile(File);
-  if (!CommentsInThisFile || CommentsInThisFile->empty())
-    return nullptr;
 
-  return getRawCommentForDeclNoCacheImpl(D, DeclLoc, *CommentsInThisFile);
+  return nullptr;
 }
 
 void ASTContext::addComment(const RawComment &RC) {
@@ -584,7 +523,6 @@ void ASTContext::attachCommentsToJustParsedDecls(ArrayRef<Decl *> Decls,
   // declaration, but also comments that *follow* the declaration -- thanks to
   // the lookahead in the lexer: we've consumed the semicolon and looked
   // ahead through comments.
-
   for (const Decl *D : Decls) {
     assert(D);
     if (D->isInvalidDecl())
@@ -592,19 +530,22 @@ void ASTContext::attachCommentsToJustParsedDecls(ArrayRef<Decl *> Decls,
 
     D = &adjustDeclToTemplate(*D);
 
-    const SourceLocation DeclLoc = getDeclLocForCommentSearch(D, SourceMgr);
-
-    if (DeclLoc.isInvalid() || !DeclLoc.isFileID())
-      continue;
-
     if (DeclRawComments.count(D) > 0)
       continue;
 
-    if (RawComment *const DocComment =
-            getRawCommentForDeclNoCacheImpl(D, DeclLoc, *CommentsInThisFile)) {
-      cacheRawCommentForDecl(*D, *DocComment);
-      comments::FullComment *FC = DocComment->parse(*this, PP, D);
-      ParsedComments[D->getCanonicalDecl()] = FC;
+    const auto DeclLocs = getDeclLocsForCommentSearch(D, SourceMgr);
+
+    for (const auto DeclLoc : DeclLocs) {
+      if (DeclLoc.isInvalid() || !DeclLoc.isFileID())
+        continue;
+
+      if (RawComment *const DocComment = getRawCommentForDeclNoCacheImpl(
+              D, DeclLoc, *CommentsInThisFile)) {
+        cacheRawCommentForDecl(*D, *DocComment);
+        comments::FullComment *FC = DocComment->parse(*this, PP, D);
+        ParsedComments[D->getCanonicalDecl()] = FC;
+        break;
+      }
     }
   }
 }
diff --git a/clang/test/Index/annotate-comments-objc.m b/clang/test/Index/annotate-comments-objc.m
index 6a48d9ae8f2cb3e..96bda58dfef20b9 100644
--- a/clang/test/Index/annotate-comments-objc.m
+++ b/clang/test/Index/annotate-comments-objc.m
@@ -46,18 +46,23 @@ - (void)method1_isdoxy4; /*!< method1_isdoxy4 IS_DOXYGEN_SINGLE */
 // attach unrelated comments in the following cases where tag decls are
 // embedded in declarators.
 
-#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_FUNCTIONS(WithSuffix)
+#define DECLARE_FUNCTIONS_COMMENTS_IN_MACRO(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; \
+  }
+
+DECLARE_FUNCTIONS_COMMENTS_IN_MACRO(WithSuffix)
+
+#define DECLARE_FUNCTIONS \
+  void functionFromMacroWithCommentFromExpansionSite(void) { typedef struct Struct_notdoxy Struct_notdoxy; }
+
+/// functionFromMacroWithCommentFromExpansionSite IS_DOXYGEN_SINGLE
+DECLARE_FUNCTIONS
 
 /// typedef_isdoxy1 IS_DOXYGEN_SINGLE
 typedef struct Struct_notdoxy *typedef_isdoxy1;
@@ -68,7 +73,6 @@ void functionFromMacro(void) { \
   /** namedEnumFromMacro IS_DOXYGEN_SINGLE */ \
   enum name { B };
 
-/// IS_DOXYGEN_NOT_ATTACHED
 DECLARE_ENUMS(namedEnumFromMacro)
 
 #endif
@@ -133,8 +137,9 @@ void functionFromMacro(void) { \
 // CHECK: annotate-comments-objc.m:41:22: EnumDecl=An_NS_ENUM_isdoxy1:{{.*}} An_NS_ENUM_isdoxy1 IS_DOXYGEN_SINGLE
 // CHECK: annotate-comments-objc.m:41:22: TypedefDecl=An_NS_ENUM_isdoxy1:{{.*}} An_NS_ENUM_isdoxy1 IS_DOXYGEN_SINGLE
 // CHECK: annotate-comments-objc.m:41:22: EnumDecl=An_NS_ENUM_isdoxy1:{{.*}} An_NS_ENUM_isdoxy1 IS_DOXYGEN_SINGLE
-// CHECK: annotate-comments-objc.m:60:1: FunctionDecl=functionFromMacro:{{.*}} BriefComment=[functionFromMacro IS_DOXYGEN_SINGLE]
-// CHECK: annotate-comments-objc.m:60:1: FunctionDecl=functionFromMacroWithSuffix:{{.*}} BriefComment=[functionFromMacroWithSuffix IS_DOXYGEN_SINGLE]
-// CHECK: annotate-comments-objc.m:63:32: TypedefDecl=typedef_isdoxy1:{{.*}} typedef_isdoxy1 IS_DOXYGEN_SINGLE
-// CHECK: annotate-comments-objc.m:72:1: EnumDecl=enumFromMacro:{{.*}} BriefComment=[enumFromMacro IS_DOXYGEN_SINGLE]
-// CHECK: annotate-comments-objc.m:72:15: EnumDecl=namedEnumFromMacro:{{.*}} BriefComment=[namedEnumFromMacro IS_DOXYGEN_SINGLE]
+// CHECK: annotate-comments-objc.m:59:1: FunctionDecl=functionFromMacro:{{.*}} BriefComment=[functionFromMacro IS_DOXYGEN_SINGLE]
+// CHECK: annotate-comments-objc.m:59:1: FunctionDecl=functionFromMacroWithSuffix:{{.*}} BriefComment=[functionFromMacroWithSuffix IS_DOXYGEN_SINGLE]
+// CHECK: annotate-comments-objc.m:65:1: FunctionDecl=functionFromMacroWithCommentFromExpansionSite:{{.*}} BriefComment=[functionFromMacroWithCommentFromExpansionSite IS_DOXYGEN_SINGLE]
+// CHECK: annotate-comments-objc.m:68:32: TypedefDecl=typedef_isdoxy1:{{.*}} typedef_isdoxy1 IS_DOXYGEN_SINGLE
+// CHECK: annotate-comments-objc.m:76:1: EnumDecl=enumFromMacro:{{.*}} BriefComment=[enumFromMacro IS_DOXYGEN_SINGLE]
+// CHECK: annotate-comments-objc.m:76:15: EnumDecl=namedEnumFromMacro:{{.*}} BriefComment=[namedEnumFromMacro IS_DOXYGEN_SINGLE]
diff --git a/clang/unittests/Tooling/SourceCodeTest.cpp b/clang/unittests/Tooling/SourceCodeTest.cpp
index 3d1dbceb63a7ffa..3641d2ee453f4a2 100644
--- a/clang/unittests/Tooling/SourceCodeTest.cpp
+++ b/clang/unittests/Tooling/SourceCodeTest.cpp
@@ -372,13 +372,11 @@ TEST(SourceCodeTest, getAssociatedRangeWithComments) {
       #define DECL /* Comment */ int x
       $r[[DECL;]])cpp");
 
-  // Does not include comments when only the decl or the comment come from a
-  // macro.
-  // FIXME: Change code to allow this.
   Visit(R"cpp(
       #define DECL int x
-      // Comment
-      $r[[DECL;]])cpp");
+      $r[[// Comment
+      DECL;]])cpp");
+  // Does not include comments when only the comment come from a macro.
   Visit(R"cpp(
       #define COMMENT /* Comment */
       COMMENT

[danakj]

oh hey, just stumbled on this, thanks for improving it!