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

Add test for public method becomes private[lib] #375

Closed
wants to merge 1 commit into from
Closed

Add test for public method becomes private[lib] #375

wants to merge 1 commit into from

Conversation

ashawley
Copy link
Contributor

@ashawley ashawley commented Aug 17, 2019

This came up as an issue when updating to MiMa 0.5.0 in ScalaCheck, see typelevel/scalacheck#509

@ashawley ashawley changed the title Add test for public method becomes private[foo] Add test for public method becomes private[lib] Aug 17, 2019
@dwijnand
Copy link
Collaborator

It passed, so this isn't an accurate minimisation.

@ashawley
Copy link
Contributor Author

ashawley commented Aug 17, 2019

There didn't appear to be a test for making a method package-private, so I wanted to verify the base case. Although, I tried to add the complexity of implicits and overloading that were in the issue from ScalaCheck. I wonder what I'm missing to reproduce this? I'll try some more...

@ashawley
Copy link
Contributor Author

ashawley commented Aug 17, 2019

Here's the test failure, but it's not the same one...

@dwijnand
Copy link
Collaborator

Yeah that the loss of a static forwarder, which is a potential bincompat concern.

@ashawley
Copy link
Contributor Author

A simple test suggests that marking a static method as package-private should be binary compatible:

package bar
class A[T](t: T)
object A {
  def foo[T](x: T): A[T] = new A(x)
  def foo[T](x: T, y: T): A[T] = new A(y)
}
package bar
class A[T](t: T)
object A {
  def foo[T]( x: T): A[T] = new A(x)
  private[bar] def foo[T](x: T, y: T): A[T] = new A(y)
}
$ sbt test-public-method-becomes-package-private-ok/test
$ cd functional-tests/src/test/public-method-becomes-package-private-nok
$ scalac -classpath $(echo target/scala-2.12/*-V1.jar) -d ./classes App.scala
$ scala -classpath $(echo target/scala-2.12/*-V1.jar):./classes baz.App
Success
$ scala -classpath $(echo target/scala-2.12/*-V2.jar):./classes baz.App
Success

Here's baz.App:

package baz
object App {
  def main(args: Array[String]): Unit = {
    bar.A.foo[Int](1,2)
    println("Success")
  }
}

Is this a false positive?

@dwijnand
Copy link
Collaborator

No. Scala doesn't use the static method, it uses the non-static method on the singleton object value. The binary incompatibility is only observable at runtime by calling from Java.

@ashawley
Copy link
Contributor Author

I've converted the example App in to Java. Assuming I'm correctly calling the static method from Java as you suggested, it still doesn't seem to throw an exception for me:

$ javac -cp ./target/scala-2.12/*-V1.jar -d ./classes App.java
$ java -cp ~/.ivy2/cache/org.scala-lang/scala-library/jars/scala-library-2.12.9.jar:$(echo target/scala-2.12/*-V1.jar):./classes baz.App
Success
$ java -cp ~/.ivy2/cache/org.scala-lang/scala-library/jars/scala-library-2.12.9.jar:$(echo target/scala-2.12/*-V2.jar):./classes baz.App
Success
package baz;
class App {
  public static void main(String[] args) {
    bar.A$.MODULE$.foo(1,2);
    System.out.println("Success");
  }
}

@dwijnand
Copy link
Collaborator

Nope. MODULE$ is the singleton instance. The static method is bar.A.foo, the same syntax you had in Scala.

@ashawley
Copy link
Contributor Author

Ok, now I follow. Yes, indeed. It fails with that single line change:

package baz;
class App {
  public static void main(String[] args) {
    bar.A.foo(1,2); // !!!
    System.out.println("Success");
  }
}

Resulting in:

Exception in thread "main" java.lang.NoClassDefFoundError: bar/A
        at baz.App.main(App.java:4)
Caused by: java.lang.ClassNotFoundException: bar.A
        at java.net.URLClassLoader.findClass(URLClassLoader.java:382)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
        at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:349)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
        ... 1 more

Thanks for explaining this! Sorry for all the questions.

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

Successfully merging this pull request may close these issues.

2 participants