Using LSP's ErrorProvider & another attempt at CompletionCollector #7579

jtulach · 2024-07-15T12:24:11Z

Continuation of

Let lsp.client display completions provided by CompletionCollector #7505 - to be reapplied by 8356885 in this PR
Use StructureProvider to populate navigator #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 enso-org/enso#10553

jtulach · 2024-07-15T12:26:05Z

...va.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/TextDocumentServiceImpl.java

@@ -1032,7 +1032,10 @@ public CompletableFuture<List<Either<Command, CodeAction>>> codeAction(CodeActio
                            continue;
                        }
                    }
-                    Optional<Diagnostic> diag = diagnostics.stream().filter(d -> entry.getKey().equals(d.getCode().getLeft())).findFirst();
+                    Optional<Diagnostic> diag = diagnostics.stream().filter(d -> {
+                        String code = d.getCode() != null ? d.getCode().getLeft() : null;


Unrelated to the core of this PR, but discovered at the same time. According to the LSP Spec code field is optional. Need to check for null.

neilcsmith-net · 2024-07-15T12:39:10Z

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

jtulach · 2024-07-15T15:04:22Z

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 ea184f7. 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 · 2024-07-15T15:12:48Z

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 #7505 (comment).

jtulach · 2024-07-15T16:16:29Z

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 ea184f7 eliminates the NPE. I never saw the AssertionError. If anyone sees it during QA, please give me steps to reproduce.

jtulach · 2024-07-16T04:00:41Z

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

jtulach · 2024-07-16T12:14:00Z

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 · 2024-07-16T14:27:55Z

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 ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) 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.

matthiasblaesing

From a quick(!) look this should fix the NullPointerException problem as the use site is now guarded.

The second exception is still there. And indeed (quote from #7505 (comment))

Context: This happens when I call for code completion in "Evaluate expression", which is now broken.

can still be used to reproduce.

SEVERE [org.openide.util.RequestProcessor]: Error in RequestProcessor org.netbeans.spi.editor.completion.support.AsyncCompletionTask
java.lang.AssertionError: AsyncCompletionTask: query=org.netbeans.modules.lsp.client.bindings.LspCompletionProviderImpl$1@5804552c: query.query(): Result set not finished by resultSet.finish()
	at org.netbeans.spi.editor.completion.support.AsyncCompletionTask.run(AsyncCompletionTask.java:200)
	at org.openide.util.RequestProcessor$Task.run(RequestProcessor.java:1403)
	at org.netbeans.modules.openide.util.GlobalLookup.execute(GlobalLookup.java:45)
	at org.openide.util.lookup.Lookups.executeWith(Lookups.java:287)
[catch] at org.openide.util.RequestProcessor$Processor.run(RequestProcessor.java:2018)

More detailed steps to reproduce:

Build from the branch backing this PR
Create a new maven project
Create a new class Dummy with this content:

import java.util.List;

public class Dummy {
  public static void main(String[] args) {
      List<String> list = List.of("X");
  }
}

Place a break point inside main (line 5)
Run "Debug file" on this
Go to "Evaluate Expression"
Enter "List."
Call completion Ctrl+Space
Observe problem

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

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.

jtulach · 2024-07-18T13:15:53Z

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 · 2024-07-18T13:18:28Z

java/java.hints/src/org/netbeans/modules/java/hints/infrastructure/JavaErrorProvider.java

@@ -94,7 +95,9 @@ public class JavaErrorProvider implements ErrorProvider {
    @Override
    public List<? extends Diagnostic> computeErrors(Context context) {
        List<Diagnostic> result = new ArrayList<>();
-
+        if (!GraphicsEnvironment.isHeadless()) {


The java.lsp.server project is using isHeadless() to detect whether the code is running in VSCode or in NetBeans IDE. Using it here to solve #7579 (comment) inquiry.

I think I would rather suggest to drop the @MimeRegistration on this class, and register the class in nbcode. A similar pattern is already used for numerous services. Although in this case, it may be necessary/better to create the registration manually in layer.xml, for other services we do it by having a subclass in nbcode with an appropriate registration annotation.

This gives me bad vibes and is only a partial solution. I placed a breakpoint in ErrorProviderBridge#run and I see three ErrorProvider there:

org.netbeans.modules.java.hints.infrastructure.JavaErrorProvider

org.netbeans.modules.editor.hints.lsp.HintsDiagnosticsProvider

org.netbeans.modules.micronaut.symbol.MicronautSymbolErrorProvider

(1) is now handled by checking GraphicsEnvironment#isHeadless, (2) ist explicitly filtered out by its name, (3) is retained.

This will become whac-a-mole because the interaction are not obvious. Would a different approach make sense where an autoloaded helper module, that depends on the modules, that provides the LSP implementation, provides the bridge only for that module? That way the interaction is easier to see and if necessary easy to drop.

I don't know the enso module name but assuming it is enso-lsp, a module enso-lsp.nb could be used to provide the integration limited to the enso type.

jtulach · 2024-07-18T13:24:06Z

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):

jtulach · 2024-07-20T05:50:19Z

java/java.hints/src/org/netbeans/modules/java/hints/infrastructure/JavaErrorProvider.java

 *
 * @author lahvac
 */
-@MimeRegistration(mimeType="text/x-java", service=ErrorProvider.class)


... drop the @MimeRegistration on this class, and register the class in nbcode.

Thanks @jlahoda. Done in 512ee11

jtulach · 2024-07-20T06:14:32Z

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.

matthiasblaesing

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.

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.

jtulach · 2024-07-22T09:17:27Z

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 · 2024-07-22T09:19:38Z

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.

matthiasblaesing

I think my main pain points are fixed. I have two concerns remaining about the implementation, that make sense to me to fix before integration.

ide/lsp.client/src/org/netbeans/modules/lsp/client/bindings/ErrorProviderBridge.java

matthiasblaesing · 2024-07-22T19:48:29Z

ide/lsp.client/src/org/netbeans/modules/lsp/client/bindings/ErrorProviderBridge.java

+            computeHints(ErrorProvider.Kind.ERRORS, p, "lsp:errors");
+            computeHints(ErrorProvider.Kind.HINTS, p, "lsp:hints");


Do the runs with ErrorProvider.Kind.ERRORS and and ErrorProvider.Kind.HINTS yield disjunct sets of error sets? If not, what is the result for the user?

The Enso error provider is only computing ERRORS and returns empty array for HINTS request.

I would expect disjoint sets to be provided, which are then automatically merged, and the joint result is presented to the used. If a provider returns the same diagnostics from both queries, it will be duplicated for the user, I think.

The main purpose of this split is to show errors to the user quickly, and then take the time needed to compute the warnings.

dbalek · 2024-07-24T07:37:36Z

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 · 2024-07-24T14:30:13Z

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 · 2024-07-24T14:31:44Z

Unless there are objections, I will merge tomorrow.

matthiasblaesing

Looks sane to me. My concerns were addressed. Thank you.

One final request: Please squash the commits into logical unittests. We don't gain anything from intermediate states of the, if they are logically squashed, in the future have a change to understand the big picture not just one random fix.

… resolved.getDocumentation() can be null

jtulach · 2024-07-25T04:11:26Z

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

jlahoda · 2024-07-25T05:51:16Z

ide/lsp.client/src/org/netbeans/modules/lsp/client/bindings/ErrorProviderBridge.java

+            computeHints(ErrorProvider.Kind.ERRORS, p, "lsp:errors");
+            computeHints(ErrorProvider.Kind.HINTS, p, "lsp:hints");


I would expect disjoint sets to be provided, which are then automatically merged, and the joint result is presented to the used. If a provider returns the same diagnostics from both queries, it will be duplicated for the user, I think.

The main purpose of this split is to show errors to the user quickly, and then take the time needed to compute the warnings.

matthiasblaesing · 2024-07-25T17:29:15Z

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

matthiasblaesing · 2024-08-08T18:18:09Z

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

Revert PR #7579 ("Using LSP's ErrorProvider & another attempt at CompletionCollector")

jtulach requested review from dbalek, sdedic and lahodaj July 15, 2024 12:24

jtulach self-assigned this Jul 15, 2024

jtulach commented Jul 15, 2024

View reviewed changes

jtulach mentioned this pull request Jul 15, 2024

Revert: Let lsp.client display completions provided by CompletionCollector (#7505) #7528

Merged

jtulach requested review from mbien, matthiasblaesing and neilcsmith-net July 15, 2024 15:02

jtulach added LSP [ci] enable Language Server Protocol tests VSCode Extension [ci] enable VSCode Extension tests labels Jul 15, 2024

mbien added ci:all-tests [ci] enable all tests ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Jul 15, 2024

enso-bot bot mentioned this pull request Jul 16, 2024

Output Stream built in is slow enso-org/enso#10550

Closed

apache locked and limited conversation to collaborators Jul 16, 2024

apache unlocked this conversation Jul 16, 2024

matthiasblaesing requested changes Jul 16, 2024

View reviewed changes

jtulach force-pushed the UseLspErrorProvider branch from ab7bdc9 to 5366560 Compare July 18, 2024 13:17

jtulach commented Jul 18, 2024

View reviewed changes

jtulach requested a review from matthiasblaesing July 18, 2024 13:24

jtulach changed the title ~~Using LSP's ErrorProvider~~ Using LSP's ErrorProvider & another attempt at CompletionCollector Jul 18, 2024

neilcsmith-net removed their request for review July 19, 2024 17:39

jtulach commented Jul 20, 2024

View reviewed changes

jtulach requested a review from lahodaj July 20, 2024 06:18

matthiasblaesing requested changes Jul 21, 2024

View reviewed changes

jtulach force-pushed the UseLspErrorProvider branch from 275ade6 to cba6ef2 Compare July 22, 2024 09:12

matthiasblaesing reviewed Jul 22, 2024

View reviewed changes

jtulach force-pushed the UseLspErrorProvider branch from 9ba53da to 993677e Compare July 24, 2024 08:37

jtulach requested a review from matthiasblaesing July 24, 2024 13:41

matthiasblaesing approved these changes Jul 24, 2024

View reviewed changes

mbien added the Need Squashing label Jul 24, 2024

jtulach added 2 commits July 25, 2024 06:07

Re-apply apache#7505 that was reverted by apache#7528 with additional…

c7eda4f

… resolved.getDocumentation() can be null

ErrorProviderBridge exposes content of ErrorProviders to NetBeans IDE

557c823

jtulach force-pushed the UseLspErrorProvider branch from 993677e to 557c823 Compare July 25, 2024 04:08

mbien removed the Need Squashing label Jul 25, 2024

jlahoda approved these changes Jul 25, 2024

View reviewed changes

jtulach merged commit 536ddb9 into apache:master Jul 25, 2024
40 checks passed

mbien mentioned this pull request Jul 25, 2024

Implement suppress warnings on for Unused hint and codeAction for suppress warnings annotations #7548

Merged

troizet mentioned this pull request Aug 8, 2024

Duplicate text in hint tooltip #7647

Closed

This was referenced Aug 11, 2024

Opt-in to use ErrorProviderBridge #7659

Closed

Revert PR #7579 ("Using LSP's ErrorProvider & another attempt at CompletionCollector") #7650

Merged

MartinBalin added a commit that referenced this pull request Sep 2, 2024

Merge pull request #7650 from matthiasblaesing/issue-7647

4fd016e

Revert PR #7579 ("Using LSP's ErrorProvider & another attempt at CompletionCollector")

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using LSP's ErrorProvider & another attempt at CompletionCollector #7579

Using LSP's ErrorProvider & another attempt at CompletionCollector #7579

jtulach commented Jul 15, 2024 •

edited

Loading

jtulach Jul 15, 2024

neilcsmith-net commented Jul 15, 2024

jtulach commented Jul 15, 2024 •

edited

Loading

mbien commented Jul 15, 2024

jtulach commented Jul 15, 2024

jtulach commented Jul 16, 2024

jtulach commented Jul 16, 2024

mbien commented Jul 16, 2024

matthiasblaesing left a comment •

edited

Loading

jtulach commented Jul 18, 2024 •

edited

Loading

jtulach Jul 18, 2024

lahodaj Jul 18, 2024

matthiasblaesing Jul 18, 2024

jtulach commented Jul 18, 2024 •

edited

Loading

jtulach Jul 20, 2024

jtulach commented Jul 20, 2024 •

edited

Loading

matthiasblaesing left a comment

jtulach commented Jul 22, 2024

jtulach commented Jul 22, 2024 •

edited

Loading

matthiasblaesing left a comment

matthiasblaesing Jul 22, 2024

jtulach Jul 24, 2024

jlahoda Jul 25, 2024

dbalek commented Jul 24, 2024

jtulach commented Jul 24, 2024 •

edited

Loading

jtulach commented Jul 24, 2024

matthiasblaesing left a comment

jtulach commented Jul 25, 2024

jlahoda Jul 25, 2024

matthiasblaesing commented Jul 25, 2024

matthiasblaesing commented Aug 8, 2024

		computeHints(ErrorProvider.Kind.ERRORS, p, "lsp:errors");
		computeHints(ErrorProvider.Kind.HINTS, p, "lsp:hints");

Using LSP's ErrorProvider & another attempt at CompletionCollector #7579

Using LSP's ErrorProvider & another attempt at CompletionCollector #7579

Conversation

jtulach commented Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

neilcsmith-net commented Jul 15, 2024

jtulach commented Jul 15, 2024 • edited Loading

mbien commented Jul 15, 2024

jtulach commented Jul 15, 2024

jtulach commented Jul 16, 2024

jtulach commented Jul 16, 2024

mbien commented Jul 16, 2024

matthiasblaesing left a comment • edited Loading

Choose a reason for hiding this comment

jtulach commented Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtulach commented Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

jtulach commented Jul 20, 2024 • edited Loading

matthiasblaesing left a comment

Choose a reason for hiding this comment

jtulach commented Jul 22, 2024

jtulach commented Jul 22, 2024 • edited Loading

matthiasblaesing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbalek commented Jul 24, 2024

jtulach commented Jul 24, 2024 • edited Loading

jtulach commented Jul 24, 2024

matthiasblaesing left a comment

Choose a reason for hiding this comment

jtulach commented Jul 25, 2024

Choose a reason for hiding this comment

matthiasblaesing commented Jul 25, 2024

matthiasblaesing commented Aug 8, 2024

jtulach commented Jul 15, 2024 •

edited

Loading

jtulach commented Jul 15, 2024 •

edited

Loading

matthiasblaesing left a comment •

edited

Loading

jtulach commented Jul 18, 2024 •

edited

Loading

jtulach commented Jul 18, 2024 •

edited

Loading

jtulach commented Jul 20, 2024 •

edited

Loading

jtulach commented Jul 22, 2024 •

edited

Loading

jtulach commented Jul 24, 2024 •

edited

Loading