[clangd] Do not reuse preamble if compile args changed

[simark]

I don't have the setup to test at the moment, but the code looks good to me.

clangd/ClangdUnit.h

public:
   std::size_t getUsedBytes() const;
 
 private:
   /// Build a new preamble for \p Inputs. If the current preamble can be reused,
   /// it is returned instead.
   /// This method is const to ensure we don't incidentally modify any fields.
   std::shared_ptr<const PreambleData>
   rebuildPreamble(CompilerInvocation &CI,
+                  const tooling::CompileCommand &Command,
                   IntrusiveRefCntPtr<vfs::FileSystem> FS,
                   llvm::MemoryBuffer &ContentsBuffer) const;
 
   Path FileName;
   bool StorePreamblesInMemory;
 
+  /// The last CompileCommand used to build AST and Preamble.
+  tooling::CompileCommand Command;
   /// The last parsed AST.
   llvm::Optional<ParsedAST> AST;
   /// The last built Preamble.
   std::shared_ptr<const PreambleData> Preamble;
   /// Utility class required by clang
   std::shared_ptr<PCHContainerOperations> PCHs;
   /// This is called after the file is parsed. This can be nullptr if there is
   /// no callback.

clangd/ClangdUnit.cpp

 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 
 using namespace clang::clangd;
 using namespace clang;
 
 namespace {
 
+bool compileCommandsAreEqual(const tooling::CompileCommand &LHS,
+                             const tooling::CompileCommand &RHS) {
+  // We don't check for Output, it should not matter to clangd.
+  return LHS.Directory == RHS.Directory && LHS.Filename == RHS.Filename &&
+         llvm::makeArrayRef(LHS.CommandLine).equals(RHS.CommandLine);
+}
+
 template <class T> std::size_t getUsedBytes(const std::vector<T> &Vec) {
   return Vec.capacity() * sizeof(T);
 }
 
 class DeclTrackingASTConsumer : public ASTConsumer {
 public:
   DeclTrackingASTConsumer(std::vector<const Decl *> &TopLevelDecls)
       : TopLevelDecls(TopLevelDecls) {}
std::unique_ptr<CompilerInvocation> CI;
     CI->getFrontendOpts().DisableFree = false;
   }
 
   std::unique_ptr<llvm::MemoryBuffer> ContentsBuffer =
       llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents, FileName);
 
   // Compute updated Preamble.
   std::shared_ptr<const PreambleData> NewPreamble =
-      rebuildPreamble(*CI, Inputs.FS, *ContentsBuffer);
+      rebuildPreamble(*CI, Inputs.CompileCommand, Inputs.FS, *ContentsBuffer);
 
   // Remove current AST to avoid wasting memory.
   AST = llvm::None;
   // Compute updated AST.
   llvm::Optional<ParsedAST> NewAST;
   {
     trace::Span Tracer("Build");
     SPAN_ATTACH(Tracer, "File", FileName);
Diagnostics.insert(Diagnostics.end(), NewAST->getDiagnostics().begin(),
                        NewAST->getDiagnostics().end());
   }
   if (ASTCallback && NewAST) {
     trace::Span Tracer("Running ASTCallback");
     ASTCallback(FileName, NewAST.getPointer());
   }
 
   // Write the results of rebuild into class fields.
+  Command = std::move(Inputs.CompileCommand);
   Preamble = std::move(NewPreamble);
   AST = std::move(NewAST);
   return Diagnostics;
 }
 
 const std::shared_ptr<const PreambleData> &CppFile::getPreamble() const {
   return Preamble;
 }
if (AST)
     Total += AST->getUsedBytes();
   if (StorePreamblesInMemory && Preamble)
     Total += Preamble->Preamble.getSize();
   return Total;
 }
 
 std::shared_ptr<const PreambleData>
 CppFile::rebuildPreamble(CompilerInvocation &CI,
+                         const tooling::CompileCommand &Command,
                          IntrusiveRefCntPtr<vfs::FileSystem> FS,
                          llvm::MemoryBuffer &ContentsBuffer) const {
   const auto &OldPreamble = this->Preamble;
   auto Bounds = ComputePreambleBounds(*CI.getLangOpts(), &ContentsBuffer, 0);
-  if (OldPreamble &&
+  if (OldPreamble && compileCommandsAreEqual(this->Command, Command) &&
       OldPreamble->Preamble.CanReuse(CI, &ContentsBuffer, Bounds, FS.get())) {
     log("Reusing preamble for file " + Twine(FileName));
     return OldPreamble;
   }
   log("Preamble for file " + Twine(FileName) +
       " cannot be reused. Attempting to rebuild it.");
 
   trace::Span Tracer("Preamble");

unittests/clangd/ClangdTests.cpp

)cpp";
   EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
   // Subsequent addDocument calls should finish without errors too.
   Server.addDocument(FooCpp, SourceContents1);
   EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
   Server.addDocument(FooCpp, SourceContents2);
   EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
 }
 
+TEST_F(ClangdVFSTest, ForceReparseCompileCommandDefines) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, DiagConsumer, FS,
+                      /*AsyncThreadsCount=*/0,
+                      /*StorePreamblesInMemory=*/true);
+
+  // No need to sync reparses, because reparses are performed on the calling
+  // thread.
+  auto FooCpp = testPath("foo.cpp");
+  const auto SourceContents = R"cpp(
+#ifdef WITH_ERROR
+this
+#endif
+
+int main() { return 0; }
+)cpp";
+  FS.Files[FooCpp] = "";
+  FS.ExpectedFile = FooCpp;
+
+  // Parse with define, we expect to see the errors.
+  CDB.ExtraClangFlags = {"-DWITH_ERROR"};
+  Server.addDocument(FooCpp, SourceContents);
+  EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
+
+  // Parse without the define, no errors should be produced.
+  CDB.ExtraClangFlags = {};
+  // Currently, addDocument never checks if CompileCommand has changed, so we
+  // expect to see the errors.
+  Server.addDocument(FooCpp, SourceContents);
+  EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
+  // But forceReparse should reparse the file with proper flags.
+  Server.forceReparse(FooCpp);
+  EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
+  // Subsequent addDocument call should finish without errors too.
+  Server.addDocument(FooCpp, SourceContents);
+  EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
+}
+
 TEST_F(ClangdVFSTest, MemoryUsage) {
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB;
   ClangdServer Server(CDB, DiagConsumer, FS,
                       /*AsyncThreadsCount=*/0,
                       /*StorePreamblesInMemory=*/true);