2.13.4+ requires explicit guidance around irrefutable unapply methods

[blast-hardcheese]

Context for this PR: scala/bug#12232 (comment)

This represents the majority of match may not be exhaustive warnings in my codebase.

Given this trivial program on 2.13.6:

import scala.meta._

object App extends App {
  val Name(name) = Term.Name("foo")
}

we get:

[info] compiling 11 Scala sources to .../target/scala-2.13/classes ...
[warn] .../src/main/scala/App.scala:16:29: match may not be exhaustive.
[warn]   val Term.Name(name) = Term.Name("foo")
[warn]                                  ^

The mechanical change is to identify places where unapply trivially returns a Some instead of an Option and constrain the result type.

[blast-hardcheese]

Turns out I'm incorrect, MiMa reports incompatibility. Happy to discuss here, or in the original issue on scala/scala

[tgodzik]

Thanks for raising this PR! This should be mostly safe, but I would like to double check it. Will take a look next week.

[tgodzik]

Thanks for the contribution again! I think we should be safe to include the changes, the only thing that is needed is to add:

ProblemFilters.exclude[IncompatibleResultTypeProblem]("scala.meta.Lit.unapply")
ProblemFilters.exclude[IncompatibleResultTypeProblem]("scala.meta.Lit.unapply")
ProblemFilters.exclude[IncompatibleResultTypeProblem]("scala.meta.Name.unapply")
ProblemFilters.exclude[IncompatibleResultTypeProblem]("scala.meta.Name.unapply")
ProblemFilters.exclude[IncompatibleResultTypeProblem]("scala.meta.tokens.Tokens.unapplySeq")
ProblemFilters.exclude[IncompatibleResultTypeProblem]("scala.meta.tokens.Tokens.unapplySeq")

to https://github.com/scalameta/scalameta/blob/main/project/Mima.scala#L30

[blast-hardcheese]

So, I added those lines, though I realized that I actually ran into this issue in my own project where the return type change actually caused ABI linking errors for me, but I didn't spend that much time to understand it due to scheduling concerns, I just bumped versions to overcome the incompatibility

[tgodzik]

ere the return type change actually caused ABI linking errors for me

I don't see the lines added in this PR, did you commit the changes?

What kind of errors did you encounter with linking?

[blast-hardcheese]

I don't see the lines added in this PR, did you commit the changes?

I reverted that commit to avoid it getting merged while I still had concerns about it. I'll do some more testing and get back to you with explicit errors, or if it happened to be a different issue I was running into, we can proceed

[blast-hardcheese]

OK. So, given the following program in App.scala:

import scala.meta._

object App extends App {
  val Name(x) = Term.Name("hello")

  println(x)
}

I ran

blast$ git checkout 613bfd02c7f97a420da7b972a51acace754dd5de
blast$ sbt 'clean ; scalametaJVM / assembly'
blast$ mv ./scalameta/scalameta/jvm/target/scala-2.13/scalameta-assembly-4.4.29+10-613bfd02-SNAPSHOT{,-master}.jar
blast$ git checkout "${this branch, with the requested changes to MiMa made}"
blast$ sbt 'scalametaJVM / assembly'
blast$ ls -l ./scalameta/scalameta/jvm/target/scala-2.13/scalameta-assembly-4.4.29+10-613bfd02-SNAPSHOT*.jar
-rw-r--r-- 1 blast staff 46180833 Nov 20 11:04 ./scalameta/scalameta/jvm/target/scala-2.13/scalameta-assembly-4.4.29+10-613bfd02-SNAPSHOT-master.jar
-rw-r--r-- 1 blast staff 46180832 Nov 20 11:08 ./scalameta/scalameta/jvm/target/scala-2.13/scalameta-assembly-4.4.29+10-613bfd02-SNAPSHOT.jar

---

That concludes setup. Now, for the test:

blast$ scalac -cp ./scalameta/scalameta/jvm/target/scala-2.13/scalameta-assembly-4.4.29+10-613bfd02-SNAPSHOT-master.jar App.scala
blast$ scala -cp ./scalameta/scalameta/jvm/target/scala-2.13/scalameta-assembly-4.4.29+10-613bfd02-SNAPSHOT-master.jar:. App
hello
blast$ scala -cp ./scalameta/scalameta/jvm/target/scala-2.13/scalameta-assembly-4.4.29+10-613bfd02-SNAPSHOT.jar:. App
Exception in thread "main" java.lang.NoSuchMethodError: scala.meta.Name$.unapply(Lscala/meta/Name;)Lscala/Option;
        at App$.delayedEndpoint$App$1(App.scala:4)
        at App$delayedInit$body.apply(App.scala:3)
        at scala.Function0.apply$mcV$sp(Function0.scala:39)
        at scala.Function0.apply$mcV$sp$(Function0.scala:39)
        at scala.runtime.AbstractFunction0.apply$mcV$sp(AbstractFunction0.scala:17)
        at scala.App.$anonfun$main$1(App.scala:76)
        at scala.App.$anonfun$main$1$adapted(App.scala:76)
        at scala.collection.IterableOnceOps.foreach(IterableOnce.scala:563)
        at scala.collection.IterableOnceOps.foreach$(IterableOnce.scala:561)
        at scala.collection.AbstractIterable.foreach(Iterable.scala:919)
        at scala.App.main(App.scala:76)
        at scala.App.main$(App.scala:74)
        at App$.main(App.scala:3)
        at App.main(App.scala)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at scala.reflect.internal.util.RichClassLoader$.$anonfun$run$extension$1(ScalaClassLoader.scala:101)
        at scala.reflect.internal.util.RichClassLoader$.run$extension(ScalaClassLoader.scala:36)
        at scala.tools.nsc.CommonRunner.run(ObjectRunner.scala:30)
        at scala.tools.nsc.CommonRunner.run$(ObjectRunner.scala:28)
        at scala.tools.nsc.ObjectRunner$.run(ObjectRunner.scala:45)
        at scala.tools.nsc.CommonRunner.runAndCatch(ObjectRunner.scala:37)
        at scala.tools.nsc.CommonRunner.runAndCatch$(ObjectRunner.scala:36)
        at scala.tools.nsc.MainGenericRunner.runTarget$1(MainGenericRunner.scala:70)
        at scala.tools.nsc.MainGenericRunner.run$1(MainGenericRunner.scala:91)
        at scala.tools.nsc.MainGenericRunner.process(MainGenericRunner.scala:103)
        at scala.tools.nsc.MainGenericRunner$.main(MainGenericRunner.scala:108)
        at scala.tools.nsc.MainGenericRunner.main(MainGenericRunner.scala)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at coursier.bootstrap.launcher.a.a(Unknown Source)
        at coursier.bootstrap.launcher.Launcher.main(Unknown Source)
blast$ scala -cp ./scalameta/scalameta/jvm/target/scala-2.13/scalameta-assembly-4.4.29+10-613bfd02-SNAPSHOT-master.jar:. App
hello

---

This matches what I was seeing in my own projects. If the goal is ABI compatibility, I do not believe this is a change that should be suppressed.

That being said, if I misunderstood the intent of ProblemFilters.exclude[IncompatibleResultTypeProblem](...) I'm happy to push up the change.

[tgodzik]

I was thinking mostly about source compatibility here, but it might be good to postpone this change until we decide to break binary compatibility to avoid issues for the users.

We could follow up with an issue and group those breaking binary compatibility into one milestone maybe?

[blast-hardcheese]

@tgodzik Circling back, I realize I neglected to create an issue, as you indicated should be done, nor did I break this PR into components. That's on me, and something I'd like to rectify.

Given that, and the fact that time has passed, where do we stand on moving forward? What should I do to advance this?

[tgodzik]

@tgodzik Circling back, I realize I neglected to create an issue, as you indicated should be done, nor did I break this PR into components. That's on me, and something I'd like to rectify.

Given that, and the fact that time has passed, where do we stand on moving forward? What should I do to advance this?

We are doing some other binary incompatible changes at this time, but until we decide on it I will maybe add a label and wait until we do decide that.