Using LSP's ErrorProvider & another attempt at CompletionCollector

[jtulach]

Continuation of

• 7505 - to be reapplied by 8356885712afdd792c1b5e32a445230b9cec9c77 in this PR

• 7483

Let's use ErrorProvider when available and display its hints in NetBeans.

The above picture shows errors provided by Enso IGV Plugin rev. 1.40.554 in NetBeans as proposed by https://github.com/enso-org/enso/pull/10553

[neilcsmith-net]

What does continuation of #7505 mean? That was reverted in #7528

[jtulach]

What does continuation of #7505 mean? That was reverted in #7528

Interesting. But I want the change in - new attempt 8356885 with additional fix in ea184f7afc76d0e9c9747c22479ab388a92cd551. I tested the TypeScript editing on explorer.ts from java/java.lsp.server/vscode project. I am getting no NullPointerException (as reported by #7528). I can debug the TypeScript:

I can evaluate expressions while debugging and I see no AssertionErrors

[mbien]

the slightly concerning aspect of the last revert was that it caused multiple issues in seemingly unrelated areas - and was only noticed during manual testing. It broke auto completion in the debug expression eval window, in a maven properties editor and @matthiasblaesing found also something in typescript (don't know the details). This indicates that there is a hole in the unit test coverage.

You can find the traces under the original PR https://github.com/apache/netbeans/pull/7505#issuecomment-2192369332.

[jtulach]

the slightly concerning aspect of the last revert was that it caused multiple issues in seemingly unrelated areas - and was only noticed during manual testing. It broke auto completion in the debug expression eval window, in a maven properties editor and @matthiasblaesing found also something in typescript (don't know the details). This indicates that there is a hole in the unit test coverage.

Let's do more QA this time.

You can find the traces under the original PR #7505 (comment).

I believe ea184f7afc76d0e9c9747c22479ab388a92cd551 eliminates the NPE. I never saw the AssertionError. If anyone sees it during QA, please give me steps to reproduce.

[jtulach]

I see failure in Check PR labels, but I am not sure why? There are LSP and VSCode Extension labels on this PR.

[jtulach]

I see failure in Check PR labels, but I am not sure why? There are LSP and VSCode Extension labels on this PR.

The CI checks are all green now, but I'd rather wait for Matthias to confirm the behavior is better when he edits TypeScript. On the other hand, I'd like these lsp.client changes to get into next version of NetBeans. My colleagues are eagerly waiting for Enso support in NetBeans/IGV and they don't build NetBeans from source.

[mbien]

I see failure in Check PR labels, but I am not sure why? There are LSP and VSCode Extension labels on this PR.

workflow runs are like container images. If you start one, the state doesn't change (restarts) until you build a new image. New ones are built when they are freshly triggered (e.g PR sync). This is also mentioned on the short reviewer guide we have on the wiki which is linked in the log when the label validation fails. So my guess is that this is what happened but I wasn't there to observe it. Neil forced a fresh workflow run and everything is green.

On the other hand, I'd like these lsp.client changes to get into next version of NetBeans.

you probably saw that I added the https://github.com/apache/netbeans/labels/ci%3Adev-build label. This makes it easier to manually test this PR. As previously mentioned, the last time this caused problems and had to be reverted, it broke completion in seemingly unrelated areas + possibly more things without causing test failures. So I don't even know what features this can influence or what to look out for.

[jtulach]

Thank you for the QA, Matthias.

Same file shows a new regression introduced by this. Before your change the unused variable "list" (line 5) is marked, but only slightly now it is reported as a warning: And I think the popup also shows, that the error is reported twice.

This one is particularly tricky. Unlike #7505 and #7483 - there is no declarative registration for "hints" (am I right @jlahoda). As such it is hard to find out whether there is a better alternative to JavaErrorManager or not. I can imagine various tricks to do such check (annotating ErrorProvider with some annotation, for example), but to keep the scope as small as possible I suggest just a simple isHeadless() check.

[jtulach]

You are hitting the codepath in LspCompletionProviderImpl.java line 55.

Thanks for the hint. With such an error identification, it is easy to fix it: 87523ab. Now the debugging experience looks as expected (I'd say):

[mbien]

@jtulach I noticed while helping to merge PRs for NB23 that this PR has no milestone set, Please set the intended milestone, thanks!

[jtulach]

@jtulach I noticed while helping to merge PRs for NB23 that this PR has no milestone set, Please set the intended milestone, thanks!

I would love to see (some of these fixes - e.g. #7505 as reapplied by 8356885712afdd792c1b5e32a445230b9cec9c77 in this PR, #7483 and the rest of this PR) in NetBeans release as soon as possible. Thus adding NB23 label. If this PR isn't in integrateable state as a whole in time, I can split this PR to smaller PRs that are integratable - but it is essential for Enso to have

• navigator content
• code completion
• errors

displayed in NetBeans. As debugging VSCode extension is really a pain. It is way easier to debug in NetBeans and only when everything works generate the final .vsix... That's why I am trying so hard to get these lsp.api bridges in.

[mbien]

freeze for NB23 was moved from 15th to the 26th (next Friday) so there is time to review/test this properly without rushing anything in https://lists.apache.org/thread/pr3dnjdkot8kny9g2htqlpcg1s9f9t45

I think the main discovery here remains that none of the existing tests picked any of the problems up. Would be good to check why that happened (is the module not loaded or something similar?). E.g regression tests for double occurrence of hint annotations might be something we could add (but I would be surprised if they weren't there yet).

[jtulach]

freeze for NB23 was moved from 15th to the 26th (next Friday) so there is time to review/test this properly without rushing anything in https://lists.apache.org/thread/pr3dnjdkot8kny9g2htqlpcg1s9f9t45

Thanks for letting me know. I believe both NetBeans as well as VSCode are working fine with the current state of this PR:

• TypeScript editing works and shows code completion and documentation
• Matthias's debugging scenario works and there are no duplications
• VSCode shows hints in Dummy.java file, can debug, offers code completion

I am confident things are OK this time. Unless I hear objections, I plan to integrate early next week.

I think the main discovery here remains that none of the existing tests picked any of the problems up. Would be good to check why that happened (is the module not loaded or something similar?). E.g regression tests for double occurrence of hint annotations might be something we could add (but I would be surprised if they weren't there yet).

Hints are certainly tested, but I think they are tested via "unit tests". Such kind of tests can detect missing functionality of own module, but cannot detect extra functionality of completely different module. Such an integration test would need NbModuleSuite with all enabled modules - I don't think NetBeans has such a test these days.

[jtulach]

The best way to break things is to test everything and then make additional commit...

This is still broken. The layer registration of JavaErrorProvider in java.lsp.server (added in 275ade6) restores the broken behavior, that should have been fixed by 512ee110f7865130c16a85d930a2217fcc4545a2. I.e. the invalid error markers are back again.

Fixed by registering the provider only for tests, as was my intention to begin with. Please accept my apology for useless round of review.

[jtulach]

I also see MicronautSymbolErrorProvider in the java lookup and while currently there does not seem to be a provider that reports the same problems in NetBeans IDE, it might quickly turn up and cause similar problems as the JavaErrorManager.

I can register MicronautSymbolErrorProvider in VSCode only just like I did for JavaErrorProvider. Or we can remove org.netbeans.modules.csl.api.HintsProvider implementation in micronaut module and let the bridge handle the error presentation. I let @dbalek decide.

[dbalek]

I also see MicronautSymbolErrorProvider in the java lookup and while currently there does not seem to be a provider that reports the same problems in NetBeans IDE, it might quickly turn up and cause similar problems as the JavaErrorManager.

I can register MicronautSymbolErrorProvider in VSCode only just like I did for JavaErrorProvider. Or we can remove org.netbeans.modules.csl.api.HintsProvider implementation in micronaut module and let the bridge handle the error presentation. I let @dbalek decide.

In an ideal case, MicronautSymbolErrorProvider should provide the errors for both NetBeans IDE and VSCode.

[jtulach]

I can register MicronautSymbolErrorProvider in VSCode only just like I did for JavaErrorProvider. Or we can remove org.netbeans.modules.csl.api.HintsProvider implementation in micronaut module and let the bridge handle the error presentation. I let @dbalek decide.

In an ideal case, MicronautSymbolErrorProvider should provide the errors for both NetBeans IDE and VSCode.

Looks like there are no doubled Micronaut warnings. Things look OK:

In such case I rather leave Micronaut support untouched for now.

[jtulach]

Unless there are objections, I will merge tomorrow.

[jtulach]

Squashed into two commits:

• Re-apply #7505 that was reverted by #7528 with additional resolved.getDocumentation() can be null
• ErrorProviderBridge exposes content of ErrorProviders to NetBeans IDE

[matthiasblaesing]

@jtulach thanks for cleaning up and explaining one real example @jlahoda thanks for chiming in and giving the overall perspective!

[matthiasblaesing]

@jtulach a new regression was found introduced by this. See #7647