[PATCH] D44272: [clangd] Support incremental document syncing

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov requested changes to this revision.
ilya-biryukov added inline comments.
This revision now requires changes to proceed.


================
Comment at: clangd/ClangdLSPServer.cpp:101
         json::obj{
-            {"textDocumentSync", 1},
+            {"textDocumentSync", 2},
             {"documentFormattingProvider", true},
----------------
A comment mentioning that 2 means incremental document sync from LSP would be useful here.


================
Comment at: clangd/ClangdLSPServer.cpp:149
 void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams &Params) {
   if (Params.contentChanges.size() != 1)
     return replyError(ErrorCode::InvalidParams,
----------------
We should handle more than a single change here.


================
Comment at: clangd/ClangdServer.h:159
   /// constructor will receive onDiagnosticsReady callback.
   void addDocument(PathRef File, StringRef Contents,
+                   WantDiagnostics WantDiags = WantDiagnostics::Auto,
----------------
I don't think we should do this on `ClangdServer` level. We will have to copy the whole file anyway before passing it to clang, so there are no performance wins here and it complicates the interface.
I suggest we update the document text from diffs on `ClangdLSPServer` level and continue passing the whole document to `ClangdServer`.
It would mean `DraftMgr` will need to move to `ClangdLSPServer`, but it's fine.



================
Comment at: clangd/DraftStore.cpp:47
+DocVersion DraftStore::updateDraft(PathRef File, StringRef Contents,
+                                   llvm::Optional<Range> range,
+                                   std::string *NewContents) {
----------------
NIT: LLVM uses `UpperCamelCase` for parameters and local variables


================
Comment at: clangd/DraftStore.cpp:57
+    if (!Entry.Draft.hasValue()) {
+      log(llvm::Twine(
+              "Trying to do incremental update on draft without content: ") +
----------------
We should return an error to the client in that case. Changing return type to `llvm::Expected<DocVersion>` and handling an error in the callers should do the trick.


================
Comment at: clangd/DraftStore.cpp:66
+
+    newContents = Entry.Draft->substr(0, startIndex);
+    newContents += Contents;
----------------
We need to check that `startIndex` and `endIndex` are inside the bounds of the string.


================
Comment at: clangd/DraftStore.h:60
   /// \return The new version of the draft for \p File.
-  DocVersion updateDraft(PathRef File, StringRef Contents);
+  DocVersion updateDraft(PathRef File, StringRef Contents,
+                         llvm::Optional<Range> Range, std::string *NewContents);
----------------
Let's model the LSP more closely here:

```
// Protocol.h
struct TextDocumentContentChangeEvent {
  llvm::Optional<Range> range;
  llvm::Optional<int>  rangeLength;
  std::string text;
};

/// DraftManager
class DraftManager {
  /// For initial didOpen.
  DocVersion addDraft(PathRef File, std::string Contents);

  /// For didChange, handles both incremental and full updates. \p Changes cannot be empty.
  DocVersion updateDraft(PathRef File, ArrayRef<TextDocumentContentChangeEvent> Changes);
};
```

Keeping track of LSP versions here could also be useful to make sure we drop any updates in between (which would definitely lead to surprising results), but it's ok if we add a FIXME and do it later.


================
Comment at: clangd/Protocol.h:289
+  llvm::Optional<Range> range;
+
+  /// The new text of the range/document.
----------------
Please add `rangeLength` from lsp spec too. On one hand, it seems redundant, but it can be used to add assertions that the text is the same.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44272