Quick fixes get corrupted when code actions from multiple Language Servers are relevant to one Java file.

[scottkurz]

Background

I'm working on Liberty Tools Eclipse which pulls in each of the LSP4Jakarta and LSP4MP language servers.

In this issue I noted that the Quick Fix that should be generated by the LSP4Jakarta is getting handled/produced/displayed incorrectly because of the presence of LSP4MP and its code action.

Instead of the "Quick Fix" I expect from the LSP4Jakarta code action, I see a mix of a Quick Fix from a LSP4MP code action and the LSP4Jakarta diagnostic I think incorrectly displayed (see the screenshot in the above issue in Liberty Tools Eclipse).

Environment:

• lsp4e v0.17.1 (the bundle, which I believe => v0.24.1 Git tag/release) in Eclipse v4.29, consuming LSP4MP 0.10.0 and LSP4Jakarta 0.2.0-SNAPSHOT, on Windows 11

Suspected area of code

I have debugged into the code to have a rough idea of where things might be going wrong, but still have some gaps in my knowledge.

I'm thinking this code is likely related to my problem, since it seems to be content to use the first response by the first LS (via executor.computeFirst(...)) .

The LSP4MP has a code action that is not scoped to any particular diagnostic:

     <extension point="org.eclipse.lsp4mp.jdt.core.javaFeatureParticipants">
      <!-- Java codeAction for generating MicroProfile OpenAPI annotations -->
      <codeAction kind="source"
                  class="org.eclipse.lsp4mp.jdt.internal.openapi.java.MicroProfileGenerateOpenAPIOperation" />
   </extension>

whereas the LSP4Jakarta code action is related to a specific diagnostic identified by the LSP4Jakarta.

Furthermore, this code changed was changed in this PR: https://github.com/eclipse/lsp4e/pull/535/files which was from around the time I first noticed the problem, in the v4.27 release from 1Q23. (However at the time I was running with a different level of lsp4e from the one in the standard package so I do NOT really have a "clean" bug report from the v4.27 release).

Other notes

UPDATE: I deleted my previous paragaraph here. All I was observing was that the marker had an attribute, 'lspCodeActions' with value mapping onto an LSP4MP code action, while having another attribute 'lspDiagnostic', with value mapping to an LSP4Jakarta diagnostic. I don't think this marker handling attribute merging is another issue in and of itself.

Next step

I'm continuing to look at this in the debugger, but figured I'd stop and write this up in case someone already understands the problem based on that description, and/or can share some pointers.

[mickaelistria]

That's an interesting case. Do you think you could reproduce the issue in some unit test?
I see the code has

LanguageServerProjectExecutor executor = LanguageServers.forProject(file.getProject())
					.withCapability(ServerCapabilities::getCodeActionProvider)
					// try to use same LS as the one that created the marker
					.withPreferredServer(LanguageServersRegistry.getInstance().getDefinition((String) attributes[0]));

Please make sure that the attributes[0] matches the right LS (LSP4Jakarta) and resolve to a proper LS definition).
Also, you could check, by looking at the transport log, whether LSP4E does query the codeActions/quickfixes in LSP4Jakarta at all, or whether it just doesn't query anything.
I would also suggest you look at how withPreferredServer is supposed to be honored; as the issue description gives the impression it's more or less ignored here.

[scottkurz]

Please make sure that the attributes[0] matches the right LS (LSP4Jakarta) and resolve to a proper LS definition).

This all looks correct stepping through in the debugger. The limited info of the JSON-RPC flows in the LS logs too in Eclipse seems to confirm my understanding.

.... I would also suggest you look at how withPreferredServer is supposed to be honored; as the issue description gives the impression it's more or less ignored here.

This looks to be "working" as far as it's implemented, but it might not be to enforce much of a "preference".

The org.eclipse.lsp4e.LanguageServers.computeFirst(...) code at
https://github.com/eclipse/lsp4e/blob/c4d9f082d74f0bb08bad3ad6b698500d8437a0e8/org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServers.java#L147
does use this "preferred" server to establish an order (to call the preferred one first). According to the comments in the computeFirst(...) method, (which I admit I can't confirm just by reading and inspecting the source code at this point), the first (non empty/null) result to return will be used.

But since the calls are all async it doesn't mean all that much in terms of establishing order to make an async call to LS 1 then to LS 2; the LS2 here has plenty of chance to return first. In this case LSP4Jakarta has over 70 code actions while LSP4MP has under 20. Since there is some looping through the actions maybe this makes it likely for LSP4MP to return first, even though LSP4Jakarta is "preferred" here.

Does it seem like it might make more sense to "wait" for the preferred LS to reply with a non-empty/null, and only then if it doesn't to loop through and call the remaining ones?

I don't immediately know how to do that but it'd be helpful to hear your thoughts at a high level before spending time on it.

Would we want/need to include some kind of wait "timeout" because we wouldn't want to block too long waiting for the preferred LS to return?

BTW, I see the recent blame shows some changes in this area: #780 but just reading the description, I'm guessing it isn't fundamentally related to the problem in this issue. (It seems to be about cancellation support).

Thx for your insights here.

[mickaelistria]

What about we just query computeAll here and let all LS propose quickfixes for the given context?
Another approach is to stick to the current LS only, but one can imagine a LS "A" being capable of providing a fix proposal for a diagnostic sent by some other LS "B" and this would be an interesting case to keep supporting.

[scottkurz]

What about we just query computeAll here and let all LS propose quickfixes for the given context?

That seems like a good next step to attempt. Not sure when I could get to trying around to trying it, but it makes sense at a high level.

Another approach is to stick to the current LS only, but one can imagine a LS "A" being capable of providing a fix proposal for a diagnostic sent by some other LS "B" and this would be an interesting case to keep supporting.

It sounds like you might not be aware of concrete usages of this just yet. I'm open to the idea that this is useful.

One thing to note about my particular case though is that it's not quite a case where LS "A" has a fix targeting a diagnostic from LS "B". It's merely a case where LS "A" has a fix that doesn't include a targetDiagnostic at all, so it's more of a "generic" fix.

See code

   <extension point="org.eclipse.lsp4mp.jdt.core.javaFeatureParticipants">
      <!-- Java codeAction for generating MicroProfile OpenAPI annotations -->
      <codeAction kind="source"
                  class="org.eclipse.lsp4mp.jdt.internal.openapi.java.MicroProfileGenerateOpenAPIOperation" />
   </extension>

And this might get into overlap of the other issue: #867 I opened since this is case also involves a "source" code action rather than a "quickfix" kind action.

[mickaelistria]

And this might get into overlap of the other issue: #867 I opened since this is case also involves a "source" code action rather than a "quickfix" kind action.

Yes, I think if we use computeAll and filter only quickfix code actions, then we have something easy to implement and which would work well for your case, but also for cases where another LS participates to quickfixes,