[clangd] Rebuild dependent files when a header is saved.

[sammccall]

Summary

Caveats:

• only works when the header is changed in the editor and the editor provides the notification
• we revalidate preambles for all open files (stat all their headers) rather than taking advantage of the fact that we know which file changed. This is much simpler!

Fixes https://github.com/clangd/clangd/issues/107

[kadircet]

should we also have a unittest for checking ast caching works as expected?

ClangdServer::getUsedBytesPerFile should allow us to do that.

We can set cache size to one, send 3 updates in respective order to foo, bar and baz, we record usedbytesperfile.
Then we perform adddocument with same contents to foo and bar and check usedbytesperfile are still the same?

clang-tools-extra/clangd/ClangdLSPServer.cpp

575

spec also specifies this as property name (optional): textDocumentSync.didSave near didSave notification (in addition to defining it as save in the struct). :(

it also says textDocumentSync can also be a number for backward compat reasons, but doesn't say when the struct was added. I hope it is not recent and we don't end up breaking clients with our capab response :/

no action needed just complaining ...

1587

nit: while here, braces and early exit.

for (..) {
  if(!Filter())
    continue;
  if(auto draft...) server->AddDoc();
}

[sammccall]

In D77847#1974126, @kadircet wrote:

should we also have a unittest for checking ast caching works as expected?

TUSchedulerTests has NoopOnEmptyChanges which I think tests exactly this, and EvictedAST which checks it's sensitive to the cache.

I don't think testing it again indirectly at this level makes is a good tradeoff, especially if we need subtle assertions via the memory usage.

clang-tools-extra/clangd/ClangdLSPServer.cpp

575

spec also specifies this as property name (optional): textDocumentSync.didSave

Good catch. VSCode and every other impl I could find implement save though, so I think that's just a spec bug. Sent https://github.com/microsoft/language-server-protocol/pull/958

(We could set both here, but I don't think it's a good idea to muddy the water)

it also says textDocumentSync can also be a number for backward compat reasons, but doesn't say when the struct was added. I hope it is not recent and we don't end up breaking clients with our capab response :/

yeah. I hadn't really internalized that all the nice compatibility hacks in the spec are for servers, and don't help much with clients.
I think this is pretty old though, and there's not much we can do about it anyway.

no action needed just complaining ...

Ack, and agreed :-)

[sammccall]

Address review comments

[kadircet]

Thanks, LGTM!

In D77847#1975940, @sammccall wrote:

In D77847#1974126, @kadircet wrote:

should we also have a unittest for checking ast caching works as expected?

TUSchedulerTests has NoopOnEmptyChanges which I think tests exactly this, and EvictedAST which checks it's sensitive to the cache.

Right, but none of them checks exactly this behaviour. The NoopOnEmptyChanges test only ensures we don't publish diagnostics again
and EvictedAST test actually asserts on the opposite, it checks the last updated file is "always" cached. Maybe extending EvictedAST
test with a "noop update" might be a better fit. Up to you though.

I don't think testing it again indirectly at this level makes is a good tradeoff, especially if we need subtle assertions via the memory usage.

To be clear I wasn't suggesting a test in ClangdLSPServer layer, but rather on ClangdServer layer. but as mentioned above, tuscheduler might
be a better level.

[sammccall]

In D77847#1976020, @kadircet wrote:

Thanks, LGTM!

In D77847#1975940, @sammccall wrote:

In D77847#1974126, @kadircet wrote:

should we also have a unittest for checking ast caching works as expected?

TUSchedulerTests has NoopOnEmptyChanges which I think tests exactly this, and EvictedAST which checks it's sensitive to the cache.

Right, but none of them checks exactly this behaviour. The NoopOnEmptyChanges test only ensures we don't publish diagnostics again
and EvictedAST test actually asserts on the opposite, it checks the last updated file is "always" cached. Maybe extending EvictedAST
test with a "noop update" might be a better fit. Up to you though.

OK, I misunderstood what we're testing, please tell me if I got it right this time :-)

Plausible bad behavior: we send a no-op change to a relatively inactive file. TUScheduler builds an AST (wasting CPU) and caches it (wasting CPU + latency by displacing a useful entry).

We want to prevent either of those from happening. Caching is indirectly observable, building an AST isn't particularly observable. If building an AST *was* observable, then we have an alternate (IMO more meaningful) way of measuring the cache effects: try to use read the maybe-evicted AST and see if it rebuilds.
So I think it would be nicer to structure a test around rebuilds.

A couple of options I can think of:

• add an API like unsigned TUScheduler::buildCount(PathRef), exposing a counter only used for testing? (Or extend the memory usage API to provide this info too)
• register a tracer for the test, and count BuildAST events (not too fragile if we assert there are exactly N > 0 of them). This seems hacky, technique/tracer may be reusable for other tests. Not sure whether that's a good thing.

(Unless you object, I'll land this patch once I have a test out for review so we're sure the existing behavior this patch depends on is there)

[kadircet]

In D77847#1976077, @sammccall wrote:

OK, I misunderstood what we're testing, please tell me if I got it right this time :-)

Plausible bad behavior: we send a no-op change to a relatively inactive file. TUScheduler builds an AST (wasting CPU) and caches it (wasting CPU + latency by displacing a useful entry).

We want to prevent either of those from happening. Caching is indirectly observable, building an AST isn't particularly observable.

Building an AST is also observable(at least in the absence of read requests), as we always generate diagnostics afterwards and that's what TUSchedulerTests.NoopOnEmptyChange actually asserts(that we don't build ASTs on noop changes).
I was suggesting to test the "cache" behaviour separately from building an ast. I know that's redundant today, as we never cache an ast if we haven't build any, but there are no tests ensuring it stays that way.

If building an AST *was* observable, then we have an alternate (IMO more meaningful) way of measuring the cache effects: try to use read the maybe-evicted AST and see if it rebuilds.
So I think it would be nicer to structure a test around rebuilds.

A couple of options I can think of:

• add an API like unsigned TUScheduler::buildCount(PathRef), exposing a counter only used for testing? (Or extend the memory usage API to provide this info too)
• register a tracer for the test, and count BuildAST events (not too fragile if we assert there are exactly N > 0 of them). This seems hacky, technique/tracer may be reusable for other tests. Not sure whether that's a good thing.

I suppose that could be a more robust way of checking if an AST was build rather than waiting for diags to be released. I believe the former would be enough(preferably just extending the existing API to not create more test-only endpoints).

(Unless you object, I'll land this patch once I have a test out for review so we're sure the existing behavior this patch depends on is there)

I totally agree, please do so.

clang-tools-extra/clangd/ClangdLSPServer.h

private:
   // Calls have signature void(const Params&, Callback<Response>).
   void onInitialize(const InitializeParams &, Callback<llvm::json::Value>);
   void onInitialized(const InitializedParams &);
   void onShutdown(const ShutdownParams &, Callback<std::nullptr_t>);
   void onSync(const NoParams &, Callback<std::nullptr_t>);
   void onDocumentDidOpen(const DidOpenTextDocumentParams &);
   void onDocumentDidChange(const DidChangeTextDocumentParams &);
   void onDocumentDidClose(const DidCloseTextDocumentParams &);
+  void onDocumentDidSave(const DidSaveTextDocumentParams &);
   void onDocumentOnTypeFormatting(const DocumentOnTypeFormattingParams &,
                                   Callback<std::vector<TextEdit>>);
   void onDocumentRangeFormatting(const DocumentRangeFormattingParams &,
                                  Callback<std::vector<TextEdit>>);
   void onDocumentFormatting(const DocumentFormattingParams &,
                             Callback<std::vector<TextEdit>>);
   // The results are serialized 'vector<DocumentSymbol>' if
   // SupportsHierarchicalDocumentSymbol is true and 'vector<SymbolInformation>'
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 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);
+  /// Requests a reparse of currently opened files using their latest source.
+  /// This will typically only rebuild if something other than the source has
+  /// changed (e.g. the CDB yields different flags, or files included in the
+  /// preamble have been modified).
+  void reparseOpenFilesIfNeeded(
+      llvm::function_ref<bool(llvm::StringRef File)> Filter);
   void applyConfiguration(const ConfigurationSettings &Settings);
 
   /// Sends a "publishSemanticHighlighting" notification to the LSP client.
   void
   publishTheiaSemanticHighlighting(const TheiaSemanticHighlightingParams &);
 
   /// Sends a "publishDiagnostics" notification to the LSP client.
   void publishDiagnostics(const PublishDiagnosticsParams &);

clang-tools-extra/clangd/ClangdLSPServer.cpp

CodeActionProvider = llvm::json::Object{
           CodeAction::INFO_KIND}}};
 
   llvm::json::Object Result{
       {{"serverInfo",
         llvm::json::Object{{"name", "clangd"},
                            {"version", getClangToolFullVersion("clangd")}}},
        {"capabilities",
         llvm::json::Object{
-            {"textDocumentSync", (int)TextDocumentSyncKind::Incremental},
+            {"textDocumentSync",
+             llvm::json::Object{
+                 {"openClose", true},
+                 {"change", (int)TextDocumentSyncKind::Incremental},
+                 {"save", true},
+             }},
             {"documentFormattingProvider", true},
             {"documentRangeFormattingProvider", true},
             {"documentOnTypeFormattingProvider",
              llvm::json::Object{
                  {"firstTriggerCharacter", "\n"},
                  {"moreTriggerCharacter", {}},
              }},
             {"codeActionProvider", std::move(CodeActionProvider)},
if (!Draft) {
     elog("Failed to update {0}: {1}", File, Draft.takeError());
     return;
   }
 
   Server->addDocument(File, Draft->Contents, encodeVersion(Draft->Version),
                       WantDiags, Params.forceRebuild);
 }
 
+void ClangdLSPServer::onDocumentDidSave(
+    const DidSaveTextDocumentParams &Params) {
+  reparseOpenFilesIfNeeded([](llvm::StringRef) { return true; });
+}
+
 void ClangdLSPServer::onFileEvent(const DidChangeWatchedFilesParams &Params) {
+  // We could also reparse all open files here. However:
+  //  - this could be frequent, and revalidating all the preambles isn't free
+  //  - this is useful e.g. when switching git branches, but we're likely to see
+  //    fresh headers but still have the old-branch main-file content
   Server->onFileEvent(Params);
 }
 
 void ClangdLSPServer::onCommand(const ExecuteCommandParams &Params,
                                 Callback<llvm::json::Value> Reply) {
   auto ApplyEdit = [this](WorkspaceEdit WE, std::string SuccessMessage,
                           decltype(Reply) Reply) {
     ApplyWorkspaceEditParams Edit;
auto New =
                                 std::move(Entry.second.compilationCommand),
                                 /*Output=*/"");
     if (Old != New) {
       CDB->setCompileCommand(File, std::move(New));
       ModifiedFiles.insert(File);
     }
   }
 
-  reparseOpenedFiles(ModifiedFiles);
+  reparseOpenFilesIfNeeded(
+      [&](llvm::StringRef File) { return ModifiedFiles.count(File) != 0; });
 }
 
 void ClangdLSPServer::publishTheiaSemanticHighlighting(
     const TheiaSemanticHighlightingParams &Params) {
   notify("textDocument/semanticHighlighting", Params);
 }
 
 void ClangdLSPServer::publishDiagnostics(
ClangdLSPServer::ClangdLSPServer(
   MsgHandler->bind("textDocument/hover", &ClangdLSPServer::onHover);
   MsgHandler->bind("textDocument/documentSymbol", &ClangdLSPServer::onDocumentSymbol);
   MsgHandler->bind("workspace/executeCommand", &ClangdLSPServer::onCommand);
   MsgHandler->bind("textDocument/documentHighlight", &ClangdLSPServer::onDocumentHighlight);
   MsgHandler->bind("workspace/symbol", &ClangdLSPServer::onWorkspaceSymbol);
   MsgHandler->bind("textDocument/didOpen", &ClangdLSPServer::onDocumentDidOpen);
   MsgHandler->bind("textDocument/didClose", &ClangdLSPServer::onDocumentDidClose);
   MsgHandler->bind("textDocument/didChange", &ClangdLSPServer::onDocumentDidChange);
+  MsgHandler->bind("textDocument/didSave", &ClangdLSPServer::onDocumentDidSave);
   MsgHandler->bind("workspace/didChangeWatchedFiles", &ClangdLSPServer::onFileEvent);
   MsgHandler->bind("workspace/didChangeConfiguration", &ClangdLSPServer::onChangeConfiguration);
   MsgHandler->bind("textDocument/symbolInfo", &ClangdLSPServer::onSymbolInfo);
   MsgHandler->bind("textDocument/typeHierarchy", &ClangdLSPServer::onTypeHierarchy);
   MsgHandler->bind("typeHierarchy/resolve", &ClangdLSPServer::onResolveTypeHierarchy);
   MsgHandler->bind("textDocument/selectionRange", &ClangdLSPServer::onSelectionRange);
   MsgHandler->bind("textDocument/documentLink", &ClangdLSPServer::onDocumentLink);
   MsgHandler->bind("textDocument/semanticTokens", &ClangdLSPServer::onSemanticTokens);
void ClangdLSPServer::onFileUpdated(PathRef File, const TUStatus &Status) {
   // reasonable time interval (e.g. 0.5s).
   if (Status.PreambleActivity == PreambleAction::Idle &&
       (Status.ASTActivity.K == ASTAction::Building ||
        Status.ASTActivity.K == ASTAction::RunningAction))
     return;
   notify("textDocument/clangd.fileStatus", Status.render(File));
 }
 
-void ClangdLSPServer::reparseOpenedFiles(
-    const llvm::StringSet<> &ModifiedFiles) {
-  if (ModifiedFiles.empty())
-    return;
+void ClangdLSPServer::reparseOpenFilesIfNeeded(
+    llvm::function_ref<bool(llvm::StringRef File)> Filter) {
   // Reparse only opened files that were modified.
   for (const Path &FilePath : DraftMgr.getActiveFiles())
-    if (ModifiedFiles.find(FilePath) != ModifiedFiles.end())
+    if (Filter(FilePath))
       if (auto Draft = DraftMgr.getDraft(FilePath)) // else disappeared in race?
         Server->addDocument(FilePath, std::move(Draft->Contents),
                             encodeVersion(Draft->Version),
                             WantDiagnostics::Auto);
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/Protocol.h

 bool fromJSON(const llvm::json::Value &, DidOpenTextDocumentParams &);
 
 struct DidCloseTextDocumentParams {
   /// The document that was closed.
   TextDocumentIdentifier textDocument;
 };
 bool fromJSON(const llvm::json::Value &, DidCloseTextDocumentParams &);
 
+struct DidSaveTextDocumentParams {
+  /// The document that was saved.
+  TextDocumentIdentifier textDocument;
+};
+bool fromJSON(const llvm::json::Value &, DidSaveTextDocumentParams &);
+
 struct TextDocumentContentChangeEvent {
   /// The range of the document that changed.
   llvm::Optional<Range> range;
 
   /// The length of the range that got replaced.
   llvm::Optional<int> rangeLength;
 
   /// The new text of the range/document.

clang-tools-extra/clangd/Protocol.cpp

bool fromJSON(const llvm::json::Value &Params, DidOpenTextDocumentParams &R) {
   return O && O.map("textDocument", R.textDocument);
 }
 
 bool fromJSON(const llvm::json::Value &Params, DidCloseTextDocumentParams &R) {
   llvm::json::ObjectMapper O(Params);
   return O && O.map("textDocument", R.textDocument);
 }
 
+bool fromJSON(const llvm::json::Value &Params, DidSaveTextDocumentParams &R) {
+  llvm::json::ObjectMapper O(Params);
+  return O && O.map("textDocument", R.textDocument);
+}
+
 bool fromJSON(const llvm::json::Value &Params, DidChangeTextDocumentParams &R) {
   llvm::json::ObjectMapper O(Params);
   if (!O)
     return false;
   O.map("forceRebuild", R.forceRebuild);  // Optional clangd extension.
   return O.map("textDocument", R.textDocument) &&
          O.map("contentChanges", R.contentChanges) &&
          O.map("wantDiagnostics", R.wantDiagnostics);

clang-tools-extra/clangd/test/initialize-params.test

 # CHECK-NEXT:        "rangeProvider": false
 # CHECK-NEXT:      },
 # CHECK-NEXT:      "signatureHelpProvider": {
 # CHECK-NEXT:        "triggerCharacters": [
 # CHECK-NEXT:          "(",
 # CHECK-NEXT:          ","
 # CHECK-NEXT:        ]
 # CHECK-NEXT:      },
-# CHECK-NEXT:      "textDocumentSync": 2,
+# CHECK-NEXT:      "textDocumentSync": {
+# CHECK-NEXT:        "change": 2,
+# CHECK-NEXT:        "openClose": true,
+# CHECK-NEXT:        "save": true
+# CHECK-NEXT:      },
 # CHECK-NEXT:      "typeHierarchyProvider": true
 # CHECK-NEXT:      "workspaceSymbolProvider": true
 # CHECK-NEXT:    },
 # CHECK-NEXT:    "serverInfo": {
 # CHECK-NEXT:      "name": "clangd",
 # CHECK-NEXT:      "version": "{{.*}}clangd version {{.*}}"
 # CHECK-NEXT:    }
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
 #      CHECK:  "id": 3,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": null
 ---
 {"jsonrpc":"2.0","method":"exit"}

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

EXPECT_THAT(Client.diagnostics("foo.cpp"),
               llvm::ValueIs(testing::ElementsAre(
                   DiagMessage("Cannot initialize a variable of type 'int' with "
                               "an lvalue of type 'const char [3]'"))));
 
   Client.didClose("foo.cpp");
   EXPECT_THAT(Client.diagnostics("foo.cpp"), llvm::ValueIs(testing::IsEmpty()));
 }
 
+TEST_F(LSPTest, DiagnosticsHeaderSaved) {
+  auto &Client = start();
+  FS.Files["foo.h"] = "#define VAR original";
+  Client.didOpen("foo.cpp", R"cpp(
+    #include "foo.h"
+    int x = VAR;
+  )cpp");
+  EXPECT_THAT(Client.diagnostics("foo.cpp"),
+              llvm::ValueIs(testing::ElementsAre(
+                  DiagMessage("Use of undeclared identifier 'original'"))));
+  // Now modify the header from within the "editor".
+  FS.Files["foo.h"] = "#define VAR changed";
+  Client.notify(
+      "textDocument/didSave",
+      llvm::json::Object{{"textDocument", Client.documentID("foo.h")}});
+  // Foo.cpp should be rebuilt with new diagnostics.
+  EXPECT_THAT(Client.diagnostics("foo.cpp"),
+              llvm::ValueIs(testing::ElementsAre(
+                  DiagMessage("Use of undeclared identifier 'changed'"))));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang