Dotty: Scala Wart: Abstract/non-final case classes

[lihaoyi]

Opening this issue, as suggested by Martin, to provide a place to discuss the individual warts brought up in the blog post Warts of the Scala Programming Language and the possibility of mitigating/fixing them in Dotty (and perhaps later in Scala 2.x). These are based on Scala 2.x behavior, which I understand Dotty follows closely, apologies in advance if it has already been fixed

---

You can inherit from case classes and extend them with new functionality:

@ case class Foo(i: Int)
defined class Foo

@ Foo(1)
res18: Foo = Foo(1)

@ Foo(1).i
res19: Int = 1

@ class Bar extends Foo(1)
defined class Bar

@ (new Bar).i
res21: Int = 1

You can even declare it an abstract case class to force someone to inherit
from it rather than instantiating it directly. If you want your
case class to not allow inheritance you should label it final

As far as I can tell, "nobody" does any of this: people don't inherit from case
classes, declare their case classes abstract, or rememebr to mark them
final. Literally the only programmer I've ever seen making good use of
abstract case classes and inheritance is probably Martin Odersky himself. I
think we should disallow it, and just force people to use normal classes if
they want to inherit from them.

[ShaneDelmore]

I have repeatedly made use of sealed abstract case class to hide constructors and allow me to supply smart constructors but if I could find another nice way to do this I would be happy to do without needing sealed abstract or final on my case classes.

[scf37]

Making case classes final will prevent making dynamic proxy classes out of them. E.g. Hibernate-like active objects

[adriaanm]

Since scala 2.12.2, you can hide case class constructors by making their constructor private and explicitly defining a private apply method in the companion (https://github.com/scala/scala/pull/5816)

[ShaneDelmore]

@adriaanm I am still torn on this. On a personal project it seems fine, but I worry about defining a private apply, then treating this class as safe, if I have one, it must have gone through my smart constructor, then finding someone added a field with a default value, and then the auto-generated apply will come back.

It's not an unworkable solution, but both making a sealed abstract case class, or making a private apply matching the default apply feel like obscure ways to do something I wish were more obvious: Make illegal states unrepresentable. Having a way to do this that didn't confuse readers (as sealed abstract does), throw exceptions (as assert/require do) or rely on an obscure implementation detail (hope nobody makes that apply public or adds a field).

I have considered making an @smart annotation with scala meta to keep the private def apply in sync with the generated version to prevent this, but native support for safe by construction is always nice if attainable. Also, you need to remember to add private def copy = ??? to the case class to prevent modification on copy. It ends up being a pretty fragile solution.

I'll have to think on this a bit and see if I can come up with any better ideas but off of the top of my head the most common use case I have are wanting value classes that can only be created in a valid state through a constructor that understands how to fail without throwing an exception, for example:

Pardon the pseudoCode, I understand this won't work -

Trait SmartConstructor {
  Private def copy = ???
  Private def apply = ??? //This would work if apply always surpressed the generated apply, currently only works if the signatures match
}

case class Data private (foo: String, bar: Int) extends SmartConstructor

Object Data {
  Def constructIfValid(foo: String, bar: Int): Option[Data] =
    If (bar > 100) 
      Some(new Data(foo, bar))
    Else
      None
}

[adriaanm]

someone added a field with a default value, and then the auto-generated apply will come back.

Yes, adding a primary constructor argument would circumvent this. You could have a lint check that ensures no apply methods in the companion have weaker access rights, but it's not something I'd want to put in the language/compiler, because I can see either way being useful.

the most common use case I have are wanting value classes that can only be created in a valid state through a constructor that understands how to fail without throwing an exception

I'd just add a require to the class body to check that invariant, if it's crucial.

[ShaneDelmore]

But a require throws exceptions, hence the desire for smart constructors in the first place.

[adriaanm]

Sure, but what's the type of new Data? Option[Data]?

I expect your use case would be supported by providing more fine grained control over compiler-generated boilerplate methods, so that you can roll your own case classes or variants on that pattern.

[ShaneDelmore]

Exactly. Option[Data] or Either[Errata, Data]. It allows you to safely express "Make invalid states unrepresentable". There is no new Data, there is only
Val maybeData: Either[Errata, Data] = Data.parse("Can this string really be turned into an instance of Data?")

[fommil]

might get done in the GSoC https://github.com/fommil/scala-data-classes/issues/4

[odersky]

Since scala 2.12.2, you can hide case class constructors by making their constructor private and explicitly defining a private apply method in the companion (scala/scala#5816)

As far as I can see, this is not yet covered in the spec? The spec is explicit that an apply method is generated except that "The definition of apply is omitted if class cc is abstract." So I think we need to change that, but how?

[NthPortal]

What if you want to have an object extend a case class?

For example

case class Foo private[example](elems: Seq[Int])

// Because it's a case class, it comes with an `equals` method 
// to tell if two factories are equivalent
case class FooFactory(maxFooSize: Int) {
  def apply(elems: Int*): Foo = { require(elems.size <= maxFooSize); new Foo(elems)}
}

object Foo extends FooFactory(10)

Or is this sort of behavior considered bad practice?

[sjrd]

val Foo = FooFactory(10)

[NthPortal]

Then Foo can't be a case class

[fommil]

@NthPortal extending a case class is considered bad practice at my work and my libre software projects. Indeed, wartremover has a wart to prevent this. Even Josh Bloch's book on Effective Java touches on some of the points in the first chapter. In stalagmite, we're making @data (alternative to case) produce only final classes.

[sjrd]

@NthPortal Ah, I think I had not understood the core of your example.

In that case I would argue that your FooFactory should definitely not be a case class, because you're only using it, as your comment says, to get the auto-equals. But you're not going to unapply it, which is the whole point of a case class. Just make it a (potentially abstract) class and write the two-line equals explicitly.

[NthPortal]

@sjrd I think you are correct in this case - it does not need to be a case class. I feel like there could be situations where it would add value, but I'm struggling to come up with one.

[fommil]

btw, since wartremover already does this I'm not sure where the boundary between scalac / wartremover should live. It seems like having this sort of thing with an independent release cycle from the compiler is a good idea. If the compiler is going to do anything, then deprecate it and don't allow it anymore.

[mpilquist]

@ShaneDelmore @adriaanm I use the hacky sealed abstract case class pattern with smart constructors quite a bit. Using require is a non-starter as it leads to runtime errors in large systems.

The change in scala/scala#5816 is great but requires manually defining copy too, or otherwise the synthetic copy will bypass the smart constructor. More control over generated methods would be great.

final case class UInt31 private (value: Int) {
  def copy(value: Int): Option[UInt31] = UInt31.fromInt(value)
}
object UInt31 {
  private def apply(value: Int): UInt31 = new UInt31(value)
  def fromInt(value: Int): Option[UInt31] =
    if (value >= 0) Some(apply(value)) else None
}

On the other hand, I'm pretty certain opaque types completely satisfy the use cases I have for the sealed abstract case class pattern.

[fommil]

oh there is a usecase for this in scalaz... we use it everywhere we need an invariant case class with no parameters. e.g. https://github.com/scalaz/scalaz/blob/series/7.3.x/core/src/main/scala/scalaz/Maybe.scala#L158-L162

There is an earlier encoding using an object instead of a case class with a custom def unapply, but using it breaks exhaustivity checks. If that could be fixed then I have no more usecases for abstract case class.

[smarter]

There is an earlier encoding using an object instead of a case class with a custom def unapply, but using it breaks exhaustivity checks. If that could be fixed then I have no more usecases for abstract case class.

Do you have a testcase?

[fommil]

@smarter https://github.com/scala/bug/issues/10660

[fommil]

also, just to close the loop on the @data macro from above. That project was abandoned because we used scalameta macros and they are now abandoned. Since macro-paradise is abandoned, it would mean rewriting it as a compiler plugin as per https://gitlab.com/fommil/scalaz-deriving/blob/master/deriving-plugin/src/main/scala/scalaz/plugins/deriving/AnnotationPlugin.scala but I don't have the time to do that.

[smarter]

@smarter scala/bug#10660

Fixed in Dotty if I reconstructed the example correctly:

sealed abstract class Maybe[A]
final case class Just[A](a: A) extends Maybe[A]
final case object Empty extends Maybe[Nothing] {
  def apply[A](): Maybe[A] = this.asInstanceOf[Maybe[A]]
  def unapply[A](e: Maybe[A]): Boolean = this == e
}

object Test {
  val a: Maybe[Int] = Just(2)
  a match {
    case Just(2) => true
    case Empty() => true
  }
}

-- [E029] Pattern Match Exhaustivity Warning: try/t10660.scala:10:2 ------------
10 |  a match {
   |  ^
   |  match may not be exhaustive.
   |  
   |  It would fail on: Just(_)

[fommil]

nice, but how does that work? How does it know it can count Empty.type? If you add a Just(_) match back in does it complain about missing Empty?

[smarter]

We can discuss this further in http://gitter.im/lampepfl/dotty if you want, but not here.

[fommil]

given the usecase in scalaz, I'm sure having a record of how this works for posterity would be good.

compiler development seems to be an endless amount of moving people to other services or rooms to have a conversation. If I ever write a compiler, I'll be sure to spend a good amount of time setting up at least 5 mailing lists, gitter rooms, github accounts, discourse sites and twitter accounts... just so I can do a basic job of sending people to the right place.

[smarter]

I'm just trying to not spam the people subscribed to this issue. For the record, my reply was:

In this case I think the exhaustiveness checker assumes that Empty cannot happen because Maybe[Nothing] is not a subtype of Maybe[Int]

So the solution you have in https://github.com/scalaz/scalaz/blob/series/7.3.x/core/src/main/scala/scalaz/Maybe.scala#L158-L162 is probably correct, it gives a correct type to Empty and then hides the magic in a private method

[fommil]

thanks @smarter. The tl;dr is that this is not "fixed in dotty" as advertised. Dotty is more rigorous meaning that one proposed encoding of parameterless invariant case classes is no longer valid, and the existing scalaz encoding (using abstract case class) is still necessary. Therefore I maintain my "no" to this ticket, unless parameterless case classes can still be abstract or there is some other mechanism to conveniently create the companion and unapply method.

[fommil]

generally I don't think the compiler should be responsible for warts and linting. The compiler should either allow or disallow certain features that are planned to be removed, with a flag to ignore the warning, allowing a grace time for users to migrate. Scalafix is proving itself to be an excellent tool in this regards, allowing the compiler to be simpler.

[ShaneDelmore]

@mpilquist Does the opaque types sip satisfy your needs for smart constructors because you only use them with anyval types with a single field? As far as I can see opaque types don’t help with my usage of sealed abstract case class for multi-field case classes.