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);
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);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
