[clangd] Don't crash on LSP calls for non-added files

[ilya-biryukov]

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.

[sammccall]

clangd/ClangdLSPServer.cpp

153

don't you want to do this before computing replacements?

155

invalidparams

[ilya-biryukov]

• Renamed crash.test to crash-non-added-files.test

[ilya-biryukov]

• Do the check in rename before calling into ClangdServer

[ilya-biryukov]

clangd/ClangdLSPServer.cpp

153

Makes sense.

155

I used InternalError, because rename should've returned an error for non-added file. If we do the check before calling rename, InvalidParams make more sense.

clangd/ClangdLSPServer.cpp

 void ClangdLSPServer::onRename(Ctx C, RenameParams &Params) {
   auto File = Params.textDocument.uri.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);
+
+  auto Code = Server.getDocument(File);
+  if (!Code)
+    return replyError(C, ErrorCode::InternalError,
+                      "onRename called for non-added file");
+
+  std::vector<TextEdit> Edits = replacementsToEdits(*Code, *Replacements);
   WorkspaceEdit WE;
   WE.changes = {{Params.textDocument.uri.uri, Edits}};
   reply(C, WE);
 }
 
 void ClangdLSPServer::onDocumentDidClose(Ctx C,
                                          DidCloseTextDocumentParams &Params) {
   Server.removeDocument(std::move(C), Params.textDocument.uri.file);
 }
 
 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);
     if (!Edits.empty()) {
       WorkspaceEdit WE;
       WE.changes = {{Params.textDocument.uri.uri, std::move(Edits)}};
       Commands.push_back(json::obj{
           {"title", llvm::formatv("Apply FixIt {0}", D.message)},

clangd/ClangdServer.h

public:
   formatOnType(StringRef Code, PathRef File, Position Pos);
 
   /// Rename all occurrences of the symbol at the \p Pos in \p File to
   /// \p NewName.
   Expected<std::vector<tooling::Replacement>> rename(const Context &Ctx,
                                                      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
   /// \p File. \p File must be in the list of added documents.
   std::string dumpAST(PathRef File);
   /// Called when an event occurs for a watched file in the workspace.
   void onFileEvent(const DidChangeWatchedFilesParams &Params);
 

clangd/ClangdServer.cpp

ClangdServer::formatOnType(StringRef Code, PathRef File, Position Pos) {
   size_t Len = CursorPos - PreviousLBracePos;
 
   return formatCode(Code, File, {tooling::Range(PreviousLBracePos, Len)});
 }
 
 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) {
     const SourceManager &SourceMgr = AST->getASTContext().getSourceManager();
     const FileEntry *FE =
         SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
     if (!FE)
       return;
for (const auto &Rep : ChangeReps) {
       //   * rename in open files
       if (Rep.getFilePath() == File)
         Replacements.push_back(Rep);
     }
   }
   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) {
     llvm::raw_string_ostream ResultOS(Result);
     if (AST) {
       clangd::dumpAST(*AST, ResultOS);
     } else {
       ResultOS << "<no-ast>";

test/clangd/crash-non-added-files.test

+# 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"}