Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
iravid opened this issue Aug 8, 2020 · 3 comments

Comments

@iravid
Copy link

iravid commented Aug 8, 2020

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
Copy link
Contributor

raboof commented Aug 10, 2020

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
Copy link
Author

iravid commented Aug 10, 2020

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

@dwijnand
Copy link
Collaborator

dwijnand commented Aug 10, 2020

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... scala/bug#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):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants