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