search

haskell / lsp

Haskell library for the Microsoft Language Server Protocol
364 stars 90 forks source link

Fix #268 by introducing a MaybeN type #425

Closed thomasjm closed 1 year ago

thomasjm commented 2 years ago

Taking a stab at fixing #268 by introducing a new MaybeN type, which is just like Maybe except that Nothing values get encoded to JSON null. This is inspired by the existing List type.

This can be used to conform properly to the LSP spec, which sometimes specifies types like a | null and sometimes a? (i.e. omit the value when absent).

The downside of this approach is that for every field you convert from Maybe to MaybeN, you have to go through all usages of that field and change them. I've included an example of doing this for InitializeParams; for full compliance we would have to do the same for all Maybe fields that are supposed to be a | null.

Of course, it's unpleasant to expose this encoding detail in the field types and make all these changes. This seemed like the most straightforward way, but a cleaner approach might be possible with TH. For example, maybe we could extend makeExtendingDataType so that it takes extra arguments indicating which type of Maybe to use, and generates suitable FromJSON/ToJSON instances manually? FWIW a search through the LSP spec indicates about 40 cases of | null to handle.

michaelpj commented 2 years ago

I was actually thinking we could tackle this alongside https://github.com/haskell/lsp/issues/421. If we completely overhaul the type generation to go off the new metaModel.json, then we can also hopefully fix this issue in a systematic way.

thomasjm commented 2 years ago

Oh wow, that would be nice!

michaelpj commented 2 years ago

Sadly I don't know when I'm going to get to that :/ So maybe we should do this anyway.

michaelpj commented 2 years ago

Maybe I'll take a stab at it this weekend. If we're going to force everyone to change how they use the types, we might as well only make them do it once :laughing:

thomasjm commented 2 years ago

That would be awesome, feel free to hit me up if there's any way I can help.

michaelpj commented 2 years ago

@thomasjm how big a problem are these issues for you? I'm chipping away at the metamodel generation approach, but it'll take a while.

thomasjm commented 2 years ago

Well, every time I file one of these PRs it's because I've encountered a compliant LSP server in the wild that doesn't work with lsp-types :). I'm trying to work with a number of these.

It's not super urgent to merge these because I just use my own fork, but I thought I'd try to contribute them all back. I'm not sure if anyone else is experiencing LSP compliance pain. If the metamodel thing can bring compliance to 100% in one fell swoop it will be awesome for increasing confidence in this library.

michaelpj commented 1 year ago

Should be fixed in master