Workaround for ambiguous implicits when using MTL type classes, fixes…

[adelbertc]

… #1210

Putting this up so folks can see what I'm doing as I do it. Here's the checklist of type classes I found that I'll be doing the change for:

• ApplicativeError + MonadError
• FunctorFilter + TraverseFilter + MonadFilter
• Alternative + MonadCombine
• MonadReader
• MonadState
• MonadWriter

More details on the change:

---

Before the MTL type classes were encoded as usual:

trait MonadReader[F[_], R] extends Monad[F] { /* stuff */ }

This change changes the encoding for the above type classes into:

trait MonadReader[F[_], R] {
  def monad: Monad[F]

  /* stuff */
}

I asked around and this seems to be a not uncommon approach [1] [2] .

Caveats:

Before given MonadReader[F, R] you would get a Monad[F] instance automagically, meaning this works:

def foo[F[_], R](implicit F: MonadReader[F, R]): F[(R, R)] =
  for {
    r1 <- F.ask
    r2 <- F.ask
  } yield (r1, r2

With this MonadReader is completely out of the subtype hierarchy so you now either need to do one of the following:

// Note the need to specify Monad[F]
def foo[F[_], R](implicit F0: Monad[F], F1: MonadReader[F, R]): F[(R, R)] = ...

def foo[F[_], R](implicit F: MonadReader[F, R]): F[(R, R)] = {
  implicit val monad = F.monad
  ...
}

I think this is well worth it to free up the ergonomics of using the MTL type classes (see #1210). The downside is these type classes get treated specially, but weighing the benefits against the cost I like this a lot more. I've thought about this for a long while and I've yet to come up with a better encoding than this given the current subtype hierarchy.

[adelbertc]

OK so the build is failing, it's unfortunate we run validateJS first but locally if I do testOnly *OneAndTests under testsJVM one of (many) strange NPEs I get are:

[info] - OneAnd[ListWrapper, Int].semigroupK.semigroupK associative *** FAILED *** (1 millisecond)
[info]   NullPointerException was thrown during property evaluation.
[info]     Message: "None"
[info]     Occurred when passed generated values (
[info]       arg0 = OneAnd(-2147483648,ListWrapper(List())),
[info]       arg1 = OneAnd(492726168,ListWrapper(List())),
[info]       arg2 = OneAnd(1,ListWrapper(List()))
[info]     )

/* elided.. */

[info]   Cause: java.lang.NullPointerException:
[info]   at cats.data.OneAnd.combine(OneAnd.scala:37)
[info]   at cats.data.OneAndInstances$$anon$7.combineK(OneAnd.scala:110)
[info]   at cats.data.OneAndInstances$$anon$7.combineK(OneAnd.scala:108)
[info]   at cats.laws.SemigroupKLaws$class.semigroupKAssociative(SemigroupKLaws.scala:11)
[info]   at cats.laws.SemigroupKLaws$$anon$1.semigroupKAssociative(SemigroupKLaws.scala:16)
[info]   at cats.laws.discipline.SemigroupKTests$$anonfun$semigroupK$1.apply(SemigroupKTests.scala:19)
[info]   at cats.laws.discipline.SemigroupKTests$$anonfun$semigroupK$1.apply(SemigroupKTests.scala:19)

Which when looking at the OneAnd code I can't figure out why. I've tried moving some stuff around but no go :( Any ideas?

EDIT Nevermind found it. Was referencing a val declared below its use site.

[codecov-io]

Current coverage is 91.61% (diff: 95.21%)

Merging #1379 into master will decrease coverage by 0.12%

@@             master      #1379   diff @@
==========================================
  Files           239        239          
  Lines          3596       3673    +77   
  Methods        3528       3609    +81   
  Messages          0          0          
  Branches         67         63     -4   
==========================================
+ Hits           3299       3365    +66   
- Misses          297        308    +11   
  Partials          0          0          

Powered by Codecov. Last update 4bb7dbb...9c639b3

[ceedubs]

@adelbertc it looks like there's a merge conflict.

I think I'm 👍 on this change. As documented in #1210, it's currently pretty much impossible to have elegant MTL-style code with Cats. I don't particularly like that it's different than how we are encoding type class hierarchies elsewhere, but I don't know of a better approach, and I guess if nothing else it's a first attempt at an alternative type class hierarchy encoding strategy that maybe we'll end up wanting to use elsewhere.

One thing that we could do is change def monad: Monad[F] to implicit def monad: Monad[F] which would allow people to do import F._ and have their implicit Monad[F] instance in scope. However, I imagine if you go down that path you are just going to run into other ambiguities eventually, so it's probably best to stick with what's here.

[adelbertc]

@ceedubs Yeah it's unfortunate it's a different encoding, but it leads to niceties. If you look at MTLTests.scala you can see we're now able to get syntax with all those instances in scope. I think the rule of thumb here is to use this encoding if the hierarchy starts "forking" and it's common enough to use type classes from either path. This is particularly common for the usual MonadReader, MonadWriter, etc. I could also see it for the various *Filters so I included those as well.

Re: changing the member to implicit, I originally had it that way but for the same reason as you stated I ended up making it non-implicit. That way you can opt in if you want to bring the instance into scope with implicit val instance = F.monad whereas if it's marked implicit already you can't opt out.

---

On a related note, I am OK if folks end up not liking having the alternate encoding, I would probably just end up making a separate cats-mtl project or something and have my own MTL-y things there, but figured I'd try in Cats proper first.

Merge conflict should be easy enough to resolve, but would like to get buy-in from other folks first. Since this touches a bunch of stuff and makes some fairly big changes, I'd like to get three more 👍 s on top of @ceedubs before proceeding. Pinging some folks who've spoken up on #1210 to get thoughts/review @johnynek @alexandru

[ceedubs]

On a related note, I am OK if folks end up not liking having the alternate encoding, I would probably just end up making a separate cats-mtl project or something and have my own MTL-y things there, but figured I'd try in Cats proper first.

Personally I don't think that there's much point in having MTL classes in Cats if they are mostly unusable, so I support the approach presented in this PR. If others really don't want to bring this style of type class encoding into cats, then at that point I'm not sure the MTL classes should even exist in Cats.

[adelbertc]

@ceedubs I'm beginning to prefer having a separate cats-mtl project and rip out the MTL type classes for Cats. Not sure how others feel about this.

If we do go forward with this, we also need to codify what constitutes an "MTL class" that we want to remove. The obvious ones are ApplicativeError, MonadError, MonadReader, MonadState, MonadWriter. The difficulty lies in the other ones I modified in this PR: MonadPlus, Alternative, FunctorFilter, TraverseFilter, and MonadFilter.

With the obvious ones ripped out, you can still get ambiguous implicits from a reasonable combination of these:

• MonadPlus + MonadFilter (ambiguous Monad)
• TraverseFilter + MonadFilter/Alternative (ambiguous Functor - this is enough to eliminate use of for-comprehensions as the last call will be map, effectively eliminating ergonomic use of monad syntax as well)

Even more ambiguities arise with the usual MTL type classes.

to name two.

That being said there's also been some issues surrounding the mere existence of some of these type classes:

• MonadFilter: Remove MonadFilter, and fix MonadCombineLaws #1342 /cc @eed3si9n
• FunctorFilter: Remove FunctorFilter #1365 /cc @kcsongor

And also some additions of similar ones:

• FunctorFlatten: Add AlternativeFlatten #1337 /cc @johnynek

I think we should take this PR to discuss how we want to approach these moving forward. (on a side note I think these problems succinctly demonstrate a big flaw in the subtype approach to encoding type classes). I think Spire has a lot of this kind of branching of type classes, would be curious to hear @non and @tixxit 's thoughts on the matter.

[ceedubs]

TraverseFilter + MonadFilter/Alternative (ambiguous Functor)

@adelbertc simply Traverse and Monad are enough to hit an ambiguous Functor instance, right?

[adelbertc]

@ceedubs Yep

[johnynek]

In my view, a reasonable rule would be to allow at most one subclass typeclass within cats, and document or have some tool check that.

I don't like having some vague notion of mtl since I don't see how ApplicativeError or Traverse can be considered mtl yet trigger the issue.

A more invasive solution is what we have discussed (methods to return subtype typeclasses rather than extension).

The algebra issue is not as accute because generally you don't accept many different algebras. You just take the least specific you need for the method so I don't think it comes up like it does here.

[adelbertc]

In my view, a reasonable rule would be to allow at most one subclass typeclass within cats, and document or have some tool check that.

What does that mean for stuff like Traverse and Applicative (both subclasses of Functor)? Or FunctorFilter, FunctorFlatten, etc?

[johnynek]

The benefit of the invasive solution is that it is simpler to review and implement generally. The downside is that it seems like a much bigger change for library consumers.

Finally, I'm not sure this is totally a disaster currently. Yes you et ambiguous implicits for Monad if you accept implicit MonadCombjne and MonadError but you can manually resolve such conflicts by explicitly choosing one right? And explicitly choosing one is likely what happens with the def monad approach so I don't really see why it is better.

[adelbertc]

@johnynek It makes the ergonomics of using Monads very poor. It destroys for-comprehensions, and while you can still explicitly write out F0.flatMap(a => a.flatMap(b => b.map(c => ...))) it become a heavy burden. With the member approach you can recover this either by just adding an additional albeit redundant Monad constraint, or making the member implicit in local scope, so syntax is recovered.

Note that it is not just the typical MTL type classes that completely break for-comprehension, just getting ambiguous Functor instances is enough. Traverse + Applicative, FunctorFilter + Applicative, FunctorFilter + FunctorFlatten, etc.

[adelbertc]

Wrote a more detailed overview of issues with subtyping + type classes: https://github.com/adelbertc/faq/blob/2e8c7953422401879c5cfcf3ffc2d24675ec0fd8/src/main/tut/subtype-typeclasses.compiled.md

EDIT Now on Typelevel blog with some more info: http://typelevel.org/blog/2016/09/30/subtype-typeclasses.html

[alexandru]

I'm beginning to prefer having a separate cats-mtl project and rip out the MTL type classes for Cats. Not sure how others feel about this.

When I first read that ticket, I didn't even know what MTL stands for, all I could infer is that the inheritance-based encoding creates problems, because indeed I bumped into some problems myself. But that's a problem, as how can you explain the separation to people unfamiliar with this? Unless the explanation is that cats-core is about the Monad and cats-mtl is everything else :-P

Personally I like this composition based encoding, even if bringing implicits in the local scope is required, however right now I have mixed feelings about having to work with 2 type-class encodings. The uninitiated can then wonder why is Monad allowed to extend Applicative, but MonadFilter isn't allowed to extend Monad. Because of popularity? Thing is designing type-classes is hard even for some of us that are familiar with Cats' hierarchy, and asking people to think about how the type hierarchy gets forked is a tough sell. It creates some confusion I think.

I don't know what the right answer is, but right now I'm thinking that consistency might be better than optimizing some use-cases at the expense of confusion, even if we end up working with F.flatMap(fa)(a => F.map(fb)(b => a + b)).

[adelbertc]

@johnynek Re: your comment on #1210 (comment), as was pointed out to me by @jbgi that the hierarchy need not be baked into the type classes themselves as that would restrict extensibility, instead it could be moved out albeit at the cost of requiring an import on every use site now.

See: https://github.com/scalaz/scalaz/blob/series/8.0.x/base/src/main/scala/BaseHierarchy.scala and https://github.com/scalaz/scalaz/blob/series/8.0.x/base/src/main/scala/package.scala - using the hierarchy requires an import scalaz._ but it can be replaced with a different import if you're using type classes from other sources.

[adelbertc]

Ping. How do we want to proceed with this?

cc @non

EDIT I am for the invasive change as it solves the root of the problem as opposed to putting lipstick on the proverbial pig (maybe a bit harsh but yeah :P )

[johnynek]

I'm pretty nervous about this. I feel like the current situation is usable
in most common cases pretty easily, but has some know trouble spots. I want
cats to be usable first, some I'm less enthusiastic about experimental
techniques.

I think this problem deserves a clear doc with what seem to me like at
least 3 ways forward laid out (continue as is, linear subclassing,
no-subclassing). I'd like to see what some common and not so common cases
look like with each of the three.
On Wed, Oct 12, 2016 at 09:03 Adelbert Chang notifications@github.com
wrote:

Ping. How do we want to proceed with this?

cc @non https://github.com/non

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#1379 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEJduZA741dW-mQuWMwoDFfKYCW4zqYks5qzS8OgaJpZM4J_r1e
.

[milessabin]

I think this is too drastic a change to merge without exploring the space of possibilities more thoroughly.

At least with respect to the motivating example here (Monad and Traverse both contributing ambiguity-making Functor instances) I think there is a workable alternative.

At least part of the problem stems from the fact that we ask for the instances of Monad[F] and Traverse[F] independently. As such, we have no guarantee that the underlying Functor[F] instances are the same. Haskell's global uniqueness constraint isn't on the table (a good thing IMO), however we can make a local guarantee of identity if we ask for the right thing: instead of asking for the two instances separately, we ask for them together: Monad[F] with Traverse[F]. Now we're guaranteed that the Functor[F] as seen from Monad[F] is consistent with the Functor[F] as seen from Traverse[F] because they're identical, and the compiler knows it.

This gets along very nicely with the way that standard instances are defined in Cats: rather than providing instances for List independently we provide a single instance which is the intersection of all of the instances and provides each of them via subtyping. Note that this doesn't prevent you from asking for instances separately, but there you have no guarantees that the underlying Functor is the same and so explicit disambiguation seems not just reasonable, but actually downright desirable.

This solves the local consistency problem, but we still have to deal with ambiguities which arise from the way that syntax is applied: we have syntax for Functor, for Monad and Traverse, all providing a map extension method. The Functor syntax is eliminated because it is less specific, however the syntax for Monad and Traverse are equally specific and hence generate ambiguity.

But we can fix this by creating more specific syntax: syntax for Monad[F] with Traverse[F]. This adds a little boilerplate but I don't think it is all that excessive. Most importantly it only has to be provided once for each common combination of instances. In use this might look like,

// Nb. all syntax is in scope ...
import ListInstances._, FunctorSyntax._, MonadSyntax._, TraverseSyntax._, MTSyntax._

def foo[F[_]](implicit MT: Monad[F] with Traverse[F]): F[Int] = {
   for {
    a <- MT.pure(10)
    b <- MT.pure(20)
  } yield a+b
}

Full example here.

[adelbertc]

I remember talking to @S11001001 about this on IRC.. I think there was concern that with isn't really intersection in that it doesn't commute? That A with B isn't the same as B with A, though I can't seem to reproduce it at the moment.

Another issue is that doesn't work for data types where the instances are definitely separately, such as monad transformers.

scala> import cats._
import cats._

scala> import cats.implicits._
import cats.implicits._

scala> def foo[F[_]](implicit F: Monad[F] with Traverse[F]): Int = 42
foo: [F[_]](implicit F: cats.Monad[F] with cats.Traverse[F])Int

scala> foo[Option]
res0: Int = 42

scala> import cats.data.OptionT
import cats.data.OptionT

scala> foo[OptionT[List, ?]]
<console>:21: error: could not find implicit value for parameter F: cats.Monad[[β]cats.data.OptionT[[+A]List[A],β]] with cats.Traverse[[β]cats.data.OptionT[[+A]List[A],β]]
       foo[OptionT[List, ?]]
          ^

scala> type Alias[A] = OptionT[List, A]
defined type alias Alias

scala> foo[Alias]
<console>:22: error: could not find implicit value for parameter F: cats.Monad[Alias] with cats.Traverse[Alias]
       foo[Alias]
          ^

scala> Monad[OptionT[List, ?]]
res2: cats.Monad[[β]cats.data.OptionT[[+A]List[A],β]] = cats.data.OptionTInstances$$anon$2@465c0f8d

scala> Traverse[OptionT[List, ?]]
res3: cats.Traverse[[β]cats.data.OptionT[[+A]List[A],β]] = cats.data.OptionTInstances2$$anon$3@2bd44feb

or simplified:

scala> import cats._
import cats._

scala> import cats.implicits._
import cats.implicits._

scala> def foo[F[_]](implicit F: Monad[F] with Traverse[F]): Int = 42
foo: [F[_]](implicit F: cats.Monad[F] with cats.Traverse[F])Int

scala> case class Foo[A](a: A)
defined class Foo

scala> implicit def monadFoo: Monad[Foo] = ???
monadFoo: cats.Monad[Foo]

scala> implicit def traverseFoo: Traverse[Foo] = ???
traverseFoo: cats.Traverse[Foo]

scala> foo[Foo]
<console>:23: error: could not find implicit value for parameter F: cats.Monad[Foo] with cats.Traverse[Foo]
       foo[Foo]
          ^

[milessabin]

A with B is equivalent to B with A with respect to subtyping: both are subtypes of A with B with C and both have A and B as supertypes. That's all that's needed for this mechanism.

I think the fact that separately defined instances don't conform to Monad[F] with Traverse[F] is a feature, not a bug ... otherwise how do you know that the separately defined instances have consistent underlying functors? The point of the intersection is that it asserts, in a checkable way, the consistency that you've been asking for in other contexts.

If you can provide an alternative proof of consistency that'd be fine. One way to do that would be to construct a local Monad[F] with Traverse[F] instance given independently defined Monad[F] and Traverse[F] instances.

[adelbertc]

Monad[F] with Traverse[F] not working for separately defined instances makes sense for the reasons you stated, but only in a world where instances are not globally unique ;)

That being said, this appears to give second class status to monad transformers or any type where instances are defined separately (Prod, Nested..). The solution works I suppose, but is very cumbersome on clients.

I asked in the Idris IRC channel to see what happens in this bit of code (which is probably syntactically incorrect but I hope communicates the message):

functorOnly :: Functor f => f ()

monadAndTraverse :: (Monad f, Traverse f) => f ()
monadAndTraverse = functorOnly

x = monadAndTraverse :: List Unit
y = monadAndTraverse @{myOwnMonad} @{myOwnTraverse}

specifically how y behaves. In Idris unnamed instances are globally unique so x's call to monadAndTraverse delegates to functorOnly just fine, but since y is passing in instances explicitly there is no guarantee which Functor instance monadAndTraverse chooses to propagate to functorOnly.

If "Cats" ends up deciding global uniqueness (interesting read on that btw: http://blog.ezyang.com/2014/07/type-classes-confluence-coherence-global-uniqueness/) is not something we care about (at the time of this writing I know I do, as well as a couple others) I think we need to come up with a more satisfying solution than making clients construct instances on the fly. Perhaps we can play games with syntax prioritization or something.

[alexandru]

While the arguments of @milessabin make sense, I'd like to draw attention to the fact that using Monad[F] with Traverse[F] makes conversions from/to alternative type-class hierarchies difficult.

Alternatives like Scalaz will be around for some time. Along with libraries that expose shims to be converted back and forth to Cats and Scalaz. Having functionality that depends on combinations like Monad[F] with Traverse[F] is bad for interoperability.

[edmundnoble]

@adelbertc saw it coming, now that MTL classes are in cats-mtl the encoding he mentioned is in use.

[adelbertc]

@edmundnoble i owe you a beer. or something beer-esque. awesome job man!