Skip to content
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

Jump to bottom

Improve support for monad stacks #1539

Merged
merged 1 commit into from Feb 10, 2020
Merged

Improve support for monad stacks #1539

merged 1 commit into from Feb 10, 2020

Conversation

edsko
Copy link
Contributor

@edsko edsko commented Jan 30, 2020

The main change in this commit is split in MonadSTM, which avoids the injectivity requirement, enabling GeneralizedNewtypeDeriving for MonadSTM. The remainder of the changes are related, and similarly
intended to faciliate the derivation of monad stacks.

Note that a similar change could be made to MonadTimer, but I didn't need it and so I've not done that yet.

With this PR, we can define something like

newtype MockTime m a = MockTime (ReaderT () m a)
  deriving ( Functor
           , Applicative
           , Monad
           , MonadTrans
           , MonadThrow
           , MonadSTM
           , MonadDelay
           , MonadTime
           )

and have it Just Work (TM). (Of course, this particular definition isn't very useful, just a proof of concept.)

@edsko edsko added consensus issues related to ouroboros-consensus networking labels Jan 30, 2020
@edsko
Copy link
Contributor Author

edsko commented Jan 30, 2020

I see that I left some stuff in a different commit, two tiny definitions. Will force-push. 99% of the work is here.

@edsko edsko force-pushed the edsko/split-MonadSTM branch 2 times, most recently from f53e3e9 to 4b170ec Compare January 30, 2020 13:54
@edsko
Copy link
Contributor Author

edsko commented Jan 30, 2020

Done.

@edsko edsko force-pushed the edsko/split-MonadSTM branch 3 times, most recently from 74e6f57 to 0e59cca Compare January 30, 2020 15:03
@edsko
Copy link
Contributor Author

edsko commented Jan 30, 2020

Am getting non-termination in the simulator. Don't yet know if it's due to these changes. Will continue digging tomorrow.

@edsko
Copy link
Contributor Author

edsko commented Jan 31, 2020

Ok, non-termination was just stupidity on my part. This PR is good to go.

This was referenced Jan 31, 2020
Merged
Closed
@edsko
Copy link
Contributor Author

edsko commented Feb 7, 2020

Rebased on latest master to resolve conflicts.

@edsko edsko mentioned this pull request Feb 7, 2020
Merged
@edsko
Copy link
Contributor Author

edsko commented Feb 7, 2020

Rebased this on #1604 which will be master soon to reduce the footprint of this PR and of #1601 .

@edsko
Copy link
Contributor Author

edsko commented Feb 7, 2020

Ok, ready again to be merged pending review by @dcoutts .

dcoutts
dcoutts approved these changes Feb 7, 2020
View reviewed changes
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

Looks fine.

Looks like there's a bunch of unrelated code changes here though, like reordering imports. No objection to doing that, but preferably in a separate PR that does only that.

ouroboros-network/test/Ouroboros/Network/PeerSelection/Test.hs
Comment on lines +348 to +349
interpretPickScript :: (MonadSTMTx stm, Ord peeraddr)
=> TVar_ stm PickScript
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to change things like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neither TVar nor STM is injective, so if you were to write TVar m a -> STM m .. , the monad m is entirely ambiguous, and you'd have to introduce a Proxy. This seemed cleaner.

ouroboros-network/test/Ouroboros/Network/PeerSelection/Test.hs
@@ -913,7 +914,7 @@ initScript = newTVarM
stepScript :: MonadSTM m => TVar m (Script a) -> m a
stepScript scriptVar = atomically (stepScriptSTM scriptVar)

stepScriptSTM :: MonadSTM m => TVar m (Script a) -> STM m a
stepScriptSTM :: MonadSTMTx stm => TVar_ stm (Script a) -> stm a
Copy link
Contributor

Choose a reason for hiding this comment

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

Required or just because we can? This leaks MonadSTMTx into app code which adds cognitive overhead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same answer as above.

ouroboros-network/test/Test/Pipe.hs
@@ -1,6 +1,7 @@
{-# LANGUAGE DataKinds #-}
{-# LANGUAGE FlexibleContexts #-}
{-# LANGUAGE ScopedTypeVariables #-}
{-# LANGUAGE TypeFamilies #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

related?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

ouroboros-network/test/Test/Pipe.hs
@@ -27,7 +28,7 @@ import Control.Tracer
import qualified Network.Mux.Bearer.Pipe as Mx
import Ouroboros.Network.Mux

import Ouroboros.Network.Block (encodeTip, decodeTip)
import Ouroboros.Network.Block (decodeTip, encodeTip)
Copy link
Contributor

Choose a reason for hiding this comment

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

related?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, just inconsistent use of the formatter in this repo.

@edsko
Improve support for monad stacks
70edb92 
The main change in this commit is split in MonadSTM, which avoids the
injectivity requirement, enabling GeneralizedNewtypeDeriving for
MonadSTM. The remainder of the changes are related, and similarly
intended to faciliate the derivation of monad stacks.
@edsko
Copy link
Contributor Author

edsko commented Feb 10, 2020

bors r+

iohk-bors bot added a commit that referenced this pull request Feb 10, 2020
@iohk-bors @edsko
Merge #1539
2275859 
1539: Improve support for monad stacks r=edsko a=edsko

The main change in this commit is split in MonadSTM, which avoids the injectivity requirement, enabling GeneralizedNewtypeDeriving for MonadSTM. The remainder of the changes are related, and similarly
intended to faciliate the derivation of monad stacks.

Note that a similar change could be made to `MonadTimer`, but I didn't need it and so I've not done that yet.

With this PR, we can define something like

```haskell
newtype MockTime m a = MockTime (ReaderT () m a)
  deriving ( Functor
           , Applicative
           , Monad
           , MonadTrans
           , MonadThrow
           , MonadSTM
           , MonadDelay
           , MonadTime
           )
```

and have it Just Work (TM). (Of course, this particular definition isn't very useful, just a proof of concept.)

Co-authored-by: Edsko de Vries <edsko@well-typed.com>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 10, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit 70edb92 into master Feb 10, 2020
@iohk-bors iohk-bors bot deleted the edsko/split-MonadSTM branch February 10, 2020 08:40
iohk-bors bot added a commit that referenced this pull request Feb 10, 2020
@iohk-bors @edsko
Merge #1601
8e7e591 
1601: Test clock changes r=edsko a=edsko

This adds tests for #1554 as a separate PR, because tests tests depend on #1539 .

Co-authored-by: Edsko de Vries <edsko@well-typed.com>
coot pushed a commit that referenced this pull request May 16, 2022
@iohk-bors @edsko
Merge #1539
8aa0e01 
1539: Improve support for monad stacks r=edsko a=edsko

The main change in this commit is split in MonadSTM, which avoids the injectivity requirement, enabling GeneralizedNewtypeDeriving for MonadSTM. The remainder of the changes are related, and similarly
intended to faciliate the derivation of monad stacks.

Note that a similar change could be made to `MonadTimer`, but I didn't need it and so I've not done that yet.

With this PR, we can define something like

```haskell
newtype MockTime m a = MockTime (ReaderT () m a)
  deriving ( Functor
           , Applicative
           , Monad
           , MonadTrans
           , MonadThrow
           , MonadSTM
           , MonadDelay
           , MonadTime
           )
```

and have it Just Work (TM). (Of course, this particular definition isn't very useful, just a proof of concept.)

Co-authored-by: Edsko de Vries <edsko@well-typed.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcoutts dcoutts dcoutts approved these changes

@mrBliss mrBliss Awaiting requested review from mrBliss

@coot coot Awaiting requested review from coot

Assignees
No one assigned
Labels
consensus issues related to ouroboros-consensus
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@edsko @dcoutts