[WIP] Several improvements to the way remote language servers may be configured through workspace configuration #9387
Conversation
dkulieshov
added
the
status/in-progress
dkulieshov
requested review from
svor,
azatsarynnyy,
vparfonov,
tolusha,
vzhukovs,
RomanNikitenko and
vinokurig
|
ci-build |
vparfonov
changed the title
dkulieshov
added
team/ide
kind/enhancement
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/4508/ |
|
ci-test |
|
ci-test build report: |
|
All tests except CSharpLanguageTest - passed |
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
|
ci-build |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/4516/ |
|
ci-test |
There was a problem hiding this comment.
LGTM
Let's consider as an LSP support next steps:
- retiring Launcher mechanism in favor of workspace configuration for particular (potentially third party) LS
- Adding simplified config option for language selecting like option to use extension/file_name instead of regexp (I am not talking about removing regexp option, it may be useful for some advanced cases)
|
Regexp for extension looks pretty simple. I mean as for me, As to retiring LS provided via installers and launchers, will there be separate issues 1) to remove those from code and 2) rework current stacks to use compose recipe type 3) make UD LS friendly, so that adding a language server is a matter of choosing it and clicking add button, since current steps are a bit complicated for an ordinary user. |
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
| @@ -76,4 +75,19 @@ public Throwable getCause() { | |||
| } | |||
| }; | |||
| } | |||
|
|
|||
| private PromiseError getPromiseError() { | |||
| @@ -118,7 +118,7 @@ private void registerFileType( | |||
| FileTypeRegistry fileTypeRegistry, | |||
| MavenResources mavenResources, | |||
| EditorRegistry editorRegistry) { | |||
| FileType pomFile = new FileType(mavenResources.maven(), null, "pom\\.xml"); | |||
| FileType pomFile = new FileType(mavenResources.maven(), null, ".*pom\\.xml"); | |||
There was a problem hiding this comment.
This would match a file named "foopom.xml", probably not what you want?
| import javax.inject.Provider; | ||
| import org.eclipse.che.api.languageserver.shared.model.LanguageDescription; | ||
|
|
||
| public class MavenLanguageDescriptionProvider implements Provider<LanguageDescription> { |
There was a problem hiding this comment.
I was wondering why we need a Provider?
| LanguageServer server, | ||
| ServerCapabilities capabilities, | ||
| String projectPath) { | ||
| public void onLSPRoxyInitialized(LanguageServerInitializedEvent event) { |
There was a problem hiding this comment.
If this module uses a sidecar, why do we have a launch script? If not, why do we have a language proxy?
There was a problem hiding this comment.
Also, the method handles LanguageServerInitializedEvent, so "onLSPRoxyInitialized" seems a strange name.
There was a problem hiding this comment.
If I understood correctly.
In fact org.eclipse.lsp4j.services.LanguageServer fits the definition of proxy quite well , because it acts as an intermediary for requests from a client to actual language server. Anyway I'm sure you know that we use it for both parallel container or installer + launcher, so it does not really matter if we have a launch script or not.
However I agree, to avoid further confusion I will make method name a bit clearer.
| return remoteProxy; | ||
| } | ||
|
|
||
| private static class InstanceHolder { |
There was a problem hiding this comment.
What is the purpose of this class?
There was a problem hiding this comment.
One of the most common implementations of a singleton pattern - Bill Pugh solution
There was a problem hiding this comment.
Which provides lazy initialization, which is totally unnecessary in this case. It just adds complication.
| * provider and the language server server that runs in the same virtual machine and does not use | ||
| * input/output streams for communication. | ||
| */ | ||
| public class EmptyCommunicationProvider implements CommunicationProvider { |
| } | ||
|
|
||
| /** Provides client capabilities */ | ||
| private static class ClientCapabilitiesProvider { |
There was a problem hiding this comment.
Seriously, what's with the providers? Just make this a static variable in InitializeParamsProvider
There was a problem hiding this comment.
That is a very interesting suggestion, however I tend to disagree and let me please explain why.
InitializeParamsProvider itself is quite big so decomposition here is something that does not require an explanation. However in what way we actually decompose is a very interesting question. In this matter I follow Grady Booch and his book "Object-oriented Analysis and Design" who says that algorithmic decomposition is a necessary part of object-oriented analysis and design, but object-oriented systems start with and emphasize decomposition into classes. In other words I prefer to split the logic into classes, not into fields or variables, but your suggestion is more of a procedural programming.
There was a problem hiding this comment.
Yes, but you're not doing algorithmic decomposition, you're just taking a constant and making it more complicated.
There was a problem hiding this comment.
Yes, but you're not doing algorithmic decomposition, you're just taking a constant and making it more complicated.
That is not true.
There was a problem hiding this comment.
I correct myself: you're taking a simple static method and making it more complicated. Algorighmic decomposition is decomposition into functions, not classes. I heartily disagree with this style.
| private final String languageServerName; | ||
| private final StatusChecker statusChecker; | ||
|
|
||
| private Process process; |
There was a problem hiding this comment.
I don't think this variable is thread safe.
|
If you look at the problem this is supposed to fix, it seems relatively straightforward to fix the config mechanism. Instead, this turned into a 147 file rewrite of the language server registration. |
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
| * | ||
| * @return regular expression provider | ||
| */ | ||
| RegexProvider getRegexpProvider(); |
There was a problem hiding this comment.
As we have discussed on calls before, I don't think replacing the kind of standard "LanguageDescription" with regexes here is something we want to do.
There was a problem hiding this comment.
I see a statement, I don't see any argument. No logical reasoning.
| * @see LanguageServerConfigProvider | ||
| * @see LanguageServerConfigInitializer | ||
| */ | ||
| public interface LanguageServerConfig { |
There was a problem hiding this comment.
Generally, I think this interface is way too complicated, as far as the client is concerned all it needs it is a way to get an instance of a language server and perhaps a way to monitor the status of this instance. The old interface nicely captured this. The fact that you're mandating the use of streams ("CommunicationProvider") in the interface is getting an implementation detail into the interface and unnecessarily restricts the implementation.
The fact that we have null implementations and null values (for example the CommunicationProvider) is a code smell that indicates we're describing an implementation, not an interface.
There was a problem hiding this comment.
Generally, I think this interface is way too complicated, as far as the client is concerned all it needs it is a way to get an instance of a language server and perhaps a way to monitor the status of this instance. The old interface nicely captured this. The fact that you're mandating the use of streams ("CommunicationProvider") in the interface is getting an implementation detail into the interface and unnecessarily restricts the implementation.
The overcomplicating is caused by the need of backward compatibility with the installers, which is expected to be removed in future, then the configuration become simpler, ComplicationProvider and all implementation dependent stuff will be removed.
The fact that we have null implementations and null values (for example the CommunicationProvider) is a code smell that indicates we're describing an implementation, not an interface.
The fact that we have null implementation of InstanceProvider is the result of a short-sighted implementation of the MavenLanguageServer and the necessity to keep backward compatibility, because it is neither an installed language server, nor a parallel container language server. Again CommunicationProvider null implementation is needed to keep backward compatibility with launchers.
| * @return <code>null</code>if a language server is not expected to be launched locally, instance | ||
| * of an installer status provider otherwise | ||
| */ | ||
| default InstallerStatusProvider getInstallerStatusProvider() { |
There was a problem hiding this comment.
We're trying to get away from installers. Why is this in the interface here? Implementation detail.
There was a problem hiding this comment.
What kind of backward compatibility? Compile time? Runtime?
| /** | ||
| * The implementation must provide aliveness status for language server that we communicate to. | ||
| */ | ||
| interface StatusChecker { |
There was a problem hiding this comment.
This interface could be simpler as
String isAlive() where a null return signals "isAlive". Also, right now it's not clear whether the implementer is allowed to return null if "isAlive()" returns true, so you have to check for null anyway.
There was a problem hiding this comment.
In object-oriented programming we're trying to keep things simple, clear and declarative way. If the status is "alive" we return true when they ask "is it alive". Shorter code is a bad replacement for a more transparent code.
There was a problem hiding this comment.
I guess we'll have to disagree on that one. Anyway, you could at least clarify the javadoc so clients don't have to check for null.
| } | ||
|
|
||
| /** Provides language server matching regular expression definitions. */ | ||
| interface RegexProvider { |
There was a problem hiding this comment.
This interface is called "RegexProvider", but provides file watch patterns, as well. This is unexpected.
There was a problem hiding this comment.
That's probably because watch patterns are represented by regular expressions as well, unless you can suggest a better name?
There was a problem hiding this comment.
I would fold it into the language server description. If something does not have a good name, it's usually not a "thing".
|
|
||
| LanguageServer languageServer = languageServerRegistry.get(id); | ||
| InitializeParams initializeParams = initializeParamsProvider.get(id); | ||
| InitializeResult initializeResult = |
There was a problem hiding this comment.
The timeout is very likely too short for jdt.ls.
| @@ -28,19 +28,19 @@ public static String removeUriScheme(String uri) { | |||
| return uri.startsWith(FILE_PROJECTS) ? uri.substring("file://".length()) : uri; | |||
| } | |||
|
|
|||
| public static boolean truish(Boolean b) { | |||
| static boolean truish(Boolean b) { | |||
There was a problem hiding this comment.
I find these could be useful outside of the package.
There was a problem hiding this comment.
Agree, might be quite possible, we need to move it somewhere in common modules.
There was a problem hiding this comment.
I'm thinking where it should be moved, do you have any idea?
There was a problem hiding this comment.
no, that's why I left them here, but public.
| @@ -0,0 +1,218 @@ | |||
| /* | |||
There was a problem hiding this comment.
Is this more or less a copy of the preexisting code or did you change something?
There was a problem hiding this comment.
Thomas, it is mostly a copy of your class (according to git history) ServerCapabilitiesOverlay.
I made some fixes as well, in master we have
if (leftOptions != null && leftOptions.isResolveProvider()
|| rightOptions != null && leftOptions.isResolveProvider()) {
result.setResolveProvider(true);
}but should be
if (leftOptions != null && leftOptions.isResolveProvider()
|| rightOptions != null && rightOptions.isResolveProvider()) {
result.setResolveProvider(true);
}and in several other places if I remember well. Why don't you ask directly? I guess you are worried that you're not mentioned as the author in javadoc, note that in ServerCapabilitiesOverlay you are not mentioned as the author in javadoc as well, but no worries I will add, my apologies.
There was a problem hiding this comment.
No, sine the class has been moved, it's hard to review the change. If it's just a copy, I don't have to go through the effort.
| @@ -0,0 +1,769 @@ | |||
| /* | |||
There was a problem hiding this comment.
I assume this is just a move of the file?
There was a problem hiding this comment.
Not exactly, there were several changes according to a new API. Again, I'm sorry for not adding you as an author, the fix is coming.
There was a problem hiding this comment.
Again, I don't care about that, only about the correctness of the code.
| Runtime runtime = workspace.getRuntime(); | ||
| if (runtime == null) { | ||
| return ImmutableMap.of(); | ||
| } |
There was a problem hiding this comment.
Are these two not error conditions that should be logged?
| this.serverCapabilitiesAccumulator = serverCapabilitiesAccumulator; | ||
| this.findId = findId; | ||
|
|
||
| this.idRegistry = registryContainer.idRegistry; |
There was a problem hiding this comment.
So the "idRegistry" is really a form of String interning to ensure that we get the same instance of the String for locking, correct? That deserves a comment, IMO.
|
ci-build |
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
tsmaeder
dismissed
their stale review
I don't think we're going to agree on the design.
|
ci-test |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/4535/ |
|
ci-test build report: |
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
|
ci-test |
|
ci-test build report: |
|
The existed selenium tests did not find new regression. |
|
I am trying to get my custom LSP integrated as sidecar using Che 6.7.0 by configuration. |
|
It has been introduced in 6.6.0. However looking at your example I'd say that the correct configuration should probably be more similar to this: "languageRegexes":"[ {\"languageId\":\"c\", \"regex\":\".\\\\.(c|h)$\"}, {\"languageId\":\"cpp\", \"regex\":\".\\\\.(cpp|hh)$\"}]"Perhaps @eivantsov may assist you more on your purposes as he is an expert in language server configuration and related things. |
|
@eivantsov - I am looking for a E2E documentation. I tried now various options but language server is not started once a click on a file with the defined extension in my workspace. I do not see any log information in the dev maschine |
|
@dkuleshov this won't work for a language id that is not in the list of predefined ones, will it? |
|
@eivantsov as far as I know should work irrespectively of language id definition |
…ured through workspace configuration (eclipse-che#9387)
Related issue: #8925
The way we configured remote language servers was not really convenient and limited to the list of hard coded supported language identifiers that corresponded to LSP specification. The idea of this pull request is to make some little changes to language server configuration so it would be more flexible and simple. While the configuration took just a little changes the internal architecture due to its limited capacity to be changed was reorganized significantly more. Configuration and most noticeable architecture changes will be covered further.
Workspace language server configuration
Previously the server attributes configuration section looked like:
Language server identifier moved to server attributes section, seems like it (along with the type) is more an attribute than a configuration of a server. Language IDs and document filters sections unfortunately do not work robustly due to their complexity, are not flexible enough and seems excessive in context of using language IDs, so they are replaced with more plain and powerful (as it seems) language regular expressions section. The new equivalent configuration snippet should look like:
So, eventually the newly modified workspace configuration taken out of this pull request should be:
{ "environments": { "default": { "machines": { "typescript-ls-machine": { "attributes": { "memoryLimitBytes": "1073741824" }, "servers": { "ls": { "attributes": { "languageRegexes": "[ {\"languageId\":\"typescript\", \"regex\":\".*\\\\.(ts|tsx)$\"}]", "internal": "true", "id": "ts-id", "type": "ls" }, "port": "4417", "protocol": "tcp" } }, "volumes": { "projects": { "path": "/projects" } }, "installers": [ "org.eclipse.che.exec", "org.eclipse.che.terminal" ], "env": {} }, "dev-machine": { "attributes": { "memoryLimitBytes": "2147483648" }, "servers": { "tomcat8-debug": { "attributes": { "some": "attribute" }, "port": "8000", "protocol": "http" }, "codeserver": { "attributes": {}, "port": "9876", "protocol": "http" }, "tomcat8": { "attributes": {}, "port": "8080", "protocol": "http" } }, "volumes": { "projects": { "path": "/projects" } }, "installers": [ "org.eclipse.che.exec", "org.eclipse.che.terminal", "org.eclipse.che.ws-agent", "org.eclipse.che.ssh" ], "env": {} } }, "recipe": { "type": "compose", "content": "services:\n typescript-ls-machine:\n image: eivantsov/tsls:0.1.11\n environment:\n MYSQL_ROOT_PASSWORD: password\n MYSQL_DATABASE: petclinic\n MYSQL_USER: petclinic\n MYSQL_PASSWORD: password\n mem_limit: 1073741824\n dev-machine:\n image: eclipse/ubuntu_jdk8\n mem_limit: 2147483648\n depends_on:\n - typescript-ls-machine\n", "contentType": "application/x-yaml" } } }, "defaultEnv": "default", "name": "TypeScript_LS_Demo", "projects": [ { "name": "pc", "attributes": { "contribute_to_branch": [ "mysql" ], "commands": [ "{\"commandLine\":\"mvn -f /projects/pc clean install -DskipTests\", \"name\":\"pc: build\", \"attributes\":{\"previewUrl\":\"\", \"goal\":\"Build\"}, \"type\":\"mvn\"}", "{\"commandLine\":\"$TOMCAT_HOME/bin/catalina.sh stop\", \"name\":\"pc: stop tomcat\", \"attributes\":{\"previewUrl\":\"\", \"goal\":\"Run\"}, \"type\":\"custom\"}", "{\"commandLine\":\"mvn -f /projects/pc clean install -DskipTests \\ncp /projects/pc/target/*.war $TOMCAT_HOME/webapps/ROOT.war \\n$TOMCAT_HOME/bin/catalina.sh run 2>&1\", \"name\":\"pc: build and deploy\", \"attributes\":{\"previewUrl\":\"${server.tomcat8}\", \"goal\":\"Run\"}, \"type\":\"mvn\"}", "{\"commandLine\":\"mvn -f /projects/pc clean install -DskipTests \\ncp /projects/pc/target/*.war $TOMCAT_HOME/webapps/ROOT.war \\n$TOMCAT_HOME/bin/catalina.sh jpda run 2>&1\", \"name\":\"pc: debug\", \"attributes\":{\"previewUrl\":\"${server.tomcat8}\", \"goal\":\"Debug\"}, \"type\":\"mvn\"}" ] }, "links": [], "type": "maven", "source": { "location": "https://github.com/che-samples/web-java-spring-petclinic.git", "type": "git", "parameters": { "branch": "mysql" } }, "path": "/pc", "problems": [], "mixins": [ "pullrequest" ] } ], "commands": [ { "commandLine": "mysql -u $MYSQL_USER -p$MYSQL_PASSWORD -e 'show databases;'", "name": "show databases", "attributes": {}, "type": "custom" } ], "links": [] }Guice language server configuration
In order to keep all related to language server configuration in one place new interface
org.eclipse.che.api.languageserver.LanguageServerConfigis introduced. It consists of several sections that define how language server is matched upon workspace item's paths, howLangaugeServerinstance is created, and how the communication betweenLanguageServerand local language server process or remote language server happens. Those aspects are correspondingly represented byorg.eclipse.che.api.languageserver.LanguageServerConfig.RegexProvider,org.eclipse.che.api.languageserver.LanguageServerConfig.InstanceProviderandorg.eclipse.che.api.languageserver.LanguageServerConfig.CommunicationProviderand must be configured to run the language server.The simplest example of such configuration is
org.eclipse.che.plugin.php.languageserver.PhpLanguageServerConfig:Where
DefaultInstanceProvideris the default implementation that reflects most common way ofLangaugeServerinstance creation and is suitable for most cases but can be replaced by a custom implementation where needed.ProcessCommunicationProviderprovides appropriateInputStreamandOutputStreaminstances for communication of proxy instance and language server, along with lazy language server process start.There is a specific binding in Guice for such configurations:
Where
"org.eclipse.che.plugin.php.languageserver"is the identifier of a corresponding language server.In case of adding language server through workspace configuration the same approach is used: a corresponding instance of
LangaugeServerConfigis automatically created,org.eclipse.che.api.languageserver.SocketCommunicationProvideris used instead.Language descriptions
Since
org.eclipse.che.api.languageserver.shared.model.LanguageDescriptionmostly used as a container for language related mime types, extensions and highlighting configuration which are not directly related to language servers (as of 3.6.0 LSP specification) but to editor configuration, all language descriptions moved to client side, the most basic example:Language server matching
It unfortunately appeared that the implementation of
LanguageServerRegistryin its current state is not as effective as expected and has some inconvenient limitations for language server matching routines, that is why in this pull request it is partially removed and will probably require improvements.First of all, despite an excellent idea of using numbers to accurately find matching language server score, it can have only one of three states -
0,5,10- and all of them are treated as matches and impacts only language servers list ordering for multi-language cases. In general nothing significant will change if we replace those magic numbers with two states - match or not match. The problem is aggravated by the fact that matching takes into account only paths and languages, not respecting feature (capability) support level, so it is quite possible to have a big matching score with rather poor functionality, seems not to be what we need.TextDocumentServicedoes not really uses language server matching for all its manipulations. While language servers comes sorted according to their match score, they are in most cases ran in parallel, which neutralizes sorting effect. Beside that it isTextDocumentServiceexactly the place where we would better benefit of specific feature support scores but not general language server scores.Finally, this matching approach does not fit quite well new simplified language server matching algorithms, because all those language ids, extensions, names, paths are to be replaced with regexes.