Skip to content
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

StreamConnectionProvider: getInitializationOptions with initial document URI #684

Open
ghentschke opened this issue Jun 5, 2023 · 7 comments

Comments

@ghentschke
Copy link
Contributor

ghentschke commented Jun 5, 2023

In order to determine the initialization options for a LS, the initial document or its uri would be very helpful in org.eclipse.cdt.lsp.internal.server.CLanguageServerStreamConnectionProvider.getInitializationOptions(URI).
In order to prevent an API change, a new method
org.eclipse.cdt.lsp.internal.server.CLanguageServerStreamConnectionProvider.getInitializationOptionsForDocument(IDocument initialDocument) could be provided.

Since the initialProject or initialPath can be outdated as mentioned in #683 as well, a solution could be to provide a LanguageServerWrapper start method with document/path/uri as argument: org.eclipse.lsp4e.LanguageServerWrapper.start(URI initialDocument) as already mentioned in #157 by @sebthom.

IMO an initialDocument would be better than an initialProject or initialPath. Both could be determined by the document.

What are your opinions?

@bastiandoetsch
Copy link
Contributor

bastiandoetsch commented Jun 5, 2023

An initialDocument wouldn't hit our (company's) use case to start the language server independent of a document for a project, as we determine diagnostics in the background that concern the whole project, not only one document. Basically, being able to start & stop the language server with (or in case of a singleton server without) any argument would be actually quite helpful.

Regarding the initializationOptions, they should just be historized when the LS is started, as they are read-only. I right now don't see the value of having a document there, as the language server instance is not document specific. Then they could be made accessible with a public method.

@ghentschke
Copy link
Contributor Author

ghentschke commented Jun 5, 2023

Basically, being able to start & stop the language server with (or in case of a singleton server without) any argument would be actually quite helpful.

How do you handle outdated initialProject/Path as described in #683 ?

I right now don't see the value of having a document there, as the language server instance is not document specific.

The use case is a C/C++ language server: I want to set the fallback options for the clangd LS. These fallback compiler flags will be determined by fetching the compiler settings for the first opened file. Therefor I need the uri/path or document of the first file to be opened.

@bastiandoetsch
Copy link
Contributor

bastiandoetsch commented Jun 5, 2023

How do you handle outdated initialProject/Path as described in #683 ?

I may not be understanding the issue - on startup of a language server, the initializationOptions contain the rootURI and workspace folders. We use these values. When the workspaceFolders change, we listen to the change method and act accordingly in the language server.

The use case is a C/C++ language server: I want to set the fallback options for the clangd LS. These fallback compiler flags will be determined by fetching the compiler settings for the first opened file. Therefor I need the uri/path or document of the first file to be opened.

Hm, why can't you just fetch the compiler settings of the file that starts up a new instance? I really only half understand what is required here, but why can't a newly started instance set the fallback options for that instance?

@ghentschke
Copy link
Contributor Author

ghentschke commented Jun 5, 2023

When the workspaceFolders change, we listen to the change method and act accordingly in the language server.

Okay. I have seen the following problem with start()/stop() of a LS and the lifetime of its wrapper:
By closing all opened editors the LS will be terminated after a given timeout. Now open a file from a different project/path.
The new LS will inherit the initialProject or path from the prior LS during start(), since the lifetime of the wrapper is longer than its server.

Hm, why can't you just fetch the compiler settings of the file that starts up a new instance?

That's what I want. But I need the file info before the new instance gets started (before StreamConnectionProvider.start() method is called in org.eclipse.lsp4e.LanguageServerWrapper.start() ).
Something like this:

var options = Optional.ofNullable(this.lspStreamProvider.getInitializationOptions(rootURI)).orElse(this.lspStreamProvider.getInitializationOptionsFromUri(initialFile));
initParams.setInitializationOptions(options);
try {
	lspStreamProvider.start();
} catch (IOException e) {
	throw new RuntimeException(e);
}

@bastiandoetsch
Copy link
Contributor

Inheriting an old URI/project path sounds like a bug - this state should not be retained, in my opinion.

@ghentschke
Copy link
Contributor Author

Inheriting an old URI/project path sounds like a bug - this state should not be retained, in my opinion.

I agree. That's why the start() method in the wrapper needs an argument for rootUri:

public synchronized void start(URI rootUri) throws IOException  {
...

@mickaelistria
Copy link
Contributor

As a 1st iteration, we can start by setting the rootUri to null when calling stop

ghentschke added a commit to ghentschke/lsp4e-bachmann-fixes that referenced this issue Jun 8, 2023
For some LS the determination of LS initialization options depends on
the first uri to be opened and not on the first project or path.
Therefor the
getInitializationOptionsFromUri(URI) method has been added to the
StreamConnectionProvider interface. It will be called when
getInitializationOptions(URI) returns null.

fixes eclipse#683 eclipse#684
ghentschke added a commit to ghentschke/lsp4e-bachmann-fixes that referenced this issue Jun 8, 2023
For some LS the determination of LS initialization options depends on
the first uri to be opened and not on the first project or path.
Therefor the
getInitializationOptionsFromUri(URI) method has been added to the
StreamConnectionProvider interface. It will be called when
getInitializationOptions(URI) returns null.

fixes eclipse#683 eclipse#684
ghentschke added a commit to ghentschke/lsp4e-bachmann-fixes that referenced this issue Jun 8, 2023
For some LS the determination of LS initialization options depends on
the first uri to be opened and not on the first project or path.
Therefor the
getInitializationOptionsFromUri(URI) method has been added to the
StreamConnectionProvider interface. It will be called when
getInitializationOptions(URI) returns null.

fixes eclipse#683 eclipse#684
ghentschke added a commit to ghentschke/lsp4e-bachmann-fixes that referenced this issue Jun 12, 2023
For some LS the determination of LS initialization options depends on
the first uri to be opened and not on the first project or path.
Therefor the
getInitializationOptionsFromUri(URI) method has been added to the
StreamConnectionProvider interface. It will be called when
getInitializationOptions(URI) returns null.

fixes eclipse#683 eclipse#684
ghentschke added a commit to ghentschke/lsp4e-bachmann-fixes that referenced this issue Jun 12, 2023
For some LS the determination of LS initialization options depends on
the first uri to be opened and not on the first project or path.
Therefor the
getInitializationOptionsFromUri(URI) method has been added to the
StreamConnectionProvider interface. It will be called when
getInitializationOptions(URI) returns null.

fixes eclipse#683 eclipse#684
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants