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

Language server does not connect if there is no codelensinlayprovider capability in the server #29

Closed
turkeylurkey opened this issue Dec 6, 2023 · 14 comments
Labels
invalid This doesn't seem right

Comments

@turkeylurkey
Copy link
Contributor

Using this code in combination with the lsp4jakarta server I see the server is started but the IDE does not connect:
image
Whereas the connection to lsp4mp is correct:
image

Examining the code we see that wrapper.connect() causes the messages to be passed. This is called in LanguageServiceAccessor line 79 which is reached from LSPCodelensInlayProvider line 95. If I modify this line to pass true for the filter:

   .getLanguageServers(file, capabilities -> capabilities.getCodeLensProvider() != null || true)

then the connection is made and the messages are processed normally.
image
By way of demonstration, if we comment out the extension point:

<!--        <codeInsight.inlayProvider language="JAVA"-->
<!--                                   implementationClass="org.microshed.lsp4ij.operations.codelens.LSPCodelensInlayProvider"/>-->

then the lsp4mp server similarly is not connected.

If you use hover or completion then the language server is started and connected by other means but I would like the diagnostics to be displayed when the file is opened. It seems the challenge is that the IDE does not call the diagnostic code (externalAnnotator) which probably would also start and connect the language servers.

Admittedly this testing is still using lsp4ij as a jar, not a plugin, but I'm not sure the behaviour would be affected. (The resources and extension points are added to the plugin.xml in the parent plugin.)

Do you have a suggestion to start and connect the language server just using diagnostics without codelens?

@angelozerr
Copy link
Contributor

.getLanguageServers(file, capabilities -> capabilities.getCodeLensProvider() != null || true)

If you need to do that,I suspect that Jakarta LS doesn't give the proper response

"codeLensProvider": {},

Here a sample with LSP4MP:

[Trace - 14:22:26] Received response 'initialize - (1)' in 1300ms.
Result: {
  "capabilities": {
    "textDocumentSync": 2,
    "hoverProvider": true,
    "completionProvider": {
      "resolveProvider": true,
      "triggerCharacters": [
        ".",
        "%",
        "\u003d",
        "$",
        "{",
        ":",
        "@",
        "\""
      ]
    },
    "definitionProvider": true,
    "documentSymbolProvider": true,
    "workspaceSymbolProvider": true,
    "codeLensProvider": {},
    "documentFormattingProvider": true,
    "documentRangeFormattingProvider": true,
    "inlayHintProvider": true
  }
}

but I would like the diagnostics to be displayed when the file is opened

If your LS publish diagnostic when file is opened,it should work? Is this problem comes from Jakarta LSonly or with LSP4MP LS too?

In your case it is working.

@angelozerr
Copy link
Contributor

I suggest really that you use our own lsp4ij, because for instance in our version we don't need to write

<externalAnnotator language="XML" implementationClass="org.microshed.lsp4ij.operations.diagnostics.LSPDiagnosticAnnotator"/>

@angelozerr
Copy link
Contributor

We did so many improvement in LSP4IJ to avoid some freeze or problem to start the connection of language server. For instance the connection should never done when IJ is indexing because it degrades performance.

When I see your code, you are using an old code https://github.com/MicroShed/lsp4ij/blob/main/src/main/java/org/microshed/lsp4ij/ConnectDocumentToLanguageServerSetupParticipant.java

If you see our code https://github.com/redhat-developer/lsp4ij/blob/main/src/main/java/com/redhat/devtools/lsp4ij/ConnectDocumentToLanguageServerSetupParticipant.java it is totally different, we connect the document to ls with non blocking read action, which improve dragsticly the performance and avoid some troubles.

In other words please align with our code or better please use our lsp4ij.

We have a PR for IJ Quarkus to consume lsp4ij redhat-developer/intellij-quarkus#1278

@angelozerr
Copy link
Contributor

Do you have a suggestion to start and connect the language server just using diagnostics without codelens?

It should work with or without codelens.

If I understand your problem it is not a problem with connect but with diagnostic which is not shown when language server doesn't support codelens, right?

By way of demonstration, if we comment out the extension point: then the lsp4mp server similarly is not connected.

connected or diagnostic is not shown when you open the file?

I treid to comment the inlay hint as you explained and it works for us.

The external annotator should be refreshed at https://github.com/MicroShed/lsp4ij/blob/fbb525da2c39a060030df79a4354aae29f75b4d4/src/main/java/org/microshed/lsp4ij/operations/diagnostics/LSPDiagnosticHandler.java#L71 but your code is old the refresh should be done in a non blocking action

var action = ReadAction.nonBlocking((Callable<Void>) () -> {
to cancel teh previous refresh and do the refresh in a read action

Please synchronize your code with our code or please use our lsp4ij.

@turkeylurkey
Copy link
Contributor Author

Just to be clear I did update to the latest code from 12/01 before asking this question. I just pushed my branch so you can see the code I tested e.g. https://github.com/MicroShed/lsp4ij/blob/q1201/src/main/java/org/microshed/lsp4ij/ConnectDocumentToLanguageServerSetupParticipant.java and https://github.com/MicroShed/lsp4ij/blob/q1201/src/main/java/org/microshed/lsp4ij/operations/diagnostics/LSPDiagnosticHandler.java

There doesn't seem to be a problem after wrapper.connect() is called but in the case of opening a Java file just displaying diagnostics doesn't seem to cause IntelliJ to call the external annotator which, I assume, would use LanguageServiceAccessor to connect. This is reminiscent of prior attempts to display diagnostics using InspectionToolProvider.

@angelozerr
Copy link
Contributor

angelozerr commented Dec 8, 2023

It is very hard to help you since I cannot reproduce your issue by removing inlay hint provider from our lsp4mp.

I have a plan to add a language server with user settings without developping an intellij plugin.

Perhaps there will have your issue or not, but it will easier to discuss by playing together with the same language server and same configuration.

@turkeylurkey
Copy link
Contributor Author

I notice that in LSPCodelensInlayProvider we have LanguageServiceAccessor.getInstance(project).getLanguageServers(file, filter) which can connect but in LSPDiagnosticAnnotator we see LanguageServiceAccessor.getInstance(file.getProject()).getStartedServers() which assumes connect has been called. These two classes are both extension points. Is there a reason to use different approaches?

Would it help to be more greedy and to call wrapper.connect() on the language server when the file is first opened (using a listener)? I suppose this assumes the LS is going to offer certain capabilities. Have you considered this?

@angelozerr
Copy link
Contributor

angelozerr commented Dec 12, 2023

LanguageServiceAccessor.getInstance(file.getProject()).getStartedServers() which assumes connect has been called.

Validation is refrehsed when textDocument/publishDiagnostics is reported which comes from a language server which is started. Do you see some textDocument/publishDiagnostics in LSP console for your Jakarta LS when you open a Java file?

I suppose this assumes the LS is going to offer certain capabilities.

Could you explain which capabilities you have in your mind. If your idea is to fix by refreshing at hand the externalAnnotator for Jakarta LS when wrapper.connect is called, I think it is a bad idea. Your problem is a bug (from Jakarta LS? from your LSP4IJ, from our LSP4IJ, but I cannot reproduce), so it must be fixed in LSP4IJ himeself without some hack.

@turkeylurkey
Copy link
Contributor Author

No, we don't see any textDocument/publishDiagnostics because there was no wrapper.connect() and hence no textDocument/didOpen messages. I'm thinking at a high level both codelens and diagnostic seem like peer capabilities and so they should have a similar implementation. I'm looking at LSPCodelensInlayProvider.collect() and LSPDiagnosticAnnotator.apply(). That is, they should both use LanguageServiceAccessor.getInstance(project).getLanguageServers(file, filter). Is that possible?

As for the other idea, are you saying that the concept of 'file open' is a tools plugin concept and not a language server plugin concept? If so I can see your point. Please confirm.

@angelozerr
Copy link
Contributor

angelozerr commented Dec 13, 2023

No, we don't see any textDocument/publishDiagnostics because there was no wrapper.connect() and hence no textDocument/didOpen messages.

It is your problem. You need to find why textDocument/didOpen is not sent. When this notification will send, it it will call your didOpen and it seems you do like LSP4MP LS (you validate the file) and the LS will report textDocument/publishDiagnostics messages.

I'm thinking at a high level both codelens and diagnostic seem like peer capabilities and so they should have a similar implementation. I'm looking at LSPCodelensInlayProvider.collect() and LSPDiagnosticAnnotator.apply(). That is, they should both use LanguageServiceAccessor.getInstance(project).getLanguageServers(file, filter). Is that possible?

  • textDocument/codeLens is a request, in other words it is the LSP client which asks to the LS server the code lens that it need to show.
  • textDocument/publishDiagnostics is a notification send by the server, in other words the LSP client doesn't ask to the LS server the error that it need to show. It is the server which notifies to the client that it need to show some errors. At this step, the LS is started.

It is the reason why textDocument/publishDiagnostics uses started servers, because if textDocument/publishDiagnostics is reported from a, LS, it means that LS is started and you don't need to force the start.

I suspect (not sure with that) that your problem is with Thread and again you don't use our last code from LSP4IJ. For instance you don't do

CompletableFuture.runAsync(() -> consumer.consume(message));
which is important in some case, and perhaps your LSP Jakarta issue with didOpen which is not done is because you done process message asynchronously. There are perhaps several messages which are received and your Thread is blocked and didOpen is not done. Please align your lsp4ij with our lsp4ij or better please use our LSP4IJ. We plan to do a release soon.

As for the other idea, are you saying that the concept of 'file open' is a tools plugin concept and not a language server plugin concept? If so I can see your point. Please confirm.

I mean that you need to follows LSP specification and not do some hack to force the didOpen or other things. Your problem is a bug and you should never force the didOpen only for your Jakarta LS. Your Jakarta LS should work in vscode, Eclipse IDE, IJ, etc without hacking didOpen.

For instance you should never done that https://github.com/MicroShed/lsp4ij/blob/fbb525da2c39a060030df79a4354aae29f75b4d4/src/main/java/org/microshed/lsp4ij/DocumentContentSynchronizer.java#L123 You did this hack to fix problem to synchronize validation, but validation should never triggered by the LSP client. We had this same problem in IJ Quarkus and we fixed it.

@fbricon
Copy link
Contributor

fbricon commented Dec 13, 2023

We already tried removing the codeLens provider of quarkus to reproduce the problem, and the server still connects on fileDidOpen, so it's doesn't seem to be an LSP4IJ issue.
Unless you can find steps to reproduce reproduce it with an LSP4IJ-based LSP (feel free to hack Quarkus or Qute if you can't use JakartaEE), we can't guess what's happening in your fork.

@turkeylurkey
Copy link
Contributor Author

Hmmm, it turns out there is a file listener, ConnectDocumentToLanguageServerSetupParticipant. Thank you for the help, @mezarin. The problem is that I did not configure it correctly in the tools plugin.

Even with it missing the codelens provider can also start the messages going to the language server. So if you remove ConnectDocumentToLanguageServerSetupParticipant and LSPCodelensInlayProvider from the xml files then didOpen is not sent:
image

Thanks for all your help!

@angelozerr
Copy link
Contributor

@turkeylurkey I m glad that you discover the problem. IMHO you should use our lsp4ij to avoid spending times to align your fork with our lsp4ij where we try to improve it day per day.

@turkeylurkey
Copy link
Contributor Author

I agree but we need to set up on our side.

@angelozerr angelozerr added the invalid This doesn't seem right label Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

3 participants