diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -633,6 +633,9 @@ if (Params.wantDiagnostics.hasValue()) WantDiags = Params.wantDiagnostics.getValue() ? WantDiagnostics::Yes : WantDiagnostics::No; + auto ForceReload = false; + if (Params.forceReload.hasValue()) + ForceReload = Params.forceReload.getValue() ? true : false; PathRef File = Params.textDocument.uri.file(); llvm::Expected Contents = @@ -647,7 +650,7 @@ return; } - Server->addDocument(File, *Contents, WantDiags); + Server->addDocument(File, *Contents, WantDiags, ForceReload); } void ClangdLSPServer::onFileEvent(const DidChangeWatchedFilesParams &Params) { diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -172,7 +172,8 @@ /// 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 ForceReload = false); /// Get the contents of \p File, which should have been added. llvm::StringRef getDocument(PathRef File) const; diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -170,7 +170,7 @@ } void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents, - WantDiagnostics WantDiags) { + WantDiagnostics WantDiags, bool ForceReload) { auto FS = FSProvider.getFileSystem(); ParseOptions Opts; @@ -186,7 +186,7 @@ Inputs.Contents = std::string(Contents); Inputs.Opts = std::move(Opts); Inputs.Index = Index; - bool NewFile = WorkScheduler.update(File, Inputs, WantDiags); + bool NewFile = WorkScheduler.update(File, Inputs, WantDiags, ForceReload); // If we loaded Foo.h, we want to make sure Foo.cpp is indexed. if (NewFile && BackgroundIdx) BackgroundIdx->boostRelated(File); diff --git a/clang-tools-extra/clangd/Preamble.h b/clang-tools-extra/clangd/Preamble.h --- a/clang-tools-extra/clangd/Preamble.h +++ b/clang-tools-extra/clangd/Preamble.h @@ -78,7 +78,7 @@ buildPreamble(PathRef FileName, CompilerInvocation &CI, std::shared_ptr OldPreamble, const tooling::CompileCommand &OldCompileCommand, - const ParseInputs &Inputs, bool StoreInMemory, + const ParseInputs &Inputs, bool StoreInMemory, bool ForceReload, PreambleParsedCallback PreambleCallback); diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/clang-tools-extra/clangd/Preamble.cpp @@ -89,7 +89,7 @@ buildPreamble(PathRef FileName, CompilerInvocation &CI, std::shared_ptr OldPreamble, const tooling::CompileCommand &OldCompileCommand, - const ParseInputs &Inputs, bool StoreInMemory, + const ParseInputs &Inputs, bool StoreInMemory, bool ForceReload, PreambleParsedCallback PreambleCallback) { // Note that we don't need to copy the input contents, preamble can live // without those. @@ -98,14 +98,15 @@ auto Bounds = ComputePreambleBounds(*CI.getLangOpts(), ContentsBuffer.get(), 0); - if (OldPreamble && + if (!ForceReload && OldPreamble && compileCommandsAreEqual(Inputs.CompileCommand, OldCompileCommand) && OldPreamble->Preamble.CanReuse(CI, ContentsBuffer.get(), Bounds, Inputs.FS.get())) { vlog("Reusing preamble for {0}", FileName); return OldPreamble; } - vlog(OldPreamble ? "Rebuilding invalidated preamble for {0}" + vlog(OldPreamble ? (ForceReload ? "Rebuilding force-reloaded preamble for {0}" + : "Rebuilding invalidated preamble for {0}") : "Building first preamble for {0}", FileName); diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h --- a/clang-tools-extra/clangd/Protocol.h +++ b/clang-tools-extra/clangd/Protocol.h @@ -652,6 +652,13 @@ /// either they will be provided for this version or some subsequent one. /// This is a clangd extension. llvm::Optional wantDiagnostics; + + /// Force the preamble to be rebuilt for this version of the file (only when + /// present and set to true). This is a workaround for how our heuristics for + /// preamble rebuilds will not detect added header files which were previously + /// missing. + /// This is a clangd extension. + llvm::Optional forceReload; }; bool fromJSON(const llvm::json::Value &, DidChangeTextDocumentParams &); diff --git a/clang-tools-extra/clangd/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp --- a/clang-tools-extra/clangd/Protocol.cpp +++ b/clang-tools-extra/clangd/Protocol.cpp @@ -432,7 +432,8 @@ llvm::json::ObjectMapper O(Params); return O && O.map("textDocument", R.textDocument) && O.map("contentChanges", R.contentChanges) && - O.map("wantDiagnostics", R.wantDiagnostics); + O.map("wantDiagnostics", R.wantDiagnostics) && + O.map("forceReload", R.forceReload); } bool fromJSON(const llvm::json::Value &E, FileChangeType &Out) { diff --git a/clang-tools-extra/clangd/TUScheduler.h b/clang-tools-extra/clangd/TUScheduler.h --- a/clang-tools-extra/clangd/TUScheduler.h +++ b/clang-tools-extra/clangd/TUScheduler.h @@ -183,8 +183,10 @@ /// If diagnostics are requested (Yes), and the context is cancelled /// before they are prepared, they may be skipped if eventual-consistency /// permits it (i.e. WantDiagnostics is downgraded to Auto). + /// If ForceReload is true, then the preamble will be rebuilt. /// Returns true if the file was not previously tracked. - bool update(PathRef File, ParseInputs Inputs, WantDiagnostics WD); + bool update(PathRef File, ParseInputs Inputs, WantDiagnostics WD, + bool ForceReload); /// Remove \p File from the list of tracked files and schedule removal of its /// resources. Pending diagnostics for closed files may not be delivered, even diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp --- a/clang-tools-extra/clangd/TUScheduler.cpp +++ b/clang-tools-extra/clangd/TUScheduler.cpp @@ -180,7 +180,7 @@ bool StorePreamblesInMemory, ParsingCallbacks &Callbacks); ~ASTWorker(); - void update(ParseInputs Inputs, WantDiagnostics); + void update(ParseInputs Inputs, WantDiagnostics, bool ForceReload); void runWithAST(llvm::StringRef Name, llvm::unique_function)> Action); @@ -366,7 +366,8 @@ #endif } -void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) { +void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags, + bool ForceReload) { llvm::StringRef TaskName = "Update"; auto Task = [=]() mutable { auto RunPublish = [&](llvm::function_ref Publish) { @@ -435,7 +436,7 @@ getPossiblyStalePreamble(); std::shared_ptr NewPreamble = buildPreamble( FileName, *Invocation, OldPreamble, OldCommand, Inputs, - StorePreambleInMemory, + StorePreambleInMemory, ForceReload, [this](ASTContext &Ctx, std::shared_ptr PP, const CanonicalIncludes &CanonIncludes) { Callbacks.onPreambleAST(FileName, Ctx, std::move(PP), CanonIncludes); @@ -884,7 +885,7 @@ } bool TUScheduler::update(PathRef File, ParseInputs Inputs, - WantDiagnostics WantDiags) { + WantDiagnostics WantDiags, bool ForceReload) { std::unique_ptr &FD = Files[File]; bool NewFile = FD == nullptr; if (!FD) { @@ -898,7 +899,7 @@ } else { FD->Contents = Inputs.Contents; } - FD->Worker->update(std::move(Inputs), WantDiags); + FD->Worker->update(std::move(Inputs), WantDiags, ForceReload); return NewFile; } diff --git a/clang-tools-extra/clangd/unittests/FileIndexTests.cpp b/clang-tools-extra/clangd/unittests/FileIndexTests.cpp --- a/clang-tools-extra/clangd/unittests/FileIndexTests.cpp +++ b/clang-tools-extra/clangd/unittests/FileIndexTests.cpp @@ -288,7 +288,7 @@ bool IndexUpdated = false; buildPreamble( FooCpp, *CI, /*OldPreamble=*/nullptr, tooling::CompileCommand(), PI, - /*StoreInMemory=*/true, + /*StoreInMemory=*/true, /*ForceReload*/false, [&](ASTContext &Ctx, std::shared_ptr PP, const CanonicalIncludes &CanonIncludes) { EXPECT_FALSE(IndexUpdated) << "Expected only a single index update"; diff --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp --- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp +++ b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp @@ -61,7 +61,8 @@ llvm::StringRef Contents, WantDiagnostics WD, llvm::unique_function CB) { WithContextValue Ctx(llvm::make_scope_exit(std::move(CB))); - S.update(File, getInputs(File, std::string(Contents)), WD); + S.update(File, getInputs(File, std::string(Contents)), WD, + /*ForceReload*/false); } static Key)>> @@ -101,7 +102,7 @@ /// any. The TUScheduler should be created with captureDiags as a /// DiagsCallback for this to work. void updateWithDiags(TUScheduler &S, PathRef File, ParseInputs Inputs, - WantDiagnostics WD, + WantDiagnostics WD, bool ForceReload, llvm::unique_function)> CB) { Path OrigFile = File.str(); WithContextValue Ctx(DiagsCallbackKey, @@ -110,14 +111,14 @@ assert(File == OrigFile); CB(std::move(Diags)); }); - S.update(File, std::move(Inputs), WD); + S.update(File, std::move(Inputs), WD, ForceReload); } void updateWithDiags(TUScheduler &S, PathRef File, llvm::StringRef Contents, - WantDiagnostics WD, + WantDiagnostics WD, bool ForceReload, llvm::unique_function)> CB) { return updateWithDiags(S, File, getInputs(File, std::string(Contents)), WD, - std::move(CB)); + ForceReload, std::move(CB)); } llvm::StringMap Files; @@ -138,7 +139,8 @@ Files[Missing] = ""; EXPECT_EQ(S.getContents(Added), ""); - S.update(Added, getInputs(Added, "x"), WantDiagnostics::No); + S.update(Added, getInputs(Added, "x"), WantDiagnostics::No, + /*ForceReload*/false); EXPECT_EQ(S.getContents(Added), "x"); // Assert each operation for missing file is an error (even if it's @@ -182,20 +184,24 @@ Notification Ready; TUScheduler S(CDB, optsForTest(), captureDiags()); auto Path = testPath("foo.cpp"); - updateWithDiags(S, Path, "", WantDiagnostics::Yes, + updateWithDiags(S, Path, "", WantDiagnostics::Yes, /*ForceReload*/false, [&](std::vector) { Ready.wait(); }); updateWithDiags(S, Path, "request diags", WantDiagnostics::Yes, + /*ForceReload*/false, [&](std::vector) { ++CallbackCount; }); updateWithDiags(S, Path, "auto (clobbered)", WantDiagnostics::Auto, + /*ForceReload*/false, [&](std::vector) { ADD_FAILURE() << "auto should have been cancelled by auto"; }); updateWithDiags(S, Path, "request no diags", WantDiagnostics::No, + /*ForceReload*/false, [&](std::vector) { ADD_FAILURE() << "no diags should not be called back"; }); updateWithDiags(S, Path, "auto (produces)", WantDiagnostics::Auto, + /*ForceReload*/false, [&](std::vector) { ++CallbackCount; }); Ready.notify(); @@ -213,15 +219,18 @@ // FIXME: we could probably use timeouts lower than 1 second here. auto Path = testPath("foo.cpp"); updateWithDiags(S, Path, "auto (debounced)", WantDiagnostics::Auto, + /*ForceReload*/false, [&](std::vector) { ADD_FAILURE() << "auto should have been debounced and canceled"; }); std::this_thread::sleep_for(std::chrono::milliseconds(200)); updateWithDiags(S, Path, "auto (timed out)", WantDiagnostics::Auto, + /*ForceReload*/false, [&](std::vector) { ++CallbackCount; }); std::this_thread::sleep_for(std::chrono::seconds(2)); updateWithDiags(S, Path, "auto (shut down)", WantDiagnostics::Auto, + /*ForceReload*/false, [&](std::vector) { ++CallbackCount; }); ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); @@ -259,7 +268,8 @@ // If the second read was stale, it would usually see A. std::this_thread::sleep_for(std::chrono::milliseconds(100)); }); - S.update(Path, getInputs(Path, "#include "), WantDiagnostics::Yes); + S.update(Path, getInputs(Path, "#include "), WantDiagnostics::Yes, + /*ForceReload*/false); S.runWithPreamble("StaleRead", Path, TUScheduler::Stale, [&](Expected Pre) { @@ -301,7 +311,7 @@ auto T = cancelableTask(); WithContext C(std::move(T.first)); updateWithDiags( - S, Path, "//" + ID, WantDiagnostics::Yes, + S, Path, "//" + ID, WantDiagnostics::Yes, /*ForceReload*/false, [&, ID](std::vector Diags) { DiagsSeen.push_back(ID); }); return std::move(T.second); }; @@ -392,7 +402,7 @@ { WithContextValue WithNonce(NonceKey, ++Nonce); updateWithDiags( - S, File, Inputs, WantDiagnostics::Auto, + S, File, Inputs, WantDiagnostics::Auto, /*ForceReload*/false, [File, Nonce, &Mut, &TotalUpdates](std::vector) { EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce)); @@ -510,7 +520,8 @@ int main() {} )cpp"; auto WithEmptyPreamble = R"cpp(int main() {})cpp"; - S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto); + S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto, + /*ForceReload*/false); S.runWithPreamble( "getNonEmptyPreamble", Foo, TUScheduler::Stale, [&](Expected Preamble) { @@ -523,7 +534,8 @@ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); // Update the file which results in an empty preamble. - S.update(Foo, getInputs(Foo, WithEmptyPreamble), WantDiagnostics::Auto); + S.update(Foo, getInputs(Foo, WithEmptyPreamble), WantDiagnostics::Auto, + /*ForceReload*/false); // Wait for the preamble is being built. ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); S.runWithPreamble( @@ -550,7 +562,8 @@ constexpr int ReadsToSchedule = 10; std::mutex PreamblesMut; std::vector Preambles(ReadsToSchedule, nullptr); - S.update(Foo, getInputs(Foo, NonEmptyPreamble), WantDiagnostics::Auto); + S.update(Foo, getInputs(Foo, NonEmptyPreamble), WantDiagnostics::Auto, + /*ForceReload*/false); for (int I = 0; I < ReadsToSchedule; ++I) { S.runWithPreamble( "test", Foo, TUScheduler::Stale, @@ -585,6 +598,7 @@ std::atomic Updated(false); Updated = false; updateWithDiags(S, Source, Contents, WantDiagnostics::Yes, + /*ForceReload*/false, [&Updated](std::vector) { Updated = true; }); bool UpdateFinished = S.blockUntilIdle(timeoutSeconds(10)); if (!UpdateFinished) @@ -615,6 +629,44 @@ ASSERT_FALSE(DoUpdate(OtherSourceContents)); } +TEST_F(TUSchedulerTests, ForceReload) { + 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"; + + // Return value indicates if the updated callback was received. + auto DoUpdate = [&](std::string Contents, bool ForceReload) -> bool { + std::atomic Updated(false); + Updated = false; + updateWithDiags(S, Source, Contents, WantDiagnostics::Yes, ForceReload, + [&Updated](std::vector) { Updated = true; }); + bool UpdateFinished = S.blockUntilIdle(timeoutSeconds(10)); + if (!UpdateFinished) + ADD_FAILURE() << "Updated has not finished in one second. Threading bug?"; + return Updated; + }; + + // Update the source contents, which should trigger an initial build with + // the header file missing. + ASSERT_TRUE(DoUpdate(SourceContents, /*ForceReload*/false)); + + // Add the header file. + Files[Header] = "int a;"; + Timestamps[Header] = time_t(0); + + // The addition of the missing header file shouldn't trigger a rebuild since + // we don't track missing files. + ASSERT_FALSE(DoUpdate(SourceContents, /*ForceReload*/false)); + + // Forcing the reload should should cause a rebuild, though. + ASSERT_TRUE(DoUpdate(SourceContents, /*ForceReload*/true)); +} TEST_F(TUSchedulerTests, NoChangeDiags) { TUScheduler S(CDB, optsForTest(), captureDiags()); @@ -622,7 +674,7 @@ auto Contents = "int a; int b;"; updateWithDiags( - S, FooCpp, Contents, WantDiagnostics::No, + S, FooCpp, Contents, WantDiagnostics::No, /*ForceReload*/false, [](std::vector) { ADD_FAILURE() << "Should not be called."; }); S.runWithAST("touchAST", FooCpp, [](Expected IA) { // Make sure the AST was actually built. @@ -634,6 +686,7 @@ // report the diagnostics, as they were not reported previously. std::atomic SeenDiags(false); updateWithDiags(S, FooCpp, Contents, WantDiagnostics::Auto, + /*ForceReload*/false, [&](std::vector) { SeenDiags = true; }); ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); ASSERT_TRUE(SeenDiags); @@ -641,7 +694,7 @@ // Subsequent request does not get any diagnostics callback because the same // diags have previously been reported and the inputs didn't change. updateWithDiags( - S, FooCpp, Contents, WantDiagnostics::Auto, + S, FooCpp, Contents, WantDiagnostics::Auto, /*ForceReload*/false, [&](std::vector) { ADD_FAILURE() << "Should not be called."; }); ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); } @@ -721,7 +774,8 @@ TUScheduler S(CDB, optsForTest(), captureDiags()); std::vector Diagnostics; updateWithDiags(S, testPath("foo.cpp"), "void test() {}", - WantDiagnostics::Yes, [&](std::vector D) { + WantDiagnostics::Yes, /*ForceReload*/false, + [&](std::vector D) { Diagnostics = std::move(D); Ready.notify(); }); @@ -745,7 +799,8 @@ TUScheduler S(CDB, optsForTest(), captureDiags()); std::vector Diagnostics; updateWithDiags(S, testPath("foo.cpp"), "void test() {}", - WantDiagnostics::Yes, [&](std::vector D) { + WantDiagnostics::Yes, /*ForceReload*/false, + [&](std::vector D) { Diagnostics = std::move(D); Ready.notify(); }); diff --git a/clang-tools-extra/clangd/unittests/TestTU.cpp b/clang-tools-extra/clangd/unittests/TestTU.cpp --- a/clang-tools-extra/clangd/unittests/TestTU.cpp +++ b/clang-tools-extra/clangd/unittests/TestTU.cpp @@ -69,7 +69,8 @@ buildPreamble(FullFilename, *CI, /*OldPreamble=*/nullptr, /*OldCompileCommand=*/Inputs.CompileCommand, Inputs, - /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr); + /*StoreInMemory=*/true, /*ForceReload*/false, + /*PreambleCallback=*/nullptr); auto AST = buildAST(FullFilename, std::move(CI), Diags.take(), Inputs, Preamble); if (!AST.hasValue()) {