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

[Haojian Wu via Phabricator via cfe-commits]

hokein 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);
----------------
ilya-biryukov wrote:
> 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.
The race condition of highlighting sounds bad (especially when a user opens a large file and closes it immediately, then the highlighting is finished and we emit it to the client). No need to fix it in this patch, just add a FIXME.


Can we use the same mechanism for Diagnostic to guard the highlighting here? or use the `DiagsMu` and `ReportDiagnostics` to guard the `callback.onMainAST()` as well (which is probably not a good idea)...


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:307
 
+TEST(SemanticHighlighting, HighlightingDiffer) {
+  struct {
----------------
jvikstrom wrote:
> ilya-biryukov wrote:
> > Can we test this in a more direct manner by specifying:
> > 1. annotated input for old highlightings,
> > 2. annotated input for new highlightings,
> > 3. the expected diff?
> > 
> > The resulting tests don't have to be real C++ then, allowing to express what we're testing in a more direct manner.
> > ```
> > {/*Old*/ "$Variable[[a]]", /*New*/ "$Class[[a]]", /*Diff*/ "{{/*line */0, "$Class[[a]]"}}
> > ```
> > 
> > It also seems that the contents of the lines could even be checked automatically (they should be equal to the corresponding line from `/*New*/`, right?), that leaves us with even simpler inputs:
> > ```
> > {/*Old*/ "$Variable[[a]]", /*New*/ "$Class[[a]]", /*DiffLines*/ "{0}}
> > ```
> That's a great idea on how to write these tests.
hmm, I have a different opinion here (I'd prefer the previous one), let's chat.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475