[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 ↗

(On Diff #195553)

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 ↗

(On Diff #195553)

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 ↗

(On Diff #195553)

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 ↗

(On Diff #195553)

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 ↗

(On Diff #195553)

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 ↗

(On Diff #195553)

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.

clangd/ClangdServer.cpp

 #include <future>
 #include <memory>
 #include <mutex>
 
 namespace clang {
 namespace clangd {
 namespace {
 
+// Expand a DiagnosticError to make it print-friendly (print the detailed
+// message, rather than "clang diagnostic").
+llvm::Error expandDiagnostics(llvm::Error Err, DiagnosticsEngine &DE) {
+  if (auto Diag = DiagnosticError::take(Err)) {
+    llvm::cantFail(std::move(Err));
+    SmallVector<char, 128> DiagMessage;
+    Diag->second.EmitToString(DE, DiagMessage);
+    return llvm::make_error<llvm::StringError>(DiagMessage,
+                                               llvm::inconvertibleErrorCode());
+  }
+  return Err;
+}
+
 class RefactoringResultCollector final
     : public tooling::RefactoringResultConsumer {
 public:
   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.
     Result = std::move(Err);
   }
 
   // Using the handle(SymbolOccurrences) from parent class.
   using tooling::RefactoringResultConsumer::handle;
 
   void handle(tooling::AtomicChanges SourceReplacements) override {
     assert(!Result.hasValue());
auto Action = [Pos](Path File, std::string NewName,
     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());
+      return CB(expandDiagnostics(Rename.takeError(),
+                                  AST.getASTContext().getDiagnostics()));
 
     Rename->invoke(ResultCollector, Context);
 
     assert(ResultCollector.Result.hasValue());
     if (!ResultCollector.Result.getValue())
-      return CB(ResultCollector.Result->takeError());
+      return CB(expandDiagnostics(ResultCollector.Result->takeError(),
+                                  AST.getASTContext().getDiagnostics()));
 
     std::vector<TextEdit> Replacements;
     for (const tooling::AtomicChange &Change : ResultCollector.Result->get()) {
       tooling::Replacements ChangeReps = Change.getReplacements();
       for (const auto &Rep : ChangeReps) {
         // FIXME: Right now we only support renaming the main file, so we
         // drop replacements not for the main file. In the future, we might
         // consider to support:

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"}