search

apache / netbeans

Apache NetBeans
https://netbeans.apache.org/
Apache License 2.0
2.65k stars 850 forks source link

Opt-in to use ErrorProviderBridge #7659

Closed jtulach closed 2 months ago

jtulach commented 2 months ago

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

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 commented 2 months ago

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

image

jtulach commented 2 months ago

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:

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:

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 commented 2 months ago

Core point: it does not matter, that https://github.com/apache/netbeans/pull/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 commented 2 months ago

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:

lahodaj commented 2 months ago

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 commented 2 months ago

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 (https://github.com/apache/netbeans/pull/7579#issuecomment-2228742269, https://github.com/apache/netbeans/pull/7579#issuecomment-2239738030, ..) 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 https://github.com/apache/netbeans/pull/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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

We currently have a single-purpose hack to disable duplicate code completions: https://github.com/apache/netbeans/blob/14d5cbce8107d9fdaeea7e87af8f73796f10c936/ide/lsp.client/src/org/netbeans/modules/lsp/client/bindings/LspCompletionProviderImpl.java#L61 (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 commented 2 months ago

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.

matthiasblaesing commented 2 months ago

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

mbien commented 2 months ago

i too am +1 for revert for NB23, reasons given at https://github.com/apache/netbeans/pull/7659#issuecomment-2284247217

lahodaj commented 2 months ago

FWIW, this shows the high-level API I have in mind: https://github.com/lahodaj/netbeans/commit/f19f4db6b04af21bc7feff345cac7cef7d84573f and this shows how it can be used (on example of Java): https://github.com/lahodaj/netbeans/commit/ec1e7ee2336d5cc272fdc6228d6715adf31eac97

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 commented 2 months ago

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

As such we should revert only https://github.com/apache/netbeans/pull/7579/commits/557c823ba5244c6d67262eca3ce27d22bc07fc78 commit.

neilcsmith-net commented 2 months ago

@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 commented 2 months ago

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

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

Aha, now I understand, this is about: https://github.com/lahodaj/netbeans/commit/ec1e7ee2336d5cc272fdc6228d6715adf31eac97

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: https://github.com/lahodaj/netbeans/commit/f19f4db6b04af21bc7feff345cac7cef7d84573f

the other commit (https://github.com/lahodaj/netbeans/commit/ec1e7ee2336d5cc272fdc6228d6715adf31eac97) 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 commented 2 months ago

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! :smile: 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?