Opt-in to use ErrorProviderBridge

[jtulach]

Fixes #7647 by designing an opt-in API for providers of ErrorProvider implementations to enable bridging of their errors into NetBeans IDE as hints.

The existing ErrorProvider subclasses (that don't override this new method) will only be used in the VSCode extension.

New providers are encouraged (by the Javadoc) to override the hintsLayerNameFor method to return non-null value. E.g. to opt-in into the delegation as provided by

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

This opt-in gives us so much needed compatibility for previously written modules and also easy path for new implementations of ErrorProvider to be used in NetBeans IDE.

[jtulach]

With this change the PHP support seems to display non-duplicated warnings and the Enso support continues to work (after overriding the isEnabled method):

[matthiasblaesing]

Thank you for the update. However, I don't think this is the right approach.

Introducing an API in the stabilization branch feels wrong. For the implementation od isEnabled: GraphicsEnvironment.isHeadless() is IMHO a bad base to decide whether or not to enable it. I can imagine a GUI tool, that is run headful, but wants to provide an LSP. It is totally intransparent, why it works based on some random system property.

From my POV the problem comes from globally hooking into the lsp.client infrastructure for all files. For normal LSP based implementations (for example typescript) the client works because it is opt-in and won't interfere, when not used.

I understand, that you want to be able to expose the enso functionality quickly, but that can be done without this global approach:

• Shortterm (now):
  • revert the changes to lsp.client
  • add CompletionProvider and implement a Hint provider in Enso module itself (or in a bridge-module). These two implementation register only functionality for their target language. You can copy your code and also reuse NetBeans code by copying it (just requires NOTICE file changes if enso is distributed as a plugin without NB)
• Longterm (next cycle or later):
  • Check if the completion provider and hint provider implemented for enso could be generalized
  • If they can be generalized (for example in the api.lsp module), add the implementations to the module
  • Update the enso module to make use of the common implementations and drop the code duplication

It would allow you to continue the work on the enso intregration and still push forward with better integration. For the NetBeans IDE it would allow a safe release and an unhasted realization.

[jtulach]

The problem with HintsController.setErrors is that it is not declarative. It is push at random approach. There is no way to detect any registration in a layer or lookup to find out if the setErrors call will happen or not. I guess I made a mistake when this API got in. But it is in and we have to live with it.

I don't think this is the right approach.

I'd like to know what others think then. I can try to implement various solutions, but the core problem remains the same:

• "identify whether an ErrorProvider has been written before or after" of Using LSP's ErrorProvider & another attempt at CompletionCollector #7579 (that's what this PR does)
• or make an incompatible change and fix as many misbehaving cases as reported (I can do that as well)

you want to be able to expose the enso functionality quickly, ... CompletionProvider and implement a Hint provider in Enso module itself (or in a bridge-module).

That's a misunderstanding. If I wanted to expose Enso functionality in NetBeans, I would do what you suggest. But I am (a NetBeans) architect. I don't want people (not me!) to write the same code twice. I want them to provide a single SPI implementation that works in both NetBeans IDE and VSCode. As #7579 demonstrated the ErrorProvider is the (sufficient enough) SPI to achieve that. All we need to do is to find out whether the delegation should be used or not.

I can imagine a GUI tool, that is run headful, but wants to provide an LSP. It is totally intransparent

People can usually imagine a lot of weird usecases (I used to be good at that when I was the architect). However for our purposes, we have just three:

• NetBeans IDE
• Apache NetBeans VSCode extension
• some NetBeans based application

The isGraphicalEnvironment is a good check for all these cases. The check is already used at various places, but I can certainly replace it with something different. An explicit system property or a check for a module present only in VSCode extension come to my mind. What's your preferred choice, if any?

I can also just fix the GsfLanguage cases and wait for another report, if that's your preferred choice.

[matthiasblaesing]

Core point: it does not matter, that #7579 was already merged. It was broken, it can be reverted, it is in the same release cycle. So its existence is not an argument.

What annoys me is, that you want to improve enso integration, that is great, but you are risking the NetBeans IDE. I had bad feeling when I saw, that you attached LSP functions to all documents, apart from explicitly lsp.client handled ones. It turned out, that the reason I pulled my veto (that only the Java ErrorProvider has the problem of crosstalk) was wrong.

The GsfErrorProvider from my POV demonstrates, that you can implement a targetted bridge, that only affects a subset of mimetypes and thus is safe.

What I'm suggesting is to implement the inversion of the GsfErrorProvider, that you can opt in as a module author to use, for example by registering some common bridge implementation.

The bridge does not need to be in NetBeans from the beginning as you can implement it in enso without a problem.

I see nothing solved by forcing this into NetBeans now.

[jtulach]

you can opt in as a module author to use, for example by registering some common bridge implementation.

This PR does exactly that. As a module author one can opt-in to enable ErrorProviderBridge.

Introducing an API in the stabilization branch feels wrong.

To allow one to opt-in, one has to create a new API!

Options:

• merge this PR with isEnabled() opt-in API for NetBeans 23
• change the opt-in API for NetBeans 23
• remove the ErrorProviderBridge from NetBeans 23

[lahodaj]

So, thinking of this: the ErrorProviders (and most other providers from the LSP APIs) are registered in the MimeLookup, i.e. they are intended for a specific mime-types. So, solving this at the mime-type seems much more reasonable than creating a new isEnabled, which everyone will need to implement, and whose implementation will be very tricky.

In other words - there should be an opt-in, based on mime-types. When one creates a support for new language, say L (e.g. mime-type text/x-l), they can create ErrorProvider, CallHierarchyProvider, etc., and register them for their mime-type. And then say, for text/x-l, use the providers that are globally installed. (There may need to be some shenanigans around things like CommandProvider, as those are global.)

This could either be hardcoded in every implementation class in lsp.client (which is mostly what the current changes are doing), or it could simply be a bridge, so this would look like an ordinary LSP server for most of lsp.client. I would be for the bridge, to separate concerns. The amount of information we can pass from the provider to the UI is limited by the LSP anyway, so I don't think I see a strong need to complicate every feature implementation in the LSP client with the need to support two usecases when it can support only one (the LSP server one), and there can be a bridge gluing things together.

All in all, I hope Enso and others wanting to reuse their implementation could do something like:

@RegisterLSPServicesForMimeType("application/x-enso")
package org.enso.tools.enso4igv;

or:

@RegisterLSPServicesForMimeType("text/x-l")
package org.enso.tools.enso4igv;

and all could be handled for them, without the need to do magic stuff in isEnabled.

[mbien]

I can also just fix the GsfLanguage cases and wait for another report, if that's your preferred choice.

@jtulach I am sure you know but this is a good time to point out that NetBeans does not have a QA team right now. Variants of this patch were reverted twice before if I remember correctly. I did also try to highlight (#7579 (comment), #7579 (comment), ..) the problem of 1) changes here have proven repeatedly to affect seemingly unrelated areas of NB and 2) there seem to be no regression tests for any of this. All taken together: this is a very bad combination of things!

So no, we can't go on like this and merge PRs to stabilize them in master (or delivery, which is worse). RC3 is soon to happen and this wouldn't be a good time to try to patch things up under those circumstances.

If you author a PR as PMC/committer and your instincts tell you that this change is risky, you absolutely should make sure to test it well before trying to get it in.

I am sorry to say this but I would advise a revert again #7650 since this is is the lowest risk action which brings us back to a state with empirical evidence that it worked fine before.

I don't believe in vetos in a project with so many cooks, so I hope we can figure out the right thing to do without invoking them.

[jtulach]

Thanks for sharing your opinions. Yes, what has happened is embarrassing. I sincerely believe it is because inherent backward incompatibility introduced by my changes. I have underestimated the impact of duplicated hints being visible. To fix that I am convinced opt-in is the right solution. Today I got a new idea for opt-in API & implementation. I'll expose it in this PR.

@lahodaj - thanks for a deep description of your preferred solution. However, I don't understand it fully and I am pretty sure, it is too late to try it for NetBeans 23 anyway.

@mbien - yes, revert might be the most conservative way forward for NetBeans 23. However as stated here, it shouldn't be full revert, but only a revert of ErrorProviderBridge. I can prepare such a PR as well - in case the API change in this PR doesn't get a go for NetBeans 23.

[jtulach]

Today I got a new idea for opt-in API & implementation.

The new opt-in API is available in dc5d4e0 - unlike the previous isEnabled version, I like the new API as it seems to make sense on its own. I consider it a gentle and especially compatible addition that fixes all known problems with ErrorProviderBridge (PHP works and so will any other module until it opts-in).

[neilcsmith-net]

It's up to @matthiasblaesing whether to modify or withdraw the revert PR - a revert PR is effectively a veto.

I'm concerned about tinkering with this for what should be the last release candidate for 23. If I was involved in releases currently I might be inclined to insist on the revert. @ebarboni should have a chance to comment and engage in the decision here too. It's potentially his workload this will impact most if any issues remain.

[jtulach]

It's up to @matthiasblaesing whether to modify or withdraw the revert PR - a revert PR is effectively a veto.

According to this comment it is not a veto, but a safety net. At least that was the state four days ago.

[lahodaj]

We currently have a single-purpose hack to disable duplicate code completions:

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

Line 61 in 14d5cbc

if (id.startsWith("Editors/"+ mime)) {

(which may seemingly be OK, but what happens when someone will register some special-purpose code completion for Enso - will that disable the lsp.client based code completion)

And here proposal for another single-purpose hack (which, moreover, will be hard to understand to the users/clients, I think).

Will we need more single-purpose hacks for other features?

I am sorry, but I fail to see how this is better than having a single clear API (e.g. in a form of an annotation) saying "for mime-type , please use api.lsp services to gather information to and let lsp.client use them to provide language support for ".

(Maybe, if needed, we could enhance that to support exclusions - like use these api.lsp services and not these other ones, as I want to implement the other ones myself - but I don't think we are there, and may never get there.)

[neilcsmith-net]

According to ... it is not a veto, but a safety net.

There's no reason it can't be both. Someone opening a revert PR is effectively saying they would have -1'd and requested changes had the technical concerns been known in advance of merging. That's how we've managed revert PRs in the past. The person who opens them gets to decide whether their concerns have been adequately addressed, as may anyone else who's in favour of the revert.

[neilcsmith-net]

We can't really do spec version increments on delivery at the moment. If this gets merged to delivery it should still use 1.28

[matthiasblaesing]

I had another look today, and still think we should revert and reevaluate after release.

• I'm not a huge fan on creating APIs/libraries without multiple users. They have the tendency to be onesided and fall out of love. We have a single user (enso), so the question is: How common will Java based LSP be in practice and if they exist how often will they be implemented using NB code and not just LSP4J directly? For the latter case a different kind of LSPBinding implementation might make sense, keeping the implementations separate, but communication in the same process.
• Implementing this in lsp.client seem arbitrary - the common base is not that big between the two use cases (integration with LSP servers and NB based partitial LSP implentations)
• Running the diagnostic routines on any change, might overload the IDE. It was an explicit implementation detail, that there are two levels of diagnostic (error and hint) and these two are run with a 500ms for the LSP server, if I'm not mistaken.
• Registration of the ErrorBridge is done outside the EDT (call to putClientProperty), this might or might not be a problem, but the function is specified to generate PropertyChangeEvents and listeners might expect to be only called from the EDT.
• The hack/workaround in LspCompletionProviderImpl (check if "real" completion provider is present) is not necessary if the provider would be explicity registered
• The changes to the HintsDiagnosticsProvider and the JavaErrorProvider are not needed anymore
• The implementation could be done first outside the core, settle down and be integrated later. The impact for the target module would be minimal.

[mbien]

i too am +1 for revert for NB23, reasons given at #7659 (comment)

[lahodaj]

FWIW, this shows the high-level API I have in mind:
lahodaj@f19f4db
and this shows how it can be used (on example of Java):
lahodaj@ec1e7ee

I am not completely happy about the lower-level API (i.e. the mark in the layer/system FS) and internals, but the high-level API (i.e. RegisterLSPServices) seems sensible to me. What do others think?

[jtulach]

We can't really do spec version increments on delivery at the moment

There is a little support for doing an API change of this kind just before RC3. Closing.

As wrote here:

The problems reported by

• Duplicate text in hint tooltip #7647
  are only related to Opt-in to use ErrorProviderBridge #7659 (comment).

As such we should revert only 557c823 commit.

[neilcsmith-net]

@lahodaj I think there's something in there worth pursuing. But I'm concerned about anything that makes lsp.client a dependency there - some of us have use for these modules without that! Is there not a way of using / enhancing mime lookup to cover the requirements here?

As this is now closed, something for discussion elsewhere?

[matthiasblaesing]

@lahodaj @neilcsmith-net the idea looks sane to me. I share concerns about dependencies aswell though. But this might work:

• Place the annotation RegisterLSPServices (we might want to bikeshed about the name) into api.lsp. That module is in the dependency tree anyway as you need it for ErrorProvider and the other LSP services.
• The annotation process could then write an *.instance file into the MimeLookup for the annotated class, making it available to other modules
• lsp.client could now read the entries from the MimeLookup and create the bridges

So users of the module, that provides the ErrorProvider, don't get additional dependencies. Users that want to use the bridges load lsp.client and get them. This would leave the question whether or not the functionality should be in lsp.client or be split of into a separate module lsp.support. From my understanding this decision could be postponed, as it is possible to split a module at a later time and provide automatic dependencies, if that happens.

@jtulach given the discussion here and in the mindset of being open for alternative implementation I would prefer to revert completely and restart with a clean approach.

[lahodaj]

First, I was thinking of putting this new stuff into a separate module, and that may happen, but I always thought it would depend on lsp.client, so using the API would automatically include lsp.client as well. (The advantage of this would be that would hopefully could break/remove the dependency between lsp.client and api.lsp, as this dependency seems not quite ideal to me either.)

Not quite sure what's the issue with including lsp.client - the sole point of this annotation/opt-in is to say to lsp.client: "please provide language/editor features for this mime-type, by using the services registered for this mime type". To me, the annotation makes no sense at all without lsp.client.

Maybe the idea is that this opt-in to use lsp.client would be sort-of optional for some language. But if that happens, wouldn't it be cleaner to have a bridge module for that language that would be enabled when lsp.client is present, and disabled otherwise.

Overall, I don't think api.lsp is a good place for the annotation - that's the server part, which knows "nothing" about the GUI, and the annotation cannot deliver what it promises without lsp.client.

[neilcsmith-net]

Not quite sure what's the issue with including lsp.client

NetBeans is a platform of libraries as well as the IDE. We publish these all to Maven for a reason. I personally am using java.sourceui in two platform applications. I do not want to suddenly have to ship lsp.client in order to continue to do so. I have a feeling that Graal's IGV might be doing the same. although with an earlier NB version? No idea who else. It was enough of an issue for me when the single file support brought in debugger dependencies that weren't there before.

I'm not sure what is really LSP specific in this anyway? Except in where it's currently needed obviously! Am I right in thinking this is registering a universal service that basically disables itself per mime type based on the existence of mime-specific providers (previous) or a flag (in your suggestion)?

[lahodaj]

Aha, now I understand, this is about:
lahodaj@ec1e7ee

that was supposed to be an example how the API/annotation is used. I could have used any language, but everything was setup for Java, so I used Java. I do not think we would want to use this for Java in production, that's the reason why the commit is separate and on a separate branch.

In other words, the (end-user API) proposal is this and only this:
lahodaj@f19f4db

the other commit (lahodaj@ec1e7ee) is a way to see how it would be used, and permits experimentation, and would not be used in production. But if we ever wanted to use the RegisterLSPServices for Java, we would simply put it into a bridge (eager) module, that would depend on lsp.client.

Sorry for the confusion.

[neilcsmith-net]

Thanks! Yes, I thought it was a curious choice of example but makes sense. Sorry, the previous debugger addition may have made me overly twitchy! 😄 As long as every module that needs to utilise the registration annotation would already be expected to have that dependency (possibly transitively) then I have no problem with it.

I'm still wondering on the specific API vs feature of (Mime)Lookup though?