-
Notifications
You must be signed in to change notification settings - Fork 45
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
✨ Update LSP types to more recent version #324
Conversation
Saw that the test failed, will investigate on Monday |
1746aab
to
427b9c3
Compare
Signed-off-by: JonahSussman <[email protected]>
427b9c3
to
f6e0d36
Compare
@JonahSussman The test failure seems to be coming from a change in the base image, it is unrelated to your change. It's being fixed. |
Further work needs to be done on this before merging into the main branch. Some things aren't quite working as expected. I think I have to get rid of a lot of the overrides that are in tables.go |
@JonahSussman, do you want reviews before that is done, or do you want to wait? |
Signed-off-by: JonahSussman <[email protected]>
Signed-off-by: JonahSussman <[email protected]>
0b8c3bf
to
6982513
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me, is this copying in a new implementation because we need to modify things or is it possible to just import the library?
Location: protocol.OrPLocation_workspace_symbol{ | ||
Value: r, | ||
}, | ||
// Location: r |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accidental?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The former. Gopls does some stuff with the types that are gopls specific. A big example is the changes made in tables.go. I looked into importing the types but it was too janky.
And yep! 197 is accidental, will fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into importing the types but it was too janky.
Maybe this answers my question from https://github.com/konveyor/analyzer-lsp/pull/324/files#r1334766707. Were you trying to import the go.lsp.dev
and it was too janky?
@JonahSussman merge conflicts |
Signed-off-by: Jonah Sussman <[email protected]>
Resolved |
@@ -61,9 +61,9 @@ Flags: | |||
|
|||
## Code Base Starting Point | |||
|
|||
Using the LSP/Protocal from ACME https://github.com/fhs/acme-lsp and stripping out anything related to serving, proxy or anything. Just keeping the types for communication | |||
Using the LSP/Protocal from Golang https://github.com/golang/tools/tree/master/gopls/internal/lsp/protocol and stripping out anything related to serving, proxy or anything. Just keeping the types for communication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is something specific stopping us from being able to use https://pkg.go.dev/go.lsp.dev/protocol ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I'm aware, I don't think that you can import stuff from a Go library that's in a directory called 'internal'. Unfortunately, the types are in such a directory. If there's another way to get around this, I'd be really interested!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing something? that pkg was not an internal
library.
Problem
I was attempting to begin work on the
jedi-language-server
integration using the external provider binary. However, I kept running into an issue where the LSP server was returning an element that looked like"typeDefinitionProvider": {},
and Go refused to unmarshal it. Looking at the code, we can see why this would be an issue - the server is returning an object, but the corresponding field in Go is a boolean:analyzer-lsp/lsp/protocol/tsprotocol.go
Lines 876 to 881 in 6ad9f5e
At first blush, this appears to be an issue with the LSP server - it's returning a nonstandard response. However, upon further investigation in the spec, this field can have multiple types, it can be a boolean OR TypeDefinitionOptions OR TypeDefinitionRegistrationOptions. This is hinted at in the comments too.
The LSP Protocol types from ACME doesn't support this functionality. You can even see issues running into the exact same thing on their repo. They copied their Go types from gopls, and as a result are using a woefully out of date version. It appears that the "or" functionality was added sometime after the version we are using now was copied.
Solution
Instead, I modified the gopls type generator slightly (see lsp/protocol/generate/README.md) and plopped it in tree. Then all you need to do is run
go generate
and you're good. It downloads the latest metamodel.json from this repo, picks the release branch to use in main.go, and uses that as a reference to generate the types. This is the same way the types we currently use were generated, it just gives us the ability to more easily update them. As a result, the impact this should have overall should be minimal.The tool also generates custom JSON marshal and unmarshal code for every
interface{}
type generated, giving a bit more type safety than we have now. Most importantly, our Go types now conform to the LSP spec by allowing each field to be one of a couple types. This is a huge win for broader LSP server compatibility! (Hopefully!)Potential wrinkles
The only wrinkle is that you need to add "ExtendedClientCapilities map[string]interface{}
json:"extendedClientCapabilities"
" to the XInitializeParams struct every time you regenerate. It's only used twice in the entire codebase. If this field isn't really needed by the code, we should remove it.Would love to get everyone's thoughts on this! While I'm confident that this change is necessary in order to get more providers added, I also understand that this change could have more effects than I'm seeing. Ultimately, this would be a huge step forward in getting more LSP servers to work with the generic provider.