[clangd] Only re-open files if their flags changed

[merge_guards_bot]

Unit tests: fail. 61795 tests passed, 1 failed and 781 were skipped.

failed: Clangd.Clangd/did-change-configuration-params.test

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[dgoldman]

• Fix broken did-change-configuration test

[merge_guards_bot]

Unit tests: pass. 61796 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[sammccall]

clang-tools-extra/clangd/ClangdLSPServer.cpp

1129

nit: llvm::StringSet<> or DenseSet<string> are the usual type here

1130

this is a clever technique. (Why not just use compilationDatabaseChanges directly? I suppose because then you have to deal more with path canonicalization?)

it risks having the CDB change concurrently and reloading those files too, though.
I guess there's not much harm in it. But in that case, why aren't we just permanently subscribing to CDB changes and re-parsing affected files? Lack of a thread to do it on?

[dgoldman]

clang-tools-extra/clangd/ClangdLSPServer.cpp

1130

Yeah I think compilationDatabaseChanges would be equivalent to what is here now, I can just swap to that.

For the perma subscribe I wasn't sure of the threading. If addDocument is thread safe I think we're okay to just call Server->AddDocument from whatever thread without holding a mutex?

[sammccall]

Let's make the minimal change here and land this.

clang-tools-extra/clangd/ClangdLSPServer.cpp

1130

ClangdServer isn't threadsafe, that's a good point.

Yeah I think compilationDatabaseChanges would be equivalent to what is here now, I can just swap to that.

This is the minimal change, let's do that.

[dgoldman]

• Swap to stringset and simpler way of tracking modified files

[dgoldman]

In D72647#1833203, @sammccall wrote:

Let's make the minimal change here and land this.

Done

clang-tools-extra/clangd/ClangdLSPServer.h

 #include "Features.inc"
 #include "FindSymbols.h"
 #include "GlobalCompilationDatabase.h"
 #include "Path.h"
 #include "Protocol.h"
 #include "Transport.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringSet.h"
 #include <memory>
 
 namespace clang {
 namespace clangd {
 
 class SymbolIndex;
 
 /// This class exposes ClangdServer's capabilities via Language Server Protocol.
private:
   std::vector<Fix> getFixes(StringRef File, const clangd::Diagnostic &D);
 
   /// Checks if completion request should be ignored. We need this due to the
   /// limitation of the LSP. Per LSP, a client sends requests for all "trigger
   /// character" we specify, but for '>' and ':' we need to check they actually
   /// produce '->' and '::', respectively.
   bool shouldRunCompletion(const CompletionParams &Params) const;
 
-  /// Forces a reparse of all currently opened files.  As a result, this method
-  /// may be very expensive.  This method is normally called when the
-  /// compilation database is changed.
-  void reparseOpenedFiles();
+  /// Forces a reparse of all currently opened files which were modified. As a
+  /// result, this method may be very expensive. This method is normally called
+  /// when the compilation database is changed.
+  void reparseOpenedFiles(const llvm::StringSet<> &ModifiedFiles);
   void applyConfiguration(const ConfigurationSettings &Settings);
 
   /// Sends a "publishSemanticHighlighting" notification to the LSP client.
   void publishSemanticHighlighting(SemanticHighlightingParams Params);
 
   /// Sends a "publishDiagnostics" notification to the LSP client.
   void publishDiagnostics(const URIForFile &File,
                           std::vector<clangd::Diagnostic> Diagnostics);

clang-tools-extra/clangd/ClangdLSPServer.cpp

void ClangdLSPServer::onResolveTypeHierarchy(
     Callback<Optional<TypeHierarchyItem>> Reply) {
   Server->resolveTypeHierarchy(Params.item, Params.resolve, Params.direction,
                                std::move(Reply));
 }
 
 void ClangdLSPServer::applyConfiguration(
     const ConfigurationSettings &Settings) {
   // Per-file update to the compilation database.
-  bool ShouldReparseOpenFiles = false;
+  llvm::StringSet<> ModifiedFiles;
   for (auto &Entry : Settings.compilationDatabaseChanges) {
-    /// The opened files need to be reparsed only when some existing
-    /// entries are changed.
     PathRef File = Entry.first;
     auto Old = CDB->getCompileCommand(File);
     auto New =
         tooling::CompileCommand(std::move(Entry.second.workingDirectory), File,
                                 std::move(Entry.second.compilationCommand),
                                 /*Output=*/"");
     if (Old != New) {
       CDB->setCompileCommand(File, std::move(New));
-      ShouldReparseOpenFiles = true;
+      ModifiedFiles.insert(File);
     }
   }
-  if (ShouldReparseOpenFiles)
-    reparseOpenedFiles();
+
+  reparseOpenedFiles(ModifiedFiles);
 }
 
 void ClangdLSPServer::publishSemanticHighlighting(
     SemanticHighlightingParams Params) {
   notify("textDocument/semanticHighlighting", Params);
 }
 
 void ClangdLSPServer::publishDiagnostics(
void ClangdLSPServer::onFileUpdated(PathRef File, const TUStatus &Status) {
   // changing, and doesn't provide much value. We may want to emit status at a
   // reasonable time interval (e.g. 0.5s).
   if (Status.Action.S == TUAction::BuildingFile ||
       Status.Action.S == TUAction::RunningAction)
     return;
   notify("textDocument/clangd.fileStatus", Status.render(File));
 }
 
-void ClangdLSPServer::reparseOpenedFiles() {
+void ClangdLSPServer::reparseOpenedFiles(
+    const llvm::StringSet<> &ModifiedFiles) {
+  if (ModifiedFiles.empty())
+    return;
+  // Reparse only opened files that were modified.
   for (const Path &FilePath : DraftMgr.getActiveFiles())
+    if (ModifiedFiles.find(FilePath) != ModifiedFiles.end())
       Server->addDocument(FilePath, *DraftMgr.getDraft(FilePath),
                           WantDiagnostics::Auto);
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/test/did-change-configuration-params.test

 # CHECK-NEXT:      }
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "uri": "file://{{.*}}/foo.c"
 # CHECK-NEXT:  }
 #
 # ERR: Updating file {{.*}}foo.c with command
 # ERR: [{{.*}}clangd-test2]
 # ERR: clang -c foo.c -Wall -Werror
-# Don't reparse the second file:
-# ERR: Skipping rebuild of the AST for {{.*}}bar.c
 ---
 {"jsonrpc":"2.0","id":5,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}