Resolve refactoring

[joyfulmantis]

I had higher hopes on what I could do here but ended up just implementing a ResolveFunction which makes what used to look like this.

resolveProvider :: PluginMethodHandler IdeState 'Method_CodeActionResolve
resolveProvider ideState pluginId codeAction@(CodeAction _ _ _ _ _ _ _ (Just resolveData)) =

Now look something like this

resolveProvider :: ResolveFunction IdeState CustomResolveType 'Method_CodeActionResolve
resolveProvider ideState pluginId codeAction uri (CustomResolveType int) =

These resolve functions are complied with the mkResoveHandler, which is now the only way to write resolve handlers.

Things I still want to investigate, and possibly handle:

• Does LSP send resolve requests even if the data field is empty? If so we need to create a default resolve handler for each that all resolve requests with empty fields are routed to, whose job is just to reflect its params.

[joyfulmantis]

Here's an example of how our function types and arguments change with the new resolve logic.

Before a resolve provider looked something like this:

resolveProvider :: PluginMethodHandler IdeState 'Method_CodeActionResolve
resolveProvider ideState pluginId codeAction@(CodeAction _ _ _ _ _ _ _ (Just resolveData)) =

Now it will look something like this

resolveProvider :: ResolveFunction IdeState CustomResolveType 'Method_CodeActionResolve
resolveProvider ideState pluginId codeAction uri (CustomResolveType int) =

The most obvious differences are now we always have a URI passed to us (necessary for almost everything, and we can start using it right away, without having to decode any data), and our data is already decoded into our custom type for us. Additionally, all responses that can be resolved will be (invisible to the plugin) wrapped in information that allows us to resolve specifically for that plugin only (it's also what allows us to provide the uri). We also no longer use pluginEnabled and combineResponses functions, or the type classes that provide them for any of the resolve methods.

[michaelpj]

At a high level I like the interface for plugins. I'm not sure why we need a separate entry in the plugin descriptor though? Why can't we just have a function that turns a ResolveFunction into a PluginHandlers like before? I don't think that the stuff in combineResponses and so on was so bad - I think with the identifying information put into the data we had an okay solution there.

[joyfulmantis]

So the ResolveFunction, and how they are handled is pretty much equivalent to what's already done for Commands. And since Commands already had their own logic for being handled, I modeled the resolve logic off of them. Commands have an extra commandId that allows plugins to declare multiple commands per plugin. However the resolve stuff has extra complexity in that it has to allow plugins to register resolve handlers for different methods, hence the necessary usage of DMap. Both the ResolveFunction for resolve based functions and the CommandFunction for commands can be implemented on top of the existing Plugin Handler infrastructure.

Why were commands originally handled differently, and what are the advantages and disadvantages of the different approaches?
I'll list below what I think are some of the non-disadvantages, disadvantages, advantages, and non-advantages.

Non-disadvantage: Extra field in the PluginDescriptor. While this at first may seem like a disadvantage, no one actually creates a plugin descriptor from scratch, everyone just edits a default one with the stuff that they want, so the only time anyone would have to deal with it is if they actually want to provide resolve handlers themselves.

Disadvantage: Technical debt. With commands and resolve handlers handled separately from the plugin handlers architecture, they need certain functions and data types, both in the hls-plugin-api, and in the core hls section of ghcide. I should add though that in the grand scheme of things, it's not large. For the resolve stuff something like 70 lines in hls-plugin-api's Types.hs and another around 50 lines in ghcide's Hls.hs. We also get to delete a bunch of lines from Types.hs (everything resolve related in the two main typeclasses there), so even If we build on top of the plugin handler architecture we could probably save a few lines, but not that many.

Advantage: Less hacky, and better safety that nothing goes wrong. The reason the combineResponses is called combineResponses.. is because it's meant to... combine responses. While with combine responses it's possible to hackily just take the first item in the list, it's not an ideal solution. (What if more than one plugin returns a response, for example, because it fails by not changing anything rather than just returning an error (something that was previously done with ghcide completions I might add). Then we have the combineErrors, which is currently completely inadequate for the job of returning a response if everything errors out (It currently returns an internal error with the contents being a (show xs)!) It's not even clear how we would be able to single out only the error we want to use as a response. There are possibilities -- we could have resolve handlers return a specific error if the request is not for them, and filter those out, but that is super hacky, and you just need one plugin forgetting to return the right error to cause mayham for everyone.
There is stuff we can enforce if we require everyone to use functions to build the handlers, but the key thing is if the resolve methods are built on top of the plugin handler infrastructure, there is nothing we can do from people running roughshod of our attempts to enforce guarantees on how things are done with helper methods by just using a normal mkPluginHandler with the resolve method.

Non? advantage: Theoretically there is a performance improvement, albeit bound to be small, with the current approach we are only decoding the data field once, and only passing that on to one plugin, If we used the pluginHandler architecture we would have to decode the data field n times for n plugins.

[michaelpj]

Checking a few things in my understanding:

• If we don't provide an instance of PluginMethod, then people can't put codeAction/resolve handlers into the normal plugin handlers, right?
• But people can put a codeAction handler into the PluginMethods, right? I guess that's fine.

However the resolve stuff has extra complexity in that it has to allow plugins to register resolve handlers for different methods, hence the necessary usage of DMap.

Right, so I guess we could have different ResolveFunctions for all of the things that could get resolved, but that would be pretty gross.

Perhaps a bad idea: what if we made PluginHandler a sum type? With the alternatives being either the handlers we have today or a resolve handler? Would that let us reuse more of the machinery? We'd have to deal with the situation where we have multiple handlers, some of which are PluginHandlers and some of which are ResolveHandlers, but surely the current code must deal with that also?

TBH, I'm not sure whether there's any reason not to do the same for command handlers... I don't have this fully loaded into my head. But at least it seems like what you're doing with the resolve handlers is closer to what's being done for the other handlers?

The reason the combineResponses is called combineResponses.. is because it's meant to... combine responses. While with combine responses it's possible to hackily just take the first item in the list, it's not an ideal solution.

Isn't your code basically doing the same thing? https://github.com/haskell/haskell-language-server/pull/3688/files#diff-7a4ae7207d77fe6892ad9315e62ebdc3ea929941d0e2d9a78e18074422657b27R240

i.e. only taking the first thing that comes up? Yes, you're filtering by the plugin id, but you might still have multiple resolve functions! And the existing code also filters with pluginEnabled... could we not merge the filtering if we went the sum-type route?

[joyfulmantis]

Okay, I completely dumped all extra logic to handle resolve stuff and wrote a mkResolveHelper that turns a ResolveFunction into a normal PluginHandlers. With the pluginEnabled logic we only route requests to the plugin listed in the data field, so as long as no plugin has more than one resolve method listed per resolve method then we should only get one potential response which should work with the current combineResponses and combineErrors.

[joyfulmantis]

So his clients will ask us to resolve even if there is no data set, but currently, we respond with the "no plugin enabled" error message, which is of type InvalidRequest, and that doesn't seem to show an error for least vscode, so we should be fine there.

[joyfulmantis]

Here we are unwrapping the PluginResolveData, to make it transparent to the resolveHandler.) However, there is one really weird bug here, where seemingly f is called with the old "params", not the updated "newParams". Have no clue why that is though.

[michaelpj]

Hmmm... would be good to track that down. A good case for having a logger in here - then you could log what's going on to see :)

[michaelpj]

LGTM!

[michaelpj]

Oh right, I do think this module could do with a Note explaining the big picture of how we make the handlers all work nicely together.

[michaelpj]

Okay sorry, I do actually have more comments 😅

[michaelpj]

These should probably go in Resolve.hs?

[joyfulmantis]

I left all the core Resolve stuff in Types, and moved the optional handler makers to Resolve. I am happy to move everything to resolve thought if you think that's better.

[michaelpj]

unclear what "no extra data" means here

[joyfulmantis]

I'm not sure either 🫤

[michaelpj]

So this is one of the cases where we would definitely want to say we are choosing not to respond rather than erroring !

[joyfulmantis]

Actually pluginEnabled should prevent us from getting to this step, so if this is happening it probably means something is pretty broken

[michaelpj]

use the aeson error!

[michaelpj]

Also I don't think this should be using the "invalidRequest" message?

[joyfulmantis]

Yeah, maybe InternalError would work better here

[michaelpj]

Do we want to give a different error if the data is missing?

[joyfulmantis]

pluginEnabled should only route the request to us if the data field is full and decodes to PluginResolveData.

[michaelpj]

Hmmm... would be good to track that down. A good case for having a logger in here - then you could log what's going on to see :)

[michaelpj]

don't you have a test helper for this? I guess it's in lsp and we need to do a release

[joyfulmantis]

Yes it's part of the lsp-test pr I did.

[michaelpj]

Hmmm... couldn't we push this stuff into the helper functions we have for making such handlers? Then, yes, you need to use those if you want to get resolve working, but if you don't care you can just write a bare handler and we won't insert extra stuff if you haven't asked for it?

[joyfulmantis]

We're case matching on the method provided, and only actually wrapping the data field if it has anything in it, and the data field is only ever used for resolve. Otherwise we would need another specific function mkPluginHandlerThatSupportsResolve, wich imo would be kinda ugly

[joyfulmantis]

The helpers in Resolve.hs are opinionated ways to provide fallback to resloveless methods if not supported, but it makes sense to support plugin users who want to write their own fallback logic. For example code lenses don't need fallbacks, and thus currently use mkPluginHandler directly even though they support resolve.

[michaelpj]

Right, so I am suggesting something just like that, I was even thinking more specific: mkCodeLensHandlerWithResolve or something. I don't think it's so bad for people to have to call a specific function to opt in to the magic?