The great de-case-classification

[armanbilge]

So that we have a better chance to evolve these compatibly in the future, as needed.

[jenshalm]

Just for reference: as you know I'm using this pattern a lot in Laika since 1.0. Recently I added a new property to such a construct for the first time and I was surprised that mima forced me to nevertheless add 3 exclusions for the change of signature in the private Impl type. I was not expecting this, tbh. I'm just mentioning this in case there are options of subtle changes in the pattern to prevent this. Or maybe this is just how mima works and we have to live with this extra maintenance burden?

[armanbilge]

add 3 exclusions for the change of signature in the private Impl type. I was not expecting this, tbh.

Do you know if they were specific to a particular Scala version?

there are options of subtle changes in the pattern to prevent this

Mm, possibly moving the Impl class to be top-level instead of nested would help. Nested classes tend to be encoded in weird ways which confuses MiMa I think.

[jenshalm]

Do you know if they were specific to a particular Scala version?

No, 3 errors for all 3 versions.

Mm, possibly moving the Impl class to be top-level instead of nested would help.

So just as package-private then?

Nested classes tend to be encoded in weird ways which confuses MiMa I think.

I checked the bytecode and they are public. Which is another thing I don't understand, I would have thought they could be encoded more defensively in such a case?

[armanbilge]

So just as package-private then?

I think I've had the best results when I just mark as unqualified private.

Which is another thing I don't understand, I would have thought they could be encoded more defensively in such a case?

🤷 that's a question for compiler engineers. Probably there is some trickyness / semantic mismatch between what non-public means in the JVM and for Scala.

[j-mie6]

Yes, the package-private is not something encodable within the JVM. The problem is that since they are accessible from outside the class, they cannot be marked as private so at a binary level they are still public.

From MiMA's perspective though, as long as the variable has always been package-private within this minor, it can be removed without causing a conflict (no user-land visibility means nobody could be hooked into it unless they hacked the package, which is their own fault, naughty user).

[armanbilge]

Ok, I think I hit all the major case classes that tend to need to evolve.

[satorg]

@armanbilge do I understand correctly, that we are deliberately getting rid of all unapply along with de-casefication?
I guess this is because it is hard to maintain unapply in a bin-compat way (because it usually differs in a return value type only) - is it correct?

[armanbilge]

I guess this is because it is hard to maintain unapply in a bin-compat way (because it usually differs in a return value type only) - is it correct?

Yes, that's correct. Actually it may be possible to do that evolution bin-compatibility with some tricks, but it's definitely not possible to do it source-compatibly.

We could bring back some/all of the unapplys and mark them as deprecated. Probably that would be the right thing to do, but quite a bit of work 😕

[satorg]

@armanbilge ,

Actually it may be possible to do that evolution bin-compatibility with some tricks, but it's definitely not possible to do it source-compatibly.

Actually, I'm thinking about an approach that would let us keep unapply-s as extractors (because they can come handy) and still maintaining both binary and source compatibility. It comes however at a price of a slightly modified syntax though, check this out:

sealed trait Duper {
  def foo: String
  def bar: Int
}
object Duper {
  private case class Impl(foo: String, bar: Int) extends Duper

  object fromFooBar {
    def apply(foo: String, bar: Int): Duper = Impl(foo, bar)
    def unapply(self: Duper) = Some((self.foo, self.bar))
  }
}

Therefore, both apply and unapply can be used in an explicit and straightforward way:

val duper = Duper.fromFooBar("123", 456)

duper match {
  case Duper.fromFooBar(foo, bar) => println(s"extracted foo=$foo, bar=$bar")
}

Now, if we want to add one more field to Duper, there's no struggle with that anymore:

sealed trait Duper {
  def foo: String
  def bar: Int
  def goo: Double
}
object Duper {
  private case class Impl(foo: String, bar: Int, goo: Double) extends Duper

  // The old object maintains the signatures of its methods unchanged.
  object fromFooBar {
    // inevitable need to provide some default value:
    def apply(foo: String, bar: Int): Duper = Impl(foo, bar, 0.0)
    def unapply(self: Duper) = Some((self.foo, self.bar))
  }
  // The new object gets its own apply/unapply
  object fromFooBarGoo {
    def apply(foo: String, bar: Int, goo: Double): Duper = Impl(foo, bar, goo)
    def unapply(self: Duper) = Some((self.foo, self.bar, self.goo))
  }
}

So it seems, this way we can keep evolving the model without breaking any kind of compatibility.

Wdyt?

[jenshalm]

@armanbilge, @satorg

In this particular case I think I would advise against preserving unapply, mainly for these reasons:

1. Many of the affected case classes are pretty large, with up to 20(!!) properties. So pattern matching against these structures is wildly inconvenient anyway. Likewise, maintaining unapply variants with 20, 21 and 22 properties for the same type in the future is not very desirable either - naming them would be a funny exercise on its own.

2. You are a build tool and not cats-core, you are in 0.x range and you are doing a binary-breaking release. As long as you do not trigger a lot of work for a lot of projects you should be fine to just break stuff. For some of the affected structures you can find zero usages within the Typelevel org, neither in pattern matches nor any other usage (e.g. JobContainer). If there is a particular type you suspect or know that existing pattern matches are very likely you could preserve unapply for those only.

I applied this "anti-case-class-pattern" in Laika for the 1.0 release which is out since October last year, and I have not yet received a single user complaint about missing unapply methods.

Btw. @satorg, since you recommended this pattern to us a while back: were you aware that mima does not actually accept signature changes to the private case class Impl construct?

[j-mie6]

Personally, I reckon we should get rid of the pattern matching, I don't see much of a concrete use for configuration pattern matching that isn't internal to the consumer. But willing to hear suggestions for use

[satorg]

@jenshalm ,

were you aware that mima does not actually accept signature changes to the private case class Impl construct?

The pattern above is not about keeping private case class Impl inside a companion object, actually. That is an orthogonal problem that can be solved in any Mima compatible way. In fact, it does not have to be a case class under the hood to make it working.

But I agree that for large models maintaining any kind of unapply is not particularly useful. For smaller models it can be, though.

[armanbilge]

@satorg your suggestion is similar to the pattern that Scalameta adopted for unapply matchers.

https://scalameta.org/docs/trees/guide.html#with-versioned-constructor-like-matchers

---

Many of the affected case classes are pretty large, with up to 20(!!) properties. So pattern matching against these structures is wildly inconvenient anyway.

Agree.

---

I don't see much of a concrete use for configuration pattern matching that isn't internal to the consumer

I do sometimes use some kind of matching to transform the auto-generated CI steps in external plugins/builds. But for the reasons Jens' points out using unapply for those becomes rather unwieldy.

https://github.com/gemini-hlsw/sbt-lucuma/blob/2f3b0db34a44aabc4c1810733639811e0dd4a3cf/core/src/main/scala/lucuma/sbtplugin/LucumaPlugin.scala#L121-L127

[satorg]

your suggestion is similar to the pattern that Scalameta adopted for unapply matchers.

I think I can take it as "it is working and not really that bad" :)

[armanbilge]

I think I can take it as "it is working and not really that bad" :)

Yes, it's workable. And not that bad for users. Probably kind of annoying for maintainers. Also we need a naming scheme. Scalameta uses versions which is honestly kind of arbitrary.

[satorg]

That said, I totally agree that extractors for 5+ fields are not that practical. Especially if most of the positions are just placeholders.
My initial intent was to avoid re-writing a lot of matchers during the transition, but, to be honest, these kind of patterns are not really cool and should be avoided anyway:

    githubWorkflowBuild := {
      githubWorkflowBuild.value.flatMap {
        case testStep @ WorkflowStep.Sbt(List("test"), _, _, _, _, _, _, _) =>

The suggested matcher out there via accessing fields directly is definitely a better bet.

[armanbilge]

Ok, thanks for the input everyone. I'm going to move forward with this as-is. We can consider restoring extractors and other things as the need comes up.

[satorg]

I wonder if it makes sense to add extends Product with Serializable to each abstract sealed class ModelName?
Or just Serializable, at least. I'm struggling to foreseen all possible consequences. Are there strong opinions "for" or "against", or just no big difference?

[armanbilge]

Definitely don't see a reason to add Serializable. Please nobody java serialize these classes LOL. Still, I concede that adding it would be harmless.

Extending Product would more-or-less enforce that it is implemented by effectively a case class. Also probably harmless but not sure of the immediate value either.

Anyway, we can add these later if they turn out to be important.