[clangd] XRef functions treat visible class definitions as primary.

[sammccall]

Summary

This means go-to-type will take us to a class definition in most cases.
As an exception, if the class is declared in a header and defined in a CC file,
then these are treated as a decl/def pair.

This is (fairly) consistent with how the index behaves, though the index
benefits from aggregation across translation units so does a simpler "main file"
check rather than explicitly "is this a header".

[kadircet]

clang-tools-extra/clangd/XRefs.cpp

291

can we extract all of this into a const TagDecl* getPublicDecl(const TagDecl* TD) ?

303

nit: we actually have the main file name in ParsedAST, but it's probably not worth propagating that into here.

303

reaching here actually implies user is in the same file that has the type definition (I know we were mostly discussing this for index behaviour, and our reasoning for the index makes completely sense) but I am not sure if taking the user to public declaration, when they're actually writing some code in the same file as the type's private implementation is still the right thing to do.

I think; if the definition is visible through AST, we should always prefer that, unless it's in a private header (which in theory we can detect here, as we've information about include graph).
do you think jumping to public interface is still desirable even when the user is already implementing something inside the private implementation file?

clang-tools-extra/clangd/XRefs.cpp

 // declaration to be.
 const NamedDecl *getPreferredDecl(const NamedDecl *D) {
   // FIXME: Canonical declarations of some symbols might refer to built-in
   // decls with possibly-invalid source locations (e.g. global new operator).
   // In such cases we should pick up a redecl with valid source location
   // instead of failing.
   D = llvm::cast<NamedDecl>(D->getCanonicalDecl());
 
+  // Prefer C++ class definitions over a forward declaration.
+  if (const auto *TD = llvm::dyn_cast<TagDecl>(D)) {
+    if (const auto *Def = TD->getDefinition()) {
+      // Exception: forward decl is exposed in a header, definition is not.
+      // (This exception matches the spirit of our indexing heuristics).
+      if (/*PreferDef=*/[&] {
+        if (Def == TD)
+          return true;
+        const auto &SM = D->getASTContext().getSourceManager();
+        FileID DefFile = SM.getDecomposedExpansionLoc(Def->getLocation()).first;
+        if (DefFile != SM.getMainFileID())
+          return true; // definition is in a header.
+        FileID DeclFile = SM.getDecomposedExpansionLoc(D->getLocation()).first;
+        if (DefFile == DeclFile)
+          return true; // def/decl are equally visible.
+        // We've established that the definition is in the main file, and the
+        // decl is some other header.
+        return isHeaderFile(SM.getFileEntryRefForID(DefFile)->getName(),
+                            D->getASTContext().getLangOpts());
+      }())
+        D = Def;
+    }
+  }
+
   // Prefer Objective-C class/protocol definitions over the forward declaration.
   if (const auto *ID = dyn_cast<ObjCInterfaceDecl>(D))
     if (const auto *DefinitionID = ID->getDefinition())
       return DefinitionID;
   if (const auto *PD = dyn_cast<ObjCProtocolDecl>(D))
     if (const auto *DefinitionID = PD->getDefinition())
       return DefinitionID;
 

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

const char *Tests[] = {
       )cpp",
       R"cpp(// Symbol concatenated inside macro (not supported)
        int *pi;
        #define POINTER(X) p ## X;
        int x = *POINTER(^i);
       )cpp",
 
       R"cpp(// Forward class declaration
-        class $decl[[Foo]];
-        class $def[[Foo]] {};
+        class Foo;
+        class [[Foo]] {};
         F^oo* foo();
       )cpp",
 
       R"cpp(// Function declaration
         void $decl[[foo]]();
         void g() { f^oo(); }
         void $def[[foo]]() {}
       )cpp",
if (!WantDecl) {
       EXPECT_TRUE(Results[0].ID) << Test;
       llvm::Optional<Range> GotDef;
       if (Results[0].Definition)
         GotDef = Results[0].Definition->range;
       EXPECT_EQ(WantDef, GotDef) << Test;
     }
   }
 }
+
+TEST(LocateSymbol, TypeForwardDeclaredInHeader) {
+  Annotations Code(R"cpp(
+    class [[X]] {};
+    ^X *x;
+  )cpp");
+
+  TestTU TU;
+  TU.HeaderCode = "class X;";
+  TU.Code = Code.code();
+
+  auto AST = TU.build();
+  auto Index = TU.index();
+  LocatedSymbol S = locateSymbolAt(AST, Code.point(), Index.get()).front();
+  EXPECT_EQ(S.Name, "X");
+  auto Filename = [&](const Location &L) {
+    return llvm::sys::path::filename(L.uri.file());
+  };
+  EXPECT_EQ(Filename(S.PreferredDeclaration), TU.HeaderFilename);
+  EXPECT_EQ(Filename(*S.Definition), TU.Filename);
+
+  S = findType(AST, Code.point()).front();
+  EXPECT_EQ(S.Name, "X");
+  EXPECT_EQ(Filename(S.PreferredDeclaration), TU.HeaderFilename);
+  EXPECT_EQ(Filename(*S.Definition), TU.Filename);
+}
+
 TEST(LocateSymbol, ValidSymbolID) {
   auto T = Annotations(R"cpp(
     #define MACRO(x, y) ((x) + (y))
     int add(int x, int y) { return $MACRO^MACRO(x, y); }
     int sum = $add^add(1, 2);
   )cpp");
 
   TestTU TU = TestTU::withCode(T.code());