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

[kbobyrev]

Summary

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.

[kadircet]

I can't fully remember the discussion but what about the case in which we don't have a declaration in the main file, but see a definition and multiple forward declarations. Is there a good reason to still include re-decls in this case?

clang-tools-extra/clangd/IncludeCleaner.cpp

57

why don't we do this inside add instead? which can take care of duplication, and remain as the only place we modify Result ?

[kbobyrev]

Resolve review comments, add justification in tests.

[kadircet]

thanks, lgtm!

clang-tools-extra/clangd/IncludeCleaner.cpp

131

nit: I'd just inline RD->getMostRecentDecl() here.

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

42

might be worth having a fixme comment here (around we should ignore all re-decls when we can see a def)

[kbobyrev]

Address review comments

clang-tools-extra/clangd/IncludeCleaner.cpp

 namespace clangd {
 namespace {
 
 /// Crawler traverses the AST and feeds in the locations of (sometimes
 /// implicitly) used symbols into \p Result.
 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());
     add(DRE->getFoundDecl());
     return true;
   }
 
   bool VisitMemberExpr(MemberExpr *ME) {
     add(ME->getMemberDecl());
     add(ME->getFoundDecl().getDecl());
     return true;
   }
 
   bool VisitTagType(TagType *TT) {
     add(TT->getDecl());
     return true;
   }
 
   bool VisitFunctionDecl(FunctionDecl *FD) {
     // Function definition will require redeclarations to be included.
     if (FD->isThisDeclarationADefinition())
       add(FD);
     return true;
   }
 
   bool VisitCXXConstructExpr(CXXConstructExpr *CCE) {
     add(CCE->getConstructor());
public:
   }
 
 private:
   using Base = RecursiveASTVisitor<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());
   }
 
   bool isNew(const void *P) { return P && Visited.insert(P).second; }
 
   ReferencedLocations &Result;
   llvm::DenseSet<const void *> Visited;
+  const SourceManager &SM;
 };
 
 // Given a set of referenced FileIDs, determines all the potentially-referenced
 // files and macros by traversing expansion/spelling locations of macro IDs.
 // This is used to map the referenced SourceLocations onto real files.
 struct ReferencedFiles {
   ReferencedFiles(const SourceManager &SM) : SM(SM) {}
   llvm::DenseSet<FileID> Files;
FileID headerResponsible(FileID ID, const SourceManager &SM,
   return ID;
 }
 
 } // namespace
 
 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;
 }
 
 llvm::DenseSet<FileID>
 findReferencedFiles(const llvm::DenseSet<SourceLocation> &Locs,
                     const IncludeStructure &Includes, const SourceManager &SM) {

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

TestCase Cases[] = {
           "int ^x();",
           "int y = x();",
       },
       // RecordDecl
       {
           "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;",
           "Integer x;",
       },
       {
           "namespace ns { struct ^X; struct ^X {}; }",
           "using ns::X;",