[clangd] Hover should return null when not hovering over anything.

[sammccall]

Summary

Also made JSON serialize Optional<T> to simplify this.

[ioeric]

lgtm

clangd/ClangdLSPServer.cpp

Server.findDocumentHighlights(
           return replyError(ErrorCode::InternalError,
                             llvm::toString(Highlights.takeError()));
         reply(json::ary(*Highlights));
       });
 }
 
 void ClangdLSPServer::onHover(TextDocumentPositionParams &Params) {
   Server.findHover(Params.textDocument.uri.file(), Params.position,
-                   [](llvm::Expected<Hover> H) {
+                   [](llvm::Expected<llvm::Optional<Hover>> H) {
                      if (!H) {
                        replyError(ErrorCode::InternalError,
                                   llvm::toString(H.takeError()));
                        return;
                      }
 
                      reply(*H);
                    });

clangd/ClangdServer.h

public:
   /// given a header file and vice versa.
   llvm::Optional<Path> switchSourceHeader(PathRef Path);
 
   /// Get document highlights for a given position.
   void findDocumentHighlights(PathRef File, Position Pos,
                               Callback<std::vector<DocumentHighlight>> CB);
 
   /// Get code hover for a given position.
-  void findHover(PathRef File, Position Pos, Callback<Hover> CB);
+  void findHover(PathRef File, Position Pos,
+                 Callback<llvm::Optional<Hover>> CB);
 
   /// Retrieve the top symbols from the workspace matching a query.
   void workspaceSymbols(StringRef Query, int Limit,
                         Callback<std::vector<SymbolInformation>> CB);
 
   /// Run formatting for \p Rng inside \p File with content \p Code.
   llvm::Expected<tooling::Replacements> formatRange(StringRef Code,
                                                     PathRef File, Range Rng);

clangd/ClangdServer.cpp

auto Action = [FS, Pos](Callback<std::vector<DocumentHighlight>> CB,
     if (!InpAST)
       return CB(InpAST.takeError());
     CB(clangd::findDocumentHighlights(InpAST->AST, Pos));
   };
 
   WorkScheduler.runWithAST("Highlights", File, Bind(Action, std::move(CB)));
 }
 
-void ClangdServer::findHover(PathRef File, Position Pos, Callback<Hover> CB) {
+void ClangdServer::findHover(PathRef File, Position Pos,
+                             Callback<llvm::Optional<Hover>> CB) {
   auto FS = FSProvider.getFileSystem();
-  auto Action = [Pos, FS](Callback<Hover> CB,
+  auto Action = [Pos, FS](Callback<llvm::Optional<Hover>> CB,
                           llvm::Expected<InputsAndAST> InpAST) {
     if (!InpAST)
       return CB(InpAST.takeError());
     CB(clangd::getHover(InpAST->AST, Pos));
   };
 
   WorkScheduler.runWithAST("Hover", File, Bind(Action, std::move(CB)));
 }

clangd/JSONExpr.h

 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/raw_ostream.h"
 #include <map>
 
 namespace clang {
 namespace clangd {
 namespace json {
+class Expr;
+template <typename T> Expr toJSON(const llvm::Optional<T> &Opt);
 
 // An Expr is an JSON value of unknown type.
 // They can be copied, but should generally be moved.
 //
 // === Composing expressions ===
 //
 // You can implicitly construct Exprs from:
 //   - strings: std::string, SmallString, formatv, StringRef, char*
if (auto *O = E.asObject()) {
     for (const auto &KV : *O)
       if (!fromJSON(KV.second, Out[llvm::StringRef(KV.first)]))
         return false;
     return true;
   }
   return false;
 }
 
+template <typename T>
+json::Expr toJSON(const llvm::Optional<T>& Opt) {
+  return Opt ? json::Expr(*Opt) : json::Expr(nullptr);
+}
+
 // Helper for mapping JSON objects onto protocol structs.
 // See file header for example.
 class ObjectMapper {
 public:
   ObjectMapper(const json::Expr &E) : O(E.asObject()) {}
 
   // True if the expression is an object.
   // Must be checked before calling map().

clangd/XRefs.h

 //
 //===---------------------------------------------------------------------===//
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_XREFS_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_XREFS_H
 
 #include "ClangdUnit.h"
 #include "Protocol.h"
 #include "index/Index.h"
+#include "llvm/ADT/Optional.h"
 #include <vector>
 
 namespace clang {
 namespace clangd {
 
 /// Get definition of symbol at a specified \p Pos.
 std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
                                       const SymbolIndex *Index = nullptr);
 
 /// Returns highlights for all usages of a symbol at \p Pos.
 std::vector<DocumentHighlight> findDocumentHighlights(ParsedAST &AST,
                                                       Position Pos);
 
 /// Get the hover information when hovering at \p Pos.
-Hover getHover(ParsedAST &AST, Position Pos);
+llvm::Optional<Hover> getHover(ParsedAST &AST, Position Pos);
 
 } // namespace clangd
 } // namespace clang
 #endif

clangd/XRefs.cpp

static Hover getHoverContents(StringRef MacroName) {
   Hover H;
 
   H.contents.value = "#define ";
   H.contents.value += MacroName;
 
   return H;
 }
 
-Hover getHover(ParsedAST &AST, Position Pos) {
+Optional<Hover> getHover(ParsedAST &AST, Position Pos) {
   const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
   SourceLocation SourceLocationBeg =
       getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID());
   // Identified symbols at a specific position.
   auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg);
 
   if (!Symbols.Macros.empty())
     return getHoverContents(Symbols.Macros[0].Name);
 
   if (!Symbols.Decls.empty())
     return getHoverContents(Symbols.Decls[0]);
 
-  return Hover();
+  return None;
 }
 
 } // namespace clangd
 } // namespace clang

test/clangd/hover.test

 # RUN: clangd -lit-test < %s | FileCheck %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":"void foo(); int main() { foo(); }\n"}}}
 ---
 {"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":0,"character":27}}}
 #      CHECK:  "id": 1,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "contents": {
 # CHECK-NEXT:      "kind": "plaintext",
 # CHECK-NEXT:      "value": "Declared in global namespace\n\nvoid foo()"
 # CHECK-NEXT:    }
 # CHECK-NEXT:  }
 # CHECK-NEXT:}
 ---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":0,"character":10}}}
+#      CHECK:  "id": 1,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": null
+---
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}

unittests/clangd/JSONExprTests.cpp

 TEST(JSONExprTests, Constructors) {
   // Lots of edge cases around empty and singleton init lists.
   EXPECT_EQ("[[[3]]]", s({{{3}}}));
   EXPECT_EQ("[[[]]]", s({{{}}}));
   EXPECT_EQ("[[{}]]", s({{obj{}}}));
   EXPECT_EQ(R"({"A":{"B":{}}})", s(obj{{"A", obj{{"B", obj{}}}}}));
   EXPECT_EQ(R"({"A":{"B":{"X":"Y"}}})",
             s(obj{{"A", obj{{"B", obj{{"X", "Y"}}}}}}));
+  EXPECT_EQ("null", s(llvm::Optional<double>()));
+  EXPECT_EQ("2.5", s(llvm::Optional<double>(2.5)));
 }
 
 TEST(JSONExprTests, StringOwnership) {
   char X[] = "Hello";
   Expr Alias = static_cast<const char *>(X);
   X[1] = 'a';
   EXPECT_EQ(R"("Hallo")", s(Alias));
 

unittests/clangd/XRefsTests.cpp

OneTest Tests[] = {
               union {
                 int abc, def;
               } v;
             };
             void g() { struct outer o; o.v.d^ef++; }
           )cpp",
           "Declared in union outer::(anonymous)\n\nint def",
       },
+      {
+          R"cpp(// Nothing
+            void foo() {
+              ^
+            }
+          )cpp",
+          "",
+      },
   };
 
   for (const OneTest &Test : Tests) {
     Annotations T(Test.Input);
     auto AST = TestTU::withCode(T.code()).build();
-    Hover H = getHover(AST, T.point());
-
-    EXPECT_EQ(H.contents.value, Test.ExpectedHover) << Test.Input;
+    if (auto H = getHover(AST, T.point())) {
+      EXPECT_EQ("", Test.ExpectedHover) << Test.Input;
+      EXPECT_EQ(H->contents.value, Test.ExpectedHover) << Test.Input;
+    } else
+      EXPECT_EQ("", Test.ExpectedHover) << Test.Input;
   }
 }
 
 TEST(GoToInclude, All) {
   MockFSProvider FS;
   IgnoreDiagnostics DiagConsumer;
   MockCompilationDatabase CDB;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());