[clang-tools-extra] r322637 - [clangd] Don't crash on LSP calls for non-added files
Ilya Biryukov via cfe-commits
cfe-commits at lists.llvm.org
Wed Jan 17 04:30:24 PST 2018
Author: ibiryukov
Date: Wed Jan 17 04:30:24 2018
New Revision: 322637
URL: http://llvm.org/viewvc/llvm-project?rev=322637&view=rev
Log:
[clangd] Don't crash on LSP calls for non-added files
Summary:
We will return errors for non-added files for now.
Another alternative for clangd would be to read non-added files from
disk and provide useful features anyway.
There are still some cases that fail with assertion (e.g., code
complete). We should address those too, but they require more subtle
changes to the code and therefore out of scope of this patch.
Reviewers: sammccall, ioeric, hokein
Reviewed By: sammccall
Subscribers: klimek, cfe-commits
Differential Revision: https://reviews.llvm.org/D42164
Added:
clang-tools-extra/trunk/test/clangd/crash-non-added-files.test
Modified:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=322637&r1=322636&r2=322637&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Wed Jan 17 04:30:24 2018
@@ -143,14 +143,19 @@ void ClangdLSPServer::onCommand(Ctx C, E
void ClangdLSPServer::onRename(Ctx C, RenameParams &Params) {
auto File = Params.textDocument.uri.file;
+ auto Code = Server.getDocument(File);
+ if (!Code)
+ return replyError(C, ErrorCode::InvalidParams,
+ "onRename called for non-added file");
+
auto Replacements = Server.rename(C, File, Params.position, Params.newName);
if (!Replacements) {
replyError(C, ErrorCode::InternalError,
llvm::toString(Replacements.takeError()));
return;
}
- std::string Code = Server.getDocument(File);
- std::vector<TextEdit> Edits = replacementsToEdits(Code, *Replacements);
+
+ std::vector<TextEdit> Edits = replacementsToEdits(*Code, *Replacements);
WorkspaceEdit WE;
WE.changes = {{Params.textDocument.uri.uri, Edits}};
reply(C, WE);
@@ -164,10 +169,14 @@ void ClangdLSPServer::onDocumentDidClose
void ClangdLSPServer::onDocumentOnTypeFormatting(
Ctx C, DocumentOnTypeFormattingParams &Params) {
auto File = Params.textDocument.uri.file;
- std::string Code = Server.getDocument(File);
- auto ReplacementsOrError = Server.formatOnType(Code, File, Params.position);
+ auto Code = Server.getDocument(File);
+ if (!Code)
+ return replyError(C, ErrorCode::InvalidParams,
+ "onDocumentOnTypeFormatting called for non-added file");
+
+ auto ReplacementsOrError = Server.formatOnType(*Code, File, Params.position);
if (ReplacementsOrError)
- reply(C, json::ary(replacementsToEdits(Code, ReplacementsOrError.get())));
+ reply(C, json::ary(replacementsToEdits(*Code, ReplacementsOrError.get())));
else
replyError(C, ErrorCode::UnknownErrorCode,
llvm::toString(ReplacementsOrError.takeError()));
@@ -176,10 +185,14 @@ void ClangdLSPServer::onDocumentOnTypeFo
void ClangdLSPServer::onDocumentRangeFormatting(
Ctx C, DocumentRangeFormattingParams &Params) {
auto File = Params.textDocument.uri.file;
- std::string Code = Server.getDocument(File);
- auto ReplacementsOrError = Server.formatRange(Code, File, Params.range);
+ auto Code = Server.getDocument(File);
+ if (!Code)
+ return replyError(C, ErrorCode::InvalidParams,
+ "onDocumentRangeFormatting called for non-added file");
+
+ auto ReplacementsOrError = Server.formatRange(*Code, File, Params.range);
if (ReplacementsOrError)
- reply(C, json::ary(replacementsToEdits(Code, ReplacementsOrError.get())));
+ reply(C, json::ary(replacementsToEdits(*Code, ReplacementsOrError.get())));
else
replyError(C, ErrorCode::UnknownErrorCode,
llvm::toString(ReplacementsOrError.takeError()));
@@ -188,10 +201,14 @@ void ClangdLSPServer::onDocumentRangeFor
void ClangdLSPServer::onDocumentFormatting(Ctx C,
DocumentFormattingParams &Params) {
auto File = Params.textDocument.uri.file;
- std::string Code = Server.getDocument(File);
- auto ReplacementsOrError = Server.formatFile(Code, File);
+ auto Code = Server.getDocument(File);
+ if (!Code)
+ return replyError(C, ErrorCode::InvalidParams,
+ "onDocumentFormatting called for non-added file");
+
+ auto ReplacementsOrError = Server.formatFile(*Code, File);
if (ReplacementsOrError)
- reply(C, json::ary(replacementsToEdits(Code, ReplacementsOrError.get())));
+ reply(C, json::ary(replacementsToEdits(*Code, ReplacementsOrError.get())));
else
replyError(C, ErrorCode::UnknownErrorCode,
llvm::toString(ReplacementsOrError.takeError()));
@@ -200,7 +217,11 @@ void ClangdLSPServer::onDocumentFormatti
void ClangdLSPServer::onCodeAction(Ctx C, CodeActionParams &Params) {
// We provide a code action for each diagnostic at the requested location
// which has FixIts available.
- std::string Code = Server.getDocument(Params.textDocument.uri.file);
+ auto Code = Server.getDocument(Params.textDocument.uri.file);
+ if (!Code)
+ return replyError(C, ErrorCode::InvalidParams,
+ "onCodeAction called for non-added file");
+
json::ary Commands;
for (Diagnostic &D : Params.context.diagnostics) {
auto Edits = getFixIts(Params.textDocument.uri.file, D);
Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=322637&r1=322636&r2=322637&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Wed Jan 17 04:30:24 2018
@@ -356,7 +356,6 @@ ClangdServer::formatOnType(StringRef Cod
Expected<std::vector<tooling::Replacement>>
ClangdServer::rename(const Context &Ctx, PathRef File, Position Pos,
llvm::StringRef NewName) {
- std::string Code = getDocument(File);
std::shared_ptr<CppFile> Resources = Units.getFile(File);
RefactoringResultCollector ResultCollector;
Resources->getAST().get()->runUnderLock([&](ParsedAST *AST) {
@@ -402,15 +401,17 @@ ClangdServer::rename(const Context &Ctx,
return Replacements;
}
-std::string ClangdServer::getDocument(PathRef File) {
- auto draft = DraftMgr.getDraft(File);
- assert(draft.Draft && "File is not tracked, cannot get contents");
- return *draft.Draft;
+llvm::Optional<std::string> ClangdServer::getDocument(PathRef File) {
+ auto Latest = DraftMgr.getDraft(File);
+ if (!Latest.Draft)
+ return llvm::None;
+ return std::move(*Latest.Draft);
}
std::string ClangdServer::dumpAST(PathRef File) {
std::shared_ptr<CppFile> Resources = Units.getFile(File);
- assert(Resources && "dumpAST is called for non-added document");
+ if (!Resources)
+ return "<non-added file>";
std::string Result;
Resources->getAST().get()->runUnderLock([&Result](ParsedAST *AST) {
Modified: clang-tools-extra/trunk/clangd/ClangdServer.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.h?rev=322637&r1=322636&r2=322637&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdServer.h (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.h Wed Jan 17 04:30:24 2018
@@ -308,11 +308,11 @@ public:
PathRef File, Position Pos,
llvm::StringRef NewName);
- /// Gets current document contents for \p File. \p File must point to a
- /// currently tracked file.
+ /// Gets current document contents for \p File. Returns None if \p File is not
+ /// currently tracked.
/// FIXME(ibiryukov): This function is here to allow offset-to-Position
/// conversions in outside code, maybe there's a way to get rid of it.
- std::string getDocument(PathRef File);
+ llvm::Optional<std::string> getDocument(PathRef File);
/// Only for testing purposes.
/// Waits until all requests to worker thread are finished and dumps AST for
Added: clang-tools-extra/trunk/test/clangd/crash-non-added-files.test
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/crash-non-added-files.test?rev=322637&view=auto
==============================================================================
--- clang-tools-extra/trunk/test/clangd/crash-non-added-files.test (added)
+++ clang-tools-extra/trunk/test/clangd/crash-non-added-files.test Wed Jan 17 04:30:24 2018
@@ -0,0 +1,45 @@
+# RUN: clangd -pretty -run-synchronously < %s | FileCheck -strict-whitespace %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+#
+Content-Length: 746
+
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
+# CHECK: "error": {
+# CHECK-NEXT: "code": -32602
+# CHECK-NEXT: "message": "onCodeAction called for non-added file"
+# CHECK-NEXT: },
+# CHECK-NEXT: "id": 2,
+Content-Length: 233
+
+{"jsonrpc":"2.0","id":3,"method":"textDocument/rangeFormatting","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":1,"character":4},"end":{"line":1,"character":12}},"options":{"tabSize":4,"insertSpaces":true}}}
+# CHECK: "error": {
+# CHECK-NEXT: "code": -32602
+# CHECK-NEXT: "message": "onDocumentRangeFormatting called for non-added file"
+# CHECK-NEXT: },
+# CHECK-NEXT: "id": 3,
+Content-Length: 153
+
+{"jsonrpc":"2.0","id":4,"method":"textDocument/formatting","params":{"textDocument":{"uri":"file:///foo.c"},"options":{"tabSize":4,"insertSpaces":true}}}
+# CHECK: "error": {
+# CHECK-NEXT: "code": -32602
+# CHECK-NEXT: "message": "onDocumentFormatting called for non-added file"
+# CHECK-NEXT: },
+# CHECK-NEXT: "id": 4,
+Content-Length: 204
+
+{"jsonrpc":"2.0","id":5,"method":"textDocument/onTypeFormatting","params":{"textDocument":{"uri":"file:///foo.c"},"position":{"line":3,"character":1},"ch":"}","options":{"tabSize":4,"insertSpaces":true}}}
+# CHECK: "error": {
+# CHECK-NEXT: "code": -32602
+# CHECK-NEXT: "message": "onDocumentOnTypeFormatting called for non-added file"
+# CHECK-NEXT: },
+# CHECK-NEXT: "id": 5,
+Content-Length: 44
+
+{"jsonrpc":"2.0","id":6,"method":"shutdown"}
+Content-Length: 33
+
+{"jsonrpc":"2.0","method":"exit"}
More information about the cfe-commits
mailing list