[clangd] Check if CompileCommand has changed on forceReparse.

[klimek]

clangd/ClangdUnitStore.cpp

45 ↗

(On Diff #109994)

Just say RemovedFile = nullptr in the struct?

clangd/ClangdUnitStore.h

45–48 ↗

(On Diff #109994)

Not obvious to me what things in there mean.

[ilya-biryukov]

• Moved assignment RemoveFile = nullptr around a bit.
• Added a comment to recreateFileIfCompileCommandChanged.

clangd/ClangdUnitStore.cpp

45 ↗

(On Diff #109994)

I'd argue that having it in the code, rather than declaration makes it more readable. Besides, default ctor of shared_ptr gives us nullptr anyway, so we can also leave it as is and remove the assignments altogether.

I've moved =nullptr assignments out of the if/else bodies. Let me know if it still looks ugly.

clangd/ClangdUnitStore.h

45–48 ↗

(On Diff #109994)

Added a comment. Hopefully makes sense now.

[klimek]

Also missing tests :)

clangd/ClangdUnitStore.cpp

45 ↗

(On Diff #109994)

Ah, I missed that it's a smart pointer; in that case, yes, remove the = nullptr.

clangd/ClangdUnitStore.h

45–48 ↗

(On Diff #109994)

Better, thanks. Now, why does this need to be shared_ptr (as opposed to unique_ptr)? Don't we always only have one?

[ilya-biryukov]

Addressed review comments.

• Added a test for forceReparse.
• Got rid of a redundant = nullptr assignment.

[ilya-biryukov]

In D36398#834904, @klimek wrote:

Also missing tests :)

Done :-)

clangd/ClangdUnitStore.h

45–48 ↗

(On Diff #109994)

This CppFile may still be referenced from the worker threads that store a pending reparse task for this file. Or there may even be an active reparse on a separate thread.

[klimek]

lg

unittests/clangd/ClangdTests.cpp

509–510 ↗

(On Diff #110155)

Nit: Comment fits in one line.

clang-tools-extra/trunk/clangd/ClangdServer.h

public:
   /// \p File is already tracked. Also schedules parsing of the AST for it on a
   /// separate thread. When the parsing is complete, DiagConsumer passed in
   /// constructor will receive onDiagnosticsReady callback.
   /// \return A future that will become ready when the rebuild (including
   /// diagnostics) is finished.
   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
   /// None, they will used only for code completion, i.e. no diagnostics update
   /// will be scheduled and a draft for \p File will not be updated.
   /// If \p OverridenContents is None, contents of the current draft for \p File
   /// will be used.
   /// If \p UsedFS is non-null, it will be overwritten by vfs::FileSystem used

clang-tools-extra/trunk/clangd/ClangdServer.cpp

 
 std::future<void> ClangdServer::removeDocument(PathRef File) {
   DraftMgr.removeDraft(File);
   std::shared_ptr<CppFile> Resources = Units.removeIfPresent(File);
   return scheduleCancelRebuild(std::move(Resources));
 }
 
 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>>
 ClangdServer::codeComplete(PathRef File, Position Pos,
                            llvm::Optional<StringRef> OverridenContents,
                            IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS) {
   std::string DraftStorage;
   if (!OverridenContents) {

clang-tools-extra/trunk/clangd/ClangdUnitStore.h

if (It == OpenedFiles.end()) {
       It = OpenedFiles
                .try_emplace(File, CppFile::Create(File, std::move(Command),
                                                   std::move(PCHs)))
                .first;
     }
     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);
 
     auto It = OpenedFiles.find(File);
     if (It == OpenedFiles.end())
       return nullptr;
     return It->second;
   }
 
   /// Removes a CppFile, stored for \p File, if it's inside collection and
   /// returns it.
   std::shared_ptr<CppFile> removeIfPresent(PathRef File);
 
 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;
 };
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/trunk/clangd/ClangdUnitStore.cpp

 //===--- ClangdUnitStore.cpp - A ClangdUnits container -----------*-C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ClangdUnitStore.h"
 #include "llvm/Support/Path.h"
+#include <algorithm>
 
 using namespace clang::clangd;
 using namespace clang;
 
 std::shared_ptr<CppFile> CppFileCollection::removeIfPresent(PathRef File) {
   std::lock_guard<std::mutex> Lock(Mutex);
 
   auto It = OpenedFiles.find(File);
   if (It == OpenedFiles.end())
     return nullptr;
 
   std::shared_ptr<CppFile> Result = It->second;
   OpenedFiles.erase(It);
   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) {
   std::vector<tooling::CompileCommand> Commands = CDB.getCompileCommands(File);
   if (Commands.empty())
     // Add a fake command line if we know nothing.
     Commands.push_back(getDefaultCompileCommand(File));
 
   // Inject the resource dir.
   // FIXME: Don't overwrite it if it's already there.
   Commands.front().CommandLine.push_back("-resource-dir=" +
                                          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/trunk/unittests/clangd/ClangdTests.cpp

 #include <string>
 std::string x;
 )cpp";
   Server.addDocument(FooCpp, SourceContentsWithError);
   EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
 }
 #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) {
     for (const auto &Item : Items) {
       if (Item.insertText == Name)
         return true;
     }
     return false;