From c274b7d4f80b0d018819aa72c401391dbef71995 Mon Sep 17 00:00:00 2001 From: Strahinja Val Markovic Date: Thu, 3 Oct 2013 13:15:43 -0700 Subject: [PATCH] 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 | 15 ++-- cpp/ycm/ClangCompleter/ClangCompleter.cpp | 20 ++---- cpp/ycm/ClangCompleter/ClangCompleter.h | 4 +- cpp/ycm/ClangCompleter/TranslationUnit.cpp | 5 +- cpp/ycm/ClangCompleter/TranslationUnit.h | 3 +- cpp/ycm/ycm_core.cpp | 1 - python/ycm/client/diagnostics_request.py | 75 -------------------- python/ycm/client/event_notification.py | 45 +++++++++++- python/ycm/completers/cpp/clang_completer.py | 15 ++-- python/ycm/server/default_settings.json | 2 +- python/ycm/server/tests/basic_test.py | 5 +- python/ycm/server/ycmd.py | 19 ++--- python/ycm/youcompleteme.py | 25 +++---- 13 files changed, 89 insertions(+), 145 deletions(-) delete mode 100644 python/ycm/client/diagnostics_request.py diff --git a/autoload/youcompleteme.vim b/autoload/youcompleteme.vim index 0c103d5c..f50952d4 100644 --- a/autoload/youcompleteme.vim +++ b/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 diff --git a/cpp/ycm/ClangCompleter/ClangCompleter.cpp b/cpp/ycm/ClangCompleter/ClangCompleter.cpp index c7239c44..a8497293 100644 --- a/cpp/ycm/ClangCompleter/ClangCompleter.cpp +++ b/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 >(); } diff --git a/cpp/ycm/ClangCompleter/ClangCompleter.h b/cpp/ycm/ClangCompleter/ClangCompleter.h index 70fb3252..7f89019e 100644 --- a/cpp/ycm/ClangCompleter/ClangCompleter.h +++ b/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 ); diff --git a/cpp/ycm/ClangCompleter/TranslationUnit.cpp b/cpp/ycm/ClangCompleter/TranslationUnit.cpp index efaaf5ff..9638c2df 100644 --- a/cpp/ycm/ClangCompleter/TranslationUnit.cpp +++ b/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_; } diff --git a/cpp/ycm/ClangCompleter/TranslationUnit.h b/cpp/ycm/ClangCompleter/TranslationUnit.h index 16110b40..d1469805 100644 --- a/cpp/ycm/ClangCompleter/TranslationUnit.h +++ b/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 ); diff --git a/cpp/ycm/ycm_core.cpp b/cpp/ycm/ycm_core.cpp index 80626d63..ba348bcc 100644 --- a/cpp/ycm/ycm_core.cpp +++ b/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 ) diff --git a/python/ycm/client/diagnostics_request.py b/python/ycm/client/diagnostics_request.py deleted file mode 100644 index 505f927c..00000000 --- a/python/ycm/client/diagnostics_request.py +++ /dev/null @@ -1,75 +0,0 @@ -#!/usr/bin/env python -# -# Copyright (C) 2013 Strahinja Val Markovic -# -# 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 . - -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 - } - diff --git a/python/ycm/client/event_notification.py b/python/ycm/client/event_notification.py index c39a4f43..2121dbfa 100644 --- a/python/ycm/client/event_notification.py +++ b/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 . -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 ): diff --git a/python/ycm/completers/cpp/clang_completer.py b/python/ycm/completers/cpp/clang_completer.py index e2f48718..a8e5e676 100644 --- a/python/ycm/completers/cpp/clang_completer.py +++ b/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 diff --git a/python/ycm/server/default_settings.json b/python/ycm/server/default_settings.json index 8dc9005e..1f55dd30 100644 --- a/python/ycm/server/default_settings.json +++ b/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" } } \ No newline at end of file +{ "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" } } \ No newline at end of file diff --git a/python/ycm/server/tests/basic_test.py b/python/ycm/server/tests/basic_test.py index 4f71da82..5ff6a218 100644 --- a/python/ycm/server/tests/basic_test.py +++ b/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 ';'" ), diff --git a/python/ycm/server/ycmd.py b/python/ycm/server/ycmd.py index ef467058..8183cb89 100755 --- a/python/ycm/server/ycmd.py +++ b/python/ycm/server/ycmd.py @@ -63,9 +63,14 @@ 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 ), - event_handler )( request_data ) + response_data = getattr( SERVER_STATE.GetFiletypeCompleter( filetypes ), + event_handler )( request_data ) + + if response_data: + return _JsonResponse( response_data ) + @app.post( '/run_completer_command' ) @@ -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') diff --git a/python/ycm/youcompleteme.py b/python/ycm/youcompleteme.py index 9741e741..2f691f72 100644 --- a/python/ycm/youcompleteme.py +++ b/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 []