Skip to content

Document exception handling in Actions/Rules #3407

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 2, 2023
Merged

Document exception handling in Actions/Rules #3407

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b0f3694
Document exception handling in Actions/Rules
michaelpj Dec 16, 2022
205adc1
Tweak
michaelpj Dec 21, 2022
4c404cf
Merge branch 'master' into mpj/document-exception-handling
michaelpj Dec 21, 2022
4f2c160
Merge branch 'master' into mpj/document-exception-handling
mergify[bot] Dec 22, 2022
fab9c2d
Merge branch 'master' into mpj/document-exception-handling
mergify[bot] Jan 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions ghcide/src/Development/IDE/Core/Shake.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 12 additions & 1 deletion hls-graph/src/Development/IDE/Graph/Internal/Types.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
-- 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, MonadFail)
deriving newtype (Monad, Applicative, Functor, MonadIO)

data SRules = SRules {
rulesExtra :: !Dynamic,
Expand All @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for nitpicking, but who assumes these actions can throw exceptions? The user, or the Rules engine? Or both? E.g., are exceptions always caught or may they bubble up and eventually crash HLS?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unclear! I'm writing this for the "user of hls-graph", which in this case is HLS itself, the point being that hls-graph does not do anything about the exceptions, but the user can. Now HLS does use actionCatch to catch such exceptions and turn them into diagnostics, but it's a bit of a layering violation to write that here. Perhaps I should say it anyway?

Conceivably some other user of hls-graph could choose to just let them bubble up and crash the system...

Copy link
Collaborator

@fendor fendor Dec 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, then slightly adapting the beginning of the sentence seems fine by me.

Suggested change
-- 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

Maybe this is better? Or maybe just go with what you think is better, I am sure your English is better than mine.

-- '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)

Expand Down