[Clangd] textDocument/definition and textDocument/declaration "bounce" between definition and declaration location when they are distinct.

Summary:
This helps minimize the disruption of not returning declarations as part of
a find-definition response (r352864).

Reviewers: hokein

Subscribers: ioeric, MaskRay, jkorous, arphaman, kadircet, ilya-biryukov

Tags: #clang

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

llvm-svn: 352953

2 files changed, 74 insertions, 15 deletions

diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index 7e7acf946f9..893d85ab61a 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -726,18 +726,41 @@ void ClangdLSPServer::onSignatureHelp(const TextDocumentPositionParams &Params,
                         std::move(Reply));
 }
 
+// Go to definition has a toggle function: if def and decl are distinct, then
+// the first press gives you the def, the second gives you the matching def.
+// getToggle() returns the counterpart location that under the cursor.
+//
+// We return the toggled location alone (ignoring other symbols) to encourage
+// editors to "bounce" quickly between locations, without showing a menu.
+static Location *getToggle(const TextDocumentPositionParams &Point,
+                           LocatedSymbol &Sym) {
+  // Toggle only makes sense with two distinct locations.
+  if (!Sym.Definition || *Sym.Definition == Sym.PreferredDeclaration)
+    return nullptr;
+  if (Sym.Definition->uri.file() == Point.textDocument.uri.file() &&
+      Sym.Definition->range.contains(Point.position))
+    return &Sym.PreferredDeclaration;
+  if (Sym.PreferredDeclaration.uri.file() == Point.textDocument.uri.file() &&
+      Sym.PreferredDeclaration.range.contains(Point.position))
+    return &*Sym.Definition;
+  return nullptr;
+}
+
 void ClangdLSPServer::onGoToDefinition(const TextDocumentPositionParams &Params,
                                        Callback<std::vector<Location>> Reply) {
   Server->locateSymbolAt(
       Params.textDocument.uri.file(), Params.position,
       Bind(
-          [&](decltype(Reply) Reply,
-              llvm::Expected<std::vector<LocatedSymbol>> Symbols) {
+          [&, Params](decltype(Reply) Reply,
+                      llvm::Expected<std::vector<LocatedSymbol>> Symbols) {
             if (!Symbols)
               return Reply(Symbols.takeError());
             std::vector<Location> Defs;
-            for (const auto &S : *Symbols)
+            for (auto &S : *Symbols) {
+              if (Location *Toggle = getToggle(Params, S))
+                return Reply(std::vector<Location>{std::move(*Toggle)});
               Defs.push_back(S.Definition.getValueOr(S.PreferredDeclaration));
+            }
             Reply(std::move(Defs));
           },
           std::move(Reply)));
@@ -749,13 +772,16 @@ void ClangdLSPServer::onGoToDeclaration(
   Server->locateSymbolAt(
       Params.textDocument.uri.file(), Params.position,
       Bind(
-          [&](decltype(Reply) Reply,
-              llvm::Expected<std::vector<LocatedSymbol>> Symbols) {
+          [&, Params](decltype(Reply) Reply,
+                      llvm::Expected<std::vector<LocatedSymbol>> Symbols) {
             if (!Symbols)
               return Reply(Symbols.takeError());
             std::vector<Location> Decls;
-            for (const auto &S : *Symbols)
-              Decls.push_back(S.PreferredDeclaration);
+            for (auto &S : *Symbols) {
+              if (Location *Toggle = getToggle(Params, S))
+                return Reply(std::vector<Location>{std::move(*Toggle)});
+              Decls.push_back(std::move(S.PreferredDeclaration));
+            }
             Reply(std::move(Decls));
           },
           std::move(Reply)));
diff --git a/clang-tools-extra/test/clangd/xrefs.test b/clang-tools-extra/test/clangd/xrefs.test
index 58ca44cb0ef..128c97ff634 100644
--- a/clang-tools-extra/test/clangd/xrefs.test
+++ b/clang-tools-extra/test/clangd/xrefs.test
@@ -1,9 +1,9 @@
 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 ---
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"int x = 0;\nint y = x;"}}}
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"extern int x;\nint x = 0;\nint y = x;"}}}
 ---
-{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":1,"character":8}}}
+{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":2,"character":8}}}
 #      CHECK:  "id": 1,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": [
@@ -11,10 +11,30 @@
 # CHECK-NEXT:      "range": {
 # CHECK-NEXT:        "end": {
 # CHECK-NEXT:          "character": 5,
-# CHECK-NEXT:          "line": 0
+# CHECK-NEXT:          "line": 1
 # CHECK-NEXT:        },
 # CHECK-NEXT:        "start": {
 # CHECK-NEXT:          "character": 4,
+# CHECK-NEXT:          "line": 1
+# CHECK-NEXT:        }
+# CHECK-NEXT:      },
+# CHECK-NEXT:      "uri": "file://{{.*}}/{{([A-Z]:/)?}}main.cpp"
+# CHECK-NEXT:    }
+# CHECK-NEXT:  ]
+---
+# Toggle: we're on the definition, so jump to the declaration.
+{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":1,"character":4}}}
+#      CHECK:  "id": 1,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": [
+# CHECK-NEXT:    {
+# CHECK-NEXT:      "range": {
+# CHECK-NEXT:        "end": {
+# CHECK-NEXT:          "character": 12,
+# CHECK-NEXT:          "line": 0
+# CHECK-NEXT:        },
+# CHECK-NEXT:        "start": {
+# CHECK-NEXT:          "character": 11,
 # CHECK-NEXT:          "line": 0
 # CHECK-NEXT:        }
 # CHECK-NEXT:      },
@@ -22,7 +42,7 @@
 # CHECK-NEXT:    }
 # CHECK-NEXT:  ]
 ---
-{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":1,"character":8}}}
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":2,"character":8}}}
 #      CHECK: "id": 1
 # CHECK-NEXT: "jsonrpc": "2.0",
 # CHECK-NEXT: "result": [
@@ -30,25 +50,38 @@
 # CHECK-NEXT:     "kind": 1,
 # CHECK-NEXT:     "range": {
 # CHECK-NEXT:       "end": {
-# CHECK-NEXT:         "character": 5,
+# CHECK-NEXT:         "character": 12,
 # CHECK-NEXT:         "line": 0
 # CHECK-NEXT:       },
 # CHECK-NEXT:       "start": {
-# CHECK-NEXT:         "character": 4,
+# CHECK-NEXT:         "character": 11,
 # CHECK-NEXT:         "line": 0
 # CHECK-NEXT:       }
 # CHECK-NEXT:     }
 # CHECK-NEXT:   },
 # CHECK-NEXT:   {
+# CHECK-NEXT:     "kind": 1,
+# CHECK-NEXT:     "range": {
+# CHECK-NEXT:       "end": {
+# CHECK-NEXT:         "character": 5,
+# CHECK-NEXT:         "line": 1
+# CHECK-NEXT:       },
+# CHECK-NEXT:       "start": {
+# CHECK-NEXT:         "character": 4,
+# CHECK-NEXT:         "line": 1
+# CHECK-NEXT:       }
+# CHECK-NEXT:     }
+# CHECK-NEXT:   },
+# CHECK-NEXT:   {
 # CHECK-NEXT:     "kind": 2,
 # CHECK-NEXT:     "range": {
 # CHECK-NEXT:       "end": {
 # CHECK-NEXT:         "character": 9,
-# CHECK-NEXT:         "line": 1
+# CHECK-NEXT:         "line": 2
 # CHECK-NEXT:       },
 # CHECK-NEXT:       "start": {
 # CHECK-NEXT:         "character": 8,
-# CHECK-NEXT:         "line": 1
+# CHECK-NEXT:         "line": 2
 # CHECK-NEXT:       }
 # CHECK-NEXT:     }
 # CHECK-NEXT:   }