From 19264ce3c1de7ba424031ca61ab2058d989653bf Mon Sep 17 00:00:00 2001 From: Ben Jackson Date: Fri, 15 Dec 2023 21:07:53 +0000 Subject: [PATCH 1/3] Only parse the initial file the first time it's opened When the language_server_completer is first constructed, it's usually a FileReadyToParse callback. But at that time, we can't actually parse anything because we haven't done the initialization exchange with the server, so we queue this up for later. The way this was implemenented was to register a "on file ready to parse" callback to update the server with the file contents at the time the completer was constructed. In practice, as the completer is usually constructed via a file ready to pase event, this actually happens immediately and we then shunt the request into "OnInitializeComplete" handlers, which are fired when the initialize exchange happens. Why don't we just put something directly in OnInitializeComplete handlers? Well because in the constructor, we don't have the request_data - we only get that in the OnFileReadyToParse callback, so we have this jank. WCGW? As a result of this dance, we actually introduced a subtle error. OnFileReadyToParse handlers are called _every time_ we have a file parse event, and - we never _remove_ this "parse the file contents as they were at the time of initialization" request from the "file ready to parse handlers" list. So every time we has a file parse event, we update the server with _stale_ data, then immediately correct it! In practice, this isn't all that noticable, but it is braindead. Simple solution is to add a "once" flag to the handler creating oneshot handlers, and clear such handlers after firing them (or converting them to initialize handlers). Jank to fix jank. It might be better to find a way to just remove these handler "lists" altogether, as there is only one other place in the codebase where one is registered, and we could always just pass request_data to constructor if we have it... --- .../language_server_completer.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/ycmd/completers/language_server/language_server_completer.py b/ycmd/completers/language_server/language_server_completer.py index a2796e81ce..9f501d825e 100644 --- a/ycmd/completers/language_server/language_server_completer.py +++ b/ycmd/completers/language_server/language_server_completer.py @@ -1005,7 +1005,8 @@ def __init__( self, user_options, connection_type = 'stdio' ): self._on_file_ready_to_parse_handlers = [] self.RegisterOnFileReadyToParse( lambda self, request_data: - self._UpdateServerWithFileContents( request_data ) + self._UpdateServerWithFileContents( request_data ), + True # once ) self._signature_help_disabled = user_options[ 'disable_signature_help' ] @@ -1898,6 +1899,12 @@ def OnFileReadyToParse( self, request_data ): if not self.ServerIsHealthy(): return + def ClearOneshotHandlers(): + self._on_file_ready_to_parse_handlers = [ + ( handler, once ) for handler, once + in self._on_file_ready_to_parse_handlers if not once + ] + # If we haven't finished initializing yet, we need to queue up all functions # registered on the FileReadyToParse event and in particular # _UpdateServerWithFileContents in reverse order of registration. This @@ -1905,13 +1912,15 @@ def OnFileReadyToParse( self, request_data ): # messages. This is important because server start up can be quite slow and # we must not block the user, while we must keep the server synchronized. if not self._initialize_event.is_set(): - for handler in reversed( self._on_file_ready_to_parse_handlers ): + for handler, _ in reversed( self._on_file_ready_to_parse_handlers ): self._OnInitializeComplete( partial( handler, request_data = request_data ) ) + ClearOneshotHandlers() return - for handler in reversed( self._on_file_ready_to_parse_handlers ): + for handler, _ in reversed( self._on_file_ready_to_parse_handlers ): handler( self, request_data ) + ClearOneshotHandlers() # Return the latest diagnostics that we have received. # @@ -2480,8 +2489,8 @@ def _OnInitializeComplete( self, handler ): self._on_initialize_complete_handlers.append( handler ) - def RegisterOnFileReadyToParse( self, handler ): - self._on_file_ready_to_parse_handlers.append( handler ) + def RegisterOnFileReadyToParse( self, handler, once=False ): + self._on_file_ready_to_parse_handlers.append( ( handler, once ) ) def GetHoverResponse( self, request_data ): From c278c9a30f1d0fa84043102843fa23fa019a5441 Mon Sep 17 00:00:00 2001 From: Ben Jackson Date: Fri, 22 Dec 2023 12:39:50 +0000 Subject: [PATCH 2/3] Fix tests not including file contents on initialisation The previous commit broke a _ton_ of tests. It turns out that the tests were relying on the bug that it fixed. Previously our "StartXServerInDirectory" utils were using a FileReadyToParse to initialize the server, but without including the actual contents. We were getting away with this because the previous bug meant that we tried to refresh the file on every subsequent FileReadyToParse, but as its contents were not included, we read the proper contents from the disk, or closed it. Now the bug is fixed, we no longer have redundant refreshes of the "first" file opened, so all the tests were broken, having the "first" file in the sever with empty contents. Solution is to include the actual contents when starting the completer server. In fact, given that it seems intuitive, we now just read file contents in BuildRequest directly when not otherwise supplied. There's no point trying that if a file path wasn't supplied though. We _could_ remove a lot of manual ReadFile calls elsewhere, but that's beyond the scope of this commit. --- ycmd/tests/rust/__init__.py | 9 +++++---- ycmd/tests/test_utils.py | 16 +++++++++++++++- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/ycmd/tests/rust/__init__.py b/ycmd/tests/rust/__init__.py index 4c604f7837..16df6fed84 100644 --- a/ycmd/tests/rust/__init__.py +++ b/ycmd/tests/rust/__init__.py @@ -44,10 +44,11 @@ def tearDownModule(): def StartRustCompleterServerInDirectory( app, directory ): app.post_json( '/event_notification', - BuildRequest( - filepath = os.path.join( directory, 'src', 'main.rs' ), - event_name = 'FileReadyToParse', - filetype = 'rust' ) ) + BuildRequest( filepath = os.path.join( directory, + 'src', + 'main.rs' ), + event_name = 'FileReadyToParse', + filetype = 'rust' ) ) WaitUntilCompleterServerReady( app, 'rust' ) diff --git a/ycmd/tests/test_utils.py b/ycmd/tests/test_utils.py index e7b725db4a..55b091b5d2 100644 --- a/ycmd/tests/test_utils.py +++ b/ycmd/tests/test_utils.py @@ -16,6 +16,7 @@ # along with ycmd. If not, see . +from ycmd.utils import ReadFile from hamcrest import ( assert_that, contains_exactly, contains_string, @@ -76,9 +77,22 @@ } ) +def GetTestFileContents( filename ): + try: + return ReadFile( filename ) + except IOError: + return '' + + def BuildRequest( **kwargs ): filepath = kwargs[ 'filepath' ] if 'filepath' in kwargs else '/foo' - contents = kwargs[ 'contents' ] if 'contents' in kwargs else '' + contents = ( + kwargs[ 'contents' ] + if 'contents' in kwargs + else GetTestFileContents( filepath ) + if 'filepath' in kwargs + else '' + ) filetype = kwargs[ 'filetype' ] if 'filetype' in kwargs else 'foo' filetypes = kwargs[ 'filetypes' ] if 'filetypes' in kwargs else [ filetype ] From 32041794cdbf9588e09b2c729903b93daae036f0 Mon Sep 17 00:00:00 2001 From: Ben Jackson Date: Fri, 22 Dec 2023 17:36:57 +0000 Subject: [PATCH 3/3] Re-instate actually updating the server on file ready to parse Perversely, in LSP completer, OnFileReadyToParse never parsed anything. At least, not in the obvious way. The original bug where we were doing redundant updates, ironically, was ensuring that we (eventually) updated the server with file contents (at some point in time) when we get OnFileReadyToParse. But by fixing that, we now never actually used the data supplied in OnFileReadyToParse, which the tests were relying heavily on. In particular, the Java tests would update file contents using only OnFileReadyToParse, which is a completely legitimate thing to do in our API. And worked. Until it didn't. Now it does again - we explicitly update server file state in OnFileReadyToParse, which seems perfectly sensible. --- ycmd/completers/language_server/language_server_completer.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ycmd/completers/language_server/language_server_completer.py b/ycmd/completers/language_server/language_server_completer.py index 9f501d825e..b5fd6ee182 100644 --- a/ycmd/completers/language_server/language_server_completer.py +++ b/ycmd/completers/language_server/language_server_completer.py @@ -1922,6 +1922,8 @@ def ClearOneshotHandlers(): handler( self, request_data ) ClearOneshotHandlers() + self._UpdateServerWithFileContents( request_data ) + # Return the latest diagnostics that we have received. # # NOTE: We also return diagnostics asynchronously via the long-polling