Better plugin error infrastructure #3717
Conversation
Merge fendor/enhance/plugin-logger-structure
Additionally further PluginError constructors and helpers refactoring
joyfulmantis
requested review from
ozkutuk,
Ailrun,
konn,
kokobd,
pepeiborra,
santiweight,
OliverMadine,
georgefst,
eddiemundo,
July541,
VeryMilkyJoe,
fendor,
drsooch and
berberman
as code owners
joyfulmantis
changed the title
| @@ -154,7 +116,7 @@ getSelectionRanges file positions = do | |||
| in fromMaybe defaultSelectionRange . findPosition pos $ codeRange | |||
|
|
|||
| -- 'positionMapping' should be applied to the output ranges before returning them | |||
| maybeToExceptT SelectionRangeOutputPositionMappingFailure . MaybeT . pure $ | |||
| maybeToExceptT (PluginDependencyFailed "toCurrentSelectionRange") . MaybeT . pure $ | |||
There was a problem hiding this comment.
Good catch! I think PluginHandlerFailed is a good name.
| @@ -48,6 +48,7 @@ selectionRangeGoldenTest testName positions = goldenGitDiff testName (testDataDi | |||
| let res = resp ^. result | |||
| pure $ fmap (showSelectionRangesForTest . absorbNull) res | |||
| case res of | |||
| Left (ResponseError (InL LSPErrorCodes_ContentModified) _ _) -> pure "" | |||
There was a problem hiding this comment.
I still don't understand, why do we produce a LSPErrorCodes_ContentModified for the empty result in this plugin? Is it because the only reason why we might produce an empty result is because we have modified the content before resolving SMethod_TextDocumentSelectionRange ?
I think Actions use AsyncExceptions for cutting off stale request, which is likely unavoidable. I don't think we should/have to remove all exceptions from |
Do you know if the underscore functions and returning Nothing in the rules are functionally equivalent? At least if that's the case we can dump the underscore functions and convert rule users of them to MaybeT |
Sorry, I do not |
|
|
||
| -- See Note [PluginError] | ||
| data PluginError | ||
| = -- |PluginInternalError should be used if something has gone horribly wrong. |
There was a problem hiding this comment.
If it's not because of the user's invalid state, then arguably it's a bug, and should never happen
I feel like this is a pretty strong statement and I don't know that I would want to commit to it. I think I'd feel a bit more comfortable with a generic "error but not a bug" error, especially since I think being more prescriptive here isn't really buying us anything.
It also seems like "is this a bug in our code or not" is the dimension that we actually care about?
| toErrorCode (PluginInvalidUserState _) = InL LSPErrorCodes_RequestFailed | ||
| -- PluginRequestRefused should never be a argument to `toResponseError`, as | ||
| -- it should be dealt with in `extensiblePlugins`, but this is here to make | ||
| -- this function complete |
There was a problem hiding this comment.
... but also I think this is pretty logical
| toErrorCode (PluginRuleFailed _) = InL LSPErrorCodes_RequestFailed | ||
| toErrorCode PluginStaleResolve = InL LSPErrorCodes_ContentModified | ||
|
|
||
| toPriority :: PluginError -> Priority |
There was a problem hiding this comment.
Haddock! good to explain when this should be used
| -- ---------------------------------------------------------------------------- | ||
|
|
||
| -- |ExceptT version of `runAction`, takes a ExceptT Action | ||
| runActionE :: MonadIO m => String -> IdeState -> ExceptT e Action a -> ExceptT e m a |
There was a problem hiding this comment.
We could consider generalizing these functions to MonadError rather than ExceptT. Not sure that buys us much, I probably would instinctively do it, but I'm used to programming in a very mtl-y way!
This PR is based on fendors, he did a lot of the initial work originally in PR #3659. I finished up what he started, and then tried a slightly different approach, where I deemphasize hierarchical errors and instead try to define the common "errors" that occur in plugins centrally in the hls-plugin-api.
What I have also done, is have all method handlers now return PluginError instead of ResponseError, this allows us to define specific error types that correspond to concrete error responses we want to send to the client and specific ways we want to log the error.
This also allows us to keep all the ExceptT infrastructure the fendor has written, give much more accurate error response codes to the server, and log non-serious errors such as rule failures with info or even debug, while still being able to log more serious errors such as uncaught exceptions or bugs in the client or plugin with error.