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

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

Closed
smarter opened this issue Dec 1, 2021 · 30 comments · Fixed by #4160
Closed

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

smarter opened this issue Dec 1, 2021 · 30 comments · Fixed by #4160

Comments

@smarter
Copy link
Contributor

smarter commented Dec 1, 2021

As discovered by @xuwei-k, see https://gist.github.com/xuwei-k/afe78856f95124c6ca5323beb7b32b5c from argonaut-io/argonaut#476

Seems like mima isn't setup for scala 3:

> ++3.0.2 coreJVM/mimaReportBinaryIssues
[info] Setting Scala version to 3.0.2 on 32 projects.
[info] Excluded 14 projects, run ++ 3.0.2 -v for more details.
[info] Reapplying settings...
[info] set current project to cats (in build file:/home/smarter/opt/cats/)
[error] stack trace is suppressed; run last coreJVM / mimaPreviousClassfiles for the full output
[error] (coreJVM / mimaPreviousClassfiles) sbt.librarymanagement.ResolveException: Error downloading org.typelevel:cats-core_3:2.1.0

(it needs to use cats-core_3:2.6.1 since that's the only previously published version for Scala 3).

Note that this is absolutely not a bug in Scala 3: we intentionally use a different type erasure algorithm for intersection/compound types, because the Scala 2 algorithm was not commutative but also because it was dependent on compiler implementation details so we couldn't fully reproduce it even if we tried, cf https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/core/unpickleScala2/Scala2Erasure.scala, scala/scala3#5139.

@smarter
Copy link
Contributor Author

smarter commented Dec 1, 2021

Broken in #3906 /cc @joroKr21

@smarter
Copy link
Contributor Author

smarter commented Dec 1, 2021

It should be possible to introduce an implicit with a different type without breaking BC by doing something like:

   @deprecated("retained for binary compatibility")
   private[cats] def catsInstancesForId: Distributive[Id] with Comonad[Id] = cats.catsInstancesForId
   implicit def catsInstancesForId2
     : Distributive[Id] with Bimonad[Id] with CommutativeMonad[Id] with NonEmptyTraverse[Id] =
    cats.catsInstancesForId

@armanbilge
Copy link
Member

armanbilge commented Dec 1, 2021

Might not want to make it private right away to avoid breaking source compatibility. Until 2.7.0 Id instances weren't even in implicit scope so they may have been directly referenced.

@djspiewak
Copy link
Member

We generally don't consider implicits to be part of source compatibility, so IMO this is something we can (and should) do in 2.7.1

@joroKr21
Copy link
Member

joroKr21 commented Dec 1, 2021

Distributive[Id] with Comonad[Id] erases to Comonad? 🤔

@joroKr21
Copy link
Member

joroKr21 commented Dec 1, 2021

I guess we were unlucky that Bimonad is lexicografically before Comonad. Do type aliases matter? 🤔 Nope
Now the tricky issue is that to restore bincompat with older versions we have to break bincompat with 2.7.0.
I guess it's fine because it affects only Scala 3 but still it's pretty bad.

@smarter
Copy link
Contributor Author

smarter commented Dec 1, 2021

Distributive[Id] with Comonad[Id] erases to Comonad?

Type erasure in Scala 3 is commutative: the erasure of A & B when A and B are unrelated traits is based on their lexicographic ordering

Do type aliases matter?

Not in Scala 3 where the erasure of an intersection is computed from the erasure of each operand of the intersection.
In Scala 2 it does matter in some contrived situations unfortunately as witnessed by https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/core/unpickleScala2/Scala2Erasure.scala, e.g. in https://github.com/lampepfl/dotty/blob/f6b6e5068cc630d8fa5c3f3e72b4335c9149a216/sbt-test/scala2-compat/erasure/scala2Lib/Api.scala#L145-L149 the presence of a type alias changes erasure (the test checks that the first letter of the method name corresponds to its erased type), even worse, two identical type aliases can end up aftecting type erasure differently: https://github.com/lampepfl/dotty/blob/f6b6e5068cc630d8fa5c3f3e72b4335c9149a216/sbt-test/scala2-compat/erasure/scala2Lib/Api.scala#L125-L132 (think how much fun I had making Scala 3 support calling these methods correctly!)

(As an aside, the Scala 3 intersection type erasure algorithm is still not as good as I wish it was: it turned out to not be associative due to an oversight, I have an alternative proposal in http://guillaume.martres.me/pathless-scala.pdf (section 5.1) but that'll have to wait for Scala 4 😬 )

@armanbilge
Copy link
Member

Seems Id is only the tip of the iceberg?

[error] Cats Free: Failed binary compatibility check against org.typelevel:cats-free_3:2.6.1! Found 6 potential problems (filtered 33)
[error]  * static method unsafeApply(java.lang.Object,scala.collection.immutable.List)cats.free.ContravariantCoyoneda in class cats.free.ContravariantCoyoneda does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.free.ContravariantCoyoneda.unsafeApply")
[error]  * abstract method ks()scala.collection.immutable.List in class cats.free.ContravariantCoyoneda does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.free.ContravariantCoyoneda.ks")
[error]  * method unsafeApply(java.lang.Object,scala.collection.immutable.List)cats.free.ContravariantCoyoneda in object cats.free.ContravariantCoyoneda does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.free.ContravariantCoyoneda.unsafeApply")
[error]  * static method unsafeApply(java.lang.Object,scala.collection.immutable.List)cats.free.Coyoneda in class cats.free.Coyoneda does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.free.Coyoneda.unsafeApply")
[error]  * abstract method ks()scala.collection.immutable.List in class cats.free.Coyoneda does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.free.Coyoneda.ks")
[error]  * method unsafeApply(java.lang.Object,scala.collection.immutable.List)cats.free.Coyoneda in object cats.free.Coyoneda does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.free.Coyoneda.unsafeApply")

@smarter
Copy link
Contributor Author

smarter commented Dec 1, 2021

Those seem to be private[cats] methods? Maybe mima knows to ignore them when reading Scala 2 pickling information but doesn't know how to interpret Scala 3 signatures in tasty /cc @dwijnand

@armanbilge
Copy link
Member

armanbilge commented Dec 1, 2021

Alternatively, does Scala 3 treat private[cats] differently from Scala 2? e.g. what if they were private[free].

@smarter
Copy link
Contributor Author

smarter commented Dec 1, 2021

Not as far as I know, it all gets erased to public methods.

@armanbilge
Copy link
Member

Yes it's very strange. That was my PR #3987 btw, and I had to add some filters but not for those. In any case should be safe to filter them on Scala 3.

@dwijnand
Copy link
Contributor

dwijnand commented Dec 2, 2021

Maybe mima knows to ignore them when reading Scala 2 pickling information but doesn't know how to interpret Scala 3 signatures in tasty /cc @dwijnand

That's correct. I parse Scala annotations from Scala 2's pickle and Scala 3's tasty, but only parse the package private information from Scala 2's pickle. Not for any true reason, it's work just waiting for a motivating case to arise.

But I must be blind this morning because I can't for the life of me see those unsafeApply and ks methods in the sources...

@smarter
Copy link
Contributor Author

smarter commented Dec 2, 2021

Thanks for the confirmation. These methods were removed in #3987

@armanbilge
Copy link
Member

Interesting, thanks for chiming in, is this a (relatively) new feature? I feel like I've had to add filters for package privates in the recent past, but maybe I'm confused :)

@dwijnand
Copy link
Contributor

dwijnand commented Dec 2, 2021

Yeah. I've added so many filters for tweaking private[sbt] methods and classes... I should have stopped and done it a long time ago. It shipped in https://github.com/lightbend/mima/releases/tag/0.9.0.

@joroKr21
Copy link
Member

joroKr21 commented Dec 2, 2021

Maybe a similar feature is needed for sealed traits 🤔

@dwijnand
Copy link
Contributor

dwijnand commented Dec 2, 2021

For new methods? That might be a good idea. Open a Mima issue if you want to record your thoughts.

@armanbilge
Copy link
Member

armanbilge commented Dec 2, 2021

Yeah. I've added so many filters for tweaking private[sbt] methods and classes... I should have stopped and done it a long time ago.

I think this is a really important change, far more than the nuisance factor. It always seemed dangerous to me that:

  1. an initially public method could be made package private
  2. someone innocently assuming that the method was always package private, could then change it and filter the MiMa warning

But, now that MiMa natively understands package private, those filters wouldn't be necessary in ordinary use, thus disabling the footgun. So I think it would be excellent if Scala 3 can get this feature as well.

@joroKr21
Copy link
Member

joroKr21 commented Dec 2, 2021

Oh that's a good point - if we're using this trick of making something package private that was public before, then we can't expect to be able to change package private methods willy nilly (change their signature, make them abstract, etc.) 🤔

@joroKr21
Copy link
Member

joroKr21 commented Dec 2, 2021

So I guess in this case checking both against old and new versions with MiMa makes sense.

@armanbilge
Copy link
Member

So I guess in this case checking both against old and new versions with MiMa makes sense.

WDYM by this?

@joroKr21
Copy link
Member

joroKr21 commented Dec 2, 2021

I mean checking against cats 1.0, 1.1, 1.2, etc. instead of just the latest version - i.e. transitivity doesn't apply without caveats

@armanbilge
Copy link
Member

I'm not sure we are talking about the same thing. But I could be confused.

In theory, I don't think checking against old versions is necessary at all. Since binary-compatible changes can only add to the public binary API, there should never be a situation where M.N has something that that M.(N+1) doesn't. So checking against M.(N+1) should be sufficient.

OTOH, I'm talking about adding MiMa filters. When you do that, you are effectively telling MiMa "I am taking full responsibility the bincompat of this code." So if in M.(N+100) you do add a filter for a method that you thought was always package private, but was actually public in M.N and everyone forgot, it doesn't matter if you check M.(N+100) against M.N because you just told MiMa that you know best.

Ideally, the only time we should have to add filters is when we are in fact breaking public APIs (in the Scala not Java sense). Which should be exceedingly rare.

@joroKr21
Copy link
Member

joroKr21 commented Dec 2, 2021

OTOH, I'm talking about adding MiMa filters. When you do that, you are effectively telling MiMa "I am taking full responsibility the bincompat of this code." So if in M.(N+100) you do add a filter for a method that you thought was always package private, but was actually public in M.N and everyone forgot, it doesn't matter if you check M.(N+100) against M.N because you just told MiMa that you know best.

Yes but if MiMa knows about sealed traits and package private methods it should let you do those changes without adding a filter. And then it matters whether you check with the older version or not.

@armanbilge
Copy link
Member

Ahhh. Yes, you are absolutely right :)

transitivity doesn't apply without caveats

Now I get this 😆

@satorg
Copy link
Contributor

satorg commented Dec 2, 2021

Sorry, I'm not completely catching this... The possibility to make some obsolete methods package private is one of vital features that allow us to add new features while maintaining the binary compatibility with older versions. So I'm wondering – will such a possibility still be preserved?

@joroKr21
Copy link
Member

joroKr21 commented Dec 2, 2021

So I'm wondering – will such a possibility still be preserved?

Yes but it shouldn't come at cost of being able to change library internals. So we just have to be careful.

@smarter
Copy link
Contributor Author

smarter commented May 24, 2022

Is there an ETA for a version of cats with this fix in?

@armanbilge
Copy link
Member

armanbilge commented May 24, 2022

Personally, I'm hoping sometime in the next few weeks, pressure is mounting to release on Scala 3 + Native.

There are snapshots available of the bincompat fix for anyone in a pinch.

https://github.com/typelevel/cats/milestone/41

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 a pull request may close this issue.

6 participants