[clangd] Always use the latest preamble

[hokein]

Summary

Fix an inconsistent behavior of using LastBuiltPreamble/NewPreamble
in TUScheduler (see the test for details), AST should always use
NewPreamble. This patch makes LastBuiltPreamble always point to
NewPreamble.

Preamble rarely fails to build, even there are errors in headers, so we
assume it would not cause performace issue for code completion.

[ilya-biryukov]

Thanks, we were previously getting inconsistent ASTs (i.e. using different preambles), depending on whether they were built in TUScheduler::update or in TUScheduler::runWithAST handlers.
Just a few NITs.

clangd/TUScheduler.cpp

373

Maybe remove the comment? It does not seem to add any additional information on top of the code.

unittests/clangd/XRefsTests.cpp

1058

Maybe use runFindDefinitions instead of the two Server.findDefinitions() + Server.blockUntilIdleForTest() calls?

1061

NIT: just use cantFail(std::move(Loc)) to get the underlying vector and remove ASSERT_TRUE.
This gives equivalent results (crashes tests with assert failure internally in LLVM) with a simpler syntax.

1073

Same here: maybe use runFindDefinitions?

[hokein]

Address review comments.

[ilya-biryukov]

LGTM

clangd/TUScheduler.cpp

std::shared_ptr<const PreambleData> OldPreamble =
         getPossiblyStalePreamble();
     std::shared_ptr<const PreambleData> NewPreamble =
         buildPreamble(FileName, *Invocation, OldPreamble, OldCommand, Inputs,
                       PCHs, StorePreambleInMemory, PreambleCallback);
 
     bool CanReuseAST = InputsAreTheSame && (OldPreamble == NewPreamble);
     {
       std::lock_guard<std::mutex> Lock(Mutex);
-      if (NewPreamble)
-        LastBuiltPreamble = NewPreamble;
+      // Always use the NewPreamble.
     }
     // Before doing the expensive AST reparse, we want to release our reference
     // to the old preamble, so it can be freed if there are no other references
     // to it.
     OldPreamble.reset();
     PreambleWasBuilt.notify();
 
     if (!CanReuseAST) {

unittests/clangd/XRefsTests.cpp

TEST(GoToInclude, All) {
   ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
   EXPECT_THAT(*Locations, IsEmpty());
 
   Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("7"));
   ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
   EXPECT_THAT(*Locations, IsEmpty());
 }
 
+TEST(GoToDefinitoin, WithPreamble) {
+  // Test stragety: AST should always use the latest preamble instead of last
+  // good preamble.
+  MockFSProvider FS;
+  IgnoreDiagnostics DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  auto FooCppUri = URIForFile{FooCpp};
+  // The trigger locations must be the same.
+  Annotations FooWithHeader(R"cpp(#include "fo^o.h")cpp");
+  Annotations FooWithoutHeader(R"cpp(double    [[fo^o]]();)cpp");
+
+  FS.Files[FooCpp] = FooWithHeader.code();
+
+  auto FooH = testPath("foo.h");
+  auto FooHUri = URIForFile{FooH};
+  Annotations FooHeader(R"cpp([[]])cpp");
+  FS.Files[FooH] = FooHeader.code();
+
+  runAddDocument(Server, FooCpp, FooWithHeader.code());
+  // GoToDefinition goes to a #include file: the result comes from the preamble.
+  Server.findDefinitions(
+      FooCpp, FooWithHeader.point(),
+      [&](llvm::Expected<std::vector<Location>> Loc) {
+        ASSERT_TRUE(bool(Loc));
+        EXPECT_THAT(*Loc, ElementsAre(Location{FooHUri, FooHeader.range()}));
+      });
+
+  // Only preamble is built, and no AST is built in this request.
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::No);
+  // We build AST here, and it should use the latest preamble rather than the
+  // stale one.
+  Server.findDefinitions(
+      FooCpp, FooWithoutHeader.point(),
+      [&](llvm::Expected<std::vector<Location>> Loc) {
+        ASSERT_TRUE(bool(Loc));
+        EXPECT_THAT(*Loc,
+                    ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
+      });
+  EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Wait for GoToDefinition";
+
+
+  // Reset test environment.
+  runAddDocument(Server, FooCpp, FooWithHeader.code());
+  // Both preamble and AST are built in this request.
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::Yes);
+  // Use the AST being built in above request.
+  Server.findDefinitions(
+      FooCpp, FooWithoutHeader.point(),
+      [&](llvm::Expected<std::vector<Location>> Loc) {
+        ASSERT_TRUE(bool(Loc));
+        EXPECT_THAT(*Loc,
+                    ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
+      });
+  EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Wait for GoToDefinition";
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang