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

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

Reviewers: ioeric

Subscribers: klimek, ilya-biryukov, MaskRay, jkorous, cfe-commits

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

llvm-svn: 333881

clang-tools-extra/clangd/ClangdLSPServer.cpp

@@ -365,7 +365,7 @@ void ClangdLSPServer::onDocumentHighlight(TextDocumentPositionParams &Params) {
 
 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()));

clang-tools-extra/clangd/ClangdServer.cpp

@@ -403,9 +403,10 @@ void ClangdServer::findDocumentHighlights(
   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());

clang-tools-extra/clangd/ClangdServer.h

@@ -158,7 +158,8 @@ public:
                               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,

clang-tools-extra/clangd/JSONExpr.h

@@ -22,6 +22,8 @@
 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.
@@ -516,6 +518,11 @@ bool fromJSON(const json::Expr &E, std::map<std::string, T> &Out) {
   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 {

clang-tools-extra/clangd/XRefs.cpp

@@ -526,7 +526,7 @@ static Hover getHoverContents(StringRef 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());
@@ -539,7 +539,7 @@ Hover getHover(ParsedAST &AST, Position Pos) {
   if (!Symbols.Decls.empty())
     return getHoverContents(Symbols.Decls[0]);
 
-  return Hover();
+  return None;
 }
 
 } // namespace clangd

clang-tools-extra/clangd/XRefs.h

@@ -16,6 +16,7 @@
 #include "ClangdUnit.h"
 #include "Protocol.h"
 #include "index/Index.h"
+#include "llvm/ADT/Optional.h"
 #include <vector>
 
 namespace clang {
@@ -30,7 +31,7 @@ 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

clang-tools-extra/test/clangd/hover.test

@@ -14,6 +14,11 @@
 # 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"}

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

@@ -38,6 +38,8 @@ TEST(JSONExprTests, Constructors) {
   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) {

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

@@ -629,14 +629,24 @@ TEST(Hover, All) {
           )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;
   }
 }