[PATCH] D61126: [clangd] Also perform merging for symbol definitions

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, ilya-biryukov.
Herald added a project: clang.

clangd currently prefers declarations from codegen files. This patch
implements that behavior for definition locations. If we have definiton
locations both coming from AST and index, clangd will perform a merging to show
the codegen file if that's the case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61126

Files:
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/unittests/clangd/XRefsTests.cpp


Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===================================================================
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -186,7 +186,8 @@
 
 TEST(LocateSymbol, WithIndexPreferredLocation) {
   Annotations SymbolHeader(R"cpp(
-        class $[[Proto]] {};
+        class $p[[Proto]] {};
+        void $f[[func]]() {};
       )cpp");
   TestTU TU;
   TU.HeaderCode = SymbolHeader.code();
@@ -195,13 +196,27 @@
 
   Annotations Test(R"cpp(// only declaration in AST.
         // Shift to make range different.
-        class [[Proto]];
-        P^roto* create();
+        class Proto;
+        void func() {}
+        P$p^roto* create() {
+          fu$f^nc();
+          return nullptr;
+        }
       )cpp");
 
   auto AST = TestTU::withCode(Test.code()).build();
-  auto Locs = clangd::locateSymbolAt(AST, Test.point(), Index.get());
-  EXPECT_THAT(Locs, ElementsAre(Sym("Proto", SymbolHeader.range())));
+  {
+    auto Locs = clangd::locateSymbolAt(AST, Test.point("p"), Index.get());
+    auto CodeGenLoc = SymbolHeader.range("p");
+    EXPECT_THAT(Locs, ElementsAre(Sym("Proto", CodeGenLoc, CodeGenLoc)));
+    llvm::errs() << Locs.front() << '\n';
+  }
+  {
+    auto Locs = clangd::locateSymbolAt(AST, Test.point("f"), Index.get());
+    auto CodeGenLoc = SymbolHeader.range("f");
+    EXPECT_THAT(Locs, ElementsAre(Sym("func", CodeGenLoc, CodeGenLoc)));
+    llvm::errs() << Locs.front() << '\n';
+  }
 }
 
 TEST(LocateSymbol, All) {
Index: clang-tools-extra/clangd/XRefs.cpp
===================================================================
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -346,28 +346,27 @@
     Index->lookup(QueryRequest, [&](const Symbol &Sym) {
       auto &R = Result[ResultIndex.lookup(Sym.ID)];
 
-      // Special case: if the AST yielded a definition, then it may not be
-      // the right *declaration*. Prefer the one from the index.
       if (R.Definition) { // from AST
-        if (auto Loc = toLSPLocation(Sym.CanonicalDeclaration, *MainFilePath))
-          R.PreferredDeclaration = *Loc;
+        // In case of generated files we prefer to omit the definition in the
+        // generated code.
         if (auto Loc = toLSPLocation(
                 getPreferredLocation(*R.Definition, Sym.Definition, Scratch),
                 *MainFilePath))
           R.Definition = *Loc;
+
+        // Special case: if the AST yielded a definition, then it may not be
+        // the right *declaration*. Prefer the one from the index.
+        if (auto Loc = toLSPLocation(Sym.CanonicalDeclaration, *MainFilePath))
+          R.PreferredDeclaration = *Loc;
       } else {
         R.Definition = toLSPLocation(Sym.Definition, *MainFilePath);
 
-        if (Sym.CanonicalDeclaration) {
-          // Use merge logic to choose AST or index declaration.
-          // We only do this for declarations as definitions from AST
-          // is generally preferred (e.g. definitions in main file).
-          if (auto Loc = toLSPLocation(
-                  getPreferredLocation(R.PreferredDeclaration,
-                                       Sym.CanonicalDeclaration, Scratch),
-                  *MainFilePath))
-            R.PreferredDeclaration = *Loc;
-        }
+        // Use merge logic to choose AST or index declaration.
+        if (auto Loc = toLSPLocation(
+                getPreferredLocation(R.PreferredDeclaration,
+                                     Sym.CanonicalDeclaration, Scratch),
+                *MainFilePath))
+          R.PreferredDeclaration = *Loc;
       }
     });
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D61126.196608.patch
Type: text/x-patch
Size: 3688 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190425/4ea4a933/attachment-0001.bin>