[clangd] Only publish preamble after rebuilds

[kadircet]

Summary

Don't invoke parsing callback for preamble if clangd is using a
previously built one.

[sammccall]

clang-tools-extra/clangd/TUScheduler.cpp

904

I think it would be clearer to invert the sense here (ReusedPreamble=false).

This avoids the "tension" of saying RebuiltPreamble=true when we haven't built one yet, and then having to ensure we satisfy that promise later.

clang-tools-extra/clangd/TUScheduler.cpp

auto Task = [=, Action = std::move(Action)]() mutable {
     Action(InputsAndAST{FileInputs, **AST});
   };
   startTask(Name, std::move(Task), /*Update=*/None, Invalidation);
 }
 
 void PreambleThread::build(Request Req) {
   assert(Req.CI && "Got preamble request with null compiler invocation");
   const ParseInputs &Inputs = Req.Inputs;
+  bool ReusedPreamble = false;
 
   Status.update([&](TUStatus &Status) {
     Status.PreambleActivity = PreambleAction::Building;
   });
-  auto _ = llvm::make_scope_exit([this, &Req] {
+  auto _ = llvm::make_scope_exit([this, &Req, &ReusedPreamble] {
     ASTPeer.updatePreamble(std::move(Req.CI), std::move(Req.Inputs),
                            LatestBuild, std::move(Req.CIDiags),
                            std::move(Req.WantDiags));
+    if (!ReusedPreamble)
       Callbacks.onPreamblePublished(FileName);
   });
 
   if (!LatestBuild || Inputs.ForceRebuild) {
     vlog("Building first preamble for {0} version {1}", FileName,
          Inputs.Version);
   } else if (isPreambleCompatible(*LatestBuild, Inputs, FileName, *Req.CI)) {
     vlog("Reusing preamble version {0} for version {1} of {2}",
          LatestBuild->Version, Inputs.Version, FileName);
+    ReusedPreamble = true;
     return;
   } else {
     vlog("Rebuilding invalidated preamble for {0} version {1} (previous was "
          "version {2})",
          FileName, Inputs.Version, LatestBuild->Version);
   }
 
   LatestBuild = clang::clangd::buildPreamble(

clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp

S.runWithAST("test", File, [&](Expected<InputsAndAST> AST) {
     EXPECT_THAT(AST->AST.preambleVersion(), InputsV0);
     EXPECT_THAT(AST->Inputs.Version, InputsV1.str());
     RunASTAction.notify();
   });
   RunASTAction.wait();
   Ready.notify();
 }
 
+TEST_F(TUSchedulerTests, OnlyPublishWhenPreambleIsBuilt) {
+  struct PreamblePublishCounter : public ParsingCallbacks {
+    PreamblePublishCounter(int &PreamblePublishCount)
+        : PreamblePublishCount(PreamblePublishCount) {}
+    void onPreamblePublished(PathRef File) override { ++PreamblePublishCount; }
+    int &PreamblePublishCount;
+  };
+
+  int PreamblePublishCount = 0;
+  TUScheduler S(CDB, optsForTest(),
+                std::make_unique<PreamblePublishCounter>(PreamblePublishCount));
+
+  Path File = testPath("foo.cpp");
+  S.update(File, getInputs(File, ""), WantDiagnostics::Auto);
+  S.blockUntilIdle(timeoutSeconds(10));
+  EXPECT_EQ(PreamblePublishCount, 1);
+  // Same contents, no publish.
+  S.update(File, getInputs(File, ""), WantDiagnostics::Auto);
+  S.blockUntilIdle(timeoutSeconds(10));
+  EXPECT_EQ(PreamblePublishCount, 1);
+  // New contents, should publish.
+  S.update(File, getInputs(File, "#define FOO"), WantDiagnostics::Auto);
+  S.blockUntilIdle(timeoutSeconds(10));
+  EXPECT_EQ(PreamblePublishCount, 2);
+}
+
 // If a header file is missing from the CDB (or inferred using heuristics), and
 // it's included by another open file, then we parse it using that files flags.
 TEST_F(TUSchedulerTests, IncluderCache) {
   static std::string Main = testPath("main.cpp"), Main2 = testPath("main2.cpp"),
                      Main3 = testPath("main3.cpp"),
                      NoCmd = testPath("no_cmd.h"),
                      Unreliable = testPath("unreliable.h"),
                      OK = testPath("ok.h"),