[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:458
+    // edit there are stale previous highlightings.
+    std::lock_guard<std::mutex> Lock(HighlightingsMutex);
+    FileToHighlightings.erase(File);
----------------
jvikstrom wrote:
> ilya-biryukov wrote:
> > Should can't we handle this on `didClose` instead?
> We are removing in didClose but the problem is that there is a race condition (I think).
> 
> If someone does some edits and closes the document right after, the highlightings for the final edit might finish being generated after the FileToHighlightings have earsed the highlightings for the file. So next time when opening the file we will have those final highlightings that were generated for the last edit in the map. 
> I don't really know how we could handle this in didClose without having another map and with an open/closed bit and checking that every time we generate new highlightings. But we'd still have to set the bit to be open every didOpen so I don't really see the benefit.
> 
> However I've head that ASTWorked is synchronous for a single file, is that the case for the didClose call as well? Because in that case there is no race condition.
You are correct, there is actually a race condition. We worked hard to eliminate it for diagnostics, but highlightings are going through a different code path in `ASTWorker`, not guarded by `DiagsMu` and `ReportDiagnostics`.

And, unfortunately, I don't think this guard here prevents it in all cases. In particular, there is still a possibility (albeit very low, I guess) that the old highlightings you are trying to remove here are still being computed. If they are reported **after** this `erase()` runs, we will end up with inconsistent highlightings.

Ideally, we would guard the diagnostics callback with the same mutex, but given our current layering it seems like a hard problem, will need to think what's the simplest way to fix this.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:79
+std::vector<LineHighlightings>
+diffHighlightings(ArrayRef<HighlightingToken> NewHighlightings,
+                  ArrayRef<HighlightingToken> OldHighlightings);
----------------
jvikstrom wrote:
> ilya-biryukov wrote:
> > Are we locked into the line-based diff implementation?
> > It works nicely while editing inside the same line, but seem to do a bad job on a common case of inserting/removing lines.
> > 
> > Does the protocol have a way to communicate this cleanly?
> We are looked into some kind of line based diff implementation as the protocol sends token line by line. 
> There should be away of solving the inserting/removing lines, but I'll have a look at that in a follow up. 
> Theia seems to do a good job of moving tokens to where they should be automatically when inserting a new line though. I want to be able to see how vscode handles it first as well though before.
It seems there no compact way to send interesting diffs then, although the clients can do a reasonably good job of moving the older version of highlightings on changes until the server sends them a new version.

The diff-based approach only seems to be helping with changes on a single line.

Which is ok, thanks for the explanation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64475/new/

https://reviews.llvm.org/D64475