Index: clangd/ClangdServer.h =================================================================== --- clangd/ClangdServer.h +++ clangd/ClangdServer.h @@ -125,7 +125,8 @@ WantDiagnostics WD = WantDiagnostics::Auto); /// Remove \p File from list of tracked files, schedule a request to free - /// resources associated with it. + /// 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); /// Run code completion for \p File at \p Pos. @@ -222,20 +223,12 @@ formatCode(llvm::StringRef Code, PathRef File, ArrayRef Ranges); - typedef uint64_t DocVersion; - - void consumeDiagnostics(PathRef File, DocVersion Version, - std::vector Diags); - tooling::CompileCommand getCompileCommand(PathRef File); const GlobalCompilationDatabase &CDB; DiagnosticsConsumer &DiagConsumer; const FileSystemProvider &FSProvider; - /// Used to synchronize diagnostic responses for added and removed files. - llvm::StringMap InternalVersion; - Path ResourceDir; // The index used to look up symbols. This could be: // - null (all index functionality is optional) @@ -255,12 +248,6 @@ llvm::Optional WorkspaceRoot; std::shared_ptr PCHs; - /// Used to serialize diagnostic callbacks. - /// FIXME(ibiryukov): get rid of an extra map and put all version counters - /// into CppFile. - std::mutex DiagnosticsMutex; - /// Maps from a filename to the latest version of reported diagnostics. - llvm::StringMap ReportedDiagnosticVersions; // WorkScheduler has to be the last member, because its destructor has to be // called before all other members to stop the worker thread that references // ClangdServer. Index: clangd/ClangdServer.cpp =================================================================== --- clangd/ClangdServer.cpp +++ clangd/ClangdServer.cpp @@ -133,19 +133,17 @@ void ClangdServer::addDocument(PathRef File, StringRef Contents, WantDiagnostics WantDiags) { - DocVersion Version = ++InternalVersion[File]; ParseInputs Inputs = {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)); + [this, FileStr](std::vector Diags) { + DiagConsumer.onDiagnosticsReady(FileStr, + std::move(Diags)); }); } void ClangdServer::removeDocument(PathRef File) { - ++InternalVersion[File]; WorkScheduler.remove(File); } @@ -444,24 +442,6 @@ WorkScheduler.runWithAST("Hover", File, Bind(Action, std::move(CB))); } -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)); -} - tooling::CompileCommand ClangdServer::getCompileCommand(PathRef File) { trace::Span Span("GetCompileCommand"); Optional C = CDB.getCompileCommand(File); Index: clangd/TUScheduler.h =================================================================== --- clangd/TUScheduler.h +++ clangd/TUScheduler.h @@ -108,7 +108,8 @@ llvm::unique_function)> OnUpdated); /// Remove \p File from the list of tracked files and schedule removal of its - /// resources. + /// resources. Pending diagnostics for closed files may not be delivered, even + /// if requested with WantDiags::Auto or WantDiags::Yes. void remove(PathRef File); /// Schedule an async task with no dependencies. Index: clangd/TUScheduler.cpp =================================================================== --- clangd/TUScheduler.cpp +++ clangd/TUScheduler.cpp @@ -252,6 +252,13 @@ bool Done; /* GUARDED_BY(Mutex) */ std::deque Requests; /* GUARDED_BY(Mutex) */ mutable std::condition_variable RequestsCV; + /// Guards a critical section for running the diagnostics callbacks. + std::mutex DiagsMu; + // Used to prevent remove document + leading to out-of-order diagnostics: + // The lifetime of the old/new ASTWorkers will overlap, but their handles + // don't. When the old handle is destroyed, the old worker will stop reporting + // diagnostics. + bool ReportDiagnostics = true; /* GUARDED_BY(DiagMu) */ }; /// A smart-pointer-like class that points to an active ASTWorker. @@ -406,6 +413,14 @@ if (WantDiags == WantDiagnostics::No) return; + { + std::lock_guard Lock(DiagsMu); + // No need to rebuild the AST if we won't send the diagnotics. However, + // note that we don't prevent preamble rebuilds. + if (!ReportDiagnostics) + return; + } + // Get the AST for diagnostics. Optional> AST = IdleASTs.take(this); if (!AST) { @@ -418,7 +433,11 @@ // spam us with updates. // Note *AST can still be null if buildAST fails. if (*AST) { - OnUpdated((*AST)->getDiagnostics()); + { + std::lock_guard Lock(DiagsMu); + if (ReportDiagnostics) + OnUpdated((*AST)->getDiagnostics()); + } trace::Span Span("Running main AST callback"); Callbacks.onMainAST(FileName, **AST); DiagsWereReported = true; @@ -513,6 +532,10 @@ void ASTWorker::stop() { { + std::lock_guard Lock(DiagsMu); + ReportDiagnostics = false; + } + { std::lock_guard Lock(Mutex); assert(!Done && "stop() called twice"); Done = true; Index: unittests/clangd/ClangdTests.cpp =================================================================== --- unittests/clangd/ClangdTests.cpp +++ unittests/clangd/ClangdTests.cpp @@ -748,7 +748,8 @@ BlockingRequests[RequestIndex](); } } - } // Wait for ClangdServer to shutdown before proceeding. + ASSERT_TRUE(Server.blockUntilIdleForTest()); + } // Check some invariants about the state of the program. std::vector Stats = DiagConsumer.takeFileStats(); Index: unittests/clangd/TUSchedulerTests.cpp =================================================================== --- unittests/clangd/TUSchedulerTests.cpp +++ unittests/clangd/TUSchedulerTests.cpp @@ -124,6 +124,8 @@ S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto, [&](std::vector Diags) { ++CallbackCount; }); Ready.notify(); + + ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); } EXPECT_EQ(2, CallbackCount); } @@ -147,6 +149,8 @@ std::this_thread::sleep_for(std::chrono::seconds(2)); S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto, [&](std::vector Diags) { ++CallbackCount; }); + + ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); } EXPECT_EQ(2, CallbackCount); } @@ -267,6 +271,8 @@ Update("U3"); Read("R3")(); Proceed.notify(); + + ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); } EXPECT_THAT(DiagsSeen, ElementsAre("U2", "U3")) << "U1 and all dependent reads were cancelled. " @@ -373,6 +379,7 @@ } } } + ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); } // TUScheduler destructor waits for all operations to finish. std::lock_guard Lock(Mut);