redhat-developer / lsp4ij

LSP Client for IntelliJ
Eclipse Public License 2.0
85 stars 16 forks source link

Exception raised if `workspace/configuration` request from server to client does not get passed a `section` #466

Open bzoracler opened 3 weeks ago

bzoracler commented 3 weeks ago

When retrieving configuration settings through workspace/configuration, the language server specifications allows leaving section unspecified:

export interface ConfigurationItem {
    /**
     * The scope to get the configuration section for.
     */
    scopeUri?: URI;

    /**
     * The configuration section asked for.
     */
    section?: string;
}

However, if section is not provided, LSP4IJ raises an exception1.

I think a default behaviour should be implemented when section is not specified - such as (1) returning the entire configuration object, which is my preference, or (2) returning null.

com.redhat.devtools.lsp4ij.client.LanguageClientImpl.findSettings

    protected Object findSettings(String section) {
        var config = createSettings();
+       if (section == null) {
+           return config;
+       }
        if (config instanceof JsonObject json) {
            String[] sections = section.split("[.]");
            return findSettings(sections, json);
        }
        return null;
    }

  1. Stack trace: https://github.com/redhat-developer/lsp4ij/blob/6a561e0c274211141a36b1c0eeeb1d3a056f1670/src/main/java/com/redhat/devtools/lsp4ij/client/LanguageClientImpl.java#L227

    Internal error: java.lang.NullPointerException: Cannot invoke "String.split(String)" because "section" is null
    
    java.util.concurrent.CompletionException: java.lang.NullPointerException: Cannot invoke "String.split(String)" because "section" is null
       at java.base/java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:315)
       at java.base/java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:320)
       at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1770)
       at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.exec(CompletableFuture.java:1760)
       at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:507)
       at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1491)
       at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:2073)
       at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:2035)
       at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:187)
    Caused by: java.lang.NullPointerException: Cannot invoke "String.split(String)" because "section" is null
       at com.redhat.devtools.lsp4ij.client.LanguageClientImpl.findSettings(LanguageClientImpl.java:227)
       at com.redhat.devtools.lsp4ij.client.LanguageClientImpl.lambda$configuration$9(LanguageClientImpl.java:203)
       at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1768)
       ... 6 more
angelozerr commented 3 weeks ago

Could you create a PR please

angelozerr commented 3 weeks ago

I think a default behaviour should be implemented when section is not specified - such as (1) returning the entire configuration object, which is my preference, or (2) returning null.

It would be nice to clarify that. Could you create an issue at https://github.com/microsoft/vscode-languageserver-node/issues please.

bzoracler commented 3 weeks ago

TL;DR the LSP specs leave the behaviour undefined, so the decision is up to LSP4IJ on what to do upon a null section request (but definitely not raise an exception). LSP4IJ must handle a null section, and IMO it should:

  1. Decide on a default behaviour if null is provided as the section;
  2. Advertise this as the default behaviour, informing server implementations that they can expect this behaviour if they request null;
  3. Have this default behaviour overridable if someone wants to make an LSP4IJ plugin.

From the specification (emphases mine):

A ConfigurationItem consists of the configuration section to ask for and an additional scope URI. The configuration section asked for is defined by the server and doesn’t necessarily need to correspond to the configuration store used by the client. So a server might ask for a configuration cpp.formatterOptions but the client stores the configuration in an XML store layout differently. It is up to the client to do the necessary conversion.

I interpret this as: The server must advertise what section it's going to request (such as null), and a compatible client must handle it. So, if the server requests a null section, it's up to the client what to return to the server; what the server/client should do for any particular configuration section (including null) isn't specified by the language server protocol.

In light of this, whether null is a sensible request from the server or not depends on the client; for generic clients which can configure one of multiple servers using a shared configuration file (e.g. VS Code's settings.json), it doesn't make sense for null to return the entire configuration as it would contain irrelevant items. LSP4IJ's user-defined Langauge Server UI isolates each language server with their own configuration settings, so returning the entire configuration may be a sensible interpretation if the server asks for null. However, since the server must advertise what it asks for, if the server advertises that it asks for null, it may not be generically compatible with other clients, but that's the server's implementation problem, not the client's.

See also some discussion about the uncertainty of what section means in https://github.com/microsoft/language-server-protocol/issues/972.