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

Value classes' public val field introduce unnecessary collisions #2514

Closed
kailuowang opened this issue Sep 19, 2018 · 5 comments
Closed

Value classes' public val field introduce unnecessary collisions #2514

kailuowang opened this issue Sep 19, 2018 · 5 comments

Comments

@kailuowang
Copy link
Contributor

kailuowang commented Sep 19, 2018

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 #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
Copy link
Contributor

ceedubs commented Sep 30, 2018

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
Copy link
Contributor Author

@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
Copy link
Contributor

ceedubs commented Sep 30, 2018

@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
Copy link
Contributor Author

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

@kailuowang kailuowang reopened this Nov 16, 2018
@kailuowang
Copy link
Contributor Author

closed by #2617

DieBauer added a commit to DieBauer/cats that referenced this issue Apr 13, 2020
This TODO is solved by fixing issue typelevel#2514. 

It is added as a separate chapter, but could also be elided.
kailuowang pushed a commit that referenced this issue Apr 19, 2020
This TODO is solved by fixing issue #2514. 

It is added as a separate chapter, but could also be elided.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants