Follow up to Seq Instances

[djspiewak]

They don't appear to be working quite correctly? Follow up to #3620

scala> import cats._
import cats._

scala> import cats.syntax.all._
import cats.syntax.all._

scala> Seq(1, 2, 3) |+| Seq(3, 4, 5)
<console>:21: error: value |+| is not a member of Seq[Int]
       Seq(1, 2, 3) |+| Seq(3, 4, 5)
                    ^

scala> def foo[F[_]: Monad, A](fa: F[A]) = ()
foo: [F[_], A](fa: F[A])(implicit evidence$1: cats.Monad[F])Unit

scala> foo(Seq(1, 2, 3))
<console>:19: error: Could not find an instance of Monad for Seq
       foo(Seq(1, 2, 3))
          ^

scala> import cats.instances.seq._
import cats.instances.seq._

scala> foo(Seq(1, 2, 3))
<console>:22: error: Could not find an instance of Monad for Seq
       foo(Seq(1, 2, 3))
          ^

I'm guessing we need to add some tests here and see if we can introspect what's going on, but I would have expected this to work.

[djspiewak]

Update to the above: these are Scala 2.12 problems. On Scala 2.13, more of it works:

scala> import cats._, cats.syntax.all._
import cats._
import cats.syntax.all._

scala> Seq(1, 2, 3) |+| Seq(3, 4, 5)
                    ^
       error: value |+| is not a member of Seq[Int]
       did you mean ++: or :++?

scala> List(1, 2, 3) |+| List(3, 4, 5)
val res1: List[Int] = List(1, 2, 3, 3, 4, 5)

scala> def foo[F[_]: Monad, A](fa: F[A]) = ()
def foo[F[_], A](fa: F[A])(implicit evidence$1: cats.Monad[F]): Unit

scala> foo(Seq(1, 2, 3))

scala> foo(List(1, 2, 3))

scala> Seq(1, 2, 3) *> List(3, 4, 5)
val res5: Seq[Int] = List(3, 4, 5, 3, 4, 5, 3, 4, 5)

So the semigroup syntax still doesn't quite work, but the monad resolution does. The final expression though is something we should probably talk about, since I think a lot of us would have expected that to fail to compile.

[JosephMoniz]

For 2.12 Seq aliased mutable.Seq and in 2.13 it now references immutable.Seq. This just means that to use these typeclasses pre 2.13 you need to explicitly convert your datastructures to immutable.Seq. I think most people were previously getting around this by explicitly converting to either List or Vector and in 2.12 land they should continue to do that imo.

The way to make it work with out of the box source parody for 2.12 and 2.13 would be to implement these typeclasses for mutable.Seq as well. I don't have the strongest opinions on if doing that would be a good idea or not though.

[djspiewak]

This is compelling, IMO. We shouldn't implement the instances for mutable.Seq (they wouldn't be lawful anyway). The mutable/immutable Seq aliasing issue is a common issue pre-2.13, so this is just another instance of it. Cool!