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(language_server)!: add capability diagnosticProvider #7444

Conversation

Sysix
Copy link
Contributor

@Sysix Sysix commented Nov 23, 2024

The current implementation checks on onChange / onSave. There they will respect the run option.
This is not optimal because the client is sending events, which will be ignored by the server.

This implementation uses the feature, that the client requests diagnostics from the server.

Diagnostics are currently published by the server to the client using a notification. This model has the advantage that for workspace wide diagnostics the server has the freedom to compute them at a server preferred point in time. On the other hand the approach has the disadvantage that the server can’t prioritize the computation for the file in which the user types or which are visible in the editor. Inferring the client’s UI state from the textDocument/didOpen and textDocument/didChange notifications might lead to false positives since these notifications are ownership transfer notifications.

The specification therefore introduces the concept of diagnostic pull requests to give a client more control over the documents for which diagnostics should be computed and at which point in time.

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_pullDiagnostics

Copy link

graphite-app bot commented Nov 23, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added A-editor Area - Editor and Language Server C-enhancement Category - New feature or request labels Nov 23, 2024
@@ -31,43 +33,26 @@ struct Backend {
client: Client,
root_uri: OnceCell<Option<Url>>,
server_linter: RwLock<ServerLinter>,
document_content_cache: DashMap<String, String>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do know now if this could lead to performance problems. We need to keep all content of any open file in the memory.

@Sysix Sysix marked this pull request as ready for review November 24, 2024 19:17
@Boshen
Copy link
Member

Boshen commented Nov 25, 2024

I've invited you and @nrayburn-tech as collaborators to help with review.

@nrayburn-tech
Copy link
Contributor

I don't think IntelliJ currently supports pulling diagnostics, so this would be a pretty big blocker for the IntelliJ plugin. Either the IntelliJ plugin would have to implement this functionality or wait until it is added to IntelliJ.
https://plugins.jetbrains.com/docs/intellij/language-server-protocol.html#supported-features - Currently supported features.
https://youtrack.jetbrains.com/issues?q=%22textDocument%2Fdiagnostic%22 - Not finding any open issues for this feature.

I don't think it's worth merging until IntelliJ has support for this. I didn't review the actual changes much given the lack of IntelliJ support.

@Sysix
Copy link
Contributor Author

Sysix commented Nov 25, 2024

I don't think it's worth merging until IntelliJ has support for this. I didn't review the actual changes much given the lack of IntelliJ support.

I dont think this will come in the near future 😢
At first the plugin is only available in commercial products:

The integration with the Language Server Protocol is created as an extension to the paid IntelliJ-based IDEs. Therefore, plugins using Language Server integration are not available in Community releases of JetBrains products and Android Studio from Google.
https://plugins.jetbrains.com/docs/intellij/language-server-protocol.html#supported-ides

The community is not happy about this: https://blog.jetbrains.com/platform/2023/07/lsp-for-plugin-developers/

Closing for now, maybe we change our minds in the future

@Sysix Sysix closed this Nov 25, 2024
@Sysix Sysix deleted the feat-language-server-capbility-diagnostic-provider branch November 25, 2024 17:23
@nrayburn-tech
Copy link
Contributor

I don’t have specifics, but my guess is that if we didn’t use the language server then the effort involved to maintain the JetBrains plugin would significantly increase. There might be community alternatives available that do something similar, but I don’t know off hand.

@Boshen is it worth discussing using something other than the official language server support in JetBrains IDEs?

FYI @IWANABETHATGUY since you started the plugin.

@Sysix
Copy link
Contributor Author

Sysix commented Dec 2, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-editor Area - Editor and Language Server C-enhancement Category - New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants