Index: clangd/ClangdServer.h =================================================================== --- clangd/ClangdServer.h +++ clangd/ClangdServer.h @@ -117,22 +117,20 @@ /// \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. + /// When \p SkipCache is true, compile commands will always be requested from + /// compilation database even if they were cached in previous invocations. void addDocument(PathRef File, StringRef Contents, - WantDiagnostics WD = WantDiagnostics::Auto); + WantDiagnostics WD = WantDiagnostics::Auto, + bool SkipCache = false); /// Remove \p File from list of tracked files, schedule a request to free /// resources associated with it. 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. - void forceReparse(PathRef File); - /// Calls forceReparse() on all currently opened files. /// As a result, this method may be very expensive. /// This method is normally called when the compilation database is changed. + /// FIXME: this method must be moved to ClangdLSPServer along with DraftMgr. void reparseOpenedFiles(); /// Run code completion for \p File at \p Pos. @@ -240,9 +238,8 @@ formatCode(llvm::StringRef Code, PathRef File, ArrayRef Ranges); - void scheduleReparseAndDiags(PathRef File, VersionedDraft Contents, - WantDiagnostics WD, - IntrusiveRefCntPtr FS); + void consumeDiagnostics(PathRef File, DocVersion Version, + std::vector Diags); CompileArgsCache CompileArgs; DiagnosticsConsumer &DiagConsumer; Index: clangd/ClangdServer.cpp =================================================================== --- clangd/ClangdServer.cpp +++ clangd/ClangdServer.cpp @@ -116,10 +116,19 @@ } void ClangdServer::addDocument(PathRef File, StringRef Contents, - WantDiagnostics WantDiags) { + WantDiagnostics WantDiags, bool SkipCache) { + if (SkipCache) + CompileArgs.invalidate(File); + DocVersion Version = DraftMgr.updateDraft(File, Contents); - scheduleReparseAndDiags(File, VersionedDraft{Version, Contents.str()}, - WantDiags, FSProvider.getFileSystem()); + ParseInputs Inputs = {CompileArgs.getCompileCommand(File), + FSProvider.getFileSystem(), Contents.str()}; + + Path FileStr = File.str(); + WorkScheduler.update(File, std::move(Inputs), WantDiags, + [this, FileStr, Version](std::vector Diags) { + consumeDiagnostics(FileStr, Version, std::move(Diags)); + }); } void ClangdServer::removeDocument(PathRef File) { @@ -128,19 +137,6 @@ WorkScheduler.remove(File); } -void ClangdServer::forceReparse(PathRef File) { - auto FileContents = DraftMgr.getDraft(File); - assert(FileContents.Draft && - "forceReparse() was called for non-added document"); - - // forceReparse promises to request new compilation flags from CDB, so we - // remove any cahced flags. - CompileArgs.invalidate(File); - - scheduleReparseAndDiags(File, std::move(FileContents), WantDiagnostics::Yes, - FSProvider.getFileSystem()); -} - void ClangdServer::codeComplete(PathRef File, Position Pos, const clangd::CodeCompleteOptions &Opts, Callback CB) { @@ -335,8 +331,8 @@ // Replacement with offset UINT_MAX and length 0 will be treated as include // insertion. tooling::Replacement R(File, /*Offset=*/UINT_MAX, 0, "#include " + ToInclude); - auto Replaces = format::cleanupAroundReplacements( - Code, tooling::Replacements(R), *Style); + auto Replaces = + format::cleanupAroundReplacements(Code, tooling::Replacements(R), *Style); if (!Replaces) return Replaces; return formatReplacements(Code, *Replaces, *Style); @@ -499,39 +495,28 @@ WorkScheduler.runWithAST("Hover", File, Bind(Action, std::move(CB))); } -void ClangdServer::scheduleReparseAndDiags( - PathRef File, VersionedDraft Contents, WantDiagnostics WantDiags, - IntrusiveRefCntPtr FS) { - tooling::CompileCommand Command = CompileArgs.getCompileCommand(File); - - DocVersion Version = Contents.Version; - Path FileStr = File.str(); - - auto Callback = [this, Version, FileStr](std::vector Diags) { - // We need to serialize access to resulting diagnostics to avoid calling - // `onDiagnosticsReady` in the wrong order. - std::lock_guard DiagsLock(DiagnosticsMutex); - DocVersion &LastReportedDiagsVersion = ReportedDiagnosticVersions[FileStr]; - // FIXME(ibiryukov): get rid of '<' comparison here. In the current - // implementation diagnostics will not be reported after version counters' - // overflow. This should not happen in practice, since DocVersion is a - // 64-bit unsigned integer. - if (Version < LastReportedDiagsVersion) - return; - LastReportedDiagsVersion = Version; - - DiagConsumer.onDiagnosticsReady(FileStr, std::move(Diags)); - }; - - WorkScheduler.update(File, - ParseInputs{std::move(Command), std::move(FS), - std::move(*Contents.Draft)}, - WantDiags, std::move(Callback)); +void ClangdServer::consumeDiagnostics(PathRef File, DocVersion Version, + std::vector Diags) { + // We need to serialize access to resulting diagnostics to avoid calling + // `onDiagnosticsReady` in the wrong order. + std::lock_guard DiagsLock(DiagnosticsMutex); + DocVersion &LastReportedDiagsVersion = ReportedDiagnosticVersions[File]; + + // FIXME(ibiryukov): get rid of '<' comparison here. In the current + // implementation diagnostics will not be reported after version counters' + // overflow. This should not happen in practice, since DocVersion is a + // 64-bit unsigned integer. + if (Version < LastReportedDiagsVersion) + return; + LastReportedDiagsVersion = Version; + + DiagConsumer.onDiagnosticsReady(File, std::move(Diags)); } void ClangdServer::reparseOpenedFiles() { for (const Path &FilePath : DraftMgr.getActiveFiles()) - forceReparse(FilePath); + addDocument(FilePath, *DraftMgr.getDraft(FilePath).Draft, + WantDiagnostics::Auto, /*SkipCache=*/true); } void ClangdServer::onFileEvent(const DidChangeWatchedFilesParams &Params) { Index: clangd/TUScheduler.cpp =================================================================== --- clangd/TUScheduler.cpp +++ clangd/TUScheduler.cpp @@ -110,6 +110,9 @@ Deadline scheduleLocked(); /// Should the first task in the queue be skipped instead of run? bool shouldSkipHeadLocked() const; + /// Rebuilds AST and pushes the new diagnostics, if required. + void rebuildASTLocked(WantDiagnostics WantDiags, + UniqueFunction)> OnUpdated); struct Request { UniqueFunction Action; @@ -213,20 +216,8 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags, UniqueFunction)> OnUpdated) { auto Task = [=](decltype(OnUpdated) OnUpdated) mutable { - FileInputs = Inputs; - auto Diags = AST.rebuild(std::move(Inputs)); - - { - std::lock_guard Lock(Mutex); - if (AST.getPreamble()) - LastBuiltPreamble = AST.getPreamble(); - LastASTSize = AST.getUsedBytes(); - } - // We want to report the diagnostics even if this update was cancelled. - // It seems more useful than making the clients wait indefinitely if they - // spam us with updates. - if (Diags && WantDiags != WantDiagnostics::No) - OnUpdated(std::move(*Diags)); + FileInputs = std::move(Inputs); + rebuildASTLocked(WantDiags, std::move(OnUpdated)); }; startTask("Update", Bind(Task, std::move(OnUpdated)), WantDiags); @@ -388,6 +379,23 @@ llvm_unreachable("Unknown WantDiagnostics"); } +void ASTWorker::rebuildASTLocked( + WantDiagnostics WantDiags, + UniqueFunction)> OnUpdated) { + auto Diags = AST.rebuild(ParseInputs(FileInputs)); + { + std::lock_guard Lock(Mutex); + if (AST.getPreamble()) + LastBuiltPreamble = AST.getPreamble(); + LastASTSize = AST.getUsedBytes(); + } + // We want to report the diagnostics even if this update was cancelled. + // It seems more useful than making the clients wait indefinitely if they + // spam us with updates. + if (Diags && WantDiags != WantDiagnostics::No) + OnUpdated(std::move(*Diags)); +} + bool ASTWorker::blockUntilIdle(Deadline Timeout) const { std::unique_lock Lock(Mutex); return wait(Lock, RequestsCV, Timeout, [&] { return Requests.empty(); }); Index: unittests/clangd/ClangdTests.cpp =================================================================== --- unittests/clangd/ClangdTests.cpp +++ unittests/clangd/ClangdTests.cpp @@ -245,13 +245,13 @@ EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); FS.Files[FooH] = ""; - Server.forceReparse(FooCpp); + Server.addDocument(FooCpp, SourceContents); auto DumpParseDifferent = dumpASTWithoutMemoryLocs(Server, FooCpp); ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics"; EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags()); FS.Files[FooH] = "int a;"; - Server.forceReparse(FooCpp); + Server.addDocument(FooCpp, SourceContents); auto DumpParse2 = dumpASTWithoutMemoryLocs(Server, FooCpp); ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics"; EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); @@ -368,15 +368,15 @@ // Now switch to C++ mode. CDB.ExtraClangFlags = {"-xc++"}; - // Currently, addDocument never checks if CompileCommand has changed, so we + // By default addDocument does not check if CompileCommand has changed, so we // expect to see the errors. runAddDocument(Server, FooCpp, SourceContents1); EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags()); runAddDocument(Server, FooCpp, SourceContents2); EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags()); - // But forceReparse should reparse the file with proper flags. - Server.forceReparse(FooCpp); - ASSERT_TRUE(Server.blockUntilIdleForTest()); + // Passing SkipCache=true will force addDocument to reparse the file with + // proper flags. + runAddDocument(Server, FooCpp, SourceContents2, /*SkipCache=*/true); EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); // Subsequent addDocument calls should finish without errors too. runAddDocument(Server, FooCpp, SourceContents1); @@ -408,12 +408,13 @@ // Parse without the define, no errors should be produced. CDB.ExtraClangFlags = {}; - // Currently, addDocument never checks if CompileCommand has changed, so we + // By default addDocument does not check if CompileCommand has changed, so we // expect to see the errors. runAddDocument(Server, FooCpp, SourceContents); EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags()); - // But forceReparse should reparse the file with proper flags. - Server.forceReparse(FooCpp); + // Passing SkipCache=true will force addDocument to reparse the file with + // proper flags. + runAddDocument(Server, FooCpp, SourceContents, /*SkipCache=*/true); ASSERT_TRUE(Server.blockUntilIdleForTest()); EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); // Subsequent addDocument call should finish without errors too. @@ -675,44 +676,31 @@ Stats.FileIsRemoved = true; }; - auto UpdateStatsOnForceReparse = [&](unsigned FileIndex) { - auto &Stats = ReqStats[FileIndex]; - - if (Stats.LastContentsHadErrors) - ++Stats.RequestsWithErrors; - else - ++Stats.RequestsWithoutErrors; - }; - - auto AddDocument = [&](unsigned FileIndex) { + auto AddDocument = [&](unsigned FileIndex, bool SkipCache) { bool ShouldHaveErrors = ShouldHaveErrorsDist(RandGen); Server.addDocument(FilePaths[FileIndex], ShouldHaveErrors ? SourceContentsWithErrors - : SourceContentsWithoutErrors); + : SourceContentsWithoutErrors, + WantDiagnostics::Auto, SkipCache); UpdateStatsOnAddDocument(FileIndex, ShouldHaveErrors); }; // Various requests that we would randomly run. auto AddDocumentRequest = [&]() { unsigned FileIndex = FileIndexDist(RandGen); - AddDocument(FileIndex); + AddDocument(FileIndex, /*SkipCache=*/false); }; auto ForceReparseRequest = [&]() { unsigned FileIndex = FileIndexDist(RandGen); - // Make sure we don't violate the ClangdServer's contract. - if (ReqStats[FileIndex].FileIsRemoved) - AddDocument(FileIndex); - - Server.forceReparse(FilePaths[FileIndex]); - UpdateStatsOnForceReparse(FileIndex); + AddDocument(FileIndex, /*SkipCache=*/true); }; auto RemoveDocumentRequest = [&]() { unsigned FileIndex = FileIndexDist(RandGen); // Make sure we don't violate the ClangdServer's contract. if (ReqStats[FileIndex].FileIsRemoved) - AddDocument(FileIndex); + AddDocument(FileIndex, /*SkipCache=*/false); Server.removeDocument(FilePaths[FileIndex]); UpdateStatsOnRemoveDocument(FileIndex); @@ -722,7 +710,7 @@ unsigned FileIndex = FileIndexDist(RandGen); // Make sure we don't violate the ClangdServer's contract. if (ReqStats[FileIndex].FileIsRemoved) - AddDocument(FileIndex); + AddDocument(FileIndex, /*SkipCache=*/false); Position Pos; Pos.line = LineDist(RandGen); @@ -741,7 +729,7 @@ unsigned FileIndex = FileIndexDist(RandGen); // Make sure we don't violate the ClangdServer's contract. if (ReqStats[FileIndex].FileIsRemoved) - AddDocument(FileIndex); + AddDocument(FileIndex, /*SkipCache=*/false); Position Pos; Pos.line = LineDist(RandGen); Index: unittests/clangd/SyncAPI.h =================================================================== --- unittests/clangd/SyncAPI.h +++ unittests/clangd/SyncAPI.h @@ -19,7 +19,8 @@ namespace clangd { // Calls addDocument and then blockUntilIdleForTest. -void runAddDocument(ClangdServer &Server, PathRef File, StringRef Contents); +void runAddDocument(ClangdServer &Server, PathRef File, StringRef Contents, + bool SkipCache = false); llvm::Expected runCodeComplete(ClangdServer &Server, PathRef File, Position Pos, Index: unittests/clangd/SyncAPI.cpp =================================================================== --- unittests/clangd/SyncAPI.cpp +++ unittests/clangd/SyncAPI.cpp @@ -11,8 +11,9 @@ namespace clang { namespace clangd { -void runAddDocument(ClangdServer &Server, PathRef File, StringRef Contents) { - Server.addDocument(File, Contents); +void runAddDocument(ClangdServer &Server, PathRef File, StringRef Contents, + bool SkipCache) { + Server.addDocument(File, Contents, WantDiagnostics::Auto, SkipCache); if (!Server.blockUntilIdleForTest()) llvm_unreachable("not idle after addDocument"); }