Catch exceptions in commands and use lsp null #3696
Conversation
joyfulmantis
requested review from
konn,
santiweight,
July541,
pepeiborra,
Ailrun and
berberman
as code owners
There was a problem hiding this comment.
Can we add a Note explaining our exception handling strategy? Then we can add to it as we go. Something like:
{- Note [Exception handling in plugins]
Plugins run in LspM, and so have access to IO. This means they are likely to throw exceptions, even if only by accident or through calling libraries that throw exceptions.
Ultimately, we're running a bunch of less-trusted IO code, so we should be robust to
it throwing.
We don't want these to bring down HLS. So we catch and log exceptions wherever we run
a handler defined in a plugin.
The flip side of this is that it's okay for plugins to throw exceptions as a way of signalling
failure!
-}
| => (SomeException -> PluginId -> T.Text) | ||
| -> String -- ^ label | ||
| => (PluginId -> SMethod method -> SomeException -> T.Text) | ||
| -> SMethod method -- ^ label |
| @@ -177,9 +191,9 @@ executeCommandHandlers recorder ecs = requestHandler SMethod_WorkspaceExecuteCom | |||
| -- If we have a command, continue to execute it | |||
| Just (Command _ innerCmdId innerArgs) | |||
| -> execCmd ide (ExecuteCommandParams Nothing innerCmdId innerArgs) | |||
| Nothing -> return $ Right $ InL A.Null | |||
| Nothing -> return $ Right $ InR Null | |||
There was a problem hiding this comment.
This is fine but I it's literally the same behaviour. The upstream spec is weird, I made an issue: microsoft/language-server-protocol#1766
| @@ -873,7 +873,7 @@ data PluginCommand ideState = forall a. (FromJSON a) => | |||
| type CommandFunction ideState a | |||
| = ideState | |||
| -> a | |||
| -> LspM Config (Either ResponseError Value) | |||
| -> LspM Config (Either ResponseError (Value |? Null)) | |||
There was a problem hiding this comment.
tbh, since they're represented the same, I'm not sure this really adds much except making things more complex for the users
There was a problem hiding this comment.
I would say less complex. LSP Types are used everywhere, and now command users don't need to use Aeson if they don't use it anywhere else (Pretty much every command returns null).
There was a problem hiding this comment.
It doesn't matter too much, but I would hope that we can fix this upstream, and then we'd have to go back again...
There was a problem hiding this comment.
Good point. Is there any reason why we can't use Aeson.Null for Null in lsp-types, perhaps by reexporting it instead of declaring our own? that would remove a source of name conflicts.
There was a problem hiding this comment.
Okay right, because it's only a constructor, not a type in its own right.
| @@ -261,22 +276,25 @@ extensibleNotificationPlugins recorder xs = mempty { P.pluginHandlers = handlers | |||
| Just fs -> do | |||
| -- We run the notifications in order, so the core ghcide provider | |||
| -- (which restarts the shake process) hopefully comes last | |||
No description provided.