Skip to content

Fixes #15736 blocking Scala 3 on Android #22632

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 11, 2025
Merged

Fixes #15736 blocking Scala 3 on Android #22632

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions compiler/src/dotty/tools/backend/jvm/BCodeBodyBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1773,8 +1773,6 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder {
val returnUnit = lambdaTarget.info.resultType.typeSymbol == defn.UnitClass
val functionalInterfaceDesc: String = generatedType.descriptor
val desc = capturedParamsTypes.map(tpe => toTypeKind(tpe)).mkString(("("), "", ")") + functionalInterfaceDesc
// TODO specialization
val instantiatedMethodType = new MethodBType(lambdaParamTypes.map(p => toTypeKind(p)), toTypeKind(lambdaTarget.info.resultType)).toASMType

val samMethod = atPhase(erasurePhase) {
val samMethods = toDenot(functionalInterface).info.possibleSamMethods.toList
Expand All @@ -1787,7 +1785,21 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder {
}

val methodName = samMethod.javaSimpleName
val samMethodType = asmMethodType(samMethod).toASMType
val samMethodBType = asmMethodType(samMethod)
val samMethodType = samMethodBType.toASMType

def boxInstantiated(instantiatedType: BType, samType: BType): BType =
if(!samType.isPrimitive && instantiatedType.isPrimitive)
boxedClassOfPrimitive(instantiatedType.asPrimitiveBType)
else instantiatedType
// TODO specialization
val instantiatedMethodBType = new MethodBType(
lambdaParamTypes.map(p => toTypeKind(p)),
boxInstantiated(toTypeKind(lambdaTarget.info.resultType), samMethodBType.returnType)
)

val instantiatedMethodType = instantiatedMethodBType.toASMType
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@lrytz lrytz Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

👍

Copy link
Contributor Author

@ddtthh ddtthh Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// scala/bug#10334: make sure that a lambda object for `T => U` has a method `apply(T)U`, not only the `(Object)Object`
// version. Using the lambda a structural type `{def apply(t: T): U}` causes a reflective lookup for this method.
val needsGenericBridge = samMethodType != instantiatedMethodType
Expand Down
Loading