A question about static forwarders for Java package private methods

[vasilmkd]

Compiler version 3.1.1

Minimized code

https://github.com/vasilmkd/static-forwarders-question

We encountered this issue in Cats Effect.

In the linked reproduction project, you can find two code paths, which correspond to each of our questions.

1. https://github.com/vasilmkd/static-forwarders-question/blob/main/src/main/java/base/Base.java and https://github.com/vasilmkd/static-forwarders-question/blob/main/src/main/scala/mypackage/MyObject.scala show a case where an abstract class defined in Java, which contains a package private Java method and a Scala object which extends this class in another, unrelated package. Inspecting the produced bytecode shows the following situation:

Compiled from "Base.java"
public abstract class base.Base {
  public base.Base();
    Code:
       0: aload_0
       1: invokespecial #1                  // Method java/lang/Object."<init>":()V
       4: return

  void packagePrivateMethod();
    Code:
       0: return
}

Compiled from "MyObject.scala"
public final class mypackage.MyObject$ extends base.Base implements java.io.Serializable {
  public static final mypackage.MyObject$ MODULE$;

  private mypackage.MyObject$();
    Code:
       0: aload_0
       1: invokespecial #13                 // Method base/Base."<init>":()V
       4: return

  public static {};
    Code:
       0: new           #2                  // class mypackage/MyObject$
       3: dup
       4: invokespecial #16                 // Method "<init>":()V
       7: putstatic     #18                 // Field MODULE$:Lmypackage/MyObject$;
      10: return

  private java.lang.Object writeReplace();
    Code:
       0: new           #22                 // class scala/runtime/ModuleSerializationProxy
       3: dup
       4: ldc           #2                  // class mypackage/MyObject$
       6: invokespecial #25                 // Method scala/runtime/ModuleSerializationProxy."<init>":(Ljava/lang/Class;)V
       9: areturn
}

and finally:

Compiled from "MyObject.scala"
public final class mypackage.MyObject {
  public static void packagePrivateMethod();
    Code:
       0: getstatic     #13                 // Field mypackage/MyObject$.MODULE$:Lmypackage/MyObject$;
       3: invokevirtual #15                 // Method mypackage/MyObject$.packagePrivateMethod:()V
       6: return
}

Notice that the package private method exists in the Java abstract class bytecode output (as it should), it doesn't exist in the bytecode output of MyObject$ (as it should), but it does exist in the bytecode output of MyObject as a public static forwarder method. Again, please take notice that this is a public static forwarder to a package private method.

Our question is, why is this necessary, and how is this safe from an encapsulation viewpoint? As a reference, Scala 2.13 and 2.12 don't compile these public static forwarders.

2. The reproduction project contains another case (which is what was exactly encountered in Cats Effect), it is the following source code: https://github.com/vasilmkd/static-forwarders-question/blob/main/src/main/scala/classvalue/MyClassValue.scala. Again, inspecting the bytecode shows us the following:

Compiled from "MyClassValue.scala"
public final class classvalue.MyClassValue$ extends java.lang.ClassValue<java.lang.String> {
  public static final classvalue.MyClassValue$ MODULE$;

  private classvalue.MyClassValue$();
    Code:
       0: aload_0
       1: invokespecial #14                 // Method java/lang/ClassValue."<init>":()V
       4: return

  public static {};
    Code:
       0: new           #2                  // class classvalue/MyClassValue$
       3: dup
       4: invokespecial #17                 // Method "<init>":()V
       7: putstatic     #19                 // Field MODULE$:Lclassvalue/MyClassValue$;
      10: return

  private java.lang.Object writeReplace();
    Code:
       0: new           #23                 // class scala/runtime/ModuleSerializationProxy
       3: dup
       4: ldc           #2                  // class classvalue/MyClassValue$
       6: invokespecial #26                 // Method scala/runtime/ModuleSerializationProxy."<init>":(Ljava/lang/Class;)V
       9: areturn

  public java.lang.String computeValue(java.lang.Class<?>);
    Code:
       0: getstatic     #35                 // Field scala/Predef$.MODULE$:Lscala/Predef$;
       3: invokevirtual #39                 // Method scala/Predef$.$qmark$qmark$qmark:()Lscala/runtime/Nothing$;
       6: athrow
       7: athrow

  public java.lang.Object computeValue(java.lang.Class);
    Code:
       0: aload_0
       1: aload_1
       2: invokevirtual #46                 // Method computeValue:(Ljava/lang/Class;)Ljava/lang/String;
       5: areturn
}

and

Compiled from "MyClassValue.scala"
public final class classvalue.MyClassValue {
  public static void bumpVersion();
    Code:
       0: getstatic     #21                 // Field classvalue/MyClassValue$.MODULE$:Lclassvalue/MyClassValue$;
       3: invokevirtual #23                 // Method classvalue/MyClassValue$.bumpVersion:()V
       6: return

  public static java.lang.ClassValue$Entry<java.lang.String> castEntry(java.lang.ClassValue$Entry<?>);
    Code:
       0: getstatic     #21                 // Field classvalue/MyClassValue$.MODULE$:Lclassvalue/MyClassValue$;
       3: aload_0
       4: invokevirtual #28                 // Method classvalue/MyClassValue$.castEntry:(Ljava/lang/ClassValue$Entry;)Ljava/lang/ClassValue$Entry;
       7: areturn

  public static java.lang.String computeValue(java.lang.Class<?>);
    Code:
       0: getstatic     #21                 // Field classvalue/MyClassValue$.MODULE$:Lclassvalue/MyClassValue$;
       3: aload_0
       4: invokevirtual #33                 // Method classvalue/MyClassValue$.computeValue:(Ljava/lang/Class;)Ljava/lang/String;
       7: areturn

  public static java.lang.Object get(java.lang.Class);
    Code:
       0: getstatic     #21                 // Field classvalue/MyClassValue$.MODULE$:Lclassvalue/MyClassValue$;
       3: aload_0
       4: invokevirtual #37                 // Method classvalue/MyClassValue$.get:(Ljava/lang/Class;)Ljava/lang/Object;
       7: areturn

  public static boolean match(java.lang.ClassValue$Entry<?>);
    Code:
       0: getstatic     #21                 // Field classvalue/MyClassValue$.MODULE$:Lclassvalue/MyClassValue$;
       3: aload_0
       4: invokevirtual #42                 // Method classvalue/MyClassValue$.match:(Ljava/lang/ClassValue$Entry;)Z
       7: ireturn

  public static void put(java.lang.Class, java.lang.Object);
    Code:
       0: getstatic     #21                 // Field classvalue/MyClassValue$.MODULE$:Lclassvalue/MyClassValue$;
       3: aload_0
       4: aload_1
       5: invokevirtual #46                 // Method classvalue/MyClassValue$.put:(Ljava/lang/Class;Ljava/lang/Object;)V
       8: return

  public static void remove(java.lang.Class<?>);
    Code:
       0: getstatic     #21                 // Field classvalue/MyClassValue$.MODULE$:Lclassvalue/MyClassValue$;
       3: aload_0
       4: invokevirtual #51                 // Method classvalue/MyClassValue$.remove:(Ljava/lang/Class;)V
       7: return

  public static java.lang.ClassValue$Version<java.lang.String> version();
    Code:
       0: getstatic     #21                 // Field classvalue/MyClassValue$.MODULE$:Lclassvalue/MyClassValue$;
       3: invokevirtual #56                 // Method classvalue/MyClassValue$.version:()Ljava/lang/ClassValue$Version;
       6: areturn
}

Notice that, again, public static forwarders were created for package private methods (to verify you can check the source code of ClassValue here).

Lately, we've been experimenting with using the -release flag of javac and scalac to publish Cats Effect using a modern JDK (17 and up), while keeping compatibility with JDK 8. Unfortunately, this broke our Scala 3 CI during the MiMa binary compatibility checks, with the following output (notice that the mentioned missing methods are all of the public static forwarders that were created for package private methods):

[error] cats-effect: Failed binary compatibility check against org.typelevel:cats-effect_3:3.2.6 (e:info.apiURL=https://typelevel.org/cats-effect/api/3.x/, e:info.versionScheme=early-semver)! Found 5 potential problems (filtered 71)
[error]  * static method bumpVersion()Unit in class cats.effect.tracing.Tracing does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.effect.tracing.Tracing.bumpVersion")
[error]  * static method castEntry(java.lang.ClassValue#Entry)java.lang.ClassValue#Entry in class cats.effect.tracing.Tracing does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.effect.tracing.Tracing.castEntry")
[error]  * static method match(java.lang.ClassValue#Entry)Boolean in class cats.effect.tracing.Tracing does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.effect.tracing.Tracing.match")
[error]  * static method put(java.lang.Class,java.lang.Object)Unit in class cats.effect.tracing.Tracing does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.effect.tracing.Tracing.put")
[error]  * static method version()java.lang.ClassValue#Version in class cats.effect.tracing.Tracing does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.effect.tracing.Tracing.version")
[error] stack trace is suppressed; run last coreJVM / mimaReportBinaryIssues for the full output
[error] (coreJVM / mimaReportBinaryIssues) Failed binary compatibility check against org.typelevel:cats-effect_3:3.2.6 (e:info.apiURL=https://typelevel.org/cats-effect/api/3.x/, e:info.versionScheme=early-semver)! Found 5 potential problems (filtered 71)
[error] Total time: 15 s, completed Mar 5, 2022, 11:50:40 AM

We tried reproducing the issue locally, and we were indeed able to. The difference seems to be the -release flag on Scala 3, which makes Scala 3 not generate the public static forwarders methods, and indeed, MiMa reports this as a binary incompatible change, as it should.

Again, we're only seeing this on Scala 3. Scala 2 does not generate these forwarder methods and the behavior doesn't change when -release is used (they are again, not generated).

Our question here is, why does using -release change the behavior of Scala specific features in Scala 3 (AFAIK, it is Scala's choice as a language to generate static forwarder methods for better Java interop).

If anything above is unclear or hard to follow, please do not hesitate to ask me for more clarification. Thank you in advance for reading.

cc @djspiewak @armanbilge

[smarter]

Again, we're only seeing this on Scala 3. Scala 2 does not generate these forwarder methods

Sounds like a bug then, a fix would be appreciated!

why does using -release change the behavior of Scala

Using -release means we read the JDK symbols from the special symbol files the JDK provide for each release instead of simply reading the classfiles, I guess the symbol files don't export these package private methods.

[smarter]

For reference, the logic for adding forwarders in Scala 3 is in https://github.com/lampepfl/dotty/blob/1b25f653855ac485f9e0513be9b27c0f29725f6f/compiler/src/dotty/tools/backend/jvm/BCodeHelpers.scala#L571-L612
The corresponding Scala 2 code is https://github.com/scala/scala/blob/ce02613de571004cf7d0e4cc22ae919d825c947f/src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala#L826-L853
Since the Scala 3 backend was forked from the Scala 2 backend ~8 years ago, we're missing a bunch of refactors/fixes, so it's possible there's one commit in the Scala 2 backend history that could simply be forward-ported, or it's possible the problem happens before (e.g., maybe the Scala 2 ClassfileParser ignores these methods in the first place, or represent them with different flags?)

[vasilmkd]

I would love to take a crack at the bug fix, and I most likely will when I find some time. The question is however, fixing this issue means breaking JVM binary compatibility, as methods that used to be available in previously compiled code would disappear after the fix. Are there any concerns and procedures over that fact?

[smarter]

If I understand this correctly, since the forwarders point to package private methods, calling them would result in a runtime error since they're not accessible, so removing them doesn't makes things any worse even if it technically breaks the ABI, and so we should be able to make an exception for it /cc @sjrd.

[vasilmkd]

That's a great summary, thanks.

[sjrd]

If I understand this correctly, since the forwarders point to package private methods, calling them would result in a runtime error since they're not accessible, so removing them doesn't makes things any worse even if it technically breaks the ABI, and so we should be able to make an exception for it

Yes, that's correct.

At least, it's correct as long as the child Scala class is in a different package than the parent Java class. If they are in the same package, the call would in fact succeed. That said, it is still a relatively serious violation of encapsulation (that is only visible to Java), so I think we should fix it.

[smarter]

Aha, found the culprit, when generating forwarders we check for accessibility using:
https://github.com/lampepfl/dotty/blob/e078e79f6fdf8a7408b9347315eccc50432b4fd1/compiler/src/dotty/tools/backend/jvm/BCodeHelpers.scala#L598 which leads to: https://github.com/lampepfl/dotty/blob/e078e79f6fdf8a7408b9347315eccc50432b4fd1/compiler/src/dotty/tools/dotc/core/SymDenotations.scala#L1391
The use of !ctx.phase.erasedTypes here means that package-private members are considered public after erasure, which matches how Scala-defined private[foo] members behave, but not how Java-defined members behave, so I think the fix is as simple as:

diff --git compiler/src/dotty/tools/dotc/core/SymDenotations.scala compiler/src/dotty/tools/dotc/core/SymDenotations.scala
index 7be005ea4b8..e1a54325af0 100644
--- compiler/src/dotty/tools/dotc/core/SymDenotations.scala
+++ compiler/src/dotty/tools/dotc/core/SymDenotations.scala
@@ -1388,7 +1388,7 @@ object SymDenotations {
     final def accessBoundary(base: Symbol)(using Context): Symbol =
       if (this.is(Private)) owner
       else if (this.isAllOf(StaticProtected)) defn.RootClass
-      else if (privateWithin.exists && !ctx.phase.erasedTypes) privateWithin
+      else if (privateWithin.exists && (!ctx.phase.erasedTypes || this.is(JavaDefined))) privateWithin
       else if (this.is(Protected)) base
       else defn.RootClass

Do you mind making a PR for that with some appropriate tests? You can make a run test (in tests/run) with a main that checks what forwarders are generated using Java runtime reflection, similar to https://github.com/lampepfl/dotty/blob/main/tests/run/forwarder.scala / https://github.com/lampepfl/dotty/blob/main/tests/run/forwarder.check except using classOf|Foo.type] to get the Class instance of the companion.

[vasilmkd]

On it!

[smarter]

except using classOf|Foo.type] to get the Class instance of the companion.

Oops no, that should be classOf[Foo] since we're looking for the static forwarders defined in the class itself.

[vasilmkd]

Yes, I already figured that out. Can you please help me with how to run the test I wrote?

[smarter]

run testCompilation tests/run/foo.scala from sbt. For more info, see http://dotty.epfl.ch/docs/contributing/testing.html