Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Implement diagnostics pull model #18404

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Oct 24, 2024

Fixes #18375

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 24, 2024
@Veykril Veykril force-pushed the veykril/push-swpmkoqqxrvu branch from 0665d41 to bf60937 Compare October 24, 2024 14:41
@Veykril
Copy link
Member Author

Veykril commented Oct 24, 2024

Okay implementing this is rather straight forward actually, that's nice

@Veykril Veykril marked this pull request as ready for review October 24, 2024 14:42
@Veykril Veykril enabled auto-merge October 24, 2024 14:45
@Veykril Veykril added this pull request to the merge queue Oct 24, 2024
@Veykril Veykril removed this pull request from the merge queue due to a manual request Oct 24, 2024
@Veykril Veykril force-pushed the veykril/push-swpmkoqqxrvu branch from bf60937 to 8b59541 Compare October 24, 2024 15:09
@Veykril Veykril enabled auto-merge October 24, 2024 15:09
@Veykril Veykril added this pull request to the merge queue Oct 24, 2024
Merged via the queue into rust-lang:master with commit 58e9871 Oct 24, 2024
9 checks passed
@Veykril Veykril deleted the veykril/push-swpmkoqqxrvu branch October 24, 2024 15:36
@rchl
Copy link

rchl commented Dec 6, 2024

I know this should be a separate issue but for now I don't have time for that so just gonna point at sublimelsp/LSP#2572

Basically, there appears to be some issues with the implementation - the server still pushes diagnostics even if client supports pull diagnostics and also it returns empty diagnostics for pull request.

@Veykril
Copy link
Member Author

Veykril commented Dec 7, 2024

Basically, there appears to be some issues with the implementation - the server still pushes diagnostics even if client supports pull diagnostics

This is intentional, the native rust-analyzer diagnostics support the push model, but the on save document handler that invokes cargo check remains using the push model as pulling makes little sense in that way (cargo is inherently pushing). The LSP does not forbid (to my knowledge) both to be used.

@jwortmann
Copy link

According to the comments in microsoft/language-server-protocol#1743 (comment) servers should not use both pull and push diagnostics at the same time (at least for a particular DocumentUri; what you are allowed to do is to register pull diagnostics only for certain URIs and then use push diagnostics for different URIs):

If a server opts into providing pull then the client pulls for diagnostics and the server doesn't need to push.

So I opt to simply add to the spec that a server if opting into pull it shouldn't push diagnostics for the same set of resources.

(though I think it was forgotten to add this last sentence to the specs explicitly)


Furthermore, since you don't seem to register an identifier for the pull diagnostics (as far as I can tell from the recording provided in sublimelsp/LSP#2572), and push diagnostics per definition don't have any identifier, it would mean that they just override each other for the same URI, i.e. the latest arrived wins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the LSP diagnostics pull model
5 participants