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.

Sign up for GitHub

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

Enable MiMa for Scala 3 #4063

Closed
wants to merge 4 commits into from
Closed

Enable MiMa for Scala 3 #4063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7c8578d
Enable MiMa for Scala 3
armanbilge Dec 1, 2021
de49f0a
Fix validateBC for Scala 3
armanbilge Dec 1, 2021
e8c4306
Fix catsInstancesForId bincompat on Scala 3
armanbilge Dec 1, 2021
7d23def
More filters
armanbilge Dec 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ jobs:
run: sbt ++${{ matrix.scala }} buildJVM bench/test

- name: Binary compatibility ${{ matrix.scala }}
if: matrix.platform == 'jvm' && (matrix.scala != '3.0.2')
if: matrix.platform == 'jvm'
run: sbt ++${{ matrix.scala }} clean validateBC

scalafix:
Expand Down
2 changes: 1 addition & 1 deletion binCompatTest/src/main/scala/catsBC/MimaExceptions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ object MimaExceptions {
cats.data.Kleisli.catsDataCommutativeFlatMapForKleisli[Option, Int],
cats.data.IRWST.catsDataStrongForIRWST[List, Int, Int, Int],
cats.data.OptionT.catsDataMonadErrorMonadForOptionT[List],
FunctionK.lift(headOption),
// FunctionK.lift(headOption), // Doesn't work in Scala 3
Copy link
Member Author

Choose a reason for hiding this comment

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

Anyone know anything about this?

Copy link
Member

Choose a reason for hiding this comment

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

That's a macro which is not implemented for Scala 3:
https://github.com/typelevel/cats/blob/main/core/src/main/scala-2.x/src/main/scala/cats/arrow/FunctionKMacros.scala

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it okay to disable, or maybe we should split this test into Scala 2 / Scala 3?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what this is testing TBH

cats.data.OptionT.catsDataMonadErrorForOptionT[Either[String, *], String],
cats.data.OptionT[Either[String, *], Int](Right(Some(17))).ensure("error")(_ => true),
"blah".leftNec[Int],
Expand Down
32 changes: 24 additions & 8 deletions build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ ThisBuild / githubWorkflowBuild := Seq(
WorkflowStep.Sbt(
List("clean", "validateBC"), // cleaning here to avoid issues with codecov
name = Some("Binary compatibility ${{ matrix.scala }}"),
cond = Some(JvmCond + " && " + Scala2Cond)
cond = Some(JvmCond)
)
)

Expand Down Expand Up @@ -339,18 +339,21 @@ def mimaPrevious(moduleName: String, scalaVer: String, ver: String, includeCats1
Version(ver) match {
case Some(Version(major, Seq(minor, patch), _)) =>
semverBinCompatVersions(major, minor, patch)
.map { case (maj, min, pat) => s"$maj.$min.$pat" }
.collect { case (maj, min, pat) if !scalaVer.startsWith("3.") || (maj >= 2 && min >= 7) => s"$maj.$min.$pat" }
case _ =>
List.empty[String]
}
}
// Safety Net For Exclusions
lazy val excludedVersions: List[String] = List()
lazy val excludedVersions: List[String] = List("2.7.0") // cursed bincompat on Scala 3
Copy link
Member Author

Choose a reason for hiding this comment

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

2.7.0 is cursed right? Unless we only want to curse it for Scala 3.


// Safety Net for Inclusions
lazy val extraVersions: List[String] = List("1.0.1", "1.1.0", "1.2.0", "1.3.1", "1.4.0", "1.5.0", "1.6.1")
lazy val extraCats1Versions: List[String] = List("1.0.1", "1.1.0", "1.2.0", "1.3.1", "1.4.0", "1.5.0", "1.6.1")
Copy link
Member

Choose a reason for hiding this comment

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

Btw checking so old versions doesn't cover new definitions (added since).
IMO we should check against the latest previous version.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are, these are just extras :) in sbt try show coreJVM/mimaPreviousArtifacts to see the list.

Copy link
Member

Choose a reason for hiding this comment

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

Oh ok I see 👍 - why do we add the extras? To be extra sure?

Copy link
Member Author

Choose a reason for hiding this comment

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

/shrug :)


(mimaVersions ++ (if (priorTo2_13(scalaVer) && includeCats1) extraVersions else Nil))
// Safety Net, starting with Scala 3 introduction in 2.6.1
lazy val extraVersions: List[String] = List("2.6.1")

(mimaVersions ++ extraVersions ++ (if (priorTo2_13(scalaVer) && includeCats1) extraCats1Versions else Nil))
.filterNot(excludedVersions.contains(_))
.map(v => "org.typelevel" %% moduleName % v)
}
Expand Down Expand Up @@ -521,7 +524,13 @@ def mimaSettings(moduleName: String, includeCats1: Boolean = true) =
exclude[DirectAbstractMethodProblem]("cats.free.ContravariantCoyoneda.k"),
exclude[ReversedAbstractMethodProblem]("cats.free.ContravariantCoyoneda.k"),
exclude[DirectAbstractMethodProblem]("cats.free.Coyoneda.k"),
exclude[ReversedAbstractMethodProblem]("cats.free.Coyoneda.k")
exclude[ReversedAbstractMethodProblem]("cats.free.Coyoneda.k"),
exclude[DirectMissingMethodProblem]("cats.free.ContravariantCoyoneda.unsafeApply"),
exclude[DirectMissingMethodProblem]("cats.free.ContravariantCoyoneda.ks"),
exclude[DirectMissingMethodProblem]("cats.free.ContravariantCoyoneda.unsafeApply"),
exclude[DirectMissingMethodProblem]("cats.free.Coyoneda.unsafeApply"),
exclude[DirectMissingMethodProblem]("cats.free.Coyoneda.ks"),
exclude[DirectMissingMethodProblem]("cats.free.Coyoneda.unsafeApply")
)
}
)
Expand Down Expand Up @@ -876,9 +885,16 @@ lazy val binCompatTest = project
// see https://github.com/typelevel/cats/pull/3079#discussion_r327181584
// see https://github.com/typelevel/cats/pull/3026#discussion_r321984342
useCoursier := false,
addCompilerPlugin(("org.typelevel" %% "kind-projector" % kindProjectorVersion).cross(CrossVersion.full)),
libraryDependencies ++= {
if (isDotty.value) Nil
else
Seq(
compilerPlugin(("org.typelevel" %% "kind-projector" % kindProjectorVersion).cross(CrossVersion.full))
)
},
libraryDependencies += mimaPrevious("cats-core", scalaVersion.value, version.value).last % Provided,
scalacOptions ++= (if (priorTo2_13(scalaVersion.value)) Seq("-Ypartial-unification") else Nil)
scalacOptions ++= (if (priorTo2_13(scalaVersion.value)) Seq("-Ypartial-unification") else Nil),
scalacOptions ++= (if (isDotty.value) Seq("-Ykind-projector") else Nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the right order? isn't it if (!isDotty.value)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure TBH, but this works for 2.x and 3 locally.

Copy link
Member Author

Choose a reason for hiding this comment

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

See also https://docs.scala-lang.org/scala3/guides/migration/plugin-kind-projector.html

)
.settings(testingDependencies)
.dependsOn(core.jvm % Test)
Expand Down
5 changes: 3 additions & 2 deletions core/src/main/scala/cats/Invariant.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,10 @@ import scala.util.control.TailCalls.TailRec

@suppressUnusedImportWarningForScalaVersionSpecific
object Invariant extends ScalaVersionSpecificInvariantInstances with InvariantInstances0 {
implicit def catsInstancesForId
private[cats] def catsInstancesForId: Distributive[Id] with Comonad[Id] = catsInstancesForIdBinCompat1
implicit def catsInstancesForIdBinCompat1
: Distributive[Id] with Bimonad[Id] with CommutativeMonad[Id] with NonEmptyTraverse[Id] =
cats.catsInstancesForId
cats.catsInstancesForIdBinCompat1
implicit def catsMonadErrorForEither[A]: MonadError[Either[A, *], A] =
cats.instances.either.catsStdInstancesForEither[A]
implicit def catsInstancesForOption
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/scala/cats/Semigroupal.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ import simulacrum.typeclass
}

object Semigroupal extends ScalaVersionSpecificSemigroupalInstances with SemigroupalArityFunctions {
implicit def catsSemigroupalForId: Semigroupal[Id] = catsInstancesForId
implicit def catsSemigroupalForId: Semigroupal[Id] = catsInstancesForIdBinCompat1
implicit def catsSemigroupalForOption: Semigroupal[Option] = cats.instances.option.catsStdInstancesForOption
implicit def catsSemigroupalForTry: Semigroupal[Try] = cats.instances.try_.catsStdInstancesForTry
implicit def catsSemigroupalForFuture(implicit ec: ExecutionContext): Semigroupal[Future] =
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/scala/cats/UnorderedFoldable.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ object UnorderedFoldable
}
}

implicit def catsNonEmptyTraverseForId: NonEmptyTraverse[Id] = catsInstancesForId
implicit def catsNonEmptyTraverseForId: NonEmptyTraverse[Id] = catsInstancesForIdBinCompat1
implicit def catsTraverseForOption: Traverse[Option] = cats.instances.option.catsStdInstancesForOption
implicit def catsTraverseForList: Traverse[List] = cats.instances.list.catsStdInstancesForList
implicit def catsTraverseForSeq: Traverse[Seq] = cats.instances.seq.catsStdInstancesForSeq
Expand Down
7 changes: 6 additions & 1 deletion core/src/main/scala/cats/package.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,12 @@ package object cats {

type Endo[A] = A => A

val catsInstancesForId: Bimonad[Id] with CommutativeMonad[Id] with NonEmptyTraverse[Id] with Distributive[Id] =
@deprecated("retained for binary compatibility", "2.7.1")
private[cats] def catsInstancesForId
: Bimonad[Id] with CommutativeMonad[Id] with Comonad[Id] with NonEmptyTraverse[Id] with Distributive[Id] =
catsInstancesForIdBinCompat1
val catsInstancesForIdBinCompat1
: Bimonad[Id] with CommutativeMonad[Id] with NonEmptyTraverse[Id] with Distributive[Id] =
Comment on lines +73 to +78
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this change? 🤔 - I thought only the forwarder in Invariant needs a change

new Bimonad[Id] with CommutativeMonad[Id] with NonEmptyTraverse[Id] with Distributive[Id] {
def pure[A](a: A): A = a
def extract[A](a: A): A = a
Expand Down