search

typelevel / cats

Lightweight, modular, and extensible library for functional programming.
https://typelevel.org/cats/
Other
5.25k stars 1.21k forks source link

Value classes' public `val` field introduce unnecessary collisions #2514

Closed kailuowang closed 5 years ago

kailuowang commented 6 years ago

In Scala 2.10 the underlying value field in a value class has to be public, For example 

final class EitherIdOpsBinCompat0[A](val value: A) extends AnyVal

In this particular case, it introduced a value val to all types that simply returns itself. There is no real use case for this it's only due to the limitation of value classes in 2.10. It collides with any implicit extension that adds a value too. See https://github.com/typelevel/cats/issues/2491 for detail in this case. This limitation is removed since Scala 2.11. And Cats dropped support for 2.10 since 1.3.

I thought of two approaches that can fix this.

Option 1: make the existing implicit conversion and value class EitherIdOpsBinCompat0 package private and deprecated; copy the value class EitherIdOpsBinCompat0 and paste it as a new value class with its value private and new implicit conversion for it. We also need a new EitherSyntaxBinCompat1 trait for maintaining BC on Scala 2.11. I believe that this would be binary compatible and hide the value field going forward, but I haven't tested it yet. The downside is, of course, the duplicated code and more boilerplate. Also if we want to use the syntax inside the package we will run into collision.

Option 2: make the value field private. It seems to me that unless user calls the value field which has no practical usage, user won't run into binary compatibility issue. This can be verified by adding a "232". leftNec[Int] line to the BC test added by #2509 ~(not merged yet)~ . We would also need to add a MiMa exception for this field.

What do you guys think?

ceedubs commented 6 years ago

If we make this package-private instead of private, would that make it so that it's only source incompatible and not binary incompatible?

kailuowang commented 6 years ago

@ceedubs unfortunately package private still conflict with other public defs. If you have another implicit extension with the same def name but public, scalac won't happily choose that one when the def is called, instead it will complain that you don't have access to the package private one. In my view this might be a scalac bug.

ceedubs commented 6 years ago

@kailuowang I see. Bummer. Option 2 sounds more appealing to me, but I have a bit of a lingering fear about it being less compatible than we think it is. @rossabaker do you know of any situation other than calling value explicitly where this might crop up in terms of binary compatibility?

kailuowang commented 5 years ago

this is auto closed by #2614 but the scope of this issue is larger

kailuowang commented 5 years ago

closed by #2617