lightbend-labs / mima

A tool for catching binary incompatibility in Scala
Apache License 2.0
459 stars 71 forks source link

False positive when adding a method to a class with the same name as a method in the companion object #532

Open iravid opened 4 years ago

iravid commented 4 years ago

Given this in a previous version:

sealed abstract class ZManaged[R, E, A]
object ZManaged {
  case class Reservation[R, E, A]
  def reserve[R, E, A](r: Reservation[R, E, A]): ZManaged[R, E, A] = ???
}

Adding a method reserve to the class itself, like so:

sealed abstract class ZManaged[R, E, A] {
  def reserve: UIO[Reservation[R, E, A]]
}

triggers DirectMissingMethodProblem:

[error] zio: Failed binary compatibility check against dev.zio:zio_2.13:1.0.0! Found 1 potential problems
[error]  * static method reserve(zio.Reservation)zio.ZManaged in class zio.ZManaged does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("zio.ZManaged.reserve")

Changing the method's name to reserve2 causes the issue to not be reported.

raboof commented 4 years ago

I think the warning is strictly speaking correct.

When there is no overlap, a static forwarder is generated from Test.class to Test$.class:

$ cat Test.scala
class Test
object Test {
  def reserve(s: String) = ???
}
$ scalac Test.scala
$ javap Test.class
Compiled from "Test.scala"
public class Test {
  public static scala.runtime.Nothing$ reserve(java.lang.String);
  public Test();
}

When adding a method with that same name to class Test, however, the static forwarder is no longer generated:

$ cat Test.scala
class Test {
  def reserve = ???
}
object Test {
  def reserve(s: String) = ???
}
$ scalac Test.scala
$ javap Test.class
Compiled from "Test.scala"
public class Test {
  public scala.runtime.Nothing$ reserve();
  public Test();
}

So the change really does make the static forwarder method disappear. (I haven't checked, but I'm guessing it could lead to ambiguity otherwise?)

Now when you only expect this method to be called from Scala code, I'm pretty sure the Scala compiler will never use the static forwarder, and instead just invoke the function on the object directly. So if you don't expect this method to be ever called from e.g. Java or Kotlin code, you can probably filter out this problem.

I don't think this is a false positive in mima: this is exactly the kind of subtle/unexpected incompatibilities where mima is valuable for catching them early.

Perhaps it'd be reasonable to have a configuration option to specify you don't expect your library to be called from JVM languages other than Scala, and in that case automatically filter out this scenario - I'm not sure it's worth it though. In any case it would be nice to document this scenario somewhere, as it's probably a common cause for confusion. Perhaps we could even mention it in the failure message?

iravid commented 4 years ago

Thanks for the explanation @raboof! Agreed that this is not a false positive; sounds very important for Java/Kotlin interop usecases.

dwijnand commented 4 years ago

I don't think this is a false positive in mima: this is exactly the kind of subtle/unexpected incompatibilities where mima is valuable for catching them early.

💯

Perhaps it'd be reasonable to have a configuration option to specify you don't expect your library to be called from JVM languages other than Scala, and in that case automatically filter out this scenario - I'm not sure it's worth it though.

An option like that would increase the silent friction that Java users of Scala libraries suffer, which I'm not sure I want to back. Also I'm not sure if/when Scala calls the static forwarders: is it never?

In any case it would be nice to document this scenario somewhere, as it's probably a common cause for confusion. Perhaps we could even mention it in the failure message?

🤔 🤔 🤔 It would be good to have docs in the compiler (spec?) that help explain the story around static forwarder emission... https://github.com/scala/bug/issues/11305, which talks about generic signatures, gives a sense of how it's a convoluted story.

Perhaps MiMa could move to giving error messages ala Skunk (from https://twitter.com/tpolecat/status/1288937569863041026):