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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks again. This generally looks LGTM, just a last drop of small nits. Will approve as soon as they land.



================
Comment at: clangd/DraftStore.cpp:75
+      return llvm::make_error<llvm::StringError>(
+          llvm::formatv("Range's end position (line={0}, character={1}) is "
+                        "before start position (line={2}, character={3}).",
----------------
Could we add a format provide for `Position` to make this formatting code shorter?
We'll need to define the corresponding specialization in `Protocol.h`:

```
namespace llvm {
  template <>
  struct format_provider<clang::clangd::Position> {
    static void format(const clang::clangd::Position &Pos, raw_ostream &OS, StringRef Style) { 
     assert(Style.empty() && "style modifiers for this type are not supported");
     OS << Pos;
    }
  };
}
```

This will allow to simplify this call site to:
```
llvm::formatv("Range's end position ({0}) is before start position ({1})", End, Start)
```


================
Comment at: clangd/DraftStore.cpp:100
+  EntryIt->second = Contents;
+  return std::move(Contents);
+}
----------------
Doing `return std::move(localvar)` is a pessimization. Use `return Contents;` instead.
 (for reference see [[https://stackoverflow.com/questions/14856344/when-should-stdmove-be-used-on-a-function-return-value|this SO post]], clang should give a warning for that case too)


================
Comment at: clangd/DraftStore.h:36
   /// Replace contents of the draft for \p File with \p Contents.
-  void updateDraft(PathRef File, StringRef Contents);
+  void addDraft(PathRef File, StringRef Contents);
+
----------------
simark wrote:
> ilya-biryukov wrote:
> > ilya-biryukov wrote:
> > > simark wrote:
> > > > ilya-biryukov wrote:
> > > > > Could we add versions from LSP's `VersionedTextDocumentIdentifier` here and make the relevant sanity checks?
> > > > > Applying diffs to the wrong version will cause everything to fall apart, so we should detect this error and signal it to the client as soon as possible.
> > > > I agree that applying diffs to the wrong version will break basically everything, but even if we detect a version mismatch, I don't see what we could do, since we don't have a mean to report the error to the client.  The only thing we could do is log it (which we already do.
> > > If we couldn't apply a diff, we should return errors from all future operations on this file until it gets closed and reopened. Otherwise clangd and the editor would see inconsistent contents for the file and results provided by clangd would be unreliable.
> > > The errors from any actions on the invalid file would actually be visible to the users.
> > > 
> > > The simplest way to achieve that is to remove the file from `DraftStore` and `ClangdServer` on errors from `updateDraft`.
> > > This will give "calling action on non-tracked file" errors for future operations and the actual root cause can be figured out from the logs.
> > We still ignore version numbers from the LSP.
> > Is this another change that didn't get in?
> The more I think about it, the less sure I am that this is the intended usage of the version.  The spec doesn't even say what the initial version of a file should be, 0 or 1?  Then, it just says that the version optionally contained in the `VersionedTextDocumentIdentifier` reflects the version of the document after having applied the edit.  But I don't think we can predict and validate what that version should be.  Most clients will probably just use a sequence number, but I guess they don't have to...
> 
> Also, I initially assumed that having N changes in the `contentChanges` array would mean that the version number would be bumped by N.  But now that I re-read it, there's really nothing that says it should behave like this.
> 
> I think that we should just record the version number and treat it as opaque, so that we can use it later when sending text edits to the client, for example.  The client can then verify that the edit is for the same version of the document that it has at the moment.
> 
> Therefore, I don't think it would really be useful in this patch.
The initial version is provided in `didOpen` (see `TextDocumentItem`).
I've also misread description of `contentChanges` in the same way. It's true that versions might grow by any  [[https://github.com/Microsoft/language-server-protocol/issues/241|any positive number]], so we can't rely on them for the checks I propose here.
Thanks for looking into that and explaining the spec :-)


================
Comment at: clangd/DraftStore.h:26
 /// filenames. The contents are owned by the DraftStore.
 class DraftStore {
 public:
----------------
Could we also mention in the comment that this class now handles applying incremental document changes?


================
Comment at: unittests/clangd/DraftStoreTests.cpp:36
+/// Send the changes one by one to updateDraft, verify the intermediate results.
+static void stepByStep(llvm::ArrayRef<IncrementalTestStep> Steps) {
+  DraftStore DS;
----------------
NIT: another redundant static




================
Comment at: unittests/clangd/DraftStoreTests.cpp:62
+/// Send all the changes at once to updateDraft, check only the final result.
+static void allAtOnce(llvm::ArrayRef<IncrementalTestStep> Steps) {
+  DraftStore DS;
----------------
NIT: another redundant `static`


================
Comment at: unittests/clangd/DraftStoreTests.cpp:66
+  Annotations FinalSrc(Steps.back().Src);
+  const char Path[] = "/hello.cpp";
+  std::vector<TextDocumentContentChangeEvent> Changes;
----------------
NIT: another instance of `constexpr StringLiteral`


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44272