[clang] ea57880 - [AST] Ensure getRawCommentsForAnyRedecl() does not miss any redecl with a comment (#108475)

[via cfe-commits]

Author: Nathan Ridge
Date: 2024-09-20T02:23:58-04:00
New Revision: ea578804c81bbad1f31a0c940c8f4378d6893ede

URL: https://github.com/llvm/llvm-project/commit/ea578804c81bbad1f31a0c940c8f4378d6893ede
DIFF: https://github.com/llvm/llvm-project/commit/ea578804c81bbad1f31a0c940c8f4378d6893ede.diff

LOG: [AST] Ensure getRawCommentsForAnyRedecl() does not miss any redecl with a comment (#108475)

The previous implementation had a bug where, if it was called on a Decl
later in the redecl chain than `LastCheckedDecl`, it could incorrectly
skip and overlook a Decl with a comment.
    
The patch addresses this by only using `LastCheckedDecl` if the input
Decl `D` is on the path from the first (canonical) Decl to
`LastCheckedDecl`.
    
An alternative that was considered was to start the iteration from the
(canonical) Decl, however this ran into problems with the modelling of
explicit template specializations in the AST where the canonical Decl
can be unusual. With the current solution, if no Decls were checked yet,
we prefer to check the input Decl over the canonical one.

Fixes https://github.com/llvm/llvm-project/issues/108145

Added: 
    clang/unittests/AST/RawCommentForDeclTest.cpp

Modified: 
    clang/docs/ReleaseNotes.rst
    clang/lib/AST/ASTContext.cpp
    clang/unittests/AST/CMakeLists.txt

Removed: 
    


################################################################################
diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index b8816d7b555e87..3f146cb9247a78 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -408,6 +408,8 @@ Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Fixed a crash that occurred when dividing by zero in complex integer division. (#GH55390).
+- Fixed a bug in ``ASTContext::getRawCommentForAnyRedecl()`` where the function could
+  sometimes incorrectly return null even if a comment was present. (#GH108145)
 
 Miscellaneous Bug Fixes
 ^^^^^^^^^^^^^^^^^^^^^^^

diff  --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index ebd4a41ee6367a..85b3984940ffc2 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -440,12 +440,26 @@ const RawComment *ASTContext::getRawCommentForAnyRedecl(
   }
 
   // Any redeclarations of D that we haven't checked for comments yet?
-  // We can't use DenseMap::iterator directly since it'd get invalid.
-  auto LastCheckedRedecl = [this, CanonicalD]() -> const Decl * {
-    return CommentlessRedeclChains.lookup(CanonicalD);
+  const Decl *LastCheckedRedecl = [&]() {
+    const Decl *LastChecked = CommentlessRedeclChains.lookup(CanonicalD);
+    bool CanUseCommentlessCache = false;
+    if (LastChecked) {
+      for (auto *Redecl : CanonicalD->redecls()) {
+        if (Redecl == D) {
+          CanUseCommentlessCache = true;
+          break;
+        }
+        if (Redecl == LastChecked)
+          break;
+      }
+    }
+    // FIXME: This could be improved so that even if CanUseCommentlessCache
+    // is false, once we've traversed past CanonicalD we still skip ahead
+    // LastChecked.
+    return CanUseCommentlessCache ? LastChecked : nullptr;
   }();
 
-  for (const auto Redecl : D->redecls()) {
+  for (const Decl *Redecl : D->redecls()) {
     assert(Redecl);
     // Skip all redeclarations that have been checked previously.
     if (LastCheckedRedecl) {

diff  --git a/clang/unittests/AST/CMakeLists.txt b/clang/unittests/AST/CMakeLists.txt
index 40d2e1ff77a601..bfa6082a6ffa4b 100644
--- a/clang/unittests/AST/CMakeLists.txt
+++ b/clang/unittests/AST/CMakeLists.txt
@@ -34,6 +34,7 @@ add_clang_unittest(ASTTests
   NamedDeclPrinterTest.cpp
   ProfilingTest.cpp
   RandstructTest.cpp
+  RawCommentForDeclTest.cpp
   RecursiveASTVisitorTest.cpp
   SizelessTypesTest.cpp
   SourceLocationTest.cpp

diff  --git a/clang/unittests/AST/RawCommentForDeclTest.cpp b/clang/unittests/AST/RawCommentForDeclTest.cpp
new file mode 100644
index 00000000000000..c81e56bf7e6b0c
--- /dev/null
+++ b/clang/unittests/AST/RawCommentForDeclTest.cpp
@@ -0,0 +1,98 @@
+//===- unittests/AST/RawCommentForDeclTestTest.cpp
+//-------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/DeclGroup.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+
+#include "gmock/gmock-matchers.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+
+struct FoundComment {
+  std::string DeclName;
+  bool IsDefinition;
+  std::string Comment;
+
+  bool operator==(const FoundComment &RHS) const {
+    return DeclName == RHS.DeclName && IsDefinition == RHS.IsDefinition &&
+           Comment == RHS.Comment;
+  }
+  friend llvm::raw_ostream &operator<<(llvm::raw_ostream &Stream,
+                                       const FoundComment &C) {
+    return Stream << "{Name: " << C.DeclName << ", Def: " << C.IsDefinition
+                  << ", Comment: " << C.Comment << "}";
+  }
+};
+
+class CollectCommentsAction : public ASTFrontendAction {
+public:
+  CollectCommentsAction(std::vector<FoundComment> &Comments)
+      : Comments(Comments) {}
+
+  std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
+                                                 llvm::StringRef) override {
+    CI.getLangOpts().CommentOpts.ParseAllComments = true;
+    return std::make_unique<Consumer>(*this);
+  }
+
+  std::vector<FoundComment> &Comments;
+
+private:
+  class Consumer : public clang::ASTConsumer {
+  private:
+    CollectCommentsAction &Action;
+
+  public:
+    Consumer(CollectCommentsAction &Action) : Action(Action) {}
+
+    bool HandleTopLevelDecl(DeclGroupRef DG) override {
+      for (Decl *D : DG) {
+        if (NamedDecl *ND = dyn_cast<NamedDecl>(D)) {
+          auto &Ctx = D->getASTContext();
+          const auto *RC = Ctx.getRawCommentForAnyRedecl(D);
+          Action.Comments.push_back(FoundComment{
+              ND->getNameAsString(), IsDefinition(D),
+              RC ? RC->getRawText(Ctx.getSourceManager()).str() : ""});
+        }
+      }
+
+      return true;
+    }
+
+    static bool IsDefinition(const Decl *D) {
+      if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
+        return FD->isThisDeclarationADefinition();
+      }
+      if (const TagDecl *TD = dyn_cast<TagDecl>(D)) {
+        return TD->isThisDeclarationADefinition();
+      }
+      return false;
+    }
+  };
+};
+
+TEST(RawCommentForDecl, DefinitionComment) {
+  std::vector<FoundComment> Comments;
+  auto Action = std::make_unique<CollectCommentsAction>(Comments);
+  ASSERT_TRUE(tooling::runToolOnCode(std::move(Action), R"cpp(
+    void f();
+
+    // f is the best
+    void f() {}
+  )cpp"));
+  EXPECT_THAT(Comments, testing::ElementsAre(
+                            FoundComment{"f", false, ""},
+                            FoundComment{"f", true, "// f is the best"}));
+}
+
+} // namespace clang