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
599 stars 143 forks source link

Using internals which may not be visible #738

Closed jonahgraham closed 1 year ago

jonahgraham commented 1 year ago

In MessageTypeAdapter we are using an internal Gson type that is exported in Orbit's gson rebundling (as x-internal) but not in the manifest of the upstream gson.

https://github.com/eclipse-lsp4j/lsp4j/blob/e32c63b7b1cea5b777bd0d99c4045f4932ad3ff7/org.eclipse.lsp4j.jsonrpc/src/main/java/org/eclipse/lsp4j/jsonrpc/json/adapters/MessageTypeAdapter.java#L46

This forces consumers of jsonrpc (within an OSGi runtime) to use the Orbit version.

jonahgraham commented 1 year ago

@cdietrich / @dhuebner looking for a second opinion - is this serious enough that we should do a quick next release? If we (collectively) can fix it immediately then I can do the release ASAP.

cdietrich commented 1 year ago

what i dont understand. why is this not seen in tests. we run non osgi tests => is this for persons using maven gson in osgi?

cdietrich commented 1 year ago

i also would prefer a .1 release otherwise we would need a new xtext release

cdietrich commented 1 year ago

i am also not aware of people using lsp4j in osgi context am not sure what about lsp4e

jonahgraham commented 1 year ago

what i dont understand. why is this not seen in tests. we run non osgi tests

Without OSGi the LSP4J can access that class (I think? as I don't know how JPMS fits in to the picture, if at all)

On more reflection I don't think this is critical, just annoying as it means LSP4J is forcing LSP4E to use the Orbit gson. We should aim to fix soon, but I don't think we need to respin for this.

merks commented 1 year ago

It’s not critical. Next normal release will be soon enough.

merks commented 1 year ago

It’s not critical. Next normal release will be soon enough.

jonahgraham commented 1 year ago

Thank you @merks for weighing in and giving me that confirmation!

dhuebner commented 1 year ago

@jonahgraham I can prepare a "fix" for this by copying the gson implementation of Primitives.isPrimitive and Primitives.isWrapperType

dhuebner commented 1 year ago

@jonahgraham I've opened a PR #739

jonahgraham commented 1 year ago

@jonahgraham I can prepare a "fix" for this by copying the gson implementation of Primitives.isPrimitive and Primitives.isWrapperType

I asked Wayne Beaton review this because I was concerned about the copying but his assessment is in this case it is fine.