[clangd] Prefer the definition of a TagDecl (e.g. class) as CanonicalDeclaration.

Summary:
Currently, we pick the first declaration of a symbol in a TU, which is considered
canonical in the clangIndex, as the canonical declaration in clangd. This causes
forward declarations that might appear in a random header to be used as a
canonical declaration, which is not desirable for features like go-to-declaration
or include insertion.

For example, for class X, we would consider the forward declaration in fwd.h to
be the canonical declaration, while the preferred canonical declaration should
be the actual definition in x.h.
```
// fwd.h
class X;  // forward decl

// x.h
class X {};
```

This patch fixes the issue by making symbol collector favor the actual definition of
a TagDecl (i.e. class/struct/enum/union) found in a header file over the first seen
declarations in a TU. Other symbol types like functions are not handled because
using the first seen declarations as canonical declarations is usually a good
heuristic for them.

Reviewers: sammccall

Subscribers: klimek, ilya-biryukov, jkorous-apple, cfe-commits

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

llvm-svn: 326313

1 files changed, 27 insertions, 12 deletions

diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index f8fb2c29570..c75184010ee 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -115,9 +115,7 @@ bool shouldFilterDecl(const NamedDecl *ND, ASTContext *ASTCtx,
   //   * enum constants in unscoped enum decl (e.g. "red" in "enum {red};")
   auto InTopLevelScope = hasDeclContext(
       anyOf(namespaceDecl(), translationUnitDecl(), linkageSpecDecl()));
-  if (match(decl(allOf(Opts.IndexMainFiles
-                           ? decl()
-                           : decl(unless(isExpansionInMainFile())),
+  if (match(decl(allOf(unless(isExpansionInMainFile()),
                        anyOf(InTopLevelScope,
                              hasDeclContext(enumDecl(InTopLevelScope,
                                                      unless(isScoped())))))),
@@ -177,11 +175,9 @@ getIncludeHeader(const SourceManager &SM, SourceLocation Loc,
 // For symbols defined inside macros:
 //   * use expansion location, if the symbol is formed via macro concatenation.
 //   * use spelling location, otherwise.
-llvm::Optional<SymbolLocation>
-getSymbolLocation(const NamedDecl &D, SourceManager &SM,
-                  const SymbolCollector::Options &Opts,
-                  const clang::LangOptions& LangOpts,
-                  std::string &FileURIStorage) {
+llvm::Optional<SymbolLocation> getSymbolLocation(
+    const NamedDecl &D, SourceManager &SM, const SymbolCollector::Options &Opts,
+    const clang::LangOptions &LangOpts, std::string &FileURIStorage) {
   SourceLocation SpellingLoc = SM.getSpellingLoc(D.getLocation());
   if (D.getLocation().isMacroID()) {
     std::string PrintLoc = SpellingLoc.printToString(SM);
@@ -209,6 +205,19 @@ getSymbolLocation(const NamedDecl &D, SourceManager &SM,
   return std::move(Result);
 }
 
+// Checks whether \p ND is a definition of a TagDecl (class/struct/enum/union)
+// in a header file, in which case clangd would prefer to use ND as a canonical
+// declaration.
+// FIXME: handle symbol types that are not TagDecl (e.g. functions), if using
+// the the first seen declaration as canonical declaration is not a good enough
+// heuristic.
+bool isPreferredDeclaration(const NamedDecl &ND, index::SymbolRoleSet Roles) {
+  using namespace clang::ast_matchers;
+  return (Roles & static_cast<unsigned>(index::SymbolRole::Definition)) &&
+         llvm::isa<TagDecl>(&ND) &&
+         match(decl(isExpansionInMainFile()), ND, ND.getASTContext()).empty();
+}
+
 } // namespace
 
 SymbolCollector::SymbolCollector(Options Opts) : Opts(std::move(Opts)) {}
@@ -241,12 +250,20 @@ bool SymbolCollector::handleDeclOccurence(
     if (index::generateUSRForDecl(ND, USR))
       return true;
 
+    const NamedDecl &OriginalDecl = *cast<NamedDecl>(ASTNode.OrigD);
     auto ID = SymbolID(USR);
-    const Symbol* BasicSymbol = Symbols.find(ID);
+    const Symbol *BasicSymbol = Symbols.find(ID);
     if (!BasicSymbol) // Regardless of role, ND is the canonical declaration.
       BasicSymbol = addDeclaration(*ND, std::move(ID));
+    else if (isPreferredDeclaration(OriginalDecl, Roles))
+      // If OriginalDecl is preferred, replace the existing canonical
+      // declaration (e.g. a class forward declaration). There should be at most
+      // one duplicate as we expect to see only one preferred declaration per
+      // TU, because in practice they are definitions.
+      BasicSymbol = addDeclaration(OriginalDecl, std::move(ID));
+
     if (Roles & static_cast<unsigned>(index::SymbolRole::Definition))
-      addDefinition(*cast<NamedDecl>(ASTNode.OrigD), *BasicSymbol);
+      addDefinition(OriginalDecl, *BasicSymbol);
   }
   return true;
 }
@@ -271,8 +288,6 @@ const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND,
   std::tie(S.Scope, S.Name) = splitQualifiedName(QName);
   S.SymInfo = index::getSymbolInfo(&ND);
   std::string FileURI;
-  // FIXME: we may want a different "canonical" heuristic than clang chooses.
-  // Clang seems to choose the first, which may not have the most information.
   if (auto DeclLoc =
           getSymbolLocation(ND, SM, Opts, ASTCtx->getLangOpts(), FileURI))
     S.CanonicalDeclaration = *DeclLoc;