From b0f3694ddbb6db552463fc11c5b85f1825014fd3 Mon Sep 17 00:00:00 2001 From: Michael Peyton Jones Date: Fri, 16 Dec 2022 11:33:46 +0000 Subject: [PATCH 1/2] Document exception handling in Actions/Rules I also took the liberty of removing the `MonadFail` instance for `Rules`, since failing in `Rules` seems much worse as it won't automatically be caught. There was only one use of that, which now has an appropriately threatening comment. --- ghcide/src/Development/IDE/Core/Shake.hs | 8 ++++++-- .../src/Development/IDE/Graph/Internal/Types.hs | 13 ++++++++++++- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/ghcide/src/Development/IDE/Core/Shake.hs b/ghcide/src/Development/IDE/Core/Shake.hs index 00655b5a4d..f6f8537c07 100644 --- a/ghcide/src/Development/IDE/Core/Shake.hs +++ b/ghcide/src/Development/IDE/Core/Shake.hs @@ -307,13 +307,17 @@ type GetStalePersistent = NormalizedFilePath -> IdeAction (Maybe (Dynamic,Positi getShakeExtras :: Action ShakeExtras getShakeExtras = do + -- Will fail the action with a pattern match failure, but be caught Just x <- getShakeExtra @ShakeExtras return x getShakeExtrasRules :: Rules ShakeExtras getShakeExtrasRules = do - Just x <- getShakeExtraRules @ShakeExtras - return x + mExtras <- getShakeExtraRules @ShakeExtras + case mExtras of + Just x -> return x + -- This will actually crash HLS + Nothing -> liftIO $ fail "missing ShakeExtras" -- | Returns the client configuration, creating a build dependency. -- You should always use this function when accessing client configuration diff --git a/hls-graph/src/Development/IDE/Graph/Internal/Types.hs b/hls-graph/src/Development/IDE/Graph/Internal/Types.hs index 8451f641a3..f3adfba81e 100644 --- a/hls-graph/src/Development/IDE/Graph/Internal/Types.hs +++ b/hls-graph/src/Development/IDE/Graph/Internal/Types.hs @@ -54,8 +54,13 @@ unwrapDynamic x = fromMaybe (error msg) $ fromDynamic x type TheRules = Map.HashMap TypeRep Dynamic +-- | A computation that defines all the rules that form part of the computation graph. +-- +-- 'Rules' has access to 'IO' through 'MonadIO'. Use of 'IO' is at your own risk: if +-- you write 'Rules' that thow exceptions, then you need to make sure to handle them +-- yourself when you run the resulting 'Rules'. newtype Rules a = Rules (ReaderT SRules IO a) - deriving newtype (Monad, Applicative, Functor, MonadIO, MonadFail) + deriving newtype (Monad, Applicative, Functor, MonadIO) data SRules = SRules { rulesExtra :: !Dynamic, @@ -67,6 +72,12 @@ data SRules = SRules { --------------------------------------------------------------------- -- ACTIONS +-- | An action representing something that can be run as part of a 'Rule'. +-- +-- 'Action's can be pure functions but also have access to 'IO' via 'MonadIO' and 'MonadUnliftIO. +-- It should be assumed that actions can throw exceptions, these can be caught with +-- 'Development.IDE.Graph.Internal.Action.actionCatch'. In particular, it is +-- permissible to use the 'MonadFail' instance, which will lead to an 'IOException'. newtype Action a = Action {fromAction :: ReaderT SAction IO a} deriving newtype (Monad, Applicative, Functor, MonadIO, MonadFail, MonadThrow, MonadCatch, MonadMask, MonadUnliftIO) From 205adc1d6231eeba6f19bd3858db583edb2dcd6f Mon Sep 17 00:00:00 2001 From: Michael Peyton Jones Date: Wed, 21 Dec 2022 14:37:12 +0000 Subject: [PATCH 2/2] Tweak --- hls-graph/src/Development/IDE/Graph/Internal/Types.hs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hls-graph/src/Development/IDE/Graph/Internal/Types.hs b/hls-graph/src/Development/IDE/Graph/Internal/Types.hs index f3adfba81e..891b358c7b 100644 --- a/hls-graph/src/Development/IDE/Graph/Internal/Types.hs +++ b/hls-graph/src/Development/IDE/Graph/Internal/Types.hs @@ -57,7 +57,7 @@ type TheRules = Map.HashMap TypeRep Dynamic -- | A computation that defines all the rules that form part of the computation graph. -- -- 'Rules' has access to 'IO' through 'MonadIO'. Use of 'IO' is at your own risk: if --- you write 'Rules' that thow exceptions, then you need to make sure to handle them +-- you write 'Rules' that throw exceptions, then you need to make sure to handle them -- yourself when you run the resulting 'Rules'. newtype Rules a = Rules (ReaderT SRules IO a) deriving newtype (Monad, Applicative, Functor, MonadIO) @@ -75,7 +75,7 @@ data SRules = SRules { -- | An action representing something that can be run as part of a 'Rule'. -- -- 'Action's can be pure functions but also have access to 'IO' via 'MonadIO' and 'MonadUnliftIO. --- It should be assumed that actions can throw exceptions, these can be caught with +-- It should be assumed that actions throw exceptions, these can be caught with -- 'Development.IDE.Graph.Internal.Action.actionCatch'. In particular, it is -- permissible to use the 'MonadFail' instance, which will lead to an 'IOException'. newtype Action a = Action {fromAction :: ReaderT SAction IO a}