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

Warning about overlapping types for Either #624

Closed
jonahgraham opened this issue May 17, 2022 · 8 comments · Fixed by #627
Closed

Warning about overlapping types for Either #624

jonahgraham opened this issue May 17, 2022 · 8 comments · Fixed by #627
Assignees
Milestone

Comments

@jonahgraham
Copy link
Contributor

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 eec8a7d in #618

Does the above one need fixing before we release 0.13.0?

cc: @KamasamaK @cdietrich @pisv

@jonahgraham jonahgraham added this to the v0.13.0 milestone May 17, 2022
@KamasamaK
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

cdietrich commented May 18, 2022

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

@cdietrich
Copy link
Contributor

will do a pr

cdietrich added a commit that referenced this issue May 18, 2022
Harmonized Naming convention and package structure

Fixes #624

Signed-off-by: Christian Dietrich <christian.dietrich@itemis.de>
@KamasamaK
Copy link
Contributor

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
Copy link
Contributor

cdietrich commented May 18, 2022

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

@cdietrich cdietrich self-assigned this May 18, 2022
@pisv
Copy link
Contributor

pisv commented May 18, 2022

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

cdietrich added a commit that referenced this issue May 18, 2022
Harmonized Naming convention and package structure

Fixes #624

Signed-off-by: Christian Dietrich <christian.dietrich@itemis.de>
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

Successfully merging a pull request may close this issue.

4 participants