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

[Simon Marchi via Phabricator via cfe-commits]

simark marked 2 inline comments as done.
simark added inline comments.


================
Comment at: clangd/ClangdLSPServer.cpp:101
         json::obj{
-            {"textDocumentSync", 1},
+            {"textDocumentSync", 2},
             {"documentFormattingProvider", true},
----------------
ilya-biryukov wrote:
> A comment mentioning that 2 means incremental document sync from LSP would be useful here.
I opted to add a TextDocumentSyncKind enum to Protocol.h and use that.  Though instead of an enum, I did a class with static const fields:

```
struct TextDocumentSyncKind {
  /// Documents should not be synced at all.
  static const int None = 0;

  /// Documents are synced by always sending the full content of the document.
  static const int Full = 1;

  /// Documents are synced by sending the full content on open.  After that
  /// only incremental updates to the document are send.
  static const int Incremental = 2;
};
```

otherwise it would've required an (int) cast when using it to generate JSON:

    {"textDocumentSync", TextDocumentSyncKind::Incremental},


================
Comment at: clangd/ClangdLSPServer.cpp:149
 void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams &Params) {
   if (Params.contentChanges.size() != 1)
     return replyError(ErrorCode::InvalidParams,
----------------
ilya-biryukov wrote:
> We should handle more than a single change here.
Ok, I'll try that.  I am not sure I interpret the spec correctly.  If you have two changes, the second change applies to the state of the document after having applied the first change, is that right?  If that's the case, I think we only need to iterate on the changes and call addDocument on them sequentially (which could be done in the DraftMgr, given the interface change to DraftStore you suggest lower).


================
Comment at: clangd/ClangdServer.cpp:557
     // 64-bit unsigned integer.
     if (Version < LastReportedDiagsVersion)
       return;
----------------
ilya-biryukov wrote:
> When you'll try remove the `DraftMgr`, this piece of code will be hard to refactor because it relies on `DocVersion`.
> This is a workaround for a possible race condition that should really be handled by TUScheduler rather than this code, but we haven't got around to fixing it yet. (It's on the list, though, would probably get in next week).
> 
> A proper workaround for now would be to add `llvm::StringMap<uint64_t> InternalVersions` field to `ClangdServer`, increment those versions on each call to `addDocument` and use them here in the same way we currently use DraftMgr's versions.
Hmm ok, it will probably make sense when I try to do it.  The `InternalVersions` map maps URIs to the latest version seen?

Is the race condition because we could get diagnostics for a stale version of the document, so we don't want to emit them?


================
Comment at: clangd/ClangdServer.h:159
   /// constructor will receive onDiagnosticsReady callback.
   void addDocument(PathRef File, StringRef Contents,
+                   WantDiagnostics WantDiags = WantDiagnostics::Auto,
----------------
ilya-biryukov wrote:
> 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.
> 
`ClangdServer` also needs `DraftMgr` for `forceReparse` and `reparseOpenedFiles`.  I guess that too would move to `ClangdLSPServer`?  I'm just not sure how to adapt the unit tests, since we don't have unittests using `ClangdLSPServer` (yet).


================
Comment at: clangd/DraftStore.cpp:47
+DocVersion DraftStore::updateDraft(PathRef File, StringRef Contents,
+                                   llvm::Optional<Range> range,
+                                   std::string *NewContents) {
----------------
ilya-biryukov wrote:
> NIT: LLVM uses `UpperCamelCase` for parameters and local variables
Woops.  I should learn to use clang-tidy.  It found other instances (the local variables) but it doesn't find the parameters not written in camel case.  Do you have an idea why?  I dumped the config and see these:

```
  - key:             readability-identifier-naming.ClassCase
    value:           CamelCase
  - key:             readability-identifier-naming.EnumCase
    value:           CamelCase
  - key:             readability-identifier-naming.UnionCase
    value:           CamelCase
  - key:             readability-identifier-naming.VariableCase
    value:           CamelCase
```

I assume there must be a `ParamCase` or something like that, but I can't find the exhaustive list of parameters for that check.


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


================
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);
----------------
ilya-biryukov wrote:
> 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.
By `DraftManager` I suppose you mean `DraftStore`?  I'm fine with that interface change.


================
Comment at: clangd/Protocol.h:289
+  llvm::Optional<Range> range;
+
+  /// The new text of the range/document.
----------------
ilya-biryukov wrote:
> 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.
Ok.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44272