[PATCH] D60821: [clangd] Emit better error messages when rename fails.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: sammccall.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ioeric, ilya-biryukov.
Herald added a project: clang.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60821

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/test/clangd/rename.test


Index: clang-tools-extra/test/clangd/rename.test
===================================================================
--- clang-tools-extra/test/clangd/rename.test
+++ clang-tools-extra/test/clangd/rename.test
@@ -29,7 +29,7 @@
 {"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"
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -44,15 +44,23 @@
 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.
-    Result = std::move(Err);
+    if (auto Diag = DiagnosticError::take(Err))
+      Result = toStringError(*Diag, Diags);
+    else
+      Result = std::move(Err);
   }
 
   // Using the handle(SymbolOccurrences) from parent class.
@@ -63,6 +71,7 @@
     Result = std::move(SourceReplacements);
   }
 
+  DiagnosticsEngine &Diags;
   llvm::Optional<llvm::Expected<tooling::AtomicChanges>> Result;
 };
 
@@ -291,7 +300,8 @@
       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());
@@ -300,8 +310,15 @@
     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);
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D60821.195553.patch
Type: text/x-patch
Size: 3139 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190417/d844d4a6/attachment-0001.bin>