[clang-tools-extra] d3a4ef3 - [clangd] IncludeClenaer: Don't mark forward declarations of a class if it's declared in the main file

[Kirill Bobyrev via cfe-commits]

Author: Kirill Bobyrev
Date: 2021-12-02T10:26:08+01:00
New Revision: d3a4ef35685c6aaad74294b678e77e1b75af1918

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

LOG: [clangd] IncludeClenaer: Don't mark forward declarations of a class if it's declared in the main file

This will mark more headers that are unrelated to used symbol but contain its
forawrd declaration. E.g. the following are examples of headers forward
declaring `llvm::StringRef`:

- clang/include/clang/Basic/Cuda.h
- llvm/include/llvm/Support/SHA256.h
- llvm/include/llvm/Support/TrigramIndex.h
- llvm/include/llvm/Support/RandomNumberGenerator.
- ... and more (~50 in total)

This patch is a reduced version of D112707 which was controversial.

Reviewed By: kadircet

Differential Revision: https://reviews.llvm.org/D114864

Added: 
    

Modified: 
    clang-tools-extra/clangd/IncludeCleaner.cpp
    clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp
index 33964d9f83ce0..76d5d626b9f88 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -33,7 +33,9 @@ namespace {
 class ReferencedLocationCrawler
     : public RecursiveASTVisitor<ReferencedLocationCrawler> {
 public:
-  ReferencedLocationCrawler(ReferencedLocations &Result) : Result(Result) {}
+  ReferencedLocationCrawler(ReferencedLocations &Result,
+                            const SourceManager &SM)
+      : Result(Result), SM(SM) {}
 
   bool VisitDeclRefExpr(DeclRefExpr *DRE) {
     add(DRE->getDecl());
@@ -120,6 +122,17 @@ class ReferencedLocationCrawler
   void add(const Decl *D) {
     if (!D || !isNew(D->getCanonicalDecl()))
       return;
+    // If we see a declaration in the mainfile, skip all non-definition decls.
+    // We only do this for classes because forward declarations are common and
+    // we don't want to include every header that forward-declares the symbol
+    // because they're often unrelated.
+    if (const auto *RD = llvm::dyn_cast<RecordDecl>(D)) {
+      if (SM.isInMainFile(RD->getMostRecentDecl()->getBeginLoc())) {
+        if (const auto *Definition = RD->getMostRecentDecl()->getDefinition())
+          Result.insert(Definition->getLocation());
+        return;
+      }
+    }
     for (const Decl *Redecl : D->redecls())
       Result.insert(Redecl->getLocation());
   }
@@ -128,6 +141,7 @@ class ReferencedLocationCrawler
 
   ReferencedLocations &Result;
   llvm::DenseSet<const void *> Visited;
+  const SourceManager &SM;
 };
 
 // Given a set of referenced FileIDs, determines all the potentially-referenced
@@ -253,7 +267,7 @@ FileID headerResponsible(FileID ID, const SourceManager &SM,
 ReferencedLocations findReferencedLocations(ParsedAST &AST) {
   trace::Span Tracer("IncludeCleaner::findReferencedLocations");
   ReferencedLocations Result;
-  ReferencedLocationCrawler Crawler(Result);
+  ReferencedLocationCrawler Crawler(Result, AST.getSourceManager());
   Crawler.TraverseAST(AST.getASTContext());
   findReferencedMacros(AST, Result);
   return Result;

diff  --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
index f72791da4030a..ddc0a99d58356 100644
--- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -39,6 +39,18 @@ TEST(IncludeCleaner, ReferencedLocations) {
           "class ^X;",
           "X *y;",
       },
+      // FIXME(kirillbobyrev): When definition is available, we don't need to
+      // mark forward declarations as used.
+      {
+          "class ^X {}; class ^X;",
+          "X *y;",
+      },
+      // We already have forward declaration in the main file, the other
+      // non-definition declarations are not needed.
+      {
+          "class ^X {}; class X;",
+          "class X; X *y;",
+      },
       // TypedefType and UsingDecls
       {
           "using ^Integer = int;",