Monad transformers switch type

[JordanMartinez]

Addresses #1305

Note:

• Did not do ReaderWriterStateT due to not knowing where the F[_] should go in the type signature (e.g. RWST[R, F, W, S, A] or RWST[R, W, S, F, A]? I'm assuming the latter...?)
• Not sure whether the swap for StateT and ContT are supposed to go deeper (e.g. also do swap in IndexedStateT)
• Some cases of using StateT[S, F, A] would not compile unless type was changed to that which it aliases: IndexedStateT[F, S, S, A]
• Not sure whether methods types or other classes should also be changed (e.g. method[F[_], A] stays the same? or becomes method[A, F[_]]? / MonadPlus[F[_], A] or MonadPlus[A, F[_]])
• "Fix Later" commit: code wasn't compiling and I'm not sure how to fix it. Figured it'd be better to get more of this PR done and then figure that out with everyone else's help.

I'm going to sleep after this, so won't respond until Monday.

[TomasMikula]

• Did not do ReaderWriterStateT due to not knowing where the F[_] should go in the type signature (e.g. RWST[R, F, W, S, A] or RWST[R, W, S, F, A]? I'm assuming the latter...?)

Yes, the latter. (I wouldn't follow Haskell religiously, but it cannot hurt to check what has worked there: The RWST monad transformer.)

• Not sure whether the swap for StateT and ContT are supposed to go deeper (e.g. also do swap in IndexedStateT)

I would say yes, if only for consistency.

• Some cases of using StateT[S, F, A] would not compile unless type was changed to that which it aliases: IndexedStateT[F, S, S, A]

This should go away after it is changed to IndexedStateT[S, S, F, A].

• Not sure whether methods types or other classes should also be changed (e.g. method[F[_], A] stays the same? or becomes method[A, F[_]]? / MonadPlus[F[_], A] or MonadPlus[A, F[_]])

I would change method type parameter order accordingly, but I don't think it would apply to these examples.

---

We should also remove the following from CONTRIBUTING.md:

Declare type parameters requiring type constructors first in type parameter declarations. This is an arbitrary choice,
but the consistency is worth it.

class Foo[F[_], A, B] // good
class Foo[A, F[_], B] // bad

[JordanMartinez]

We should also remove the following

Declare type parameters requiring type constructors first in type parameter declarations. This is an arbitrary choice,
but the consistency is worth it.
class Foo[F[], A, B] // good
class Foo[A, F[], B] // bad

Should this not necessarily be removed entirely but just simply changed to say something else. For example, we could change it to read "The higher-kinded type (i.e. F[_] / G[_]) should always go before the thing it enapsulates (i.e. A / B), assuming such a thing is included in the method/class"? If so, these examples would be correct:

def method0[F](fa: F[_]): F[_] = ???
def method1[F, A](fa: F[A]): F[A] = ???
def method2[F, A, G, B](fa: F[A], f: A => G[B]): G[B] = ???

case class Clazz0[F[_]]
case class Clazz1[F[_], A]
case class Clazz2[F[_], A, G[_]]

I'm asking partly because I do not fully understand yet what is gained by switching these type parameters, nor what problems might arise with the above examples

I'll address your other comments later. Hopefully by the end of the day.

[TomasMikula]

I do not fully understand yet what is gained by switching these type parameters

It is just optimizing for the most common use case in conjunction with partial unification. It's just more common in practice that one needs to infer StateT[S, ?[_], ?] than StateT[?, F, ?], so we choose the order of type parameters such that the more common use case can be inferred using partial unification, which works from the right.

val foo: StateT[S, F, A] = ???
def bar[MT[_[_], _], G[_], B](mtgb: MT[G, B]): Unit = ???

bar(foo) // trying to unify MT[G, B] with StateT[S, F, A],
         // B is unified with A, G is unified with F,
         // and MT is unified with StateT[S, ?[_], ?]

[JordanMartinez]

Thanks. I also read through scala/scala#5102 and that explained a bit more context, too.

[JordanMartinez]

For ContT's type alias hierarchy...

// changes to ContT[R, Id, A]
type Cont[R, A] = ContT[Id, R, A] 

// does this change to ContsT[R, Id, M, A] ???
type ContT[M[_], R, A] = ContsT[Id, M, R, A]

// Does this change to IndexedContsT[R, W, M, R, A] ???
type ContsT[W[_], M[_], R, A] = IndexedContsT[W, M, R, R, A]

[JordanMartinez]

Note: the current code does not make the corresponding swap in the test package. I'll need to update it for that, too.

[TomasMikula]

As in other monad transformers, make the second to last type parameter be the monad M[_].

As for the comonad type parameter W[_], I wouldn't change its position in this PR, so keep it as the first type parameter for now. (Unless someone has a compelling use case for it, we might even remove the W[_] parameter altogether in the future; see #1737.)

[JordanMartinez]

As in other monad transformers, make the second to last type parameter be the monad M[_].

Looks like I'll need to fix RWST since I put F[_] in-between S1 and S2

[JordanMartinez]

Oh, nvm. I did it correctly the first time.

[JordanMartinez]

In EndomorphicTest

implicit def endoEqual[F[_], G[_[_], _, _], A](
    implicit F: Equal[G[F, A, A]]
  ): Equal[Endomorphic[G[F, ?, ?], A]] =
    Equal.equalBy(_.run)

checkAll(monoid.laws[Endomorphic[Kleisli[?, Option, ?], Int]])
checkAll(semigroup.laws[Endomorphic[Cokleisli[Option, ?, ?], Int]])

It fails at monoidLaws because it expected G[F, ?, ?], not G[?, F, ?]. I guess this means I have to swap Cokleisli's types as well...?

[JordanMartinez]

Kleisli's tests are failing because it can't figure out that it needs to wrap it in a class to get some methods that seem related to Arrow:

>>>
|||
-*-
mapsnd
first
proleft
***

[TomasMikula]

I suggest to leave Kleisli unchanged in this PR, and define ReaderT as

type ReaderT[A, F[_], B] = Kleisli[F, A, B]

If that causes problems, leave out ReaderT for now as well. (In the future we might want to hide the fact that ReaderT is an alias for Kleisli.)

It is in fact the case that when we talk about Kleisli arrows, we have a monad F[_] in mind before talking about the source and target objects A, B.

On the other hand, when we use the ReaderT monad transformer, we typically know the input type A and allow the additional effect monad F[_] to be specified later.

This is also how things are in Haskell (Kleisli, ReaderT):

Kleisli m a b
ReaderT r m a

[JordanMartinez]

Alright, I'll try that out and see how it goes.

Also, I'm still not sure why EitherT is failing in the example usage:

[error] /scalaz/example/src/main/scala/scalaz/example/UnapplyInference.scala:13:22: 
          value :-> is not a member of scalaz.EitherT[Int,Option,Int]
[error]     println((eitherT :-> (_ - 1)).run) // Some(Right(0))
[error]                      ^
[error] /scalaz/example/src/main/scala/scalaz/example/UnapplyInference.scala:24:63: 
          value bisequence is not a member of scalaz.EitherT[List[Int],Option,List[Int]]
[error]     val bisequence: List[EitherT[Int, Option, Int]] = eitherT.bisequence[List, Int, Int]
[error] 

[JordanMartinez]

I'll push the EitherT type swap commit (cherrypick) once I see how this does since it is known that the EitherT commit has compilation issues.

[JordanMartinez]

Aye. The code compiles, but some test is looping infinitely and causing the build to time out.

[JordanMartinez]

I've done my part. Now I need help from the community:

• Figure out why compiler can't find EitherT's implicit Bitraverse/Bifunctor instances
• Figure out why the tests stop running after Coproduct.traverse1's second property test
• Feedback on PR (e.g. whether type parameters are in correct places for methods and whatnot)
• Edit: Determine what CONTRIBUTING.MD should say in regards to type order as mentioned at the bottom of Tomas' comment

[virdis]

@JordanMartinez I think this PR should be open against scalaz8 branch, that could explain the failures you have listed above. Think this is mislabeled issue.

[JordanMartinez]

So... The last comment that implies this should be done for the 7.3.x series in #1305 was a mistake? If so... that really sucks, haha!

[jdegoes]

@JordanMartinez Thanks for making it this far! Folks will chime in as they have time, and I'll do a review of the type parameter ordering when I get a chance.

[jdegoes]

@JordanMartinez The order looks correct to me! I did not check that all type classes were so modified, but the ones in this pull request look good. 👍

[JordanMartinez]

@jdegoes Thanks for the feedback!

I looked into the tests not responding and have narrowed the problem down to the StateT commit. When I tested that commit, the tests stopped responding just like the build here. So, I cherrypicked the WriterT commit on top of the HEAD of the 7.3.x series when I initially started this PR and ran the tests then. They passed without issue. I did the same cherrypick-then-test method for the remaining commits in the following order and they all passed: ReaderWriterStateT, ContT, ReaderT, EitherT

I'm looking into why the EitherT example isn't compiling next.

[TomasMikula]

The problem with EitherT comes down to whether we will primarily use it as a monad transformer or as a bifunctor. This PR chooses the monad transformer use case, i.e. parameter order EitherT[A, M[_], B]. Such EitherT[A, M[_], B] will not unify with F[X, Y] (where we would like to infer F := EitherT[?, M, ?], X := A, Y := B) out of the box.

However, we can instruct the compiler to consider this possibility by adding a new case of Unapply2 inference to object Unapply2 to adapt types Foo[A, M[_], B] to shape Bar[A, B] (TomasMikula@23cbe16):

implicit def unapplyMAFB[TC[_[_, _]], M0[_, _[_], _], A0, F0[_], B0](implicit TC0: TC[M0[?, F0, ?]]): Unapply2[TC, M0[A0, F0, B0]] {
  type M[X, Y] = M0[X, F0, Y]
  type A = A0
  type B = B0
} =
  new Unapply2[TC, M0[A0, F0, B0]] {
    type M[X, Y] = M0[X, F0, Y]
    type A = A0
    type B = B0
    def TC = TC0
    def leibniz = refl
  }

[JordanMartinez]

I can't get to this now, but hopefully will before the end of today. Thanks for the explanation @TomasMikula!

[JordanMartinez]

If this PR is merged now, the StateT commit can be merged in a different PR. However, the issue of what CONTRIBUTING.MD says hasn't been resolved. I wonder if that should be further discussed in the original issue that this PR addresses.

[TomasMikula]

However, the issue of what CONTRIBUTING.MD says hasn't been resolved. I wonder if that should be further discussed in the original issue that this PR addresses.

Or just open a PR removing it and discuss there.

[JordanMartinez]

Or just open a PR removing it and discuss there.

See #1841.

[JordanMartinez]

Don't merge this yet. Similar to the StateT Arbitrary comment in #1838, the type parameters are not swapped for the Arbitrary's of EitherT, etc.

[JordanMartinez]

After checking this again, only the Abritraries of EitherT, IndexedContsT, and WriterT's type params weren't swapped yet. The others were swapped already in their original commit.

[JordanMartinez]

Not sure whether this is waiting for other members' approval, or the core contributors are just busy, or it is believed that this PR isn't ready for merging due to my prior comment. So, just to clarify, the final issue has been resolved and I believe this is ready for merging/review from others (if that's what's needed).

[jdegoes]

@jonm Looks good! Thank you Jordan!