Draft: Introduce hierarchical error types and error handling for plugins #3659

fendor · 2023-06-14T11:28:22Z

Essentially, infuse the existing pluginResponse infrastructure with steroids:

Provide a simple interface for performing the work you usually want to do
Allow fine-grained control how errors are finally reported to the client
Hierarchical error types, somewhat analogous to the logger

ghcide/src/Development/IDE/Core/PluginUtils.hs

joyfulmantis · 2023-06-14T14:46:26Z

Yeah, sorry I only saw the DRAFT marking after commenting 😅

michaelpj

I like the direction. At a high level my suggestions are:

Stuff inside Action already throws a bunch. Let's just use exceptions there, and have "strict" versions of the use functions that can throw if the thing isn't there.
Using ExceptT for the "main line" of plugin code seems nice. It would be nice if we could make the error type there fully generic so plugins can provide their own. I'm not sure how to do that because of the normalized file path stuff. The only suggestion I have is that we could also use exceptions there, and provide a throwing variant that runs in MonadIO. It's not like anyone wants to handle the failures there anyway!

michaelpj · 2023-06-24T13:07:44Z

ghcide/src/Development/IDE/Core/Shake.hs

@@ -964,11 +964,15 @@ useWithStale key file = runIdentity <$> usesWithStale key (Identity file)

 -- | Request a Rule result, it not available return the last computed result which may be stale.
 --   Errors out if none available.
+--
+-- The thrown error is a 'BadDependency' error which is caught by the rule system.


Maybe link to the definition of Action, which also explains that it's okay to throw inside Actions.

michaelpj · 2023-06-24T13:10:13Z

ghcide/src/Development/IDE/Plugin/TypeLenses.hs

-                )
+    nfp <- PluginUtils.withPluginError $ getNormalizedFilePath uri
+    env <- hscEnv . fst <$>
+      PluginUtils.runAction "codeLens.GhcSession" ideState


Any reason not to merge these all into one Action?

I did not want to impose Plugin changes for no reason, to keep it easier to review.
Also, I think some people prefer this style, because otherwise you have to return a big tuple and pattern match on it. I don't think there is any performance impact.

michaelpj · 2023-06-24T13:16:00Z

hls-plugin-api/src/Ide/PluginUtils.hs

+
+type PluginHandler e m a = ExceptT e m a
+
+pluginResponse :: Monad m => ExceptT PluginError m a -> m (Either ResponseError a)


What about

handlePluginError :: Pretty e => e -> ResponseError handlePluginError msg = ResponseError Internalerror (renderPrettyWhatever msg) Nothing pluginResponse :: Monad m => ExceptT e m a -> m (Either ResponseError a)

pluginResponse' is more generic still but I'm not sure if people really need that power?

Hmm, we can't make it completely generic as it is now because of the attempt to fold the getNormalizedFilePath functions in...

michaelpj · 2023-06-24T13:17:24Z

hls-plugin-api/src/Ide/PluginUtils.hs

+  fmap (first handlePluginError)
+    . runExceptT
+
+pluginResponse' :: Monad m => (e -> ResponseError) -> ExceptT e m a -> m (Either ResponseError a)


This is nearly pluginResponse . withExceptT handleError, right? Maybe we should just let people do this inline, withExceptT is not so exotic...

Generally, I think if we can nudge people towards just using standard Control.Monad.Except stuff that seems good

michaelpj · 2023-06-25T15:29:13Z

hls-plugin-api/src/Ide/PluginUtils.hs

+  | PluginUriToNormalizedFilePath J.Uri
+  | PluginErrorMessage T.Text
+
+prettyPluginError :: PluginError -> T.Text


instance Pretty PluginError?

michaelpj · 2023-06-25T18:38:19Z

ghcide/src/Development/IDE/Core/PluginUtils.hs

+-- | useE is useful to implement functions that aren’t rules but need shortcircuiting
+-- e.g. getDefinition.
+useWithStaleFast :: IdeRule k v => k -> NormalizedFilePath -> ExceptT GhcidePluginError IdeAction (v, PositionMapping)
+useWithStaleFast k = maybeToExceptT (RuleFailed k) . MaybeT . Shake.useWithStaleFast k


I'm not totally sure why this gets a different error than useWithStale 🤔 More generally I think it would be good for the errors to match the function names. We can rename the functions... but let's keep the two consistent.

michaelpj · 2023-06-25T18:39:15Z

ghcide/src/Development/IDE/Core/PluginUtils.hs

+    join $ shakeEnqueue (shakeExtras ide) (mkDelayedAction herald Logger.Debug $ runExceptT act)
+
+-- | Request a Rule result, it not available return the last computed result which may be stale.
+--   Errors out if none available.


Can I request that we not use "errors out", it's very ambiguous when we have both ExceptT and exceptions in the picture. Maybe just "throws an exception"?

michaelpj · 2023-06-25T18:41:24Z

hls-plugin-api/src/Ide/PluginUtils.hs

-  fmap (first (\msg -> ResponseError InternalError (fromString msg) Nothing))
-    . runExceptT
+withError :: Functor m => (e' -> e) -> ExceptT e' m a -> ExceptT e m a
+withError = withExceptT


Again, I think I'm broadly in favour of getting people to use "normal" ExceptT functions as much as possible.

michaelpj · 2023-06-25T18:44:36Z

ghcide/src/Development/IDE/Core/PluginUtils.hs

+    => k -> NormalizedFilePath -> ExceptT e Action (v, PositionMapping)
+useWithStale_ key file = ExceptT $ fmap Right $ Shake.useWithStale_ key file
+
+useWithStale :: IdeRule k v


Okay, so serious question: since we're in Action here, and it's safe to throw exceptions, is there any reason not to throw these errors as exceptions too?

That would give us an (I think) sensible heuristic:

Functions that run in Action throw exceptions

Functions that run outside Action use ExceptT

That also suggests that the functions here could just become "stricter" versions of useWithStale and friends, which throw RuleNotReady instead of RuleFailed or whatever. Then this module mostly goes away and we instead add a few functions to Shake.hs

michaelpj · 2023-06-25T18:48:54Z

ghcide/src/Development/IDE/Spans/Pragmas.hs

-getFirstPragma (PluginId pId) state nfp = handleMaybeM "Could not get NextPragmaInfo" $ do
-  ghcSession <- liftIO $ runAction (T.unpack pId <> ".GhcSession") state $ useWithStale GhcSession nfp
-  (_, fileContents) <- liftIO $ runAction (T.unpack pId <> ".GetFileContents") state $ getFileContents nfp
+getFirstPragma :: MonadIO m => PluginId -> IdeState -> NormalizedFilePath -> ExceptT GhcidePluginError m NextPragmaInfo


If we did what I suggested, this would "just" be in MonadIO. I think that's okay: if it's in MonadIO it can throw!

michaelpj · 2023-06-25T18:54:35Z

hls-plugin-api/src/Ide/PluginUtils.hs

@@ -263,27 +277,64 @@ allLspCmdIds pid commands = concatMap go commands

 -- ---------------------------------------------------------------------

-getNormalizedFilePath :: Monad m => Uri -> ExceptT String m NormalizedFilePath
-getNormalizedFilePath uri = handleMaybe errMsg
+getNormalizedFilePath :: Monad m => Uri -> ExceptT PluginError m NormalizedFilePath


So if we did what I'm suggesting, we'd also provide

getNormalizedFilePathIO :: MonadIO m => Uri -> m NormalizedFilePath

michaelpj · 2023-06-25T18:57:40Z

I realise that what I'm proposing is actually more radical than I maybe thought. It pretty much amounts to:

All plugin helper functions provide a variant that runs in MonadIO and throws on error. Plugins all run in MonadIO and have exception catching at the top level, so why not?
Plugins should just be expected to return ExceptT ResponseError m (Result...). If they want to use ExceptT with their own error type and use standard ExceptT to translate it to a ResponseError, that's pretty much completely up to them! At most we maybe could provide toResponseError :: Pretty e => e -> ResponseError

Perhaps this is a bit heretical, I'm basically proposing that we embrace exceptions in plugins. But maybe it would be fine?

fendor · 2023-06-26T08:29:21Z

I like the idea of embracing exceptions, catching them and providing sensible log messages.
I am going to move the API towards what you are suggesting.

joyfulmantis · 2023-06-26T11:55:17Z

It would be nice if there was a PluginError type that wrapped ResponseErrors. The reason is that resolve methods will need to throw a ContentModified ResponseError if they are unable to resolve the provided request, and it would be nice to be able to use the available ExceptT pluginResponse infrastructure while at the same time being able to throw specific response errors.

joyfulmantis · 2023-07-09T04:34:10Z

If you end up going ahead with this, I think it might be a good idea to first merge the general infrastructure as a parallel system to what we have now. Then anyone interested in migrating the plugins, can do that piece by piece. Finally once everything is migrated, the old stuff can be removed.

joyfulmantis · 2023-07-30T05:02:15Z

Closing this as #3717 was merged

fendor requested review from joyfulmantis, July541, berberman and pepeiborra as code owners June 14, 2023 11:28

joyfulmantis reviewed Jun 14, 2023

View reviewed changes

ghcide/src/Development/IDE/Core/PluginUtils.hs Show resolved Hide resolved

fendor marked this pull request as draft June 14, 2023 15:01

fendor force-pushed the enhance/plugin-logger-structure branch 2 times, most recently from 7aad4b6 to 1e077da Compare June 15, 2023 19:26

fendor changed the title ~~Draft: Improve logs of plugins~~ Draft: Introduce hierarchical error types and error handling for plugins Jun 15, 2023

fendor force-pushed the enhance/plugin-logger-structure branch 2 times, most recently from 526fc9a to 565e016 Compare June 15, 2023 19:46

WIP

8adb03e

fendor force-pushed the enhance/plugin-logger-structure branch from 565e016 to 8adb03e Compare June 19, 2023 09:30

michaelpj reviewed Jun 25, 2023

View reviewed changes

joyfulmantis mentioned this pull request Jul 22, 2023

Better plugin error infrastructure #3717

Merged

joyfulmantis closed this Jul 30, 2023


		type PluginHandler e m a = ExceptT e m a

		pluginResponse :: Monad m => ExceptT PluginError m a -> m (Either ResponseError a)

Uh oh!

Draft: Introduce hierarchical error types and error handling for plugins #3659

Draft: Introduce hierarchical error types and error handling for plugins #3659

Uh oh!

Conversation

fendor commented Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

joyfulmantis commented Jun 14, 2023 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelpj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelpj commented Jun 25, 2023

Uh oh!

fendor commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyfulmantis commented Jun 26, 2023

Uh oh!

joyfulmantis commented Jul 9, 2023

Uh oh!

joyfulmantis commented Jul 30, 2023

Uh oh!

Uh oh!

fendor commented Jun 14, 2023 •

edited

Loading

joyfulmantis commented Jun 14, 2023 via email •

edited

Loading

fendor commented Jun 26, 2023 •

edited

Loading