Skip to content

Language Server Protocol client #629

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 20 commits into from
Sep 13, 2018
Merged

Conversation

jlahoda
Copy link
Contributor

@jlahoda jlahoda commented Jul 8, 2018

This is a crude prototype of a Language Server Protocol client for NetBeans.

It currently supports:
-diagnostics from the server
-go-to-declaration
-code completion

It uses the Eclipse LSP4J (https://github.com/eclipse/lsp4j) to handle the protocol and to provide the model for the messages (I used to have a custom implementation of the protocol, but it seemed wasteful to maintain that).

There are currently two ways to use a server from the IDE:
-a module provides bindings for a particular language, starts and registers a LS. I have a crude prototype of such a module using KotlinLanguageServer, but not good enough to that publish, yet.
-the user starts the LS manually somehow, and then binds the IDE to the running LS.

To see the client in action using the latter approach, the following can be used:
-clone https://github.com/fwcd/KotlinLanguageServer
--build it (gradle install)
--unpack build/distributions/kotlin-language-server-0.1.1.zip somewhere
-clone https://github.com/jlahoda/nb-lsp-helpers
--build it (ant jar in run-lsp)
--go to run-lsp/dist and run: "java -jar run-lsp.jar /bin/kotlin-language-server"
-run NetBeans, be sure there are no files opened in the editor, use the search feature in the top right corner, type "Connect", select "Connect to Language Server" action. Fill in your project's root (e.g. the path to the checked-out KotlinLanguageServer), port is 9965, extensions "kt". In a while the IDE should connect to a the LS. Then open a kt file (from the specified project), and the above features should work.

@Chris2011
Copy link
Contributor

Only to let you know: https://issues.apache.org/jira/browse/NETBEANS-180 So this PR will resolve, more or less, this ticket.

Copy link
Contributor

@jtulach jtulach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider importing Eclipse OSGi bundles as OSGi bundles.

release.external/guava-21.0.jar=modules/ext/guava-21.0.jar
release.external/org.eclipse.lsp4j-0.4.1.jar=modules/ext/org.eclipse.lsp4j-0.4.1.jar
release.external/org.eclipse.lsp4j.generator-0.4.1.jar=modules/ext/org.eclipse.lsp4j.generator-0.4.1.jar
release.external/org.eclipse.lsp4j.jsonrpc-0.4.1.jar=modules/ext/org.eclipse.lsp4j.jsonrpc-0.4.1.jar
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume these libraries are themselves OSGi bundles. If so, I prefer to package them and consume them like OSGi bundles. Just like Mylyn does with its OSGi bundles.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, I guess a question here is if we want to export this APIs, and be liable for it. The advantage of wrapping the jars is that we can limit the exposure of the API.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the context of friend dependencies discussion (raging on in parallel), I see an attempt to bring private only copy of popular libraries like gson or guava going the exactly opposite way. Attitude like this brought NetBeans to the "overuse of friend deps" which leads to hacks and tricks to workaround it.

I prefer exposing popular 3rd party libraries (especially those packaged as OSGi bundles) as reusable components.

Copy link
Contributor Author

@jlahoda jlahoda Aug 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a significant difference between e.g. html.editor and guava. It is not reasonably possible to have multiple copies of html.editor in the IDE, while the side-effects of having multiple copies of guava should be fairly limited. I.e. if some other module needs guava, there's no need to do "hacks and tricks", it can have its own copy.

I wonder if there may be multiple versions of an OSGi module under the NB-OSGi combined module system, and if it will select the proper one based on dependencies.

If not, then with too much reusing, we may putting ourselves into a corner where upgrading various libraries will lead to significant amount of time spent ensuring things still work after the upgrade. And the time, I think, might be better spent on actual features and bugfixes of this project.

@ActionRegistration(
displayName = "#CTL_ConnectToLanguageServer"
)
@Messages("CTL_ConnectToLanguageServer=Connect to Language Server")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a natural place for this action could be a pop-up menu on the non-recognized data objects. E.g. if the IDE doesn't know a .kt file it's popup menu would suggest to connect to a language server.

@timfel
Copy link

timfel commented Jul 25, 2018

A few observations:

  • Go to symbol seems not to be implemented with the documentSymbol message
  • Custom lsp actions are not implemented, e.g. the codeLens or codeAction LSP messages

Copy link
Member

@geertjanw geertjanw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great, what needs to be done for a first release of this feature?

Copy link

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OSGi bundles would better be used as OSGi bundles, not as a wrapped JARs. Edit: turns out that except missing ide/c.google.gson/build.xml, it is already prepared as OSGi bundle.

* @return a collection of {@code NavigatorPanel}s for the given file
* null is allowed
*/
public @CheckForNull Collection<? extends NavigatorPanel> panelsFor(@NonNull FileObject file);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first place in the spi.navigator API where FileObject is exposed. Until now the navigator API (not implementation) was FileSystem API free.

</dependency>
</module-dependencies>
<friend-packages>
<friend>org.netbeans.modules.kotlin.support</friend>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clearly this module shall serve as an API for many projects to hook in. It is not only for few "friends" - expose the API in a regular way. Use arch.xml to specify the stability category (under development, I assume). Provide some documentation and initial version of apichanges.xml. Let the module be visible in the overall Javadoc (e.g. change nbbuild/build.properties).

@@ -46,13 +46,13 @@ public void testGetProviders () throws Exception {
ProviderRegistry providerReg = ProviderRegistry.getInstance();

System.out.println("Asking for non-existent type...");
assertEquals(0, providerReg.getProviders("image/non_existent_type").size());
assertEquals(0, providerReg.getProviders("image/non_existent_type", null).size());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not only existing tests shall be fixed, but new one(s) covering the DynamicRegistration shall be added.

@jlahoda jlahoda added the API Change [ci] enable extra API related tests label Sep 13, 2018
@JaroslavTulach JaroslavTulach merged commit f2b7bd0 into apache:master Sep 13, 2018
@Chris2011
Copy link
Contributor

How will syntax highlighting work? I mean how to get it from the LS @jlahoda?

@geertjanw
Copy link
Member

Not supported yet by LSP: microsoft/vscode-languageserver-node#367

@jlahoda
Copy link
Contributor Author

jlahoda commented Oct 10, 2018

The simple answer is that the LSP does not support highlighting at this point. So no highlighting through the LSP. See for example:
eclipse-lsp4j/lsp4j#40

This may get into the protocol eventually, see:
microsoft/vscode-languageserver-node#367

@Chris2011
Copy link
Contributor

Ok, thx.

@rkraneis
Copy link

rkraneis commented Jan 28, 2019

Great work! FWIW, for all on a sensible OS, there is no need to clone and build nb-lsp-helpers, as it more or less just replicates socat -v tcp-listen:9965,reuseaddr exec:/bin/kotlin-language-server. LSP with NetBeans is already quite nice to use as the Connect dialog remembers connection settings 👍

@jlahoda
Copy link
Contributor Author

jlahoda commented Jan 28, 2019

@rkraneis - thanks for the pointer, didn't know about the tool! Thanks!

@devMls
Copy link

devMls commented Jan 4, 2020

Hi

This feature can will be used ti support freemarker like eclipse do? Jbosstools use lsp ti support freemarker and NetBeans freemarker plugin is outdated and with dime bugs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Change [ci] enable extra API related tests enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants