search

eclipse-lsp4j / lsp4j

A Java implementation of the language server protocol intended to be consumed by tools and language servers implemented in Java.
https://eclipse.org/lsp4j
Other
613 stars 145 forks source link

Warning about overlapping types for Either #624

Closed jonahgraham closed 2 years ago

jonahgraham commented 2 years ago

There is a warning in LSP4J on this line:

https://github.com/eclipse/lsp4j/blob/3f1f2bdc39c0c1d779db7c32d52787b38f2ddaec/org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend#L3114

WARNING:The json types of an Either must be distinct. (file:/home/jenkins/agent/workspace/lsp4j-multi-build_main/org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend line : 3114 column : 36)

There was a similar issue in DebugProtocol that was fixed by eec8a7d4cf010f44c2e60a92b51dce77d733191c in #618

Does the above one need fixing before we release 0.13.0?

cc: @KamasamaK @cdietrich @pisv

KamasamaK commented 2 years ago

The one for RestartArguments.arguments makes some sense, but I fail to see the overlap between Range and InsertReplaceRange. I'm curious what it's finding to determine lack of distinctiveness here.

jonahgraham commented 2 years ago

I see what you mean - I have some time carved out on Thursday when I will look into this if no one else has solved it by then.

cdietrich commented 2 years ago

there is alreay @JsonAdapter(CompletionItemTextEditTypeAdapter) i assume something similat be added there too

cdietrich commented 2 years ago

will do a pr

KamasamaK commented 2 years ago

there is alreay @JsonAdapter(CompletionItemTextEditTypeAdapter) i assume something similat be added there too

I think I created that one because of this warning too. At least both of those types have an overlapping property String newText. Although the other distinct properties are @NonNull so there shouldn't ever be ambiguity in practice. I'm just curious what determines when that warning is used. Either I'm missing something or I'm expecting too much "smarts" from whatever is emitting that warning.

cdietrich commented 2 years ago

yes this i did not understand, but it looks like it needs it for all Either<SomeObject,SomeOtherObject> cause it boils it down to object and then says both is object

pisv commented 2 years ago

it looks like it needs it for all Either<SomeObject,SomeOtherObject> cause it boils it down to object and then says both is object

+1, see the method implementation at https://github.com/eclipse/lsp4j/blob/3f1f2bdc39c0c1d779db7c32d52787b38f2ddaec/org.eclipse.lsp4j.jsonrpc/src/main/java/org/eclipse/lsp4j/jsonrpc/json/adapters/EitherTypeAdapter.java#L175

pisv commented 2 years ago

For the record, the warning is emitted from https://github.com/eclipse/lsp4j/blob/3f1f2bdc39c0c1d779db7c32d52787b38f2ddaec/org.eclipse.lsp4j.generator/src/main/java/org/eclipse/lsp4j/generator/JsonRpcDataProcessor.xtend#L92