fix: code actions on save not fixing issues if saving quickly #2540
Conversation
| def do_document_diagnostic_async(self, view: sublime.View, version: int | None = None) -> None: | ||
| mgr = self.session.manager() | ||
| if not mgr: | ||
| if not mgr or not self.has_capability("diagnosticProvider"): | ||
| return | ||
| if mgr.should_ignore_diagnostics(self._last_known_uri, self.session.config): | ||
| return | ||
| if version is None: | ||
| version = view.change_count() | ||
| if self.has_capability("diagnosticProvider"): | ||
| if self._document_diagnostic_pending_response: | ||
| self.session.cancel_request(self._document_diagnostic_pending_response) | ||
| params: DocumentDiagnosticParams = {'textDocument': text_document_identifier(view)} | ||
| identifier = self.get_capability("diagnosticProvider.identifier") | ||
| if identifier: | ||
| params['identifier'] = identifier | ||
| result_id = self.session.diagnostics_result_ids.get(self._last_known_uri) | ||
| if result_id is not None: | ||
| params['previousResultId'] = result_id | ||
| self._document_diagnostic_pending_response = self.session.send_request_async( | ||
| Request.documentDiagnostic(params, view), | ||
| partial(self._on_document_diagnostic_async, version), | ||
| partial(self._on_document_diagnostic_error_async, version) | ||
| ) | ||
| if self._document_diagnostic_pending_request: | ||
| if self._document_diagnostic_pending_request.version == version: | ||
| return | ||
| self.session.cancel_request(self._document_diagnostic_pending_request.request_id) | ||
| params: DocumentDiagnosticParams = {'textDocument': text_document_identifier(view)} | ||
| identifier = self.get_capability("diagnosticProvider.identifier") | ||
| if identifier: | ||
| params['identifier'] = identifier | ||
| result_id = self.session.diagnostics_result_ids.get(self._last_known_uri) | ||
| if result_id is not None: | ||
| params['previousResultId'] = result_id | ||
| request_id = self.session.send_request_async( | ||
| Request.documentDiagnostic(params, view), | ||
| partial(self._on_document_diagnostic_async, version), | ||
| partial(self._on_document_diagnostic_error_async, version) | ||
| ) | ||
| self._document_diagnostic_pending_request = PendingDocumentDiagnosticRequest(version, request_id) | ||
|
|
There was a problem hiding this comment.
Moved check for self.has_capability("diagnosticProvider") to the top which changed indentation quite a bit and made it harder to review. Enable "hide whitespace changes" to make it easier to read.
Apart from that, added a check and bail for the case when requesting diagnostics for document version for which there already is a pending request. Without the added check it would cancel existing and trigger a new one which is a waste if the document hasn't changed since.
The code looks ok, but I cannot test it. |
|
Might help to run multiple servers and/or test on vue files that are often slower to lint. |
|
I guess it depends on how fast the diagnostics arrive. I get them almost instant even on the main branch. Looking at this code, this will make sure to pull diagnostics(only applicable to servers that support the pull model) before requesting code actions. |
|
The issue is that LSP didn't even request diagnostics before triggering code actions on save. It's not important that the diagnostics response is received, only that the request is sent. (Of course it's also necessary to enable code action on save which I forgot to mention - |
|
yes I had code actions on save already enabled. Oh, I knew that the LSP spec allows to send diagnostics to in the CodeActionContext. I thought the we implemented to send diagnostics when requesting code actions.... I haven't seen that code in a while, and that topic is a bit blurry. |
|
It's not relevant for this issue which diagnostics are included in code actions request. Only that we trigger the diagnostic request itself. |
There was a problem hiding this comment.
I tried to test the bug, but for me LSP-eslint doesn't report any diagnostics at all. Perhaps I'm missing some required configuration file?
I guess to make it easier to reproduce, you could temporarily increase the FEATURES_TIMEOUT debounce time in the code.
The code mostly looks good to me (besides see the comment below, maybe).
plugin/code_actions.py
Outdated
| # Pull for diagnostics to ensure that server computes them before receiving code action request. | ||
| sb.do_document_diagnostic_async(view) |
There was a problem hiding this comment.
I probably would have put this part under if request: in _collect_code_actions_async (line 165), because it's not strictly necessary for creating the Request object.
There was a problem hiding this comment.
makes sense
Ideally perhaps this should be trigger before any code action request. While normal usage should not require that, if someone uses a macro or some sort of automation then it could become a problem then too.
There was a problem hiding this comment.
Actually this code is used for any code action request so all good.
|
Can try on https://github.com/rchl/lsp-log-parser - clone the project and run |
Yes, increasing the timeout helped to test this out. Tested, works ✅ |
Fix issue with code actions on save not fixing diagnostic errors that were just introduced, when saving immediately after changing the document.
For servers that support the pull diagnostics model we have to assume that the server doesn't re-compute diagnostics until client asks for them. Thus we have to ask just before sending code actions request or otherwise it might not provide fixes for issues that were just introduced.
To test, with a LSP-eslint running on a file, create an issue (like a trailing space) and save the file immediately. The issue is often not fixed on the first try.
Fixes #2511