[clangd] Check if CompileCommand has changed on forceReparse.

Reviewers: krasimir, bkramer, klimek

Reviewed By: klimek

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D36398

llvm-svn: 310819

clang-tools-extra/clangd/ClangdServer.cpp

@@ -154,9 +154,20 @@ std::future<void> ClangdServer::removeDocument(PathRef File) {
 }
 
 std::future<void> ClangdServer::forceReparse(PathRef File) {
-  // The addDocument schedules the reparse even if the contents of the file
-  // never changed, so we just call it here.
-  return addDocument(File, getDocument(File));
+  auto FileContents = DraftMgr.getDraft(File);
+  assert(FileContents.Draft &&
+         "forceReparse() was called for non-added document");
+
+  auto TaggedFS = FSProvider.getTaggedFileSystem(File);
+  auto Recreated = Units.recreateFileIfCompileCommandChanged(
+      File, ResourceDir, CDB, PCHs, TaggedFS.Value);
+
+  // Note that std::future from this cleanup action is ignored.
+  scheduleCancelRebuild(std::move(Recreated.RemovedFile));
+  // Schedule a reparse.
+  return scheduleReparseAndDiags(File, std::move(FileContents),
+                                 std::move(Recreated.FileInCollection),
+                                 std::move(TaggedFS));
 }
 
 Tagged<std::vector<CompletionItem>>

clang-tools-extra/clangd/ClangdServer.h

@@ -192,10 +192,13 @@ public:
   std::future<void> addDocument(PathRef File, StringRef Contents);
   /// Remove \p File from list of tracked files, schedule a request to free
   /// resources associated with it.
-  /// \return A future that will become ready the file is removed and all
-  /// associated reosources are freed.
+  /// \return A future that will become ready when the file is removed and all
+  /// associated resources are freed.
   std::future<void> removeDocument(PathRef File);
   /// Force \p File to be reparsed using the latest contents.
+  /// Will also check if CompileCommand, provided by GlobalCompilationDatabase
+  /// for \p File has changed. If it has, will remove currently stored Preamble
+  /// and AST and rebuild them from scratch.
   std::future<void> forceReparse(PathRef File);
 
   /// Run code completion for \p File at \p Pos. If \p OverridenContents is not

clang-tools-extra/clangd/ClangdUnitStore.cpp

@@ -9,6 +9,7 @@
 
 #include "ClangdUnitStore.h"
 #include "llvm/Support/Path.h"
+#include <algorithm>
 
 using namespace clang::clangd;
 using namespace clang;
@@ -25,6 +26,32 @@ std::shared_ptr<CppFile> CppFileCollection::removeIfPresent(PathRef File) {
   return Result;
 }
 
+CppFileCollection::RecreateResult
+CppFileCollection::recreateFileIfCompileCommandChanged(
+    PathRef File, PathRef ResourceDir, GlobalCompilationDatabase &CDB,
+    std::shared_ptr<PCHContainerOperations> PCHs,
+    IntrusiveRefCntPtr<vfs::FileSystem> VFS) {
+  auto NewCommand = getCompileCommand(CDB, File, ResourceDir);
+
+  std::lock_guard<std::mutex> Lock(Mutex);
+
+  RecreateResult Result;
+
+  auto It = OpenedFiles.find(File);
+  if (It == OpenedFiles.end()) {
+    It = OpenedFiles
+             .try_emplace(File, CppFile::Create(File, std::move(NewCommand),
+                                                std::move(PCHs)))
+             .first;
+  } else if (!compileCommandsAreEqual(It->second->getCompileCommand(),
+                                      NewCommand)) {
+    Result.RemovedFile = std::move(It->second);
+    It->second = CppFile::Create(File, std::move(NewCommand), std::move(PCHs));
+  }
+  Result.FileInCollection = It->second;
+  return Result;
+}
+
 tooling::CompileCommand
 CppFileCollection::getCompileCommand(GlobalCompilationDatabase &CDB,
                                      PathRef File, PathRef ResourceDir) {
@@ -39,3 +66,12 @@ CppFileCollection::getCompileCommand(GlobalCompilationDatabase &CDB,
                                          std::string(ResourceDir));
   return std::move(Commands.front());
 }
+
+bool CppFileCollection::compileCommandsAreEqual(
+    tooling::CompileCommand const &LHS, tooling::CompileCommand const &RHS) {
+  // tooling::CompileCommand.Output is ignored, it's not relevant for clangd.
+  return LHS.Directory == RHS.Directory &&
+         LHS.CommandLine.size() == RHS.CommandLine.size() &&
+         std::equal(LHS.CommandLine.begin(), LHS.CommandLine.end(),
+                    RHS.CommandLine.begin());
+}

clang-tools-extra/clangd/ClangdUnitStore.h

@@ -42,6 +42,25 @@ public:
     return It->second;
   }
 
+  struct RecreateResult {
+    /// A CppFile, stored in this CppFileCollection for the corresponding
+    /// filepath after calling recreateFileIfCompileCommandChanged.
+    std::shared_ptr<CppFile> FileInCollection;
+    /// If a new CppFile had to be created to account for changed
+    /// CompileCommand, a previous CppFile instance will be returned in this
+    /// field.
+    std::shared_ptr<CppFile> RemovedFile;
+  };
+
+  /// Similar to getOrCreateFile, but will replace a current CppFile for \p File
+  /// with a new one if CompileCommand, provided by \p CDB has changed.
+  /// If a currently stored CppFile had to be replaced, the previous instance
+  /// will be returned in RecreateResult.RemovedFile.
+  RecreateResult recreateFileIfCompileCommandChanged(
+      PathRef File, PathRef ResourceDir, GlobalCompilationDatabase &CDB,
+      std::shared_ptr<PCHContainerOperations> PCHs,
+      IntrusiveRefCntPtr<vfs::FileSystem> VFS);
+
   std::shared_ptr<CppFile> getFile(PathRef File) {
     std::lock_guard<std::mutex> Lock(Mutex);
 
@@ -59,6 +78,9 @@ private:
   tooling::CompileCommand getCompileCommand(GlobalCompilationDatabase &CDB,
                                             PathRef File, PathRef ResourceDir);
 
+  bool compileCommandsAreEqual(tooling::CompileCommand const &LHS,
+                               tooling::CompileCommand const &RHS);
+
   std::mutex Mutex;
   llvm::StringMap<std::shared_ptr<CppFile>> OpenedFiles;
 };

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

@@ -501,6 +501,53 @@ std::string x;
 }
 #endif // LLVM_ON_UNIX
 
+TEST_F(ClangdVFSTest, ForceReparseCompileCommand) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
+  ClangdServer Server(CDB, DiagConsumer, FS,
+                      /*RunSynchronously=*/true);
+  // No need to sync reparses, because RunSynchronously is set
+  // to true.
+
+  auto FooCpp = getVirtualTestFilePath("foo.cpp");
+  const auto SourceContents1 = R"cpp(
+template <class T>
+struct foo { T x; };
+)cpp";
+  const auto SourceContents2 = R"cpp(
+template <class T>
+struct bar { T x; };
+)cpp";
+
+  FS.Files[FooCpp] = "";
+  FS.ExpectedFile = FooCpp;
+
+  // First parse files in C mode and check they produce errors.
+  CDB.ExtraClangFlags = {"-xc"};
+  Server.addDocument(FooCpp, SourceContents1);
+  EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
+  Server.addDocument(FooCpp, SourceContents2);
+  EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
+
+  // Now switch to C++ mode.
+  CDB.ExtraClangFlags = {"-xc++"};
+  // Currently, addDocument never checks if CompileCommand has changed, so we
+  // expect to see the errors.
+  Server.addDocument(FooCpp, SourceContents1);
+  EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
+  Server.addDocument(FooCpp, SourceContents2);
+  EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
+  // But forceReparse should reparse the file with proper flags.
+  Server.forceReparse(FooCpp);
+  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());
+}
+
 class ClangdCompletionTest : public ClangdVFSTest {
 protected:
   bool ContainsItem(std::vector<CompletionItem> const &Items, StringRef Name) {