[clang] Add `forceReload` clangd extension to 'textDocument/didChange'

[dgoldman]

Summary

• This option forces a preamble rebuild to handle the odd case of a missing header file being added

[dgoldman]

• Fix comment

[merge_guards_bot]

Unit tests: pass. 62418 tests passed, 0 failed and 845 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 2 warnings. 2 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

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

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[merge_guards_bot]

Unit tests: pass. 62418 tests passed, 0 failed and 845 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 2 warnings. 2 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

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

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[sammccall]

Thanks for this. It makes me a bit sad but I don't really see a clean way to make this "just work".

clang-tools-extra/clangd/ClangdLSPServer.cpp

650

Rather than plumbing this down many levels as yet-another-bool-param, can we shove it into either ParseInputs or ParseOptions?
e.g. bool ParseInputs::AllowCached = true; and set it to false here?

It's kind of a stretch for the API as we won't make use of it everywhere that ParseInputs is used, but I think it's better than adding a niche boolean param everywhere.

clang-tools-extra/clangd/Protocol.h

656

Nit: the preamble is an implementation detail here, an even higher-level description would be e.g.

Force a complete rebuild of the file, ignoring all cached state. Slow!
This is useful to defeat clangd's assumption that missing headers stay missing.

661

just bool forceReload = false; - we don't bother modelling this as a tristate if there's a well-defined defaut.

(Though I'm sure there are places where we are inconsistent...)

661

somehow "rebuild" seems clearer than "reload" to me, because we're not being very specific about what we're loading.

Also allowCache (with inverted sense) might be clearer.

clang-tools-extra/clangd/TUScheduler.cpp

438

no need to pass a separate parameter through to buildPreamble - if you're forcing a preamble rebuild then just don't bother getting the old preamble. OldPreamble is null and you get the behaviour you want.

(The logging will be slightly off - "first preamble" - but I don't think it's worth fixing)

[dgoldman]

• Refactor to use forceRebuild and ParseOptions

[dgoldman]

clang-tools-extra/clangd/ClangdLSPServer.cpp

650

Done, this cleaned up the tests

[dgoldman]

• Rebase

[sammccall]

Looks pretty good now, thanks! Test needs to be more precise (doesn't actually test the behavior at present, I think).

clang-tools-extra/clangd/Compiler.h

41

Hmm, on thinking further I do think this belongs in ParseInputs next to Contents or CompileCommands, as these things are things that affect the parse but *don't* invalidate caches.

41

this is a good place for a comment explaining what this does specifically (prevents reuse of cached preamble/ast)

clang-tools-extra/clangd/Protocol.h

660

= false, and remove "disabled by default" from comment

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

619

This test doesn't actually verify that the preamble was rebuilt, just that the AST was.
Checking the actual diagnostics would do that.

631

I don't think this abstraction is useful since you just have 3 calls and two of them need the actual diagnostics.

You could have diag callbacks inline easily enough:

vector<vector<Diag>> diags;
update(..., [] { diags.push_back(...) })
update(..., [] { ADD_FAILURE(...) })
update(..., [] { diags.push_back(...) })
blockUntilIdle();
EXPECT_THAT(diags, ElementsAre(
  /* First */ElementsAre(...missing header...),
  /* Last */IsEmpty());

[dgoldman]

• Fixes for tests and InputFiles

[sammccall]

LG, thanks! Just a nit about further simplifying the test, up to you.

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

630

this still feels just a bit obfuscated, what about more directly:

ParseInputs I = getInputs(Source, SourceContents);
updateWithDiags(..., I, [] { ... });
Files[Header] = ...;
updateWithDiags(...);
I.ForceRebuild = true;
updateWithDiags(...);

[dgoldman]

• Minor test fixes

[dgoldman]

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

630

still needed to recreate ParseInputs but done

[nridge]

Which client(s) use or plan to use this extension?

[dgoldman]

In D73916#1881396, @nridge wrote:

Which client(s) use or plan to use this extension?

SourceKit-LSP https://github.com/apple/sourcekit-lsp/pull/242

clang-tools-extra/clangd/ClangdLSPServer.cpp

if (!Contents) {
     // the client.  It is better to remove the draft and let further operations
     // fail rather than giving wrong results.
     DraftMgr.removeDraft(File);
     Server->removeDocument(File);
     elog("Failed to update {0}: {1}", File, Contents.takeError());
     return;
   }
 
-  Server->addDocument(File, *Contents, WantDiags);
+  Server->addDocument(File, *Contents, WantDiags, Params.forceRebuild);
 }
 
 void ClangdLSPServer::onFileEvent(const DidChangeWatchedFilesParams &Params) {
   Server->onFileEvent(Params);
 }
 
 void ClangdLSPServer::onCommand(const ExecuteCommandParams &Params,
                                 Callback<llvm::json::Value> Reply) {

clang-tools-extra/clangd/ClangdServer.h

ClangdServer(const GlobalCompilationDatabase &CDB,
                const FileSystemProvider &FSProvider, const Options &Opts,
                Callbacks *Callbacks = nullptr);
 
   /// Add a \p File to the list of tracked C++ files or update the contents if
   /// \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.
   void addDocument(PathRef File, StringRef Contents,
-                   WantDiagnostics WD = WantDiagnostics::Auto);
+                   WantDiagnostics WD = WantDiagnostics::Auto,
+                   bool ForceRebuild = false);
 
   /// Get the contents of \p File, which should have been added.
   llvm::StringRef getDocument(PathRef File) const;
 
   /// Remove \p File from list of tracked files, schedule a request to free
   /// resources associated with it. Pending diagnostics for closed files may not
   /// be delivered, even if requested with WantDiags::Auto or WantDiags::Yes.
   void removeDocument(PathRef File);

clang-tools-extra/clangd/ClangdServer.cpp

BackgroundIdx = std::make_unique<BackgroundIndex>(
         });
     AddIndex(BackgroundIdx.get());
   }
   if (DynamicIdx)
     AddIndex(DynamicIdx.get());
 }
 
 void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
-                               WantDiagnostics WantDiags) {
+                               WantDiagnostics WantDiags, bool ForceRebuild) {
   auto FS = FSProvider.getFileSystem();
 
   ParseOptions Opts;
   Opts.ClangTidyOpts = tidy::ClangTidyOptions::getDefaults();
   // FIXME: call tidy options builder on the worker thread, it can do IO.
   if (GetClangTidyOptions)
     Opts.ClangTidyOpts = GetClangTidyOptions(*FS, File);
   Opts.SuggestMissingIncludes = SuggestMissingIncludes;
 
   // Compile command is set asynchronously during update, as it can be slow.
   ParseInputs Inputs;
   Inputs.FS = FS;
   Inputs.Contents = std::string(Contents);
+  Inputs.ForceRebuild = ForceRebuild;
   Inputs.Opts = std::move(Opts);
   Inputs.Index = Index;
   bool NewFile = WorkScheduler.update(File, Inputs, WantDiags);
   // If we loaded Foo.h, we want to make sure Foo.cpp is indexed.
   if (NewFile && BackgroundIdx)
     BackgroundIdx->boostRelated(File);
 }
 

clang-tools-extra/clangd/Compiler.h

public:
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const clang::Diagnostic &Info) override;
 };
 
 // Options to run clang e.g. when parsing AST.
 struct ParseOptions {
   tidy::ClangTidyOptions ClangTidyOpts;
   bool SuggestMissingIncludes = false;
 };
 
 /// Information required to run clang, e.g. to parse AST or do code completion.
 struct ParseInputs {
   tooling::CompileCommand CompileCommand;
   IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS;
   std::string Contents;
+  // Prevent reuse of the cached preamble/AST. Slow! Useful to workaround
+  // clangd's assumption that missing header files will stay missing.
+  bool ForceRebuild = false;
   // Used to recover from diagnostics (e.g. find missing includes for symbol).
   const SymbolIndex *Index = nullptr;
   ParseOptions Opts;
 };
 
 /// Builds compiler invocation that could be used to build AST or preamble.
 std::unique_ptr<CompilerInvocation>
 buildCompilerInvocation(const ParseInputs &Inputs, clang::DiagnosticConsumer &D,

clang-tools-extra/clangd/Protocol.h

struct DidChangeTextDocumentParams {
   /// The actual content changes.
   std::vector<TextDocumentContentChangeEvent> contentChanges;
 
   /// Forces diagnostics to be generated, or to not be generated, for this
   /// version of the file. If not set, diagnostics are eventually consistent:
   /// either they will be provided for this version or some subsequent one.
   /// This is a clangd extension.
   llvm::Optional<bool> wantDiagnostics;
+
+  /// Force a complete rebuild of the file, ignoring all cached state. Slow!
+  /// This is useful to defeat clangd's assumption that missing headers will
+  /// stay missing.
+  /// This is a clangd extension.
+  bool forceRebuild = false;
 };
 bool fromJSON(const llvm::json::Value &, DidChangeTextDocumentParams &);
 
 enum class FileChangeType {
   /// The file got created.
   Created = 1,
   /// The file got changed.
   Changed = 2,
   /// The file got deleted.

clang-tools-extra/clangd/Protocol.cpp

 
 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, DidChangeTextDocumentParams &R) {
   llvm::json::ObjectMapper O(Params);
-  return O && O.map("textDocument", R.textDocument) &&
+  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);
 }
 
 bool fromJSON(const llvm::json::Value &E, FileChangeType &Out) {
   if (auto T = E.getAsInteger()) {
     if (*T < static_cast<int>(FileChangeType::Created) ||
         *T > static_cast<int>(FileChangeType::Deleted))

clang-tools-extra/clangd/TUScheduler.cpp

if (!Invocation) {
       Callbacks.onFailedAST(FileName, std::move(CompilerInvocationDiags),
                             RunPublish);
       // Make sure anyone waiting for the preamble gets notified it could not
       // be built.
       PreambleWasBuilt.notify();
       return;
     }
 
     std::shared_ptr<const PreambleData> OldPreamble =
-        getPossiblyStalePreamble();
+        Inputs.ForceRebuild ? std::shared_ptr<const PreambleData>()
+                            : getPossiblyStalePreamble();
     std::shared_ptr<const PreambleData> NewPreamble = buildPreamble(
         FileName, *Invocation, OldPreamble, OldCommand, Inputs,
         StorePreambleInMemory,
         [this](ASTContext &Ctx, std::shared_ptr<clang::Preprocessor> PP,
                const CanonicalIncludes &CanonIncludes) {
           Callbacks.onPreambleAST(FileName, Ctx, std::move(PP), CanonIncludes);
         });
 

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

TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   ASSERT_FALSE(DoUpdate(OtherSourceContents));
 
   // Update to the compile commands should also cause a rebuild.
   CDB.ExtraClangFlags.push_back("-DSOMETHING");
   ASSERT_TRUE(DoUpdate(OtherSourceContents));
   ASSERT_FALSE(DoUpdate(OtherSourceContents));
 }
 
+TEST_F(TUSchedulerTests, ForceRebuild) {
+  TUScheduler S(CDB, optsForTest(), captureDiags());
+
+  auto Source = testPath("foo.cpp");
+  auto Header = testPath("foo.h");
+
+  auto SourceContents = R"cpp(
+      #include "foo.h"
+      int b = a;
+    )cpp";
+
+  ParseInputs Inputs = getInputs(Source, SourceContents);
+
+  // Update the source contents, which should trigger an initial build with
+  // the header file missing.
+  updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
+                  [](std::vector<Diag> Diags) {
+    EXPECT_THAT(
+        Diags,
+        ElementsAre(
+            Field(&Diag::Message, "'foo.h' file not found"),
+            Field(&Diag::Message, "use of undeclared identifier 'a'")));
+  });
+
+  // Add the header file. We need to recreate the inputs since we changed a
+  // file from underneath the test FS.
+  Files[Header] = "int a;";
+  Timestamps[Header] = time_t(1);
+  Inputs = getInputs(Source, SourceContents);
+
+  // The addition of the missing header file shouldn't trigger a rebuild since
+  // we don't track missing files.
+  updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
+                  [](std::vector<Diag> Diags) {
+    ADD_FAILURE() << "Did not expect diagnostics for missing header update";
+  });
+
+  // Forcing the reload should should cause a rebuild which no longer has any
+  // errors.
+  Inputs.ForceRebuild = true;
+  updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
+                  [](std::vector<Diag> Diags) {
+    EXPECT_THAT(Diags, IsEmpty());
+  });
+
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+}
 TEST_F(TUSchedulerTests, NoChangeDiags) {
   TUScheduler S(CDB, optsForTest(), captureDiags());
 
   auto FooCpp = testPath("foo.cpp");
   auto Contents = "int a; int b;";
 
   updateWithDiags(
       S, FooCpp, Contents, WantDiagnostics::No,
TEST_F(TUSchedulerTests, CommandLineErrors) {
   CDB.ExtraClangFlags = {"-fsome-unknown-flag"};
 
   // (!) 'Ready' must live longer than TUScheduler.
   Notification Ready;
 
   TUScheduler S(CDB, optsForTest(), captureDiags());
   std::vector<Diag> Diagnostics;
   updateWithDiags(S, testPath("foo.cpp"), "void test() {}",
-                  WantDiagnostics::Yes, [&](std::vector<Diag> D) {
+                  WantDiagnostics::Yes,
+                  [&](std::vector<Diag> D) {
                     Diagnostics = std::move(D);
                     Ready.notify();
                   });
   Ready.wait();
 
   EXPECT_THAT(
       Diagnostics,
       ElementsAre(AllOf(
           Field(&Diag::ID, Eq(diag::err_drv_unknown_argument)),
           Field(&Diag::Name, Eq("drv_unknown_argument")),
           Field(&Diag::Message, "unknown argument: '-fsome-unknown-flag'"))));
 }
 
 TEST_F(TUSchedulerTests, CommandLineWarnings) {
   // We should not see warnings from command-line parsing.
   CDB.ExtraClangFlags = {"-Wsome-unknown-warning"};
 
   // (!) 'Ready' must live longer than TUScheduler.
   Notification Ready;
 
   TUScheduler S(CDB, optsForTest(), captureDiags());
   std::vector<Diag> Diagnostics;
   updateWithDiags(S, testPath("foo.cpp"), "void test() {}",
-                  WantDiagnostics::Yes, [&](std::vector<Diag> D) {
+                  WantDiagnostics::Yes,
+                  [&](std::vector<Diag> D) {
                     Diagnostics = std::move(D);
                     Ready.notify();
                   });
   Ready.wait();
 
   EXPECT_THAT(Diagnostics, IsEmpty());
 }