Resolve 2: Support for resolve in hls-hlint-plugin #3679
Conversation
Because the plugin-api does everything for us now.
| withIndefiniteProgress "Executing code action..." Cancellable $ do | ||
| resolveResult <- resolveProvider ideState pluginId ca | ||
| case resolveResult of | ||
| Right CodeAction {_edit = Just wedits } -> do |
There was a problem hiding this comment.
What happens if it has the command set? It probably shouldn't, right? But maybe we should fail or at least log there.
There was a problem hiding this comment.
Also, if it's cheap it would be nice to check that all the other fields of the code action are the same as the input one, i.e. the resolve handler only gave us an edit. In the more generalized case, we should check that the only fields that differ are the ones that the client says it can resolve. Then we can log if the handler does something wrong there.
There was a problem hiding this comment.
Well, I guess also the command and data fields should be unset?
There was a problem hiding this comment.
I think, according to the spec, we can only change the set of resolvable fields, so even though it would make sense to dump the data field, it still needs to stay the same as when we received it. (We also don't have the client capabilities so can't do anything with that). Finally in this case the only thing we can execute on is the returned WorkspaceEdit. We can return an error if any other field is changed, but not sure about the performance penalty of doing that.
There was a problem hiding this comment.
Can't we pass in the client caps here?
I don't know if it's worth doing the checking of field setting, it just seems like a way to sanity check plugins, which seems somewhat useful.
There was a problem hiding this comment.
Okay, I guess we can't pass in the client capabilities here, but we should be able to pull them from the monad itself, since the plugin handler is effectively in the Lsp monad already
There was a problem hiding this comment.
Ah I see, this is exactly the sort of thing we could do if we had the client caps when making the handlers!
| resolveResult <- resolveProvider ideState pluginId ca | ||
| case resolveResult of | ||
| Right CodeAction {_edit = Just wedits } -> do | ||
| _ <- sendRequest SMethod_WorkspaceApplyEdit (ApplyWorkspaceEditParams Nothing wedits) (\_ -> pure ()) |
There was a problem hiding this comment.
It seems like we probably should care a little about the response? e.g. we probably want to do something if we fail, like at least log
There was a problem hiding this comment.
That's quite hard here, as we can't use a recorder (That's defined in the ghcide package, so cyclic dependency), and we only have a callback to work with.
There was a problem hiding this comment.
Ugh. Okay, I guess we can't do much here, but that's rather unsatisfying. Maybe we need to move some of the recorder stuff into hls-plugin-api as the "lowest" package.
There was a problem hiding this comment.
I would really like to be able to do some logging in the generic functions you're defining, that way we can get somewhat-decent logging for lots of plugins by defining it in one place.
Commands and resolve act very similarly, and since hlint already heavily uses commands, I am choosing to first move everything to use commands, and then offer resolve if it's available, falling back to commands for clients that don't support it.