[clangd] Emit better error messages when rename fails.

[hokein]

Summary

Currently we emit an unfriendly "clang diagnostic" message when rename fails. This
patch makes clangd to emit a detailed diagnostic message.

[sammccall]

clang-tools-extra/clangd/ClangdServer.cpp

57

why inject the DE here (and handle mapping errors both in the caller and here) rather than always doing the mapping in the caller?

60

I'm confused about this, take() seems to return Optional<PartialDiagnosticAt>, but you're passing it as a DiagnosticError ... oh, DiagnosticError has a constructor from PartialDiagnosticAt.

This seems unneccesarily confusing, just pass the PartialDiagnostic?

315

can we avoid this duplication by giving a helper function more responsibilities?
e.g. llvm::Error expandDiagnostics(llvm::Error, DiagnosticsEngine&)?

[hokein]

Address comments.

[hokein]

clang-tools-extra/clangd/ClangdServer.cpp

57

We don't have control of the Err caller for this class (the Error is passed via the handleError interface)...so we need the DE in this class to do the expanding.

315

SG!

[sammccall]

LG, thanks!

I do think it can be further simplified, but if not then land as-is.

clang-tools-extra/clangd/ClangdServer.cpp

57

But we just store the err in Result, and then ClangdServer::rename does

if (!ResultCollector.Result.getValue())
      return CB(ResultCollector.Result->takeError());

can it wrap the takeError() in expandDiagnostics()?

it still has access to the diagnosticsengine I think

[hokein]

Simplify the code further.

clang-tools-extra/clangd/ClangdServer.cpp

 #include <future>
 #include <memory>
 #include <mutex>
 
 namespace clang {
 namespace clangd {
 namespace {
 
+llvm::Error toStringError(DiagnosticError DiagErr, DiagnosticsEngine &DE) {
+  SmallVector<char, 128> DiagMessage;
+  DiagErr.getDiagnostic().second.EmitToString(DE, DiagMessage);
+  return llvm::make_error<llvm::StringError>(DiagMessage,
+                                             llvm::inconvertibleErrorCode());
+}
+
 class RefactoringResultCollector final
     : public tooling::RefactoringResultConsumer {
 public:
+  RefactoringResultCollector(DiagnosticsEngine &DE) : Diags(DE) {}
   void handleError(llvm::Error Err) override {
     assert(!Result.hasValue());
-    // FIXME: figure out a way to return better message for DiagnosticError.
-    // clangd uses llvm::toString to convert the Err to string, however, for
-    // DiagnosticError, only "clang diagnostic" will be generated.
+    if (auto Diag = DiagnosticError::take(Err))
+      Result = toStringError(*Diag, Diags);
+    else
       Result = std::move(Err);
   }
 
   // Using the handle(SymbolOccurrences) from parent class.
   using tooling::RefactoringResultConsumer::handle;
 
   void handle(tooling::AtomicChanges SourceReplacements) override {
     assert(!Result.hasValue());
     Result = std::move(SourceReplacements);
   }
 
+  DiagnosticsEngine &Diags;
   llvm::Optional<llvm::Expected<tooling::AtomicChanges>> Result;
 };
 
 // Update the FileIndex with new ASTs and plumb the diagnostics responses.
 struct UpdateIndexCallbacks : public ParsingCallbacks {
   UpdateIndexCallbacks(FileIndex *FIndex, DiagnosticsConsumer &DiagConsumer)
       : FIndex(FIndex), DiagConsumer(DiagConsumer) {}
 
void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName,
                           Callback<std::vector<TextEdit>> CB) {
   auto Action = [Pos](Path File, std::string NewName,
                       Callback<std::vector<TextEdit>> CB,
                       llvm::Expected<InputsAndAST> InpAST) {
     if (!InpAST)
       return CB(InpAST.takeError());
     auto &AST = InpAST->AST;
 
-    RefactoringResultCollector ResultCollector;
+    RefactoringResultCollector ResultCollector(
+        AST.getASTContext().getDiagnostics());
     const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
     SourceLocation SourceLocationBeg =
         clangd::getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID());
     tooling::RefactoringRuleContext Context(
         AST.getASTContext().getSourceManager());
     Context.setASTContext(AST.getASTContext());
     auto Rename = clang::tooling::RenameOccurrences::initiate(
         Context, SourceRange(SourceLocationBeg), NewName);
-    if (!Rename)
-      return CB(Rename.takeError());
+    if (!Rename) {
+      auto Error = Rename.takeError();
+      if (auto Diag = DiagnosticError::take(Error)) {
+        llvm::cantFail(std::move(Error));
+        return CB(toStringError(*Diag, AST.getASTContext().getDiagnostics()));
+      }
+
+      return CB(std::move(Error));
+    }
 
     Rename->invoke(ResultCollector, Context);
 
     assert(ResultCollector.Result.hasValue());
     if (!ResultCollector.Result.getValue())
       return CB(ResultCollector.Result->takeError());
 
     std::vector<TextEdit> Replacements;

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

 # CHECK-NEXT:        }
 # CHECK-NEXT:      ]
 # CHECK-NEXT:    }
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","id":2,"method":"textDocument/rename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":2},"newName":"bar"}}
 #      CHECK:  "error": {
 # CHECK-NEXT:    "code": -32001,
-# CHECK-NEXT:    "message": "clang diagnostic"
+# CHECK-NEXT:    "message": "there is no symbol at the given location"
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "id": 2,
 # CHECK-NEXT:  "jsonrpc": "2.0"
 ---
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}