Fixing the diagnostic-related race conditions

Now, every FileReadyToParse event returns diagnostics, if any. This is instead
of the previous system where the diagnostics were being fetched in a different
request (this caused race conditions).

autoload/youcompleteme.vim

@@ -258,9 +258,6 @@ function! s:OnCursorHold()
   endif
 
   call s:SetUpCompleteopt()
-  " Order is important here; we need to extract any done diagnostics before
-  " reparsing the file again
-  call s:UpdateDiagnosticNotifications()
   call s:OnFileReadyToParse()
 endfunction
 
@@ -270,9 +267,14 @@ function! s:OnFileReadyToParse()
   " happen for special buffers.
   call s:SetUpYcmChangedTick()
 
+  " Order is important here; we need to extract any done diagnostics before
+  " reparsing the file again. If we sent the new parse request first, then
+  " the response would always be pending when we called
+  " UpdateDiagnosticNotifications.
+  call s:UpdateDiagnosticNotifications()
+
   let buffer_changed = b:changedtick != b:ycm_changedtick.file_ready_to_parse
   if buffer_changed
-    py ycm_state.RequestDiagnosticsForCurrentFile()
     py ycm_state.OnFileReadyToParse()
   endif
   let b:ycm_changedtick.file_ready_to_parse = b:changedtick
@@ -328,7 +330,6 @@ function! s:OnCursorMovedNormalMode()
     return
   endif
 
-  call s:UpdateDiagnosticNotifications()
   call s:OnFileReadyToParse()
 endfunction
 
@@ -339,7 +340,6 @@ function! s:OnInsertLeave()
   endif
 
   let s:omnifunc_mode = 0
-  call s:UpdateDiagnosticNotifications()
   call s:OnFileReadyToParse()
   py ycm_state.OnInsertLeave()
   if g:ycm_autoclose_preview_window_after_completion ||
@@ -660,13 +660,14 @@ function! s:ForceCompile()
 endfunction
 
 
+" TODO: Make this work again.
 function! s:ForceCompileAndDiagnostics()
   let compilation_succeeded = s:ForceCompile()
   if !compilation_succeeded
     return
   endif
 
-  call s:UpdateDiagnosticNotifications()
+  " call s:UpdateDiagnosticNotifications()
   echom "Diagnostics refreshed."
 endfunction
 

cpp/ycm/ClangCompleter/ClangCompleter.cpp

@@ -58,17 +58,6 @@ ClangCompleter::~ClangCompleter() {
 }
 
 
-std::vector< Diagnostic > ClangCompleter::DiagnosticsForFile(
-  const std::string &filename ) {
-  shared_ptr< TranslationUnit > unit = translation_unit_store_.Get( filename );
-
-  if ( !unit )
-    return std::vector< Diagnostic >();
-
-  return unit->LatestDiagnostics();
-}
-
-
 bool ClangCompleter::UpdatingTranslationUnit( const std::string &filename ) {
   shared_ptr< TranslationUnit > unit = translation_unit_store_.Get( filename );
 
@@ -82,7 +71,7 @@ bool ClangCompleter::UpdatingTranslationUnit( const std::string &filename ) {
 }
 
 
-void ClangCompleter::UpdateTranslationUnit(
+std::vector< Diagnostic > ClangCompleter::UpdateTranslationUnit(
   const std::string &filename,
   const std::vector< UnsavedFile > &unsaved_files,
   const std::vector< std::string > &flags ) {
@@ -94,13 +83,14 @@ void ClangCompleter::UpdateTranslationUnit(
       translation_unit_created );
 
   if ( !unit )
-    return;
+    return std::vector< Diagnostic >();
 
   try {
     // There's no point in reparsing a TU that was just created, it was just
     // parsed in the TU constructor
     if ( !translation_unit_created )
-      unit->Reparse( unsaved_files );
+      return unit->Reparse( unsaved_files );
+    return unit->LatestDiagnostics();
   }
 
   catch ( ClangParseError & ) {
@@ -109,6 +99,8 @@ void ClangCompleter::UpdateTranslationUnit(
     // TU map.
     translation_unit_store_.Remove( filename );
   }
+
+  return std::vector< Diagnostic >();
 }
 
 

cpp/ycm/ClangCompleter/ClangCompleter.h

@@ -43,11 +43,9 @@ public:
   ClangCompleter();
   ~ClangCompleter();
 
-  std::vector< Diagnostic > DiagnosticsForFile( const std::string &filename );
-
   bool UpdatingTranslationUnit( const std::string &filename );
 
-  void UpdateTranslationUnit(
+  std::vector< Diagnostic > UpdateTranslationUnit(
     const std::string &filename,
     const std::vector< UnsavedFile > &unsaved_files,
     const std::vector< std::string > &flags );

cpp/ycm/ClangCompleter/TranslationUnit.cpp

@@ -111,12 +111,15 @@ bool TranslationUnit::IsCurrentlyUpdating() const {
 }
 
 
-void TranslationUnit::Reparse(
+std::vector< Diagnostic > TranslationUnit::Reparse(
   const std::vector< UnsavedFile > &unsaved_files ) {
   std::vector< CXUnsavedFile > cxunsaved_files =
     ToCXUnsavedFiles( unsaved_files );
 
   Reparse( cxunsaved_files );
+
+  unique_lock< mutex > lock( diagnostics_mutex_ );
+  return latest_diagnostics_;
 }
 
 

cpp/ycm/ClangCompleter/TranslationUnit.h

@@ -56,7 +56,8 @@ public:
 
   bool IsCurrentlyUpdating() const;
 
-  void Reparse( const std::vector< UnsavedFile > &unsaved_files );
+  std::vector< Diagnostic > Reparse(
+      const std::vector< UnsavedFile > &unsaved_files );
 
   void ReparseForIndexing( const std::vector< UnsavedFile > &unsaved_files );
 

cpp/ycm/ycm_core.cpp

@@ -95,7 +95,6 @@ BOOST_PYTHON_MODULE(ycm_core)
     .def( vector_indexing_suite< std::vector< UnsavedFile > >() );
 
   class_< ClangCompleter, boost::noncopyable >( "ClangCompleter" )
-    .def( "DiagnosticsForFile", &ClangCompleter::DiagnosticsForFile )
     .def( "GetDeclarationLocation", &ClangCompleter::GetDeclarationLocation )
     .def( "GetDefinitionLocation", &ClangCompleter::GetDefinitionLocation )
     .def( "DeleteCachesForFile", &ClangCompleter::DeleteCachesForFile )

python/ycm/client/diagnostics_request.py

@@ -1,75 +0,0 @@
-#!/usr/bin/env python
-#
-# Copyright (C) 2013  Strahinja Val Markovic  <val@markovic.io>
-#
-# This file is part of YouCompleteMe.
-#
-# YouCompleteMe is free software: you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation, either version 3 of the License, or
-# (at your option) any later version.
-#
-# YouCompleteMe is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with YouCompleteMe.  If not, see <http://www.gnu.org/licenses/>.
-
-import traceback
-from ycm import vimsupport
-from ycm.client.base_request import ( BaseRequest, BuildRequestData,
-                                     JsonFromFuture )
-
-
-class DiagnosticsRequest( BaseRequest ):
-  def __init__( self ):
-    super( DiagnosticsRequest, self ).__init__()
-    self._cached_response = None
-
-
-  def Start( self ):
-    request_data = BuildRequestData()
-
-    try:
-      self._response_future = self.PostDataToHandlerAsync( request_data,
-                                                           'diagnostics' )
-    except:
-      vimsupport.EchoText( traceback.format_exc() )
-
-
-  def Done( self ):
-    return self._response_future.done()
-
-
-  def Response( self ):
-    if self._cached_response:
-      return self._cached_response
-
-    if not self._response_future:
-      return []
-    try:
-      self._cached_response = [ _ConvertDiagnosticDataToVimData( x )
-                                for x in JsonFromFuture(
-                                    self._response_future ) ]
-      return self._cached_response
-    except Exception as e:
-      vimsupport.PostVimMessage( str( e ) )
-      return []
-
-
-def _ConvertDiagnosticDataToVimData( diagnostic ):
-  # see :h getqflist for a description of the dictionary fields
-  # Note that, as usual, Vim is completely inconsistent about whether
-  # line/column numbers are 1 or 0 based in its various APIs. Here, it wants
-  # them to be 1-based.
-  return {
-    'bufnr' : vimsupport.GetBufferNumberForFilename( diagnostic[ 'filepath' ]),
-    'lnum'  : diagnostic[ 'line_num' ] + 1,
-    'col'   : diagnostic[ 'column_num' ] + 1,
-    'text'  : diagnostic[ 'text' ],
-    'type'  : diagnostic[ 'kind' ],
-    'valid' : 1
-  }
-

python/ycm/client/event_notification.py

@@ -17,14 +17,16 @@
 # You should have received a copy of the GNU General Public License
 # along with YouCompleteMe.  If not, see <http://www.gnu.org/licenses/>.
 
-from ycm.client.base_request import BaseRequest, BuildRequestData
-
+from ycm import vimsupport
+from ycm.client.base_request import ( BaseRequest, BuildRequestData,
+                                     JsonFromFuture )
 
 class EventNotification( BaseRequest ):
   def __init__( self, event_name, extra_data = None ):
     super( EventNotification, self ).__init__()
     self._event_name = event_name
     self._extra_data = extra_data
+    self._cached_response = None
 
 
   def Start( self ):
@@ -33,7 +35,44 @@ class EventNotification( BaseRequest ):
       request_data.update( self._extra_data )
     request_data[ 'event_name' ] = self._event_name
 
-    self.PostDataToHandlerAsync( request_data, 'event_notification' )
+    self._response_future = self.PostDataToHandlerAsync( request_data,
+                                                         'event_notification' )
+
+
+  def Done( self ):
+    return self._response_future.done()
+
+
+  def Response( self ):
+    if self._cached_response:
+      return self._cached_response
+
+    if not self._response_future or self._event_name != 'FileReadyToParse':
+      return []
+
+    try:
+      self._cached_response = [ _ConvertDiagnosticDataToVimData( x )
+                                for x in JsonFromFuture(
+                                    self._response_future ) ]
+      return self._cached_response
+    except Exception as e:
+      vimsupport.PostVimMessage( str( e ) )
+      return []
+
+
+def _ConvertDiagnosticDataToVimData( diagnostic ):
+  # see :h getqflist for a description of the dictionary fields
+  # Note that, as usual, Vim is completely inconsistent about whether
+  # line/column numbers are 1 or 0 based in its various APIs. Here, it wants
+  # them to be 1-based.
+  return {
+    'bufnr' : vimsupport.GetBufferNumberForFilename( diagnostic[ 'filepath' ]),
+    'lnum'  : diagnostic[ 'line_num' ] + 1,
+    'col'   : diagnostic[ 'column_num' ] + 1,
+    'text'  : diagnostic[ 'text' ],
+    'type'  : diagnostic[ 'kind' ],
+    'valid' : 1
+  }
 
 
 def SendEventNotificationAsync( event_name, extra_data = None ):

python/ycm/completers/cpp/clang_completer.py

@@ -201,11 +201,15 @@ class ClangCompleter( Completer ):
       self._logger.info( NO_COMPILE_FLAGS_MESSAGE )
       raise ValueError( NO_COMPILE_FLAGS_MESSAGE )
 
-    self._completer.UpdateTranslationUnit(
+    diagnostics = self._completer.UpdateTranslationUnit(
       ToUtf8IfNeeded( filename ),
       self.GetUnsavedFilesVector( request_data ),
       flags )
 
+    self._diagnostic_store = DiagnosticsToDiagStructure( diagnostics )
+    return [ ConvertToDiagnosticResponse( x ) for x in
+             diagnostics[ : self._max_diagnostics_to_display ] ]
+
 
   def OnBufferUnload( self, request_data ):
     self._completer.DeleteCachesForFile(
@@ -217,15 +221,6 @@ class ClangCompleter( Completer ):
         ToUtf8IfNeeded( request_data[ 'filepath' ] ) )
 
 
-  def GetDiagnosticsForCurrentFile( self, request_data ):
-    filename = request_data[ 'filepath' ]
-    diagnostics = self._completer.DiagnosticsForFile(
-        ToUtf8IfNeeded( filename ) )
-    self._diagnostic_store = DiagnosticsToDiagStructure( diagnostics )
-    return [ ConvertToDiagnosticResponse( x ) for x in
-             diagnostics[ : self._max_diagnostics_to_display ] ]
-
-
   def GetDetailedDiagnostic( self, request_data ):
     current_line = request_data[ 'line_num' ] + 1
     current_column = request_data[ 'column_num' ] + 1

python/ycm/server/default_settings.json

@@ -1 +1 @@
-{
  "filepath_completion_use_working_dir": 0,
  "min_num_of_chars_for_completion": 2,
  "semantic_triggers": {},
  "collect_identifiers_from_comments_and_strings": 0,
  "filetype_specific_completion_to_disable": {
    "gitcommit": 1,
  },
  "collect_identifiers_from_tags_files": 0,
  "extra_conf_globlist": [],
  "global_ycm_extra_conf": "",
  "confirm_extra_conf": 1,
  "complete_in_comments": 0,
  "complete_in_strings": 1,
  "min_num_identifier_candidate_chars": 0,
  "max_diagnostics_to_display": 30,
  "auto_stop_csharp_server": 1,
  "seed_identifiers_with_syntax": 0,
  "csharp_server_port": 2000,
  "filetype_whitelist": {
    "*": "1"
  },
  "auto_start_csharp_server": 1,
  "filetype_blacklist": {
    "tagbar": "1",
    "qf": "1",
    "notes": "1",
    "markdown": "1",
    "unite": "1",
    "text": "1"
  }
}
+{
  "filepath_completion_use_working_dir": 0,
  "min_num_of_chars_for_completion": 2,
  "semantic_triggers": {},
  "collect_identifiers_from_comments_and_strings": 0,
  "filetype_specific_completion_to_disable": {
    "gitcommit": 1
  },
  "collect_identifiers_from_tags_files": 0,
  "extra_conf_globlist": [],
  "global_ycm_extra_conf": "",
  "confirm_extra_conf": 1,
  "complete_in_comments": 0,
  "complete_in_strings": 1,
  "min_num_identifier_candidate_chars": 0,
  "max_diagnostics_to_display": 30,
  "auto_stop_csharp_server": 1,
  "seed_identifiers_with_syntax": 0,
  "csharp_server_port": 2000,
  "filetype_whitelist": {
    "*": "1"
  },
  "auto_start_csharp_server": 1,
  "filetype_blacklist": {
    "tagbar": "1",
    "qf": "1",
    "notes": "1",
    "markdown": "1",
    "unite": "1",
    "text": "1"
  }
}

python/ycm/server/tests/basic_test.py

@@ -283,7 +283,7 @@ def DefinedSubcommands_WorksWhenNoExplicitCompleterTargetSpecified_test():
 
 
 @with_setup( Setup )
-def GetDiagnostics_ClangCompleter_ZeroBasedLineAndColumn_test():
+def Diagnostics_ClangCompleter_ZeroBasedLineAndColumn_test():
   app = TestApp( ycmd.app )
   contents = """
 struct Foo {
@@ -314,8 +314,7 @@ struct Foo {
     'event_name': 'FileReadyToParse',
   } )
 
-  app.post_json( '/event_notification', event_data )
-  results = app.post_json( '/diagnostics', diag_data ).json
+  results = app.post_json( '/event_notification', event_data ).json
   assert_that( results,
                contains(
                   has_entries( { 'text': contains_string( "expected ';'" ),

python/ycm/server/ycmd.py

@@ -63,10 +63,15 @@ def EventNotification():
   getattr( SERVER_STATE.GetGeneralCompleter(), event_handler )( request_data )
 
   filetypes = request_data[ 'filetypes' ]
+  response_data = None
   if SERVER_STATE.FiletypeCompletionUsable( filetypes ):
-    getattr( SERVER_STATE.GetFiletypeCompleter( filetypes ),
+    response_data = getattr( SERVER_STATE.GetFiletypeCompleter( filetypes ),
                              event_handler )( request_data )
 
+  if response_data:
+    return _JsonResponse( response_data )
+
+
 
 @app.post( '/run_completer_command' )
 def RunCompleterCommand():
@@ -94,16 +99,6 @@ def GetCompletions():
   return _JsonResponse( completer.ComputeCandidates( request_data ) )
 
 
-@app.post( '/diagnostics' )
-def GetDiagnostics():
-  LOGGER.info( 'Received diagnostics request')
-  request_data = request.json
-  completer = _GetCompleterForRequestData( request_data )
-
-  return _JsonResponse( completer.GetDiagnosticsForCurrentFile(
-      request_data ) )
-
-
 @app.get( '/user_options' )
 def GetUserOptions():
   LOGGER.info( 'Received user options GET request')

python/ycm/youcompleteme.py

@@ -29,8 +29,8 @@ from ycm.completers.general import syntax_parse
 from ycm.client.base_request import BaseRequest, BuildRequestData
 from ycm.client.command_request import SendCommandRequest
 from ycm.client.completion_request import CompletionRequest
-from ycm.client.diagnostics_request import DiagnosticsRequest
-from ycm.client.event_notification import SendEventNotificationAsync
+from ycm.client.event_notification import ( SendEventNotificationAsync,
+                                            EventNotification )
 
 try:
   from UltiSnips import UltiSnips_Manager
@@ -45,7 +45,7 @@ class YouCompleteMe( object ):
     self._user_options = user_options
     self._omnicomp = OmniCompleter( user_options )
     self._latest_completion_request = None
-    self._latest_diagnostics_request = None
+    self._latest_file_parse_request = None
     self._server_stdout = None
     self._server_stderr = None
     self._server_popen = None
@@ -147,7 +147,9 @@ class YouCompleteMe( object ):
     if self._user_options[ 'seed_identifiers_with_syntax' ]:
       self._AddSyntaxDataIfNeeded( extra_data )
 
-    SendEventNotificationAsync( 'FileReadyToParse', extra_data )
+    self._latest_file_parse_request = EventNotification( 'FileReadyToParse',
+                                                          extra_data )
+    self._latest_file_parse_request.Start()
 
 
   def OnBufferUnload( self, deleted_buffer_file ):
@@ -176,23 +178,18 @@ class YouCompleteMe( object ):
 
 
   def DiagnosticsForCurrentFileReady( self ):
-    return bool( self._latest_diagnostics_request and
-                 self._latest_diagnostics_request.Done() )
-
-
-  def RequestDiagnosticsForCurrentFile( self ):
-    self._latest_diagnostics_request = DiagnosticsRequest()
-    self._latest_diagnostics_request.Start()
+    return bool( self._latest_file_parse_request and
+                 self._latest_file_parse_request.Done() )
 
 
   def GetDiagnosticsFromStoredRequest( self ):
-    if self._latest_diagnostics_request:
-      to_return = self._latest_diagnostics_request.Response()
+    if self._latest_file_parse_request:
+      to_return = self._latest_file_parse_request.Response()
       # We set the diagnostics request to None because we want to prevent
       # Syntastic from repeatedly refreshing the buffer with the same diags.
       # Setting this to None makes DiagnosticsForCurrentFileReady return False
       # until the next request is created.
-      self._latest_diagnostics_request = None
+      self._latest_file_parse_request = None
       return to_return
     return []