Ensure didChange notification is never sent after didClose (#2438)

* Ensure didChange is never sent after didClose

This fixes for example the Pyright warning
LSP-pyright: Received change text document command for closed file <URI>
when a file is saved and closed immediately after changes were applied.

* Missed something

* Add test

* Maybe like this?

* Try something else

* Simplify expression to save one unnecessary API call

view.change_count() returns 0 if the view isn't valid anymore (closed),
so we can simply use short-circuit evaluation for this and don't need
the is_valid() API call.

* Exempt Linux

* Small tweak to save an API call

* Revert "Exempt Linux"

This reverts commit 4dd2e91.

* Fix failing test on Linux

* actually this test passes locally with this line uncommented

* Revert, apparently it fails on the CI...

This reverts commit 43ede82.

* try a slightly different approach just to see... test pass locally

* Revert "try a slightly different approach just to see... test pass locally"

the test still fail on the CI

This reverts commit 11c5ecb.

---------

Co-authored-by: Предраг Николић <[email protected]>

Author: jwortmann

plugin/session_buffer.py

@@ -166,8 +166,9 @@ def _check_did_open(self, view: sublime.View) -> None:
                 self._do_document_link_async(view, version)
             self.session.notify_plugin_on_session_buffer_change(self)
 
-    def _check_did_close(self) -> None:
+    def _check_did_close(self, view: sublime.View) -> None:
         if self.opened and self.should_notify_did_close():
+            self.purge_changes_async(view, suppress_requests=True)
             self.session.send_notification(did_close(uri=self._last_known_uri))
             self.opened = False
 
@@ -202,9 +203,9 @@ def remove_session_view(self, sv: SessionViewProtocol) -> None:
         self._clear_semantic_token_regions(sv.view)
         self.session_views.remove(sv)
         if len(self.session_views) == 0:
-            self._on_before_destroy()
+            self._on_before_destroy(sv.view)
 
-    def _on_before_destroy(self) -> None:
+    def _on_before_destroy(self, view: sublime.View) -> None:
         self.remove_all_inlay_hints()
         if self.has_capability("diagnosticProvider") and self.session.config.diagnostics_mode == "open_files":
             self.session.m_textDocument_publishDiagnostics({'uri': self._last_known_uri, 'diagnostics': []})
@@ -216,7 +217,7 @@ def _on_before_destroy(self) -> None:
         # in unregistering ourselves from the session.
         if not self.session.exiting:
             # Only send textDocument/didClose when we are the only view left (i.e. there are no other clones).
-            self._check_did_close()
+            self._check_did_close(view)
             self.session.unregister_session_buffer_async(self)
 
     def register_capability_async(
@@ -308,15 +309,15 @@ def on_revert_async(self, view: sublime.View) -> None:
 
     on_reload_async = on_revert_async
 
-    def purge_changes_async(self, view: sublime.View) -> None:
+    def purge_changes_async(self, view: sublime.View, suppress_requests: bool = False) -> None:
         if self._pending_changes is None:
             return
         sync_kind = self.text_sync_kind()
         if sync_kind == TextDocumentSyncKind.None_:
             return
         if sync_kind == TextDocumentSyncKind.Full:
             changes = None
-            version = view.change_count()
+            version = view.change_count() or self._pending_changes.version
         else:
             changes = self._pending_changes.changes
             version = self._pending_changes.version
@@ -329,23 +330,28 @@ def purge_changes_async(self, view: sublime.View) -> None:
         finally:
             self._pending_changes = None
         self.session.notify_plugin_on_session_buffer_change(self)
-        sublime.set_timeout_async(lambda: self._on_after_change_async(view, version))
+        sublime.set_timeout_async(lambda: self._on_after_change_async(view, version, suppress_requests))
 
-    def _on_after_change_async(self, view: sublime.View, version: int) -> None:
+    def _on_after_change_async(self, view: sublime.View, version: int, suppress_requests: bool = False) -> None:
         if self._is_saving:
             self._has_changed_during_save = True
             return
-        self._do_color_boxes_async(view, version)
-        self.do_document_diagnostic_async(view, version)
-        if self.session.config.diagnostics_mode == "workspace" and \
-                not self.session.workspace_diagnostics_pending_response and \
-                self.session.has_capability('diagnosticProvider.workspaceDiagnostics'):
-            self._workspace_diagnostics_debouncer_async.debounce(
-                self.session.do_workspace_diagnostics_async, timeout_ms=WORKSPACE_DIAGNOSTICS_TIMEOUT)
-        self.do_semantic_tokens_async(view)
-        if userprefs().link_highlight_style in ("underline", "none"):
-            self._do_document_link_async(view, version)
-        self.do_inlay_hints_async(view)
+        if suppress_requests:
+            return
+        try:
+            self._do_color_boxes_async(view, version)
+            self.do_document_diagnostic_async(view, version)
+            if self.session.config.diagnostics_mode == "workspace" and \
+                    not self.session.workspace_diagnostics_pending_response and \
+                    self.session.has_capability('diagnosticProvider.workspaceDiagnostics'):
+                self._workspace_diagnostics_debouncer_async.debounce(
+                    self.session.do_workspace_diagnostics_async, timeout_ms=WORKSPACE_DIAGNOSTICS_TIMEOUT)
+            self.do_semantic_tokens_async(view)
+            if userprefs().link_highlight_style in ("underline", "none"):
+                self._do_document_link_async(view, version)
+            self.do_inlay_hints_async(view)
+        except MissingUriError:
+            pass
 
     def on_pre_save_async(self, view: sublime.View) -> None:
         self._is_saving = True
@@ -357,7 +363,7 @@ def on_pre_save_async(self, view: sublime.View) -> None:
     def on_post_save_async(self, view: sublime.View, new_uri: DocumentUri) -> None:
         self._is_saving = False
         if new_uri != self._last_known_uri:
-            self._check_did_close()
+            self._check_did_close(view)
             self._last_known_uri = new_uri
             self._check_did_open(view)
         else:

tests/test_single_document.py

@@ -84,31 +84,6 @@ def test_did_close(self) -> 'Generator':
         self.view.close()
         yield from self.await_message("textDocument/didClose")
 
-    def test_did_change(self) -> 'Generator':
-        assert self.view
-        self.maxDiff = None
-        self.insert_characters("A")
-        yield from self.await_message("textDocument/didChange")
-        # multiple changes are batched into one didChange notification
-        self.insert_characters("B\n")
-        self.insert_characters("🙂\n")
-        self.insert_characters("D")
-        promise = YieldPromise()
-        yield from self.await_message("textDocument/didChange", promise)
-        self.assertEqual(promise.result(), {
-            'contentChanges': [
-                {'rangeLength': 0, 'range': {'start': {'line': 0, 'character': 1}, 'end': {'line': 0, 'character': 1}}, 'text': 'B'},   # noqa
-                {'rangeLength': 0, 'range': {'start': {'line': 0, 'character': 2}, 'end': {'line': 0, 'character': 2}}, 'text': '\n'},  # noqa
-                {'rangeLength': 0, 'range': {'start': {'line': 1, 'character': 0}, 'end': {'line': 1, 'character': 0}}, 'text': '🙂'},  # noqa
-                # Note that this is character offset (2) is correct (UTF-16).
-                {'rangeLength': 0, 'range': {'start': {'line': 1, 'character': 2}, 'end': {'line': 1, 'character': 2}}, 'text': '\n'},  # noqa
-                {'rangeLength': 0, 'range': {'start': {'line': 2, 'character': 0}, 'end': {'line': 2, 'character': 0}}, 'text': 'D'}],  # noqa
-            'textDocument': {
-                'version': self.view.change_count(),
-                'uri': filename_to_uri(TEST_FILE_PATH)
-            }
-        })
-
     def test_sends_save_with_purge(self) -> 'Generator':
         assert self.view
         self.view.settings().set("lsp_format_on_save", False)
@@ -371,6 +346,54 @@ def test_progress(self) -> 'Generator':
         self.assertEqual(result, {"general": "kenobi"})
 
 
+class SingleDocumentTestCase2(TextDocumentTestCase):
+
+    def test_did_change(self) -> 'Generator':
+        assert self.view
+        self.maxDiff = None
+        self.insert_characters("A")
+        yield from self.await_message("textDocument/didChange")
+        # multiple changes are batched into one didChange notification
+        self.insert_characters("B\n")
+        self.insert_characters("🙂\n")
+        self.insert_characters("D")
+        promise = YieldPromise()
+        yield from self.await_message("textDocument/didChange", promise)
+        self.assertEqual(promise.result(), {
+            'contentChanges': [
+                {'rangeLength': 0, 'range': {'start': {'line': 0, 'character': 1}, 'end': {'line': 0, 'character': 1}}, 'text': 'B'},   # noqa
+                {'rangeLength': 0, 'range': {'start': {'line': 0, 'character': 2}, 'end': {'line': 0, 'character': 2}}, 'text': '\n'},  # noqa
+                {'rangeLength': 0, 'range': {'start': {'line': 1, 'character': 0}, 'end': {'line': 1, 'character': 0}}, 'text': '🙂'},  # noqa
+                # Note that this is character offset (2) is correct (UTF-16).
+                {'rangeLength': 0, 'range': {'start': {'line': 1, 'character': 2}, 'end': {'line': 1, 'character': 2}}, 'text': '\n'},  # noqa
+                {'rangeLength': 0, 'range': {'start': {'line': 2, 'character': 0}, 'end': {'line': 2, 'character': 0}}, 'text': 'D'}],  # noqa
+            'textDocument': {
+                'version': self.view.change_count(),
+                'uri': filename_to_uri(TEST_FILE_PATH)
+            }
+        })
+
+
+class SingleDocumentTestCase3(TextDocumentTestCase):
+
+    @classmethod
+    def get_test_name(cls) -> str:
+        return "testfile2"
+
+    def test_did_change_before_did_close(self) -> 'Generator':
+        assert self.view
+        self.view.window().run_command("chain", {
+            "commands": [
+                ["insert", {"characters": "TEST"}],
+                ["save", {"async": False}],
+                ["close", {}]
+            ]
+        })
+        yield from self.await_message('textDocument/didChange')
+        # yield from self.await_message('textDocument/didSave')  # TODO why is this not sent?
+        yield from self.await_message('textDocument/didClose')
+
+
 class WillSaveWaitUntilTestCase(TextDocumentTestCase):
 
     @classmethod