Add unapply methods that return an Option for compatibility?

[smarter]

Here's an interesting problem minmized from https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/repl/ammonite/Protocol.scala that causes dotty-compiler-bootstrapped/repl to go into an infinite loop:

class Foo
case class Bar(x: Int) extends Foo

object Bar {
  def unapply(foo: Foo): Option[Int] = foo match {
    case foo: Bar =>
      unapply(foo)
  }
}

With scalac, the unapply call goes to the generated def unapply(x$1: Bar): Option[Int] , but with dotty no such method exist, instead we only generate a def unapply(x$1: Bar): Bar, so the previously perfectly correct code becomes an infinite loop!

I think we can fix this by always generating an unapply method that returns an Option, even if we don't use it.

[odersky]

Great that you got to the bottom of this! I think you meant:

case foo: Bar =>
  unapply(foo)

It turns out that neither this method nor the original method in Ammonite is needed. They just duplicate the typecheck that the compiler does anyway. One can and should delete them.

I am not very keen about generating a new method. It's ugly and will get us into overloading troubles. On the other hand, it certainly is a serious incompatibility with Scala 2.

[smarter]

I think you meant: ...

Yes, edited.

It turns out that neither this method nor the original method in Ammonite is needed. They just duplicate the typecheck that the compiler does anyway. One can and should delete them.

I tried to delete the unapply(ti: TermAction) in https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/repl/ammonite/Protocol.scala and got an ambigous overload error because of the other unapply (unapply(ti: TermInfo)), removing that one breaks everything. Did you find a way to get it to compile?

[odersky]

I see. Well, in that case just inline TermState.unapply(ts) to Some(....)? I still think it's a corner case which should not force us to use an ugly compilation scheme.

[OlivierBlanvillain]

Introducing an unapplyWorkaround method with the same signature that the scalac generated one does the trick.

[smarter]

@olafurpg Could you try to get some statistics on how often Scala projects explicitly use unapply ? (That is, unapply was written by the user and not generated by pattern-matching).

[OlivierBlanvillain]

I would expect the unapply usage to be very high. What we are interested in are direct calls to compiler generated unapply, which won't be so easy to differentiate from user defined ones...

In the common case the dotty/scalac difference would result in a compilation error (the return type of unapply changed from Option[TupleN] to A), this is a very sneaky example where it doesn't break compilation (because of overloading)...

@smarter I really don't see an alternative to your suggestion (maybe under -language:Scala2?), or snippets like the following won't cross compile:

case class A(i: Int, s: String)
val a: Option[(Int, String)] = A.unapply(A(1, "s"))

[odersky]

I'd say, let's wait and see how much breaks in practice.

[smarter]

I'd say, let's wait and see how much breaks in practice.

Sure.

I am not very keen about generating a new method. It's ugly and will get us into overloading troubles.

For future reference: I think we could get around overloading issue by marking the generated method as Bridge and preferring non-bridge to bridge in overloading resolution, I can't think of a case where this would change the current semantics.

[DarkDimius]

@smarter, using Bridge flag for this use case is likely to break real bridge generation. It wont generate bridges to this bridge.

[jtjeferreira]

This would help play-json cross compilation in code like this

case class Car(id: Long, models: Map[String, String])
implicit val CarFormat = (
  (__ \ Symbol("id")).format[Long] and
    (__ \ Symbol("models")).format[Map[String, String]]
)(Car.apply, unlift(Car.unapply))

Actually the Option could be dropped but unapply should return (Long, Map[Stringm String]) instead of Car...

[dwijnand]

Let's try: Tuple.fromProductTyped(car)

[dwijnand]

Indeed:

scala> case class Car(id: Long, models: Map[String, String])
// defined case class Car

scala> def unapply(car: Car) = Tuple.fromProductTyped(car)
def unapply(car: Car): (Long, Map[String, String])

[bishabosha]

scala> def unapply(car: Car) = Tuple.fromProductTyped(car)
def unapply(car: Car): (Long, Map[String, String])

I think this secretly returns a hidden generic tuple type leading to #10958

[xuwei-k]

xuwei-k/case-class-to-tuple-cross-build@93b6fc9 🤔

[dwijnand]

Well, we missed the train on this for 3.0.0 and it hasn't seemed to be a major issue. Let's not do this.

[OlegYch]

uh, excuse me, what was the point of replacing previously useful unapply with identity?

[dwijnand]

It allowed us to generalise cheap, option-less unapply methods to all Product types, not just case classes.

[ornicar]

In case someone else comes here looking for the generated case class unapply function, here's how I replaced it:

  def unapply[P <: Product](p: P)(using m: scala.deriving.Mirror.ProductOf[P]): Option[m.MirroredElemTypes] =
    Some(Tuple.fromProductTyped(p))

Now you can just use unapply where you would previously use Foo.unapply