Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updating to scala 2.13-M4 #2335

Merged
merged 20 commits into from
Jul 31, 2018
Merged

Conversation

kailuowang
Copy link
Contributor

@kailuowang kailuowang commented Jul 24, 2018

Will fix #2267
The progress so far is probably less 10%.

Done:

All dependencies are updated (except sbt-scoverage which is locally publish on my machine)

Identified work remaining:

  • sbt-scoverage 2.13-M4 release, blocking issue temporarily disable scoverage on 2.13
  • either update export-hook to 2.13-M4 or remove it from alleycats.
  • find a way to cross-compile deprecated collection API temporary remove the deprecation warning for 2.13

Update: it's by and large working. Personally, I think it's ready for a release of 1.2 on 2.13-M4
I created an issue for the steps I temporarily skipped in build on 2.13-M4 #2347 namely, the following are disabled on 2.13 build

  • code coverage disabled
  • deprecations are ignored. It's just too many backbreaking deprecations in scala.collections. I think the tooling/documentation on supporting cross compiling against collection API could get a bit better.
  • tut docs are disable, also due to the deprecated collection APIs.
  • doctests are disabled, due to deprecated collection API and different toString implementations in collections.

@kailuowang
Copy link
Contributor Author

Update: I managed to get all dependencies to 2.13-M4 (had to publishLocal sbt-scoverage but I think they'll release soon).
That being said, I think cross compiling against the new collection seems to be more work than I thought.
There are many places that we encounter backward breaking deprecations. It means to cross compile we'll need to create 2.13 specific code for them. @SethTisue I assume this is a common issue among libraries that cross compile. Is there a defacto solution yet?
FWIW, here are the errors I got so far on core.

[info] Compiling 47 Scala sources to /Users/kailuowang/projects/cats/kernel/.jvm/target/scala-2.13.0-M4/classes ...
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/Eq.scala:113:39: type TraversableOnce in package scala is deprecated (since 2.13.0): Use IterableOnce instead of TraversableOnce
[error]     override def combineAllOption(es: TraversableOnce[Eq[A]]): Option[Eq[A]] =
[error]                                       ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/Eq.scala:114:14: method isEmpty in class IterableOnceExtensionMethods is deprecated (since 2.13.0): Use .iterator.isEmpty instead of .isEmpty on IterableOnce
[error]       if (es.isEmpty) None
[error]              ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/Eq.scala:116:31: method toVector in class IterableOnceExtensionMethods is deprecated (since 2.13.0): Use Vector.from(it) instead of it.toVector on IterableOnce
[error]         val materialized = es.toVector
[error]                               ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/Eq.scala:129:39: type TraversableOnce in package scala is deprecated (since 2.13.0): Use IterableOnce instead of TraversableOnce
[error]     override def combineAllOption(es: TraversableOnce[Eq[A]]): Option[Eq[A]] =
[error]                                       ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/Eq.scala:130:14: method isEmpty in class IterableOnceExtensionMethods is deprecated (since 2.13.0): Use .iterator.isEmpty instead of .isEmpty on IterableOnce
[error]       if (es.isEmpty) None
[error]              ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/Eq.scala:132:31: method toVector in class IterableOnceExtensionMethods is deprecated (since 2.13.0): Use Vector.from(it) instead of it.toVector on IterableOnce
[error]         val materialized = es.toVector
[error]                               ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/Monoid.scala:35:22: type TraversableOnce in package scala is deprecated (since 2.13.0): Use IterableOnce instead of TraversableOnce
[error]   def combineAll(as: TraversableOnce[A]): A =
[error]                      ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/Monoid.scala:36:8: method foldLeft in class IterableOnceExtensionMethods is deprecated (since 2.13.0): Use .iterator.foldLeft instead of .foldLeft on IterableOnce
[error]     as.foldLeft(empty)(combine)
[error]        ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/Monoid.scala:38:37: type TraversableOnce in package scala is deprecated (since 2.13.0): Use IterableOnce instead of TraversableOnce
[error]   override def combineAllOption(as: TraversableOnce[A]): Option[A] =
[error]                                     ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/Monoid.scala:39:12: method isEmpty in class IterableOnceExtensionMethods is deprecated (since 2.13.0): Use .iterator.isEmpty instead of .isEmpty on IterableOnce
[error]     if (as.isEmpty) None else Some(combineAll(as))
[error]            ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/Monoid.scala:49:55: type TraversableOnce in package scala is deprecated (since 2.13.0): Use IterableOnce instead of TraversableOnce
[error]   def combineAll[@sp(Int, Long, Float, Double) A](as: TraversableOnce[A])(implicit ev: M[A]): A =
[error]                                                       ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/Semigroup.scala:40:28: type TraversableOnce in package scala is deprecated (since 2.13.0): Use IterableOnce instead of TraversableOnce
[error]   def combineAllOption(as: TraversableOnce[A]): Option[A] =
[error]                            ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/Semigroup.scala:69:31: type TraversableOnce in package scala is deprecated (since 2.13.0): Use IterableOnce instead of TraversableOnce
[error]   def combineAllOption[A](as: TraversableOnce[A])(implicit ev: S[A]): Option[A] =
[error]                               ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/StaticMethods.scala:15:9: overriding method + in trait MapOps of type [V1 >: V](kv: (K, V1))scala.collection.immutable.Map[K,V1];
[error]   method + needs `override' modifier
[error]     def +[V2 >: V](kv: (K, V2)): Map[K, V2] = m.toMap + kv
[error]         ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/StaticMethods.scala:16:9: overriding method - in trait MapOps of type (key: K)scala.collection.immutable.Map[K,V];
[error]   method - cannot override final member
[error]     def -(key: K): Map[K, V] = m.toMap - key
[error]         ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/StaticMethods.scala:11:25: class WrappedMutableMap needs to be abstract, since:
[error] it has 2 unimplemented members.
[error] /** As seen from class WrappedMutableMap, the missing signatures are as follows.
[error]  *  For convenience, these are usable as stub implementations.
[error]  */
[error]   def remove(key: K): scala.collection.immutable.Map[K,V] = ???
[error]   def updated[V1 >: V](key: K,value: V1): scala.collection.immutable.Map[K,V1] = ???
[error]   private[kernel] class WrappedMutableMap[K, V](m: mutable.Map[K, V]) extends Map[K, V] {
[error]                         ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/StaticMethods.scala:78:62: type TraversableOnce in package scala is deprecated (since 2.13.0): Use IterableOnce instead of TraversableOnce
[error]   def combineAllIterable[A, R](b: mutable.Builder[A, R], xs: TraversableOnce[Iterable[A]]): R = {
[error]                                                              ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/StaticMethods.scala:79:8: method foreach in class IterableOnceExtensionMethods is deprecated (since 2.13.0): Use .iterator.foreach(...) instead of .foreach(...) on IterableOnce
[error]     xs.foreach(b ++= _)
[error]        ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/StaticMethods.scala:118:26: type TraversableOnce in package scala is deprecated (since 2.13.0): Use IterableOnce instead of TraversableOnce
[error]   def orderedHash[A](xs: TraversableOnce[A])(implicit A: Hash[A]): Int = {
[error]                          ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/StaticMethods.scala:122:8: method foreach in class IterableOnceExtensionMethods is deprecated (since 2.13.0): Use .iterator.foreach(...) instead of .foreach(...) on IterableOnce
[error]     xs foreach { x =>
[error]        ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/list.scala:93:31: type TraversableOnce in package scala is deprecated (since 2.13.0): Use IterableOnce instead of TraversableOnce
[error]   override def combineAll(xs: TraversableOnce[List[A]]): List[A] =
[error]                               ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/map.scala:72:32: type TraversableOnce in package scala is deprecated (since 2.13.0): Use IterableOnce instead of TraversableOnce
[error]   override def combineAll(xss: TraversableOnce[Map[K, V]]): Map[K, V] = {
[error]                                ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/map.scala:74:9: method foreach in class IterableOnceExtensionMethods is deprecated (since 2.13.0): Use .iterator.foreach(...) instead of .foreach(...) on IterableOnce
[error]     xss.foreach { m =>
[error]         ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/queue.scala:57:31: type TraversableOnce in package scala is deprecated (since 2.13.0): Use IterableOnce instead of TraversableOnce
[error]   override def combineAll(xs: TraversableOnce[Queue[A]]): Queue[A] =
[error]                               ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:7:55: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   implicit def catsKernelStdOrderForStream[A: Order]: Order[Stream[A]] =
[error]                                                       ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:9:49: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   implicit def catsKernelStdMonoidForStream[A]: Monoid[Stream[A]] =
[error]                                                 ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:14:69: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   implicit def catsKernelStdPartialOrderForStream[A: PartialOrder]: PartialOrder[Stream[A]] =
[error]                                                                     ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:17:53: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   implicit def catsKernelStdHashForStream[A: Hash]: Hash[Stream[A]] =
[error]                                                     ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:22:49: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   implicit def catsKernelStdEqForStream[A: Eq]: Eq[Stream[A]] =
[error]                                                 ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:26:53: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error] class StreamOrder[A](implicit ev: Order[A]) extends Order[Stream[A]] {
[error]                                                     ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:27:19: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   def compare(xs: Stream[A], ys: Stream[A]): Int =
[error]                   ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:27:34: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   def compare(xs: Stream[A], ys: Stream[A]): Int =
[error]                                  ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:32:67: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error] class StreamPartialOrder[A](implicit ev: PartialOrder[A]) extends PartialOrder[Stream[A]] {
[error]                                                                   ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:33:26: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   def partialCompare(xs: Stream[A], ys: Stream[A]): Double =
[error]                          ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:33:41: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   def partialCompare(xs: Stream[A], ys: Stream[A]): Double =
[error]                                         ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:38:74: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error] class StreamHash[A](implicit ev: Hash[A]) extends StreamEq[A]()(ev) with Hash[Stream[A]] {
[error]                                                                          ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:39:16: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   def hash(xs: Stream[A]): Int = StaticMethods.orderedHash(xs)
[error]                ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:42:47: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error] class StreamEq[A](implicit ev: Eq[A]) extends Eq[Stream[A]] {
[error]                                               ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:43:15: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   def eqv(xs: Stream[A], ys: Stream[A]): Boolean =
[error]               ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:43:30: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   def eqv(xs: Stream[A], ys: Stream[A]): Boolean =
[error]                              ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:48:31: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error] class StreamMonoid[A] extends Monoid[Stream[A]] {
[error]                               ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:49:14: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   def empty: Stream[A] = Stream.empty
[error]              ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:49:26: value Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   def empty: Stream[A] = Stream.empty
[error]                          ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:50:18: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   def combine(x: Stream[A], y: Stream[A]): Stream[A] = x ++ y
[error]                  ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:50:32: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   def combine(x: Stream[A], y: Stream[A]): Stream[A] = x ++ y
[error]                                ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:50:44: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   def combine(x: Stream[A], y: Stream[A]): Stream[A] = x ++ y
[error]                                            ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:52:28: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   override def combineN(x: Stream[A], n: Int): Stream[A] =
[error]                            ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:52:48: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   override def combineN(x: Stream[A], n: Int): Stream[A] =
[error]                                                ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:53:36: value Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]     StaticMethods.combineNIterable(Stream.newBuilder[A], x, n)
[error]                                    ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:55:31: type TraversableOnce in package scala is deprecated (since 2.13.0): Use IterableOnce instead of TraversableOnce
[error]   override def combineAll(xs: TraversableOnce[Stream[A]]): Stream[A] =
[error]                               ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:55:60: type Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]   override def combineAll(xs: TraversableOnce[Stream[A]]): Stream[A] =
[error]                                                            ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/stream.scala:56:38: value Stream in package scala is deprecated (since 2.13.0): Use LazyList instead of Stream
[error]     StaticMethods.combineAllIterable(Stream.newBuilder[A], xs)
[error]                                      ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/string.scala:25:31: type TraversableOnce in package scala is deprecated (since 2.13.0): Use IterableOnce instead of TraversableOnce
[error]   override def combineAll(xs: TraversableOnce[String]): String = {
[error]                               ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/string.scala:27:8: method foreach in class IterableOnceExtensionMethods is deprecated (since 2.13.0): Use .iterator.foreach(...) instead of .foreach(...) on IterableOnce
[error]     xs.foreach(sb.append)
[error]        ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/unit.scala:36:37: type TraversableOnce in package scala is deprecated (since 2.13.0): Use IterableOnce instead of TraversableOnce
[error]   override def combineAllOption(as: TraversableOnce[Unit]): Option[Unit] =
[error]                                     ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/unit.scala:37:12: method isEmpty in class IterableOnceExtensionMethods is deprecated (since 2.13.0): Use .iterator.isEmpty instead of .isEmpty on IterableOnce
[error]     if (as.isEmpty) None else Some(())
[error]            ^
[error] /Users/kailuowang/projects/cats/kernel/src/main/scala/cats/kernel/instances/vector.scala:55:31: type TraversableOnce in package scala is deprecated (since 2.13.0): Use IterableOnce instead of TraversableOnce
[error]   override def combineAll(xs: TraversableOnce[Vector[A]]): Vector[A] =
[error]                               ^
[error] 57 errors found
[error] (kernelJVM / Compile / compileIncremental) Compilation failed
[error] Total time: 8 s, completed Jul 24, 2018 2:33:39 PM

@mpilquist
Copy link
Member

@kailuowang
Copy link
Contributor Author

@mpilquist I haven't, thanks for the pointer.

@SethTisue
Copy link
Member

I've replied at #2267

@kailuowang
Copy link
Contributor Author

@SethTisue do you happen to have a quick example project of building 2.13 and 2.12 specific source? I've done similar thing before but it was a bit hacky, wonder if sbt 1.1 enabled some new techniques.

@SethTisue
Copy link
Member

SethTisue commented Jul 24, 2018

do you happen to have a quick example project of building 2.13 and 2.12 specific source? I've done similar thing before but it was a bit hacky, wonder if sbt 1.1 enabled some new techniques.

if you have src/main/scala-2.11, src/main/scala-2.12, and so on, sbt will pick those up automatically with zero added configuration (according to scalaBinaryVersion). it works for src/test too

if you want something custom like "use these sources for 2.11 and 2.12, and these other sources for 2.13 and above", then you need something like:

unmanagedSourceDirectories +=
  (CrossVersion.partialVersion(scalaVersion.value) match {
      case (2, y) if y <= 12 => new File(scalaSource.value.getPath + "_2.11-12")
      case _ => new File(scalaSource.value.getPath + "_2.13")
  })

that kind of thing, whatever exact logic and naming you want

@ghost
Copy link

ghost commented Jul 24, 2018

would an infile implementation be of use, eg:

if (Compiler.is213) x.foo else x.bar

if so, I think we could do something along the lines as we do in catalysts.platform but by scala version, not platform

@kailuowang
Copy link
Contributor Author

Thanks @SethTisue

@kailuowang
Copy link
Contributor Author

FWIW, for cross projects (build scalajs too), I ended up using the following settings to support scala-version-specific source directories

  Compile / unmanagedSourceDirectories ++= {
    val bd = baseDirectory.value
    (CrossVersion.partialVersion(scalaVersion.value) match {
      case Some((2, y)) if y <= 12 =>
        CrossType.Pure.sharedSrcDir(bd, "main").toList map (f => file(f.getPath + "-2.12-"))
      case _ => Nil
    })
  },

@ghost
Copy link

ghost commented Jul 25, 2018

I'll push what i did in catalysts soon/later. We can can compare.....

@kailuowang
Copy link
Contributor Author

I might found a change in implicit priority.

ambiguous implicit values:
[error] both method catsDataMonadErrorMonadForOptionT in class OptionTInstances1 of type F[]cats.MonadError[[β$11$]cats.data.OptionT[F,β$11$],Unit]
[error] and method catsDataMonadForOptionT in class OptionTInstances of type F[
]cats.Monad[[β$3$]cats.data.OptionT[F,β$3$]]
[error] match expected type cats.Applicative[[β$0$]cats.data.OptionT[cats.Eval,β$0$]]
[error] val a: OptionT[Eval, Int] = 1.pure[OptionT[Eval, ?]]
[error] ^

Note that the two conflicting instances MonadError and Monad are defined in two different classes in the inheritance hierarchy, but they have exactly the same input siganture:

  implicit def catsDataMonadForOptionT[F[_]](implicit F0: Monad[F]): Monad[OptionT[F, ?]]

and

implicit def catsDataMonadErrorMonadForOptionT[F[_]](implicit F0: Monad[F]): MonadError[OptionT[F, ?], Unit]

The return type of the second one is a subtype of the first one.
The fix on our side is to simply remove the first one since it's redundant. But is this something worth reporting? @SethTisue

@ghost
Copy link

ghost commented Jul 25, 2018

In the 2.13.0-M3 build, implicit resolutions was a big factor in the build, eg see https://travis-ci.org/typelevel/cats/jobs/355615303

So might be worth raising an issue, even if no longer an issue here?

@ghost
Copy link

ghost commented Jul 25, 2018

Alot of the build changes are extremely similar to the ones I've done in sbt-catalysts, but not pushed yet.

I wonder if now might be a convenient time to hook cats up with sbt-catalysts? Not a full migration, just things like scalac-options, helper methods etc.if you push what you have. I can have a look. Your call

@kailuowang
Copy link
Contributor Author

Another issue is that the toString implementation of collections is changed e.g. from Map(1 -> b) to TreeMap(1 -> b). This breaks our doctests since they assert on repl outputs. I am going to temporarily disable doctests on 2.13 for now and decide later if we have the bandwidth to have a cross compile solution.

@kailuowang
Copy link
Contributor Author

I wonder if now might be a convenient time to hook Cats up with sbt-catalysts? Not a full migration, just things like scalac-options, helper methods etc.

Sounds a good idea. Let me finish up this update to get a full sense of what can be shared, then I'll push something to sbt-catalysts. I think for now it's not a bad idea to just copy paste between projects.

@kailuowang
Copy link
Contributor Author

A new implicit resolution change that might be a blocker for us.

ambiguous implicit values:
[error] both method catsDataSemigroupForOptionT in class OptionTInstances of type [F[], A](implicit F0: cats.Semigroup[F[Option[A]]])cats.Semigroup[cats.data.OptionT[F,A]]
[error] and method catsDataMonoidForOptionT in class OptionTInstances0 of type [F[
], A](implicit F0: cats.Monoid[F[Option[A]]])cats.Monoid[cats.data.OptionT[F,A]]
[error] match expected type cats.kernel.Semigroup[cats.data.OptionT[cats.tests.ListWrapper,Int]]
[error] Semigroup[OptionT[ListWrapper, Int]]

Unlike the previous ambiguous error, there is no redundancy in it.
The two methods are

  implicit def catsDataSemigroupForOptionT[F[_], A](implicit F0: Semigroup[F[Option[A]]]): Semigroup[OptionT[F, A]]

and

 implicit def catsDataMonoidForOptionT[F[_], A](implicit F0: Monoid[F[Option[A]]]): Monoid[OptionT[F, A]] 

Since they are defined in different classes in the inheritance hierarchy, they should NOT have caused conflicts. It doesn't appear that this is happening to all implicit definitions organized through class inheritance, but I am seeing multiple of such errors for multiple data structures, e.g. Kleisli, OneAnd and OptionT

@kailuowang
Copy link
Contributor Author

so the problem is probably the wrong order of instances in our priority hierarchy.
from @joroKr21 's comment

There seem to be many such cases that I would deem "implicit priority inversion". For example Trampoline is a concrete instance of Free yet the Monad instance for Trampoline is defined in a superclass of the Monad instance for Free. Another example is the MonadError instance for EitherT being defined in a superclass of the Monad instance for EitherT, although MonadError is a subclass of Monad.

I'll start addressing these tomorrow.

@kailuowang
Copy link
Contributor Author

kailuowang commented Jul 26, 2018

Found a problem with the ParallelInstance for IorT
In IorT.scala there are two Parallel instance definition. It is prioritized so that if the user has a Parallel instance in scope it would resolve one for IorT that uses the parallel effect. The other instance that only requires Monad[F], this prioritization no longer resolves in 2.13-M4. Right now I can't think of a solution that is binary compatible and source compatible.

See discussion here and here

@smarter
Copy link
Contributor

smarter commented Jul 26, 2018

By adding a dummy type refinement to the instance in the subclass you should be able to make it more specific, e.g.:

class Foo[A, B]

class FooLow1 {
  implicit def one[T]: Foo[T, T] = ???
}

object Foo extends FooLow1 {
  implicit def two[T, S]: Foo[T, S] { type Dummy } = ???

  def test[X] = {
    implicitly[Foo[X, X]] // Picks "two"
  }
}

If I understand your specific case exactly this would be:

  implicit def catsDataParallelForIorTWithParallelEffect[M[_], F[_], E]
(implicit P: Parallel[M, F], E: Semigroup[E]): Parallel[IorT[M, E, ?], IorT[F, E, ?]] { type Dummy } = new Parallel[IorT[M, E, ?], IorT[F, E, ?]]
  {
    type Dummy
    // Rest as before
    // ...
  }

@SethTisue
Copy link
Member

SethTisue commented Jul 27, 2018

the discussion on this has been spread out around different venues... the latest seems to be that @smarter's suggestion (also made at https://gitter.im/scala/contributors?at=5b5a437f3396495b333e6102) seems promising, @kailuowang will try it

@kailuowang
Copy link
Contributor Author

@smarter that worked. Thanks!

@joroKr21
Copy link
Member

joroKr21 commented Jul 27, 2018

Extra type parameter with equality constraint might work if we define our own higher-kinded type equality:

sealed trait HkEq[F[_], G[_]] {
  def apply[A](fa: F[A]): G[A]
}

object HkEq {
  implicit def refl[F[_]]: HkEq[F, F] = new HkEq[F, F] {
    def apply[A](fa: F[A]): F[A] = fa
  }
}

But of course that would change the type signature.
I guess then could leave old method non-implicit for compatibility.

@kailuowang
Copy link
Contributor Author

update, we still have a bunch of back-breaking deprecations. I am going to simply ignore them for now, i.e. disable --deprecation and --fatal-warning on 2.13.

@kailuowang
Copy link
Contributor Author

Update: all modules are updated to 2.13. The only thing blocking us from releasing cats on 2.13-M4 is scoverage. Given the complexity of scoverage update (they are considering re-write coverage report file format from xml to something else) I suggest that we just disable coverage on 2.13 for now

@LukaJCB
Copy link
Member

LukaJCB commented Jul 28, 2018

👍

Copy link
Contributor

@johnynek johnynek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this. It’s a lot of work and not the most fun. Great job!

mimaBinaryIssueFilters ++= {
import com.typesafe.tools.mima.core._
import com.typesafe.tools.mima.core.ProblemFilters._
//Only sealed abstract classes that provide implicit instances to companion objects are allowed here, since they don't affect usage outside of the file.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a good option to add to mima: ignore changes when the or trait is sealed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a placeholder issue at the mima repo, but it needs fleshing out to become actionable. lightbend-labs/mima#237

@codecov-io
Copy link

codecov-io commented Jul 28, 2018

Codecov Report

Merging #2335 into 1.2.x will decrease coverage by 0.08%.
The diff coverage is 80.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##            1.2.x    #2335      +/-   ##
==========================================
- Coverage   95.09%   95.01%   -0.09%     
==========================================
  Files         345      349       +4     
  Lines        5991     5993       +2     
  Branches      219      225       +6     
==========================================
- Hits         5697     5694       -3     
- Misses        294      299       +5
Impacted Files Coverage Δ
core/src/main/scala/cats/NonEmptyTraverse.scala 100% <ø> (ø) ⬆️
...lleycats-core/src/main/scala/alleycats/Empty.scala 0% <ø> (ø) ⬆️
...12-/cats/kernel/compat/WrappedMutableMapBase.scala 0% <0%> (ø)
...ala-2.12-/cats/kernel/compat/TraversableOnce.scala 0% <0%> (ø)
...2.12-/alleycats/compat/IterableEmptyInstance.scala 0% <0%> (ø)
...e/src/main/scala-2.12-/cats/compat/SortedSet.scala 0% <0%> (ø)
core/src/main/scala/cats/data/IorT.scala 97.79% <100%> (ø) ⬆️
core/src/main/scala/cats/data/NonEmptySet.scala 98.78% <100%> (+0.03%) ⬆️
core/src/main/scala/cats/data/Kleisli.scala 97.08% <100%> (-0.03%) ⬇️
core/src/main/scala/cats/instances/parallel.scala 100% <100%> (ø) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16f9d81...228bbe1. Read the comment docs.

@kailuowang kailuowang changed the title [WIP] updating to scala 2.13-M4 Updating to scala 2.13-M4 Jul 30, 2018
@kailuowang
Copy link
Contributor Author

kailuowang commented Jul 30, 2018

Update: it's by and large working. Personally, I think it's ready for a release of 1.2 on 2.13-M4
I created an issue for the steps I temporarily skipped in build on 2.13-M4 #2347 namely, the following are disabled on 2.13 build

  • code coverage disabled
  • deprecations are ignored. It's just too many backbreaking deprecations in scala.collections. I think the tooling/documentation on supporting cross compiling against collection API could get a bit better.
  • tut docs are disable, also due to the deprecated collection APIs.
  • doctests are disabled, due to deprecated collection API and different toString implementations in collections.

Please vote on this comment if you are for (👍 ) or against (👎 ) releasing 1.2.0 on 2.13 in the current status.

@milessabin
Copy link
Member

:shipit:

@ghost
Copy link

ghost commented Jul 31, 2018

You've added issues for the flaky bits, no doubt consumer projects will find their own itches with M4, but only when this is released.

So my vote goes to ship it ASAP 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants