From 772f5470651a5eeb4a80704512521a52409e8876 Mon Sep 17 00:00:00 2001 From: Jacob Dufault Date: Thu, 21 Sep 2017 19:25:33 -0700 Subject: [PATCH] Update diagnostics on document change, not code completion. This also changes the API used for reporting diagnostics, which will hopefully be more reliable. This requires reparsing the document, though, so it is much slower. We do this after reporting code completion though, so hopefully the performance delay is not too noticable. --- src/atomic_object.h | 11 ++ src/clang_complete.cc | 277 ++++++++++++++++--------------------- src/clang_complete.h | 15 +- src/command_line.cc | 25 ++-- src/language_server_api.cc | 7 + src/language_server_api.h | 2 + 6 files changed, 153 insertions(+), 184 deletions(-) diff --git a/src/atomic_object.h b/src/atomic_object.h index 432a9cff..a953f37c 100644 --- a/src/atomic_object.h +++ b/src/atomic_object.h @@ -33,6 +33,17 @@ struct AtomicObject { return std::move(value_); } + template + void WithLock(TAction action) { + std::unique_lock lock(mutex_); + bool had_value = !!value_; + action(value_); + bool has_value = !!value_; + + if (had_value != has_value) + cv_.notify_one(); + } + private: std::unique_ptr value_; mutable std::mutex mutex_; diff --git a/src/clang_complete.cc b/src/clang_complete.cc index 4a861a11..ee2c140f 100644 --- a/src/clang_complete.cc +++ b/src/clang_complete.cc @@ -5,6 +5,8 @@ #include "platform.h" #include "timer.h" +#include + #include #include @@ -354,64 +356,12 @@ void CompletionParseMain(ClangCompleteManager* completion_manager) { } } -void CompletionDiagnosticsDelayedRefreshMain( - ClangCompleteManager* completion_manager) { - constexpr int kSecondsToWaitForDiagnosticsRefresh = 5; - - // Refreshes diagnostics a few seconds after the final code completion, since - // we don't get a language server request. - while (true) { - std::unique_lock l( - completion_manager->delayed_diagnostic_wakeup_mtx_); - completion_manager->delayed_diagnostic_wakeup_cv_.wait(l); - - // Check for spurious wakeup. - if (!completion_manager->delayed_diagnostic_last_completion_position_) - continue; - - while (true) { - // Get completion request info. - if (!l.owns_lock()) - l.lock(); - lsTextDocumentPositionParams location = - *completion_manager->delayed_diagnostic_last_completion_position_; - completion_manager->delayed_diagnostic_last_completion_position_.reset(); - l.unlock(); - - // Wait five seconds. If there was another completion request, start the - // waiting process over again. - std::this_thread::sleep_for( - std::chrono::seconds(kSecondsToWaitForDiagnosticsRefresh)); - l.lock(); - bool has_completion_since_sleeping = - completion_manager->delayed_diagnostic_last_completion_position_ - .has_value(); - l.unlock(); - if (has_completion_since_sleeping) - continue; - - // Push completion request to the end of the line. clang may report errors - // if completing in the middle of the line. Not a perfect hueristic, but - // probably good enough. - // TODO: Consider scanning for a semicolon if this doesn't work well in - // practice. - location.position.character = 10000; - - // Make completion request to get refreshed diagnostics. - auto request = MakeUnique(); - request->location = location; - completion_manager->completion_request_.SetIfEmpty(std::move(request)); - break; - } - } -} - void CompletionQueryMain(ClangCompleteManager* completion_manager) { while (true) { // Fetching the completion request blocks until we have a request. std::unique_ptr request = completion_manager->completion_request_.Take(); - std::string path = request->location.textDocument.uri.GetPath(); + std::string path = request->document.uri.GetPath(); std::shared_ptr session = completion_manager->TryGetSession(path, true /*create_if_needed*/); @@ -419,113 +369,111 @@ void CompletionQueryMain(ClangCompleteManager* completion_manager) { std::lock_guard lock(session->tu_lock); EnsureDocumentParsed(completion_manager, session, &session->tu, &session->index); - - // Language server is 0-based, clang is 1-based. - unsigned line = request->location.position.line + 1; - unsigned column = request->location.position.character + 1; - - std::cerr << "[complete] Completing at " << line << ":" << column - << std::endl; - Timer timer; std::vector unsaved = completion_manager->working_files_->AsUnsavedFiles(); - timer.ResetAndPrint("[complete] Fetching unsaved files"); - timer.Reset(); - unsigned const kCompleteOptions = - CXCodeComplete_IncludeMacros | CXCodeComplete_IncludeBriefComments; - CXCodeCompleteResults* cx_results = clang_codeCompleteAt( - session->tu->cx_tu, session->file.filename.c_str(), line, column, - unsaved.data(), (unsigned)unsaved.size(), kCompleteOptions); - if (!cx_results) { - timer.ResetAndPrint("[complete] Code completion failed"); - if (request->on_complete) - request->on_complete({}, false /*is_cached_result*/); - continue; - } + // Emit code completion data. + if (request->position) { + // Language server is 0-based, clang is 1-based. + unsigned line = request->position->line + 1; + unsigned column = request->position->character + 1; - timer.ResetAndPrint("[complete] clangCodeCompleteAt"); - std::cerr << "[complete] Got " << cx_results->NumResults << " results" - << std::endl; + std::cerr << "[complete] Completing at " << line << ":" << column + << std::endl; - { - if (request->on_complete) { - NonElidedVector ls_result; - ls_result.reserve(cx_results->NumResults); + timer.ResetAndPrint("[complete] Fetching unsaved files"); - timer.Reset(); - for (unsigned i = 0; i < cx_results->NumResults; ++i) { - CXCompletionResult& result = cx_results->Results[i]; - - // TODO: Try to figure out how we can hide base method calls without - // also hiding method implementation assistance, ie, - // - // void Foo::* { - // } - // - - if (clang_getCompletionAvailability(result.CompletionString) == - CXAvailability_NotAvailable) - continue; - - // TODO: fill in more data - lsCompletionItem ls_completion_item; - - // kind/label/detail/docs/sortText - ls_completion_item.kind = GetCompletionKind(result.CursorKind); - BuildDetailString(result.CompletionString, ls_completion_item.label, - ls_completion_item.detail, - ls_completion_item.insertText, - &ls_completion_item.parameters_); - ls_completion_item.insertText += "$0"; - ls_completion_item.documentation = clang::ToString( - clang_getCompletionBriefComment(result.CompletionString)); - ls_completion_item.sortText = (const char)uint64_t( - GetCompletionPriority(result.CompletionString, result.CursorKind, - ls_completion_item.label)); - - ls_result.push_back(ls_completion_item); - } - timer.ResetAndPrint("[complete] Building " + - std::to_string(ls_result.size()) + - " completion results"); - - request->on_complete(ls_result, false /*is_cached_result*/); - timer.ResetAndPrint("[complete] Running user-given completion func"); + timer.Reset(); + unsigned const kCompleteOptions = + CXCodeComplete_IncludeMacros | CXCodeComplete_IncludeBriefComments; + CXCodeCompleteResults* cx_results = clang_codeCompleteAt( + session->tu->cx_tu, session->file.filename.c_str(), line, column, + unsaved.data(), (unsigned)unsaved.size(), kCompleteOptions); + if (!cx_results) { + timer.ResetAndPrint("[complete] Code completion failed"); + if (request->on_complete) + request->on_complete({}, false /*is_cached_result*/); + continue; } - if (completion_manager->config_->diagnosticsOnCodeCompletion) { - unsigned num_diagnostics = - clang_codeCompleteGetNumDiagnostics(cx_results); - NonElidedVector ls_diagnostics; - for (unsigned i = 0; i < num_diagnostics; ++i) { - CXDiagnostic cx_diag = clang_codeCompleteGetDiagnostic(cx_results, i); - optional diagnostic = - BuildAndDisposeDiagnostic(cx_diag, path); - if (diagnostic) - ls_diagnostics.push_back(*diagnostic); - } - completion_manager->on_diagnostic_(session->file.filename, - ls_diagnostics); - timer.ResetAndPrint("[complete] Build diagnostics"); - } - } + timer.ResetAndPrint("[complete] clangCodeCompleteAt"); + std::cerr << "[complete] Got " << cx_results->NumResults << " results" + << std::endl; - // Make sure |ls_results| is destroyed before clearing |cx_results|. - clang_disposeCodeCompleteResults(cx_results); - timer.ResetAndPrint("[complete] clang_disposeCodeCompleteResults"); - - if (completion_manager->config_->diagnosticsOnCodeCompletion && - request->is_user_completion) { { - std::lock_guard lock( - completion_manager->delayed_diagnostic_wakeup_mtx_); - completion_manager->delayed_diagnostic_last_completion_position_ = - request->location; + if (request->on_complete) { + NonElidedVector ls_result; + ls_result.reserve(cx_results->NumResults); + + timer.Reset(); + for (unsigned i = 0; i < cx_results->NumResults; ++i) { + CXCompletionResult& result = cx_results->Results[i]; + + // TODO: Try to figure out how we can hide base method calls without + // also hiding method implementation assistance, ie, + // + // void Foo::* { + // } + // + + if (clang_getCompletionAvailability(result.CompletionString) == + CXAvailability_NotAvailable) + continue; + + // TODO: fill in more data + lsCompletionItem ls_completion_item; + + // kind/label/detail/docs/sortText + ls_completion_item.kind = GetCompletionKind(result.CursorKind); + BuildDetailString(result.CompletionString, ls_completion_item.label, + ls_completion_item.detail, + ls_completion_item.insertText, + &ls_completion_item.parameters_); + ls_completion_item.insertText += "$0"; + ls_completion_item.documentation = clang::ToString( + clang_getCompletionBriefComment(result.CompletionString)); + ls_completion_item.sortText = + (const char)uint64_t(GetCompletionPriority( + result.CompletionString, result.CursorKind, + ls_completion_item.label)); + + ls_result.push_back(ls_completion_item); + } + timer.ResetAndPrint("[complete] Building " + + std::to_string(ls_result.size()) + + " completion results"); + + request->on_complete(ls_result, false /*is_cached_result*/); + timer.ResetAndPrint("[complete] Running user-given completion func"); + } } - completion_manager->delayed_diagnostic_wakeup_cv_.notify_one(); + + // Make sure |ls_results| is destroyed before clearing |cx_results|. + clang_disposeCodeCompleteResults(cx_results); + } + + // Emit diagnostics. + if (request->emit_diagnostics) { + timer.Reset(); + clang_reparseTranslationUnit( + session->tu->cx_tu, unsaved.size(), unsaved.data(), + clang_defaultReparseOptions(session->tu->cx_tu)); + timer.ResetAndPrint("[complete] clang_reparseTranslationUnit"); + + size_t num_diagnostics = clang_getNumDiagnostics(session->tu->cx_tu); + NonElidedVector ls_diagnostics; + ls_diagnostics.reserve(num_diagnostics); + for (unsigned i = 0; i < num_diagnostics; ++i) { + CXDiagnostic cx_diag = clang_getDiagnostic(session->tu->cx_tu, i); + optional diagnostic = + BuildAndDisposeDiagnostic(cx_diag, path); + if (diagnostic) + ls_diagnostics.push_back(*diagnostic); + } + completion_manager->on_diagnostic_(session->file.filename, + ls_diagnostics); } continue; @@ -596,11 +544,6 @@ ClangCompleteManager::ClangCompleteManager(Config* config, CompletionQueryMain(this); }); - new std::thread([&]() { - SetCurrentThreadName("completediagnosticsrefresh"); - CompletionDiagnosticsDelayedRefreshMain(this); - }); - new std::thread([&]() { SetCurrentThreadName("completeparse"); CompletionParseMain(this); @@ -612,13 +555,31 @@ ClangCompleteManager::~ClangCompleteManager() {} void ClangCompleteManager::CodeComplete( const lsTextDocumentPositionParams& completion_location, const OnComplete& on_complete) { - // completion thread will create the CompletionSession if needed. + completion_request_.WithLock( + [&](std::unique_ptr& request_storage) { + // Ensure that we have a request. + if (!request_storage) + request_storage = MakeUnique(); - auto request = MakeUnique(); - request->location = completion_location; - request->on_complete = on_complete; - request->is_user_completion = true; - completion_request_.Set(std::move(request)); + // Make the request send out code completion information. + request_storage->document = completion_location.textDocument; + request_storage->position = completion_location.position; + request_storage->on_complete = on_complete; + }); +} + +void ClangCompleteManager::DiagnosticsUpdate( + const lsTextDocumentIdentifier& document) { + completion_request_.WithLock( + [&](std::unique_ptr& request_storage) { + // Ensure that we have a request. + if (!request_storage) + request_storage = MakeUnique(); + + // Make the request emit diagnostics. + request_storage->document = document; + request_storage->emit_diagnostics = true; + }); } void ClangCompleteManager::NotifyView(const std::string& filename) { diff --git a/src/clang_complete.h b/src/clang_complete.h index 64a3ba31..6e02aa7f 100644 --- a/src/clang_complete.h +++ b/src/clang_complete.h @@ -63,9 +63,10 @@ struct ClangCompleteManager { std::string path; }; struct CompletionRequest { - lsTextDocumentPositionParams location; - OnComplete on_complete; - bool is_user_completion = false; + lsTextDocumentIdentifier document; + optional position; + OnComplete on_complete; // May be null/empty. + bool emit_diagnostics = false; }; ClangCompleteManager(Config* config, @@ -78,6 +79,8 @@ struct ClangCompleteManager { // completion results are available. |on_complete| may run on any thread. void CodeComplete(const lsTextDocumentPositionParams& completion_location, const OnComplete& on_complete); + // Request a diagnostics update. + void DiagnosticsUpdate(const lsTextDocumentIdentifier& document); // Notify the completion manager that |filename| has been viewed and we // should begin preloading completion data. @@ -114,10 +117,4 @@ struct ClangCompleteManager { // Parse requests. The path may already be parsed, in which case it should be // reparsed. ThreadedQueue parse_requests_; - // Used to wakeup the delayed diagnostics thread. - std::mutex delayed_diagnostic_wakeup_mtx_; - std::condition_variable delayed_diagnostic_wakeup_cv_; - // Access under |delayed_diagnostic_wakeup_mtx_|. - optional - delayed_diagnostic_last_completion_position_; }; diff --git a/src/command_line.cc b/src/command_line.cc index ed53b9a3..dde5ffa6 100644 --- a/src/command_line.cc +++ b/src/command_line.cc @@ -728,6 +728,7 @@ struct IndexManager { //////////////////////////////////////////////////////////////////////////////// enum class FileParseQuery { NeedsParse, DoesNotNeedParse, BadFile }; + std::vector DoParseFile( Config* config, WorkingFiles* working_files, @@ -860,27 +861,15 @@ std::vector DoParseFile( PerformanceImportFile perf; std::vector> indexes = Parse( config, file_consumer_shared, path, args, file_contents, &perf, index); - // LOG_S(INFO) << "Parsing " << path << " gave " << indexes.size() << " - // indexes"; - for (std::unique_ptr& new_index : indexes) { Timer time; - if (is_interactive) - EmitDiagnostics(working_files, new_index->path, new_index->diagnostics_); - - // TODO: don't load cached index. We don't need to do this when indexer - // always exports dependency tree. Sanity check that verifies we did not - // generate a new index for a file whose timestamp did not change. - //{ - // IndexFile* previous_index = cache_loader->TryLoad(new_index->path); - // assert(!previous_index || GetLastModificationTime(new_index->path) != - // previous_index->last_modification_time); - //} - - // Note: we are reusing the parent perf. - perf.index_load_cached = time.ElapsedMicrosecondsAndReset(); + // Always emit diagnostics. This makes it easier to identify indexing + // problems. + EmitDiagnostics(working_files, new_index->path, new_index->diagnostics_); + // When main thread does IdMap request it will request the previous index if + // needed. LOG_S(INFO) << "Emitting index result for " << new_index->path; result.push_back(Index_DoIdMap(std::move(new_index), perf, is_interactive, true /*write_to_disk*/)); @@ -1663,6 +1652,8 @@ bool QueryDbMainLoop(Config* config, std::string path = msg->params.textDocument.uri.GetPath(); working_files->OnChange(msg->params); clang_complete->NotifyEdit(path); + clang_complete->DiagnosticsUpdate( + msg->params.textDocument.AsTextDocumentIdentifier()); break; } diff --git a/src/language_server_api.cc b/src/language_server_api.cc index ea4c821d..9c1471c8 100644 --- a/src/language_server_api.cc +++ b/src/language_server_api.cc @@ -24,6 +24,13 @@ void Reflect(Reader& visitor, lsRequestId& id) { MessageRegistry* MessageRegistry::instance_ = nullptr; +lsTextDocumentIdentifier +lsVersionedTextDocumentIdentifier::AsTextDocumentIdentifier() const { + lsTextDocumentIdentifier result; + result.uri = uri; + return result; +} + // Reads a JsonRpc message. |read| returns the next input character. optional ReadJsonRpcContentFrom( std::function()> read) { diff --git a/src/language_server_api.h b/src/language_server_api.h index 336f48f7..2b5d6785 100644 --- a/src/language_server_api.h +++ b/src/language_server_api.h @@ -338,6 +338,8 @@ struct lsVersionedTextDocumentIdentifier { lsDocumentUri uri; // The version number of this document. int version = 0; + + lsTextDocumentIdentifier AsTextDocumentIdentifier() const; }; MAKE_REFLECT_STRUCT(lsVersionedTextDocumentIdentifier, uri, version);