[clangd] Don't navigate to forward class declaration when go to definition.

[hokein]

Summary

For some cases, GoToDefinition will navigate to the forward class
declaration, we should always navigate to the class definition.

[sammccall]

clangd/XRefs.cpp

67–78

Can you add some motivation here? e.g. the forward decl example

[hokein]

• Revise the way of checking definition.
• Add a test for function declaration.

[hokein]

With more test, it turns out that ASTNode.OrigD is not always pointed to the definition, so we can't rely on D or ASTNode.OrigD :(.

I revised the way of checking definition, it should works for major cases. Please review it again.

[ilya-biryukov]

clangd/XRefs.cpp

72

That seems like a useful helper on its own, maybe create a helper called Decl* getDefinition(Decl* D) and use it instead?
It's implementation can be as short as the one we currently have for IsDefinition, since all interesting Decl types have getDefinition method.

[hokein]

Use getDefinition to simply the code.

[hokein]

clangd/XRefs.cpp

72

Good point!

clangd/XRefs.cpp

 //===--- XRefs.cpp ----------------------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 #include "XRefs.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexingAction.h"
 namespace clang {
 namespace clangd {
 using namespace llvm;
 namespace {
 
+// Get the definition from a given declaration `D`.
+// Return nullptr if no definition is found, or the declaration type of `D` is
+// not supported.
+const Decl* GetDefinition(const Decl* D) {
+  assert(D);
+  if (const auto *TD = dyn_cast<TagDecl>(D))
+    return TD->getDefinition();
+  else if (const auto *VD = dyn_cast<VarDecl>(D))
+    return VD->getDefinition();
+  else if (const auto *FD = dyn_cast<FunctionDecl>(D))
+    return FD->getDefinition();
+  return nullptr;
+}
+
 /// Finds declarations locations that a given source location refers to.
 class DeclarationAndMacrosFinder : public index::IndexDataConsumer {
   std::vector<const Decl *> Decls;
   std::vector<const MacroInfo *> MacroInfos;
   const SourceLocation &SearchedLocation;
   const ASTContext &AST;
   Preprocessor &PP;
 
std::vector<const MacroInfo *> takeMacroInfos() {
     return std::move(MacroInfos);
   }
 
   bool
   handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
                       ArrayRef<index::SymbolRelation> Relations, FileID FID,
                       unsigned Offset,
                       index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
-    if (isSearchedLocation(FID, Offset))
+    if (isSearchedLocation(FID, Offset)) {
+      // Find and add definition declarations (for GoToDefinition).
+      // We don't use parameter `D`, as Parameter `D` is the canonical
+      // declaration, which is the first declaration of a redeclarable
+      // declaration, and it could be a forward declaration.
+      if (const auto* Def = GetDefinition(D)) {
+        Decls.push_back(Def);
+      } else {
+        // Couldn't find a definition, fall back to use `D`.
         Decls.push_back(D);
+      }
+    }
     return true;
   }
 
 private:
   bool isSearchedLocation(FileID FID, unsigned Offset) const {
     const SourceManager &SourceMgr = AST.getSourceManager();
     return SourceMgr.getFileOffset(SearchedLocation) == Offset &&
            SourceMgr.getFileID(SearchedLocation) == FID;

unittests/clangd/XRefsTests.cpp

const char *Tests[] = {
 
       R"cpp(// Macro
         #define MACRO 0
         #define [[MACRO 1]]
         int main() { return ^MACRO; }
         #define MACRO 2
         #undef macro
       )cpp",
+
+      R"cpp(// Forward class declaration
+        class Foo;
+        [[class Foo {}]];
+        F^oo* foo();
+      )cpp",
+
+      R"cpp(// Function declaration
+        void foo();
+        void g() { f^oo(); }
+        [[void foo() {}]]
+      )cpp",
   };
   for (const char *Test : Tests) {
     Annotations T(Test);
     auto AST = build(T.code());
     EXPECT_THAT(findDefinitions(Context::empty(), AST, T.point()),
                 ElementsAre(RangeIs(T.range())))
         << Test;
   }
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang