ilya-biryukov updated this revision to Diff 130127. ilya-biryukov added a comment.
- Do the check in rename before calling into ClangdServer Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42164 Files: clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clangd/ClangdServer.h test/clangd/crash-non-added-files.test
Index: test/clangd/crash-non-added-files.test =================================================================== --- /dev/null +++ test/clangd/crash-non-added-files.test @@ -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"} Index: clangd/ClangdServer.h =================================================================== --- clangd/ClangdServer.h +++ clangd/ClangdServer.h @@ -308,11 +308,11 @@ 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 Index: clangd/ClangdServer.cpp =================================================================== --- clangd/ClangdServer.cpp +++ clangd/ClangdServer.cpp @@ -356,7 +356,6 @@ 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 @@ 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) { Index: clangd/ClangdLSPServer.cpp =================================================================== --- clangd/ClangdLSPServer.cpp +++ clangd/ClangdLSPServer.cpp @@ -143,14 +143,19 @@ 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,43 +169,59 @@ 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())); } 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())); } 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())); } 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);
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits