Incorrect return type for textDocument/documentSymbol

[kittaakos]

According to the LSP specification the return type of the textDocument/documentSymbol should be the type of DocumentSymbol[] | SymbolInformation[] | null.

The current Java API has List<Either<SymbolInformation, DocumentSymbol>> return type which is equivalent to (DocumentSymbol | SymbolInformation)[] | null.

It should be Either<List<DocumentSymbol>, List<SymbolInformation>> instead to comply with the protocol.

I think this is a bug.

[spoenemann]

I think it is a bug in the protocol.

In the initial implementation done in #217, the type was declared as you are suggesting. I later changed that to the more relaxed type because

• it is more consistent with other services such as codeAction, which has a return type of (Command | CodeAction)[] | null, and
• it is much easier to handle. From the perspective of a LanguageClient, the result of a documentSymbol request has to be parsed into the Java equivalent. If we would take the strict implementation, we'd have to check that the type of all array elements is the same, and throw some parse exception if it is not. I think it makes more sense to check this on a higher level, i.e. to be more relaxed while parsing and to build some semantic checks into the services.

[kittaakos]

In the initial implementation done in #217, the type was declared as you are suggesting. I later changed that to the more relaxed type because

it is more consistent with other services such as codeAction, which has a return type of (Command | CodeAction)[] | null, and
it is much easier to handle. From the perspective of a LanguageClient, the result of a documentSymbol request has to be parsed into the Java equivalent.

This all makes sense, but I think the difference between the codeAction and the documentSymbol is that you get what the client is capable of when you ask for the symbols. (Assuming that the LS can provide both.) In other words, if

textDocument.documentSymbol.hierarchicalDocumentSymbolSupport = true

then you get an array of DocumentSymbol. Otherwise, an array of SymbolInformation. If the client cannot handle the hierarchical format or the LS cannot provide it, you get the SymbolInformation. But never mixed.

---

For the codeActions, it is rather an optional performance optimization. From here:
"[...] server providers should be aware that if the code action is expensive to compute or the edits are huge it might still be beneficial if the result is simply a command [...]"

So it makes sense to mix the Commands and CodeActions in the same result. I do not see the benefit of mixing the DocumentSymbols and the SymbolInformations. Maybe one has a use-case for that.

---

If we would take the strict implementation, we'd have to check that the type of all array elements is the same, and throw some parse exception if it is not.

I think this is not the best analogy. Let's consider we would like to do a textDocument/codeAction with the following parameter:

const params: CodeActionParams = { /* initialize */ };
params.context.only = ['quickfix', 'refactor'];

We are not checking and throwing a runtime exception if any of the CodeActions of the result has a different kind than requested by the params. So it can be, for instance, source. Nothing happens.

---

How should we proceed with this? Does this worth a follow-up question (as a GH issue) in the Microsoft/language-server-protocol repository?

[akosyakov]

Returning a mixed list of symbol informations and document symbols won't work properly in VS Code, clients are expecting to have either one or another:

• https://github.com/Microsoft/vscode-languageserver-node/blob/b065c9c3a7b99ace488452fa59009be70d63def1/client/src/client.ts#L1620-L1625
• https://code.visualstudio.com/docs/extensionAPI/vscode-api#DocumentSymbolProvider

If it is hard to parse on our side, I think we should ask on the LSP repo, maybe they will be willing to change it and adopt in VS Code as well. In order to parse we can check each element if it is a document symbol or a symbol information, and if one is, then assume that others as well.

[tsmaeder]

@spoenemann it may be easier to implement in lsp4j, but handling the result is actually easier if you get a Either<List<X>, List<Y>> then dispatching on the member types. Also, one has to reference the spec to justify assuming that all members are of the same type when handling the results. If the signature was like specified in LSP, it would be obvious from the API. As an example, you already managed to confuse @akosyakov, for instance ;-)

[kittaakos]

it may be easier to implement in lsp4j, but handling the result is actually easier if you get a Either<List<X>, List<Y>> then dispatching on the member types.

It should not matter what is easier to implement or to consume, but what is defined in the specification. As @akosyakov pointed out, if we offer the current LSP4J API to clients as is, their LS implementation could break at runtime.

Although this comment clearly states the desired return type of the textDocument/documentSymbol language feature, I would like to ask @dbaeumer to chime in with his opinion, and confirm whether this is a bug in the protocol or in LSP4J.

[spoenemann]

It should not matter what is easier to implement or to consume, but what is defined in the specification.

The specification is clearly designed for TypeScript. Implementing it 100% correctly in Java is not possible because of the heavy usage of the TypeScript type system. But since the underlying message format is just JSON, it doesn't matter so much how close the Java type definitions are to the TypeScript types as long as we can produce compatible JSON data.

[spoenemann]

Otherwise, I'm fine with changing the response type to the original one as requested, but we should think about whether breaking the API again is really worth it.

[kittaakos]

breaking the API again is really worth it.

Otherwise, it can break at runtime, that's my concern.

[svenefftinge]

I agree with @spoenemann, it might be more correct and constraining to change it as suggested, but it will break existing clients. Nobody would return a heterogenous list here anyway. A middle ground would be to add that constraint to the JavaDoc instead of changing the signature.

[dbaeumer]

No, the decision to only allow DocumentSymbol[] | SymbolInformation[] was on purpose. And as pointed out correctly DocumentSymbol[] is only a valid return value if textDocument.documentSymbol.hierarchicalDocumentSymbolSupport = true.

We decided that way since the first one allows for a hierarchical display and the values can be used in an outline view. The first one is basically a flat list. If we mix them it is not clear how we would display them.

[kittaakos]

Thanks for the clarification, @dbaeumer.

A middle ground would be to add that constraint to the JavaDoc instead of changing the signature.

@svenefftinge, are we good with this approach? I adapt this GH issue and take care of the JavaDoc updates.

[cdietrich]

@kittaakos so we will keep the current api?

[kittaakos]

Yes, that's the plan. I still need to update the JavaDoc for the 0.6.0 release. Do you have any concerns, objections @cdietrich?

[cdietrich]

no i just want to verify we need no action in Xtext for that