Only report diagnostics from code completion document parse.

Also use shared_ptr, hopefully fix memory crashes.

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<clang::TranslationUnit>* 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<clang::TranslationUnit>(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<lsDiagnostic> ls_diagnostics;
+    unsigned num_diagnostics = clang_getNumDiagnostics((*tu)->cx_tu);
+    for (unsigned i = 0; i < num_diagnostics; ++i) {
+      optional<lsDiagnostic> 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<clang::TranslationUnit> 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<std::mutex> 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<lsDiagnostic> 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<lsDiagnostic> 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<CompletionSession> LruSessionCache::TryTakeEntry(const std::string& filename) {
+std::shared_ptr<CompletionSession> LruSessionCache::TryTakeEntry(const std::string& filename) {
   for (int i = 0; i < entries_.size(); ++i) {
     if (entries_[i]->file.filename == filename) {
-      std::unique_ptr<CompletionSession> result = std::move(entries_[i]);
+      std::shared_ptr<CompletionSession> result = std::move(entries_[i]);
       entries_.erase(entries_.begin() + i);
       return result;
     }
@@ -409,7 +405,7 @@ std::unique_ptr<CompletionSession> LruSessionCache::TryTakeEntry(const std::stri
   return nullptr;
 }
 
-void LruSessionCache::InsertEntry(std::unique_ptr<CompletionSession> session) {
+void LruSessionCache::InsertEntry(std::shared_ptr<CompletionSession> 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<CompletionSession> 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)
+ClangCompleteManager::ClangCompleteManager(Config* config, Project* project, WorkingFiles* working_files, OnDiagnostic on_diagnostic)
-    : config_(config), project_(project), working_files_(working_files),
+    : 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<CompletionSession> session = view_sessions_.TryTakeEntry(filename)) {
+  if (std::shared_ptr<CompletionSession> session = view_sessions_.TryTakeEntry(filename)) {
     edit_sessions_.InsertEntry(std::move(session));
   }
 

src/clang_complete.h

@@ -34,7 +34,7 @@ struct CompletionSession {
 };
 
 struct LruSessionCache {
-  std::vector<std::unique_ptr<CompletionSession>> entries_;
+  std::vector<std::shared_ptr<CompletionSession>> 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<CompletionSession> TryTakeEntry(const std::string& fiilename);
+  std::shared_ptr<CompletionSession> TryTakeEntry(const std::string& fiilename);
   // Inserts an entry. Evicts the oldest unused entry if there is no space.
-  void InsertEntry(std::unique_ptr<CompletionSession> session);
+  void InsertEntry(std::shared_ptr<CompletionSession> session);
 };
 
 struct ClangCompleteManager {
-  using OnComplete = std::function<void(NonElidedVector<lsCompletionItem> results, NonElidedVector<lsDiagnostic> diagnostics)>;
+  using OnDiagnostic = std::function<void(std::string path, NonElidedVector<lsDiagnostic> diagnostics)>;
+  using OnComplete = std::function<void(NonElidedVector<lsCompletionItem> 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.

src/clang_utils.cc

@@ -21,7 +21,7 @@ lsRange GetLsRangeForFixIt(const CXSourceRange& range) {
 
 }  // namespace
 
-optional<lsDiagnostic> BuildDiagnostic(CXDiagnostic diagnostic) {
+optional<lsDiagnostic> BuildAndDisposeDiagnostic(CXDiagnostic diagnostic) {
   // Skip diagnostics in system headers.
   CXSourceLocation diag_loc = clang_getDiagnosticLocation(diagnostic);
   if (clang_equalLocations(diag_loc, clang_getNullLocation()) ||

src/clang_utils.h

@@ -9,7 +9,7 @@
 
 using namespace std::experimental;
 
-optional<lsDiagnostic> BuildDiagnostic(CXDiagnostic diagnostic);
+optional<lsDiagnostic> BuildAndDisposeDiagnostic(CXDiagnostic diagnostic);
 
 // Returns the absolute path to |file|.
 std::string FileName(CXFile file);

src/command_line.cc

@@ -93,7 +93,6 @@ struct CodeCompleteCache {
   optional<std::string> cached_path;
   optional<lsPosition> cached_completion_position;
   NonElidedVector<lsCompletionItem> cached_results;
-  NonElidedVector<lsDiagnostic> cached_diagnostics;
 
   bool IsCacheValid(lsTextDocumentPositionParams position) const {
     return cached_path == position.textDocument.uri.GetPath() &&
@@ -1243,7 +1242,25 @@ optional<lsTextEdit> BuildAutoImplementForFunction(QueryDatabase* db, WorkingFil
   return nullopt;
 }
 
+void EmitDiagnostics(WorkingFiles* working_files, std::string path, NonElidedVector<lsDiagnostic> 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
+    // Publish lines skipped by the preprocessor if this is an interactive
-    // |is_interactive| is true, as we may have previously published diagnostic
+    // index.
-    // for that file. If the user has diagnostics on files they are not
+    if (is_interactive) {
-    // 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.
       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
-          // TODO: We shouldn't be updating actual indexed content here, but we
+        // need to use the latest indexed content for the remapping.
-          // need to use the latest indexed content for the remapping.
+        // TODO: We should also remap diagnostics.
-          // TODO: We should also remap diagnostics.
+        if (indexed_content)
-          if (indexed_content)
+          working_file->SetIndexContent(*indexed_content);
-            working_file->SetIndexContent(*indexed_content);
+        PublishInactiveLines(working_file, new_index->skipped_by_preprocessor);
-
-          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<lsCompletionItem> results, NonElidedVector<lsDiagnostic> diagnostics) {
+            (Ipc_TextDocumentComplete* msg, NonElidedVector<lsCompletionItem> results) {
-
-            auto ipc = IpcManager::instance();
 
             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);
+            IpcManager::instance()->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;
 
             // 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<Ipc_TextDocumentComplete*>(message.release()), std::placeholders::_1, std::placeholders::_2);
+          }, static_cast<Ipc_TextDocumentComplete*>(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<lsCompletionItem> results, NonElidedVector<lsDiagnostic> diagnostics) {
+            ClangCompleteManager::OnComplete freshen_global =
+              [global_code_complete_cache]
+              (NonElidedVector<lsCompletionItem> 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;

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<IndexParam*>(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<lsDiagnostic> ls_diagnostic = BuildDiagnostic(diagnostic);
-    if (ls_diagnostic)
-      db->diagnostics.push_back(*ls_diagnostic);
-  }
 }
 
 CXIdxClientFile enteredMainFile(CXClientData client_data,

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<lsDiagnostic> diagnostics;
-
   // Source ranges that were not processed.
   std::vector<Range> skipped_by_preprocessor;
 

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