Fixes #15736 blocking Scala 3 on Android

[ddtthh]

Fixes #15736

Box native instantiated method return type if sam method return type is not a primitive type to satisfy conditions specified in
https://docs.oracle.com/javase/8/docs/api/java/lang/invoke/LambdaMetafactory.html
Condition is not enforced by JVM but by Android ART.

[ddtthh]

Just to provide some more information on my reasoning for the fix:

• Scala represents primitive types as the respective Scala types, so there is no difference between boxed and primitive.
• The JVM backend converts those to Java primitive types in method and function signatures, so there is no java.lang.Boolean in signatures ever unless it was in the original Scala code.
• If the sam method type contains primitive types, the respective types on the implementing method have to be primitive as well, so there is no issue in that direction.
• If the sam method type contains more general reference types, the implementation method type can contain primitive parameters
• For argument types this is not an issue, as argument types cannot be narrowed on implementation, scala will generate a bridge method with the reference types.
• For the return type it is an issue, as an implementation may have a narrowed return type, this is probably why scala 3 does not generate the bridge method in that case. And even if a bridge method is generated due to parameter types, it still has the same return type.
• JVM metafactory does automatically adapt the implementing method to the sam type for primitive parameters, so calling a function returning boolean for a sam method signature returning a ref type is fine.
• The instantiated method type is checked at runtime. The JVM adapts this type as well, but the specification does not state that. Android D3 emits a runtime check for the primitive type even if the sam method returns a ref type.

Following these observations I've only added a assertion to check the condition for the whole signature and just implemented boxing of the return type in the instantiated method type.

I've also compiled most of the compiler run tests into an android app to check whether there are more bytecode issues with android, and there seem not to be any.

[lrytz]

I have some memory of a semantic mismatch in null unboxing between Scala and LambdaMetaFactory: scala/scala#4463 (comment). Just posting that as-is, I haven't looked at this PR or the ticket in detail yet.

[ddtthh]

I have some memory of a semantic mismatch in null unboxing between Scala and LambdaMetaFactory: scala/scala#4463 (comment). Just posting that as-is, I haven't looked at this PR or the ticket in detail yet.

Interesting, yes current scala 3 still does the unboxing manually in the adapted method for the parameters. It does not do the boxing for the return value and it does not need to as there is not special null case for boxing.

[lrytz]

Given

class Foo[T]():
  val f = (x: T) => true

we have (Object)Object as the SAM signature, (Object)Z as the implementation method signature, and now with this PR we'll have (Object)Boolean as the "dynamicMethodType".

I don't really know what dynamicMethodType is for, the documentation is very thin. LMF is creating a method with that signature because we put it in the bridges list:

scala> println(((x: String) => true).getClass.getMethods.filter(_.getName == "apply").mkString("\n"))
public java.lang.Object rs$line$1$$$Lambda/0x0000000800620000.apply(java.lang.Object)
public java.lang.Boolean rs$line$1$$$Lambda/0x0000000800620000.apply(java.lang.String)

before this PR, the apply method had return type primitive boolean.

I was going to ask if it makes sense to come up with a new signature for dynamicMethodType out of thin air... But then I tested Java

@FunctionalInterface
interface SP<R> {
    R test(String str);
}

public class A {
  public static boolean impl(String s) { return true; }
  public SP<Boolean> f() { return A::impl; }
}

And that produces

    INVOKEDYNAMIC test()LSP; [
      // handle kind 0x6 : INVOKESTATIC
      java/lang/invoke/LambdaMetafactory.metafactory(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite;
      // arguments:
      (Ljava/lang/String;)Ljava/lang/Object;,    <<< interfaceMethodType
      // handle kind 0x6 : INVOKESTATIC
      A.impl(Ljava/lang/String;)Z,               <<< implementation method
      (Ljava/lang/String;)Ljava/lang/Boolean;    <<< dynamicMethodType
    ]

That's a bit less thin air because of the explicit SP<Boolean>, but at least it shows similar LMF args as this PR is producing.

I guess the alternative solution to this PR is emitting $anonfun$adapted methods for primitive result types, like Scala 2 (see https://github.com/scala/scala3/blob/3.6.3/compiler/src/dotty/tools/dotc/transform/Erasure.scala#L464-L488).

[lrytz]

This is also used in bridgeTypes below, so the change also affects the altMethods argument of LMF.altMetafactory.

After digging a little, it seems these bridges are there (since Scala 2) to support structural calls on LMF function instances, see https://github.com/scala/scala3/blob/3.6.3/compiler/src/dotty/tools/backend/jvm/BCodeBodyBuilder.scala#L1791-L1793

But trying that in Scala 3, the code actually doesn't type check

scala> val f = (x: String) => x
val f: String => String = Lambda/0x0000000800662000@4e1fd34b

scala> val g: { def apply(x: String): String } = f
-- [E007] Type Mismatch Error: -------------------------------------------------
1 |val g: { def apply(x: String): String } = f
  |                                          ^
  |                            Found:    (f : String => String)
  |                            Required: Object{def apply(x: String): String}

The reason seems to be a fix for an unsoundness with structural types: #12214 (comment) / scala/bug#10414.

Indeed, in Scala 2:

scala> class B[T, R] extends Function1[T, R] { def apply(s: T): R = null.asInstanceOf[R] }
scala> val f: String => String = new B[String, String]
scala> val g: { def apply(s: String): String } = f
scala> g("")
java.lang.NoSuchMethodException: B.apply(java.lang.String)
  at java.base/java.lang.Class.getMethod(Class.java:2395)

(Thanks to @dwijnand for digging in there with me).

So I'm not sure if it's even worth to make LMF synthesize any bridges in Scala 3. @smarter wdyt?

[ddtthh]

I'm not quite sure what you mean by "dynamicMethodType", I assume you mean what the LMF documentation calls "instantiatedMethodType". This just injects a dynamic typecheck on that type before and after invoking the implementing method. I assume it is for settings like this:

class Foo[T]:
    val f: T => String = _ => "asd"
val g = Foo[String].f.asInstanceOf[Any => String]
g(Nil) // class cast exception from Nil to String

f has type Function1 with a method apply with erased type (Object;)Object and the implementing Method has erased type (Object);String
To generate the function object for g one could call the metafactory with sam type (Object;)Object, invokedType (Object;)String and an instantiated type of (String;)String. The generated proxy would then ensure the class cast exception.

Scala however does generate a bridge with type (String;)String, uses that and an invoked type of (String;)String.

I could not produce any situation where the instantiated type would actually be narrower than the invoked type. So probably you could just set it to the sam type all the time without losing any class cast exceptions. That is a somewhat riskier change though than what I did.

Whatever you chose to do, the instantiated method type has to be a compatible with the sam type without adapting between primitive and box types to conform to the documentation of LMF.

[lrytz]

not quite sure what you mean by "dynamicMethodType"

dynamicMethodType in the LMF documentation / instantiatedMethodType in the compiler backend source code. We should have called it the same in the compiler sources...

Scala however does generate a bridge with type (String;)String

Do you mean concretely in your class Foo[T] example? I think this is not the case. Take a look at the indy bytecode, or use reflection to list the apply methods of g. There are two: (Object)Object and (Object)String, but no (String)String. Note that g is the same object/instance as f.

Generally, Scala or the JVM have no guarantees about checking dynamic types and ensuring ClassCastExceptions are issued. For example:

scala> class C[T] { def f(t: T) = "x" }
scala> (new C[String]).asInstanceOf[C[Option[Int]]].f(None) // no CCE
val res0: String = x

I could not produce any situation where the instantiated type would actually be narrower than the invoked type.

I think you're right, we don't do that in Scala. Java uses that feature for method references:

public class A {
  static String impl(Object s) { return ""; }
  F<String, String> f = A::impl;
}

interface F<T, R> { R m(T s); }

gives

    INVOKEDYNAMIC m()LF; [
      // handle kind 0x6 : INVOKESTATIC
      java/lang/invoke/LambdaMetafactory.metafactory(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite;
      // arguments:
      (Ljava/lang/Object;)Ljava/lang/Object;, 
      // handle kind 0x6 : INVOKESTATIC
      A.impl(Ljava/lang/Object;)Ljava/lang/String;, 
      (Ljava/lang/String;)Ljava/lang/String;
    ]

the instantiated method type has to be a compatible with the sam type without adapting between primitive and box types to conform to the documentation of LMF.

👍

[ddtthh]

dynamicMethodType in the LMF documentation

Ah, for Java 11 it was still called instantiatedMethodType, they have changed that to dynamicMethodType at some point.

Do you mean concretely in your class Foo[T] example?

No, not concretely. It could be used, but Scala uses the bridge with (String)String. I believe Scala never relies on the dynamicMethodType.

Either scala generates some kind of bridge when instantiating type parameters or it just ignores the runtime type in case of casting with asInstanceOf. Which is fine, there are no guarantees on runtime type checking as you've said.

[lrytz]

I think we can remove that for that for the final version

[keynmol]

Hello! Just to add a datapoint, this PR is very important to enable Android development with Scala 3. I hope it's not forgotten (thanks @lrytz for reviewing and massive thanks to @ddtthh for fixing it in the first place!).

1. I've been using @mcallisto's template: https://github.com/mcallisto/Scala-3-Android-Studio-template
2. Added some innocuous code that uses STTP with OkHTTP backend (don't ask how I arrived at this combo, it's a horrible story)

At that point, Mario discovered that if we use minifyEnabled on the project, then the app runs fine, but the reloading/rebuilding is very slow.
Not using minifyEnabled leads to errors like this:

                                                                                                    FATAL EXCEPTION: pool-2-thread-1
Process: com.example.scalacoremodule, PID: 28264
java.lang.VerifyError: Verifier rejected class sttp.model.UriInterpolator$$$ExternalSyntheticLambda5: boolean sttp.model.UriInterpolator$$$ExternalSyntheticLambda5.apply(java.lang.String) failed to verify: boolean sttp.model.UriInterpolator$$$ExternalSyntheticLambda5.apply(java.lang.String): [0x4] check-cast on unexpected class Boolean (declaration of 'sttp.model.UriInterpolator$$$ExternalSyntheticLambda5' appears in /data/app/~~VjsHCjk11_HYHZQ25nv_dw==/com.example.scalacoremodule-xvs-Aubm6kiBLi6pM04avQ==/base.apk!classes8.dex)
	at sttp.model.UriInterpolator$.interpolate(UriInterpolator.scala:63)
	at sttp.model.UriInterpolator$UriContext.uri(UriInterpolator.scala:39)
	at com.example.core.Foo$.getProjectInfo(Foo.scala:26)
	at com.example.core.Foo.getProjectInfo(Unknown Source:2)
	at com.example.scalacoremodule.MainActivityKt$Form$1.invoke$lambda$8$lambda$7$lambda$6(MainActivity.kt:70)
	at com.example.scalacoremodule.MainActivityKt$Form$1.$r8$lambda$witBcHAba7sO4rT2bj-8PrHKr8Y(Unknown Source:0)
	at com.example.scalacoremodule.MainActivityKt$Form$1$$ExternalSyntheticLambda5.run(D8$$SyntheticClass:0)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:644)
	at java.lang.Thread.run(Thread.java:1012)

In various places of the STTP libraries.

So I

1. Published this PR locally
2. Published and recompiled all affected STTP modules, with new Scala version
3. Finally, was able to reload the app without having to turn minification on. Which is much, much faster!

https://github.com/user-attachments/assets/3e0bb9b9-49ea-470b-b99f-6de72dbe0784

[mcallisto]

At that point, Mario discovered that if we use minifyEnabled on the project, then the app runs fine, but the reloading/rebuilding is very slow.

Excellent work @keynmol and thank you @ddtthh for this breakthrough. Just to set the record straight this is not my discovery, it was suggested to me by @pfn on Discord > Scala > scala-android.

[lrytz]

(I don't want to be the enemy of good here with my complicated review, but this is changing bytecode of lambdas and should not be yolo'd.)

Ping @smarter, or maybe @sjrd.

[Gedochao]

@ddtthh note that the check for the Scala 3 CLA is failing on the CI. Signing it is necessary for contributors.
https://contribute.akka.io/cla/scala
Please touch the last commit to re-run the CI once it's signed.

[sjrd]

(I don't want to be the enemy of good here with my complicated review, but this is changing bytecode of lambdas and should not be yolo'd.)

Ping @smarter, or maybe @sjrd.

The proposed change looks reasonable to me. It doesn't seem risky to me. If it didn't work it would haven blown up hundreds of time in the standard library alone. And I don't see how it could make performance worse, given that boxing must happen one way or another.

I'm not sure about the bridges issue. Supposedly the bridge generation needs to be in sync with whatever signature we use for the "main" SAM method. So should be fine as is?

[lrytz]

I assume the change to dynamicMethodType/instantiatedMethodType is fine, though I still didn't find documentation about what this type is used for.. Your explanation is hopefully enough

This just injects a dynamic typecheck on that type before and after invoking the implementing method

But I'm really unsure about generating a bridge method in the lambda with that same signature. In current Scala 3:

scala> val f = (x: String) => true
val f: String => Boolean = Lambda/0x00000008005c4400@37f41a81

scala> val g: { def apply(x: String): Boolean } = f.asInstanceOf[{ def apply(x: String): Boolean }]
val g: Object{def apply(x: String): Boolean} = Lambda/0x00000008005c4400@37f41a81

scala> import scala.reflect.Selectable.reflectiveSelectable

scala> g.apply("")
val res0: Boolean = true

The cast is needed because of #12214 (comment) / scala/bug#10414.

Since that bridge can only ever be used with a cast, do we even want to generate it?

After this PR, the bridge has a different signature and we get

scala> g.apply("")
java.lang.NoSuchMethodException: rs$line$1$$$Lambda/0x00000008005ac400.apply(java.lang.String)
  at java.base/java.lang.Class.getMethod(Class.java:2395)

So the bridge seems even less useful to have. Concretely, add this change:

-      val bridgeTypes = (
-        if (needsGenericBridge)
-          instantiatedMethodType +: overriddenMethodTypes
-        else
-          overriddenMethodTypes
-      ).distinct.filterNot(_ == samMethodType)
+      val bridgeTypes = overriddenMethodTypes.distinct.filterNot(_ == samMethodType)

[ddtthh]

I assume the change to dynamicMethodType/instantiatedMethodType is fine, though I still didn't find documentation about what this type is used for.. Your explanation is hopefully enough

Java 21 docs says: "dynamicMethodType - The signature and return type that should be enforced dynamically at invocation time." I've checked that the JVM does this by calling LMF manually.

But I'm really unsure about generating a bridge method in the lambda with that same signature. In current Scala 3:

After this PR, the bridge has a different signature and we get

scala> g.apply("")
java.lang.NoSuchMethodException: rs$line$1$$$Lambda/0x00000008005ac400.apply(java.lang.String)
  at java.base/java.lang.Class.getMethod(Class.java:2395)

I can't replicate this. The signature changes from (String)Z to (String)Boolean. But for me, the call still succeeds. Looking at the source of reflectiveSelectable applyDynamic selects the method without considering the return type. So NoSuchMethodException would be odd.

As the signature of applyDynamic is :(Ljava/lang/String;Lscala/collection/immutable/Seq;Lscala/collection/immutable/Seq;)Ljava/lang/Object; there isn't even a difference in the byte-code at the call side, as the returned boolean has to be unboxed either way.

[lrytz]

I can't replicate this.

Oh I'm very sorry, I mixed up with a locally patched version 🤦‍♂️ I still think we should get rid of the bridge, but it's a separate issue.

So I agree to go ahead with this PR, I'd just remove the assertion (lines 1801-1807).

[ddtthh]

So I agree to go ahead with this PR, I'd just remove the assertion (lines 1801-1807).

Thank you for reviewing it.
The assertion can be removed, I did put it in to ensure that there are no other cases I've missed.

[lrytz]

LGTM, but can you squash the commits into one? We try to keep the git history compact.

---

@ddtthh @keynmol @mcallisto by the way: what is the toolchain for Scala on Android these days?

A while back, Maciej Gorywoda was active in the space and wrote this article on the website: https://docs.scala-lang.org/tutorials/scala-on-android.html. But IIUC, the template mentioned above (https://github.com/mcallisto/Scala-3-Android-Studio-template) doesn't use GraalVM / native code but converts the Scala .class files to dex with d8? Does that toolchain fully support Java 8 (version 52) .class files as produced by Scala 2 / 3?

We recently announced that Scala 3 is bumping its bytecode to Java 17 (version 61)

• Announcement: https://www.scala-lang.org/news/next-scala-lts-jdk.html
• Android was mentioned in the forum discussion (https://contributors.scala-lang.org/t/next-scala-3-lts-version-will-increase-minimum-required-jdk-version/7018), but I'm not sure it was conclusive?
• Is there a difference to make between bytecode features and JDK API? The latter should not be a problem thanks to the -release Scala compiler flag.

Having up to date documentation, an example repo, known issues, etc would be really highly appreciated! We'd be happy to help out getting up to date information to the scala-lang website (cc @SethTisue).

[SethTisue]

Is this sufficiently safe to backport to 3.3 LTS?

Am I right to guess that in order for to actually build an Android application, every Scala 3 based library on the app's classpath must have been built with this fix? If that's the case, then backporting this to LTS would hugely speedup the timetable on which the fix will actually help end users.

Followup question: if my guess is right, then do Scala 2 libraries also need the fix, and do we thus need to backport this to Scala 2...?

[Gedochao]

cc @tgodzik @WojciechMazur @prolativ

[tgodzik]

Is this sufficiently safe to backport to 3.3 LTS?

Am I right to guess that in order for to actually build an Android application, every Scala 3 based library on the app's classpath must have been built with this fix? If that's the case, then backporting this to LTS would hugely speedup the timetable on which the fix will actually help end users.

Followup question: if my guess is right, then do Scala 2 libraries also need the fix, and do we thus need to backport this to Scala 2...?

Will try to backport it to 3.3.7. 3.3.6 will contain everything up to 3.6.4

[ddtthh]

Is this sufficiently safe to backport to 3.3 LTS?

It should be, the previous changes to that code are 4 years old.

Am I right to guess that in order for to actually build an Android application, every Scala 3 based library on the app's classpath must have been built with this fix?

Yes.

Followup question: if my guess is right, then do Scala 2 libraries also need the fix, and do we thus need to backport this to Scala 2...?

As far as I know, it does not have a similar issue, and hasn't the backend also been rewritten for Scala 3? Scala 2.11 also was perfectly supported on Android.

[keynmol]

Given existing code compiled by older version of Scala 3, is there a purely mechanical way of

1. Finding the offending part of the byte code
2. Patching it?

I'm thinking of something like a SBT plugin that could handle post-processing existing library jars which don't have this fix.

The fact that it's a runtime check by Android makes the whole situation more annoying as you might not exercise all the paths where this could crash.

[SethTisue]

hasn't the backend also been rewritten for Scala 3?

actually no! it was brought over from the Scala 2 compiler pretty much as-is. there has been some divergence since, but no rewrite

[ddtthh]

1. Finding the offending part of the byte code
2. Patching it?

Yes, there is. You just have to go through the bootstrap methods listed in the class file and replace the primitive types in the dynamic signature if the interface signature has a reference type.
Someone mentioned that minification during the build process fixes this as well. I guess that the types are corrected there as a side-effect.

[sideeffffect]

@lrytz

converts the Scala .class files to dex with d8? Does that toolchain fully support Java 8 (version 52) .class files as produced by Scala 2 / 3?

Is there a difference to make between bytecode features and JDK API?

The latest Android has support for a subset of Java 17 bytecode (even some subset of 21)and a subset of the Java 17 standard library. More details are here

• https://developer.android.com/build/jdks#compileSdk
• https://developer.android.com/about/versions/14/features#core-libraries
• https://developer.android.com/studio/write/java11-default-support-table

[rjolly]

Followup question: if my guess is right, then do Scala 2 libraries also need the fix, and do we thus need to backport this to Scala 2...?

I think 2.12+ needs the fix.

[SethTisue]

I think 2.12+ needs the fix.

I don't think so? Or, depends on what you mean by "needs'.

Our ambition level is to support Android development on Scala 3. Since Scala 3 can use 2.13 libraries, and in particular uses the 2.13 standard library, 2.13 enters the picture. But 2.12 is not in the picture.

I can see that 2.12 could be eligible for the fix, but we're very reluctant at this point to make any but the most necessary changes to 2.12, as per the "minimal maintenance" wording on https://www.scala-lang.org/development/

[rjolly]

@SethTisue Of course. I forgot that 2.13 = 3.