search

snoyberg / classy-prelude

A typeclass-based Prelude.
108 stars 15 forks source link

Function passed to `mapM_` shouldn't need to return `()` #76

Closed adnelson closed 9 years ago

adnelson commented 9 years ago

The signature of mapM_ in the classy-prelude reads 

mapM_ :: (Monad m, MonoFoldable c) => (Element c -> m ()) -> c -> m ()

But there shouldn't be any restriction on what the passed-in function returns: it gets ignored anyway. This also introduces a backwards incompatibility with Prelude.mapM, which has the signature

mapM_ :: Monad m => (a -> m b) -> [a] -> m ()

Looking into it, the culprit is in omapM_. 

omapM_ :: (MonoFoldable mono, Monad m) => (Element mono -> m ()) -> mono -> m ()
omapM_ f = ofoldr ((>>) . f) (return ())

The explicit signature puts an artificial restriction on omapM_. The "actual" type is

>>> :t \f -> ofoldr ((>>) . f) (return ())
\f -> ofoldr ((>>) . f) (return ())
  :: (MonoFoldable mono, Monad m) =>
     (Element mono -> m a) -> mono -> m ()

Which is analogous to the signature of Prelude.mapM_.

gregwebs commented 9 years ago

There is probably the same as #74. Let's leave this ticket open until this is documented in the code

adnelson commented 9 years ago

So I read Michael's article on the subject, and I guess I'm still confused: the implementation of omapM_ still allows for any function of a -> m b. Only the declaration restricts the type to a -> m (). Is this therefore allowing for a compiler optimization of some kind? If that were the case, wouldn't the compiler be able to apply this optimization given an argument that does have type a -> m ()?

snoyberg commented 9 years ago

Whether GHC should be able to optimize this away is a nice theoretical discussion to have, to which I don't really have any answers. In practice, it doesn't optimize that away, which is why I made the change. Personally, I think this is the right type signature for the function anyway, and the one in base is too lax, but that's a personal preference.

adnelson commented 9 years ago

Fair enough. It's a relatively minor complaint. I'm not sure that it's the "right" signature, but I suppose there's nothing wrong with calling void on the function you're passing in to mapM_.