From dec484ed0dacd58c60cdbd21fe9ccc73e2f96dcd Mon Sep 17 00:00:00 2001 From: Jacob Dufault Date: Fri, 9 Jun 2017 21:13:16 -0700 Subject: [PATCH] Only report diagnostics from code completion document parse. Also use shared_ptr, hopefully fix memory crashes. --- src/clang_complete.cc | 52 +++++++++++------------ src/clang_complete.h | 12 +++--- src/clang_utils.cc | 2 +- src/clang_utils.h | 2 +- src/command_line.cc | 96 ++++++++++++++++++------------------------- src/indexer.cc | 24 ----------- src/indexer.h | 3 -- src/test.cc | 5 --- 8 files changed, 74 insertions(+), 122 deletions(-) diff --git a/src/clang_complete.cc b/src/clang_complete.cc index f6a6c2a1..c9fdd128 100644 --- a/src/clang_complete.cc +++ b/src/clang_complete.cc @@ -237,7 +237,8 @@ void BuildDetailString(CXCompletionString completion_string, std::string& label, } } -void EnsureDocumentParsed(CompletionSession* session, +void EnsureDocumentParsed(ClangCompleteManager* manager, + CompletionSession* session, std::unique_ptr* tu, clang::Index* index) { // Nothing to do. We already have a translation unit. @@ -252,6 +253,18 @@ void EnsureDocumentParsed(CompletionSession* session, std::cerr << "[complete] Creating completion session with arguments " << StringJoin(args) << std::endl; *tu = MakeUnique(index, session->file.filename, args, unsaved, Flags()); std::cerr << "[complete] Done creating active; did_fail=" << (*tu)->did_fail << std::endl; + + // Build diagnostics. + if (!(*tu)->did_fail) { + NonElidedVector ls_diagnostics; + unsigned num_diagnostics = clang_getNumDiagnostics((*tu)->cx_tu); + for (unsigned i = 0; i < num_diagnostics; ++i) { + optional diagnostic = BuildAndDisposeDiagnostic(clang_getDiagnostic((*tu)->cx_tu, i)); + if (diagnostic) + ls_diagnostics.push_back(*diagnostic); + } + manager->on_diagnostic_(session->file.filename, ls_diagnostics); + } } void CompletionParseMain(ClangCompleteManager* completion_manager) { @@ -273,7 +286,7 @@ void CompletionParseMain(ClangCompleteManager* completion_manager) { } std::unique_ptr parsing; - EnsureDocumentParsed(session, &parsing, &session->index); + EnsureDocumentParsed(completion_manager, session, &parsing, &session->index); // Activate new translation unit. // tu_last_parsed_at is only read by this thread, so it doesn't need to be under the mutex. @@ -292,7 +305,7 @@ void CompletionQueryMain(ClangCompleteManager* completion_manager) { CompletionSession* session = completion_manager->TryGetSession(path, true /*create_if_needed*/); std::lock_guard lock(session->tu_lock); - EnsureDocumentParsed(session, &session->tu, &session->index); + EnsureDocumentParsed(completion_manager, session, &session->tu, &session->index); // Language server is 0-based, clang is 1-based. unsigned line = request->location.position.line + 1; @@ -314,7 +327,7 @@ void CompletionQueryMain(ClangCompleteManager* completion_manager) { kCompleteOptions); if (!cx_results) { timer.ResetAndPrint("[complete] Code completion failed"); - request->on_complete({}, {}); + request->on_complete({}); continue; } @@ -355,27 +368,10 @@ void CompletionQueryMain(ClangCompleteManager* completion_manager) { } timer.ResetAndPrint("[complete] Building " + std::to_string(ls_result.size()) + " completion results"); - // Build diagnostics. - NonElidedVector ls_diagnostics; - timer.Reset(); - /* - unsigned num_diagnostics = clang_codeCompleteGetNumDiagnostics(cx_results); - std::cerr << "!! There are " + std::to_string(num_diagnostics) + " diagnostics to build\n"; - for (unsigned i = 0; i < num_diagnostics; ++i) { - std::cerr << "!! Building diagnostic " + std::to_string(i) + "\n"; - CXDiagnostic cx_diag = clang_codeCompleteGetDiagnostic(cx_results, i); - optional diagnostic = BuildDiagnostic(cx_diag); - if (diagnostic) - ls_diagnostics.push_back(*diagnostic); - } - timer.ResetAndPrint("[complete] Build diagnostics"); - */ - - clang_disposeCodeCompleteResults(cx_results); timer.ResetAndPrint("[complete] clang_disposeCodeCompleteResults"); - request->on_complete(ls_result, ls_diagnostics); + request->on_complete(ls_result); continue; } @@ -398,10 +394,10 @@ CompletionSession* LruSessionCache::TryGetEntry(const std::string& filename) { return nullptr; } -std::unique_ptr LruSessionCache::TryTakeEntry(const std::string& filename) { +std::shared_ptr LruSessionCache::TryTakeEntry(const std::string& filename) { for (int i = 0; i < entries_.size(); ++i) { if (entries_[i]->file.filename == filename) { - std::unique_ptr result = std::move(entries_[i]); + std::shared_ptr result = std::move(entries_[i]); entries_.erase(entries_.begin() + i); return result; } @@ -409,7 +405,7 @@ std::unique_ptr LruSessionCache::TryTakeEntry(const std::stri return nullptr; } -void LruSessionCache::InsertEntry(std::unique_ptr session) { +void LruSessionCache::InsertEntry(std::shared_ptr session) { if (entries_.size() >= max_entries_) entries_.pop_back(); entries_.insert(entries_.begin(), std::move(session)); @@ -418,8 +414,8 @@ void LruSessionCache::InsertEntry(std::unique_ptr session) { ClangCompleteManager::ParseRequest::ParseRequest(const std::string& path) : request_time(std::chrono::high_resolution_clock::now()), path(path) {} -ClangCompleteManager::ClangCompleteManager(Config* config, Project* project, WorkingFiles* working_files) - : config_(config), project_(project), working_files_(working_files), +ClangCompleteManager::ClangCompleteManager(Config* config, Project* project, WorkingFiles* working_files, OnDiagnostic on_diagnostic) + : config_(config), project_(project), working_files_(working_files), on_diagnostic_(on_diagnostic), view_sessions_(kMaxViewSessions), edit_sessions_(kMaxEditSessions) { new std::thread([&]() { SetCurrentThreadName("completequery"); @@ -471,7 +467,7 @@ void ClangCompleteManager::NotifyEdit(const std::string& filename) { if (edit_sessions_.TryGetEntry(filename)) return; - if (std::unique_ptr session = view_sessions_.TryTakeEntry(filename)) { + if (std::shared_ptr session = view_sessions_.TryTakeEntry(filename)) { edit_sessions_.InsertEntry(std::move(session)); } diff --git a/src/clang_complete.h b/src/clang_complete.h index 5a53cca1..e199230f 100644 --- a/src/clang_complete.h +++ b/src/clang_complete.h @@ -34,7 +34,7 @@ struct CompletionSession { }; struct LruSessionCache { - std::vector> entries_; + std::vector> entries_; int max_entries_; LruSessionCache(int max_entries); @@ -43,13 +43,14 @@ struct LruSessionCache { // likely to be evicted. CompletionSession* TryGetEntry(const std::string& filename); // TryGetEntry, except the return value captures ownership. - std::unique_ptr TryTakeEntry(const std::string& fiilename); + std::shared_ptr TryTakeEntry(const std::string& fiilename); // Inserts an entry. Evicts the oldest unused entry if there is no space. - void InsertEntry(std::unique_ptr session); + void InsertEntry(std::shared_ptr session); }; struct ClangCompleteManager { - using OnComplete = std::function results, NonElidedVector diagnostics)>; + using OnDiagnostic = std::function diagnostics)>; + using OnComplete = std::function results)>; struct ParseRequest { ParseRequest(const std::string& path); @@ -62,7 +63,7 @@ struct ClangCompleteManager { OnComplete on_complete; }; - ClangCompleteManager(Config* config, Project* project, WorkingFiles* working_files); + ClangCompleteManager(Config* config, Project* project, WorkingFiles* working_files, OnDiagnostic on_diagnostic); // Start a code completion at the given location. |on_complete| will run when // completion results are available. |on_complete| may run on any thread. @@ -88,6 +89,7 @@ struct ClangCompleteManager { Config* config_; Project* project_; WorkingFiles* working_files_; + OnDiagnostic on_diagnostic_; // Sessions which have never had a real text-edit applied, but are preloaded // to give a fast initial experience. diff --git a/src/clang_utils.cc b/src/clang_utils.cc index 6f018e90..c6b15767 100644 --- a/src/clang_utils.cc +++ b/src/clang_utils.cc @@ -21,7 +21,7 @@ lsRange GetLsRangeForFixIt(const CXSourceRange& range) { } // namespace -optional BuildDiagnostic(CXDiagnostic diagnostic) { +optional BuildAndDisposeDiagnostic(CXDiagnostic diagnostic) { // Skip diagnostics in system headers. CXSourceLocation diag_loc = clang_getDiagnosticLocation(diagnostic); if (clang_equalLocations(diag_loc, clang_getNullLocation()) || diff --git a/src/clang_utils.h b/src/clang_utils.h index 6701b6fe..5e3494ea 100644 --- a/src/clang_utils.h +++ b/src/clang_utils.h @@ -9,7 +9,7 @@ using namespace std::experimental; -optional BuildDiagnostic(CXDiagnostic diagnostic); +optional BuildAndDisposeDiagnostic(CXDiagnostic diagnostic); // Returns the absolute path to |file|. std::string FileName(CXFile file); diff --git a/src/command_line.cc b/src/command_line.cc index cbd5be53..d87abcc1 100644 --- a/src/command_line.cc +++ b/src/command_line.cc @@ -93,7 +93,6 @@ struct CodeCompleteCache { optional cached_path; optional cached_completion_position; NonElidedVector cached_results; - NonElidedVector cached_diagnostics; bool IsCacheValid(lsTextDocumentPositionParams position) const { return cached_path == position.textDocument.uri.GetPath() && @@ -1243,7 +1242,25 @@ optional BuildAutoImplementForFunction(QueryDatabase* db, WorkingFil return nullopt; } +void EmitDiagnostics(WorkingFiles* working_files, std::string path, NonElidedVector diagnostics) { + // Emit diagnostics. + Out_TextDocumentPublishDiagnostics diagnostic_response; + diagnostic_response.params.uri = lsDocumentUri::FromPath(path); + diagnostic_response.params.diagnostics = diagnostics; + IpcManager::instance()->SendOutMessageToClient(IpcId::TextDocumentPublishDiagnostics, diagnostic_response); + // Cache diagnostics so we can show fixits. + // TODO/FIXME: this function can run concurrently on any thread!!!! + // TODO/FIXME: this function can run concurrently on any thread!!!! + // TODO/FIXME: this function can run concurrently on any thread!!!! + // TODO/FIXME: this function can run concurrently on any thread!!!! + // TODO/FIXME: this function can run concurrently on any thread!!!! + // TODO/FIXME: this function can run concurrently on any thread!!!! + // TODO/FIXME: this function can run concurrently on any thread!!!! + WorkingFile* working_file = working_files->GetFileByFilename(path); + if (working_file) + working_file->diagnostics = diagnostics; +} @@ -1585,38 +1602,21 @@ void ParseFile(Config* config, // Note: we are reusing the parent perf. perf.index_load_cached = time.ElapsedMicrosecondsAndReset(); - // Publish diagnostics. We should only need to publish empty diagnostics if - // |is_interactive| is true, as we may have previously published diagnostic - // for that file. If the user has diagnostics on files they are not - // editing, then they can either edit the file, in which case - // |is_interactive| will be true, or they can change the flags cquery runs - // with, in which case vscode will get restarted. - if (is_interactive || !new_index->diagnostics.empty()) { - // Emit diagnostics. - Out_TextDocumentPublishDiagnostics diag; - diag.params.uri = lsDocumentUri::FromPath(new_index->path); - diag.params.diagnostics = new_index->diagnostics; - IpcManager::instance()->SendOutMessageToClient(IpcId::TextDocumentPublishDiagnostics, diag); - - // Cache diagnostics so we can show fixits. + // Publish lines skipped by the preprocessor if this is an interactive + // index. + if (is_interactive) { WorkingFile* working_file = working_files->GetFileByFilename(new_index->path); if (working_file) { - working_file->diagnostics = new_index->diagnostics; - // Publish source ranges disabled by preprocessor. - if (is_interactive) { - // TODO: We shouldn't be updating actual indexed content here, but we - // need to use the latest indexed content for the remapping. - // TODO: We should also remap diagnostics. - if (indexed_content) - working_file->SetIndexContent(*indexed_content); - - PublishInactiveLines(working_file, new_index->skipped_by_preprocessor); - } + // TODO: We shouldn't be updating actual indexed content here, but we + // need to use the latest indexed content for the remapping. + // TODO: We should also remap diagnostics. + if (indexed_content) + working_file->SetIndexContent(*indexed_content); + PublishInactiveLines(working_file, new_index->skipped_by_preprocessor); } } - // Any any existing dependencies to |new_index| that were there before, // because we will not reparse them if they haven't changed. // TODO: indexer should always include dependencies. This doesn't let us remove old dependencies. @@ -2347,8 +2347,6 @@ bool QueryDbMainLoop( std::string path = msg->params.textDocument.uri.GetPath(); WorkingFile* file = working_files->GetFileByFilename(path); - // TODO: We should scan include directories to add any missing paths - // It shouldn't be possible, but sometimes vscode will send queries out // of order, ie, we get completion request before buffer content update. std::string buffer_line; @@ -2389,9 +2387,7 @@ bool QueryDbMainLoop( ClangCompleteManager::OnComplete callback = std::bind( [working_files, global_code_complete_cache, non_global_code_complete_cache, is_global_completion] - (Ipc_TextDocumentComplete* msg, NonElidedVector results, NonElidedVector diagnostics) { - - auto ipc = IpcManager::instance(); + (Ipc_TextDocumentComplete* msg, NonElidedVector results) { Out_TextDocumentComplete complete_response; complete_response.id = msg->id; @@ -2399,54 +2395,42 @@ bool QueryDbMainLoop( complete_response.result.items = results; // Emit completion results. - ipc->SendOutMessageToClient(IpcId::TextDocumentCompletion, complete_response); - - // Emit diagnostics. - Out_TextDocumentPublishDiagnostics diagnostic_response; - diagnostic_response.params.uri = msg->params.textDocument.uri; - diagnostic_response.params.diagnostics = diagnostics; - ipc->SendOutMessageToClient(IpcId::TextDocumentPublishDiagnostics, diagnostic_response); - - std::string path = msg->params.textDocument.uri.GetPath(); - - // Cache diagnostics so we can show fixits. - WorkingFile* working_file = working_files->GetFileByFilename(path); - if (working_file) - working_file->diagnostics = diagnostics; + IpcManager::instance()->SendOutMessageToClient(IpcId::TextDocumentCompletion, complete_response); // Cache completion results. + std::string path = msg->params.textDocument.uri.GetPath(); if (is_global_completion) { global_code_complete_cache->cached_path = path; global_code_complete_cache->cached_results = results; - global_code_complete_cache->cached_diagnostics = diagnostics; } else { non_global_code_complete_cache->cached_path = path; non_global_code_complete_cache->cached_completion_position = msg->params.position; non_global_code_complete_cache->cached_results = results; - non_global_code_complete_cache->cached_diagnostics = diagnostics; } delete msg; - }, static_cast(message.release()), std::placeholders::_1, std::placeholders::_2); + }, static_cast(message.release()), std::placeholders::_1); if (is_global_completion && global_code_complete_cache->cached_path == path && !global_code_complete_cache->cached_results.empty()) { std::cerr << "[complete] Early-returning cached global completion results at " << msg->params.position.ToString() << std::endl; - ClangCompleteManager::OnComplete freshen_global = [global_code_complete_cache](NonElidedVector results, NonElidedVector diagnostics) { + ClangCompleteManager::OnComplete freshen_global = + [global_code_complete_cache] + (NonElidedVector results) { + std::cerr << "[complete] Updated global completion cache" << std::endl; // note: path is updated in the normal completion handler. global_code_complete_cache->cached_results = results; - global_code_complete_cache->cached_diagnostics = diagnostics; }; clang_complete->CodeComplete(msg->params, std::move(freshen_global)); // Note: callback will delete the message (ie, |params|) so we need to run completion_manager->CodeComplete before |callback|. - callback(global_code_complete_cache->cached_results, global_code_complete_cache->cached_diagnostics); + callback(global_code_complete_cache->cached_results); } else if (non_global_code_complete_cache->IsCacheValid(msg->params)) { std::cerr << "[complete] Using cached completion results at " << msg->params.position.ToString() << std::endl; - callback(non_global_code_complete_cache->cached_results, non_global_code_complete_cache->cached_diagnostics); + callback(non_global_code_complete_cache->cached_results); } else { clang_complete->CodeComplete(msg->params, std::move(callback)); @@ -2521,7 +2505,7 @@ bool QueryDbMainLoop( if (signature_cache->IsCacheValid(params)) { std::cerr << "[complete] Using cached completion results at " << params.position.ToString() << std::endl; - callback(signature_cache->cached_results, signature_cache->cached_diagnostics); + callback(signature_cache->cached_results); } else { clang_complete->CodeComplete(params, std::move(callback)); @@ -3215,7 +3199,9 @@ void QueryDbMain(Config* config, MultiQueueWaiter* waiter) { Project project; WorkingFiles working_files; - ClangCompleteManager clang_complete(config, &project, &working_files); + ClangCompleteManager clang_complete( + config, &project, &working_files, + std::bind(&EmitDiagnostics, &working_files, std::placeholders::_1, std::placeholders::_2)); IncludeComplete include_complete(config, &project); CodeCompleteCache global_code_complete_cache; CodeCompleteCache non_global_code_complete_cache; diff --git a/src/indexer.cc b/src/indexer.cc index 5fbf9fc4..9fa8b220 100644 --- a/src/indexer.cc +++ b/src/indexer.cc @@ -367,30 +367,6 @@ int abortQuery(CXClientData client_data, void* reserved) { void diagnostic(CXClientData client_data, CXDiagnosticSet diagnostics, void* reserved) { - IndexParam* param = static_cast(client_data); - - // Print any diagnostics to std::cerr - for (unsigned i = 0; i < clang_getNumDiagnosticsInSet(diagnostics); ++i) { - CXDiagnostic diagnostic = clang_getDiagnosticInSet(diagnostics, i); - - // Skip diagnostics in system headers. - CXSourceLocation diag_loc = clang_getDiagnosticLocation(diagnostic); - if (clang_Location_isInSystemHeader(diag_loc)) - continue; - - // Get db so we can attribute diagnostic to the right indexed file. - CXFile file; - unsigned int line, column; - clang_getSpellingLocation(diag_loc, &file, &line, &column, nullptr); - IndexFile* db = ConsumeFile(param, file); - if (!db) - continue; - - // Build diagnostic. - optional ls_diagnostic = BuildDiagnostic(diagnostic); - if (ls_diagnostic) - db->diagnostics.push_back(*ls_diagnostic); - } } CXIdxClientFile enteredMainFile(CXClientData client_data, diff --git a/src/indexer.h b/src/indexer.h index efd45e86..84eef60c 100644 --- a/src/indexer.h +++ b/src/indexer.h @@ -472,9 +472,6 @@ struct IndexFile { // header we need to lookup the original translation unit and reindex that. std::string import_file; - // Diagnostics found when indexing the file. This is not saved. - NonElidedVector diagnostics; - // Source ranges that were not processed. std::vector skipped_by_preprocessor; diff --git a/src/test.cc b/src/test.cc index 81a0f5fc..9fbf15b1 100644 --- a/src/test.cc +++ b/src/test.cc @@ -152,11 +152,6 @@ void RunTests() { if (db) { VerifySerializeToFrom(db); actual_output = db->ToString(); - - for (lsDiagnostic diag : db->diagnostics) { - std::cerr << db->path << ":" << diag.range.start.line << ":" << diag.range.end.character << ": " << diag.message << std::endl; - } - } // Compare output via rapidjson::Document to ignore any formatting