search

haskell / mtl

The Monad Transformer Library
http://www.haskell.org/haskellwiki/Monad_Transformers
Other
362 stars 63 forks source link

add (existing mtl typeclass) instances for SelectT #81

Closed chessai closed 3 years ago

chessai commented 4 years ago

Not many instances for this. Should it have any more?

Should follow the merge of #80

turion commented 4 years ago

You've added AccumT instances and SelectT instances both in one go here.

chessai commented 4 years ago

You've added AccumT instances and SelectT instances both in one go here.

Yes, it was forked from the PR backing #80. I intend to rebase after #80 is merged

chessai commented 4 years ago

Would like a review from @RyanGlScott @emilypi (and @turion maybe?)

turion commented 4 years ago

Are you going to add more instances? Or documentation why other typical instances won't work?

chessai commented 4 years ago

No, I am not going to add more instances, since I think they should not exist. I am not sure where any documentation pointing to the lack of instances should exist, if at all. To my understanding, SelectT is equivalent in representation and implementation to ContT but not intended to be used as a Continuation monad, i.e. its type signifies a different intention, and it is its own beast. So, while we could give it a MonadCont instance, this seems to go against its spirit, in the same vein that giving AccumT an instance of (Monoid w, Monad m) => MonadWriter w (AccumT w m) would go against the spirit of AccumT (in that it is not meant to be a Writer). Reader can lift through SelectT in the same way as ContT and I don't see that being an issue. Ultimately I think that we will only have MonadReader and the yet-to-be MonadCont instances for SelectT.

turion commented 4 years ago

To my understanding, SelectT is equivalent in representation and implementation to ContT but not intended to be used as a Continuation monad, i.e. its type signifies a different intention, and it is its own beast. So, while we could give it a MonadCont instance, this seems to go against its spirit, in the same vein that giving AccumT an instance of (Monoid w, Monad m) => MonadWriter w (AccumT w m) would go against the spirit of AccumT (in that it is not meant to be a Writer).

Yes, that I absolutely agree with.

Reader can lift through SelectT in the same way as ContT and I don't see that being an issue. Ultimately I think that we will only have MonadReader and the yet-to-be MonadCont instances for SelectT.

And that's my actual question. Sorry for being somewhat imprecise, let me specify:

Is there going to be an instance MonadState s m => MonadState s (SelectT a m)? And the same for MonadWriter, etc.?

chessai commented 4 years ago

@turion i don't know if MonadWriter is appropriate, but the MonadState instance should be fine.

chessai commented 4 years ago

@RyanGlScott @emilypi could you take a look at this when you have the time?

turion commented 4 years ago

@chessai Why wouldn't MonadWriter be appropriate? Maybe it can be easily done, then it should be added. Or it can't be done, then there should be documentation explaining why not.

RyanGlScott commented 4 years ago

I agree that it wouldn't hurt to add a comment somewhere explaining why SelectT (and, by extension, ContT) doesn't have certain instances. Currently, the only documentation about this is on the Haddocks for SelectT (and ContT) itself, which simply states that it "is not a functor on the category of monads, and many operations cannot be lifted through it." This is true, although that's admittedly quite impenetrable for someone like me, who knows next to nothing about category theory :)

chessai commented 3 years ago

i agree, but i think that it should come in a separate PR, the one that actuallys adds Control.Monad.Select. Otherwise, the explanation feels a little dangly.

chessai commented 3 years ago

here is an example of explaining the reasoning for ContT (holds for SelectT as well):

review

chessai commented 3 years ago

I suppose the documentation could go in the .Class modules instead. What are your thoughts?

turion commented 3 years ago

Maybe the statement "is not a functor on the category of monads" could simply be explained with a type signature, such as "i.e. there is no function (forall a . m a -> n a) -> SelectT m b -> SelectT n b", and a link to http://hackage.haskell.org/package/mmorph-1.1.3/docs/Control-Monad-Morph.html#t:MFunctor. Then one can look a bit at the type signature for a while and notice that the m r is at the wrong end of the -> for such a function to exist, and be satisfied.

I suppose the documentation could go in the .Class modules instead. What are your thoughts?

Inside mtl there is no other good place, right? I guess transformers right in https://hackage.haskell.org/package/transformers-0.5.5.0/docs/Control-Monad-Trans-Select.html#t:SelectT would be better even.

RyanGlScott commented 3 years ago

I agree that the documentation for ContT in transformers itself could use some further exposition. It feels somewhat sad to have to refer to another package's Haddocks as "vague and uninteresting" from another set of Haddocks. :)

Even if the Haddocks in transformers were improved, however, I quite like seeing the explanation of how MonadWriter and ContT don't mix. Even if I was told that you can't write a function of type (forall a . m a -> n a) -> SelectT m b -> SelectT n b, I don't think the connection between that and MonadWriter would be evident to me unless it was explained in the level of detail found in https://github.com/haskell/mtl/pull/81#issuecomment-696532554. I don't have a strong opinion about where to put this, but it definitely feels like in belongs somewhere in mtl.

turion commented 3 years ago

it definitely feels like in belongs somewhere in mtl.

One argument in that direction is that we would expect the instance of such a class, if it would exist, to be in mtl. So people come to mtl in search of that instance, and then they should either the instance or an explanation why it doesn't exist.

emilypi commented 3 years ago

@chessai I suggest we move doc changes to a separate PR. It sounds like like we all have thoughts on this, and it shouldn't block the instances.