Skip to content

[clang] Prioritze decl comments from macro expansion site #65481

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 26, 2023
Merged

[clang] Prioritze decl comments from macro expansion site #65481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
197 changes: 69 additions & 128 deletions clang/lib/AST/ASTContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// 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) {
Expand Down Expand Up @@ -584,27 +523,29 @@ 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())
continue;

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const auto DeclLocs = getDeclLocsForCommentSearch(D, SourceMgr);
const SourceLocation 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;
}
}
}
}
Expand Down
48 changes: 30 additions & 18 deletions clang/test/Index/annotate-comments-objc.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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;
Expand All @@ -68,9 +73,14 @@ void functionFromMacro(void) { \
/** namedEnumFromMacro IS_DOXYGEN_SINGLE */ \
enum name { B };

/// IS_DOXYGEN_NOT_ATTACHED
DECLARE_ENUMS(namedEnumFromMacro)

#define MYENUM(name) enum name
struct Foo {
/// Vehicles IS_DOXYGEN_SINGLE
MYENUM(Vehicles) { Car, Motorbike, Boat} a;
};

#endif

// RUN: rm -rf %t
Expand Down Expand Up @@ -133,8 +143,10 @@ 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]
// CHECK: annotate-comments-objc.m:81:10: EnumDecl=Vehicles:{{.*}} Vehicles IS_DOXYGEN_SINGLE
8 changes: 3 additions & 5 deletions clang/unittests/Tooling/SourceCodeTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down