[clangd] Test Context can be used for file status. NFC

[ilya-biryukov]

Summary

The test checks the context in which we run addDocument can be used to
approximate the lifetime of processing the file for diagnostics.

[ilya-biryukov]

clangd/TUScheduler.h

77 ↗

(On Diff #171981)

To avoid boilerplate this could be changed to:

using DiagnosticsResult = Optional<vector<Diag>>;

Would still keep it a named typedef to allow documenting what the None value means.

[sammccall]

Discussed offline a bit. We should be judicious with intrusive changes to support out-of-tree clients, and this interface seems a bit messy.

There are alternatives with advantages:

1. less intrusive: clients can create a context before calling addDocument and observe its death to know that there will be no more notifications. This is a bit subtle, we can add a test.
2. avoid the special case: we could store diagnostics so we can always emit them (move out of ParsedAST). I actually think they belong in ParsedAST, and this is a bit wasteful, so not sure about this.
3. address the use case directly: the client wants to know what's going on with the file. We could define a per-file status structure, and send the client a stream of updates. (Via LSP: a notification like diagnostics themselves). This can be used for "is building", "has a fallback CompileCommand", etc. And debug info: size in index, AST size, preamble size...

My preference would be pushing clients towards 1 for now and getting to 3 eventually.

[ilya-biryukov]

• Instead of changing the interface, added a test we can rely on Context to give us what we need

[ilya-biryukov]

Added a test that the context behaves the way we want it to.

[ilya-biryukov]

• Fix a typo

[sammccall]

unittests/clangd/ClangdTests.cpp

1037

this tests a lot of things, and after 10 minutes I can't understand exactly what.

AIUI the thing we really want to verify is that if TUScheduler::update decides not to update, then context death is a timely signal that it did so.

(Doing this test at the ClangdServer level makes it more black-box, but I don't think that justifies the complexity.)

So isn't it enough to schedule 3 updates, make sure the middle one does no work, and verify its context is destroyed in between the other two? e.g.

atomic<int> Counter(0);
TUScheduler S;
Notification Start;
S.update("a.cc", inputs1, WantDiagnostics::Yes, []{
  // ensure we finish scheduling before everything finishes, else
  // we're not really testing anything
  Start.wait();
  EXPECT_EQ(1, ++Counter);
});
{
  WithContextValue WatchDestructor(llvm::make_scope_exit() {
    EXPECT_EQ(2, ++Counter);
  });
  S.update("a.cc", inputs1, WantDiagnostics::Yes, []{
    FAIL() << "Expect no diagnostics, inputs unchanged";
  });
}
S.update("a.cc", inputs2, WantDiagnostics::Yes, []{
  EXPECT_EQ(3, ++Counter);
});

EXPECT_EQ(0, Counter);
Start.notify();
S.blockUntilIdle();
EXPECT_EQ(3, Counter);

unittests/clangd/ClangdTests.cpp

 #include "Annotations.h"
 #include "ClangdLSPServer.h"
 #include "ClangdServer.h"
 #include "Matchers.h"
 #include "SyncAPI.h"
 #include "TestFS.h"
 #include "URI.h"
 #include "clang/Config/config.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <algorithm>
auto Completions = cantFail(runCodeComplete(Server, SourcePath, Code.point(),
                                               clangd::CodeCompleteOptions()))
                          .Completions;
   EXPECT_EQ(CountStats["foo.h"], Before);
   EXPECT_THAT(Completions,
               ElementsAre(Field(&CodeCompletion::Name, "TestSym")));
 }
 #endif
 
+TEST(ClangdTests, AddDocumentTracksDiagnostics) {
+  // We have clients relying on the fact that the Context passed to addDocument
+  // dies around the time we're done computing diagnostics for that particular
+  // request. This test checks this behavior does not break.
+
+  // Used to account for all diagnostic responses.
+  class TestDiagConsumer : public DiagnosticsConsumer {
+  public:
+    ~TestDiagConsumer() {
+      if (PendingDiags)
+        ADD_FAILURE() << "Diagnostics where not consumed";
+    }
+    void onDiagnosticsReady(PathRef, std::vector<Diag>) override {
+      ASSERT_FALSE(PendingDiags) << "Diagnostics were not consumed";
+      PendingDiags = true;
+    }
+
+    LLVM_NODISCARD bool consumeDiags() { return PendingDiags.exchange(false); }
+
+  private:
+    std::atomic<bool> PendingDiags{false};
+  };
+  MockCompilationDatabase CDB;
+  MockFSProvider FS;
+  TestDiagConsumer DiagConsumer;
+  ClangdServer S(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  StringRef SourceContents = "int foo() { return 10; }";
+  StringRef OtherSourceContents = "int a;";
+  std::atomic<bool> Called;
+  // New document is added, diagnostics must be produced.
+  Called = false;
+  {
+    WithContextValue SetCtx(llvm::make_scope_exit([&]() {
+      ASSERT_TRUE(DiagConsumer.consumeDiags());
+      Called = true;
+    }));
+    S.addDocument(FooCpp, SourceContents);
+  }
+  ASSERT_TRUE(S.blockUntilIdleForTest());
+  // Add a document with the same contents. The diagnostics will *not* be
+  // produced, but the callback should still happen when processing finishes.
+  Called = false;
+  {
+    WithContextValue SetCtx(llvm::make_scope_exit([&]() {
+      ASSERT_FALSE(DiagConsumer.consumeDiags());
+      Called = true;
+    }));
+    S.addDocument(FooCpp, SourceContents);
+  }
+  ASSERT_TRUE(S.blockUntilIdleForTest());
+  ASSERT_TRUE(Called);
+  // Check the case when contents change, but no diagnostics were requested.
+  Called = false;
+  {
+    WithContextValue SetCtx(llvm::make_scope_exit([&]() {
+      ASSERT_FALSE(DiagConsumer.consumeDiags());
+      Called = true;
+    }));
+    S.addDocument(FooCpp, OtherSourceContents, WantDiagnostics::No);
+  }
+  ASSERT_TRUE(S.blockUntilIdleForTest());
+  ASSERT_TRUE(Called);
+
+  // When ClangdServer gets many Auto and No requests, some of them might be
+  // dropped. But all the contexts should have the lifetime we expect anyway.
+  constexpr int RequestsCount = 10000;
+  std::atomic<int> Callbacks(0);
+  std::atomic<int> ConsumedDiags(0);
+  StringRef Sources[] = {SourceContents, OtherSourceContents};
+  for (int I = 0; I < RequestsCount; ++I) {
+    auto WD = I % 5 == 0 ? WantDiagnostics::No : WantDiagnostics::Auto;
+    WithContextValue SetCtx(llvm::make_scope_exit([&, WD, I]() {
+      EXPECT_EQ(Callbacks, I);
+      ++Callbacks;
+
+      bool HadDiags = DiagConsumer.consumeDiags();
+      if (HadDiags)
+        ++ConsumedDiags;
+      if (WD == WantDiagnostics::No)
+        EXPECT_FALSE(HadDiags);
+      // If WD == WantDiagnostics::Auto, HadDiags might have any of the two
+      // values.
+    }));
+    // Note that we always change the source to make sure we don't hit the
+    // "inputs did not change" optimization.
+    S.addDocument(FooCpp, Sources[I % 2], WD);
+  }
+  ASSERT_TRUE(S.blockUntilIdleForTest());
+  ASSERT_EQ(Callbacks, RequestsCount);
+  log("Test consumed {0} diagnostic responses", ConsumedDiags);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang