[Clang][Comments] Attach comments to decl even if preproc directives are in between (#88367)

### Background

It's surprisingly common for C++ code in the wild to conditionally
show/hide declarations to Doxygen through the use of preprocessor
directives. One especially common version of this pattern is
demonstrated below:

```cpp
/// @brief Test comment
#ifdef DOXYGEN_BUILD_ENABLED
template<typename T>
#else
template <typename T>
typename std::enable_if<std::is_integral<T>::value>::type
#endif
void f() {}
```

There are more examples I've collected below to demonstrate usage of
this pattern:
- Example 1:
[Magnum](https://github.com/mosra/magnum/blob/8538610fa27e1db37070eaabe34f1e4e41648bab/src/Magnum/Resource.h#L117-L127)
- Example 2:
[libcds](https://github.com/khizmax/libcds/blob/9985d2a87feaa3e92532e28f4ab762a82855a49c/cds/container/michael_list_nogc.h#L36-L54)
- Example 3:
[rocPRIM](https://github.com/ROCm/rocPRIM/blob/609ae19565ff6a3499168b76a0be5652762e24f6/rocprim/include/rocprim/block/detail/block_reduce_raking_reduce.hpp#L60-L65)
 
From my research, it seems like the most common rationale for this
functionality is hiding difficult-to-parse code from Doxygen, especially
where template metaprogramming is concerned.

Currently, Clang does not support attaching comments to decls if there
are preprocessor comments between the comment and the decl. This is
enforced here:
https://github.com/llvm/llvm-project/blob/b6ebea7972cd05a8e4dcf0d5a54f2c793999995a/clang/lib/AST/ASTContext.cpp#L284-L287

Alongside preprocessor directives, any instance of `;{}#@` between a
comment and decl will cause the comment to not be attached to the decl.

#### Rationale

It would be nice for Clang-based documentation tools, such as
[hdoc](https://hdoc.io), to support code using this pattern. Users
expect to see comments attached to the relevant decl — even if there is
an `#ifdef` in the way — which Clang does not currently do.

#### History

Originally, commas were also in the list of "banned" characters, but
were removed in `b534d3a0ef69`
([link](https://github.com/llvm/llvm-project/commit/b534d3a0ef6970f5e42f10ba5cfcb562d8b184e1))
because availability macros often have commas in them. From my reading
of the code, it appears that the original intent of the code was to
exclude macros and decorators between comments and decls, possibly in an
attempt to properly attribute comments to macros (discussed further in
"Complications", below). There's some more discussion here:
https://reviews.llvm.org/D125061.

### Change

This modifies Clang comment parsing so that comments are attached to
subsequent declarations even if there are preprocessor directives
between the end of the comment and the start of the decl. Furthermore,
this change:

- Adds tests to verify that comments are attached to their associated
decls even if there are preprocessor directives in between
- Adds tests to verify that current behavior has not changed (i.e. use
of the other characters between comment and decl will result in the
comment not being attached to the decl)
- Updates existing `lit` tests which would otherwise break.

#### Complications

Clang [does not yet
support](https://github.com/llvm/llvm-project/issues/38206) attaching
doc comments to macros. Consequently, the change proposed in this RFC
affects cases where a doc comment attached to a macro is followed
immediately by a normal declaration. In these cases, the macro's doc
comments will be attached to the subsequent decl. Previously they would
be ignored because any preprocessor directives between a comment and a
decl would result in the comment not being attached to the decl. An
example of this is shown below.

```cpp
/// Doc comment for a function-like macro
/// @param n
///    A macro argument
#define custom_sqrt(n) __internal_sqrt(n)

int __internal_sqrt(int n) { return __builtin_sqrt(n); }

// NB: the doc comment for the custom_sqrt macro will actually be attached to __internal_sqrt!
```

There is a real instance of this problem in the Clang codebase, namely
here:
https://github.com/llvm/llvm-project/blob/be10070f91b86a6f126d2451852242bfcb2cd366/clang/lib/Headers/amxcomplexintrin.h#L65-L114

As part of this RFC, I've added a semicolon to break up the Clang
comment parsing so that the `-Wdocumentation` errors go away, but this
is a hack. The real solution is to fix Clang comment parsing so that doc
comments are properly attached to macros, however this would be a large
change that is outside of the scope of this RFC.

1 files changed, 310 insertions, 0 deletions

diff --git a/clang/unittests/AST/DeclTest.cpp b/clang/unittests/AST/DeclTest.cpp
index 16aa2b5..8dca011 100644
--- a/clang/unittests/AST/DeclTest.cpp
+++ b/clang/unittests/AST/DeclTest.cpp
@@ -576,3 +576,313 @@ void instantiate_template() {
   EXPECT_EQ(GetNameInfoRange(Matches[1]), "<input.cc:6:14, col:15>");
   EXPECT_EQ(GetNameInfoRange(Matches[2]), "<input.cc:6:14, col:15>");
 }
+
+TEST(Decl, CommentsAttachedToDecl1) {
+  const SmallVector<StringRef> Sources{
+      R"(
+        /// Test comment
+        void f();
+      )",
+
+      R"(
+        /// Test comment
+
+        void f();
+      )",
+
+      R"(
+        /// Test comment
+        #if 0
+        // tralala
+        #endif
+        void f();
+      )",
+
+      R"(
+        /// Test comment
+
+        #if 0
+        // tralala
+        #endif
+
+        void f();
+      )",
+
+      R"(
+        /// Test comment
+        #ifdef DOCS
+        template<typename T>
+        #endif
+        void f();
+      )",
+
+      R"(
+        /// Test comment
+
+        #ifdef DOCS
+        template<typename T>
+        #endif
+
+        void f();
+      )",
+  };
+
+  for (const auto code : Sources) {
+    auto AST = tooling::buildASTFromCodeWithArgs(code, /*Args=*/{"-std=c++20"});
+    ASTContext &Ctx = AST->getASTContext();
+
+    auto const *F = selectFirst<FunctionDecl>(
+        "id", match(functionDecl(hasName("f")).bind("id"), Ctx));
+    ASSERT_NE(F, nullptr);
+
+    auto const *C = Ctx.getRawCommentForDeclNoCache(F);
+    ASSERT_NE(C, nullptr);
+    EXPECT_EQ(C->getRawText(Ctx.getSourceManager()), "/// Test comment");
+  }
+}
+
+TEST(Decl, CommentsAttachedToDecl2) {
+  const SmallVector<StringRef> Sources{
+      R"(
+        /** Test comment
+         */
+        void f();
+      )",
+
+      R"(
+        /** Test comment
+         */
+
+        void f();
+      )",
+
+      R"(
+        /** Test comment
+         */
+        #if 0
+        /* tralala */
+        #endif
+        void f();
+      )",
+
+      R"(
+        /** Test comment
+         */
+
+        #if 0
+        /* tralala */
+        #endif
+
+        void f();
+      )",
+
+      R"(
+        /** Test comment
+         */
+        #ifdef DOCS
+        template<typename T>
+        #endif
+        void f();
+      )",
+
+      R"(
+        /** Test comment
+         */
+
+        #ifdef DOCS
+        template<typename T>
+        #endif
+
+        void f();
+      )",
+  };
+
+  for (const auto code : Sources) {
+    auto AST = tooling::buildASTFromCodeWithArgs(code, /*Args=*/{"-std=c++20"});
+    ASTContext &Ctx = AST->getASTContext();
+
+    auto const *F = selectFirst<FunctionDecl>(
+        "id", match(functionDecl(hasName("f")).bind("id"), Ctx));
+    ASSERT_NE(F, nullptr);
+
+    auto const *C = Ctx.getRawCommentForDeclNoCache(F);
+    ASSERT_NE(C, nullptr);
+    EXPECT_EQ(C->getRawText(Ctx.getSourceManager()),
+              "/** Test comment\n         */");
+  }
+}
+
+TEST(Decl, CommentsAttachedToDecl3) {
+  const SmallVector<StringRef> Sources{
+      R"(
+        /// @brief Test comment
+        void f();
+      )",
+
+      R"(
+        /// @brief Test comment
+
+        void f();
+      )",
+
+      R"(
+        /// @brief Test comment
+        #if 0
+        // tralala
+        #endif
+        void f();
+      )",
+
+      R"(
+        /// @brief Test comment
+
+        #if 0
+        // tralala
+        #endif
+
+        void f();
+      )",
+
+      R"(
+        /// @brief Test comment
+        #ifdef DOCS
+        template<typename T>
+        #endif
+        void f();
+      )",
+
+      R"(
+        /// @brief Test comment
+
+        #ifdef DOCS
+        template<typename T>
+        #endif
+
+        void f();
+     )",
+  };
+
+  for (const auto code : Sources) {
+    auto AST = tooling::buildASTFromCodeWithArgs(code, /*Args=*/{"-std=c++20"});
+    ASTContext &Ctx = AST->getASTContext();
+
+    auto const *F = selectFirst<FunctionDecl>(
+        "id", match(functionDecl(hasName("f")).bind("id"), Ctx));
+    ASSERT_NE(F, nullptr);
+
+    auto const *C = Ctx.getRawCommentForDeclNoCache(F);
+    ASSERT_NE(C, nullptr);
+    EXPECT_EQ(C->getRawText(Ctx.getSourceManager()), "/// @brief Test comment");
+  }
+}
+
+TEST(Decl, CommentsAttachedToDecl4) {
+  const SmallVector<StringRef> Sources{
+      R"(
+        /** \brief Test comment
+         */
+        void f();
+      )",
+
+      R"(
+        /** \brief Test comment
+         */
+
+        void f();
+      )",
+
+      R"(
+        /** \brief Test comment
+         */
+        #if 0
+        /* tralala */
+        #endif
+        void f();
+      )",
+
+      R"(
+        /** \brief Test comment
+         */
+
+        #if 0
+        /* tralala */
+        #endif
+
+        void f();
+      )",
+
+      R"(
+        /** \brief Test comment
+         */
+        #ifdef DOCS
+        template<typename T>
+        #endif
+        void f();
+      )",
+
+      R"(
+        /** \brief Test comment
+         */
+
+        #ifdef DOCS
+        template<typename T>
+        #endif
+
+        void f();
+     )",
+  };
+
+  for (const auto code : Sources) {
+    auto AST = tooling::buildASTFromCodeWithArgs(code, /*Args=*/{"-std=c++20"});
+    ASTContext &Ctx = AST->getASTContext();
+
+    auto const *F = selectFirst<FunctionDecl>(
+        "id", match(functionDecl(hasName("f")).bind("id"), Ctx));
+    ASSERT_NE(F, nullptr);
+
+    auto const *C = Ctx.getRawCommentForDeclNoCache(F);
+    ASSERT_NE(C, nullptr);
+    EXPECT_EQ(C->getRawText(Ctx.getSourceManager()),
+              "/** \\brief Test comment\n         */");
+  }
+}
+
+/// This example intentionally inserts characters between a doc comment and the
+/// associated declaration to verify that the comment does not become associated
+/// with the FunctionDecl.
+/// By default, Clang does not allow for other declarations (aside from
+/// preprocessor directives, as shown above) to be placed between a doc comment
+/// and a declaration.
+TEST(Decl, CommentsAttachedToDecl5) {
+  const SmallVector<StringRef> Sources{
+      R"(
+        /// Test comment
+        ;
+        void f();
+      )",
+
+      R"(
+        /// Test comment
+        // @
+        void f();
+      )",
+
+      R"(
+        /// Test comment
+        // {}
+        void f();
+      )",
+  };
+
+  for (const auto code : Sources) {
+    auto AST = tooling::buildASTFromCodeWithArgs(code, /*Args=*/{"-std=c++20"});
+    ASTContext &Ctx = AST->getASTContext();
+
+    auto const *F = selectFirst<FunctionDecl>(
+        "id", match(functionDecl(hasName("f")).bind("id"), Ctx));
+    ASSERT_NE(F, nullptr);
+
+    auto const *C = Ctx.getRawCommentForDeclNoCache(F);
+    ASSERT_EQ(C, nullptr);
+  }
+}