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

Improve the "Backwards compatible case classes" guide with Scala 2 and 3 specifics #2788

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

@julienrf julienrf self-assigned this May 4, 2023
@sjrd
Copy link
Member

sjrd commented May 4, 2023

Not to remove the need for your improvement, but:

  • (Scala 2) you have to add the compiler option -Xsource:3
  • (Scala 2) define the copy method with all the current fields manually and set it as private
  • (Scala 2) define a private unapply method in the companion object (note that by doing that the case class loses the ability to be used as an extractor in match expressions)
  • (Scala 3) define a custom fromProduct method in the companion object

makes it painfully obvious to me that this is not a viable strategy. At least not if you have Scala 2 in the mix. If you get any of those things wrong (by omitting it or even by writing the wrong signature), you end up with generated stuff anyway. And then you cannot get rid of it later.

TBH, I think we should not recommend this for Scala 2 at all. Developers will inevitably get it wrong. I would get it wrong.

@sideeffffect
Copy link
Contributor Author

painfully obvious to me that this is not a viable strategy

Maybe that has been my plan all along 😈

But seriously now. I think there is a value for providing guidelines even for Scala 2. There are many eager volunteers maintaining popular libraries who would love to prevent their users from suffering from binary incompatibilities. These libraries are cross-compiled for both Scala 2 and 3. And having a guideline for both could help them.
What would be the point not having this in the guide? It's complicated, yes, but it's not targeted at the ordinary Scala developer audience.

Anyway, I'm glad we now have a seed of an initiative to fix this pesky problem once and for all: https://contributors.scala-lang.org/t/can-we-make-adding-a-parameter-with-a-default-value-binary-compatible/6132

* in Scala 2, you have to add the compiler option `-Xsource:3`
* (Scala 2) you have to add the compiler option `-Xsource:3`
* (Scala 2) define the `copy` method with all the current fields manually and set it as `private`
* (Scala 2) define a private `unapply` method in the companion object (note that by doing that the case class loses the ability to be used as an extractor in match expressions)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why make this Scala 2 only? AFAIK it is also required in Scala 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not according to @lihaoyi

unapply: I think as of Scala 3 this will work right out of the box: unapply no longer returns Option[TupleN[...]] as it did in Scala 2, and instead just returns Person, with pattern matching just relying on the ._1 ._2 etc. fields to work. Thus, a p match{ case Person(first, last) => ???} callsite compiled against case class Person(first: String, last: String) should continue to work even when Person has evolved into case class Person(first: String, last: String, country: String = "unknown", number: Option[String] = None)

https://contributors.scala-lang.org/t/can-we-make-adding-a-parameter-with-a-default-value-binary-compatible/6132?u=sideeffffect

Copy link
Contributor

@julienrf julienrf Sep 16, 2023

Choose a reason for hiding this comment

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

That would be a source incompatibility, but if someone links an already compiled program with the new version of the case class, then they may get a run-time crash. I think it is preferable to recommend making unapply private in both Scala 2 and 3.

* (Scala 2) you have to add the compiler option `-Xsource:3`
* (Scala 2) define the `copy` method with all the current fields manually and set it as `private`
* (Scala 2) define a private `unapply` method in the companion object (note that by doing that the case class loses the ability to be used as an extractor in match expressions)
* (Scala 3) define a custom `fromProduct` method in the companion object
Copy link
Contributor

Choose a reason for hiding this comment

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

That one is interesting. We need to investigate further:

  • what is the current state of mirrors (without defining fromProduct)
  • demonstrate in a project example that defining fromProduct is enough to get type class derivation working on case classes with a private primary constructor, and that it works as expected when fields are added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can thank @armanbilge for discovering this

http4s/http4s#7066 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

That discussion needs to be resolved before we can advertise implementing fromProduct.

@julienrf julienrf assigned sideeffffect and unassigned julienrf May 5, 2023
@armanbilge
Copy link

Continuing from #2788 (comment):

  • what is the current state of mirrors (without defining fromProduct)
  • demonstrate in a project example that defining fromProduct is enough to get type class derivation working on case classes with a private primary constructor, and that it works as expected when fields are added.

I am currently still skeptical that case classes can be evolved backwards-compatibly when you take into account derivation.

e.g. consider evolving case class Foo(bar: String) to case class Foo(bar: String, baz: Int). For source- and binary-compatibility we can add a constructor with a default value def apply(bar: String) = Foo(bar, 0) and follow all the other changes described in the guide.

Now, suppose someone downstream is using the mirror to derive a JSON decoder for Foo. Previously, that decoder would accept something like { "bar": "..." } but when they compile against the new API it will start expecting something like { "bar": "...", "baz": 42 } and raise a decoding error otherwise. So this would not be a compatible change and the program would break for previously valid input, even though the authors of Foo were careful to evolve it compatibly.

Specifically, mirror does not have a way to indicate which members were "added later" and what their default values are. It may be possible to emulate this in ad-hoc ways using Option or default parameters but I do not feel so sure about that.

@sideeffffect
Copy link
Contributor Author

Now, suppose someone downstream is using the mirror to derive a JSON decoder for Foo. Previously, that decoder would accept something like { "bar": "..." } but when they compile against the new API it will start expecting something like { "bar": "...", "baz": 42 } and raise a decoding error otherwise.

The question is, whether defining custom fromProduct could help with that. I think it could.

But in any case, this is not a problem with binary compatibility, just to be clear. All the classes and methods from the old version are still present in the new.

@lihaoyi
Copy link
Contributor

lihaoyi commented May 5, 2023

Now, suppose someone downstream is using the mirror to derive a JSON decoder for Foo. Previously, that decoder would accept something like { "bar": "..." } but when they compile against the new API it will start expecting something like { "bar": "...", "baz": 42 } and raise a decoding error otherwise. So this would not be a compatible change and the program would break for previously valid input, even though the authors of Foo were careful to evolve it compatibly.

Not all decoders will be able to handle backwards compatibility in all possible cases. But many decoders can handle backwards compatibility in common cases.

"Add a new param on the left with a default value" is perhaps the most common one, and many JSON decoders already handle that in a backwards compatible manner by instantiating the default during de-serialization if the new field is not received in the input. uPickle goes the extra step to avoid emitting the key-value pair during serialization if the current value is equal to the default, to provide backwards compatibility during serialization as well, but that's a design choice and other libraries do it differently.

Being backwards compatible is certainly doable, within limits, and that's something that is up to the thing being derived to deal with

@julienrf
Copy link
Contributor

julienrf commented May 8, 2023

Here is a scenario that breaks mirrors and unapply (in Scala 3):

  • in v1.1.0, define case class Foo private (x: Int, s: Option[String] = None)
  • in v1.2.0, replace s with y: Option[Int]: case class Foo private (x: Int, y: Option[Int] = None). It is possible to make that change in a backward binary-compatible way by still defining def s: Option[String] = None in the class.
  • Now, consider a program that depends on v1.1.0. By summoning a Mirror[Foo] it becomes possible to convert instances to a tuple of type (Int, Option[String]) and vice versa. In that program, if we pattern match on a value of type Foo with a “constructor pattern”, ie case Foo(x, s) => …, s has type Option[String].
  • Last, suppose that our program is now linked with the classfiles of v1.2.0. The mirror instance lives in the new classfiles, which means that the value it will accept or return will have different type than the type the program expects. Furthermore, the code that uses pattern matching will break at run-time because the type of the second field will be Option[Int] instead of the expected Option[String].

The conclusion is that Mirrors provide an equivalent to both apply and unapply, which both lead to binary incompatibilities.

@sideeffffect
Copy link
Contributor Author

sideeffffect commented May 8, 2023

@julienrf this is a great example where case classes can be evolved in a generally incompatible way that even MiMa wouldn't detect. (Maybe https://github.com/scalacenter/tasty-mima would be of help here?)

But I think it's intuitive even to Scala beginners that removing fields from case class constructor is problematic.

Anyway, in this PR we're trying to come up with a guide how to evolve case classes without breaking, not how to break them 😂 (there are thousand other ways that are even simpler, but I see how this example is interesting because of MiMa).

I think @lihaoyi has a great point here:

Being backwards compatible is certainly doable, within limits, and that's something that is up to the thing being derived to deal with

There's only so much you as a case class author can do. Then you have to draw a line beyond which it's the case class user's (the one doing the deriving and/or the Type Class author) responsibility.

@armanbilge
Copy link

Then you have to draw a line beyond which it's the case class user's (the one doing the deriving and/or the Type Class author) responsibility.

Yes. I believe it's the case class author's responsibility to evolve it compatibly, including providing default values for fields added after-the-fact that can be used by authors of typeclass derivation mechanisms. Unfortunately the Mirror API offers no way to indicate default values, so currently this can only be done in ad-hoc ways.

@sideeffffect
Copy link
Contributor Author

@lihaoyi you've mentioned that

many JSON decoders already handle that in a backwards compatible manner by instantiating the default during de-serialization if the new field is not received in the input

Could you tell us more about this? Are you doing this with uPickle on Scala 3? Are you using Mirror for that? Have you came across the issue @armanbilge has described? If yes, how did you work around it?

@bishabosha
Copy link
Member

Hi all, what is the status of this PR?

@julienrf julienrf marked this pull request as draft September 16, 2023 14:17
@julienrf
Copy link
Contributor

Thank you for pinging us @bishabosha. There are still at least two discussions that need to be resolved before we can move forward with this PR. @sideeffffect, would you be interested in investigating the points raised in those discussions?

@SethTisue SethTisue changed the title Improve the "Backwards comptaible case classes" guide with Scala 2 and 3 specifics Improve the "Backwards compatible case classes" guide with Scala 2 and 3 specifics Feb 13, 2024
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.

6 participants