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

Enable MiMa for Scala 3 #4063

Closed
wants to merge 4 commits into from
Closed

Conversation

armanbilge
Copy link
Member

@armanbilge armanbilge commented Dec 1, 2021

Will also fix #4062. First commit should fail to demonstrate MiMa is running.

@armanbilge armanbilge marked this pull request as draft December 1, 2021 20:42
@@ -348,9 +348,12 @@ def mimaPrevious(moduleName: String, scalaVer: String, ver: String, includeCats1
lazy val excludedVersions: List[String] = List()

// 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 :)

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.

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.

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

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.

@armanbilge
Copy link
Member Author

How do we feel about static methods for Java? @johnynek you pushed for conservatism in typelevel/cats-effect#2343 (comment).

[error] Cats core: Failed binary compatibility check against org.typelevel:cats-core_2.13:2.6.1! Found 2 potential problems
[error]  * static method catsInstancesForId()cats.Distributive in interface cats.Invariant does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.Invariant.catsInstancesForId")
[error]  * static method catsInstancesForId()cats.Bimonad in class cats.package does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.package.catsInstancesForId")

@joroKr21
Copy link
Member

joroKr21 commented Dec 1, 2021

That's the issue in #4062 we were discussing though - the issue is that the type signature changed.

@armanbilge
Copy link
Member Author

armanbilge commented Dec 1, 2021

Yes, except it's now fixed for Scala. However, by marking the old method private[cats] dissapears the static forwarders which AFAIK are only used by Java. That's what MiMa is complaining about, not the non-static methods that originally brought our attention to this issue. Also note this is 2.13 and not 3.

Comment on lines +73 to +78
@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] =
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

@joroKr21
Copy link
Member

joroKr21 commented Dec 1, 2021

Here is another suggestion - add one more forwarder of type Bimonad[Id] with CommutativeMonad[Id] to object Monad and restore the old type of the forwarder in object Invariant 😄 - or should it be to Applicative? Sorry, that's tricky.

Edit: I think it should be in Functor

@smarter
Copy link
Contributor

smarter commented Dec 1, 2021

I wonder if bincompat with both cats-2.6.1_3 and cats-2.7.0_3 could be kept using @targetName to generate overloads (requiring a scala-2/ and scala-3/ version of the original file):

import scala.annotation.targetName
// ...
// 2.6.1 compat
private[cats] def catsInstancesForId: Distributive[Id] with Comonad[Id] = ...
// 2.7.0 compat
private[cats] @targetName("catsInstancesForId") def catsInstancesForIdCompat270:
     : Distributive[Id] with Bimonad[Id] with CommutativeMonad[Id] with NonEmptyTraverse[Id] = ...
// new method
implicit def catsInstancesForIdBinCompat1
    : Distributive[Id] with Bimonad[Id] with CommutativeMonad[Id] with NonEmptyTraverse[Id] = ...

@satorg
Copy link
Contributor

satorg commented Dec 1, 2021

I wonder if bincompat with both cats-2.6.1_3 and cats-2.7.0_3 could be kept using @targetName to generate overloads (requiring a scala-2/ and scala-3/ version of the original file):

But this annotation only exists in Scala3's library, right?

@smarter
Copy link
Contributor

smarter commented Dec 2, 2021

Yes, that would be in a scala-3/ version of the file, the scala-2/ version only needs one version of catsInstancesForId to keep compat with 2.6.1 and 2.7.0.

@armanbilge
Copy link
Member Author

So, while main has moved on to 2.8.0-SNAPSHOT I'm wondering if we should make a patch release of 2.7.1 with this bincompat fixed. Because otherwise, the 2.7 series will be a (small, Scala 3-only) break in the binary-compatibility of the 2.x series as a whole.

@satorg
Copy link
Contributor

satorg commented Jan 27, 2022

@armanbilge just curious – is this PR still in progress?

@armanbilge
Copy link
Member Author

@satorg
Copy link
Contributor

satorg commented Jan 28, 2022

So, while main has moved on to 2.8.0-SNAPSHOT I'm wondering if we should make a patch release of 2.7.1 with this bincompat fixed. ...

I would say no need to patch 2.7.x unless there're complains from people who use it in some prod-ish projects and encountered issues with. But I guess if there were such issues we would already know about it.

@satorg
Copy link
Contributor

satorg commented Jan 28, 2022

I wonder if bincompat with both cats-2.6.1_3 and cats-2.7.0_3 could be kept using @TargetNAME to generate overloads.

From my prospective, if it is possible to maintain bin.compatibility without spitting sources into Scala2 and Scala3 versions then it would be the best bet. But I realize that in some cases it may not be feasible – then @targetName can come in handy of course.

@armanbilge
Copy link
Member Author

Superseded by #4160.

@armanbilge armanbilge closed this Apr 4, 2022
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.

cats 2.7.0 broke binary compatibility for Scala 3 (because mima wasn't run)
5 participants