Catch exceptions in commands and use lsp null #3696
Conversation
joyfulmantis
requested review from
Ailrun,
July541,
berberman,
konn,
pepeiborra and
santiweight
as code owners
michaelpj
left a comment
michaelpj
left a comment
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 |
| 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
| = 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.
| logWith recorder Warning (LogNoPluginForMethod $ Some m) | ||
| Just fs -> do | ||
| -- We run the notifications in order, so the core ghcide provider | ||
| -- (which restarts the shake process) hopefully comes last |
3 participants
No description provided.