[PATCH] D144456: [clangd] Publish diagnostics with stale preambles

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Looks pretty good. I think we can build the main file & preamble in parallel. Otherwise nits.



================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:946
+    // gurantee eventual consistency.
+    if (LatestPreamble && Config::current().Diagnostics.AllowStalePreamble)
+      generateDiagnostics(std::make_unique<CompilerInvocation>(*Invocation),
----------------
We should probably kick off the preamble rebuild before doing this synchronous AST rebuild, so the two can run in parallel.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1131
+    // Use latest file contents instead of the preamble inputs whenever we can
+    // emit stale diagnostics. This ensures diagnostics are newer backwards,
+    // e.g. after emitting stale diagnostics for version N + 1, if a preamble
----------------
This explanation focuses on the potential problems (why the other behavior is wrong) and on the AllowStalePreamble mode only. I think it would be more enlightening to talk both about problems and correctness, and about the old mode and the new mode.

Maybe:
```
The file may have been edited since we started building this preamble.

If diagnostics need a fresh preamble, we must use the old version that matches the preamble. We make forward progress as updatePreamble() receives increasing versions, and this is the only place we emit diagnostics.

If diagnostics can use a stale preamble, we use the current contents of the file instead.
This provides more up-to-date diagnostics, and avoids diagnostics going backwards (we may have already emitted staler-preamble diagnostics for the new version). We still have eventual consistency: at some point updatePreamble() will catch up to the current file.
```


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1202
+TEST_F(TUSchedulerTests, PublishWithStalePreamble) {
+  class BlockPreambleThread : public ParsingCallbacks {
+  public:
----------------
class comment?

// Callbacks that blocks the preamble thread after the first preamble is built.


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1225
+
+    std::optional<std::pair<std::string, std::string>>
+    WaitForNewDiags(TUScheduler &S, PathRef File, ParseInputs PI) {
----------------
having a method on the PreambleCallbacks which calls update() on the scheduler seems pretty confusing - outside its responsibilities.

I'd suggest just moving this method and its associated state out of the class, and have onMainAST just invoke a callback. (I see the appeal of wrapping this in a class, I just don't think it should be *this* class)


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1236
+          });
+      if (!ReceivedDiags)
+        return std::nullopt;
----------------
this nullopt stuff isn't used in the test, ADD_FAILURE and return something random instead?


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1260
+
+  Notification BlockPreamble;
+  auto DiagCallbacks = std::make_unique<BlockPreambleThread>(BlockPreamble);
----------------
this should rather be *Unblock*Preamble, I think, as that's what triggering it does?

I'd suggest using this name rather than `N` inside the class too, would make it a bit less mysterious


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1272
+  PI.Version = PI.Contents = "1";
+  ASSERT_THAT(BlockForDiags(PI), testing::Pair("1", PI.Version));
+
----------------
nit: Pair("1", "1") - tests should be explicit rather than DRY
maybe even `Pair(/*Preamble=*/"1", /*Main=*/"1")`

(and in the next two - the "eventual consistency" assert is probably fine as-is I think)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144456