tliron / glsp

Language Server Protocol SDK for Go
Apache License 2.0
165 stars 22 forks source link

`ctx context.Context` is not propagated anywhere #27

Open nfx opened 6 months ago

nfx commented 6 months ago

i expected callbacks to have this shape:

TextDocumentCodeAction: func(ctx context.Context, context *glsp.Context, params *protocol.CodeActionParams) (any, error) {
...

or even just

TextDocumentCodeAction: func(ctx context.Context, params *protocol.CodeActionParams) (any, error) {
   gctx := glsp.FromContext(ctx)
   ...

or even just (you can generate concrete types with go generate ./... and not have loosely-typed lspctx.Notify(protocol.ServerTextDocumentPublishDiagnostics, &protocol.PublishDiagnosticsParams{

TextDocumentCodeAction: func(ctx context.Context, params *protocol.CodeActionParams) (any, error) {
   /// ..
   protocol.NotifyTextDocumentPublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{
    URI:         uri,
    Diagnostics: res.Diagnostics,
   })

....

but at the moment it's quite difficult to work with this library

nfx commented 6 months ago

you can parse the LSP spec with some markdown & https://github.com/dop251/goja and have things pretty code-generatable.

e.g. instead of TextDocumentCodeAction: func(ctx context.Context, params *protocol.CodeActionParams) (any, error) {, you can have clients to write func (x *myImpl) TextDocumentCodeAction(ctx context.Context, params *protocol.CodeActionParams) (protocol.CodeActionResponse, error) { if you have interfaces:

type TextDocumentCodeActions interface {
   TextDocumentCodeAction(context.Context, *CodeActionParams) (CodeActionResponse, error)
}
tliron commented 6 months ago

As for the first comment, that is a good idea. I think rather than change the entire API, I will add the Go context into the GLSP context as a field. What do you think? I'll submit a PR and let you review.

As for the second comment, it doesn't seem related to the first. :) Yes, generating much of this library automatically was considered, whether with goja or really anything else (a Go code generator doesn't actually have to be written in Go, you know). But I found that it would be quicker for me to do things manually, also as an exercise of reading the entire LSP spec and understanding it. The initial version of this library took just 2 days of work! If you have an idea of redoing this library via code generation, would be happy to see your implementation of such a generator.

nfx commented 6 months ago

it's just the convention for the Go ecosystem nowadays to have ctx context.Context as the first parameter of everything involved with IO 🤷‍♂️

tliron commented 6 months ago

it's just the convention for the Go ecosystem nowadays to have ctx context.Context as the first parameter of everything involved with IO 🤷‍♂️

Sure, but it's also bad practice to inject arbitrary user data into the Go context, which ostensibly is designed for IO cancellation. The user data feature was included in Go context to provide extra user data related to cancellation. Of course it's abused quite a lot. :)

I also think it's cumbersome to add more than one context to every function call. So, my humble design decision is to include an optional Go context into GLSP context, which is already provided.

I just merged the fix, I think it's very straightforward. Could you please take a look and test?